To share missing SCTRL.{U}WXN and SCR.SIF in short format walker, use
get_S1prot instead of open-coded checks.

Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---
 target/arm/ptw.c | 41 ++++++++++++++++++-----------------------
 1 file changed, 18 insertions(+), 23 deletions(-)

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 50eed0f811..0d003a9f7d 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -85,6 +85,10 @@ static bool get_phys_addr_gpc(CPUARMState *env, S1Translate *ptw,
                               GetPhysAddrResult *result,
                               ARMMMUFaultInfo *fi);
 
+static int get_S1prot(CPUARMState *env, ARMMMUIdx mmu_idx, bool is_aa64,
+                      int user_rw, int prot_rw, int xn, int pxn,
+                      ARMSecuritySpace in_pa, ARMSecuritySpace out_pa);
+
 /* This mapping is common between ID_AA64MMFR0.PARANGE and TCR_ELx.{I}PS. */
 static const uint8_t pamax_map[] = {
     [0] = 32,
@@ -1148,7 +1152,6 @@ static bool get_phys_addr_v6(CPUARMState *env, S1Translate *ptw,
     hwaddr phys_addr;
     uint32_t dacr;
     bool ns;
-    int user_prot;
 
     /* Pagetable walk.  */
     /* Lookup l1 descriptor.  */
@@ -1243,13 +1246,13 @@ static bool get_phys_addr_v6(CPUARMState *env, S1Translate *ptw,
     if (domain_prot == 3) {
         result->f.prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
     } else {
-        if (pxn && !regime_is_user(env, mmu_idx)) {
-            xn = 1;
-        }
-        if (xn && access_type == MMU_INST_FETCH) {
-            fi->type = ARMFault_Permission;
-            goto do_fault;
-        }
+        int ap_usr;
+        int ap_priv;
+        ARMSecuritySpace out_space = ARMSS_NonSecure;
+
+        /* NS bit is ignored in NWd. */
+        if (result->f.attrs.space == ARMSS_Secure && !ns)
+            out_space = ARMSS_Secure;
 
         if (arm_feature(env, ARM_FEATURE_V6K) &&
                 (regime_sctlr(env, mmu_idx) & SCTLR_AFE)) {
@@ -1259,28 +1262,20 @@ static bool get_phys_addr_v6(CPUARMState *env, S1Translate *ptw,
                 fi->type = ARMFault_AccessFlag;
                 goto do_fault;
             }
-            result->f.prot = simple_ap_to_rw_prot(env, mmu_idx, ap >> 1);
-            user_prot = simple_ap_to_rw_prot_is_user(ap >> 1, 1);
+            ap_priv = simple_ap_to_rw_prot(env, mmu_idx, ap >> 1);
+            ap_usr = simple_ap_to_rw_prot_is_user(ap >> 1, 1);
         } else {
-            result->f.prot = ap_to_rw_prot(env, mmu_idx, ap, domain_prot);
-            user_prot = ap_to_rw_prot_is_user(env, mmu_idx, ap, domain_prot, 1);
-        }
-        if (result->f.prot && !xn) {
-            result->f.prot |= PAGE_EXEC;
+            ap_priv = ap_to_rw_prot(env, mmu_idx, ap, domain_prot);
+            ap_usr = ap_to_rw_prot_is_user(env, mmu_idx, ap, domain_prot, 1);
         }
+
+        result->f.prot = get_S1prot(env, mmu_idx, false, ap_usr, ap_priv, xn, pxn,
+                                    result->f.attrs.space, out_space);
         if (!(result->f.prot & (1 << access_type))) {
             /* Access permission fault.  */
             fi->type = ARMFault_Permission;
             goto do_fault;
         }
-        if (regime_is_pan(env, mmu_idx) &&
-            !regime_is_user(env, mmu_idx) &&
-            user_prot &&
-            access_type != MMU_INST_FETCH) {
-            /* Privileged Access Never fault */
-            fi->type = ARMFault_Permission;
-            goto do_fault;
-        }
     }
     if (ns) {
         /* The NS bit will (as required by the architecture) have no effect if
-- 
2.46.0

On Sun, 17 Nov 2024 at 13:50, Pavel Skripkin <paskripkin@gmail.com> wrote:
>
> To share missing SCTRL.{U}WXN and SCR.SIF in short format walker, use
> get_S1prot instead of open-coded checks.
>
> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
> ---
>  target/arm/ptw.c | 41 ++++++++++++++++++-----------------------
>  1 file changed, 18 insertions(+), 23 deletions(-)
>
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index 50eed0f811..0d003a9f7d 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -85,6 +85,10 @@ static bool get_phys_addr_gpc(CPUARMState *env, S1Translate *ptw,
>                                GetPhysAddrResult *result,
>                                ARMMMUFaultInfo *fi);
>
> +static int get_S1prot(CPUARMState *env, ARMMMUIdx mmu_idx, bool is_aa64,
> +                      int user_rw, int prot_rw, int xn, int pxn,
> +                      ARMSecuritySpace in_pa, ARMSecuritySpace out_pa);
> +
>  /* This mapping is common between ID_AA64MMFR0.PARANGE and TCR_ELx.{I}PS. */
>  static const uint8_t pamax_map[] = {
>      [0] = 32,
> @@ -1148,7 +1152,6 @@ static bool get_phys_addr_v6(CPUARMState *env, S1Translate *ptw,
>      hwaddr phys_addr;
>      uint32_t dacr;
>      bool ns;
> -    int user_prot;
>
>      /* Pagetable walk.  */
>      /* Lookup l1 descriptor.  */
> @@ -1243,13 +1246,13 @@ static bool get_phys_addr_v6(CPUARMState *env, S1Translate *ptw,
>      if (domain_prot == 3) {
>          result->f.prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
>      } else {
> -        if (pxn && !regime_is_user(env, mmu_idx)) {
> -            xn = 1;
> -        }
> -        if (xn && access_type == MMU_INST_FETCH) {
> -            fi->type = ARMFault_Permission;
> -            goto do_fault;
> -        }
> +        int ap_usr;
> +        int ap_priv;
> +        ARMSecuritySpace out_space = ARMSS_NonSecure;
> +
> +        /* NS bit is ignored in NWd. */
> +        if (result->f.attrs.space == ARMSS_Secure && !ns)
> +            out_space = ARMSS_Secure;

Instead of doing this, I think we should align the code a bit
more closely to how we do it in the lpae codepath. So before
"if (domain_prot == 3)" put

     out_space = ptw->in_space;
     if (ns) {
        /*
         * The NS bit will (as required by the architecture) have no effect if
         * the CPU doesn't support TZ or this is a non-secure translation
         * regime, because the output space will already be non-secure.
         */
         out_space = ARMSS_NonSecure;
     }

and then delete the current "if (ns)" code block at the end of
get_phys_addr_v6() and replace it with an unconditional

    result->f.attrs.space = out_space;
    result->f.attrs.secure = arm_space_is_secure(out_space);

That way we're determining the output space in exactly one
place, and the code at the end that updates result->f.attrs
is the same as the way we do it in the lpae function.

(PS: note that QEMU coding style mandates braces on if()
statements even if there is just one statement in the body.)

Otherwise I think this looks good -- thanks for doing
the refactoring.

-- PMM