[Qemu-devel] [PATCH v2 0/3] tcg-arm: LPAE: fix and extend xn control

[Qemu-devel] [PATCH v2 0/3] tcg-arm: LPAE: fix and extend xn control

[Andrew Jones]

This series fixes and extends the determination of whether or
not an address is executable for LPAE translations. The main
patch is 3/3, and describes the details in its commit message.
Patch 1/3 prepares for patch 2/3, which is prep for 3/3, and also
fixes a potential problem with checking access permissions on
v6 processors when they use the simple AP format.

Changes since v1:
* There are now 3 patches instead of 5. This is due to approaching
  the prep patches in a different way. Most notably, simple AP
  format is now handled with its own function. [Peter Maydell]
* A check of SCTLR_AFE is guarded with ARM_FEATURE_V6K [Peter Maydell]
* All other cleanups suggested by Peter Maydell.

Tested by booting Linux on mach-virt with cortex-a15 and cortex-a57
(just up to 'Unable to mount root fs'), and also with a kvm-unit-tests
test. The curious can check out the unit test here [1].

Thanks in advance for reviews!

drew

[1] https://github.com/rhdrjones/kvm-unit-tests/commit/ee553e4bb795b0150e31c794bf8953dfb08d619a

Andrew Jones (3):
  target-arm: convert check_ap to ap_to_rw_prot
  target-arm: fix get_phys_addr_v6/SCTLR_AFE access check
  target-arm: get_phys_addr_lpae: more xn control

 target-arm/helper.c | 213 +++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 154 insertions(+), 59 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v2 1/3] target-arm: convert check_ap to ap_to_rw_prot

[Andrew Jones]

Instead of mixing access permission checking with access permissions
to page protection flags translation, just do the translation, and
leave it to the caller to check the protection flags against the access
type. Also rename to ap_to_rw_prot to better describe the new behavior.

Signed-off-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c | 49 +++++++++++++++++++------------------------------
 1 file changed, 19 insertions(+), 30 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 3bc20af04f012..d1e70a86a647c 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -4903,34 +4903,23 @@ static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx)
     }
 }
 
