[Qemu-devel] [PATCH 0/2] tcg-arm: fix and extend instruction execution control

[Qemu-devel] [PATCH 0/2] tcg-arm: fix and extend instruction execution control

[Andrew Jones]

We're currently assuming EL1 can execute code it shouldn't, and that
EL0 shouldn't execute code it can. Fix those cases, and also extend
instruction execution control to handle WXN and more.

The first patch addresses EL0 faulting when it should be allowed to
execute. The second patch addresses EL1 not faulting when it should,
as well as adds in additional execution control.

Andrew Jones (2):
  tcg-aarch64: user doesn't need R/W access to exec
  tcg-arm: more instruction execution control

 target-arm/helper.c | 103 ++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 80 insertions(+), 23 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 1/2] tcg-aarch64: user doesn't need R/W access to exec

[Andrew Jones]

Table D4-32 shows that execute access from EL0 doesn't depend
on AP[1].

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

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 3ef0f1f38eda5..7c30a2669a0f2 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -4787,7 +4787,7 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
     hwaddr descaddr, descmask;
     uint32_t tableattrs;
     target_ulong page_size;
-    uint32_t attrs;
+    uint32_t attrs, ap;
     int32_t granule_sz = 9;
     int32_t va_size = 32;
     int32_t tbi = 0;
@@ -4952,14 +4952,20 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
         /* Access flag */
         goto do_fault;
     }
+
     fault_type = permission_fault;
-    if (is_user && !(attrs & (1 << 4))) {
-        /* Unprivileged access not enabled */
-        goto do_fault;
+    ap = extract32(attrs, 4, 2); /* AP[2:1] */
+
+    *prot = 0;
+    if (!is_user || (ap & 1)) {
+        *prot |= PAGE_READ;
+        *prot |= !(ap & 2) ? PAGE_WRITE : 0;
     }
-    *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
+
+    *prot |= PAGE_EXEC;
     if ((arm_feature(env, ARM_FEATURE_V8) && is_user && (attrs & (1 << 12))) ||
         (!arm_feature(env, ARM_FEATURE_V8) && (attrs & (1 << 12))) ||
+        (!arm_el_is_aa64(env, 1) && is_user && !(ap & 1)) ||
         (!is_user && (attrs & (1 << 11)))) {
         /* XN/UXN or PXN. Since we only implement EL0/EL1 we unconditionally
          * treat XN/UXN as UXN for v8.
@@ -4969,12 +4975,11 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
         }
         *prot &= ~PAGE_EXEC;
     }
-    if (attrs & (1 << 5)) {
-        /* Write access forbidden */
-        if (access_type == 1) {
-            goto do_fault;
-        }
-        *prot &= ~PAGE_WRITE;
+
+    if ((*prot == 0)
+            || (!(*prot & PAGE_WRITE) && access_type == 1)
+            || (!(*prot & PAGE_EXEC) && access_type == 2)) {
+        goto do_fault;
     }
 
     *phys_ptr = descaddr;
-- 
1.9.3

[Qemu-devel] [PATCH 2/2] tcg-arm: more instruction execution control

[Andrew Jones]

Cleanup XN/PXN handling in get_phys_addr_lpae, and implement all but
EL2 support of the following ARMv8 sections

  D4.5.1 Memory access control: Access permissions for instruction
         execution
  G4.7.2 Execute-never restrictions on instruction fetching

G4.7.2 matches the ARMv7 section B3.7.2 when long-descriptors are used.

Signed-off-by: Andrew Jones <drjones@redhat.com>

---
v3:
  - correct logic for (is_user && !(ap & 1)) case
  - base on "user doesn't need R/W access to exec" patch
v2: correct assert in el2 case

I didn't test this with secure mode (I didn't even check if that's
currently possible), but I did test all other EL1&0 XN controls with
both tcg-arm (cortex-a15) and tcg-aarch64 (cortex-a57) by hacking up
some tests with kvm-unit-tests/arm[64]. I also booted Linux (just
up to looking for a rootfs) with both.

 target-arm/helper.c | 80 +++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 66 insertions(+), 14 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 7c30a2669a0f2..715e0f47bec7d 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -4516,6 +4516,69 @@ static inline int check_ap(CPUARMState *env, int ap, int domain_prot,
   }
 }
 
