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

[Peter Maydell]

On Sun, 23 Jul 2023 at 16:24, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> 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.

Does it? HCR_EL2 says "behaves as 0 if EL2 is not enabled in the
current Security state". If the current Security state is Root then
EL2 isn't enabled (because there's no such thing as EL2 Root), so the
function should return 0, shouldn't it?

I did think about pushing the ARMSecuritySpace down further
so arm_is_el2_enabled_secstate() also called it.

thanks
-- PMM