[PATCH 00/14] target/arm/ptw: Cleanups and a few bugfixes

[PATCH 00/14] target/arm/ptw: Cleanups and a few bugfixes

[Peter Maydell]

Based-on: 20230710152130.3928330-1-peter.maydell@linaro.org
("target/arm: Fix ptw bugs introduced by FEAT_RME changes")

While I was fixing the ptw bug in the series above, I noticed
that we had a somewhat confusing mix of ptw->in_space and
ptw->in_secure, where in theory the two are supposed to be
in sync and you can figure out the in_secure state from the
in_space. This patch series' principal aim is to clean that
up by removing the in_secure and out_secure fields in the
S1Translate struct.

The first three patches are fixes for (minor) bugs I noticed
while I was trying to do this refactoring because they're
in or around places that were using in_secure.
The next three are basically plumbing: passing ARMSecurityState
arguments instead of boolean is_secure arguments.
The next four patches then can get rid of uses of the
in_secure and out_secure fields and drop them entirely.
Finally, the last four patches are minor bug fixes for
various corner cases that I noticed while I was testing this.

I don't expect to land this series until we reopen for
8.2 development, but I might as well put it out on the
list for review, since I've written it.

thanks
-- PMM

Peter Maydell (14):
  target/arm/ptw: Don't set fi->s1ptw for UnsuppAtomicUpdate fault
  target/arm/ptw: Don't report GPC faults on stage 1 ptw as stage2
    faults
  target/arm/ptw: Set s1ns bit in fault info more consistently
  target/arm/ptw: Pass ptw into get_phys_addr_pmsa*() and
    get_phys_addr_disabled()
  target/arm/ptw: Pass ARMSecurityState to regime_translation_disabled()
  target/arm/ptw: Pass an ARMSecuritySpace to arm_hcr_el2_eff_secstate()
  target/arm/ptw: Only fold in NSTable bit effects in Secure state
  target/arm/ptw: Remove last uses of ptw->in_secure
  target/arm/ptw: Remove S1Translate::in_secure
  target/arm/ptw: Drop S1Translate::out_secure
  target/arm/ptw: Set attributes correctly for MMU disabled data
    accesses
  target/arm/ptw: Check for block descriptors at invalid levels
  target/arm/ptw: Report stage 2 fault level for stage 2 faults on stage
    1 ptw
  target/arm: Adjust PAR_EL1.SH for Device and Normal-NC memory types

 target/arm/cpu.h    |   2 +-
 target/arm/helper.c |  22 ++++-
 target/arm/ptw.c    | 190 +++++++++++++++++++++++++++-----------------
 3 files changed, 135 insertions(+), 79 deletions(-)

-- 
2.34.1

[PATCH 01/14] target/arm/ptw: Don't set fi->s1ptw for UnsuppAtomicUpdate fault

[Peter Maydell]

For an Unsupported Atomic Update fault where the stage 1 translation
table descriptor update can't be done because it's to an unsupported
memory type, this is a stage 1 abort (per the Arm ARM R_VSXXT).  This
means we should not set fi->s1ptw, because this will cause the code
in the get_phys_addr_lpae() error-exit path to mark it as stage 2.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/ptw.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 8f94100c61f..bafeb876ad7 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -701,7 +701,6 @@ static uint64_t arm_casq_ptw(CPUARMState *env, uint64_t old_val,
 
     if (unlikely(!host)) {
         fi->type = ARMFault_UnsuppAtomicUpdate;
-        fi->s1ptw = true;
         return 0;
     }
 
-- 
2.34.1

Re: [PATCH 01/14] target/arm/ptw: Don't set fi->s1ptw for UnsuppAtomicUpdate fault

[Richard Henderson]

On 7/14/23 16:46, Peter Maydell wrote:
> For an Unsupported Atomic Update fault where the stage 1 translation
> table descriptor update can't be done because it's to an unsupported
> memory type, this is a stage 1 abort (per the Arm ARM R_VSXXT).  This
> means we should not set fi->s1ptw, because this will cause the code
> in the get_phys_addr_lpae() error-exit path to mark it as stage 2.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/ptw.c | 1 -
>   1 file changed, 1 deletion(-)

Confusingly named, but correctly documented.

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


r~

[PATCH 02/14] target/arm/ptw: Don't report GPC faults on stage 1 ptw as stage2 faults

[Peter Maydell]

In S1_ptw_translate() we set up the ARMMMUFaultInfo if the attempt to
translate the page descriptor address into a physical address fails.
This used to only be possible if we are doing a stage 2 ptw for that
descriptor address, and so the code always sets fi->stage2 and
fi->s1ptw to true.  However, with FEAT_RME it is also possible for
the lookup of the page descriptor address to fail because of a
Granule Protection Check fault.  These should not be reported as
stage 2, otherwise arm_deliver_fault() will incorrectly set
HPFAR_EL2.  Similarly the s1ptw bit should only be set for stage 2
faults on stage 1 translation table walks, i.e.  not for GPC faults.

Add a comment to the the other place where we might detect a
stage2-fault-on-stage-1-ptw, in arm_casq_ptw(), noting why we know in
that case that it must really be a stage 2 fault and not a GPC fault.

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

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index bafeb876ad7..eb57ebd897b 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -600,8 +600,8 @@ static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw,
         fi->type = ARMFault_GPCFOnWalk;
     }
     fi->s2addr = addr;
