diff mbox series

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

Message ID 20230714154648.327466-7-peter.maydell@linaro.org
State New
Headers show
Series target/arm/ptw: Cleanups and a few bugfixes | expand

Commit Message

Peter Maydell July 14, 2023, 3:46 p.m. UTC 
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(-)

Comments

Richard Henderson July 23, 2023, 3:24 p.m. UTC | #1 
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~
Peter Maydell July 24, 2023, 1:42 p.m. UTC | #2 
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
Peter Maydell July 24, 2023, 2:38 p.m. UTC | #3 
On Mon, 24 Jul 2023 at 14:42, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> 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 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
Richard Henderson July 25, 2023, 6:36 p.m. UTC | #4 
On 7/24/23 07:38, Peter Maydell wrote:
>> 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.

Yes, that's how I read it.


r~
diff mbox series

Patch

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 register has no effect if EL2 is not enabled in the
          * current Security state".  This is ARMv8.4-SecEL2 speak for
@@ -5799,7 +5800,7 @@  uint64_t arm_hcr_el2_eff(CPUARMState *env)
     if (arm_feature(env, ARM_FEATURE_M)) {
         return 0;
     }
-    return arm_hcr_el2_eff_secstate(env, arm_is_secure_below_el3(env));
+    return arm_hcr_el2_eff_secstate(env, arm_security_space_below_el3(env));
 }
 
 /*
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 63dd8e3cbe1..9e45160e1ba 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -209,9 +209,9 @@  static bool regime_translation_disabled(CPUARMState *env, ARMMMUIdx mmu_idx,
                                         ARMSecuritySpace space)
 {
     uint64_t hcr_el2;
-    bool is_secure = arm_space_is_secure(space);
 
     if (arm_feature(env, ARM_FEATURE_M)) {
+        bool is_secure = arm_space_is_secure(space);
         switch (env->v7m.mpu_ctrl[is_secure] &
                 (R_V7M_MPU_CTRL_ENABLE_MASK | R_V7M_MPU_CTRL_HFNMIENA_MASK)) {
         case R_V7M_MPU_CTRL_ENABLE_MASK:
@@ -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);
 
     switch (mmu_idx) {
     case ARMMMUIdx_Stage2:
@@ -530,7 +530,6 @@  static bool fault_s1ns(ARMSecuritySpace space, ARMMMUIdx s2_mmu_idx)
 static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw,
                              hwaddr addr, ARMMMUFaultInfo *fi)
 {
-    bool is_secure = ptw->in_secure;
     ARMMMUIdx mmu_idx = ptw->in_mmu_idx;
     ARMMMUIdx s2_mmu_idx = ptw->in_ptw_idx;
     uint8_t pte_attrs;
@@ -587,7 +586,7 @@  static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw,
     }
 
     if (regime_is_stage2(s2_mmu_idx)) {
-        uint64_t hcr = arm_hcr_el2_eff_secstate(env, is_secure);
+        uint64_t hcr = arm_hcr_el2_eff_secstate(env, ptw->in_space);
 
         if ((hcr & HCR_PTW) && S2_attrs_are_device(hcr, pte_attrs)) {
             /*
@@ -3066,7 +3065,6 @@  static bool get_phys_addr_disabled(CPUARMState *env,
                                    ARMMMUFaultInfo *fi)
 {
     ARMMMUIdx mmu_idx = ptw->in_mmu_idx;
-    bool is_secure = arm_space_is_secure(ptw->in_space);
     uint8_t memattr = 0x00;    /* Device nGnRnE */
     uint8_t shareability = 0;  /* non-shareable */
     int r_el;
@@ -3112,7 +3110,7 @@  static bool get_phys_addr_disabled(CPUARMState *env,
 
         /* Fill in cacheattr a-la AArch64.TranslateAddressS1Off. */
         if (r_el == 1) {
-            uint64_t hcr = arm_hcr_el2_eff_secstate(env, is_secure);
+            uint64_t hcr = arm_hcr_el2_eff_secstate(env, ptw->in_space);
             if (hcr & HCR_DC) {
                 if (hcr & HCR_DCT) {
                     memattr = 0xf0;  /* Tagged, Normal, WB, RWA */
@@ -3149,7 +3147,6 @@  static bool get_phys_addr_twostage(CPUARMState *env, S1Translate *ptw,
 {
     hwaddr ipa;
     int s1_prot, s1_lgpgsz;
-    bool is_secure = ptw->in_secure;
     ARMSecuritySpace in_space = ptw->in_space;
     bool ret, ipa_secure;
     ARMCacheAttrs cacheattrs1;
@@ -3212,7 +3209,7 @@  static bool get_phys_addr_twostage(CPUARMState *env, S1Translate *ptw,
     }
 
     /* Combine the S1 and S2 cache attributes. */
-    hcr = arm_hcr_el2_eff_secstate(env, is_secure);
+    hcr = arm_hcr_el2_eff_secstate(env, in_space);
     if (hcr & HCR_DC) {
         /*
          * HCR.DC forces the first stage attributes to