[PATCH 1/2] arm/ptw: factor out wxn logic to separate functions

[PATCH 1/2] arm/ptw: factor out wxn logic to separate functions

[Pavel Skripkin]

The next patch will add support for WXN for short descriptor format. To
prevent code duplication, wxn logic was factored out to separate
functions.

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

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 9849949508..2a3933adec 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -1130,6 +1130,26 @@ do_fault:
     return true;
 }
 
+static bool arm_has_wxn(CPUARMState *env)
+{
+    /* TODO have_wxn should be replaced with
+     *   ARM_FEATURE_V8 || (ARM_FEATURE_V7 && ARM_FEATURE_EL2)
+     * when ARM_FEATURE_EL2 starts getting set. For now we assume all LPAE
+     * compatible processors have EL2, which is required for [U]WXN.
+     */
+    return arm_feature(env, ARM_FEATURE_LPAE);
+}
+
+static bool arm_wxn_enabled(CPUARMState *env, ARMMMUIdx mmu_idx)
+{
+    return arm_has_wxn(env) && (regime_sctlr(env, mmu_idx) & SCTLR_WXN);
+}
+
+static bool arm_uwxn_enabled(CPUARMState *env, ARMMMUIdx mmu_idx)
+{
+    return arm_has_wxn(env) && (regime_sctlr(env, mmu_idx) & SCTLR_UWXN);
+}
+
 static bool get_phys_addr_v6(CPUARMState *env, S1Translate *ptw,
                              uint32_t address, MMUAccessType access_type,
                              GetPhysAddrResult *result, ARMMMUFaultInfo *fi)
@@ -1370,8 +1390,7 @@ static int get_S1prot(CPUARMState *env, ARMMMUIdx mmu_idx, bool is_aa64,
     ARMCPU *cpu = env_archcpu(env);
     bool is_user = regime_is_user(env, mmu_idx);
     int prot_rw, user_rw;
-    bool have_wxn;
-    int wxn = 0;
+    int wxn = arm_wxn_enabled(env, mmu_idx);
 
     assert(!regime_is_stage2(mmu_idx));
 
@@ -1432,18 +1451,6 @@ static int get_S1prot(CPUARMState *env, ARMMMUIdx mmu_idx, bool is_aa64,
             g_assert_not_reached();
         }
     }