-    fi->stage2 = true;
-    fi->s1ptw = true;
+    fi->stage2 = regime_is_stage2(s2_mmu_idx);
+    fi->s1ptw = fi->stage2;
     fi->s1ns = !is_secure;
     return false;
 }
@@ -719,6 +719,12 @@ static uint64_t arm_casq_ptw(CPUARMState *env, uint64_t old_val,
         env->tlb_fi = NULL;
 
         if (unlikely(flags & TLB_INVALID_MASK)) {
+            /*
+             * We know this must be a stage 2 fault because the granule
+             * protection table does not separately track read and write
+             * permission, so all GPC faults are caught in S1_ptw_translate():
+             * we only get here for "readable but not writeable".
+             */
             assert(fi->type != ARMFault_None);
             fi->s2addr = ptw->out_virt;
             fi->stage2 = true;
-- 
2.34.1

Re: [PATCH 02/14] target/arm/ptw: Don't report GPC faults on stage 1 ptw as stage2 faults

[Richard Henderson]

On 7/14/23 16:46, Peter Maydell wrote:
> In S1_ptw_translate() we set up the ARMMMUFaultInfo if the attempt to
> translate the page descriptor address into a physical address fails.
> This used to only be possible if we are doing a stage 2 ptw for that
> descriptor address, and so the code always sets fi->stage2 and
> fi->s1ptw to true.  However, with FEAT_RME it is also possible for
> the lookup of the page descriptor address to fail because of a
> Granule Protection Check fault.  These should not be reported as
> stage 2, otherwise arm_deliver_fault() will incorrectly set
> HPFAR_EL2.  Similarly the s1ptw bit should only be set for stage 2
> faults on stage 1 translation table walks, i.e.  not for GPC faults.
> 
> Add a comment to the the other place where we might detect a
> stage2-fault-on-stage-1-ptw, in arm_casq_ptw(), noting why we know in
> that case that it must really be a stage 2 fault and not a GPC fault.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/ptw.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)

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

r~

[PATCH 03/14] target/arm/ptw: Set s1ns bit in fault info more consistently

[Peter Maydell]

The s1ns bit in ARMMMUFaultInfo is documented as "true if
we faulted on a non-secure IPA while in secure state". Both the
places which look at this bit only do so after having confirmed
that this is a stage 2 fault and we're dealing with Secure EL2,
which leaves the ptw.c code free to set the bit to any random
value in the other cases.

Instead of taking advantage of that freedom, consistently
make the bit be set to false for the "not a stage 2 fault
for Secure EL2" cases. This removes some cases where we
were using an 'is_secure' boolean and leaving the reader
guessing about whether that was the right thing for Realm
and Root cases.

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

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index eb57ebd897b..67078ae3509 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -514,6 +514,17 @@ static ARMSecuritySpace S2_security_space(ARMSecuritySpace s1_space,
     }
 }
 
+static bool fault_s1ns(ARMSecuritySpace space, ARMMMUIdx s2_mmu_idx)
+{
+    /*
+     * For stage 2 faults in Secure EL22, S1NS indicates
+     * whether the faulting IPA is in the Secure or NonSecure
+     * IPA space. For all other kinds of fault, it is false.
+     */
+    return space == ARMSS_Secure && regime_is_stage2(s2_mmu_idx)
+        && s2_mmu_idx == ARMMMUIdx_Stage2_S;
+}
+
 /* Translate a S1 pagetable walk through S2 if needed.  */
 static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw,
                              hwaddr addr, ARMMMUFaultInfo *fi)
@@ -586,7 +597,7 @@ static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw,
             fi->s2addr = addr;
             fi->stage2 = true;
             fi->s1ptw = true;
-            fi->s1ns = !is_secure;
+            fi->s1ns = fault_s1ns(ptw->in_space, s2_mmu_idx);
             return false;
         }
     }
@@ -602,7 +613,7 @@ static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw,
     fi->s2addr = addr;
     fi->stage2 = regime_is_stage2(s2_mmu_idx);
     fi->s1ptw = fi->stage2;
-    fi->s1ns = !is_secure;
+    fi->s1ns = fault_s1ns(ptw->in_space, s2_mmu_idx);
     return false;
 }
 
@@ -729,7 +740,7 @@ static uint64_t arm_casq_ptw(CPUARMState *env, uint64_t old_val,
             fi->s2addr = ptw->out_virt;
             fi->stage2 = true;
             fi->s1ptw = true;
-            fi->s1ns = !ptw->in_secure;
+            fi->s1ns = fault_s1ns(ptw->in_space, ptw->in_ptw_idx);
             return 0;
         }
 
@@ -2030,7 +2041,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
     fi->level = level;
     /* Tag the error as S2 for failed S1 PTW at S2 or ordinary S2.  */
     fi->stage2 = fi->s1ptw || regime_is_stage2(mmu_idx);
-    fi->s1ns = mmu_idx == ARMMMUIdx_Stage2;
+    fi->s1ns = fault_s1ns(ptw->in_space, mmu_idx);
     return true;
 }
 
-- 
2.34.1

Re: [PATCH 03/14] target/arm/ptw: Set s1ns bit in fault info more consistently

[Richard Henderson]

