Re: [PATCH 06/14] target/arm/ptw: Pass an ARMSecuritySpace to arm_hcr_el2_eff_secstate()

[Richard Henderson <richard.henderson@linaro.org>]

On 7/14/23 16:46, Peter Maydell wrote:
> arm_hcr_el2_eff_secstate() takes a bool secure, which it uses to
> determine whether EL2 is enabled in the current security state.
> With the advent of FEAT_RME this is no longer sufficient, because
> EL2 can be enabled for Secure state but not for Root, and both
> of those will pass 'secure == true' in the callsites in ptw.c.
> 
> As it happens in all of our callsites in ptw.c we either avoid making
> the call or else avoid using the returned value if we're doing a
> translation for Root, so this is not a behaviour change even if the
> experimental FEAT_RME is enabled.  But it is less confusing in the
> ptw.c code if we avoid the use of a bool secure that duplicates some
> of the information in the ArmSecuritySpace argument.
> 
> Make arm_hcr_el2_eff_secstate() take an ARMSecuritySpace argument
> instead.
> 
> Note that since arm_hcr_el2_eff() uses the return value from
> arm_security_space_below_el3() for the 'space' argument, its
> behaviour does not change even when at EL3 (Root security state) and
> it continues to tell you what EL2 would be if you were in it.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/cpu.h    |  2 +-
>   target/arm/helper.c |  7 ++++---
>   target/arm/ptw.c    | 13 +++++--------
>   3 files changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 4d6c0f95d59..3743a9e2f8a 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -2555,7 +2555,7 @@ static inline bool arm_is_el2_enabled(CPUARMState *env)
>    * "for all purposes other than a direct read or write access of HCR_EL2."
>    * Not included here is HCR_RW.
>    */
> -uint64_t arm_hcr_el2_eff_secstate(CPUARMState *env, bool secure);
> +uint64_t arm_hcr_el2_eff_secstate(CPUARMState *env, ARMSecuritySpace space);
>   uint64_t arm_hcr_el2_eff(CPUARMState *env);
>   uint64_t arm_hcrx_el2_eff(CPUARMState *env);
>   
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index d08c058e424..1e45fdb47c9 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -5731,11 +5731,12 @@ static void hcr_writelow(CPUARMState *env, const ARMCPRegInfo *ri,
>    * Bits that are not included here:
>    * RW       (read from SCR_EL3.RW as needed)
>    */
> -uint64_t arm_hcr_el2_eff_secstate(CPUARMState *env, bool secure)
> +uint64_t arm_hcr_el2_eff_secstate(CPUARMState *env, ARMSecuritySpace space)
>   {
>       uint64_t ret = env->cp15.hcr_el2;
>   
> -    if (!arm_is_el2_enabled_secstate(env, secure)) {
> +    if (space == ARMSS_Root ||
> +        !arm_is_el2_enabled_secstate(env, arm_space_is_secure(space))) {
>           /*

This is confusing, as without any larger context it certainly looks wrong.

> @@ -230,7 +230,7 @@ static bool regime_translation_disabled(CPUARMState *env, ARMMMUIdx mmu_idx,
>          }
>      }
>  
> -    hcr_el2 = arm_hcr_el2_eff_secstate(env, is_secure);
> +    hcr_el2 = arm_hcr_el2_eff_secstate(env, space);

Here, it's not clear, and could produce an "incorrect" result, though of course the value 
is not actually used unless mmu_idx requires it.

It might be better to sink the computation down into the two cases that require it.  With 
that, a local definition like

static inline uint64_t arm_hcr_el2_ptwspace(...)
{
     assert(space != ARMSS_Root);
     return arm_hcr_el2_eff_secstate(env, arm_space_is_secure(space));
}

could be the thing.


r~