-
-    /* TODO have_wxn should be replaced with
-     *   ARM_FEATURE_V8 || (ARM_FEATURE_V7 && ARM_FEATURE_EL2)
-     * when ARM_FEATURE_EL2 starts getting set. For now we assume all LPAE
-     * compatible processors have EL2, which is required for [U]WXN.
-     */
-    have_wxn = arm_feature(env, ARM_FEATURE_LPAE);
-
-    if (have_wxn) {
-        wxn = regime_sctlr(env, mmu_idx) & SCTLR_WXN;
-    }
-
     if (is_aa64) {
         if (regime_has_2_ranges(mmu_idx) && !is_user) {
             xn = pxn || (user_rw & PAGE_WRITE);
@@ -1455,10 +1462,8 @@ static int get_S1prot(CPUARMState *env, ARMMMUIdx mmu_idx, bool is_aa64,
             if (is_user) {
                 xn = xn || !(user_rw & PAGE_READ);
             } else {
-                int uwxn = 0;
-                if (have_wxn) {
-                    uwxn = regime_sctlr(env, mmu_idx) & SCTLR_UWXN;
-                }
+                int uwxn = arm_uwxn_enabled(env, mmu_idx);
+
                 xn = xn || !(prot_rw & PAGE_READ) || pxn ||
                      (uwxn && (user_rw & PAGE_WRITE));
             }
-- 
2.46.0

[PATCH 1/2] arm/ptw: factor out wxn logic to separate functions

[Pavel Skripkin]

The next patch will add support for WXN for short descriptor format. To
prevent code duplication, wxn logic was factored out to separate
functions.

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

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 9849949508..2a3933adec 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -1130,6 +1130,26 @@ do_fault:
     return true;
 }
 
+static bool arm_has_wxn(CPUARMState *env)
+{
+    /* TODO have_wxn should be replaced with
+     *   ARM_FEATURE_V8 || (ARM_FEATURE_V7 && ARM_FEATURE_EL2)
+     * when ARM_FEATURE_EL2 starts getting set. For now we assume all LPAE
+     * compatible processors have EL2, which is required for [U]WXN.
+     */
+    return arm_feature(env, ARM_FEATURE_LPAE);
+}
+
+static bool arm_wxn_enabled(CPUARMState *env, ARMMMUIdx mmu_idx)
+{
+    return arm_has_wxn(env) && (regime_sctlr(env, mmu_idx) & SCTLR_WXN);
+}
+
+static bool arm_uwxn_enabled(CPUARMState *env, ARMMMUIdx mmu_idx)
+{
+    return arm_has_wxn(env) && (regime_sctlr(env, mmu_idx) & SCTLR_UWXN);
+}
+
 static bool get_phys_addr_v6(CPUARMState *env, S1Translate *ptw,
                              uint32_t address, MMUAccessType access_type,
                              GetPhysAddrResult *result, ARMMMUFaultInfo *fi)
@@ -1370,8 +1390,7 @@ static int get_S1prot(CPUARMState *env, ARMMMUIdx mmu_idx, bool is_aa64,
     ARMCPU *cpu = env_archcpu(env);
     bool is_user = regime_is_user(env, mmu_idx);
     int prot_rw, user_rw;
-    bool have_wxn;
-    int wxn = 0;
+    int wxn = arm_wxn_enabled(env, mmu_idx);
 
     assert(!regime_is_stage2(mmu_idx));
 
@@ -1432,18 +1451,6 @@ static int get_S1prot(CPUARMState *env, ARMMMUIdx mmu_idx, bool is_aa64,
             g_assert_not_reached();
         }
     }
-
-    /* TODO have_wxn should be replaced with
-     *   ARM_FEATURE_V8 || (ARM_FEATURE_V7 && ARM_FEATURE_EL2)
-     * when ARM_FEATURE_EL2 starts getting set. For now we assume all LPAE
-     * compatible processors have EL2, which is required for [U]WXN.
-     */
-    have_wxn = arm_feature(env, ARM_FEATURE_LPAE);
-
-    if (have_wxn) {
-        wxn = regime_sctlr(env, mmu_idx) & SCTLR_WXN;
-    }
-
     if (is_aa64) {
         if (regime_has_2_ranges(mmu_idx) && !is_user) {
             xn = pxn || (user_rw & PAGE_WRITE);
@@ -1455,10 +1462,8 @@ static int get_S1prot(CPUARMState *env, ARMMMUIdx mmu_idx, bool is_aa64,
             if (is_user) {
                 xn = xn || !(user_rw & PAGE_READ);
             } else {
-                int uwxn = 0;
-                if (have_wxn) {
-                    uwxn = regime_sctlr(env, mmu_idx) & SCTLR_UWXN;
-                }
+                int uwxn = arm_uwxn_enabled(env, mmu_idx);
+
                 xn = xn || !(prot_rw & PAGE_READ) || pxn ||
                      (uwxn && (user_rw & PAGE_WRITE));
             }
-- 
2.46.0

[PATCH 2/2] arm/ptw: respect sctlr.{u}wxn in get_phys_addr_v6

[Pavel Skripkin]

get_phys_addr_v6() is used for decoding armv7's short descriptor format.
Based on ARM ARM AArch32.S1SDHasPermissionsFault(), WXN should be
respected in !LPAE mode as well.

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

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 2a3933adec..892932620e 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -1169,6 +1169,8 @@ static bool get_phys_addr_v6(CPUARMState *env, S1Translate *ptw,
     uint32_t dacr;
     bool ns;
     int user_prot;
+    int wxn = arm_wxn_enabled(env, mmu_idx);
+    int uwxn = arm_uwxn_enabled(env, mmu_idx);
 
     /* Pagetable walk.  */
     /* Lookup l1 descriptor.  */
@@ -1288,6 +1290,15 @@ static bool get_phys_addr_v6(CPUARMState *env, S1Translate *ptw,
         if (result->f.prot && !xn) {
             result->f.prot |= PAGE_EXEC;
         }
+
+        if (result->f.prot & PAGE_WRITE) {
+            /* WXN works for PL1&0, while UWXN only for PL0. */
+            if (wxn)
+                result->f.prot &= ~PAGE_EXEC;
+            if (uwxn && regime_is_user(env, mmu_idx))
+                result->f.prot &= ~PAGE_EXEC;
+        }
+
         if (!(result->f.prot & (1 << access_type))) {
             /* Access permission fault.  */
             fi->type = ARMFault_Permission;
-- 
2.46.0

Re: [PATCH 2/2] arm/ptw: respect sctlr.{u}wxn in get_phys_addr_v6

[Peter Maydell]

On Thu, 14 Nov 2024 at 16:59, Pavel Skripkin <paskripkin@gmail.com> wrote:
>
> get_phys_addr_v6() is used for decoding armv7's short descriptor format.
> Based on ARM ARM AArch32.S1SDHasPermissionsFault(), WXN should be
> respected in !LPAE mode as well.
>
> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
> ---
>  target/arm/ptw.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)

Instead of this, would it be possible to have get_phys_addr_v6()
call get_S1prot() (replacing most of the existing open-coded
handling of PAN, simple vs classing AP model, etc) ? I haven't
looked at the fine detail, so we might need to tweak get_S1prot()
if it's missing logic that only matters in the short-descriptor
case, but I think that would be better than having two places
that need updating for new architectural features.

For instance, we are also missing the handling for SCR.SIF with
short descriptors which you can see in the S1SDHasPermissionsFault()
pseudocode and which we would get if we could make the
short-descriptor code call get_S1Prot().

Couple of minor notes for v2:
 * if you're sending a multi-patch patchset, please use a
   cover-letter email. Some of our automated tooling gets
   confused by multi-patch patches without cover letters
   (and, conversely, by single-patches with cover letters)
 * our coding style says all if() statements should have
   braces, even for single-line bodies

thanks
-- PMM

Re: [PATCH 2/2] arm/ptw: respect sctlr.{u}wxn in get_phys_addr_v6

[Pavel Skripkin]

Hi Peter,

Peter Maydell <peter.maydell@linaro.org> says:
> On Thu, 14 Nov 2024 at 16:59, Pavel Skripkin <paskripkin@gmail.com> wrote:
>>
>> get_phys_addr_v6() is used for decoding armv7's short descriptor format.
>> Based on ARM ARM AArch32.S1SDHasPermissionsFault(), WXN should be
>> respected in !LPAE mode as well.
>>
>> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
>> ---
>>  target/arm/ptw.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
> 
> Instead of this, would it be possible to have get_phys_addr_v6()
> call get_S1prot() (replacing most of the existing open-coded
> handling of PAN, simple vs classing AP model, etc) ? I haven't
> looked at the fine detail, so we might need to tweak get_S1prot()
> if it's missing logic that only matters in the short-descriptor
> case, but I think that would be better than having two places
> that need updating for new architectural features.
> 

I also thought about that, but suspected there was a reason it was not 
used at the first place.

Will take a look, if it's possible.

> For instance, we are also missing the handling for SCR.SIF with
> short descriptors which you can see in the S1SDHasPermissionsFault()
> pseudocode and which we would get if we could make the
> short-descriptor code call get_S1Prot().
> 
> Couple of minor notes for v2:
>   * if you're sending a multi-patch patchset, please use a
>     cover-letter email. Some of our automated tooling gets
>     confused by multi-patch patches without cover letters
>     (and, conversely, by single-patches with cover letters)
>   * our coding style says all if() statements should have
>     braces, even for single-line bodies
> 


Thanks for guidance!



With regards,
Pavel Skripkin

Re: [PATCH 2/2] arm/ptw: respect sctlr.{u}wxn in get_phys_addr_v6

[Peter Maydell]

On Fri, 15 Nov 2024 at 16:54, Pavel Skripkin <paskripkin@gmail.com> wrote:
>
> Hi Peter,
>
> Peter Maydell <peter.maydell@linaro.org> says:
> > On Thu, 14 Nov 2024 at 16:59, Pavel Skripkin <paskripkin@gmail.com> wrote:
> >>
> >> get_phys_addr_v6() is used for decoding armv7's short descriptor format.
> >> Based on ARM ARM AArch32.S1SDHasPermissionsFault(), WXN should be
> >> respected in !LPAE mode as well.
> >>
> >> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
> >> ---
> >>  target/arm/ptw.c | 11 +++++++++++
> >>  1 file changed, 11 insertions(+)
> >
> > Instead of this, would it be possible to have get_phys_addr_v6()
> > call get_S1prot() (replacing most of the existing open-coded
> > handling of PAN, simple vs classing AP model, etc) ? I haven't
> > looked at the fine detail, so we might need to tweak get_S1prot()
> > if it's missing logic that only matters in the short-descriptor
> > case, but I think that would be better than having two places
> > that need updating for new architectural features.
> >
>
> I also thought about that, but suspected there was a reason it was not
> used at the first place.

I think it's likely purely for historical reasons. The
short-descriptor handling code is older, and has
had to change less to accommodate new architectural
features. When we originally created get_S1prot()
we were doing it as a refactoring to deal with
increasing complexity in the previously open-coded
handling of access bits in get_phys_addr_lpae() and
I don't think we really thought much about the possibility
of code-sharing with the short-descriptor codepath.

thanks
-- PMM