Re: [PATCH 07/22] target/arm: Introduce ARMSecuritySpace

[Peter Maydell <peter.maydell@linaro.org>]

On Tue, 24 Jan 2023 at 00:01, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Introduce both the enumeration and functions to retrieve
> the current state, and state outside of EL3.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu.h    | 87 +++++++++++++++++++++++++++++++++++----------
>  target/arm/helper.c | 46 ++++++++++++++++++++++++
>  2 files changed, 115 insertions(+), 18 deletions(-)

> +/* Return true if @space is secure, in the pre-v9 sense. */
> +static inline bool arm_space_is_secure(ARMSecuritySpace space)
> +{
> +    return space == ARMSS_Secure || space == ARMSS_Root;
> +}


> +/**
> + * arm_is_secure:
> + * @env: cpu context
> + *
> + * Return true if the processor is in secure state.
> + */
>  static inline bool arm_is_secure(CPUARMState *env)
>  {
> -    if (arm_is_el3_or_mon(env)) {
> -        return true;
> -    }
> -    return arm_is_secure_below_el3(env);
> +    ARMSecuritySpace ss = arm_security_space(env);
> +    return ss == ARMSS_Secure || ss == ARMSS_Root;

maybe
  return arm_space_is_secure(arm_security_space(env));

?

>  }
>
>  /*
> @@ -2457,11 +2498,21 @@ static inline bool arm_is_el2_enabled(CPUARMState *env)
>  }
>
>  #else
> +static inline ARMSecuritySpace arm_security_space_below_el3(CPUARMState *env)
> +{
> +    return ARMSS_NonSecure;
> +}
> +
>  static inline bool arm_is_secure_below_el3(CPUARMState *env)
>  {
>      return false;
>  }
>
> +static inline ARMSecuritySpace arm_security_space(CPUARMState *env)
> +{
> +    return ARMSS_NonSecure;
> +}
> +
>  static inline bool arm_is_secure(CPUARMState *env)
>  {
>      return false;
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 7bd15e9d17..bf78a1d74e 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -12280,3 +12280,49 @@ void aarch64_sve_change_el(CPUARMState *env, int old_el,
>      }
>  }
>  #endif
> +
> +#ifndef CONFIG_USER_ONLY
> +ARMSecuritySpace arm_security_space(CPUARMState *env)
> +{
> +    if (!arm_feature(env, ARM_FEATURE_EL3)) {

The old code had a comment
-        /* If EL3 is not supported then the secure state is implementation
-         * defined, in which case QEMU defaults to non-secure.
which should probably go here I guess.

(Though it's not quite so true for R-profile, where you
don't get to make the impdef choice: v8R-32 is always
NS, and v8R-64 always S. If we ever have to implement
the latter this will probably cause some mild pain.)


Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM