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

[Qemu-devel] [PATCH 0/5] 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 4/5, and describes the details in its commit message.
Patches 1-3 prepare for the main patch, and patch 5/5 is a
code cleanup made possible by introducing a new helper function
with the main patch.

This is actually a second version of [1], now based on Peter's
recent translation regime support. However, as the series
has changed substantially, I'm not calling this 'v2'. I believe
I've addressed all of Peter's comments on that first posting in
this refresh though.

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 [2].

Thanks in advance for reviews!

drew

[1] http://lists.gnu.org/archive/html/qemu-devel/2015-01/msg01433.html
[2] https://github.com/rhdrjones/kvm-unit-tests/commit/ee553e4bb795b0150e31c794bf8953dfb08d619a

Andrew Jones (5):
  target-arm: convert check_ap to get_rw_prot
  target-arm: enable get_rw_prot to take simple AP
  target-arm: add an is_user param to get_rw_prot
  target-arm: get_phys_addr_lpae: more xn control
  target-arm: apply get_S1prot to get_phys_addr_v6

 target-arm/helper.c | 193 ++++++++++++++++++++++++++++++++++------------------
 1 file changed, 126 insertions(+), 67 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 1/5] target-arm: convert check_ap to get_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. As this function only considers READ/WRITE, not EXEC, then name
it accordingly.

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

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 1a1a00577e780..610f305c4d661 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -4692,34 +4692,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 get_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;
@@ -4732,7 +4721,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;
         }
@@ -4741,16 +4730,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();
     }
 }
 
@@ -4872,12 +4861,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 = get_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:
@@ -4993,14 +4982,12 @@ 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 = get_rw_prot(env, mmu_idx, ap, domain_prot);
+        *prot |= *prot && !xn ? PAGE_EXEC : 0;
+        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

Re: [Qemu-devel] [PATCH 1/5] target-arm: convert check_ap to get_rw_prot

[Peter Maydell]

On 12 February 2015 at 15:05, Andrew Jones <drjones@redhat.com> wrote:
> 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. As this function only considers READ/WRITE, not EXEC, then name
> it accordingly.
>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  target-arm/helper.c | 47 +++++++++++++++++------------------------------
>  1 file changed, 17 insertions(+), 30 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 1a1a00577e780..610f305c4d661 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4692,34 +4692,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
> + */

Given that the 'ap' parameter isn't just the AP bits this
could use a mention in the comment:

/* Translate section/page access permissions to page
 * R/W protection flags. The 'ap' parameter is the concatenation
 * of the APX:AP bits (with APX zero for the descriptor formats
 * which don't have it).
 */

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

-- PMM

Re: [Qemu-devel] [PATCH 1/5] target-arm: convert check_ap to get_rw_prot

[Peter Maydell]

On 10 March 2015 at 15:07, Peter Maydell <peter.maydell@linaro.org> wrote:
> Given that the 'ap' parameter isn't just the AP bits this
> could use a mention in the comment:
>
> /* Translate section/page access permissions to page
>  * R/W protection flags. The 'ap' parameter is the concatenation
>  * of the APX:AP bits (with APX zero for the descriptor formats
>  * which don't have it).
>  */

Ignore this -- "APX" is an ARMv6-ism, and that bit is just
AP[2] in v7 and up.

-- PMM

Re: [Qemu-devel] [PATCH 1/5] target-arm: convert check_ap to get_rw_prot

[Peter Maydell]

On 12 February 2015 at 15:05, Andrew Jones <drjones@redhat.com> wrote:
> +        *prot = get_rw_prot(env, mmu_idx, ap, domain_prot);
> +        *prot |= *prot && !xn ? PAGE_EXEC : 0;

I'm not a great fan of complicated ?: expressions, incidentally;
I think this is clearer to read as
   if (*prot && !xn) {
       *prot |= PAGE_EXEC;
   }

-- PMM

Re: [Qemu-devel] [PATCH 1/5] target-arm: convert check_ap to get_rw_prot

[Andrew Jones]

On Tue, Mar 10, 2015 at 03:12:20PM +0000, Peter Maydell wrote:
> On 12 February 2015 at 15:05, Andrew Jones <drjones@redhat.com> wrote:
> > +        *prot = get_rw_prot(env, mmu_idx, ap, domain_prot);
> > +        *prot |= *prot && !xn ? PAGE_EXEC : 0;
> 
> I'm not a great fan of complicated ?: expressions, incidentally;
> I think this is clearer to read as
>    if (*prot && !xn) {
>        *prot |= PAGE_EXEC;
>    }
> 
> -- PMM

OK, I'll change it if we need a v2.

Thanks,
drew

[Qemu-devel] [PATCH 2/5] target-arm: enable get_rw_prot to take simple AP

[Andrew Jones]

Teach get_rw_prot about the simple AP format AP[2:1]. An additional
switch was added, as opposed to converting ap := AP[2:1] to AP[2:0]
with a simple shift - and then modifying cases 0,2,4,6, because the
resulting code is easier to read with the switch.

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

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 610f305c4d661..b63ec7b7979f9 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -4698,12 +4698,32 @@ static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx)
 static inline int get_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
                               int ap, int domain_prot)
 {
+    bool simple_ap = regime_using_lpae_format(env, mmu_idx)
+                         || (regime_sctlr(env, mmu_idx) & SCTLR_AFE);
+    bool domain_prot_valid = !regime_using_lpae_format(env, mmu_idx);
     bool is_user = regime_is_user(env, mmu_idx);
 
-    if (domain_prot == 3) {
+    if (domain_prot_valid && domain_prot == 3) {
         return PAGE_READ | PAGE_WRITE;
     }
 
+    /* ap is AP[2:1] */
+    if (simple_ap) {
+        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();
+        }
+    }
+
+    /* ap is AP[2:0] */
     switch (ap) {
     case 0:
         if (arm_feature(env, ARM_FEATURE_V7)) {
-- 
1.9.3

Re: [Qemu-devel] [PATCH 2/5] target-arm: enable get_rw_prot to take simple AP

[Peter Maydell]

On 12 February 2015 at 15:05, Andrew Jones <drjones@redhat.com> wrote:
> Teach get_rw_prot about the simple AP format AP[2:1]. An additional
> switch was added, as opposed to converting ap := AP[2:1] to AP[2:0]
> with a simple shift - and then modifying cases 0,2,4,6, because the
> resulting code is easier to read with the switch.
>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  target-arm/helper.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 610f305c4d661..b63ec7b7979f9 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4698,12 +4698,32 @@ static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx)
>  static inline int get_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
>                                int ap, int domain_prot)
>  {
> +    bool simple_ap = regime_using_lpae_format(env, mmu_idx)
> +                         || (regime_sctlr(env, mmu_idx) & SCTLR_AFE);

We should check arm_feature(env, ARM_FEATURE_V6K) && (SCTLR.AFE is set);
that bit isn't defined til v6K.

> +    bool domain_prot_valid = !regime_using_lpae_format(env, mmu_idx);

Given that the lpae code path is totally separate (and not even
calling this function yet), can't you just have it pass in a
zero domain_prot ? Or have the callers do the domain protection
check themselves...

>      bool is_user = regime_is_user(env, mmu_idx);
>
> -    if (domain_prot == 3) {
> +    if (domain_prot_valid && domain_prot == 3) {
>          return PAGE_READ | PAGE_WRITE;
>      }
>
> +    /* ap is AP[2:1] */
> +    if (simple_ap) {
> +        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();
> +        }
> +    }

I'm confused. Even if we're using the simple-permissions
model, the ap parameter is still AP[2:0]. Shouldn't this
switch be for cases 0, 2, 4, 6 ?

> +
> +    /* ap is AP[2:0] */
>      switch (ap) {
>      case 0:
>          if (arm_feature(env, ARM_FEATURE_V7)) {
> --
> 1.9.3
>

-- PMM

Re: [Qemu-devel] [PATCH 2/5] target-arm: enable get_rw_prot to take simple AP

[Andrew Jones]

On Tue, Mar 10, 2015 at 03:22:55PM +0000, Peter Maydell wrote:
> On 12 February 2015 at 15:05, Andrew Jones <drjones@redhat.com> wrote:
> > Teach get_rw_prot about the simple AP format AP[2:1]. An additional
> > switch was added, as opposed to converting ap := AP[2:1] to AP[2:0]
> > with a simple shift - and then modifying cases 0,2,4,6, because the
> > resulting code is easier to read with the switch.
> >
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  target-arm/helper.c | 22 +++++++++++++++++++++-
> >  1 file changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index 610f305c4d661..b63ec7b7979f9 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -4698,12 +4698,32 @@ static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx)
> >  static inline int get_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
> >                                int ap, int domain_prot)
> >  {
> > +    bool simple_ap = regime_using_lpae_format(env, mmu_idx)
> > +                         || (regime_sctlr(env, mmu_idx) & SCTLR_AFE);
> 
> We should check arm_feature(env, ARM_FEATURE_V6K) && (SCTLR.AFE is set);
> that bit isn't defined til v6K.

Indeed. Will send v2 for that.

> 
> > +    bool domain_prot_valid = !regime_using_lpae_format(env, mmu_idx);
> 
> Given that the lpae code path is totally separate (and not even
> calling this function yet), can't you just have it pass in a
> zero domain_prot ? Or have the callers do the domain protection
> check themselves...

domain_prot=0 is a valid access permission (no access), so I didn't
want to overload the meaning with 'not used'. I can move the check to
the callers that need it though. It would actually be nice to remove
the need for a 0 place holder from the other callers.

> 
> >      bool is_user = regime_is_user(env, mmu_idx);
> >
> > -    if (domain_prot == 3) {
> > +    if (domain_prot_valid && domain_prot == 3) {
> >          return PAGE_READ | PAGE_WRITE;
> >      }
> >
> > +    /* ap is AP[2:1] */
> > +    if (simple_ap) {
> > +        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();
> > +        }
> > +    }
> 
> I'm confused. Even if we're using the simple-permissions
> model, the ap parameter is still AP[2:0]. Shouldn't this
> switch be for cases 0, 2, 4, 6 ?

Depends on how we choose to implement the callers. Currently
I only require the caller to send in 2 bits for the simple
model. If we want to require them to send in 3, then we'll
need to shift a zero in for the lpae caller, rather than
shift a zero out for the v6 caller.

> 
> > +
> > +    /* ap is AP[2:0] */
> >      switch (ap) {
> >      case 0:
> >          if (arm_feature(env, ARM_FEATURE_V7)) {
> > --
> > 1.9.3
> >
> 
> -- PMM

Re: [Qemu-devel] [PATCH 2/5] target-arm: enable get_rw_prot to take simple AP

[Peter Maydell]

On 10 March 2015 at 16:32, Andrew Jones <drjones@redhat.com> wrote:
> On Tue, Mar 10, 2015 at 03:22:55PM +0000, Peter Maydell wrote:
>> I'm confused. Even if we're using the simple-permissions
>> model, the ap parameter is still AP[2:0]. Shouldn't this
>> switch be for cases 0, 2, 4, 6 ?
>
> Depends on how we choose to implement the callers. Currently
> I only require the caller to send in 2 bits for the simple
> model. If we want to require them to send in 3, then we'll
> need to shift a zero in for the lpae caller, rather than
> shift a zero out for the v6 caller.

You have to have the callers just pass in AP[2:0], unless
you want them to have to duplicate the "are we using the
simple permissions model?" condition to figure out whether
to shift the argument around, which doesn't seem very
sensible.

-- PMM

Re: [Qemu-devel] [PATCH 2/5] target-arm: enable get_rw_prot to take simple AP

[Andrew Jones]

On Tue, Mar 10, 2015 at 04:41:45PM +0000, Peter Maydell wrote:
> On 10 March 2015 at 16:32, Andrew Jones <drjones@redhat.com> wrote:
> > On Tue, Mar 10, 2015 at 03:22:55PM +0000, Peter Maydell wrote:
> >> I'm confused. Even if we're using the simple-permissions
> >> model, the ap parameter is still AP[2:0]. Shouldn't this
> >> switch be for cases 0, 2, 4, 6 ?
> >
> > Depends on how we choose to implement the callers. Currently
> > I only require the caller to send in 2 bits for the simple
> > model. If we want to require them to send in 3, then we'll
> > need to shift a zero in for the lpae caller, rather than
> > shift a zero out for the v6 caller.
> 
> You have to have the callers just pass in AP[2:0], unless
> you want them to have to duplicate the "are we using the
> simple permissions model?" condition to figure out whether
> to shift the argument around, which doesn't seem very
> sensible.
>

The v6 caller is the only one that could be either-or, and it
already checked regime_sctlr(env, mmu_idx) & SCTLR_AFE, as it
wanted to see if it cared about the access flag anyway. I
suspect any new callers that could be either-or would do the
same.

drew

[Qemu-devel] [PATCH 3/5] target-arm: add an is_user param to get_rw_prot

[Andrew Jones]

Give callers the ability to get page protection flags for PL0, even
if not currently operating in PL0. This puts the burden of determining
'is_user' on the caller (again - it was this way before regime_is_user
was introduced), but will be useful in a following patch.

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

diff --git a/target-arm/helper.c b/target-arm/helper.c
index b63ec7b7979f9..df78f481e92f4 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -4696,12 +4696,11 @@ static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx)
  * R/W protection flags
  */
 static inline int get_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
-                              int ap, int domain_prot)
+                              bool is_user, int ap, int domain_prot)
 {
     bool simple_ap = regime_using_lpae_format(env, mmu_idx)
                          || (regime_sctlr(env, mmu_idx) & SCTLR_AFE);
     bool domain_prot_valid = !regime_using_lpae_format(env, mmu_idx);
-    bool is_user = regime_is_user(env, mmu_idx);
 
     if (domain_prot_valid && domain_prot == 3) {
         return PAGE_READ | PAGE_WRITE;
@@ -4881,7 +4880,8 @@ static int get_phys_addr_v5(CPUARMState *env, uint32_t address, int access_type,
         }
         code = 15;
     }
-    *prot = get_rw_prot(env, mmu_idx, ap, domain_prot);
+    *prot = get_rw_prot(env, mmu_idx, regime_is_user(env, mmu_idx),
+                        ap, domain_prot);
     *prot |= *prot ? PAGE_EXEC : 0;
     if (!(*prot & (1 << access_type))) {
         /* Access permission fault.  */
@@ -4989,7 +4989,9 @@ static int get_phys_addr_v6(CPUARMState *env, uint32_t address, int access_type,
     if (domain_prot == 3) {
         *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
     } else {
-        if (pxn && !regime_is_user(env, mmu_idx)) {
+        bool is_user = regime_is_user(env, mmu_idx);
+
+        if (pxn && !is_user) {
             xn = 1;
         }
         if (xn && access_type == 2)
@@ -5002,7 +5004,7 @@ static int get_phys_addr_v6(CPUARMState *env, uint32_t address, int access_type,
             code = (code == 15) ? 6 : 3;
             goto do_fault;
         }
-        *prot = get_rw_prot(env, mmu_idx, ap, domain_prot);
+        *prot = get_rw_prot(env, mmu_idx, is_user, ap, domain_prot);
         *prot |= *prot && !xn ? PAGE_EXEC : 0;
         if (!(*prot & (1 << access_type))) {
             /* Access permission fault.  */
-- 
1.9.3

[Qemu-devel] [PATCH 4/5] 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, which also handles short-
descriptors and v6 XN determination. 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 | 111 ++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 86 insertions(+), 25 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index df78f481e92f4..20e5753bd216d 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -4695,8 +4695,8 @@ static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx)
 /* Translate section/page access permissions to page
  * R/W protection flags
  */
-static inline int get_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
-                              bool is_user, int ap, int domain_prot)
+static int get_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
+                       bool is_user, int ap, int domain_prot)
 {
     bool simple_ap = regime_using_lpae_format(env, mmu_idx)
                          || (regime_sctlr(env, mmu_idx) & SCTLR_AFE);
@@ -4762,6 +4762,84 @@ static inline int get_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
     }
 }
 
+/* Translate section/page access permissions to protection flags */
+static int get_S1prot(CPUARMState *env, ARMMMUIdx mmu_idx, bool is_aa64,
+                      int ap, int domain_prot, int ns, int xn, int pxn)
+{
+    bool domain_prot_valid = !regime_using_lpae_format(env, mmu_idx);
+    bool is_user = regime_is_user(env, mmu_idx);
+    bool have_wxn;
+    int prot_rw, user_rw;
+    int wxn = 0;
+
+    assert(mmu_idx != ARMMMUIdx_S2NS);
+
+    if (domain_prot_valid && domain_prot == 3) {
+        return PAGE_READ | PAGE_WRITE | PAGE_EXEC;
+    }
+
+    user_rw = get_rw_prot(env, mmu_idx, true, ap, domain_prot);
+    if (is_user) {
+        prot_rw = user_rw;
+    } else {
+        prot_rw = get_rw_prot(env, mmu_idx, false, ap, domain_prot);
+    }
+
+    if (ns && arm_is_secure(env) && (env->cp15.scr_el3 & SCR_SIF)) {
+        return prot_rw;
+    }
+
+    /* TODO have_wxn should be replaced with arm_feature(env, ARM_FEATURE_EL2),
+     * when ARM_FEATURE_EL2 starts getting set. For now we assume all v7
+     * compatible processors have EL2, which is required for [U]WXN.
+     */
+    have_wxn = arm_feature(env, ARM_FEATURE_V7);
+
+    if (have_wxn) {
+        wxn = regime_sctlr(env, mmu_idx) & SCTLR_WXN;
+    }
+
+    if (is_aa64) {
+        assert(arm_feature(env, ARM_FEATURE_V8));
+        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_V6K)) {
+        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)
 {
@@ -5047,7 +5125,6 @@ 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);
 
     /* TODO:
@@ -5220,31 +5297,15 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
         /* Access flag */
         goto do_fault;
     }
+
+    *prot = get_S1prot(env, mmu_idx, va_size == 64, extract32(attrs, 4, 2), 0,
+                       extract32(attrs, 3, 1), extract32(attrs, 12, 1),
+                       extract32(attrs, 11, 1));
+
     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 4/5] target-arm: get_phys_addr_lpae: more xn control

[Andrew Jones]

On Thu, Feb 12, 2015 at 04:05:06PM +0100, Andrew Jones 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, which also handles short-
> descriptors and v6 XN determination. It may even work in EL2,
> when support for that comes, but, as the function name implies,
> it only works for stage 1 translations.

Just because I like replying to myself so much... re: feature checking

> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  target-arm/helper.c | 111 ++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 86 insertions(+), 25 deletions(-)
> 
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index df78f481e92f4..20e5753bd216d 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4695,8 +4695,8 @@ static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx)
>  /* Translate section/page access permissions to page
>   * R/W protection flags
>   */
> -static inline int get_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
> -                              bool is_user, int ap, int domain_prot)
> +static int get_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
> +                       bool is_user, int ap, int domain_prot)
>  {
>      bool simple_ap = regime_using_lpae_format(env, mmu_idx)
>                           || (regime_sctlr(env, mmu_idx) & SCTLR_AFE);
> @@ -4762,6 +4762,84 @@ static inline int get_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
>      }
>  }
>  
> +/* Translate section/page access permissions to protection flags */
> +static int get_S1prot(CPUARMState *env, ARMMMUIdx mmu_idx, bool is_aa64,
> +                      int ap, int domain_prot, int ns, int xn, int pxn)
> +{
> +    bool domain_prot_valid = !regime_using_lpae_format(env, mmu_idx);
> +    bool is_user = regime_is_user(env, mmu_idx);
> +    bool have_wxn;
> +    int prot_rw, user_rw;
> +    int wxn = 0;
> +
> +    assert(mmu_idx != ARMMMUIdx_S2NS);
> +
> +    if (domain_prot_valid && domain_prot == 3) {
> +        return PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> +    }
> +
> +    user_rw = get_rw_prot(env, mmu_idx, true, ap, domain_prot);
> +    if (is_user) {
> +        prot_rw = user_rw;
> +    } else {
> +        prot_rw = get_rw_prot(env, mmu_idx, false, ap, domain_prot);
> +    }
> +
> +    if (ns && arm_is_secure(env) && (env->cp15.scr_el3 & SCR_SIF)) {
> +        return prot_rw;
> +    }
> +
> +    /* TODO have_wxn should be replaced with arm_feature(env, ARM_FEATURE_EL2),
> +     * when ARM_FEATURE_EL2 starts getting set. For now we assume all v7
> +     * compatible processors have EL2, which is required for [U]WXN.
> +     */
> +    have_wxn = arm_feature(env, ARM_FEATURE_V7);

I probably should have used ARM_FEATURE_LPAE here instead to be a bit closer
to the truth.

> +
> +    if (have_wxn) {
> +        wxn = regime_sctlr(env, mmu_idx) & SCTLR_WXN;
> +    }
> +
> +    if (is_aa64) {
> +        assert(arm_feature(env, ARM_FEATURE_V8));
> +        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_V6K)) {
> +        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));

I think it's OK to trust the caller to only send a non-zero pxn when they
have arm_feature(env, ARM_FEATURE_PXN). However, it just occurred to me that
checking that feature before using pxn may have been a nice touch...

> +            }
> +            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)
>  {
> @@ -5047,7 +5125,6 @@ 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);
>  
>      /* TODO:
> @@ -5220,31 +5297,15 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>          /* Access flag */
>          goto do_fault;
>      }
> +
> +    *prot = get_S1prot(env, mmu_idx, va_size == 64, extract32(attrs, 4, 2), 0,
> +                       extract32(attrs, 3, 1), extract32(attrs, 12, 1),
> +                       extract32(attrs, 11, 1));
> +
>      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 4/5] target-arm: get_phys_addr_lpae: more xn control

[Peter Maydell]

On 12 February 2015 at 15:05, 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, which also handles short-
> descriptors and v6 XN determination. 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 | 111 ++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 86 insertions(+), 25 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index df78f481e92f4..20e5753bd216d 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4695,8 +4695,8 @@ static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx)
>  /* Translate section/page access permissions to page
>   * R/W protection flags
>   */
> -static inline int get_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
> -                              bool is_user, int ap, int domain_prot)
> +static int get_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
> +                       bool is_user, int ap, int domain_prot)

...why does this suddenly lose its 'inline' ?

>  {
>      bool simple_ap = regime_using_lpae_format(env, mmu_idx)
>                           || (regime_sctlr(env, mmu_idx) & SCTLR_AFE);
> @@ -4762,6 +4762,84 @@ static inline int get_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
>      }
>  }
>
> +/* Translate section/page access permissions to protection flags */

This is LPAE-format only so it would be nice to mention that in the comment
and function name.

> +static int get_S1prot(CPUARMState *env, ARMMMUIdx mmu_idx, bool is_aa64,
> +                      int ap, int domain_prot, int ns, int xn, int pxn)
> +{
> +    bool domain_prot_valid = !regime_using_lpae_format(env, mmu_idx);

But this is always false!

> +    bool is_user = regime_is_user(env, mmu_idx);
> +    bool have_wxn;
> +    int prot_rw, user_rw;
> +    int wxn = 0;
> +
> +    assert(mmu_idx != ARMMMUIdx_S2NS);
> +
> +    if (domain_prot_valid && domain_prot == 3) {
> +        return PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> +    }
> +
> +    user_rw = get_rw_prot(env, mmu_idx, true, ap, domain_prot);
> +    if (is_user) {
> +        prot_rw = user_rw;
> +    } else {
> +        prot_rw = get_rw_prot(env, mmu_idx, false, ap, domain_prot);
> +    }

I think it would be much better not to try to use the short-descriptor
get_rw_prot function. For one thing, we know for definite that we
won't be using the old-fashioned AP[2:0] access format, and that
we don't have to worry about the domain protection stuff. So it's
much simpler and better not to tangle it up with the legacy stuff.
(Pull the simple-access-permissions check out into its own function
if you like.)

For instance, you're missing a shift here on the ap bits, because
get_rw_prot needs AP[2:0] and 'ap' here is AP[2:1].

> +
> +    if (ns && arm_is_secure(env) && (env->cp15.scr_el3 & SCR_SIF)) {
> +        return prot_rw;
> +    }
> +
> +    /* TODO have_wxn should be replaced with arm_feature(env, ARM_FEATURE_EL2),
> +     * when ARM_FEATURE_EL2 starts getting set. For now we assume all v7
> +     * compatible processors have EL2, which is required for [U]WXN.
> +     */
> +    have_wxn = arm_feature(env, ARM_FEATURE_V7);

ARMv8 CPUs without EL2 still have WXN, I think.

> +
> +    if (have_wxn) {
> +        wxn = regime_sctlr(env, mmu_idx) & SCTLR_WXN;
> +    }
> +
> +    if (is_aa64) {
> +        assert(arm_feature(env, ARM_FEATURE_V8));

I wouldn't bother with this assert.

> +        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_V6K)) {

Always true, since you can't have long format descriptors
unless this is at least 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;

Doesn't this lose us the "you need read permission to execute"
check (for 32-bit)? Something in here should be doing a
PAGE_READ check to see if we can have PAGE_EXEC.

> +        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)
>  {
> @@ -5047,7 +5125,6 @@ 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);
>
>      /* TODO:
> @@ -5220,31 +5297,15 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>          /* Access flag */
>          goto do_fault;
>      }
> +
> +    *prot = get_S1prot(env, mmu_idx, va_size == 64, extract32(attrs, 4, 2), 0,
> +                       extract32(attrs, 3, 1), extract32(attrs, 12, 1),
> +                       extract32(attrs, 11, 1));

Urgh. It would be easier to understand if you just passed attrs
into get_S1prot and had it pick the fields apart, because then
you can match them up with variable names without cross-referencing
against the function definition.

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


-- PMM

Re: [Qemu-devel] [PATCH 4/5] target-arm: get_phys_addr_lpae: more xn control

[Andrew Jones]

On Tue, Mar 10, 2015 at 03:56:11PM +0000, Peter Maydell wrote:
> On 12 February 2015 at 15:05, 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, which also handles short-
> > descriptors and v6 XN determination. 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 | 111 ++++++++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 86 insertions(+), 25 deletions(-)
> >
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index df78f481e92f4..20e5753bd216d 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -4695,8 +4695,8 @@ static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx)
> >  /* Translate section/page access permissions to page
> >   * R/W protection flags
> >   */
> > -static inline int get_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
> > -                              bool is_user, int ap, int domain_prot)
> > +static int get_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
> > +                       bool is_user, int ap, int domain_prot)
> 
> ...why does this suddenly lose its 'inline' ?

Adding another caller, and thought it was a bit fat for explicit
inlining, but have no problem returning it.

> 
> >  {
> >      bool simple_ap = regime_using_lpae_format(env, mmu_idx)
> >                           || (regime_sctlr(env, mmu_idx) & SCTLR_AFE);
> > @@ -4762,6 +4762,84 @@ static inline int get_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
> >      }
> >  }
> >
> > +/* Translate section/page access permissions to protection flags */
> 
> This is LPAE-format only so it would be nice to mention that in the comment
> and function name.

Not after the next patch. I probably should have made it completely
LPAE-only first, then added two more patches, one preparing it for
non-LPAE, and then the next patch, or just done a better job pointing
that out in the commit message.