-/* Check section/page access permissions.
-   Returns the page protection flags, or zero if the access is not
-   permitted.  */
-static inline int check_ap(CPUARMState *env, ARMMMUIdx mmu_idx,
-                           int ap, int domain_prot,
-                           int access_type)
-{
-    int prot_ro;
+/* Translate section/page access permissions to page
+ * R/W protection flags
+ */
+static inline int ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
+                                int ap, int domain_prot)
+{
     bool is_user = regime_is_user(env, mmu_idx);
 
     if (domain_prot == 3) {
         return PAGE_READ | PAGE_WRITE;
     }
 
-    if (access_type == 1) {
-        prot_ro = 0;
-    } else {
-        prot_ro = PAGE_READ;
-    }
-
     switch (ap) {
     case 0:
         if (arm_feature(env, ARM_FEATURE_V7)) {
             return 0;
         }
-        if (access_type == 1) {
-            return 0;
-        }
         switch (regime_sctlr(env, mmu_idx) & (SCTLR_S | SCTLR_R)) {
         case SCTLR_S:
             return is_user ? 0 : PAGE_READ;
@@ -4943,7 +4932,7 @@ static inline int check_ap(CPUARMState *env, ARMMMUIdx mmu_idx,
         return is_user ? 0 : PAGE_READ | PAGE_WRITE;
     case 2:
         if (is_user) {
-            return prot_ro;
+            return PAGE_READ;
         } else {
             return PAGE_READ | PAGE_WRITE;
         }
@@ -4952,16 +4941,16 @@ static inline int check_ap(CPUARMState *env, ARMMMUIdx mmu_idx,
     case 4: /* Reserved.  */
         return 0;
     case 5:
-        return is_user ? 0 : prot_ro;
+        return is_user ? 0 : PAGE_READ;
     case 6:
-        return prot_ro;
+        return PAGE_READ;
     case 7:
         if (!arm_feature(env, ARM_FEATURE_V6K)) {
             return 0;
         }
-        return prot_ro;
+        return PAGE_READ;
     default:
-        abort();
+        g_assert_not_reached();
     }
 }
 
@@ -5083,12 +5072,12 @@ static int get_phys_addr_v5(CPUARMState *env, uint32_t address, int access_type,
         }
         code = 15;
     }
-    *prot = check_ap(env, mmu_idx, ap, domain_prot, access_type);
-    if (!*prot) {
+    *prot = ap_to_rw_prot(env, mmu_idx, ap, domain_prot);
+    *prot |= *prot ? PAGE_EXEC : 0;
+    if (!(*prot & (1 << access_type))) {
         /* Access permission fault.  */
         goto do_fault;
     }
-    *prot |= PAGE_EXEC;
     *phys_ptr = phys_addr;
     return 0;
 do_fault:
@@ -5204,14 +5193,14 @@ static int get_phys_addr_v6(CPUARMState *env, uint32_t address, int access_type,
             code = (code == 15) ? 6 : 3;
             goto do_fault;
         }
-        *prot = check_ap(env, mmu_idx, ap, domain_prot, access_type);
-        if (!*prot) {
+        *prot = ap_to_rw_prot(env, mmu_idx, ap, domain_prot);
+        if (*prot && !xn) {
+            *prot |= PAGE_EXEC;
+        }
+        if (!(*prot & (1 << access_type))) {
             /* Access permission fault.  */
             goto do_fault;
         }
-        if (!xn) {
-            *prot |= PAGE_EXEC;
-        }
     }
     *phys_ptr = phys_addr;
     return 0;
-- 
1.9.3

[Qemu-devel] [PATCH v2 2/3] target-arm: fix get_phys_addr_v6/SCTLR_AFE access check

[Andrew Jones]

Introduce simple_ap_to_rw_prot(), which has the same behavior as
ap_to_rw_prot(), but takes the 2-bit simple AP[2:1] instead of
the 3-bit AP[2:0]. Use this in get_phys_addr_v6 when SCTLR_AFE
is set, as that bit indicates we should be using the simple AP
format.

It's unlikely this path is getting used. I don't see CR_AFE
getting used by Linux, so possibly not. If it had been, then
the check would have been wrong for all but AP[2:1] = 0b11.
Anyway, this should fix it up, in case it ever does get used.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 target-arm/helper.c | 49 ++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 42 insertions(+), 7 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index d1e70a86a647c..d996659652f8d 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -4905,6 +4905,11 @@ static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx)
 
 /* Translate section/page access permissions to page
  * R/W protection flags
+ *
+ * @env:         CPUARMState
+ * @mmu_idx:     MMU index indicating required translation regime
+ * @ap:          The 3-bit access permissions (AP[2:0])
+ * @domain_prot: The 2-bit domain access permissions
  */
 static inline int ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
                                 int ap, int domain_prot)
@@ -4954,6 +4959,32 @@ static inline int ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
     }
 }
 