On 7/14/23 16:46, Peter Maydell wrote:
> The s1ns bit in ARMMMUFaultInfo is documented as "true if
> we faulted on a non-secure IPA while in secure state". Both the
> places which look at this bit only do so after having confirmed
> that this is a stage 2 fault and we're dealing with Secure EL2,
> which leaves the ptw.c code free to set the bit to any random
> value in the other cases.
> 
> Instead of taking advantage of that freedom, consistently
> make the bit be set to false for the "not a stage 2 fault
> for Secure EL2" cases. This removes some cases where we
> were using an 'is_secure' boolean and leaving the reader
> guessing about whether that was the right thing for Realm
> and Root cases.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/ptw.c | 19 +++++++++++++++----
>   1 file changed, 15 insertions(+), 4 deletions(-)

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

r~

[PATCH 04/14] target/arm/ptw: Pass ptw into get_phys_addr_pmsa*() and get_phys_addr_disabled()

[Peter Maydell]

In commit 6d2654ffacea813916176 we created the S1Translate struct and
used it to plumb through various arguments that we were previously
passing one-at-a-time to get_phys_addr_v5(), get_phys_addr_v6(), and
get_phys_addr_lpae().  Extend that pattern to get_phys_addr_pmsav5(),
get_phys_addr_pmsav7(), get_phys_addr_pmsav8() and
get_phys_addr_disabled(), so that all the get_phys_addr_* functions
we call from get_phys_addr_nogpc() take the S1Translate struct rather
than the mmu_idx and is_secure bool.

(This refactoring is a prelude to having the called functions look
at ptw->is_space rather than using an is_secure boolean.)

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

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 67078ae3509..a873fbe0239 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -2045,15 +2045,19 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
     return true;
 }
 