> 
> > +static int get_S1prot(CPUARMState *env, ARMMMUIdx mmu_idx, bool is_aa64,
> > +                      int ap, int domain_prot, int ns, int xn, int pxn)
> > +{
> > +    bool domain_prot_valid = !regime_using_lpae_format(env, mmu_idx);
> 
> But this is always false!

Same response as above - not after next patch.

> 
> > +    bool is_user = regime_is_user(env, mmu_idx);
> > +    bool have_wxn;
> > +    int prot_rw, user_rw;
> > +    int wxn = 0;
> > +
> > +    assert(mmu_idx != ARMMMUIdx_S2NS);
> > +
> > +    if (domain_prot_valid && domain_prot == 3) {
> > +        return PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> > +    }
> > +
> > +    user_rw = get_rw_prot(env, mmu_idx, true, ap, domain_prot);
> > +    if (is_user) {
> > +        prot_rw = user_rw;
> > +    } else {
> > +        prot_rw = get_rw_prot(env, mmu_idx, false, ap, domain_prot);
> > +    }
> 
> I think it would be much better not to try to use the short-descriptor
> get_rw_prot function. For one thing, we know for definite that we
> won't be using the old-fashioned AP[2:0] access format, and that
> we don't have to worry about the domain protection stuff. So it's
> much simpler and better not to tangle it up with the legacy stuff.
> (Pull the simple-access-permissions check out into its own function
> if you like.)

legacy stuff is here for next patch too

> 
> For instance, you're missing a shift here on the ap bits, because
> get_rw_prot needs AP[2:0] and 'ap' here is AP[2:1].

Don't need the shift because get_rw_prot supports the 2-bit format.

> 
> > +
> > +    if (ns && arm_is_secure(env) && (env->cp15.scr_el3 & SCR_SIF)) {
> > +        return prot_rw;
> > +    }
> > +
> > +    /* TODO have_wxn should be replaced with arm_feature(env, ARM_FEATURE_EL2),
> > +     * when ARM_FEATURE_EL2 starts getting set. For now we assume all v7
> > +     * compatible processors have EL2, which is required for [U]WXN.
> > +     */
> > +    have_wxn = arm_feature(env, ARM_FEATURE_V7);
> 
> ARMv8 CPUs without EL2 still have WXN, I think.

I think so too. So V8 || (V7 && EL2) would be the most appropriate.

> 
> > +
> > +    if (have_wxn) {
> > +        wxn = regime_sctlr(env, mmu_idx) & SCTLR_WXN;
> > +    }
> > +
> > +    if (is_aa64) {
> > +        assert(arm_feature(env, ARM_FEATURE_V8));
> 
> I wouldn't bother with this assert.

OK

> 
> > +        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_V6K)) {
> 
> Always true, since you can't have long format descriptors
> unless this is at least v7.

Next patch again.

> 
> > +        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;
> 
> Doesn't this lose us the "you need read permission to execute"
> check (for 32-bit)? Something in here should be doing a
> PAGE_READ check to see if we can have PAGE_EXEC.

It's there. It's the '!user_rw' and the '!prot_rw'
> 
> > +        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)
> >  {
> > @@ -5047,7 +5125,6 @@ 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);
> >
> >      /* TODO:
> > @@ -5220,31 +5297,15 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
> >          /* Access flag */
> >          goto do_fault;
> >      }
> > +
> > +    *prot = get_S1prot(env, mmu_idx, va_size == 64, extract32(attrs, 4, 2), 0,
> > +                       extract32(attrs, 3, 1), extract32(attrs, 12, 1),
> > +                       extract32(attrs, 11, 1));
> 
> Urgh. It would be easier to understand if you just passed attrs
> into get_S1prot and had it pick the fields apart, because then
> you can match them up with variable names without cross-referencing
> against the function definition.

I can pick them apart before passing into local well-named variables.
I'd prefer to keep the helper function independent of get_phys_addr_lpae's
internal bitmap.

> 
> > +
> >      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
> >
> 
> 
> -- PMM

Re: [Qemu-devel] [PATCH 4/5] target-arm: get_phys_addr_lpae: more xn control

[Peter Maydell]

On 10 March 2015 at 16:48, Andrew Jones <drjones@redhat.com> wrote:
> On Tue, Mar 10, 2015 at 03:56:11PM +0000, Peter Maydell wrote:

>> For instance, you're missing a shift here on the ap bits, because
>> get_rw_prot needs AP[2:0] and 'ap' here is AP[2:1].
>
> Don't need the shift because get_rw_prot supports the 2-bit format.

No it doesn't...

>> Doesn't this lose us the "you need read permission to execute"
>> check (for 32-bit)? Something in here should be doing a
>> PAGE_READ check to see if we can have PAGE_EXEC.
>
> It's there. It's the '!user_rw' and the '!prot_rw'

Ah yes, and that works because you can't have a page which
is writable but not readable (which is what I'd forgotten).

-- PMM

Re: [Qemu-devel] [PATCH 4/5] target-arm: get_phys_addr_lpae: more xn control

[Andrew Jones]

On Tue, Mar 10, 2015 at 04:55:53PM +0000, Peter Maydell wrote:
> On 10 March 2015 at 16:48, Andrew Jones <drjones@redhat.com> wrote:
> > On Tue, Mar 10, 2015 at 03:56:11PM +0000, Peter Maydell wrote:
> 
> >> For instance, you're missing a shift here on the ap bits, because
> >> get_rw_prot needs AP[2:0] and 'ap' here is AP[2:1].
> >
> > Don't need the shift because get_rw_prot supports the 2-bit format.
> 
> No it doesn't...

Yes it does :-) That's the support patch 2/5 adds.

> 
> >> Doesn't this lose us the "you need read permission to execute"
> >> check (for 32-bit)? Something in here should be doing a
> >> PAGE_READ check to see if we can have PAGE_EXEC.
> >
> > It's there. It's the '!user_rw' and the '!prot_rw'
> 
> Ah yes, and that works because you can't have a page which
> is writable but not readable (which is what I'd forgotten).
> 
> -- PMM

Re: [Qemu-devel] [PATCH 4/5] target-arm: get_phys_addr_lpae: more xn control

[Peter Maydell]

On 10 March 2015 at 17:02, Andrew Jones <drjones@redhat.com> wrote:
> On Tue, Mar 10, 2015 at 04:55:53PM +0000, Peter Maydell wrote:
>> On 10 March 2015 at 16:48, Andrew Jones <drjones@redhat.com> wrote:
>> > On Tue, Mar 10, 2015 at 03:56:11PM +0000, Peter Maydell wrote:
>>
>> >> For instance, you're missing a shift here on the ap bits, because
>> >> get_rw_prot needs AP[2:0] and 'ap' here is AP[2:1].
>> >
>> > Don't need the shift because get_rw_prot supports the 2-bit format.
>>
>> No it doesn't...
>
> Yes it does :-) That's the support patch 2/5 adds.

