diff mbox

[RFC,3/8] target-arm: Add support for S2 page-table protection bits

Message ID 1442672127-26223-4-git-send-email-edgar.iglesias@gmail.com
State New
Headers show

Commit Message

Edgar E. Iglesias Sept. 19, 2015, 2:15 p.m. UTC 
From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/helper.c | 48 +++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 45 insertions(+), 3 deletions(-)

Comments

Peter Maydell Sept. 23, 2015, 4:55 p.m. UTC | #1 
On 19 September 2015 at 07:15, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target-arm/helper.c | 48 +++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 45 insertions(+), 3 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 33be8c2..6f0ed51 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -6008,6 +6008,38 @@ simple_ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, int ap)
>      return simple_ap_to_rw_prot_is_user(ap, regime_is_user(env, mmu_idx));
>  }
>
> +/* Translate S2 section/page access permissions to protection flags
> + *
> + * @env:     CPUARMState
> + * @ap:      The 2-bit simple AP (AP[2:1])

I think this should read "The 2-bit stage2 access permissions (S2AP)".
The interpretation of the field differs from the simple-AP bits.
You could helpfully call the argument "s2ap" as well.

> + * @xn:      XN (execute-never) bit
> + */
> +static int get_S2prot(CPUARMState *env, int ap, int xn)
> +{
> +    int prot_rw;
> +
> +    switch (ap) {
> +    default:
> +    case 0:
> +        prot_rw = 0;
> +        break;
> +    case 1:
> +        prot_rw = PAGE_READ | PAGE_EXEC;
> +        break;
> +    case 2:
> +        prot_rw = PAGE_WRITE;
> +        break;
> +    case 3:
> +        prot_rw = PAGE_READ | PAGE_EXEC | PAGE_WRITE;
> +        break;
> +    }
> +
> +    if (xn) {
> +        prot_rw &= ~PAGE_EXEC;
> +    }

This isn't right -- the XN bit controls executability and the
S2AP bits don't affect it at all. I think you want:

    int prot_rw = 0;
    if (s2ap & 1) {
        prot_rw |= PAGE_READ;
    }
    if (s2ap & 2) {
        prot_rw |= PAGE_WRITE;
    }
    if (!xn) {
        prot_rw |= PAGE_EXEC;
    }


> +    return prot_rw;
> +}
> +
>  /* Translate section/page access permissions to protection flags
>   *
>   * @env:     CPUARMState
> @@ -6617,6 +6649,11 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>          /* Extract attributes from the descriptor and merge with table attrs */
>          attrs = extract64(descriptor, 2, 10)
>              | (extract64(descriptor, 52, 12) << 10);
> +
> +        if (mmu_idx == ARMMMUIdx_S2NS) {
> +            /* The following extractions do not apply to S2.  */

I think it would be clearer to say
  /* Stage 2 table descriptors do not include any attribute fields. */

> +            break;
> +        }

and then here put
      /* Merge in attributes from table descriptors */

and delete the now-redundant "and merge with table attrs" part from the
comment at the start of this hunk.

