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

[Peter Maydell]

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.

> +    }
> +#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.

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.)

thanks
-- PMM