+/* Translate section/page access permissions to page
+ * R/W protection flags.
+ *
+ * @env:     CPUARMState
+ * @mmu_idx: MMU index indicating required translation regime
+ * @ap:      The 2-bit simple AP (AP[2:1])
+ */
+static inline int
+simple_ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, int ap)
+{
+    bool is_user = regime_is_user(env, mmu_idx);
+
+    switch (ap) {
+    case 0:
+        return is_user ? 0 : PAGE_READ | PAGE_WRITE;
+    case 1:
+        return PAGE_READ | PAGE_WRITE;
+    case 2:
+        return is_user ? 0 : PAGE_READ;
+    case 3:
+        return PAGE_READ;
+    default:
+        g_assert_not_reached();
+    }
+}
+
 static bool get_level1_table_address(CPUARMState *env, ARMMMUIdx mmu_idx,
                                      uint32_t *table, uint32_t address)
 {
@@ -5186,14 +5217,18 @@ static int get_phys_addr_v6(CPUARMState *env, uint32_t address, int access_type,
         if (xn && access_type == 2)
             goto do_fault;
 
-        /* The simplified model uses AP[0] as an access control bit.  */
-        if ((regime_sctlr(env, mmu_idx) & SCTLR_AFE)
-                && (ap & 1) == 0) {
-            /* Access flag fault.  */
-            code = (code == 15) ? 6 : 3;
-            goto do_fault;
+        if (arm_feature(env, ARM_FEATURE_V6K) &&
+                (regime_sctlr(env, mmu_idx) & SCTLR_AFE)) {
+            /* The simplified model uses AP[0] as an access control bit.  */
+            if ((ap & 1) == 0) {
+                /* Access flag fault.  */
+                code = (code == 15) ? 6 : 3;
+                goto do_fault;
+            }
+            *prot = simple_ap_to_rw_prot(env, mmu_idx, ap >> 1);
+        } else {
+            *prot = ap_to_rw_prot(env, mmu_idx, ap, domain_prot);
         }
-        *prot = ap_to_rw_prot(env, mmu_idx, ap, domain_prot);
         if (*prot && !xn) {
             *prot |= PAGE_EXEC;
         }
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2 2/3] target-arm: fix get_phys_addr_v6/SCTLR_AFE access check

[Peter Maydell]

On 10 March 2015 at 21:06, Andrew Jones <drjones@redhat.com> wrote:
> Introduce simple_ap_to_rw_prot(), which has the same behavior as
> ap_to_rw_prot(), but takes the 2-bit simple AP[2:1] instead of
> the 3-bit AP[2:0]. Use this in get_phys_addr_v6 when SCTLR_AFE
> is set, as that bit indicates we should be using the simple AP
> format.
>
> It's unlikely this path is getting used. I don't see CR_AFE
> getting used by Linux, so possibly not. If it had been, then
> the check would have been wrong for all but AP[2:1] = 0b11.
> Anyway, this should fix it up, in case it ever does get used.
>
> Signed-off-by: Andrew Jones <drjones@redhat.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[Qemu-devel] [PATCH v2 3/3] target-arm: get_phys_addr_lpae: more xn control

[Andrew Jones]

This patch makes the following changes to the determination of
whether an address is executable, when translating addresses
using LPAE.

1. No longer assumes that PL0 can't execute when it can't read.
   It can in AArch64, a difference from AArch32.
2. Use va_size == 64 to determine we're in AArch64, rather than
   arm_feature(env, ARM_FEATURE_V8), which is insufficient.
3. Add additional XN determinants
   - NS && is_secure && (SCR & SCR_SIF)
   - WXN && (prot & PAGE_WRITE)
   - AArch64: (prot_PL0 & PAGE_WRITE)
   - AArch32: UWXN && (prot_PL0 & PAGE_WRITE)
   - XN determination should also work in secure mode (untested)
   - XN may even work in EL2 (currently impossible to test)
4. Cleans up the bloated PAGE_EXEC condition - by removing it.

The helper get_S1prot is introduced. It may even work in EL2,
when support for that comes, but, as the function name implies,
it only works for stage 1 translations.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 target-arm/helper.c | 129 ++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 100 insertions(+), 29 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index d996659652f8d..c457e9ab8c85a 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -4962,15 +4962,11 @@ static inline int ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
 /* Translate section/page access permissions to page
  * R/W protection flags.
  *
- * @env:     CPUARMState
- * @mmu_idx: MMU index indicating required translation regime
  * @ap:      The 2-bit simple AP (AP[2:1])
+ * @is_user: TRUE if accessing from PL0
  */
-static inline int
-simple_ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, int ap)
+static inline int simple_ap_to_rw_prot_is_user(int ap, bool is_user)
 {
-    bool is_user = regime_is_user(env, mmu_idx);
-
     switch (ap) {
     case 0:
         return is_user ? 0 : PAGE_READ | PAGE_WRITE;
@@ -4985,6 +4981,94 @@ simple_ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, int ap)
     }
 }
 