>          attrs |= extract32(tableattrs, 0, 2) << 11; /* XN, PXN */
>          attrs |= extract32(tableattrs, 3, 1) << 5; /* APTable[1] => AP[2] */
>          /* The sense of AP[1] vs APTable[0] is reversed, as APTable[0] == 1
> @@ -6638,11 +6675,16 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>      }
>
>      ap = extract32(attrs, 4, 2);
> -    ns = extract32(attrs, 3, 1);
>      xn = extract32(attrs, 12, 1);
> -    pxn = extract32(attrs, 11, 1);
>
> -    *prot = get_S1prot(env, mmu_idx, va_size == 64, ap, ns, xn, pxn);
> +    if (mmu_idx == ARMMMUIdx_S2NS) {
> +        ns = true;
> +        *prot = get_S2prot(env, ap, xn);
> +    } else {
> +        ns = extract32(attrs, 3, 1);
> +        pxn = extract32(attrs, 11, 1);
> +        *prot = get_S1prot(env, mmu_idx, va_size == 64, ap, ns, xn, pxn);
> +    }
>
>      fault_type = permission_fault;
>      if (!(*prot & (1 << access_type))) {
> --
> 1.9.1
>

thanks
-- PMM
Edgar E. Iglesias Oct. 1, 2015, 6:44 p.m. UTC | #2 
On Wed, Sep 23, 2015 at 09:55:05AM -0700, Peter Maydell wrote:
> On 19 September 2015 at 07:15, Edgar E. Iglesias
> <edgar.iglesias@gmail.com> wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  target-arm/helper.c | 48 +++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 45 insertions(+), 3 deletions(-)
> >
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index 33be8c2..6f0ed51 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -6008,6 +6008,38 @@ simple_ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, int ap)
> >      return simple_ap_to_rw_prot_is_user(ap, regime_is_user(env, mmu_idx));
> >  }
> >
> > +/* Translate S2 section/page access permissions to protection flags
> > + *
> > + * @env:     CPUARMState
> > + * @ap:      The 2-bit simple AP (AP[2:1])
> 
> I think this should read "The 2-bit stage2 access permissions (S2AP)".
> The interpretation of the field differs from the simple-AP bits.
> You could helpfully call the argument "s2ap" as well.
> 
> > + * @xn:      XN (execute-never) bit
> > + */
> > +static int get_S2prot(CPUARMState *env, int ap, int xn)
> > +{
> > +    int prot_rw;
> > +
> > +    switch (ap) {
> > +    default:
> > +    case 0:
> > +        prot_rw = 0;
> > +        break;
> > +    case 1:
> > +        prot_rw = PAGE_READ | PAGE_EXEC;
> > +        break;
> > +    case 2:
> > +        prot_rw = PAGE_WRITE;
> > +        break;
> > +    case 3:
> > +        prot_rw = PAGE_READ | PAGE_EXEC | PAGE_WRITE;
> > +        break;
> > +    }
> > +
> > +    if (xn) {
> > +        prot_rw &= ~PAGE_EXEC;
> > +    }
> 
> This isn't right -- the XN bit controls executability and the
> S2AP bits don't affect it at all. I think you want:
> 
>     int prot_rw = 0;
>     if (s2ap & 1) {
>         prot_rw |= PAGE_READ;
>     }
>     if (s2ap & 2) {
>         prot_rw |= PAGE_WRITE;
>     }
>     if (!xn) {
>         prot_rw |= PAGE_EXEC;
>     }


Thanks, this was the stuff I was worried about when we talked about it
last time. I've got the following now which seems to be the same as
you suggest:

static int get_S2prot(CPUARMState *env, int s2ap, int xn)
{
    int prot;

    prot = s2ap & 1 ? PAGE_READ : 0;
    prot |= s2ap & 2 ? PAGE_WRITE : 0;
    if (!xn) {
        prot |= PAGE_EXEC;
    }
    return prot;
}


I've also fixed up your other comments on this.

Thanks,
Edgar


> 
> 
> > +    return prot_rw;
> > +}
> > +
> >  /* Translate section/page access permissions to protection flags
> >   *
> >   * @env:     CPUARMState
> > @@ -6617,6 +6649,11 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
> >          /* Extract attributes from the descriptor and merge with table attrs */
> >          attrs = extract64(descriptor, 2, 10)
> >              | (extract64(descriptor, 52, 12) << 10);
> > +
> > +        if (mmu_idx == ARMMMUIdx_S2NS) {
> > +            /* The following extractions do not apply to S2.  */
> 
> I think it would be clearer to say
>   /* Stage 2 table descriptors do not include any attribute fields. */
> 
> > +            break;
> > +        }
> 
> and then here put
>       /* Merge in attributes from table descriptors */
> 
> and delete the now-redundant "and merge with table attrs" part from the
> comment at the start of this hunk.
> 
> >          attrs |= extract32(tableattrs, 0, 2) << 11; /* XN, PXN */
> >          attrs |= extract32(tableattrs, 3, 1) << 5; /* APTable[1] => AP[2] */
> >          /* The sense of AP[1] vs APTable[0] is reversed, as APTable[0] == 1
> > @@ -6638,11 +6675,16 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
> >      }
> >
> >      ap = extract32(attrs, 4, 2);
> > -    ns = extract32(attrs, 3, 1);
> >      xn = extract32(attrs, 12, 1);
> > -    pxn = extract32(attrs, 11, 1);
> >
> > -    *prot = get_S1prot(env, mmu_idx, va_size == 64, ap, ns, xn, pxn);
> > +    if (mmu_idx == ARMMMUIdx_S2NS) {
> > +        ns = true;
> > +        *prot = get_S2prot(env, ap, xn);
> > +    } else {
> > +        ns = extract32(attrs, 3, 1);
> > +        pxn = extract32(attrs, 11, 1);
> > +        *prot = get_S1prot(env, mmu_idx, va_size == 64, ap, ns, xn, pxn);
> > +    }
> >
> >      fault_type = permission_fault;
> >      if (!(*prot & (1 << access_type))) {
> > --
> > 1.9.1
> >
> 
> thanks
> -- PMM
Peter Maydell Oct. 1, 2015, 7:48 p.m. UTC | #3 
On 1 October 2015 at 19:44, Edgar E. Iglesias <edgar.iglesias@xilinx.com> wrote:
> On Wed, Sep 23, 2015 at 09:55:05AM -0700, Peter Maydell wrote:
>> This isn't right -- the XN bit controls executability and the
>> S2AP bits don't affect it at all. I think you want:
>>
>>     int prot_rw = 0;
>>     if (s2ap & 1) {
>>         prot_rw |= PAGE_READ;
>>     }
>>     if (s2ap & 2) {
>>         prot_rw |= PAGE_WRITE;
>>     }
>>     if (!xn) {
>>         prot_rw |= PAGE_EXEC;
>>     }
>
>
> Thanks, this was the stuff I was worried about when we talked about it
> last time. I've got the following now which seems to be the same as
> you suggest:
>
> static int get_S2prot(CPUARMState *env, int s2ap, int xn)
> {
>     int prot;
>
>     prot = s2ap & 1 ? PAGE_READ : 0;
>     prot |= s2ap & 2 ? PAGE_WRITE : 0;
>     if (!xn) {
>         prot |= PAGE_EXEC;
>     }
>     return prot;
> }

Yep, that's the right logic, though it seems a bit odd not
to consistently use either 'if' or '?:' for all 3 bits.

thanks
-- PMM
Edgar E. Iglesias Oct. 1, 2015, 7:52 p.m. UTC | #4 
On Thu, Oct 01, 2015 at 08:48:17PM +0100, Peter Maydell wrote:
> On 1 October 2015 at 19:44, Edgar E. Iglesias <edgar.iglesias@xilinx.com> wrote:
> > On Wed, Sep 23, 2015 at 09:55:05AM -0700, Peter Maydell wrote:
> >> This isn't right -- the XN bit controls executability and the
> >> S2AP bits don't affect it at all. I think you want:
> >>
> >>     int prot_rw = 0;
> >>     if (s2ap & 1) {
> >>         prot_rw |= PAGE_READ;
> >>     }
> >>     if (s2ap & 2) {
> >>         prot_rw |= PAGE_WRITE;
> >>     }
> >>     if (!xn) {
> >>         prot_rw |= PAGE_EXEC;
> >>     }
> >
> >
> > Thanks, this was the stuff I was worried about when we talked about it
> > last time. I've got the following now which seems to be the same as
> > you suggest:
> >
> > static int get_S2prot(CPUARMState *env, int s2ap, int xn)
> > {
> >     int prot;
> >
> >     prot = s2ap & 1 ? PAGE_READ : 0;
> >     prot |= s2ap & 2 ? PAGE_WRITE : 0;
> >     if (!xn) {
> >         prot |= PAGE_EXEC;
> >     }
> >     return prot;
> > }
> 
> Yep, that's the right logic, though it seems a bit odd not
> to consistently use either 'if' or '?:' for all 3 bits.

OK, that's true, I'll change it to all ifs.

Thanks,
Edgar
diff mbox

Patch

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 33be8c2..6f0ed51 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -6008,6 +6008,38 @@  simple_ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, int ap)
     return simple_ap_to_rw_prot_is_user(ap, regime_is_user(env, mmu_idx));
 }
 
+/* Translate S2 section/page access permissions to protection flags
+ *
+ * @env:     CPUARMState
+ * @ap:      The 2-bit simple AP (AP[2:1])
+ * @xn:      XN (execute-never) bit
+ */
+static int get_S2prot(CPUARMState *env, int ap, int xn)
+{
+    int prot_rw;
+
+    switch (ap) {
+    default:
+    case 0:
+        prot_rw = 0;
+        break;
+    case 1:
+        prot_rw = PAGE_READ | PAGE_EXEC;
+        break;
+    case 2:
+        prot_rw = PAGE_WRITE;
+        break;
+    case 3:
+        prot_rw = PAGE_READ | PAGE_EXEC | PAGE_WRITE;
+        break;
+    }
+
+    if (xn) {
+        prot_rw &= ~PAGE_EXEC;
+    }
+    return prot_rw;
+}
+
 /* Translate section/page access permissions to protection flags
  *
  * @env:     CPUARMState
@@ -6617,6 +6649,11 @@  static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
         /* Extract attributes from the descriptor and merge with table attrs */
         attrs = extract64(descriptor, 2, 10)
             | (extract64(descriptor, 52, 12) << 10);
+
+        if (mmu_idx == ARMMMUIdx_S2NS) {
+            /* The following extractions do not apply to S2.  */
+            break;
+        }
         attrs |= extract32(tableattrs, 0, 2) << 11; /* XN, PXN */
         attrs |= extract32(tableattrs, 3, 1) << 5; /* APTable[1] => AP[2] */
         /* The sense of AP[1] vs APTable[0] is reversed, as APTable[0] == 1
@@ -6638,11 +6675,16 @@  static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
     }
 
     ap = extract32(attrs, 4, 2);
-    ns = extract32(attrs, 3, 1);
     xn = extract32(attrs, 12, 1);
-    pxn = extract32(attrs, 11, 1);
 
-    *prot = get_S1prot(env, mmu_idx, va_size == 64, ap, ns, xn, pxn);
+    if (mmu_idx == ARMMMUIdx_S2NS) {
+        ns = true;
+        *prot = get_S2prot(env, ap, xn);
+    } else {
+        ns = extract32(attrs, 3, 1);
+        pxn = extract32(attrs, 11, 1);
+        *prot = get_S1prot(env, mmu_idx, va_size == 64, ap, ns, xn, pxn);
+    }
 
     fault_type = permission_fault;
     if (!(*prot & (1 << access_type))) {