+/* Check section/page execution permission.
+ *
+ * Returns PAGE_EXEC if execution is permitted, otherwise zero.
+ *
+ * Supports AArch64, AArch32, and v7-LPAE. Doesn't yet support
+ * EL2. Actually, for v7-LPAE, we'd also need to emulate Virt
+ * Extensions to truly have WXN/UWXN. We don't support checking
+ * for that yet, so we just assume we have them.
+ *
+ * @env         : CPUARMState
+ * @is_user     : 0 for privileged access, 1 for user
+ * @ap          : Access permissions (AP[2:1]) from descriptor
+ * @ns          : (NSTable || NS) from descriptors
+ * @xn          : ([U]XNTable || [U]XN) from descriptors
+ * @pxn         : (PXNTable || PXN) from descriptors
+ */
+static int check_xn_lpae(CPUARMState *env, int is_user, int ap,
+                         int ns, int xn, int pxn)
+{
+    int wxn;
+
+    switch (arm_current_el(env)) {
+    case 0:
+    case 1:
+        wxn = env->cp15.sctlr_el[1] & SCTLR_WXN;
+        if (arm_el_is_aa64(env, 1)) {
+            if (is_user && !(ap & 1)) {
+                wxn = 0;
+            }
+            pxn = pxn || ((ap & 1) && !(ap & 2));
+            xn = is_user ? xn : pxn;
+        } else {
+            int uwxn = env->cp15.sctlr_el[1] & SCTLR_UWXN;
+            pxn = pxn || (uwxn && (ap & 1) && !(ap & 2));
+            xn = xn || (!is_user && pxn) || (is_user && !(ap & 1));
+        }
+        if (arm_is_secure(env)) {
+            xn = xn || (ns && (env->cp15.scr_el3 & SCR_SIF));
+        }
+        break;
+    case 2:
+        /* TODO actually support EL2 */
+        assert(false);
+
+        if (arm_el_is_aa64(env, 2)) {
+            wxn = env->cp15.sctlr_el[2] & SCTLR_WXN;
+        } else {
+            /* wxn = HSCTLR.WXN */
+        }
+        break;
+    case 3:
+        if (arm_el_is_aa64(env, 3)) {
+            wxn = env->cp15.sctlr_el[3] & SCTLR_WXN;
+        } else {
+            wxn = 0;
+        }
+        xn = xn || (ns && (env->cp15.scr_el3 & SCR_SIF));
+        break;
+    }
+
+    return xn || (!(ap & 2) && wxn) ? 0 : PAGE_EXEC;
+}
+
 static bool get_level1_table_address(CPUARMState *env, uint32_t *table,
                                          uint32_t address)
 {
@@ -4941,7 +5004,7 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
         if (extract32(tableattrs, 2, 1)) {
             attrs &= ~(1 << 4);
         }
-        /* Since we're always in the Non-secure state, NSTable is ignored. */
+        attrs |= extract32(tableattrs, 4, 1) << 3; /* NS */
         break;
     }
     /* Here descaddr is the final physical address, and attributes
@@ -4962,19 +5025,8 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
         *prot |= !(ap & 2) ? PAGE_WRITE : 0;
     }
 
-    *prot |= PAGE_EXEC;
-    if ((arm_feature(env, ARM_FEATURE_V8) && is_user && (attrs & (1 << 12))) ||
-        (!arm_feature(env, ARM_FEATURE_V8) && (attrs & (1 << 12))) ||
-        (!arm_el_is_aa64(env, 1) && is_user && !(ap & 1)) ||
-        (!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;
-    }
+    *prot |= check_xn_lpae(env, is_user, ap, extract32(attrs, 3, 1),
+                           extract32(attrs, 12, 1), extract32(attrs, 11, 1));
 
     if ((*prot == 0)
             || (!(*prot & PAGE_WRITE) && access_type == 1)
-- 
1.9.3

Re: [Qemu-devel] [PATCH 1/2] tcg-aarch64: user doesn't need R/W access to exec

[Peter Maydell]

On 13 January 2015 at 15:48, Andrew Jones <drjones@redhat.com> wrote:
> Table D4-32 shows that execute access from EL0 doesn't depend
> on AP[1].

This commit message is a bit sparse, which confused me
for a bit. It would be worth beefing it up a bit:

target-arm: 64-bit EL0 code can execute from unreadable pages

In AArch64 mode, a page can be executable even if it is not
readable (a difference from AArch32). Instead of bailing out
early if the page is not readable, just add "32 bit and
page not readable" to the list of conditions that make a
page non-executable, and check whether the protections and
the access type are compatible once at the end of the function.

> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  target-arm/helper.c | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 3ef0f1f38eda5..7c30a2669a0f2 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4787,7 +4787,7 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>      hwaddr descaddr, descmask;
>      uint32_t tableattrs;
>      target_ulong page_size;
> -    uint32_t attrs;
> +    uint32_t attrs, ap;
>      int32_t granule_sz = 9;
>      int32_t va_size = 32;
>      int32_t tbi = 0;
> @@ -4952,14 +4952,20 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>          /* Access flag */
>          goto do_fault;
>      }
> +
>      fault_type = permission_fault;
> -    if (is_user && !(attrs & (1 << 4))) {
> -        /* Unprivileged access not enabled */
> -        goto do_fault;
> +    ap = extract32(attrs, 4, 2); /* AP[2:1] */
> +
> +    *prot = 0;
> +    if (!is_user || (ap & 1)) {
> +        *prot |= PAGE_READ;
> +        *prot |= !(ap & 2) ? PAGE_WRITE : 0;

Personally I would find
     if (!(ap & 2)) {
         *prot |= PAGE_WRITE;
     }

clearer.

>      }
> -    *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> +
> +    *prot |= PAGE_EXEC;
>      if ((arm_feature(env, ARM_FEATURE_V8) && is_user && (attrs & (1 << 12))) ||
>          (!arm_feature(env, ARM_FEATURE_V8) && (attrs & (1 << 12))) ||
> +        (!arm_el_is_aa64(env, 1) && is_user && !(ap & 1)) ||
>          (!is_user && (attrs & (1 << 11)))) {
>          /* XN/UXN or PXN. Since we only implement EL0/EL1 we unconditionally
>           * treat XN/UXN as UXN for v8.
> @@ -4969,12 +4975,11 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>          }

There is a "if access_type == 2 goto do_fault" check just
above this hunk which you can delete, because we're now
doing that check in the code you add below.

>          *prot &= ~PAGE_EXEC;
>      }
> -    if (attrs & (1 << 5)) {
> -        /* Write access forbidden */
> -        if (access_type == 1) {
> -            goto do_fault;
> -        }
> -        *prot &= ~PAGE_WRITE;
> +
> +    if ((*prot == 0)
> +            || (!(*prot & PAGE_WRITE) && access_type == 1)
> +            || (!(*prot & PAGE_EXEC) && access_type == 2)) {
> +        goto do_fault;

Why isn't this just
    if (!(*prot & (1 << access_type))) {

? (Or at least, why doesn't it treat PAGE_READ the same way
as the other two bits?) As it is I think we'll treat a page
that is marked exec-not-readable as if it were readable.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/2] tcg-arm: more instruction execution control

[Peter Maydell]

On 13 January 2015 at 15:48, Andrew Jones <drjones@redhat.com> wrote:
> Cleanup XN/PXN handling in get_phys_addr_lpae, and implement all but
> EL2 support of the following ARMv8 sections
>
>   D4.5.1 Memory access control: Access permissions for instruction
>          execution
>   G4.7.2 Execute-never restrictions on instruction fetching
>
> G4.7.2 matches the ARMv7 section B3.7.2 when long-descriptors are used.
>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
>
> ---
> v3:
>   - correct logic for (is_user && !(ap & 1)) case
>   - base on "user doesn't need R/W access to exec" patch
> v2: correct assert in el2 case
>
> I didn't test this with secure mode (I didn't even check if that's
> currently possible), but I did test all other EL1&0 XN controls with
> both tcg-arm (cortex-a15) and tcg-aarch64 (cortex-a57) by hacking up
> some tests with kvm-unit-tests/arm[64]. I also booted Linux (just
> up to looking for a rootfs) with both.
>
>  target-arm/helper.c | 80 +++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 66 insertions(+), 14 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 7c30a2669a0f2..715e0f47bec7d 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4516,6 +4516,69 @@ static inline int check_ap(CPUARMState *env, int ap, int domain_prot,
>    }
>  }
>
> +/* Check section/page execution permission.
> + *
> + * Returns PAGE_EXEC if execution is permitted, otherwise zero.
> + *
> + * Supports AArch64, AArch32, and v7-LPAE. Doesn't yet support
> + * EL2. Actually, for v7-LPAE, we'd also need to emulate Virt
> + * Extensions to truly have WXN/UWXN. We don't support checking
> + * for that yet, so we just assume we have them.

If you want to know if the Virtualization Extensions are
present, check feature bit ARM_FEATURE_EL2.

In general this code seems to be rather short on feature
bit checks -- notice that the code it is replacing was
doing checks on the V8 feature bit, for instance.

> + *
> + * @env         : CPUARMState
> + * @is_user     : 0 for privileged access, 1 for user
> + * @ap          : Access permissions (AP[2:1]) from descriptor
> + * @ns          : (NSTable || NS) from descriptors
> + * @xn          : ([U]XNTable || [U]XN) from descriptors
> + * @pxn         : (PXNTable || PXN) from descriptors
> + */
> +static int check_xn_lpae(CPUARMState *env, int is_user, int ap,
> +                         int ns, int xn, int pxn)
> +{
> +    int wxn;
> +
> +    switch (arm_current_el(env)) {
> +    case 0:

If arm_current_el() is 0 but EL3 is 32 bit then the
controlling environment here is the Secure PL1, which
is EL3, and falling through to look at sctlr_el[1] is
wrong. (SCTLR(S) is sctlr_el[3].)

(Also get_phys_addr() can be called via the ats_write()
functions, which means that "arm_current_el()" and "the
address translation regime we're trying to determine the
answer for" aren't necessarily the same...)

I think we're very soon going to need to bite the bullet and
make this code have a concept of the current "translation
regime", as the ARM ARM terms it...

If you wish to avoid that (and I wouldn't object, given
this patchset is snowballing in scope already) then do what
get_phys_addr() does to get the relevant SCTLR:
    uint32_t sctlr = A32_BANKED_CURRENT_REG_GET(env, sctlr);
This will work for everything we currently support:
 * AArch64 EL1 (with an EL0 either AArch32 or AArch64)
 * AArch32 EL1
 * TZ for the limited case of EL3 == AArch32 (and so no EL1)

> +    case 1:
> +        wxn = env->cp15.sctlr_el[1] & SCTLR_WXN;
> +        if (arm_el_is_aa64(env, 1)) {
> +            if (is_user && !(ap & 1)) {
> +                wxn = 0;
> +            }

I don't understand what we're doing here: as far as I can see
the ARM ARM simply defines that WXN is the bit from the
relevant SCTLR, and it always applies if the page is writable.
I have a feeling you're re-calculating half of "is this
page writable" here, and then doing the other half below.
It would be cleaner to pass in the 'prot' bits that have
already been calculated.

> +            pxn = pxn || ((ap & 1) && !(ap & 2));

Very weird way to write "ap == 1" ?

> +            xn = is_user ? xn : pxn;
> +        } else {
> +            int uwxn = env->cp15.sctlr_el[1] & SCTLR_UWXN;
> +            pxn = pxn || (uwxn && (ap & 1) && !(ap & 2));
> +            xn = xn || (!is_user && pxn) || (is_user && !(ap & 1));
> +        }
> +        if (arm_is_secure(env)) {
> +            xn = xn || (ns && (env->cp15.scr_el3 & SCR_SIF));
> +        }
> +        break;
> +    case 2:
> +        /* TODO actually support EL2 */
> +        assert(false);
> +
> +        if (arm_el_is_aa64(env, 2)) {
> +            wxn = env->cp15.sctlr_el[2] & SCTLR_WXN;
> +        } else {
> +            /* wxn = HSCTLR.WXN */
> +        }

You can just write this as
    wxn = env->cp15.sctlr_el[2] & SCTLR_WXN;

because SCTLR_EL2 and HSCTLR are architecturally mapped and so
we will store the state in the same env->cp15 field whether
EL2 is 32 or 64 bit.

> +        break;
> +    case 3:
> +        if (arm_el_is_aa64(env, 3)) {
> +            wxn = env->cp15.sctlr_el[3] & SCTLR_WXN;
> +        } else {
> +            wxn = 0;

This should also be checking sctlr_el[3]'s WXN bit.
(That's SCTLR(S) in AArch32 terms.)

You're also missing the handling of SCTLR.UWXN here.
(Remember that if EL3 is AArch32 then all the traditional
"operating system" PL1 privilege level code runs at EL3,
not EL1.)

> +        }
> +        xn = xn || (ns && (env->cp15.scr_el3 & SCR_SIF));
> +        break;
> +    }
> +
> +    return xn || (!(ap & 2) && wxn) ? 0 : PAGE_EXEC;

I think this is the other half of "recalculate whether
the page is writable". Something more like this would be clearer:

  if (xn || (wxn && (prot & PAGE_WRITE))) {
      return 0;
  }
  return PAGE_EXEC;


> +}
> +
>  static bool get_level1_table_address(CPUARMState *env, uint32_t *table,
>                                           uint32_t address)
>  {
> @@ -4941,7 +5004,7 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>          if (extract32(tableattrs, 2, 1)) {
>              attrs &= ~(1 << 4);
>          }
> -        /* Since we're always in the Non-secure state, NSTable is ignored. */
> +        attrs |= extract32(tableattrs, 4, 1) << 3; /* NS */
>          break;
>      }
>      /* Here descaddr is the final physical address, and attributes
> @@ -4962,19 +5025,8 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>          *prot |= !(ap & 2) ? PAGE_WRITE : 0;
>      }
>
> -    *prot |= PAGE_EXEC;
> -    if ((arm_feature(env, ARM_FEATURE_V8) && is_user && (attrs & (1 << 12))) ||
> -        (!arm_feature(env, ARM_FEATURE_V8) && (attrs & (1 << 12))) ||
> -        (!arm_el_is_aa64(env, 1) && is_user && !(ap & 1)) ||
> -        (!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;
> -    }
> +    *prot |= check_xn_lpae(env, is_user, ap, extract32(attrs, 3, 1),
> +                           extract32(attrs, 12, 1), extract32(attrs, 11, 1));
>
>      if ((*prot == 0)
>              || (!(*prot & PAGE_WRITE) && access_type == 1)
> --
> 1.9.3
>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/2] tcg-arm: more instruction execution control

[Peter Maydell]

On 16 January 2015 at 16:16, Peter Maydell <peter.maydell@linaro.org> wrote:
> I think we're very soon going to need to bite the bullet and
> make this code have a concept of the current "translation
> regime", as the ARM ARM terms it...

In fact I think we need to do it now. I'll try to
write some code to at least get a conceptual framework
in place and send it out next week sometime.

-- PMM

Re: [Qemu-devel] [PATCH 1/2] tcg-aarch64: user doesn't need R/W access to exec

[Andrew Jones]

On Fri, Jan 16, 2015 at 02:52:21PM +0000, Peter Maydell wrote:
> On 13 January 2015 at 15:48, Andrew Jones <drjones@redhat.com> wrote:
> > Table D4-32 shows that execute access from EL0 doesn't depend
> > on AP[1].
> 
> This commit message is a bit sparse, which confused me
> for a bit. It would be worth beefing it up a bit:
> 
> target-arm: 64-bit EL0 code can execute from unreadable pages
> 
> In AArch64 mode, a page can be executable even if it is not
> readable (a difference from AArch32). Instead of bailing out
> early if the page is not readable, just add "32 bit and
> page not readable" to the list of conditions that make a
> page non-executable, and check whether the protections and
> the access type are compatible once at the end of the function.

OK

> 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  target-arm/helper.c | 27 ++++++++++++++++-----------
> >  1 file changed, 16 insertions(+), 11 deletions(-)
> >
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index 3ef0f1f38eda5..7c30a2669a0f2 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -4787,7 +4787,7 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
> >      hwaddr descaddr, descmask;
> >      uint32_t tableattrs;
> >      target_ulong page_size;
> > -    uint32_t attrs;
> > +    uint32_t attrs, ap;
> >      int32_t granule_sz = 9;
> >      int32_t va_size = 32;
> >      int32_t tbi = 0;
> > @@ -4952,14 +4952,20 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
> >          /* Access flag */
> >          goto do_fault;
> >      }
> > +
> >      fault_type = permission_fault;
> > -    if (is_user && !(attrs & (1 << 4))) {
> > -        /* Unprivileged access not enabled */
> > -        goto do_fault;
> > +    ap = extract32(attrs, 4, 2); /* AP[2:1] */
> > +
> > +    *prot = 0;
> > +    if (!is_user || (ap & 1)) {
> > +        *prot |= PAGE_READ;
> > +        *prot |= !(ap & 2) ? PAGE_WRITE : 0;
> 
> Personally I would find
>      if (!(ap & 2)) {
>          *prot |= PAGE_WRITE;
>      }
> 
> clearer.

OK

> 
> >      }
> > -    *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> > +
> > +    *prot |= PAGE_EXEC;
> >      if ((arm_feature(env, ARM_FEATURE_V8) && is_user && (attrs & (1 << 12))) ||
> >          (!arm_feature(env, ARM_FEATURE_V8) && (attrs & (1 << 12))) ||
> > +        (!arm_el_is_aa64(env, 1) && is_user && !(ap & 1)) ||
> >          (!is_user && (attrs & (1 << 11)))) {
> >          /* XN/UXN or PXN. Since we only implement EL0/EL1 we unconditionally
> >           * treat XN/UXN as UXN for v8.
> > @@ -4969,12 +4975,11 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
> >          }
> 
> There is a "if access_type == 2 goto do_fault" check just
> above this hunk which you can delete, because we're now
> doing that check in the code you add below.

Right. Will do, alternatively I should have brought the PAGE_EXEC
handling below in with patch 2/2, which was my plan, but forgot
to split it out.

> 
> >          *prot &= ~PAGE_EXEC;
> >      }
> > -    if (attrs & (1 << 5)) {
> > -        /* Write access forbidden */
> > -        if (access_type == 1) {
> > -            goto do_fault;
> > -        }
> > -        *prot &= ~PAGE_WRITE;
> > +
> > +    if ((*prot == 0)
> > +            || (!(*prot & PAGE_WRITE) && access_type == 1)
> > +            || (!(*prot & PAGE_EXEC) && access_type == 2)) {
> > +        goto do_fault;
> 
> Why isn't this just
>     if (!(*prot & (1 << access_type))) {

yeah, that would be better

> 
> ? (Or at least, why doesn't it treat PAGE_READ the same way
> as the other two bits?) As it is I think we'll treat a page
> that is marked exec-not-readable as if it were readable.

Oh yes, we should check PAGE_READ as well

Thanks for the review. I see from another mail that you'll be sending some
patches I should base the next version on. So I'll hold off on sending a
revised patch until I see that.

drew

Re: [Qemu-devel] [PATCH 2/2] tcg-arm: more instruction execution control

[Andrew Jones]

On Fri, Jan 16, 2015 at 04:16:02PM +0000, Peter Maydell wrote:
> On 13 January 2015 at 15:48, Andrew Jones <drjones@redhat.com> wrote:
> > Cleanup XN/PXN handling in get_phys_addr_lpae, and implement all but
> > EL2 support of the following ARMv8 sections
> >
> >   D4.5.1 Memory access control: Access permissions for instruction
> >          execution
> >   G4.7.2 Execute-never restrictions on instruction fetching
> >
> > G4.7.2 matches the ARMv7 section B3.7.2 when long-descriptors are used.
> >
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> >
> > ---
> > v3:
> >   - correct logic for (is_user && !(ap & 1)) case
> >   - base on "user doesn't need R/W access to exec" patch
> > v2: correct assert in el2 case
> >
> > I didn't test this with secure mode (I didn't even check if that's
> > currently possible), but I did test all other EL1&0 XN controls with
> > both tcg-arm (cortex-a15) and tcg-aarch64 (cortex-a57) by hacking up
> > some tests with kvm-unit-tests/arm[64]. I also booted Linux (just
> > up to looking for a rootfs) with both.
> >
> >  target-arm/helper.c | 80 +++++++++++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 66 insertions(+), 14 deletions(-)
> >
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index 7c30a2669a0f2..715e0f47bec7d 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -4516,6 +4516,69 @@ static inline int check_ap(CPUARMState *env, int ap, int domain_prot,
> >    }
> >  }
> >
> > +/* Check section/page execution permission.
> > + *
> > + * Returns PAGE_EXEC if execution is permitted, otherwise zero.
> > + *
> > + * Supports AArch64, AArch32, and v7-LPAE. Doesn't yet support
> > + * EL2. Actually, for v7-LPAE, we'd also need to emulate Virt
> > + * Extensions to truly have WXN/UWXN. We don't support checking
> > + * for that yet, so we just assume we have them.
> 
> If you want to know if the Virtualization Extensions are
> present, check feature bit ARM_FEATURE_EL2.

OK
> 
> In general this code seems to be rather short on feature
> bit checks -- notice that the code it is replacing was
> doing checks on the V8 feature bit, for instance.

The code it's replacing wasn't using the V8 feature bit correctly.
It was using it to determine AArch64 vs. AArch32, not just V8 vs.
V7.

> 
> > + *
> > + * @env         : CPUARMState
> > + * @is_user     : 0 for privileged access, 1 for user
> > + * @ap          : Access permissions (AP[2:1]) from descriptor
> > + * @ns          : (NSTable || NS) from descriptors
> > + * @xn          : ([U]XNTable || [U]XN) from descriptors
> > + * @pxn         : (PXNTable || PXN) from descriptors
> > + */
> > +static int check_xn_lpae(CPUARMState *env, int is_user, int ap,
> > +                         int ns, int xn, int pxn)
> > +{
> > +    int wxn;
> > +
> > +    switch (arm_current_el(env)) {
> > +    case 0:
> 
> If arm_current_el() is 0 but EL3 is 32 bit then the
> controlling environment here is the Secure PL1, which
> is EL3, and falling through to look at sctlr_el[1] is
> wrong. (SCTLR(S) is sctlr_el[3].)
> 
> (Also get_phys_addr() can be called via the ats_write()
> functions, which means that "arm_current_el()" and "the
> address translation regime we're trying to determine the
> answer for" aren't necessarily the same...)
> 
> I think we're very soon going to need to bite the bullet and
> make this code have a concept of the current "translation
> regime", as the ARM ARM terms it...
> 
> If you wish to avoid that (and I wouldn't object, given
> this patchset is snowballing in scope already) then do what
> get_phys_addr() does to get the relevant SCTLR:
>     uint32_t sctlr = A32_BANKED_CURRENT_REG_GET(env, sctlr);
> This will work for everything we currently support:
>  * AArch64 EL1 (with an EL0 either AArch32 or AArch64)
>  * AArch32 EL1
>  * TZ for the limited case of EL3 == AArch32 (and so no EL1)

Thanks. I see from another mail that you've opted to get the
translation regime worked up sooner than later. I'll wait for
that to base a revision of this patch on.

> 
> > +    case 1:
> > +        wxn = env->cp15.sctlr_el[1] & SCTLR_WXN;
> > +        if (arm_el_is_aa64(env, 1)) {
> > +            if (is_user && !(ap & 1)) {
> > +                wxn = 0;
> > +            }
> 
> I don't understand what we're doing here: as far as I can see
> the ARM ARM simply defines that WXN is the bit from the
> relevant SCTLR, and it always applies if the page is writable.
> I have a feeling you're re-calculating half of "is this
> page writable" here, and then doing the other half below.
> It would be cleaner to pass in the 'prot' bits that have
> already been calculated.

When EL0 doesn't have read/write, it can still execute (that's
what the first patch of this 2 patch series addressed). It can
still execute even when WXN is set, which is what this condition
addresses. It wasn't clear from the table that that's how it
should work, but it doesn't say it doesn't work that way either,
and real hardware shows WXN gets ignored.

> 
> > +            pxn = pxn || ((ap & 1) && !(ap & 2));
> 
> Very weird way to write "ap == 1" ?

True, I guess I was trying to code too closely to the
table in the ARM ARM wrt to AP[2:1].

> 
> > +            xn = is_user ? xn : pxn;
> > +        } else {
> > +            int uwxn = env->cp15.sctlr_el[1] & SCTLR_UWXN;
> > +            pxn = pxn || (uwxn && (ap & 1) && !(ap & 2));
> > +            xn = xn || (!is_user && pxn) || (is_user && !(ap & 1));
> > +        }
> > +        if (arm_is_secure(env)) {
> > +            xn = xn || (ns && (env->cp15.scr_el3 & SCR_SIF));
> > +        }
> > +        break;
> > +    case 2:
> > +        /* TODO actually support EL2 */
> > +        assert(false);
> > +
> > +        if (arm_el_is_aa64(env, 2)) {
> > +            wxn = env->cp15.sctlr_el[2] & SCTLR_WXN;
> > +        } else {
> > +            /* wxn = HSCTLR.WXN */
> > +        }
> 
> You can just write this as
>     wxn = env->cp15.sctlr_el[2] & SCTLR_WXN;
> 
> because SCTLR_EL2 and HSCTLR are architecturally mapped and so
> we will store the state in the same env->cp15 field whether
> EL2 is 32 or 64 bit.

OK

> 
> > +        break;
> > +    case 3:
> > +        if (arm_el_is_aa64(env, 3)) {
> > +            wxn = env->cp15.sctlr_el[3] & SCTLR_WXN;
> > +        } else {
> > +            wxn = 0;
> 
> This should also be checking sctlr_el[3]'s WXN bit.
> (That's SCTLR(S) in AArch32 terms.)

I didn't see WXN defined for secure state of AArch32/V7.
Maybe I missed it?

> 
> You're also missing the handling of SCTLR.UWXN here.
> (Remember that if EL3 is AArch32 then all the traditional
> "operating system" PL1 privilege level code runs at EL3,
> not EL1.)

I don't completely understand how much EL1 behavior is
present at EL3 (always all?). Anyway, I left out UWXN here,
because I don't recall explicitly seeing it defined for EL3.
Maybe I missed it?

> 
> > +        }
> > +        xn = xn || (ns && (env->cp15.scr_el3 & SCR_SIF));
> > +        break;
> > +    }
> > +
> > +    return xn || (!(ap & 2) && wxn) ? 0 : PAGE_EXEC;
> 
> I think this is the other half of "recalculate whether
> the page is writable". Something more like this would be clearer:
> 
>   if (xn || (wxn && (prot & PAGE_WRITE))) {
>       return 0;
>   }
>   return PAGE_EXEC;
> 

Agreed. I was getting too close to the spec, and wanting to see
AP[2:1] like in the table. Using PAGE_* would look much nicer in
the code. I'll change it.

> 
> > +}
> > +
> >  static bool get_level1_table_address(CPUARMState *env, uint32_t *table,
> >                                           uint32_t address)
> >  {
> > @@ -4941,7 +5004,7 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
> >          if (extract32(tableattrs, 2, 1)) {
> >              attrs &= ~(1 << 4);
> >          }
> > -        /* Since we're always in the Non-secure state, NSTable is ignored. */
> > +        attrs |= extract32(tableattrs, 4, 1) << 3; /* NS */
> >          break;
> >      }
> >      /* Here descaddr is the final physical address, and attributes
> > @@ -4962,19 +5025,8 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
> >          *prot |= !(ap & 2) ? PAGE_WRITE : 0;
> >      }
> >
> > -    *prot |= PAGE_EXEC;
> > -    if ((arm_feature(env, ARM_FEATURE_V8) && is_user && (attrs & (1 << 12))) ||
> > -        (!arm_feature(env, ARM_FEATURE_V8) && (attrs & (1 << 12))) ||
> > -        (!arm_el_is_aa64(env, 1) && is_user && !(ap & 1)) ||
> > -        (!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;
> > -    }
> > +    *prot |= check_xn_lpae(env, is_user, ap, extract32(attrs, 3, 1),
> > +                           extract32(attrs, 12, 1), extract32(attrs, 11, 1));
> >
> >      if ((*prot == 0)
> >              || (!(*prot & PAGE_WRITE) && access_type == 1)
> > --
> > 1.9.3
> >
> 
> thanks
> -- PMM

Thank you!

drew

Re: [Qemu-devel] [PATCH 2/2] tcg-arm: more instruction execution control

[Andrew Jones]

On Fri, Jan 16, 2015 at 07:02:55PM +0000, Peter Maydell wrote:
> On 16 January 2015 at 16:16, Peter Maydell <peter.maydell@linaro.org> wrote:
> > I think we're very soon going to need to bite the bullet and
> > make this code have a concept of the current "translation
> > regime", as the ARM ARM terms it...
> 
> In fact I think we need to do it now. I'll try to
> write some code to at least get a conceptual framework
> in place and send it out next week sometime.

Sounds good. Thanks!

drew