+static inline int
+simple_ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, int ap)
+{
+    return simple_ap_to_rw_prot_is_user(ap, regime_is_user(env, mmu_idx));
+}
+
+/* Translate section/page access permissions to protection flags
+ *
+ * @env:     CPUARMState
+ * @mmu_idx: MMU index indicating required translation regime
+ * @is_aa64: TRUE if AArch64
+ * @ap:      The 2-bit simple AP (AP[2:1])
+ * @ns:      NS (non-secure) bit
+ * @xn:      XN (execute-never) bit
+ * @pxn:     PXN (privileged execute-never) bit
+ */
+static int get_S1prot(CPUARMState *env, ARMMMUIdx mmu_idx, bool is_aa64,
+                      int ap, int ns, int xn, int pxn)
+{
+    bool is_user = regime_is_user(env, mmu_idx);
+    int prot_rw, user_rw;
+    bool have_wxn;
+    int wxn = 0;
+
+    assert(mmu_idx != ARMMMUIdx_S2NS);
+
+    user_rw = simple_ap_to_rw_prot_is_user(ap, true);
+    if (is_user) {
+        prot_rw = user_rw;
+    } else {
+        prot_rw = simple_ap_to_rw_prot_is_user(ap, false);
+    }
+
+    if (ns && arm_is_secure(env) && (env->cp15.scr_el3 & SCR_SIF)) {
+        return prot_rw;
+    }
+
+    /* 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) {
+        switch (regime_el(env, mmu_idx)) {
+        case 1:
+            if (is_user && !user_rw) {
+                wxn = 0;
+            } else if (!is_user) {
+                xn = pxn || (user_rw & PAGE_WRITE);
+            }
+            break;
+        case 2:
+        case 3:
+            break;
+        }
+    } else if (arm_feature(env, ARM_FEATURE_V7)) {
+        switch (regime_el(env, mmu_idx)) {
+        case 1:
+        case 3:
+            if (is_user) {
+                xn = xn || !user_rw;
+            } else {
+                int uwxn = 0;
+                if (have_wxn) {
+                    uwxn = regime_sctlr(env, mmu_idx) & SCTLR_UWXN;
+                }
+                xn = xn || !prot_rw || pxn || (uwxn && (user_rw & PAGE_WRITE));
+            }
+            break;
+        case 2:
+            break;
+        }
+    } else {
+        xn = wxn = 0;
+    }
+
+    if (xn || (wxn && (prot_rw & PAGE_WRITE))) {
+        return prot_rw;
+    }
+    return prot_rw | PAGE_EXEC;
+}
+
 static bool get_level1_table_address(CPUARMState *env, ARMMMUIdx mmu_idx,
                                      uint32_t *table, uint32_t address)
 {
@@ -5273,8 +5357,8 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
     int32_t granule_sz = 9;
     int32_t va_size = 32;
     int32_t tbi = 0;
-    bool is_user;
     TCR *tcr = regime_tcr(env, mmu_idx);
+    int ap, ns, xn, pxn;
 
     /* TODO:
      * This code assumes we're either a 64-bit EL1 or a 32-bit PL1;
@@ -5446,31 +5530,18 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
         /* Access flag */
         goto do_fault;
     }
+
+    ap = extract32(attrs, 4, 2);
+    ns = extract32(attrs, 3, 1);
+    xn = extract32(attrs, 12, 1);
+    pxn = extract32(attrs, 11, 1);
+
+    *prot = get_S1prot(env, mmu_idx, va_size == 64, ap, ns, xn, pxn);
+
     fault_type = permission_fault;
-    is_user = regime_is_user(env, mmu_idx);
-    if (is_user && !(attrs & (1 << 4))) {
-        /* Unprivileged access not enabled */
+    if (!(*prot & (1 << access_type))) {
         goto do_fault;
     }
-    *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
-    if ((arm_feature(env, ARM_FEATURE_V8) && is_user && (attrs & (1 << 12))) ||
-        (!arm_feature(env, ARM_FEATURE_V8) && (attrs & (1 << 12))) ||
-        (!is_user && (attrs & (1 << 11)))) {
-        /* XN/UXN or PXN. Since we only implement EL0/EL1 we unconditionally
-         * treat XN/UXN as UXN for v8.
-         */
-        if (access_type == 2) {
-            goto do_fault;
-        }
-        *prot &= ~PAGE_EXEC;
-    }
-    if (attrs & (1 << 5)) {
-        /* Write access forbidden */
-        if (access_type == 1) {
-            goto do_fault;
-        }
-        *prot &= ~PAGE_WRITE;
-    }
 
     *phys_ptr = descaddr;
     *page_size_ptr = page_size;
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2 3/3] target-arm: get_phys_addr_lpae: more xn control