-static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
-                                 MMUAccessType access_type, ARMMMUIdx mmu_idx,
-                                 bool is_secure, GetPhysAddrResult *result,
+static bool get_phys_addr_pmsav5(CPUARMState *env,
+                                 S1Translate *ptw,
+                                 uint32_t address,
+                                 MMUAccessType access_type,
+                                 GetPhysAddrResult *result,
                                  ARMMMUFaultInfo *fi)
 {
     int n;
     uint32_t mask;
     uint32_t base;
+    ARMMMUIdx mmu_idx = ptw->in_mmu_idx;
     bool is_user = regime_is_user(env, mmu_idx);
+    bool is_secure = arm_space_is_secure(ptw->in_space);
 
     if (regime_translation_disabled(env, mmu_idx, is_secure)) {
         /* MPU disabled.  */
@@ -2210,14 +2214,18 @@ static bool pmsav7_use_background_region(ARMCPU *cpu, ARMMMUIdx mmu_idx,
     return regime_sctlr(env, mmu_idx) & SCTLR_BR;
 }
 
-static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
-                                 MMUAccessType access_type, ARMMMUIdx mmu_idx,
-                                 bool secure, GetPhysAddrResult *result,
+static bool get_phys_addr_pmsav7(CPUARMState *env,
+                                 S1Translate *ptw,
+                                 uint32_t address,
+                                 MMUAccessType access_type,
+                                 GetPhysAddrResult *result,
                                  ARMMMUFaultInfo *fi)
 {
     ARMCPU *cpu = env_archcpu(env);
     int n;
+    ARMMMUIdx mmu_idx = ptw->in_mmu_idx;
     bool is_user = regime_is_user(env, mmu_idx);
+    bool secure = arm_space_is_secure(ptw->in_space);
 
     result->f.phys_addr = address;
     result->f.lg_page_size = TARGET_PAGE_BITS;
@@ -2736,12 +2744,16 @@ void v8m_security_lookup(CPUARMState *env, uint32_t address,
     }
 }
 
-static bool get_phys_addr_pmsav8(CPUARMState *env, uint32_t address,
-                                 MMUAccessType access_type, ARMMMUIdx mmu_idx,
-                                 bool secure, GetPhysAddrResult *result,
+static bool get_phys_addr_pmsav8(CPUARMState *env,
+                                 S1Translate *ptw,
+                                 uint32_t address,
+                                 MMUAccessType access_type,
+                                 GetPhysAddrResult *result,
                                  ARMMMUFaultInfo *fi)
 {
     V8M_SAttributes sattrs = {};
+    ARMMMUIdx mmu_idx = ptw->in_mmu_idx;
+    bool secure = arm_space_is_secure(ptw->in_space);
     bool ret;
 
     if (arm_feature(env, ARM_FEATURE_M_SECURITY)) {
@@ -3045,12 +3057,15 @@ static ARMCacheAttrs combine_cacheattrs(uint64_t hcr,
  * MMU disabled.  S1 addresses within aa64 translation regimes are
  * still checked for bounds -- see AArch64.S1DisabledOutput().
  */
-static bool get_phys_addr_disabled(CPUARMState *env, target_ulong address,
+static bool get_phys_addr_disabled(CPUARMState *env,
+                                   S1Translate *ptw,
+                                   target_ulong address,
                                    MMUAccessType access_type,
-                                   ARMMMUIdx mmu_idx, bool is_secure,
                                    GetPhysAddrResult *result,
                                    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;
@@ -3252,8 +3267,8 @@ static bool get_phys_addr_nogpc(CPUARMState *env, S1Translate *ptw,
     case ARMMMUIdx_Phys_Root:
     case ARMMMUIdx_Phys_Realm:
         /* Checking Phys early avoids special casing later vs regime_el. */
-        return get_phys_addr_disabled(env, address, access_type, mmu_idx,
-                                      is_secure, result, fi);
+        return get_phys_addr_disabled(env, ptw, address, access_type,
+                                      result, fi);
 
     case ARMMMUIdx_Stage1_E0:
     case ARMMMUIdx_Stage1_E1:
@@ -3321,16 +3336,16 @@ static bool get_phys_addr_nogpc(CPUARMState *env, S1Translate *ptw,
 
         if (arm_feature(env, ARM_FEATURE_V8)) {
             /* PMSAv8 */
-            ret = get_phys_addr_pmsav8(env, address, access_type, mmu_idx,
-                                       is_secure, result, fi);
+            ret = get_phys_addr_pmsav8(env, ptw, address, access_type,
+                                       result, fi);
         } else if (arm_feature(env, ARM_FEATURE_V7)) {
             /* PMSAv7 */
-            ret = get_phys_addr_pmsav7(env, address, access_type, mmu_idx,
-                                       is_secure, result, fi);
+            ret = get_phys_addr_pmsav7(env, ptw, address, access_type,
+                                       result, fi);
         } else {
             /* Pre-v7 MPU */
-            ret = get_phys_addr_pmsav5(env, address, access_type, mmu_idx,
-                                       is_secure, result, fi);
+            ret = get_phys_addr_pmsav5(env, ptw, address, access_type,
+                                       result, fi);
         }
         qemu_log_mask(CPU_LOG_MMU, "PMSA MPU lookup for %s at 0x%08" PRIx32
                       " mmu_idx %u -> %s (prot %c%c%c)\n",
@@ -3348,8 +3363,8 @@ static bool get_phys_addr_nogpc(CPUARMState *env, S1Translate *ptw,
     /* Definitely a real MMU, not an MPU */
 
     if (regime_translation_disabled(env, mmu_idx, is_secure)) {
-        return get_phys_addr_disabled(env, address, access_type, mmu_idx,
-                                      is_secure, result, fi);
+        return get_phys_addr_disabled(env, ptw, address, access_type,
+                                      result, fi);
     }
 
     if (regime_using_lpae_format(env, mmu_idx)) {
-- 
2.34.1

Re: [PATCH 04/14] target/arm/ptw: Pass ptw into get_phys_addr_pmsa*() and get_phys_addr_disabled()

[Richard Henderson]

On 7/14/23 16:46, Peter Maydell wrote:
> In commit 6d2654ffacea813916176 we created the S1Translate struct and
> used it to plumb through various arguments that we were previously
> passing one-at-a-time to get_phys_addr_v5(), get_phys_addr_v6(), and
> get_phys_addr_lpae().  Extend that pattern to get_phys_addr_pmsav5(),
> get_phys_addr_pmsav7(), get_phys_addr_pmsav8() and
> get_phys_addr_disabled(), so that all the get_phys_addr_* functions
> we call from get_phys_addr_nogpc() take the S1Translate struct rather
> than the mmu_idx and is_secure bool.
> 
> (This refactoring is a prelude to having the called functions look
> at ptw->is_space rather than using an is_secure boolean.)
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/ptw.c | 57 ++++++++++++++++++++++++++++++------------------
>   1 file changed, 36 insertions(+), 21 deletions(-)

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

r~

[PATCH 05/14] target/arm/ptw: Pass ARMSecurityState to regime_translation_disabled()

[Peter Maydell]

Plumb the ARMSecurityState through to regime_translation_disabled()
rather than just a bool is_secure.

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

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index a873fbe0239..63dd8e3cbe1 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -206,9 +206,10 @@ static uint64_t regime_ttbr(CPUARMState *env, ARMMMUIdx mmu_idx, int ttbrn)
 
 /* Return true if the specified stage of address translation is disabled */
 static bool regime_translation_disabled(CPUARMState *env, ARMMMUIdx mmu_idx,
-                                        bool is_secure)
+                                        ARMSecuritySpace space)
 {
     uint64_t hcr_el2;
+    bool is_secure = arm_space_is_secure(space);
 
     if (arm_feature(env, ARM_FEATURE_M)) {
         switch (env->v7m.mpu_ctrl[is_secure] &
@@ -2057,9 +2058,8 @@ static bool get_phys_addr_pmsav5(CPUARMState *env,
     uint32_t base;
     ARMMMUIdx mmu_idx = ptw->in_mmu_idx;
     bool is_user = regime_is_user(env, mmu_idx);
-    bool is_secure = arm_space_is_secure(ptw->in_space);
 
-    if (regime_translation_disabled(env, mmu_idx, is_secure)) {
+    if (regime_translation_disabled(env, mmu_idx, ptw->in_space)) {
         /* MPU disabled.  */
         result->f.phys_addr = address;
         result->f.prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
@@ -2231,7 +2231,7 @@ static bool get_phys_addr_pmsav7(CPUARMState *env,
     result->f.lg_page_size = TARGET_PAGE_BITS;
     result->f.prot = 0;
 
-    if (regime_translation_disabled(env, mmu_idx, secure) ||
+    if (regime_translation_disabled(env, mmu_idx, ptw->in_space) ||
         m_is_ppb_region(env, address)) {
         /*
          * MPU disabled or M profile PPB access: use default memory map.
@@ -2475,7 +2475,8 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
      * are done in arm_v7m_load_vector(), which always does a direct
      * read using address_space_ldl(), rather than going via this function.
      */
-    if (regime_translation_disabled(env, mmu_idx, secure)) { /* MPU disabled */
+    if (regime_translation_disabled(env, mmu_idx, arm_secure_to_space(secure))) {
+        /* MPU disabled */
         hit = true;
     } else if (m_is_ppb_region(env, address)) {
         hit = true;
@@ -3303,7 +3304,7 @@ static bool get_phys_addr_nogpc(CPUARMState *env, S1Translate *ptw,
          */
         ptw->in_mmu_idx = mmu_idx = s1_mmu_idx;
         if (arm_feature(env, ARM_FEATURE_EL2) &&
-            !regime_translation_disabled(env, ARMMMUIdx_Stage2, is_secure)) {
+            !regime_translation_disabled(env, ARMMMUIdx_Stage2, ptw->in_space)) {
             return get_phys_addr_twostage(env, ptw, address, access_type,
                                           result, fi);
         }
@@ -3362,7 +3363,7 @@ static bool get_phys_addr_nogpc(CPUARMState *env, S1Translate *ptw,
 
     /* Definitely a real MMU, not an MPU */
 
-    if (regime_translation_disabled(env, mmu_idx, is_secure)) {
+    if (regime_translation_disabled(env, mmu_idx, ptw->in_space)) {
         return get_phys_addr_disabled(env, ptw, address, access_type,
                                       result, fi);
     }
-- 
2.34.1

Re: [PATCH 05/14] target/arm/ptw: Pass ARMSecurityState to regime_translation_disabled()

[Richard Henderson]

On 7/14/23 16:46, Peter Maydell wrote:
> Plumb the ARMSecurityState through to regime_translation_disabled()
> rather than just a bool is_secure.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/ptw.c | 15 ++++++++-------
>   1 file changed, 8 insertions(+), 7 deletions(-)

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

r~

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

[Peter Maydell]

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 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
-- 
2.34.1

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

[Richard Henderson]

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~

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

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

[Peter Maydell]

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

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

[Richard Henderson]

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~

[PATCH 07/14] target/arm/ptw: Only fold in NSTable bit effects in Secure state

[Peter Maydell]

When we do a translation in Secure state, the NSTable bits in table
descriptors may downgrade us to NonSecure; we update ptw->in_secure
and ptw->in_space accordingly.  We guard that check correctly with a
conditional that means it's only applied for Secure stage 1
translations.  However, later on in get_phys_addr_lpae() we fold the
effects of the NSTable bits into the final descriptor attributes
bits, and there we do it unconditionally regardless of the CPU state.
That means that in Realm state (where in_secure is false) we will set
bit 5 in attrs, and later use it to decide to output to non-secure
space.

We don't in fact need to do this folding in at all any more (since
commit 2f1ff4e7b9f30c): if an NSTable bit was set then we have
already set ptw->in_space to ARMSS_NonSecure, and in that situation
we don't look at attrs bit 5.  The only thing we still need to deal
with is the real NS bit in the final descriptor word, so we can just
drop the code that ORed in the NSTable bit.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/ptw.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 9e45160e1ba..c30d3fe69a0 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -1884,11 +1884,10 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
      * Extract attributes from the (modified) descriptor, and apply
      * table descriptors. Stage 2 table descriptors do not include
      * any attribute fields. HPD disables all the table attributes
-     * except NSTable.
+     * except NSTable (which we have already handled).
      */
     attrs = new_descriptor & (MAKE_64BIT_MASK(2, 10) | MAKE_64BIT_MASK(50, 14));
     if (!regime_is_stage2(mmu_idx)) {
-        attrs |= !ptw->in_secure << 5; /* NS */
         if (!param.hpd) {
             attrs |= extract64(tableattrs, 0, 2) << 53;     /* XN, PXN */
             /*
-- 
2.34.1

Re: [PATCH 07/14] target/arm/ptw: Only fold in NSTable bit effects in Secure state

[Richard Henderson]

On 7/14/23 16:46, Peter Maydell wrote:
> When we do a translation in Secure state, the NSTable bits in table
> descriptors may downgrade us to NonSecure; we update ptw->in_secure
> and ptw->in_space accordingly.  We guard that check correctly with a
> conditional that means it's only applied for Secure stage 1
> translations.  However, later on in get_phys_addr_lpae() we fold the
> effects of the NSTable bits into the final descriptor attributes
> bits, and there we do it unconditionally regardless of the CPU state.
> That means that in Realm state (where in_secure is false) we will set
> bit 5 in attrs, and later use it to decide to output to non-secure
> space.
> 
> We don't in fact need to do this folding in at all any more (since
> commit 2f1ff4e7b9f30c): if an NSTable bit was set then we have
> already set ptw->in_space to ARMSS_NonSecure, and in that situation
> we don't look at attrs bit 5.  The only thing we still need to deal
> with is the real NS bit in the final descriptor word, so we can just
> drop the code that ORed in the NSTable bit.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/ptw.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)

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

r~

[PATCH 08/14] target/arm/ptw: Remove last uses of ptw->in_secure

[Peter Maydell]

Replace the last uses of ptw->in_secure with appropriate
checks on ptw->in_space.

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

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index c30d3fe69a0..bc834675fb2 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -3247,7 +3247,6 @@ static bool get_phys_addr_nogpc(CPUARMState *env, S1Translate *ptw,
                                       ARMMMUFaultInfo *fi)
 {
     ARMMMUIdx mmu_idx = ptw->in_mmu_idx;
-    bool is_secure = ptw->in_secure;
     ARMMMUIdx s1_mmu_idx;
 
     /*
@@ -3255,8 +3254,8 @@ static bool get_phys_addr_nogpc(CPUARMState *env, S1Translate *ptw,
      * cannot upgrade a NonSecure translation regime's attributes
      * to Secure or Realm.
      */
-    result->f.attrs.secure = is_secure;
     result->f.attrs.space = ptw->in_space;
+    result->f.attrs.secure = arm_space_is_secure(ptw->in_space);
 
     switch (mmu_idx) {
     case ARMMMUIdx_Phys_S:
@@ -3270,8 +3269,12 @@ static bool get_phys_addr_nogpc(CPUARMState *env, S1Translate *ptw,
     case ARMMMUIdx_Stage1_E0:
     case ARMMMUIdx_Stage1_E1:
     case ARMMMUIdx_Stage1_E1_PAN:
-        /* First stage lookup uses second stage for ptw. */
-        ptw->in_ptw_idx = is_secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2;
+        /*
+         * First stage lookup uses second stage for ptw; only
+         * Secure has both S and NS IPA and starts with Stage2_S.
+         */
+        ptw->in_ptw_idx = (ptw->in_space == ARMSS_Secure) ?
+            ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2;
         break;
 
     case ARMMMUIdx_Stage2:
-- 
2.34.1

Re: [PATCH 08/14] target/arm/ptw: Remove last uses of ptw->in_secure

[Richard Henderson]

On 7/14/23 16:46, Peter Maydell wrote:
> Replace the last uses of ptw->in_secure with appropriate
> checks on ptw->in_space.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/ptw.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)

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

r~

[PATCH 09/14] target/arm/ptw: Remove S1Translate::in_secure

[Peter Maydell]

We no longer look at the in_secure field of the S1Translate struct
anyway, so we can remove it and all the code which sets it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/ptw.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index bc834675fb2..77b8382ceff 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -51,13 +51,6 @@ typedef struct S1Translate {
      *    value being Stage2 vs Stage2_S distinguishes those.
      */
     ARMSecuritySpace in_space;
-    /*
-     * in_secure: whether the translation regime is a Secure one.
-     * This is always equal to arm_space_is_secure(in_space).
-     * If a Secure ptw is "downgraded" to NonSecure by an NSTable bit,
-     * this field is updated accordingly.
-     */
-    bool in_secure;
     /*
      * in_debug: is this a QEMU debug access (gdbstub, etc)? Debug
      * accesses will not update the guest page table access flags
@@ -545,7 +538,6 @@ static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw,
         S1Translate s2ptw = {
             .in_mmu_idx = s2_mmu_idx,
             .in_ptw_idx = ptw_idx_for_stage_2(env, s2_mmu_idx),
-            .in_secure = arm_space_is_secure(s2_space),
             .in_space = s2_space,
             .in_debug = true,
         };
@@ -1782,7 +1774,6 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
         QEMU_BUILD_BUG_ON(ARMMMUIdx_Phys_S + 1 != ARMMMUIdx_Phys_NS);
         QEMU_BUILD_BUG_ON(ARMMMUIdx_Stage2_S + 1 != ARMMMUIdx_Stage2);
         ptw->in_ptw_idx += 1;
-        ptw->in_secure = false;
         ptw->in_space = ARMSS_NonSecure;
     }
 
@@ -3165,7 +3156,6 @@ static bool get_phys_addr_twostage(CPUARMState *env, S1Translate *ptw,
 
     ptw->in_s1_is_el0 = ptw->in_mmu_idx == ARMMMUIdx_Stage1_E0;
     ptw->in_mmu_idx = ipa_secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2;
-    ptw->in_secure = ipa_secure;
     ptw->in_space = ipa_space;
     ptw->in_ptw_idx = ptw_idx_for_stage_2(env, ptw->in_mmu_idx);
 
@@ -3401,7 +3391,6 @@ bool get_phys_addr_with_secure(CPUARMState *env, target_ulong address,
 {
     S1Translate ptw = {
         .in_mmu_idx = mmu_idx,
-        .in_secure = is_secure,
         .in_space = arm_secure_to_space(is_secure),
     };
     return get_phys_addr_gpc(env, &ptw, address, access_type, result, fi);
@@ -3473,7 +3462,6 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
     }
 
     ptw.in_space = ss;
-    ptw.in_secure = arm_space_is_secure(ss);
     return get_phys_addr_gpc(env, &ptw, address, access_type, result, fi);
 }
 
@@ -3487,7 +3475,6 @@ hwaddr arm_cpu_get_phys_page_attrs_debug(CPUState *cs, vaddr addr,
     S1Translate ptw = {
         .in_mmu_idx = mmu_idx,
         .in_space = ss,
-        .in_secure = arm_space_is_secure(ss),
         .in_debug = true,
     };
     GetPhysAddrResult res = {};
-- 
2.34.1

Re: [PATCH 09/14] target/arm/ptw: Remove S1Translate::in_secure

[Richard Henderson]

On 7/14/23 16:46, Peter Maydell wrote:
> We no longer look at the in_secure field of the S1Translate struct
> anyway, so we can remove it and all the code which sets it.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/ptw.c | 13 -------------
>   1 file changed, 13 deletions(-)

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

r~

[PATCH 10/14] target/arm/ptw: Drop S1Translate::out_secure

[Peter Maydell]

We only use S1Translate::out_secure in two places, where we are
setting up MemTxAttrs for a page table load. We can use
arm_space_is_secure(ptw->out_space) instead, which guarantees
that we're setting the MemTxAttrs secure and space fields
consistently, and allows us to drop the out_secure field in
S1Translate entirely.

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

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 77b8382ceff..2be6bf302b0 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -63,7 +63,6 @@ typedef struct S1Translate {
      * Stage 2 is indicated by in_mmu_idx set to ARMMMUIdx_Stage2{,_S}.
      */
     bool in_s1_is_el0;
-    bool out_secure;
     bool out_rw;
     bool out_be;
     ARMSecuritySpace out_space;
@@ -551,7 +550,6 @@ static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw,
         pte_attrs = s2.cacheattrs.attrs;
         ptw->out_host = NULL;
         ptw->out_rw = false;
-        ptw->out_secure = s2.f.attrs.secure;
         ptw->out_space = s2.f.attrs.space;
     } else {
 #ifdef CONFIG_TCG
@@ -570,7 +568,6 @@ static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw,
         ptw->out_phys = full->phys_addr | (addr & ~TARGET_PAGE_MASK);
         ptw->out_rw = full->prot & PAGE_WRITE;
         pte_attrs = full->pte_attrs;
-        ptw->out_secure = full->attrs.secure;
         ptw->out_space = full->attrs.space;
 #else
         g_assert_not_reached();
@@ -628,8 +625,8 @@ static uint32_t arm_ldl_ptw(CPUARMState *env, S1Translate *ptw,
     } else {
         /* Page tables are in MMIO. */
         MemTxAttrs attrs = {
-            .secure = ptw->out_secure,
             .space = ptw->out_space,
+            .secure = arm_space_is_secure(ptw->out_space),
         };
         AddressSpace *as = arm_addressspace(cs, attrs);
         MemTxResult result = MEMTX_OK;
@@ -674,8 +671,8 @@ static uint64_t arm_ldq_ptw(CPUARMState *env, S1Translate *ptw,
     } else {
         /* Page tables are in MMIO. */
         MemTxAttrs attrs = {
-            .secure = ptw->out_secure,
             .space = ptw->out_space,
+            .secure = arm_space_is_secure(ptw->out_space),
         };
         AddressSpace *as = arm_addressspace(cs, attrs);
         MemTxResult result = MEMTX_OK;
-- 
2.34.1

Re: [PATCH 10/14] target/arm/ptw: Drop S1Translate::out_secure

[Richard Henderson]

On 7/14/23 16:46, Peter Maydell wrote:
> We only use S1Translate::out_secure in two places, where we are
> setting up MemTxAttrs for a page table load. We can use
> arm_space_is_secure(ptw->out_space) instead, which guarantees
> that we're setting the MemTxAttrs secure and space fields
> consistently, and allows us to drop the out_secure field in
> S1Translate entirely.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/ptw.c | 7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)

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

r~

[PATCH 11/14] target/arm/ptw: Set attributes correctly for MMU disabled data accesses

[Peter Maydell]

When the MMU is disabled, data accesses should be Device nGnRnE,
Outer Shareable, Untagged.  We handle the other cases from
AArch64.S1DisabledOutput() correctly but missed this one.
Device nGnRnE is memattr == 0, so the only part we were missing
was that shareability should be set to 2 for both insn fetches
and data accesses.

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

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 2be6bf302b0..e4210abc148 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -3106,11 +3106,13 @@ static bool get_phys_addr_disabled(CPUARMState *env,
                 }
             }
         }
-        if (memattr == 0 && access_type == MMU_INST_FETCH) {
-            if (regime_sctlr(env, mmu_idx) & SCTLR_I) {
-                memattr = 0xee;  /* Normal, WT, RA, NT */
-            } else {
-                memattr = 0x44;  /* Normal, NC, No */
+        if (memattr == 0) {
+            if (access_type == MMU_INST_FETCH) {
+                if (regime_sctlr(env, mmu_idx) & SCTLR_I) {
+                    memattr = 0xee;  /* Normal, WT, RA, NT */
+                } else {
+                    memattr = 0x44;  /* Normal, NC, No */
+                }
             }
             shareability = 2; /* outer shareable */
         }
-- 
2.34.1

Re: [PATCH 11/14] target/arm/ptw: Set attributes correctly for MMU disabled data accesses

[Richard Henderson]

On 7/14/23 16:46, Peter Maydell wrote:
> When the MMU is disabled, data accesses should be Device nGnRnE,
> Outer Shareable, Untagged.  We handle the other cases from
> AArch64.S1DisabledOutput() correctly but missed this one.
> Device nGnRnE is memattr == 0, so the only part we were missing
> was that shareability should be set to 2 for both insn fetches
> and data accesses.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/ptw.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)

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

r~

[PATCH 12/14] target/arm/ptw: Check for block descriptors at invalid levels

[Peter Maydell]

The architecture doesn't permit block descriptors at any arbitrary
level of the page table walk; it depends on the granule size which
levels are permitted.  We implemented only a partial version of this
check which assumes that block descriptors are valid at all levels
except level 3, which meant that we wouldn't deliver the Translation
fault for all cases of this sort of guest page table error.

Implement the logic corresponding to the pseudocode
AArch64.DecodeDescriptorType() and AArch64.BlockDescSupported().

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

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index e4210abc148..ed46bb82a75 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -1549,6 +1549,25 @@ static int check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, uint64_t tcr,
     return INT_MIN;
 }
 
+static bool lpae_block_desc_valid(ARMCPU *cpu, bool ds,
+                                  ARMGranuleSize gran, int level)
+{
+    /*
+     * See pseudocode AArch46.BlockDescSupported(): block descriptors
+     * are not valid at all levels, depending on the page size.
+     */
+    switch (gran) {
+    case Gran4K:
+        return (level == 0 && ds) || level == 1 || level == 2;
+    case Gran16K:
+        return (level == 1 && ds) || level == 2;
+    case Gran64K:
+        return (level == 1 && arm_pamax(cpu) == 52) || level == 2;
+    default:
+        g_assert_not_reached();
+    }
+}
+
 /**
  * get_phys_addr_lpae: perform one stage of page table walk, LPAE format
  *
@@ -1784,8 +1803,10 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
     new_descriptor = descriptor;
 
  restart_atomic_update:
-    if (!(descriptor & 1) || (!(descriptor & 2) && (level == 3))) {
-        /* Invalid, or the Reserved level 3 encoding */
+    if (!(descriptor & 1) ||
+        (!(descriptor & 2) &&
+         !lpae_block_desc_valid(cpu, param.ds, param.gran, level))) {
+        /* Invalid, or a block descriptor at an invalid level */
         goto do_translation_fault;
     }
 
-- 
2.34.1

Re: [PATCH 12/14] target/arm/ptw: Check for block descriptors at invalid levels

[Richard Henderson]

On 7/14/23 16:46, Peter Maydell wrote:
> The architecture doesn't permit block descriptors at any arbitrary
> level of the page table walk; it depends on the granule size which
> levels are permitted.  We implemented only a partial version of this
> check which assumes that block descriptors are valid at all levels
> except level 3, which meant that we wouldn't deliver the Translation
> fault for all cases of this sort of guest page table error.
> 
> Implement the logic corresponding to the pseudocode
> AArch64.DecodeDescriptorType() and AArch64.BlockDescSupported().
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/ptw.c | 25 +++++++++++++++++++++++--
>   1 file changed, 23 insertions(+), 2 deletions(-)

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

r~

[PATCH 13/14] target/arm/ptw: Report stage 2 fault level for stage 2 faults on stage 1 ptw

[Peter Maydell]

When we report faults due to stage 2 faults during a stage 1
page table walk, the 'level' parameter should be the level
of the walk in stage 2 that faulted, not the level of the
walk in stage 1. Correct the reporting of these faults.

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

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index ed46bb82a75..d1de934702d 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -2046,9 +2046,13 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
  do_translation_fault:
     fi->type = ARMFault_Translation;
  do_fault:
-    fi->level = level;
-    /* Tag the error as S2 for failed S1 PTW at S2 or ordinary S2.  */
-    fi->stage2 = fi->s1ptw || regime_is_stage2(mmu_idx);
+    if (fi->s1ptw) {
+        /* Retain the existing stage 2 fi->level */
+        assert(fi->stage2);
+    } else {
+        fi->level = level;
+        fi->stage2 = regime_is_stage2(mmu_idx);
+    }
     fi->s1ns = fault_s1ns(ptw->in_space, mmu_idx);
     return true;
 }
-- 
2.34.1

Re: [PATCH 13/14] target/arm/ptw: Report stage 2 fault level for stage 2 faults on stage 1 ptw

[Richard Henderson]

On 7/14/23 16:46, Peter Maydell wrote:
> When we report faults due to stage 2 faults during a stage 1
> page table walk, the 'level' parameter should be the level
> of the walk in stage 2 that faulted, not the level of the
> walk in stage 1. Correct the reporting of these faults.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/ptw.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)

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

r~

[PATCH 14/14] target/arm: Adjust PAR_EL1.SH for Device and Normal-NC memory types

[Peter Maydell]

The PAR_EL1.SH field documents that for the cases of:
 * Device memory
 * Normal memory with both Inner and Outer Non-Cacheable
the field should be 0b10 rather than whatever was in the
translation table descriptor field. (In the pseudocode this
is handled by PAREncodeShareability().) Perform this
adjustment when assembling a PAR value.

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

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 1e45fdb47c9..f9c00827018 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -3342,6 +3342,19 @@ static CPAccessResult ats_access(CPUARMState *env, const ARMCPRegInfo *ri,
 }
 
 #ifdef CONFIG_TCG
+static int par_el1_shareability(GetPhysAddrResult *res)
+{
+    /*
+     * The PAR_EL1.SH field must be 0b10 for Device or Normal-NC
+     * memory -- see pseudocode PAREncodeShareability().
+     */
+    if (((res->cacheattrs.attrs & 0xf0) == 0) ||
+        res->cacheattrs.attrs == 0x44 || res->cacheattrs.attrs == 0x40) {
+        return 2;
+    }
+    return res->cacheattrs.shareability;
+}
+
 static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
                              MMUAccessType access_type, ARMMMUIdx mmu_idx,
                              bool is_secure)
@@ -3470,7 +3483,7 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
                 par64 |= (1 << 9); /* NS */
             }
             par64 |= (uint64_t)res.cacheattrs.attrs << 56; /* ATTR */
-            par64 |= res.cacheattrs.shareability << 7; /* SH */
+            par64 |= par_el1_shareability(&res) << 7; /* SH */
         } else {
             uint32_t fsr = arm_fi_to_lfsc(&fi);
 
-- 
2.34.1

Re: [PATCH 14/14] target/arm: Adjust PAR_EL1.SH for Device and Normal-NC memory types

[Richard Henderson]

On 7/14/23 16:46, Peter Maydell wrote:
> The PAR_EL1.SH field documents that for the cases of:
>   * Device memory
>   * Normal memory with both Inner and Outer Non-Cacheable
> the field should be 0b10 rather than whatever was in the
> translation table descriptor field. (In the pseudocode this
> is handled by PAREncodeShareability().) Perform this
> adjustment when assembling a PAR value.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/helper.c | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)

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

r~