On Mon, 24 Jul 2023 at 14:42, Peter Maydell <peter.mayd...@linaro.org> wrote:
>
> On Sun, 23 Jul 2023 at 16:24, Richard Henderson
> <richard.hender...@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.mayd...@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 guess there's an argument that what the spec really means is
"the security state described by the current effective value
of SCR_EL3.{NSE,NS}" (to steal language from the docs of the
AT operations), though. (If we wanted to implement that we could
assert(space != ARMSS_Root) and make sure we didn't call it
in that case.) I'll think about it...
-- PMM