No, because patch 2 doesn't do anything in the callers to
make them pass only bits [2:1]. So after patch 2 get_rw_prot
still requires bits [2:0]. Except it's broken, because the
function itself assumes it gets bits [2:1].

Having thought a bit more about it, I think we should
just have two totally separate functions for the old
style and simplified permission checks, because we can
always call the correct one (always old for v5, either
one for v6 since we already have the SCTLR.AFE check,
and the simplified one for the lpae code path).

-- PMM

Re: [Qemu-devel] [PATCH 4/5] target-arm: get_phys_addr_lpae: more xn control

[Andrew Jones]

On Tue, Mar 10, 2015 at 05:14:21PM +0000, Peter Maydell wrote:
> On 10 March 2015 at 17:02, Andrew Jones <drjones@redhat.com> wrote:
> > On Tue, Mar 10, 2015 at 04:55:53PM +0000, Peter Maydell wrote:
> >> On 10 March 2015 at 16:48, Andrew Jones <drjones@redhat.com> wrote:
> >> > On Tue, Mar 10, 2015 at 03:56:11PM +0000, Peter Maydell wrote:
> >>
> >> >> For instance, you're missing a shift here on the ap bits, because
> >> >> get_rw_prot needs AP[2:0] and 'ap' here is AP[2:1].
> >> >
> >> > Don't need the shift because get_rw_prot supports the 2-bit format.
> >>
> >> No it doesn't...
> >
> > Yes it does :-) That's the support patch 2/5 adds.
> 
> No, because patch 2 doesn't do anything in the callers to
> make them pass only bits [2:1]. So after patch 2 get_rw_prot
> still requires bits [2:0]. Except it's broken, because the
> function itself assumes it gets bits [2:1].

You've lost me. Patch 2 adds support for 2-bit ap, but
doesn't remove support for 3-bit. There are not callers
expecting it to support the simple model as 2 or 3 bits
at that time, except v6, but that was broken already, and
we fix it in patch 5 (and we add the ap shift there too).
IOW, how can preparing a function for new callers, while
still supporting the old callers, be 'broken'?

> 
> Having thought a bit more about it, I think we should
> just have two totally separate functions for the old
> style and simplified permission checks, because we can
> always call the correct one (always old for v5, either
> one for v6 since we already have the SCTLR.AFE check,
> and the simplified one for the lpae code path).

I like that.

Thanks,
drew

Re: [Qemu-devel] [PATCH 4/5] target-arm: get_phys_addr_lpae: more xn control

[Peter Maydell]

[-- Attachment #1.1: Type: text/plain, Size: 1212 bytes --]

On 10 March 2015 at 17:28, Andrew Jones <drjones@redhat.com> wrote:
> On Tue, Mar 10, 2015 at 05:14:21PM +0000, Peter Maydell wrote:[?]
>> No, because patch 2 doesn't do anything in the callers to
>> make them pass only bits [2:1]. So after patch 2 get_rw_prot
>> still requires bits [2:0]. Except it's broken, because the
>> function itself assumes it gets bits [2:1].
>
> You've lost me. Patch 2 adds support for 2-bit ap, but
> doesn't remove support for 3-bit. There are not callers
> expecting it to support the simple model as 2 or 3 bits
> at that time, except v6, but that was broken already, and
> we fix it in patch 5 (and we add the ap shift there too).
> IOW, how can preparing a function for new callers, while
> still supporting the old callers, be 'broken'?

(This is all somewhat academic at this point given we're
splitting the two functions up, but:)

Because it's not being consistent with itself -- if
it wants to accept AP[2:1] only then the v6 callsite
needs to change, otherwise it needs to handle AP[2:0] and
look only at bits 2:1 in that. You eventually fixed
this in patch 5 with an extra shift, but if you wanted
to do it that way then that part should have been in
patch 2...

-- PMM

[-- Attachment #1.2: Type: text/html, Size: 1685 bytes --]

[-- Attachment #2: 333.png --]
[-- Type: image/png, Size: 646 bytes --]

Re: [Qemu-devel] [PATCH 4/5] target-arm: get_phys_addr_lpae: more xn control

[Andrew Jones]

On Thu, Feb 12, 2015 at 04:05:06PM +0100, Andrew Jones 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, which also handles short-
> descriptors and v6 XN determination. 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 | 111 ++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 86 insertions(+), 25 deletions(-)
> 
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index df78f481e92f4..20e5753bd216d 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4695,8 +4695,8 @@ static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx)
>  /* Translate section/page access permissions to page
>   * R/W protection flags
>   */
> -static inline int get_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
> -                              bool is_user, int ap, int domain_prot)
> +static int get_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
> +                       bool is_user, int ap, int domain_prot)
>  {
>      bool simple_ap = regime_using_lpae_format(env, mmu_idx)
>                           || (regime_sctlr(env, mmu_idx) & SCTLR_AFE);
> @@ -4762,6 +4762,84 @@ static inline int get_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
>      }
>  }
>  
> +/* Translate section/page access permissions to protection flags */
> +static int get_S1prot(CPUARMState *env, ARMMMUIdx mmu_idx, bool is_aa64,
> +                      int ap, int domain_prot, int ns, int xn, int pxn)
> +{
> +    bool domain_prot_valid = !regime_using_lpae_format(env, mmu_idx);
> +    bool is_user = regime_is_user(env, mmu_idx);
> +    bool have_wxn;
> +    int prot_rw, user_rw;
> +    int wxn = 0;
> +
> +    assert(mmu_idx != ARMMMUIdx_S2NS);
> +
> +    if (domain_prot_valid && domain_prot == 3) {
> +        return PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> +    }
> +
> +    user_rw = get_rw_prot(env, mmu_idx, true, ap, domain_prot);
> +    if (is_user) {
> +        prot_rw = user_rw;
> +    } else {
> +        prot_rw = get_rw_prot(env, mmu_idx, false, ap, domain_prot);
> +    }
> +
> +    if (ns && arm_is_secure(env) && (env->cp15.scr_el3 & SCR_SIF)) {
> +        return prot_rw;
> +    }
> +
> +    /* TODO have_wxn should be replaced with arm_feature(env, ARM_FEATURE_EL2),
> +     * when ARM_FEATURE_EL2 starts getting set. For now we assume all v7
> +     * compatible processors have EL2, which is required for [U]WXN.
> +     */
> +    have_wxn = arm_feature(env, ARM_FEATURE_V7);
> +
> +    if (have_wxn) {
> +        wxn = regime_sctlr(env, mmu_idx) & SCTLR_WXN;
> +    }
> +
> +    if (is_aa64) {
> +        assert(arm_feature(env, ARM_FEATURE_V8));
> +        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_V6K)) {
> +        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)
>  {
> @@ -5047,7 +5125,6 @@ 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);
>  
>      /* TODO:
> @@ -5220,31 +5297,15 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>          /* Access flag */
>          goto do_fault;
>      }

I just realized that I somehow dropped hunk 2 of patch 2 of the initial
post, http://lists.gnu.org/archive/html/qemu-devel/2015-01/msg01435.html

