Re: [Qemu-devel] [PATCH v5 02/33] target-arm: add arm_is_secure() function

[Greg Bellows]

On 6 October 2014 09:56, Peter Maydell <address@hidden> wrote:

On 30 September 2014 22:49, Greg Bellows <address@hidden> wrote:
> From: Fabian Aggeler <address@hidden>
>
> arm_is_secure() function allows to determine CPU security state
> if the CPU implements Security Extensions/EL3.
> arm_is_secure_below_el3() returns true if CPU is in secure state
> below EL3.
>
> Signed-off-by: Sergey Fedorov <address@hidden>
> Signed-off-by: Fabian Aggeler <address@hidden>
> Signed-off-by: Greg Bellows <address@hidden>
> ---
>  target-arm/cpu.h | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 81fffd2..10afef0 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -753,6 +753,44 @@ static inline int arm_feature(CPUARMState *env, int feature)
>      return (env->features & (1ULL << feature)) != 0;
>  }
>
> +
> +/* Return true if exception level below EL3 is in secure state */
> +static inline bool arm_is_secure_below_el3(CPUARMState *env)
> +{
> +#if !defined(CONFIG_USER_ONLY)
> +    if (arm_feature(env, ARM_FEATURE_EL3)) {
> +        return !(env->cp15.scr_el3 & SCR_NS);
> +    } else if (arm_feature(env, ARM_FEATURE_EL2)) {
> +        return false;
> +    } else {
> +        /* IMPDEF: QEMU defaults to non-secure */
> +        return false;

I would be happy to fold both these identical 'return false'
cases together and have a comment that it's only IMPDEF
if EL2 isn't implemented.

Yes, this makes sense.  Fixed in v6.

 

> +    }
> +#else
> +    return false;
> +#endif
> +}
> +
> +/* Return true if the processor is in secure state */
> +static inline bool arm_is_secure(CPUARMState *env)
> +{
> +#if !defined(CONFIG_USER_ONLY)
> +    if (arm_feature(env, ARM_FEATURE_EL3)) {
> +        if (env->aarch64 && extract32(env->pstate, 2, 2) == 3) {
> +            /* CPU currently in Aarch64 state and EL3 */

Nit: "AArch64" with two capital 'A's (here and elsewhere).

> +            return true;
> +        } else if (!env->aarch64 &&
> +                (env->uncached_cpsr & CPSR_M) == ARM_CPU_MODE_MON) {
> +            /* CPU currently in Aarch32 state and monitor mode */
> +            return true;
> +        }
> +    }
> +    return arm_is_secure_below_el3(env);
> +#else
> +    return false;
> +#endif
> +}

I checked your git tree and we don't actually use
arm_is_secure_below_el3() anywhere except in
arm_is_secure(), do we? That suggests to me we should
just fold the two functions together.

This is true and I contemplated this myself.  The reason I did not fold them together is because they match what is defined in the ARM v8 ARM and the below_el3 pseudo-function is actually used elsewhere in the spec separate from isSecure().  Honestly, I can go whichever way, so given the above what is your preference?

 

Can these functions live in internals.h rather than cpu.h?
(The difference is that internals.h is restricted to only
target-arm/ code whereas cpu.h is auto-included for a much
wider set of files.)

I can move the code, but how does it differ from the likes of arm_feature() or arm_el_is_aa64()?  They seem to serve the same utility purpose.

 

thanks
-- PMM