Currently the code in target/arm/helper.c mostly checks the PAN bits
in env->pstate or env->uncached_cpsr directly when it wants to know
if PAN is enabled, because in most callsites we know whether we are
in AArch64 or AArch32. We do have an arm_pan_enabled() function, but
we only use it in a few places where the code might run in either an
AArch32 or AArch64 context.

For FEAT_NV, when HCR_EL2.{NV,NV1} is {1,1} PAN is always disabled
even when the PSTATE.PAN bit is set, the "is PAN enabled" test
becomes more complicated. Make all places that check for PAN use
arm_pan_enabled(), so we have a place to put the FEAT_NV test.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 3270fb11049..4b0e46cfaae 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -263,6 +263,15 @@ void init_cpreg_list(ARMCPU *cpu)
     g_list_free(keys);
 }
 
+static bool arm_pan_enabled(CPUARMState *env)
+{
+    if (is_a64(env)) {
+        return env->pstate & PSTATE_PAN;
+    } else {
+        return env->uncached_cpsr & CPSR_PAN;
+    }
+}
+
 /*
  * Some registers are not accessible from AArch32 EL3 if SCR.NS == 0.
  */
@@ -3598,7 +3607,7 @@ static void ats_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
             g_assert(ss != ARMSS_Secure);  /* ARMv8.4-SecEL2 is 64-bit only */
             /* fall through */
         case 1:
-            if (ri->crm == 9 && (env->uncached_cpsr & CPSR_PAN)) {
+            if (ri->crm == 9 && arm_pan_enabled(env)) {
                 mmu_idx = ARMMMUIdx_Stage1_E1_PAN;
             } else {
                 mmu_idx = ARMMMUIdx_Stage1_E1;
@@ -3714,7 +3723,7 @@ static void ats_write64(CPUARMState *env, const ARMCPRegInfo *ri,
     case 0:
         switch (ri->opc1) {
         case 0: /* AT S1E1R, AT S1E1W, AT S1E1RP, AT S1E1WP */
-            if (ri->crm == 9 && (env->pstate & PSTATE_PAN)) {
+            if (ri->crm == 9 && arm_pan_enabled(env)) {
                 mmu_idx = regime_e20 ?
                           ARMMMUIdx_E20_2_PAN : ARMMMUIdx_Stage1_E1_PAN;
             } else {
@@ -12222,15 +12231,6 @@ ARMMMUIdx arm_v7m_mmu_idx_for_secstate(CPUARMState *env, bool secstate)
 }
 #endif
 
-static bool arm_pan_enabled(CPUARMState *env)
-{
-    if (is_a64(env)) {
-        return env->pstate & PSTATE_PAN;
-    } else {
-        return env->uncached_cpsr & CPSR_PAN;
-    }
-}
-
 ARMMMUIdx arm_mmu_idx_el(CPUARMState *env, int el)
 {
     ARMMMUIdx idx;
-- 
2.34.1

On 12/18/23 22:32, Peter Maydell wrote:
> Currently the code in target/arm/helper.c mostly checks the PAN bits
> in env->pstate or env->uncached_cpsr directly when it wants to know
> if PAN is enabled, because in most callsites we know whether we are
> in AArch64 or AArch32. We do have an arm_pan_enabled() function, but
> we only use it in a few places where the code might run in either an
> AArch32 or AArch64 context.
> 
> For FEAT_NV, when HCR_EL2.{NV,NV1} is {1,1} PAN is always disabled
> even when the PSTATE.PAN bit is set, the "is PAN enabled" test
> becomes more complicated. Make all places that check for PAN use
> arm_pan_enabled(), so we have a place to put the FEAT_NV test.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/helper.c | 22 +++++++++++-----------
>   1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 3270fb11049..4b0e46cfaae 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -263,6 +263,15 @@ void init_cpreg_list(ARMCPU *cpu)
>       g_list_free(keys);
>   }
>   
> +static bool arm_pan_enabled(CPUARMState *env)
> +{
> +    if (is_a64(env)) {
> +        return env->pstate & PSTATE_PAN;
> +    } else {
> +        return env->uncached_cpsr & CPSR_PAN;
> +    }
> +}

Worth splitting out helpers aa{32,64}_pan_enabled to avoid the is_a64 check when context 
dictates?

Either way,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

On Wed, 27 Dec 2023 at 22:50, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 12/18/23 22:32, Peter Maydell wrote:
> > Currently the code in target/arm/helper.c mostly checks the PAN bits
> > in env->pstate or env->uncached_cpsr directly when it wants to know
> > if PAN is enabled, because in most callsites we know whether we are
> > in AArch64 or AArch32. We do have an arm_pan_enabled() function, but
> > we only use it in a few places where the code might run in either an
> > AArch32 or AArch64 context.
> >
> > For FEAT_NV, when HCR_EL2.{NV,NV1} is {1,1} PAN is always disabled
> > even when the PSTATE.PAN bit is set, the "is PAN enabled" test
> > becomes more complicated. Make all places that check for PAN use
> > arm_pan_enabled(), so we have a place to put the FEAT_NV test.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >   target/arm/helper.c | 22 +++++++++++-----------
> >   1 file changed, 11 insertions(+), 11 deletions(-)
> >
> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index 3270fb11049..4b0e46cfaae 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -263,6 +263,15 @@ void init_cpreg_list(ARMCPU *cpu)
> >       g_list_free(keys);
> >   }
> >
> > +static bool arm_pan_enabled(CPUARMState *env)
> > +{
> > +    if (is_a64(env)) {
> > +        return env->pstate & PSTATE_PAN;
> > +    } else {
> > +        return env->uncached_cpsr & CPSR_PAN;
> > +    }
> > +}
>
> Worth splitting out helpers aa{32,64}_pan_enabled to avoid the is_a64 check when context
> dictates?

Doesn't really seem worthwhile to me -- we only know this
for a couple of subcases of AT instructions, which aren't
all that common in guest execution, and the cost of
is_a64() is going to be completely swamped by the cost
of actually doing the address translation...

thanks
-- PMM