-        /* Since we're always in the Non-secure state, NSTable is ignored. */
+        attrs |= extract32(tableattrs, 4, 1) << 3; /* NS */

Unfortunately that's also missing from the v2 of this series, that I posted
yesterday. I'll wait for other comments before sending a v3 though.

> +
> +    *prot = get_S1prot(env, mmu_idx, va_size == 64, extract32(attrs, 4, 2), 0,
> +                       extract32(attrs, 3, 1), extract32(attrs, 12, 1),
> +                       extract32(attrs, 11, 1));
> +
>      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
> 

[Qemu-devel] [PATCH 5/5] target-arm: apply get_S1prot to get_phys_addr_v6

[Andrew Jones]

Now that we have get_S1prot, we can apply it to get_phys_addr_v6
for a minor code cleanup.

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

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 20e5753bd216d..c41305e7e2bdf 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -5064,30 +5064,19 @@ static int get_phys_addr_v6(CPUARMState *env, uint32_t address, int access_type,
         }
         code = 15;
     }
-    if (domain_prot == 3) {
-        *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
-    } else {
-        bool is_user = regime_is_user(env, mmu_idx);
-
-        if (pxn && !is_user) {
-            xn = 1;
-        }
-        if (xn && access_type == 2)
-            goto do_fault;
-
+    if (regime_sctlr(env, mmu_idx) & SCTLR_AFE) {
         /* The simplified model uses AP[0] as an access control bit.  */
-        if ((regime_sctlr(env, mmu_idx) & SCTLR_AFE)
-                && (ap & 1) == 0) {
+        if ((ap & 1) == 0) {
             /* Access flag fault.  */
             code = (code == 15) ? 6 : 3;
             goto do_fault;
         }
-        *prot = get_rw_prot(env, mmu_idx, is_user, ap, domain_prot);
-        *prot |= *prot && !xn ? PAGE_EXEC : 0;
-        if (!(*prot & (1 << access_type))) {
-            /* Access permission fault.  */
-            goto do_fault;
-        }
+        ap >>= 1;
+    }
+    *prot = get_S1prot(env, mmu_idx, false, ap, domain_prot, 0, xn, pxn);
+    if (!(*prot & (1 << access_type))) {
+        /* Access permission fault.  */
+        goto do_fault;
     }
     *phys_ptr = phys_addr;
     return 0;
-- 
1.9.3

Re: [Qemu-devel] [PATCH 5/5] target-arm: apply get_S1prot to get_phys_addr_v6

[Andrew Jones]

On Thu, Feb 12, 2015 at 04:05:07PM +0100, Andrew Jones wrote:
> Now that we have get_S1prot, we can apply it to get_phys_addr_v6
> for a minor code cleanup.

Actually, I should point out that this isn't just a cleanup, but
also a fix. See below.

> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  target-arm/helper.c | 27 ++++++++-------------------
>  1 file changed, 8 insertions(+), 19 deletions(-)
> 
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 20e5753bd216d..c41305e7e2bdf 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -5064,30 +5064,19 @@ static int get_phys_addr_v6(CPUARMState *env, uint32_t address, int access_type,
>          }
>          code = 15;
>      }
> -    if (domain_prot == 3) {
> -        *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> -    } else {
> -        bool is_user = regime_is_user(env, mmu_idx);
> -
> -        if (pxn && !is_user) {
> -            xn = 1;
> -        }
> -        if (xn && access_type == 2)
> -            goto do_fault;
> -
> +    if (regime_sctlr(env, mmu_idx) & SCTLR_AFE) {
>          /* The simplified model uses AP[0] as an access control bit.  */
> -        if ((regime_sctlr(env, mmu_idx) & SCTLR_AFE)
> -                && (ap & 1) == 0) {
> +        if ((ap & 1) == 0) {
>              /* Access flag fault.  */
>              code = (code == 15) ? 6 : 3;
>              goto do_fault;
>          }
> -        *prot = get_rw_prot(env, mmu_idx, is_user, ap, domain_prot);
> -        *prot |= *prot && !xn ? PAGE_EXEC : 0;
> -        if (!(*prot & (1 << access_type))) {
> -            /* Access permission fault.  */
> -            goto do_fault;
> -        }
> +        ap >>= 1;

The original code didn't take into account that it may be calling check_ap
with a simple AP, AP[2:1]. The code should have always been similar to
the above, i.e.

        if (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 = <handle simple AP somehow>;
        } else {
            *prot = check_ap(env, mmu_idx, ap, domain_prot, access_type);
        }
        if (!*prot) {
            /* Access permission fault.  */
            goto do_fault;
        }
        if (!xn) {
            *prot |= PAGE_EXEC;
        }

As a simple AP wouldn't be properly translated to protection flags with
check_ap (except for case 6), then I think this should have caused some
problems. Maybe this path just hasn't been tested? I don't see CR_AFE
getting used by Linux, so possibly not.

I should update the commit message to point this fix out. Or, actually,
I should probably add another patch to the series (3/6), which addresses
just this issue, and builds it on patch 2 "...to take simple AP". Peter,
please let me know your preference.

Thanks,
drew

> +    }
> +    *prot = get_S1prot(env, mmu_idx, false, ap, domain_prot, 0, xn, pxn);
> +    if (!(*prot & (1 << access_type))) {
> +        /* Access permission fault.  */
> +        goto do_fault;
>      }
>      *phys_ptr = phys_addr;
>      return 0;
> -- 
> 1.9.3
> 
> 
> 

Re: [Qemu-devel] [PATCH 5/5] target-arm: apply get_S1prot to get_phys_addr_v6

[Peter Maydell]

On 12 February 2015 at 17:08, Andrew Jones <drjones@redhat.com> wrote:
> On Thu, Feb 12, 2015 at 04:05:07PM +0100, Andrew Jones wrote:
>> Now that we have get_S1prot, we can apply it to get_phys_addr_v6
>> for a minor code cleanup.

I think this is a bad idea -- better to keep the long
and short descriptor code paths separate. It's too easy
to get confused otherwise.

> Actually, I should point out that this isn't just a cleanup, but
> also a fix. See below.

> The original code didn't take into account that it may be calling check_ap
> with a simple AP, AP[2:1].

No, because check_ap() always takes AP[2:0]...

-- PMM

Re: [Qemu-devel] [PATCH 5/5] target-arm: apply get_S1prot to get_phys_addr_v6

[Andrew Jones]

On Tue, Mar 10, 2015 at 03:57:27PM +0000, Peter Maydell wrote:
> On 12 February 2015 at 17:08, Andrew Jones <drjones@redhat.com> wrote:
> > On Thu, Feb 12, 2015 at 04:05:07PM +0100, Andrew Jones wrote:
> >> Now that we have get_S1prot, we can apply it to get_phys_addr_v6
> >> for a minor code cleanup.
> 
> I think this is a bad idea -- better to keep the long
> and short descriptor code paths separate. It's too easy
> to get confused otherwise.

I don't mind keeping them separate, but I disagree with it being
confusing keeping them together :-)

> 
> > Actually, I should point out that this isn't just a cleanup, but
> > also a fix. See below.
> 
> > The original code didn't take into account that it may be calling check_ap
> > with a simple AP, AP[2:1].
> 
> No, because check_ap() always takes AP[2:0]...

No, it's really wrong. It's not the 2 vs. 3 bit issue that's the
problem, it's the cases. You snipped most of my reply to myself.
This part is pertinent

> As a simple AP wouldn't be properly translated to protection flags with
> check_ap (except for case 6), then I think this should have caused some
> problems. Maybe this path just hasn't been tested? I don't see CR_AFE
> getting used by Linux, so possibly not.

So, yes, a simple (3-bit) ap would be handled by the 8-case switch with
cases 0, 2, 4, and 6, but only case 6 would give the correct result.

Thanks for the review.

drew

Re: [Qemu-devel] [PATCH 5/5] target-arm: apply get_S1prot to get_phys_addr_v6

[Peter Maydell]

On 10 March 2015 at 16:54, Andrew Jones <drjones@redhat.com> wrote:
> On Tue, Mar 10, 2015 at 03:57:27PM +0000, Peter Maydell wrote:
>> On 12 February 2015 at 17:08, Andrew Jones <drjones@redhat.com> wrote:
>> > Actually, I should point out that this isn't just a cleanup, but
>> > also a fix. See below.
>>
>> > The original code didn't take into account that it may be calling check_ap
>> > with a simple AP, AP[2:1].
>>
>> No, because check_ap() always takes AP[2:0]...
>
> No, it's really wrong. It's not the 2 vs. 3 bit issue that's the
> problem, it's the cases. You snipped most of my reply to myself.
> This part is pertinent
>
>> As a simple AP wouldn't be properly translated to protection flags with
>> check_ap (except for case 6), then I think this should have caused some
>> problems. Maybe this path just hasn't been tested? I don't see CR_AFE
>> getting used by Linux, so possibly not.
>
> So, yes, a simple (3-bit) ap would be handled by the 8-case switch with
> cases 0, 2, 4, and 6, but only case 6 would give the correct result.

Well, we didn't correctly support the simple permission model at all
before your patches. But the point is that check_ap() always takes
AP[2:0] regardless.

Hmm, or you could have check_simple_ap() be totally separate
from check_ap(), and call it from inside the check in the v6
code path for AFE that we already have.

-- PMM

Re: [Qemu-devel] [PATCH 5/5] target-arm: apply get_S1prot to get_phys_addr_v6

[Andrew Jones]

On Tue, Mar 10, 2015 at 05:03:48PM +0000, Peter Maydell wrote:
> On 10 March 2015 at 16:54, Andrew Jones <drjones@redhat.com> wrote:
> > On Tue, Mar 10, 2015 at 03:57:27PM +0000, Peter Maydell wrote:
> >> On 12 February 2015 at 17:08, Andrew Jones <drjones@redhat.com> wrote:
> >> > Actually, I should point out that this isn't just a cleanup, but
> >> > also a fix. See below.
> >>
> >> > The original code didn't take into account that it may be calling check_ap
> >> > with a simple AP, AP[2:1].
> >>
> >> No, because check_ap() always takes AP[2:0]...
> >
> > No, it's really wrong. It's not the 2 vs. 3 bit issue that's the
> > problem, it's the cases. You snipped most of my reply to myself.
> > This part is pertinent
> >
> >> As a simple AP wouldn't be properly translated to protection flags with
> >> check_ap (except for case 6), then I think this should have caused some
> >> problems. Maybe this path just hasn't been tested? I don't see CR_AFE
> >> getting used by Linux, so possibly not.
> >
> > So, yes, a simple (3-bit) ap would be handled by the 8-case switch with
> > cases 0, 2, 4, and 6, but only case 6 would give the correct result.
> 
> Well, we didn't correctly support the simple permission model at all
> before your patches. But the point is that check_ap() always takes
> AP[2:0] regardless.
> 
> Hmm, or you could have check_simple_ap() be totally separate
> from check_ap(), and call it from inside the check in the v6
> code path for AFE that we already have.

Agreed. Creating a check_simple_ap function for the new switch added
by 2/5 would look better.

drew

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

[Andrew Jones]

On Thu, Feb 12, 2015 at 04:05:02PM +0100, Andrew Jones wrote:
> This series fixes and extends the determination of whether or
> not an address is executable for LPAE translations. The main
> patch is 4/5, and describes the details in its commit message.
> Patches 1-3 prepare for the main patch, and patch 5/5 is a
> code cleanup made possible by introducing a new helper function
> with the main patch.
> 
> This is actually a second version of [1], now based on Peter's
> recent translation regime support. However, as the series
> has changed substantially, I'm not calling this 'v2'. I believe
> I've addressed all of Peter's comments on that first posting in
> this refresh though.
> 
> 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 [2].
> 
> Thanks in advance for reviews!
> 
> drew
> 
> [1] http://lists.gnu.org/archive/html/qemu-devel/2015-01/msg01433.html
> [2] https://github.com/rhdrjones/kvm-unit-tests/commit/ee553e4bb795b0150e31c794bf8953dfb08d619a
> 
> Andrew Jones (5):
>   target-arm: convert check_ap to get_rw_prot
>   target-arm: enable get_rw_prot to take simple AP
>   target-arm: add an is_user param to get_rw_prot
>   target-arm: get_phys_addr_lpae: more xn control
>   target-arm: apply get_S1prot to get_phys_addr_v6
> 
>  target-arm/helper.c | 193 ++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 126 insertions(+), 67 deletions(-)
> 
> -- 
> 1.9.3
> 
> 
>

Ping? This isn't a huge priority, but I wouldn't want it to get lost either.

Thanks,
drew

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

[Peter Maydell]

On 25 February 2015 at 00:06, Andrew Jones <drjones@redhat.com> wrote:
> Ping? This isn't a huge priority, but I wouldn't want it to get lost either.

I'm on holiday :-) Easy stuff is getting done but things that
take time (read: code review) are postponed til mid-March.
You can wait til then, or find somebody else on list to review...

-- PMM

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

[Andrew Jones]

On Wed, Feb 25, 2015 at 12:08:16AM +0900, Peter Maydell wrote:
> On 25 February 2015 at 00:06, Andrew Jones <drjones@redhat.com> wrote:
> > Ping? This isn't a huge priority, but I wouldn't want it to get lost either.
> 
> I'm on holiday :-) Easy stuff is getting done but things that
> take time (read: code review) are postponed til mid-March.
> You can wait til then, or find somebody else on list to review...
> 
> -- PMM
>

As much activity as I've seen from you lately, I had no idea you were
on vacation :-) Enjoy!

I'm OK waiting until mid-March (of course other reviewers are welcome
to jump in too, I just don't have anybody specific in mind to cc atm.)

Thanks,
drew