[Peter Maydell]

On 10 March 2015 at 21:06, Andrew Jones <drjones@redhat.com> wrote:
> This patch makes the following changes to the determination of
> whether an address is executable, when translating addresses
> using LPAE.
>
> 1. No longer assumes that PL0 can't execute when it can't read.
>    It can in AArch64, a difference from AArch32.
> 2. Use va_size == 64 to determine we're in AArch64, rather than
>    arm_feature(env, ARM_FEATURE_V8), which is insufficient.
> 3. Add additional XN determinants
>    - NS && is_secure && (SCR & SCR_SIF)
>    - WXN && (prot & PAGE_WRITE)
>    - AArch64: (prot_PL0 & PAGE_WRITE)
>    - AArch32: UWXN && (prot_PL0 & PAGE_WRITE)
>    - XN determination should also work in secure mode (untested)
>    - XN may even work in EL2 (currently impossible to test)
> 4. Cleans up the bloated PAGE_EXEC condition - by removing it.
>
> The helper get_S1prot is introduced. It may even work in EL2,
> when support for that comes, but, as the function name implies,
> it only works for stage 1 translations.
>
> Signed-off-by: Andrew Jones <drjones@redhat.com>

I like the general shape of this patch. Minor comment below:

> ---
>  target-arm/helper.c | 129 ++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 100 insertions(+), 29 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index d996659652f8d..c457e9ab8c85a 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4962,15 +4962,11 @@ static inline int ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
>  /* Translate section/page access permissions to page
>   * R/W protection flags.
>   *
> - * @env:     CPUARMState
> - * @mmu_idx: MMU index indicating required translation regime
>   * @ap:      The 2-bit simple AP (AP[2:1])
> + * @is_user: TRUE if accessing from PL0
>   */
> -static inline int
> -simple_ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, int ap)
> +static inline int simple_ap_to_rw_prot_is_user(int ap, bool is_user)
>  {
> -    bool is_user = regime_is_user(env, mmu_idx);
> -
>      switch (ap) {
>      case 0:
>          return is_user ? 0 : PAGE_READ | PAGE_WRITE;
> @@ -4985,6 +4981,94 @@ simple_ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, int ap)
>      }
>  }
>
> +static inline int
> +simple_ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, int ap)
> +{
> +    return simple_ap_to_rw_prot_is_user(ap, regime_is_user(env, mmu_idx));
> +}
> +
> +/* Translate section/page access permissions to protection flags
> + *
> + * @env:     CPUARMState
> + * @mmu_idx: MMU index indicating required translation regime
> + * @is_aa64: TRUE if AArch64
> + * @ap:      The 2-bit simple AP (AP[2:1])
> + * @ns:      NS (non-secure) bit
> + * @xn:      XN (execute-never) bit
> + * @pxn:     PXN (privileged execute-never) bit
> + */
> +static int get_S1prot(CPUARMState *env, ARMMMUIdx mmu_idx, bool is_aa64,
> +                      int ap, int ns, int xn, int pxn)
> +{
> +    bool is_user = regime_is_user(env, mmu_idx);
> +    int prot_rw, user_rw;
> +    bool have_wxn;
> +    int wxn = 0;
> +
> +    assert(mmu_idx != ARMMMUIdx_S2NS);
> +
> +    user_rw = simple_ap_to_rw_prot_is_user(ap, true);
> +    if (is_user) {
> +        prot_rw = user_rw;
> +    } else {
> +        prot_rw = simple_ap_to_rw_prot_is_user(ap, false);
> +    }
> +
> +    if (ns && arm_is_secure(env) && (env->cp15.scr_el3 & SCR_SIF)) {
> +        return prot_rw;
> +    }
> +
> +    /* 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) {
> +        switch (regime_el(env, mmu_idx)) {
> +        case 1:
> +            if (is_user && !user_rw) {
> +                wxn = 0;

I don't understand this. We ignore the WXN bit if this is
a user access and the page is not readable ?

I also find the naming of this variable "user_rw" (and
to a lesser extent "prot_rw") very confusing. I keep
misreading "if (user_rw)" as meaning "if this page is
read-write for the user", when in fact it only means
"if this page is readable for the user".

Maybe it would be less confusing if we always did tests
against a set of PAGE_* flags rather than doing an
is/is-not-zero test?

The rest looked OK to me.

-- PMM

Re: [Qemu-devel] [PATCH v2 3/3] target-arm: get_phys_addr_lpae: more xn control

[Andrew Jones]

On Wed, Mar 11, 2015 at 05:02:00PM +0000, Peter Maydell wrote:
> On 10 March 2015 at 21:06, Andrew Jones <drjones@redhat.com> wrote:
> > This patch makes the following changes to the determination of
> > whether an address is executable, when translating addresses
> > using LPAE.
> >
> > 1. No longer assumes that PL0 can't execute when it can't read.
> >    It can in AArch64, a difference from AArch32.
> > 2. Use va_size == 64 to determine we're in AArch64, rather than
> >    arm_feature(env, ARM_FEATURE_V8), which is insufficient.
> > 3. Add additional XN determinants
> >    - NS && is_secure && (SCR & SCR_SIF)
> >    - WXN && (prot & PAGE_WRITE)
> >    - AArch64: (prot_PL0 & PAGE_WRITE)
> >    - AArch32: UWXN && (prot_PL0 & PAGE_WRITE)
> >    - XN determination should also work in secure mode (untested)
> >    - XN may even work in EL2 (currently impossible to test)
> > 4. Cleans up the bloated PAGE_EXEC condition - by removing it.
> >
> > The helper get_S1prot is introduced. It may even work in EL2,
> > when support for that comes, but, as the function name implies,
> > it only works for stage 1 translations.
> >
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> 
> I like the general shape of this patch. Minor comment below:
> 
> > ---
> >  target-arm/helper.c | 129 ++++++++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 100 insertions(+), 29 deletions(-)
> >
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index d996659652f8d..c457e9ab8c85a 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -4962,15 +4962,11 @@ static inline int ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
> >  /* Translate section/page access permissions to page
> >   * R/W protection flags.
> >   *
> > - * @env:     CPUARMState
> > - * @mmu_idx: MMU index indicating required translation regime
> >   * @ap:      The 2-bit simple AP (AP[2:1])
> > + * @is_user: TRUE if accessing from PL0
> >   */
> > -static inline int
> > -simple_ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, int ap)
> > +static inline int simple_ap_to_rw_prot_is_user(int ap, bool is_user)
> >  {
> > -    bool is_user = regime_is_user(env, mmu_idx);
> > -
> >      switch (ap) {
> >      case 0:
> >          return is_user ? 0 : PAGE_READ | PAGE_WRITE;
> > @@ -4985,6 +4981,94 @@ simple_ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, int ap)
> >      }
> >  }
> >
> > +static inline int
> > +simple_ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, int ap)
> > +{
> > +    return simple_ap_to_rw_prot_is_user(ap, regime_is_user(env, mmu_idx));
> > +}
> > +
> > +/* Translate section/page access permissions to protection flags
> > + *
> > + * @env:     CPUARMState
> > + * @mmu_idx: MMU index indicating required translation regime
> > + * @is_aa64: TRUE if AArch64
> > + * @ap:      The 2-bit simple AP (AP[2:1])
> > + * @ns:      NS (non-secure) bit
> > + * @xn:      XN (execute-never) bit
> > + * @pxn:     PXN (privileged execute-never) bit
> > + */
> > +static int get_S1prot(CPUARMState *env, ARMMMUIdx mmu_idx, bool is_aa64,
> > +                      int ap, int ns, int xn, int pxn)
> > +{
> > +    bool is_user = regime_is_user(env, mmu_idx);
> > +    int prot_rw, user_rw;
> > +    bool have_wxn;
> > +    int wxn = 0;
> > +
> > +    assert(mmu_idx != ARMMMUIdx_S2NS);
> > +
> > +    user_rw = simple_ap_to_rw_prot_is_user(ap, true);
> > +    if (is_user) {
> > +        prot_rw = user_rw;
> > +    } else {
> > +        prot_rw = simple_ap_to_rw_prot_is_user(ap, false);
> > +    }
> > +
> > +    if (ns && arm_is_secure(env) && (env->cp15.scr_el3 & SCR_SIF)) {
> > +        return prot_rw;
> > +    }
> > +
> > +    /* 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) {
> > +        switch (regime_el(env, mmu_idx)) {
> > +        case 1:
> > +            if (is_user && !user_rw) {
> > +                wxn = 0;
> 
> I don't understand this. We ignore the WXN bit if this is
> a user access and the page is not readable ?

Yup. If the page is not readable or writeable, AP[1]=0. I almost
submitted an errata to the ARM ARM when I saw this on the 2nd line
of table D4-32. I thought it must be a typo. However I tested it
on hardware, and it works this way. So at least the weirdness has
been implemented consistently...

> 
> I also find the naming of this variable "user_rw" (and
> to a lesser extent "prot_rw") very confusing. I keep
> misreading "if (user_rw)" as meaning "if this page is
> read-write for the user", when in fact it only means
> "if this page is readable for the user".

Right, user_rw really means ("user prot" & (PAGE_READ | PAGE_WRITE)),
but as you can't have PAGE_WRITE without PAGE_READ, then when user_rw
is non-zero we know it means PAGE_READ is set, but we still have to
check for PAGE_WRITE explicitly when we care about it.

> 
> Maybe it would be less confusing if we always did tests
> against a set of PAGE_* flags rather than doing an
> is/is-not-zero test?

Sounds good. Will respin with this change, and also adding that hunk
I forgot that collects NSTable bits.
> 
> The rest looked OK to me.

Thanks!

drew

Re: [Qemu-devel] [PATCH v2 3/3] target-arm: get_phys_addr_lpae: more xn control

[Peter Maydell]

On 11 March 2015 at 17:42, Andrew Jones <drjones@redhat.com> wrote:
> On Wed, Mar 11, 2015 at 05:02:00PM +0000, Peter Maydell wrote:

>> > +    if (is_aa64) {
>> > +        switch (regime_el(env, mmu_idx)) {
>> > +        case 1:
>> > +            if (is_user && !user_rw) {
>> > +                wxn = 0;
>>
>> I don't understand this. We ignore the WXN bit if this is
>> a user access and the page is not readable ?
>
> Yup. If the page is not readable or writeable, AP[1]=0. I almost
> submitted an errata to the ARM ARM when I saw this on the 2nd line
> of table D4-32. I thought it must be a typo. However I tested it
> on hardware, and it works this way. So at least the weirdness has
> been implemented consistently...

Still confused. If the page isn't readable or writable
then WXN isn't going to kick in anyway because WXN only
affects writable pages. I don't see what the case is
where this bit of code will make a difference.

-- PMM

Re: [Qemu-devel] [PATCH v2 3/3] target-arm: get_phys_addr_lpae: more xn control

[Andrew Jones]

On Wed, Mar 11, 2015 at 05:49:39PM +0000, Peter Maydell wrote:
> On 11 March 2015 at 17:42, Andrew Jones <drjones@redhat.com> wrote:
> > On Wed, Mar 11, 2015 at 05:02:00PM +0000, Peter Maydell wrote:
> 
> >> > +    if (is_aa64) {
> >> > +        switch (regime_el(env, mmu_idx)) {
> >> > +        case 1:
> >> > +            if (is_user && !user_rw) {
> >> > +                wxn = 0;
> >>
> >> I don't understand this. We ignore the WXN bit if this is
> >> a user access and the page is not readable ?
> >
> > Yup. If the page is not readable or writeable, AP[1]=0. I almost
> > submitted an errata to the ARM ARM when I saw this on the 2nd line
> > of table D4-32. I thought it must be a typo. However I tested it
> > on hardware, and it works this way. So at least the weirdness has
> > been implemented consistently...
> 
> Still confused. If the page isn't readable or writable
> then WXN isn't going to kick in anyway because WXN only
> affects writable pages. I don't see what the case is
> where this bit of code will make a difference.
>

Ah, that is true. Too bad I didn't read this before sending v3,
as I could have removed it, if you prefer. I had it here to
be explicit about the ignoring of wxn - matching the spec, but
you're right, it's useless code. Should I send a v4?

drew

Re: [Qemu-devel] [PATCH v2 3/3] target-arm: get_phys_addr_lpae: more xn control

[Peter Maydell]

On 11 March 2015 at 18:10, Andrew Jones <drjones@redhat.com> wrote:
> On Wed, Mar 11, 2015 at 05:49:39PM +0000, Peter Maydell wrote:
>> Still confused. If the page isn't readable or writable
>> then WXN isn't going to kick in anyway because WXN only
>> affects writable pages. I don't see what the case is
>> where this bit of code will make a difference.
>>
>
> Ah, that is true. Too bad I didn't read this before sending v3,
> as I could have removed it, if you prefer. I had it here to
> be explicit about the ignoring of wxn - matching the spec, but
> you're right, it's useless code. Should I send a v4?

Yes, please send a v4.

I don't see what you mean about matching the spec, though.
The spec doesn't say anything about "ignore WXN if the
page isn't readable". It just straightforwardly says "if
the WXN bit is set then writable regions are treated as
XN", which is exactly what the code at the bottom of your
function does.

-- PMM

Re: [Qemu-devel] [PATCH v2 3/3] target-arm: get_phys_addr_lpae: more xn control

[Andrew Jones]

On Wed, Mar 11, 2015 at 06:15:47PM +0000, Peter Maydell wrote:
> On 11 March 2015 at 18:10, Andrew Jones <drjones@redhat.com> wrote:
> > On Wed, Mar 11, 2015 at 05:49:39PM +0000, Peter Maydell wrote:
> >> Still confused. If the page isn't readable or writable
> >> then WXN isn't going to kick in anyway because WXN only
> >> affects writable pages. I don't see what the case is
> >> where this bit of code will make a difference.
> >>
> >
> > Ah, that is true. Too bad I didn't read this before sending v3,
> > as I could have removed it, if you prefer. I had it here to
> > be explicit about the ignoring of wxn - matching the spec, but
> > you're right, it's useless code. Should I send a v4?
> 
> Yes, please send a v4.
> 
> I don't see what you mean about matching the spec, though.
> The spec doesn't say anything about "ignore WXN if the
> page isn't readable". It just straightforwardly says "if
> the WXN bit is set then writable regions are treated as
> XN", which is exactly what the code at the bottom of your
> function does.
>

My interpretation of SCTLR_EL1.WXN was just wrong. There is
talks about "EL1&0", and I assumed it meant that when WXN is
on, then both EL1 and EL0 should lose executability. However
it can certainly be interpreted as applying to them both, but
based on their respective access permissions, which is how
I guess I should have interpreted it.

v4 coming

drew 

Re: [Qemu-devel] [PATCH v2 3/3] target-arm: get_phys_addr_lpae: more xn control

[Peter Maydell]

On 11 March 2015 at 18:30, Andrew Jones <drjones@redhat.com> wrote:
> My interpretation of SCTLR_EL1.WXN was just wrong. There is
> talks about "EL1&0"

That's just the name of the translation regime (ie
there's a shared set of page tables that handle
translation for both EL1 and EL0).

> and I assumed it meant that when WXN is
> on, then both EL1 and EL0 should lose executability.

No; and you can see an example of this in the table:
UXN=0, PXN=0, AP[2:1] = 00, WXN=0 is RWX for EL1 and
X for EL0. Setting WXN changes the EL1 rights to just
RW (because the page is writable there) but leaves the
EL0 rights as X (because there the page is not writable).

> However
> it can certainly be interpreted as applying to them both, but
> based on their respective access permissions, which is how
> I guess I should have interpreted it.

Yes, this is the correct interpretation.

-- PMM