[Qemu-devel] [PATCH v4 00/13] arm: Steps towards EL2 support round 5

[Qemu-devel] [PATCH v4 00/13] arm: Steps towards EL2 support round 5

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Hi,

Another round of patches towards EL2 support. This one adds partial
support for 2-stage MMU. The AArch32/ARMv7 support is untested.

Some of the details of error reporting are intentionally missing, I
was thinking to add those incrementally as they get quite involved
(e.g the register target and memory access size).

Comments welcome!

Best regards,
Edgar

v3 -> v4:
* Introduce inputsize to simplify and better match ref manuals
* Rename granule_sz to stride to better match ref manuals
* Add support for AArch32 negative S2 t0sz
* Add support for computing the AArch32 S2 PTW starting level.
* Add support for trapping on bad S2 starting levels

v2 -> v3:
* Prettify comments for ARMMMUFaultInfo
* Add S2 translation for 32bit S1 PTWs
* Add more comments to S2 PTW starting level computation.

v1 -> v2:
* Fix HPFAR_EL2 access checks
* Prettify computation of starting level for S2 PTW
* Improve description of ap argument to get_S2prot
* Fix EXEC protection in get_S2prot
* Improve comments on S2 PTW attribute extraction
* Add comment describing ARMMMUFaultInfo

Edgar E. Iglesias (13):
  target-arm: Add HPFAR_EL2
  target-arm: lpae: Make t0sz and t1sz signed integers
  target-arm: Add support for AArch32 S2 negative t0sz
  target-arm: lpae: Replace tsz with computed inputsize
  target-arm: lpae: Rename granule_sz to stride
  target-arm: Add computation of starting level for S2 PTW
  target-arm: Add support for S2 page-table protection bits
  target-arm: Avoid inline for get_phys_addr
  target-arm: Add ARMMMUFaultInfo
  target-arm: Add S2 translation to 64bit S1 PTWs
  target-arm: Add S2 translation to 32bit S1 PTWs
  target-arm: Route S2 MMU faults to EL2
  target-arm: Add support for S1 + S2 MMU translations

 target-arm/cpu.h       |   1 +
 target-arm/helper.c    | 375 ++++++++++++++++++++++++++++++++++++++++---------
 target-arm/internals.h |  40 +++++-
 target-arm/op_helper.c |  17 ++-
 4 files changed, 361 insertions(+), 72 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v4 01/13] target-arm: Add HPFAR_EL2

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/cpu.h    |  1 +
 target-arm/helper.c | 12 ++++++++++++
 2 files changed, 13 insertions(+)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 493f9d0..aaace9b 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -279,6 +279,7 @@ typedef struct CPUARMState {
             };
             uint64_t far_el[4];
         };
+        uint64_t hpfar_el2;
         union { /* Translation result. */
             struct {
                 uint64_t _unused_par_0;
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 8367997..5a5e5f0 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -3223,6 +3223,10 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
     { .name = "CNTHP_CTL_EL2", .state = ARM_CP_STATE_BOTH,
       .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 2, .opc2 = 1,
       .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+    { .name = "HPFAR_EL2", .state = ARM_CP_STATE_BOTH,
+      .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
+      .access = PL2_RW, .accessfn = access_el3_aa32ns_aa64any,
+      .type = ARM_CP_CONST, .resetvalue = 0 },
     REGINFO_SENTINEL
 };
 
@@ -3444,6 +3448,14 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
       .resetvalue = 0,
       .writefn = gt_hyp_ctl_write, .raw_writefn = raw_write },
 #endif
+    { .name = "HPFAR", .state = ARM_CP_STATE_AA32,
+      .cp = 15, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
+      .access = PL2_RW, .accessfn = access_el3_aa32ns,
+      .fieldoffset = offsetof(CPUARMState, cp15.hpfar_el2) },
+    { .name = "HPFAR_EL2", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
+      .access = PL2_RW,
+      .fieldoffset = offsetof(CPUARMState, cp15.hpfar_el2) },
     REGINFO_SENTINEL
 };
 
-- 
1.9.1

[Qemu-devel] [PATCH v4 02/13] target-arm: lpae: Make t0sz and t1sz signed integers

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Make t0sz and t1sz signed integers to match tsz and to make
it easier to implement support for AArch32 negative t0sz.
t1sz is changed for consistensy.

No functional change.

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

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 5a5e5f0..4e19838 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -6471,12 +6471,12 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
      * This is a Non-secure PL0/1 stage 1 translation, so controlled by
      * TTBCR/TTBR0/TTBR1 in accordance with ARM ARM DDI0406C table B-32:
      */
-    uint32_t t0sz = extract32(tcr->raw_tcr, 0, 6);
+    int32_t t0sz = extract32(tcr->raw_tcr, 0, 6);
     if (va_size == 64) {
         t0sz = MIN(t0sz, 39);
         t0sz = MAX(t0sz, 16);
     }
-    uint32_t t1sz = extract32(tcr->raw_tcr, 16, 6);
+    int32_t t1sz = extract32(tcr->raw_tcr, 16, 6);
     if (va_size == 64) {
         t1sz = MIN(t1sz, 39);
         t1sz = MAX(t1sz, 16);
-- 
1.9.1

Re: [Qemu-devel] [PATCH v4 02/13] target-arm: lpae: Make t0sz and t1sz signed integers

[Peter Maydell]

On 14 October 2015 at 23:55, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Make t0sz and t1sz signed integers to match tsz and to make
> it easier to implement support for AArch32 negative t0sz.
> t1sz is changed for consistensy.
>
> No functional change.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target-arm/helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

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

thanks
-- PMM

[Qemu-devel] [PATCH v4 03/13] target-arm: Add support for AArch32 S2 negative t0sz

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Add support for AArch32 S2 negative t0sz. In preparation for
using 40bit IPAs on AArch32.

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

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 4e19838..a8a46db 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -6475,6 +6475,17 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
     if (va_size == 64) {
         t0sz = MIN(t0sz, 39);
         t0sz = MAX(t0sz, 16);
+    } else {
+        bool sext = extract32(t0sz, 4, 1);
+        bool sign = extract32(t0sz, 3, 1);
+        t0sz = sextract32(t0sz, 0, 4);
+
+        /* If the sign-extend bit is not the same as t0sz[3], the result
+         * is unpredictable. Flag this as a guest error.  */
+        if (sign != sext) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "AArch32: VTCR.S / VTCR.T0SZ[3] missmatch\n");
+        }
     }
     int32_t t1sz = extract32(tcr->raw_tcr, 16, 6);
     if (va_size == 64) {
-- 
1.9.1

Re: [Qemu-devel] [PATCH v4 03/13] target-arm: Add support for AArch32 S2 negative t0sz

[Peter Maydell]

On 14 October 2015 at 23:55, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Add support for AArch32 S2 negative t0sz. In preparation for
> using 40bit IPAs on AArch32.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target-arm/helper.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 4e19838..a8a46db 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -6475,6 +6475,17 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>      if (va_size == 64) {
>          t0sz = MIN(t0sz, 39);
>          t0sz = MAX(t0sz, 16);
> +    } else {
> +        bool sext = extract32(t0sz, 4, 1);
> +        bool sign = extract32(t0sz, 3, 1);
> +        t0sz = sextract32(t0sz, 0, 4);
> +
> +        /* If the sign-extend bit is not the same as t0sz[3], the result
> +         * is unpredictable. Flag this as a guest error.  */
> +        if (sign != sext) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "AArch32: VTCR.S / VTCR.T0SZ[3] missmatch\n");
> +        }

Shouldn't this be guarded by a check on whether this is an s2
translation, since the 4-bit signed T0SZ and the S bit are only for
the VTCR, not for the normal TTBCRs ?

That is, we have 3 cases here for determining t0sz:
 * AArch64 6-bit unsigned field
 * AArch32 stage 1 3-bit unsigned field
 * AArch32 stage 2 4-bit signed field
so we need more than just a single if/else.

It's true that bits 3 and 4 are RES0 for TTBCR, but if we're
going to actually start logging guest errors here maybe we
should actually report the real problem (RES0 bits being set)
for that case.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 03/13] target-arm: Add support for AArch32 S2 negative t0sz

[Edgar E. Iglesias]

On Fri, Oct 23, 2015 at 04:29:35PM +0100, Peter Maydell wrote:
> On 14 October 2015 at 23:55, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > Add support for AArch32 S2 negative t0sz. In preparation for
> > using 40bit IPAs on AArch32.
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  target-arm/helper.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index 4e19838..a8a46db 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -6475,6 +6475,17 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
> >      if (va_size == 64) {
> >          t0sz = MIN(t0sz, 39);
> >          t0sz = MAX(t0sz, 16);
> > +    } else {
> > +        bool sext = extract32(t0sz, 4, 1);
> > +        bool sign = extract32(t0sz, 3, 1);
> > +        t0sz = sextract32(t0sz, 0, 4);
> > +
> > +        /* If the sign-extend bit is not the same as t0sz[3], the result
> > +         * is unpredictable. Flag this as a guest error.  */
> > +        if (sign != sext) {
> > +            qemu_log_mask(LOG_GUEST_ERROR,
> > +                          "AArch32: VTCR.S / VTCR.T0SZ[3] missmatch\n");
> > +        }
> 
> Shouldn't this be guarded by a check on whether this is an s2
> translation, since the 4-bit signed T0SZ and the S bit are only for
> the VTCR, not for the normal TTBCRs ?

Yes, sounds good. I've changed the patch to the following:

@@ -6521,8 +6521,24 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
      */
     int32_t t0sz = extract32(tcr->raw_tcr, 0, 6);
     if (va_size == 64) {
+        /* AArch64 translation.  */
         t0sz = MIN(t0sz, 39);
         t0sz = MAX(t0sz, 16);
+    } else if (mmu_idx != ARMMMUIdx_S2NS) {
+        /* AArch32 stage 1 translation.  */
+        t0sz = extract32(t0sz, 0, 3);
+    } else {
+        /* AArch32 stage 2 translation.  */
+        bool sext = extract32(t0sz, 4, 1);
+        bool sign = extract32(t0sz, 3, 1);
+        t0sz = sextract32(t0sz, 0, 4);
+
+        /* If the sign-extend bit is not the same as t0sz[3], the result
+         * is unpredictable. Flag this as a guest error.  */
+        if (sign != sext) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "AArch32: VTCR.S / VTCR.T0SZ[3] missmatch\n");
+        }
     }


We can also remove the error log and add more complete checks in future
patches if you prefer...

Cheers,
Edgar




> 
> That is, we have 3 cases here for determining t0sz:
>  * AArch64 6-bit unsigned field
>  * AArch32 stage 1 3-bit unsigned field
>  * AArch32 stage 2 4-bit signed field
> so we need more than just a single if/else.
> 
> It's true that bits 3 and 4 are RES0 for TTBCR, but if we're
> going to actually start logging guest errors here maybe we
> should actually report the real problem (RES0 bits being set)
> for that case.
> 
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH v4 03/13] target-arm: Add support for AArch32 S2 negative t0sz

[Peter Maydell]

On 26 October 2015 at 09:20, Edgar E. Iglesias
<edgar.iglesias@xilinx.com> wrote:
> Yes, sounds good. I've changed the patch to the following:
>
> @@ -6521,8 +6521,24 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>       */
>      int32_t t0sz = extract32(tcr->raw_tcr, 0, 6);
>      if (va_size == 64) {
> +        /* AArch64 translation.  */
>          t0sz = MIN(t0sz, 39);
>          t0sz = MAX(t0sz, 16);
> +    } else if (mmu_idx != ARMMMUIdx_S2NS) {
> +        /* AArch32 stage 1 translation.  */
> +        t0sz = extract32(t0sz, 0, 3);
> +    } else {
> +        /* AArch32 stage 2 translation.  */
> +        bool sext = extract32(t0sz, 4, 1);
> +        bool sign = extract32(t0sz, 3, 1);
> +        t0sz = sextract32(t0sz, 0, 4);
> +
> +        /* If the sign-extend bit is not the same as t0sz[3], the result
> +         * is unpredictable. Flag this as a guest error.  */
> +        if (sign != sext) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "AArch32: VTCR.S / VTCR.T0SZ[3] missmatch\n");
> +        }
>      }
>

Looks good, but maybe we should just do all the extracts
on tcr->raw_tcr, rather than extracting 6 bits of it and
then re-extracting some subset of bits from that extract
(for the 32-bit stage 1 case in particular it would be
simpler).

-- PMM

Re: [Qemu-devel] [PATCH v4 03/13] target-arm: Add support for AArch32 S2 negative t0sz

[Edgar E. Iglesias]

On Mon, Oct 26, 2015 at 09:52:12AM +0000, Peter Maydell wrote:
> On 26 October 2015 at 09:20, Edgar E. Iglesias
> <edgar.iglesias@xilinx.com> wrote:
> > Yes, sounds good. I've changed the patch to the following:
> >
> > @@ -6521,8 +6521,24 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
> >       */
> >      int32_t t0sz = extract32(tcr->raw_tcr, 0, 6);
> >      if (va_size == 64) {
> > +        /* AArch64 translation.  */
> >          t0sz = MIN(t0sz, 39);
> >          t0sz = MAX(t0sz, 16);
> > +    } else if (mmu_idx != ARMMMUIdx_S2NS) {
> > +        /* AArch32 stage 1 translation.  */
> > +        t0sz = extract32(t0sz, 0, 3);
> > +    } else {
> > +        /* AArch32 stage 2 translation.  */
> > +        bool sext = extract32(t0sz, 4, 1);
> > +        bool sign = extract32(t0sz, 3, 1);
> > +        t0sz = sextract32(t0sz, 0, 4);
> > +
> > +        /* If the sign-extend bit is not the same as t0sz[3], the result
> > +         * is unpredictable. Flag this as a guest error.  */
> > +        if (sign != sext) {
> > +            qemu_log_mask(LOG_GUEST_ERROR,
> > +                          "AArch32: VTCR.S / VTCR.T0SZ[3] missmatch\n");
> > +        }
> >      }
> >
> 
> Looks good, but maybe we should just do all the extracts
> on tcr->raw_tcr, rather than extracting 6 bits of it and
> then re-extracting some subset of bits from that extract
> (for the 32-bit stage 1 case in particular it would be
> simpler).

OK, I've rearranged the code a bit to use raw_tcr.

Thanks,
Edgar

[Qemu-devel] [PATCH v4 04/13] target-arm: lpae: Replace tsz with computed inputsize

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Remove the tsz variable and introduce inputsize.
This simplifies the code a little and makes it easier to
compare with the reference manuals.

No functional change.

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

diff --git a/target-arm/helper.c b/target-arm/helper.c
index a8a46db..9da50ab 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -6416,7 +6416,6 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
     MMUFaultType fault_type = translation_fault;
     uint32_t level = 1;
     uint32_t epd = 0;
-    int32_t tsz;
     uint32_t tg;
     uint64_t ttbr;
     int ttbr_select;
@@ -6426,6 +6425,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
     uint32_t attrs;
     int32_t granule_sz = 9;
     int32_t va_size = 32;
+    int inputsize;
     int32_t tbi = 0;
     TCR *tcr = regime_tcr(env, mmu_idx);
     int ap, ns, xn, pxn;
@@ -6523,7 +6523,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
         if (el < 2) {
             epd = extract32(tcr->raw_tcr, 7, 1);
         }
-        tsz = t0sz;
+        inputsize = va_size - t0sz;
 
         tg = extract32(tcr->raw_tcr, 14, 2);
         if (tg == 1) { /* 64KB pages */
@@ -6538,7 +6538,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
 
         ttbr = regime_ttbr(env, mmu_idx, 1);
         epd = extract32(tcr->raw_tcr, 23, 1);
-        tsz = t1sz;
+        inputsize = va_size - t1sz;
 
         tg = extract32(tcr->raw_tcr, 30, 2);
         if (tg == 3)  { /* 64KB pages */
@@ -6550,7 +6550,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
     }
 
     /* Here we should have set up all the parameters for the translation:
-     * va_size, ttbr, epd, tsz, granule_sz, tbi
+     * va_size, inputsize, ttbr, epd, granule_sz, tbi
      */
 
     if (epd) {
@@ -6565,27 +6565,27 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
      * of strides (granule_sz bits at a time) needed to consume the bits
      * of the input address. In the pseudocode this is:
      *  level = 4 - RoundUp((inputsize - grainsize) / stride)
-     * where their 'inputsize' is our 'va_size - tsz', 'grainsize' is
+     * where their 'inputsize' is our 'inputsize', 'grainsize' is
      * our 'granule_sz + 3' and 'stride' is our 'granule_sz'.
      * Applying the usual "rounded up m/n is (m+n-1)/n" and simplifying:
-     *     = 4 - (va_size - tsz - granule_sz - 3 + granule_sz - 1) / granule_sz
-     *     = 4 - (va_size - tsz - 4) / granule_sz;
+     *     = 4 - (inputsize - granule_sz - 3 + granule_sz - 1) / granule_sz
+     *     = 4 - (inputsize - 4) / granule_sz;
      */
-    level = 4 - (va_size - tsz - 4) / granule_sz;
+    level = 4 - (inputsize - 4) / granule_sz;
 
     /* Clear the vaddr bits which aren't part of the within-region address,
      * so that we don't have to special case things when calculating the
      * first descriptor address.
      */
-    if (tsz) {
-        address &= (1ULL << (va_size - tsz)) - 1;
+    if (va_size != inputsize) {
+        address &= (1ULL << inputsize) - 1;
     }
 
     descmask = (1ULL << (granule_sz + 3)) - 1;
 
     /* Now we can extract the actual base address from the TTBR */
     descaddr = extract64(ttbr, 0, 48);
-    descaddr &= ~((1ULL << (va_size - tsz - (granule_sz * (4 - level)))) - 1);
+    descaddr &= ~((1ULL << (inputsize - (granule_sz * (4 - level)))) - 1);
 
     /* Secure accesses start with the page table in secure memory and
      * can be downgraded to non-secure at any step. Non-secure accesses
-- 
1.9.1

Re: [Qemu-devel] [PATCH v4 04/13] target-arm: lpae: Replace tsz with computed inputsize

[Peter Maydell]

On 14 October 2015 at 23:55, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Remove the tsz variable and introduce inputsize.
> This simplifies the code a little and makes it easier to
> compare with the reference manuals.
>
> No functional change.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target-arm/helper.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)

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

thanks
-- PMM

[Qemu-devel] [PATCH v4 05/13] target-arm: lpae: Rename granule_sz to stride

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Rename granule_sz to stride to better match the reference manuals.

No functional change.

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

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 9da50ab..79b4c03 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -6423,7 +6423,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
     uint32_t tableattrs;
     target_ulong page_size;
     uint32_t attrs;
-    int32_t granule_sz = 9;
+    int32_t stride = 9;
     int32_t va_size = 32;
     int inputsize;
     int32_t tbi = 0;
@@ -6527,10 +6527,10 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
 
         tg = extract32(tcr->raw_tcr, 14, 2);
         if (tg == 1) { /* 64KB pages */
-            granule_sz = 13;
+            stride = 13;
         }
         if (tg == 2) { /* 16KB pages */
-            granule_sz = 11;
+            stride = 11;
         }
     } else {
         /* We should only be here if TTBR1 is valid */
@@ -6542,15 +6542,15 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
 
         tg = extract32(tcr->raw_tcr, 30, 2);
         if (tg == 3)  { /* 64KB pages */
-            granule_sz = 13;
+            stride = 13;
         }
         if (tg == 1) { /* 16KB pages */
-            granule_sz = 11;
+            stride = 11;
         }
     }
 
     /* Here we should have set up all the parameters for the translation:
-     * va_size, inputsize, ttbr, epd, granule_sz, tbi
+     * va_size, inputsize, ttbr, epd, stride, tbi
      */
 
     if (epd) {
@@ -6562,16 +6562,16 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
 
     /* The starting level depends on the virtual address size (which can be
      * up to 48 bits) and the translation granule size. It indicates the number
-     * of strides (granule_sz bits at a time) needed to consume the bits
+     * of strides (stride bits at a time) needed to consume the bits
      * of the input address. In the pseudocode this is:
      *  level = 4 - RoundUp((inputsize - grainsize) / stride)
      * where their 'inputsize' is our 'inputsize', 'grainsize' is
-     * our 'granule_sz + 3' and 'stride' is our 'granule_sz'.
+     * our 'stride + 3' and 'stride' is our 'stride'.
      * Applying the usual "rounded up m/n is (m+n-1)/n" and simplifying:
-     *     = 4 - (inputsize - granule_sz - 3 + granule_sz - 1) / granule_sz
-     *     = 4 - (inputsize - 4) / granule_sz;
+     *     = 4 - (inputsize - stride - 3 + stride - 1) / stride
+     *     = 4 - (inputsize - 4) / stride;
      */
-    level = 4 - (inputsize - 4) / granule_sz;
+    level = 4 - (inputsize - 4) / stride;
 
     /* Clear the vaddr bits which aren't part of the within-region address,
      * so that we don't have to special case things when calculating the
@@ -6581,11 +6581,11 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
         address &= (1ULL << inputsize) - 1;
     }
 
-    descmask = (1ULL << (granule_sz + 3)) - 1;
+    descmask = (1ULL << (stride + 3)) - 1;
 
     /* Now we can extract the actual base address from the TTBR */
     descaddr = extract64(ttbr, 0, 48);
-    descaddr &= ~((1ULL << (inputsize - (granule_sz * (4 - level)))) - 1);
+    descaddr &= ~((1ULL << (inputsize - (stride * (4 - level)))) - 1);
 
     /* Secure accesses start with the page table in secure memory and
      * can be downgraded to non-secure at any step. Non-secure accesses
@@ -6597,7 +6597,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
         uint64_t descriptor;
         bool nstable;
 
-        descaddr |= (address >> (granule_sz * (4 - level))) & descmask;
+        descaddr |= (address >> (stride * (4 - level))) & descmask;
         descaddr &= ~7ULL;
         nstable = extract32(tableattrs, 4, 1);
         descriptor = arm_ldq_ptw(cs, descaddr, !nstable);
@@ -6622,7 +6622,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
          * These are basically the same thing, although the number
          * of bits we pull in from the vaddr varies.
          */
-        page_size = (1ULL << ((granule_sz * (4 - level)) + 3));
+        page_size = (1ULL << ((stride * (4 - level)) + 3));
         descaddr |= (address & (page_size - 1));
         /* Extract attributes from the descriptor and merge with table attrs */
         attrs = extract64(descriptor, 2, 10)
-- 
1.9.1

Re: [Qemu-devel] [PATCH v4 05/13] target-arm: lpae: Rename granule_sz to stride

[Peter Maydell]

On 14 October 2015 at 23:55, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Rename granule_sz to stride to better match the reference manuals.
>
> No functional change.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target-arm/helper.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)

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

thanks
-- PMM

[Qemu-devel] [PATCH v4 06/13] target-arm: Add computation of starting level for S2 PTW

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

The starting level for S2 pagetable walks is computed
differently from the S1 starting level. Implement the S2
variant.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/helper.c    | 117 +++++++++++++++++++++++++++++++++++++++++++------
 target-arm/internals.h |  25 +++++++++++
 2 files changed, 129 insertions(+), 13 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 79b4c03..8530f7e 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -6406,12 +6406,72 @@ typedef enum {
     permission_fault = 3,
 } MMUFaultType;
 
+/*
+ * check_s2_startlevel
+ * @cpu:        ARMCPU
+ * @is_aa64:    True if the translation regime is in AArch64 state
+ * @startlevel: Suggested starting level
+ * @inputsize:  Bitsize of IPAs
+ * @stride:     Page-table stride (See the ARM ARM)
+ *
+ * Returns true if the suggested starting level is OK and false otherwise.
+ */
+static bool check_s2_startlevel(ARMCPU *cpu, bool is_aa64, int startlevel,
+                                int inputsize, int stride)
+{
+    /* Negative levels are never allowed.  */
+    if (startlevel < 0) {
+        return false;
+    }
+
+    if (is_aa64) {
+        unsigned int pamax = arm_pamax(cpu);
+
+        switch (stride) {
+        case 13: /* 64KB Pages.  */
+            if (startlevel < 1 || (startlevel == 0 && pamax <= 42)) {
+                return false;
+            }
+            break;
+        case 11: /* 16KB Pages.  */
+            if (startlevel < 1 || (startlevel == 0 && pamax <= 40)) {
+                return false;
+            }
+            break;
+        case 9: /* 4KB Pages.  */
+            if (startlevel == 0 && pamax <= 42) {
+                return false;
+            }
+            break;
+        default:
+            g_assert_not_reached();
+        }
+    } else {
+        const int grainsize = stride + 3;
+        int startsizecheck;
+
+        /* AArch32 only supports 4KB pages. Assert on that.  */
+        assert(stride == 9);
+
+        if (startlevel == 0) {
+            return false;
+        }
+
+        startsizecheck = inputsize - ((3 - startlevel) * stride + grainsize);
+        if (startsizecheck < 1 || startsizecheck > stride + 4) {
+            return false;
+        }
+    }
+    return true;
+}
+
 static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
                                int access_type, ARMMMUIdx mmu_idx,
                                hwaddr *phys_ptr, MemTxAttrs *txattrs, int *prot,
                                target_ulong *page_size_ptr, uint32_t *fsr)
 {
-    CPUState *cs = CPU(arm_env_get_cpu(env));
+    ARMCPU *cpu = arm_env_get_cpu(env);
+    CPUState *cs = CPU(cpu);
     /* Read an LPAE long-descriptor translation table. */
     MMUFaultType fault_type = translation_fault;
     uint32_t level = 1;
@@ -6560,18 +6620,49 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
         goto do_fault;
     }
 
-    /* The starting level depends on the virtual address size (which can be
-     * up to 48 bits) and the translation granule size. It indicates the number
-     * of strides (stride bits at a time) needed to consume the bits
-     * of the input address. In the pseudocode this is:
-     *  level = 4 - RoundUp((inputsize - grainsize) / stride)
-     * where their 'inputsize' is our 'inputsize', 'grainsize' is
-     * our 'stride + 3' and 'stride' is our 'stride'.
-     * Applying the usual "rounded up m/n is (m+n-1)/n" and simplifying:
-     *     = 4 - (inputsize - stride - 3 + stride - 1) / stride
-     *     = 4 - (inputsize - 4) / stride;
-     */
-    level = 4 - (inputsize - 4) / stride;
+    if (mmu_idx != ARMMMUIdx_S2NS) {
+        /* The starting level depends on the virtual address size (which can
+         * be up to 48 bits) and the translation granule size. It indicates
+         * the number of strides (stride bits at a time) needed to
+         * consume the bits of the input address. In the pseudocode this is:
+         *  level = 4 - RoundUp((inputsize - grainsize) / stride)
+         * where their 'inputsize' is our 'inputsize', 'grainsize' is
+         * our 'stride + 3' and 'stride' is our 'stride'.
+         * Applying the usual "rounded up m/n is (m+n-1)/n" and simplifying:
+         * = 4 - (inputsize - stride - 3 + stride - 1) / stride
+         * = 4 - (inputsize - 4) / stride;
+         */
+        level = 4 - (inputsize - 4) / stride;
+    } else {
+        /* For stage 2 translations the starting level is specified by the
+         * VCTR_EL2.SL0 field (whose interpretation depends on the page size)
+         */
+        int startlevel = extract32(tcr->raw_tcr, 6, 2);
+        bool ok;
+
+        if (va_size == 32 || stride == 9) {
+            /* AArch32 or 4KB pages */
+            startlevel = 2 - startlevel;
+        } else {
+            /* 16KB or 64KB pages */
+            startlevel = 3 - startlevel;
+        }
+
+        /* Check that the starting level is valid. */
+        ok = check_s2_startlevel(cpu, va_size == 64, startlevel,
+                                 inputsize, stride);
+        if (!ok) {
+            /* AArch64 reports these as level 0 faults.
+             * AArch32 reports these as level 1 faults.
+             */
+            level = va_size == 64 ? 0 : 1;
+            fault_type = translation_fault;
+            goto do_fault;
+        }
+
+        /* The starting level looks good, use it.  */
+        level = startlevel;
+    }
 
     /* Clear the vaddr bits which aren't part of the within-region address,
      * so that we don't have to special case things when calculating the
diff --git a/target-arm/internals.h b/target-arm/internals.h
index 36a56aa..8bd37eb 100644
--- a/target-arm/internals.h
+++ b/target-arm/internals.h
@@ -152,6 +152,31 @@ static inline void update_spsel(CPUARMState *env, uint32_t imm)
     aarch64_restore_sp(env, cur_el);
 }
 
+/*
+ * arm_pamax
+ * @cpu: ARMCPU
+ *
+ * Returns the implementation defined bit-width of physical addresses.
+ * The ARMv8 reference manuals refer to this as PAMax().
+ */
+static inline unsigned int arm_pamax(ARMCPU *cpu)
+{
+    static const unsigned int pamax_map[] = {
+        [0] = 32,
+        [1] = 36,
+        [2] = 40,
+        [3] = 42,
+        [4] = 44,
+        [5] = 48,
+    };
+    unsigned int parange = extract32(cpu->id_aa64mmfr0, 0, 4);
+
+    /* id_aa64mmfr0 is a read-only register so values outside of the
+     * supported mappings can be considered an implementation error.  */
+    assert(parange < ARRAY_SIZE(pamax_map));
+    return pamax_map[parange];
+}
+
 /* Return true if extended addresses are enabled.
  * This is always the case if our translation regime is 64 bit,
  * but depends on TTBCR.EAE for 32 bit.
-- 
1.9.1

Re: [Qemu-devel] [PATCH v4 06/13] target-arm: Add computation of starting level for S2 PTW

[Peter Maydell]

On 14 October 2015 at 23:55, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> The starting level for S2 pagetable walks is computed
> differently from the S1 starting level. Implement the S2
> variant.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target-arm/helper.c    | 117 +++++++++++++++++++++++++++++++++++++++++++------
>  target-arm/internals.h |  25 +++++++++++
>  2 files changed, 129 insertions(+), 13 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 79b4c03..8530f7e 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -6406,12 +6406,72 @@ typedef enum {
>      permission_fault = 3,
>  } MMUFaultType;
>
> +/*
> + * check_s2_startlevel
> + * @cpu:        ARMCPU
> + * @is_aa64:    True if the translation regime is in AArch64 state
> + * @startlevel: Suggested starting level
> + * @inputsize:  Bitsize of IPAs
> + * @stride:     Page-table stride (See the ARM ARM)
> + *
> + * Returns true if the suggested starting level is OK and false otherwise.
> + */
> +static bool check_s2_startlevel(ARMCPU *cpu, bool is_aa64, int startlevel,
> +                                int inputsize, int stride)
> +{
> +    /* Negative levels are never allowed.  */
> +    if (startlevel < 0) {
> +        return false;
> +    }
> +
> +    if (is_aa64) {
> +        unsigned int pamax = arm_pamax(cpu);
> +
> +        switch (stride) {
> +        case 13: /* 64KB Pages.  */
> +            if (startlevel < 1 || (startlevel == 0 && pamax <= 42)) {
> +                return false;
> +            }

I'm having trouble matching these up with the ARM ARM pseudocode,
which says for this case for instance
   if (level == 0 || (level == 1 && PAMax() <= 42)) then basefound = FALSE;

(as an aside, the pseudocode uses 'startlevel' for the raw SL0
field value and 'level' for the 3 - startlevel (or 2 - startlevel)
value, so it's a bit confusing to use startlevel for both here.)

> +            break;
> +        case 11: /* 16KB Pages.  */
> +            if (startlevel < 1 || (startlevel == 0 && pamax <= 40)) {
> +                return false;
> +            }
> +            break;
> +        case 9: /* 4KB Pages.  */
> +            if (startlevel == 0 && pamax <= 42) {
> +                return false;
> +            }
> +            break;
> +        default:
> +            g_assert_not_reached();
> +        }
> +    } else {
> +        const int grainsize = stride + 3;
> +        int startsizecheck;
> +
> +        /* AArch32 only supports 4KB pages. Assert on that.  */
> +        assert(stride == 9);
> +
> +        if (startlevel == 0) {
> +            return false;
> +        }
> +
> +        startsizecheck = inputsize - ((3 - startlevel) * stride + grainsize);
> +        if (startsizecheck < 1 || startsizecheck > stride + 4) {
> +            return false;
> +        }
> +    }
> +    return true;
> +}
> +
>  static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>                                 int access_type, ARMMMUIdx mmu_idx,
>                                 hwaddr *phys_ptr, MemTxAttrs *txattrs, int *prot,
>                                 target_ulong *page_size_ptr, uint32_t *fsr)
>  {
> -    CPUState *cs = CPU(arm_env_get_cpu(env));
> +    ARMCPU *cpu = arm_env_get_cpu(env);
> +    CPUState *cs = CPU(cpu);
>      /* Read an LPAE long-descriptor translation table. */
>      MMUFaultType fault_type = translation_fault;
>      uint32_t level = 1;
> @@ -6560,18 +6620,49 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>          goto do_fault;
>      }
>
> -    /* The starting level depends on the virtual address size (which can be
> -     * up to 48 bits) and the translation granule size. It indicates the number
> -     * of strides (stride bits at a time) needed to consume the bits
> -     * of the input address. In the pseudocode this is:
> -     *  level = 4 - RoundUp((inputsize - grainsize) / stride)
> -     * where their 'inputsize' is our 'inputsize', 'grainsize' is
> -     * our 'stride + 3' and 'stride' is our 'stride'.
> -     * Applying the usual "rounded up m/n is (m+n-1)/n" and simplifying:
> -     *     = 4 - (inputsize - stride - 3 + stride - 1) / stride
> -     *     = 4 - (inputsize - 4) / stride;
> -     */
> -    level = 4 - (inputsize - 4) / stride;
> +    if (mmu_idx != ARMMMUIdx_S2NS) {
> +        /* The starting level depends on the virtual address size (which can
> +         * be up to 48 bits) and the translation granule size. It indicates
> +         * the number of strides (stride bits at a time) needed to
> +         * consume the bits of the input address. In the pseudocode this is:
> +         *  level = 4 - RoundUp((inputsize - grainsize) / stride)
> +         * where their 'inputsize' is our 'inputsize', 'grainsize' is
> +         * our 'stride + 3' and 'stride' is our 'stride'.
> +         * Applying the usual "rounded up m/n is (m+n-1)/n" and simplifying:
> +         * = 4 - (inputsize - stride - 3 + stride - 1) / stride
> +         * = 4 - (inputsize - 4) / stride;
> +         */
> +        level = 4 - (inputsize - 4) / stride;
> +    } else {
> +        /* For stage 2 translations the starting level is specified by the
> +         * VCTR_EL2.SL0 field (whose interpretation depends on the page size)

VTCR_EL2.

> +         */
> +        int startlevel = extract32(tcr->raw_tcr, 6, 2);
> +        bool ok;
> +
> +        if (va_size == 32 || stride == 9) {
> +            /* AArch32 or 4KB pages */
> +            startlevel = 2 - startlevel;
> +        } else {
> +            /* 16KB or 64KB pages */
> +            startlevel = 3 - startlevel;
> +        }
> +
> +        /* Check that the starting level is valid. */
> +        ok = check_s2_startlevel(cpu, va_size == 64, startlevel,
> +                                 inputsize, stride);
> +        if (!ok) {
> +            /* AArch64 reports these as level 0 faults.
> +             * AArch32 reports these as level 1 faults.
> +             */
> +            level = va_size == 64 ? 0 : 1;
> +            fault_type = translation_fault;
> +            goto do_fault;
> +        }
> +
> +        /* The starting level looks good, use it.  */
> +        level = startlevel;
> +    }
>
>      /* Clear the vaddr bits which aren't part of the within-region address,
>       * so that we don't have to special case things when calculating the
> diff --git a/target-arm/internals.h b/target-arm/internals.h
> index 36a56aa..8bd37eb 100644
> --- a/target-arm/internals.h
> +++ b/target-arm/internals.h
> @@ -152,6 +152,31 @@ static inline void update_spsel(CPUARMState *env, uint32_t imm)
>      aarch64_restore_sp(env, cur_el);
>  }
>
> +/*
> + * arm_pamax
> + * @cpu: ARMCPU
> + *
> + * Returns the implementation defined bit-width of physical addresses.
> + * The ARMv8 reference manuals refer to this as PAMax().
> + */
> +static inline unsigned int arm_pamax(ARMCPU *cpu)
> +{
> +    static const unsigned int pamax_map[] = {
> +        [0] = 32,
> +        [1] = 36,
> +        [2] = 40,
> +        [3] = 42,
> +        [4] = 44,
> +        [5] = 48,
> +    };
> +    unsigned int parange = extract32(cpu->id_aa64mmfr0, 0, 4);
> +
> +    /* id_aa64mmfr0 is a read-only register so values outside of the
> +     * supported mappings can be considered an implementation error.  */
> +    assert(parange < ARRAY_SIZE(pamax_map));
> +    return pamax_map[parange];
> +}
> +
>  /* Return true if extended addresses are enabled.
>   * This is always the case if our translation regime is 64 bit,
>   * but depends on TTBCR.EAE for 32 bit.
> --
> 1.9.1
>

looks ok otherwise

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 06/13] target-arm: Add computation of starting level for S2 PTW

[Edgar E. Iglesias]

On Fri, Oct 23, 2015 at 05:26:52PM +0100, Peter Maydell wrote:
> On 14 October 2015 at 23:55, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > The starting level for S2 pagetable walks is computed
> > differently from the S1 starting level. Implement the S2
> > variant.
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  target-arm/helper.c    | 117 +++++++++++++++++++++++++++++++++++++++++++------
> >  target-arm/internals.h |  25 +++++++++++
> >  2 files changed, 129 insertions(+), 13 deletions(-)
> >
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index 79b4c03..8530f7e 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -6406,12 +6406,72 @@ typedef enum {
> >      permission_fault = 3,
> >  } MMUFaultType;
> >
> > +/*
> > + * check_s2_startlevel
> > + * @cpu:        ARMCPU
> > + * @is_aa64:    True if the translation regime is in AArch64 state
> > + * @startlevel: Suggested starting level
> > + * @inputsize:  Bitsize of IPAs
> > + * @stride:     Page-table stride (See the ARM ARM)
> > + *
> > + * Returns true if the suggested starting level is OK and false otherwise.
> > + */
> > +static bool check_s2_startlevel(ARMCPU *cpu, bool is_aa64, int startlevel,
> > +                                int inputsize, int stride)
> > +{
> > +    /* Negative levels are never allowed.  */
> > +    if (startlevel < 0) {
> > +        return false;
> > +    }
> > +
> > +    if (is_aa64) {
> > +        unsigned int pamax = arm_pamax(cpu);
> > +
> > +        switch (stride) {
> > +        case 13: /* 64KB Pages.  */
> > +            if (startlevel < 1 || (startlevel == 0 && pamax <= 42)) {
> > +                return false;
> > +            }
> 
> I'm having trouble matching these up with the ARM ARM pseudocode,
> which says for this case for instance
>    if (level == 0 || (level == 1 && PAMax() <= 42)) then basefound = FALSE;

Not sure what I was thinking... I've rewritten all of it to match the
specs (I hope).

> 
> (as an aside, the pseudocode uses 'startlevel' for the raw SL0
> field value and 'level' for the 3 - startlevel (or 2 - startlevel)
> value, so it's a bit confusing to use startlevel for both here.)


Yes, I've changed the naming of to better match specs.

Thanks!
Edgar


> 
> > +            break;
> > +        case 11: /* 16KB Pages.  */
> > +            if (startlevel < 1 || (startlevel == 0 && pamax <= 40)) {
> > +                return false;
> > +            }
> > +            break;
> > +        case 9: /* 4KB Pages.  */
> > +            if (startlevel == 0 && pamax <= 42) {
> > +                return false;
> > +            }
> > +            break;
> > +        default:
> > +            g_assert_not_reached();
> > +        }
> > +    } else {
> > +        const int grainsize = stride + 3;
> > +        int startsizecheck;
> > +
> > +        /* AArch32 only supports 4KB pages. Assert on that.  */
> > +        assert(stride == 9);
> > +
> > +        if (startlevel == 0) {
> > +            return false;
> > +        }
> > +
> > +        startsizecheck = inputsize - ((3 - startlevel) * stride + grainsize);
> > +        if (startsizecheck < 1 || startsizecheck > stride + 4) {
> > +            return false;
> > +        }
> > +    }
> > +    return true;
> > +}
> > +
> >  static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
> >                                 int access_type, ARMMMUIdx mmu_idx,
> >                                 hwaddr *phys_ptr, MemTxAttrs *txattrs, int *prot,
> >                                 target_ulong *page_size_ptr, uint32_t *fsr)
> >  {
> > -    CPUState *cs = CPU(arm_env_get_cpu(env));
> > +    ARMCPU *cpu = arm_env_get_cpu(env);
> > +    CPUState *cs = CPU(cpu);
> >      /* Read an LPAE long-descriptor translation table. */
> >      MMUFaultType fault_type = translation_fault;
> >      uint32_t level = 1;
> > @@ -6560,18 +6620,49 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
> >          goto do_fault;
> >      }
> >
> > -    /* The starting level depends on the virtual address size (which can be
> > -     * up to 48 bits) and the translation granule size. It indicates the number
> > -     * of strides (stride bits at a time) needed to consume the bits
> > -     * of the input address. In the pseudocode this is:
> > -     *  level = 4 - RoundUp((inputsize - grainsize) / stride)
> > -     * where their 'inputsize' is our 'inputsize', 'grainsize' is
> > -     * our 'stride + 3' and 'stride' is our 'stride'.
> > -     * Applying the usual "rounded up m/n is (m+n-1)/n" and simplifying:
> > -     *     = 4 - (inputsize - stride - 3 + stride - 1) / stride
> > -     *     = 4 - (inputsize - 4) / stride;
> > -     */
> > -    level = 4 - (inputsize - 4) / stride;
> > +    if (mmu_idx != ARMMMUIdx_S2NS) {
> > +        /* The starting level depends on the virtual address size (which can
> > +         * be up to 48 bits) and the translation granule size. It indicates
> > +         * the number of strides (stride bits at a time) needed to
> > +         * consume the bits of the input address. In the pseudocode this is:
> > +         *  level = 4 - RoundUp((inputsize - grainsize) / stride)
> > +         * where their 'inputsize' is our 'inputsize', 'grainsize' is
> > +         * our 'stride + 3' and 'stride' is our 'stride'.
> > +         * Applying the usual "rounded up m/n is (m+n-1)/n" and simplifying:
> > +         * = 4 - (inputsize - stride - 3 + stride - 1) / stride
> > +         * = 4 - (inputsize - 4) / stride;
> > +         */
> > +        level = 4 - (inputsize - 4) / stride;
> > +    } else {
> > +        /* For stage 2 translations the starting level is specified by the
> > +         * VCTR_EL2.SL0 field (whose interpretation depends on the page size)
> 
> VTCR_EL2.
> 
> > +         */
> > +        int startlevel = extract32(tcr->raw_tcr, 6, 2);
> > +        bool ok;
> > +
> > +        if (va_size == 32 || stride == 9) {
> > +            /* AArch32 or 4KB pages */
> > +            startlevel = 2 - startlevel;
> > +        } else {
> > +            /* 16KB or 64KB pages */
> > +            startlevel = 3 - startlevel;
> > +        }
> > +
> > +        /* Check that the starting level is valid. */
> > +        ok = check_s2_startlevel(cpu, va_size == 64, startlevel,
> > +                                 inputsize, stride);
> > +        if (!ok) {
> > +            /* AArch64 reports these as level 0 faults.
> > +             * AArch32 reports these as level 1 faults.
> > +             */
> > +            level = va_size == 64 ? 0 : 1;
> > +            fault_type = translation_fault;
> > +            goto do_fault;
> > +        }
> > +
> > +        /* The starting level looks good, use it.  */
> > +        level = startlevel;
> > +    }
> >
> >      /* Clear the vaddr bits which aren't part of the within-region address,
> >       * so that we don't have to special case things when calculating the
> > diff --git a/target-arm/internals.h b/target-arm/internals.h
> > index 36a56aa..8bd37eb 100644
> > --- a/target-arm/internals.h
> > +++ b/target-arm/internals.h
> > @@ -152,6 +152,31 @@ static inline void update_spsel(CPUARMState *env, uint32_t imm)
> >      aarch64_restore_sp(env, cur_el);
> >  }
> >
> > +/*
> > + * arm_pamax
> > + * @cpu: ARMCPU
> > + *
> > + * Returns the implementation defined bit-width of physical addresses.
> > + * The ARMv8 reference manuals refer to this as PAMax().
> > + */
> > +static inline unsigned int arm_pamax(ARMCPU *cpu)
> > +{
> > +    static const unsigned int pamax_map[] = {
> > +        [0] = 32,
> > +        [1] = 36,
> > +        [2] = 40,
> > +        [3] = 42,
> > +        [4] = 44,
> > +        [5] = 48,
> > +    };
> > +    unsigned int parange = extract32(cpu->id_aa64mmfr0, 0, 4);
> > +
> > +    /* id_aa64mmfr0 is a read-only register so values outside of the
> > +     * supported mappings can be considered an implementation error.  */
> > +    assert(parange < ARRAY_SIZE(pamax_map));
> > +    return pamax_map[parange];
> > +}
> > +
> >  /* Return true if extended addresses are enabled.
> >   * This is always the case if our translation regime is 64 bit,
> >   * but depends on TTBCR.EAE for 32 bit.
> > --
> > 1.9.1
> >
> 
> looks ok otherwise
> 
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH v4 06/13] target-arm: Add computation of starting level for S2 PTW

[Edgar E. Iglesias]

On Fri, Oct 23, 2015 at 05:26:52PM +0100, Peter Maydell wrote:
> On 14 October 2015 at 23:55, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > The starting level for S2 pagetable walks is computed
> > differently from the S1 starting level. Implement the S2
> > variant.
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  target-arm/helper.c    | 117 +++++++++++++++++++++++++++++++++++++++++++------
> >  target-arm/internals.h |  25 +++++++++++
> >  2 files changed, 129 insertions(+), 13 deletions(-)
> >
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index 79b4c03..8530f7e 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -6406,12 +6406,72 @@ typedef enum {
> >      permission_fault = 3,
> >  } MMUFaultType;
> >
> > +/*
> > + * check_s2_startlevel
> > + * @cpu:        ARMCPU
> > + * @is_aa64:    True if the translation regime is in AArch64 state
> > + * @startlevel: Suggested starting level
> > + * @inputsize:  Bitsize of IPAs
> > + * @stride:     Page-table stride (See the ARM ARM)
> > + *
> > + * Returns true if the suggested starting level is OK and false otherwise.
> > + */
> > +static bool check_s2_startlevel(ARMCPU *cpu, bool is_aa64, int startlevel,
> > +                                int inputsize, int stride)
> > +{
> > +    /* Negative levels are never allowed.  */
> > +    if (startlevel < 0) {
> > +        return false;
> > +    }
> > +
> > +    if (is_aa64) {
> > +        unsigned int pamax = arm_pamax(cpu);
> > +
> > +        switch (stride) {
> > +        case 13: /* 64KB Pages.  */
> > +            if (startlevel < 1 || (startlevel == 0 && pamax <= 42)) {
> > +                return false;
> > +            }
> 
> I'm having trouble matching these up with the ARM ARM pseudocode,
> which says for this case for instance
>    if (level == 0 || (level == 1 && PAMax() <= 42)) then basefound = FALSE;
> 
> (as an aside, the pseudocode uses 'startlevel' for the raw SL0
> field value and 'level' for the 3 - startlevel (or 2 - startlevel)
> value, so it's a bit confusing to use startlevel for both here.)
> 
> > +            break;
> > +        case 11: /* 16KB Pages.  */
> > +            if (startlevel < 1 || (startlevel == 0 && pamax <= 40)) {
> > +                return false;
> > +            }
> > +            break;
> > +        case 9: /* 4KB Pages.  */
> > +            if (startlevel == 0 && pamax <= 42) {
> > +                return false;
> > +            }
> > +            break;
> > +        default:
> > +            g_assert_not_reached();
> > +        }
> > +    } else {
> > +        const int grainsize = stride + 3;
> > +        int startsizecheck;
> > +
> > +        /* AArch32 only supports 4KB pages. Assert on that.  */
> > +        assert(stride == 9);
> > +
> > +        if (startlevel == 0) {
> > +            return false;
> > +        }
> > +
> > +        startsizecheck = inputsize - ((3 - startlevel) * stride + grainsize);
> > +        if (startsizecheck < 1 || startsizecheck > stride + 4) {
> > +            return false;
> > +        }
> > +    }
> > +    return true;
> > +}
> > +
> >  static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
> >                                 int access_type, ARMMMUIdx mmu_idx,
> >                                 hwaddr *phys_ptr, MemTxAttrs *txattrs, int *prot,
> >                                 target_ulong *page_size_ptr, uint32_t *fsr)
> >  {
> > -    CPUState *cs = CPU(arm_env_get_cpu(env));
> > +    ARMCPU *cpu = arm_env_get_cpu(env);
> > +    CPUState *cs = CPU(cpu);
> >      /* Read an LPAE long-descriptor translation table. */
> >      MMUFaultType fault_type = translation_fault;
> >      uint32_t level = 1;
> > @@ -6560,18 +6620,49 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
> >          goto do_fault;
> >      }
> >
> > -    /* The starting level depends on the virtual address size (which can be
> > -     * up to 48 bits) and the translation granule size. It indicates the number
> > -     * of strides (stride bits at a time) needed to consume the bits
> > -     * of the input address. In the pseudocode this is:
> > -     *  level = 4 - RoundUp((inputsize - grainsize) / stride)
> > -     * where their 'inputsize' is our 'inputsize', 'grainsize' is
> > -     * our 'stride + 3' and 'stride' is our 'stride'.
> > -     * Applying the usual "rounded up m/n is (m+n-1)/n" and simplifying:
> > -     *     = 4 - (inputsize - stride - 3 + stride - 1) / stride
> > -     *     = 4 - (inputsize - 4) / stride;
> > -     */
> > -    level = 4 - (inputsize - 4) / stride;
> > +    if (mmu_idx != ARMMMUIdx_S2NS) {
> > +        /* The starting level depends on the virtual address size (which can
> > +         * be up to 48 bits) and the translation granule size. It indicates
> > +         * the number of strides (stride bits at a time) needed to
> > +         * consume the bits of the input address. In the pseudocode this is:
> > +         *  level = 4 - RoundUp((inputsize - grainsize) / stride)
> > +         * where their 'inputsize' is our 'inputsize', 'grainsize' is
> > +         * our 'stride + 3' and 'stride' is our 'stride'.
> > +         * Applying the usual "rounded up m/n is (m+n-1)/n" and simplifying:
> > +         * = 4 - (inputsize - stride - 3 + stride - 1) / stride
> > +         * = 4 - (inputsize - 4) / stride;
> > +         */
> > +        level = 4 - (inputsize - 4) / stride;
> > +    } else {
> > +        /* For stage 2 translations the starting level is specified by the
> > +         * VCTR_EL2.SL0 field (whose interpretation depends on the page size)
> 
> VTCR_EL2.

Fixed

Thanks!
Edgar

[Qemu-devel] [PATCH v4 07/13] target-arm: Add support for S2 page-table protection bits

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

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

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 8530f7e..d1ffcdf 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -6015,6 +6015,28 @@ 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
+ * @s2ap:    The 2-bit stage2 access permissions (S2AP)
+ * @xn:      XN (execute-never) bit
+ */
+static int get_S2prot(CPUARMState *env, int s2ap, int xn)
+{
+    int prot = 0;
+
+    if (s2ap & 1) {
+        prot |= PAGE_READ;
+    }
+    if (s2ap & 2) {
+        prot |= PAGE_WRITE;
+    }
+    if (!xn) {
+        prot |= PAGE_EXEC;
+    }
+    return prot;
+}
+
 /* Translate section/page access permissions to protection flags
  *
  * @env:     CPUARMState
@@ -6715,9 +6737,15 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
          */
         page_size = (1ULL << ((stride * (4 - level)) + 3));
         descaddr |= (address & (page_size - 1));
-        /* Extract attributes from the descriptor and merge with table attrs */
+        /* Extract attributes from the descriptor */
         attrs = extract64(descriptor, 2, 10)
             | (extract64(descriptor, 52, 12) << 10);
+
+        if (mmu_idx == ARMMMUIdx_S2NS) {
+            /* Stage 2 table descriptors do not include any attribute fields */
+            break;
+        }
+        /* Merge in attributes from table descriptors */
         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
@@ -6739,11 +6767,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

Re: [Qemu-devel] [PATCH v4 07/13] target-arm: Add support for S2 page-table protection bits

[Peter Maydell]

On 14 October 2015 at 23:55, 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 | 41 +++++++++++++++++++++++++++++++++++++----
>  1 file changed, 37 insertions(+), 4 deletions(-)
>

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

thanks
-- PMM

[Qemu-devel] [PATCH v4 08/13] target-arm: Avoid inline for get_phys_addr

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Avoid inline for get_phys_addr() to prepare for future recursive use.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/helper.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index d1ffcdf..17d0590 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -15,10 +15,10 @@
 #define ARM_CPU_FREQ 1000000000 /* FIXME: 1 GHz, should be configurable */
 
 #ifndef CONFIG_USER_ONLY
-static inline bool get_phys_addr(CPUARMState *env, target_ulong address,
-                                 int access_type, ARMMMUIdx mmu_idx,
-                                 hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
-                                 target_ulong *page_size, uint32_t *fsr);
+static bool get_phys_addr(CPUARMState *env, target_ulong address,
+                          int access_type, ARMMMUIdx mmu_idx,
+                          hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
+                          target_ulong *page_size, uint32_t *fsr);
 
 /* Definitions for the PMCCNTR and PMCR registers */
 #define PMCRD   0x8
@@ -7059,10 +7059,10 @@ static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
  * @page_size: set to the size of the page containing phys_ptr
  * @fsr: set to the DFSR/IFSR value on failure
  */
-static inline bool get_phys_addr(CPUARMState *env, target_ulong address,
-                                 int access_type, ARMMMUIdx mmu_idx,
-                                 hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
-                                 target_ulong *page_size, uint32_t *fsr)
+static bool get_phys_addr(CPUARMState *env, target_ulong address,
+                          int access_type, ARMMMUIdx mmu_idx,
+                          hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
+                          target_ulong *page_size, uint32_t *fsr)
 {
     if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
         /* TODO: when we support EL2 we should here call ourselves recursively
-- 
1.9.1

[Qemu-devel] [PATCH v4 09/13] target-arm: Add ARMMMUFaultInfo

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Introduce ARMMMUFaultInfo to propagate MMU Fault information
across the MMU translation code path. This is in preparation for
adding Stage-2 translation.

No functional changes.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/helper.c    | 32 ++++++++++++++++++++------------
 target-arm/internals.h | 15 ++++++++++++++-
 target-arm/op_helper.c |  3 ++-
 3 files changed, 36 insertions(+), 14 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 17d0590..13d6c21 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -18,7 +18,8 @@
 static bool get_phys_addr(CPUARMState *env, target_ulong address,
                           int access_type, ARMMMUIdx mmu_idx,
                           hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
-                          target_ulong *page_size, uint32_t *fsr);
+                          target_ulong *page_size, uint32_t *fsr,
+                          ARMMMUFaultInfo *fi);
 
 /* Definitions for the PMCCNTR and PMCR registers */
 #define PMCRD   0x8
@@ -1774,9 +1775,10 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
     bool ret;
     uint64_t par64;
     MemTxAttrs attrs = {};
+    ARMMMUFaultInfo fi = {};
 
     ret = get_phys_addr(env, value, access_type, mmu_idx,
-                        &phys_addr, &attrs, &prot, &page_size, &fsr);
+                        &phys_addr, &attrs, &prot, &page_size, &fsr, &fi);
     if (extended_addresses_enabled(env)) {
         /* fsr is a DFSR/IFSR value for the long descriptor
          * translation table format, but with WnR always clear.
@@ -6167,7 +6169,8 @@ static uint64_t arm_ldq_ptw(CPUState *cs, hwaddr addr, bool is_secure)
 static bool get_phys_addr_v5(CPUARMState *env, uint32_t address,
                              int access_type, ARMMMUIdx mmu_idx,
                              hwaddr *phys_ptr, int *prot,
-                             target_ulong *page_size, uint32_t *fsr)
+                             target_ulong *page_size, uint32_t *fsr,
+                             ARMMMUFaultInfo *fi)
 {
     CPUState *cs = CPU(arm_env_get_cpu(env));
     int code;
@@ -6280,7 +6283,8 @@ do_fault:
 static bool get_phys_addr_v6(CPUARMState *env, uint32_t address,
                              int access_type, ARMMMUIdx mmu_idx,
                              hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
-                             target_ulong *page_size, uint32_t *fsr)
+                             target_ulong *page_size, uint32_t *fsr,
+                             ARMMMUFaultInfo *fi)
 {
     CPUState *cs = CPU(arm_env_get_cpu(env));
     int code;
@@ -6490,7 +6494,8 @@ static bool check_s2_startlevel(ARMCPU *cpu, bool is_aa64, int startlevel,
 static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
                                int access_type, ARMMMUIdx mmu_idx,
                                hwaddr *phys_ptr, MemTxAttrs *txattrs, int *prot,
-                               target_ulong *page_size_ptr, uint32_t *fsr)
+                               target_ulong *page_size_ptr, uint32_t *fsr,
+                               ARMMMUFaultInfo *fi)
 {
     ARMCPU *cpu = arm_env_get_cpu(env);
     CPUState *cs = CPU(cpu);
@@ -7062,7 +7067,8 @@ static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
 static bool get_phys_addr(CPUARMState *env, target_ulong address,
                           int access_type, ARMMMUIdx mmu_idx,
                           hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
-                          target_ulong *page_size, uint32_t *fsr)
+                          target_ulong *page_size, uint32_t *fsr,
+                          ARMMMUFaultInfo *fi)
 {
     if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
         /* TODO: when we support EL2 we should here call ourselves recursively
@@ -7121,13 +7127,13 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,
 
     if (regime_using_lpae_format(env, mmu_idx)) {
         return get_phys_addr_lpae(env, address, access_type, mmu_idx, phys_ptr,
-                                  attrs, prot, page_size, fsr);
+                                  attrs, prot, page_size, fsr, fi);
     } else if (regime_sctlr(env, mmu_idx) & SCTLR_XP) {
         return get_phys_addr_v6(env, address, access_type, mmu_idx, phys_ptr,
-                                attrs, prot, page_size, fsr);
+                                attrs, prot, page_size, fsr, fi);
     } else {
         return get_phys_addr_v5(env, address, access_type, mmu_idx, phys_ptr,
-                                prot, page_size, fsr);
+                                prot, page_size, fsr, fi);
     }
 }
 
@@ -7136,7 +7142,8 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,
  * fsr with ARM DFSR/IFSR fault register format value on failure.
  */
 bool arm_tlb_fill(CPUState *cs, vaddr address,
-                  int access_type, int mmu_idx, uint32_t *fsr)
+                  int access_type, int mmu_idx, uint32_t *fsr,
+                  ARMMMUFaultInfo *fi)
 {
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
@@ -7147,7 +7154,7 @@ bool arm_tlb_fill(CPUState *cs, vaddr address,
     MemTxAttrs attrs = {};
 
     ret = get_phys_addr(env, address, access_type, mmu_idx, &phys_addr,
-                        &attrs, &prot, &page_size, fsr);
+                        &attrs, &prot, &page_size, fsr, fi);
     if (!ret) {
         /* Map a single [sub]page.  */
         phys_addr &= TARGET_PAGE_MASK;
@@ -7170,9 +7177,10 @@ hwaddr arm_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
     bool ret;
     uint32_t fsr;
     MemTxAttrs attrs = {};
+    ARMMMUFaultInfo fi = {};
 
     ret = get_phys_addr(env, addr, 0, cpu_mmu_index(env, false), &phys_addr,
-                        &attrs, &prot, &page_size, &fsr);
+                        &attrs, &prot, &page_size, &fsr, &fi);
 
     if (ret) {
         return -1;
diff --git a/target-arm/internals.h b/target-arm/internals.h
index 8bd37eb..412827b 100644
--- a/target-arm/internals.h
+++ b/target-arm/internals.h
@@ -414,8 +414,21 @@ bool arm_is_psci_call(ARMCPU *cpu, int excp_type);
 void arm_handle_psci_call(ARMCPU *cpu);
 #endif
 
+/**
+ * ARMMMUFaultInfo: Information describing an ARM MMU Fault
+ * @s2addr: Address that caused a fault at stage 2
+ * @stage2: True if we faulted at stage 2
+ * @s1ptw: True if we faulted at stage 2 while doing a stage 1 page-table walk
+ */
+typedef struct ARMMMUFaultInfo ARMMMUFaultInfo;
+struct ARMMMUFaultInfo {
+    target_ulong s2addr;
+    bool stage2;
+    bool s1ptw;
+};
+
 /* Do a page table walk and add page to TLB if possible */
 bool arm_tlb_fill(CPUState *cpu, vaddr address, int rw, int mmu_idx,
-                  uint32_t *fsr);
+                  uint32_t *fsr, ARMMMUFaultInfo *fi);
 
 #endif
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index 1425a1d..7ff3c61 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -83,8 +83,9 @@ void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
 {
     bool ret;
     uint32_t fsr = 0;
+    struct ARMMMUFaultInfo fi = {0};
 
-    ret = arm_tlb_fill(cs, addr, is_write, mmu_idx, &fsr);
+    ret = arm_tlb_fill(cs, addr, is_write, mmu_idx, &fsr, &fi);
     if (unlikely(ret)) {
         ARMCPU *cpu = ARM_CPU(cs);
         CPUARMState *env = &cpu->env;
-- 
1.9.1

Re: [Qemu-devel] [PATCH v4 09/13] target-arm: Add ARMMMUFaultInfo

[Peter Maydell]

On 14 October 2015 at 23:55, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Introduce ARMMMUFaultInfo to propagate MMU Fault information
> across the MMU translation code path. This is in preparation for
> adding Stage-2 translation.
>
> No functional changes.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> @@ -1774,9 +1775,10 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
>      bool ret;
>      uint64_t par64;
>      MemTxAttrs attrs = {};
> +    ARMMMUFaultInfo fi = {};

Why are most of these initialized with "{}" ...

> @@ -83,8 +83,9 @@ void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
>  {
>      bool ret;
>      uint32_t fsr = 0;
> +    struct ARMMMUFaultInfo fi = {0};

...but this one uses "{0}" ?

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 09/13] target-arm: Add ARMMMUFaultInfo

[Edgar E. Iglesias]

On Fri, Oct 23, 2015 at 05:53:09PM +0100, Peter Maydell wrote:
> On 14 October 2015 at 23:55, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > Introduce ARMMMUFaultInfo to propagate MMU Fault information
> > across the MMU translation code path. This is in preparation for
> > adding Stage-2 translation.
> >
> > No functional changes.
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > @@ -1774,9 +1775,10 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
> >      bool ret;
> >      uint64_t par64;
> >      MemTxAttrs attrs = {};
> > +    ARMMMUFaultInfo fi = {};
> 
> Why are most of these initialized with "{}" ...
> 
> > @@ -83,8 +83,9 @@ void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
> >  {
> >      bool ret;
> >      uint32_t fsr = 0;
> > +    struct ARMMMUFaultInfo fi = {0};
> 
> ...but this one uses "{0}" ?

No reason.. I've removed the struct and zero so it's now:

    ARMMMUFaultInfo fi = {};

everywhere.

Thanks,
Edgar


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

[Qemu-devel] [PATCH v4 10/13] target-arm: Add S2 translation to 64bit S1 PTWs

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Add support for applying S2 translation to 64bit S1
page-table walks.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/helper.c    | 50 ++++++++++++++++++++++++++++++++++++++++++++++++--
 target-arm/op_helper.c |  4 ++--
 2 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 13d6c21..784cd0b 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -21,6 +21,12 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,
                           target_ulong *page_size, uint32_t *fsr,
                           ARMMMUFaultInfo *fi);
 
+static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
+                               int access_type, ARMMMUIdx mmu_idx,
+                               hwaddr *phys_ptr, MemTxAttrs *txattrs, int *prot,
+                               target_ulong *page_size_ptr, uint32_t *fsr,
+                               ARMMMUFaultInfo *fi);
+
 /* Definitions for the PMCCNTR and PMCR registers */
 #define PMCRD   0x8
 #define PMCRC   0x4
@@ -6143,6 +6149,32 @@ static bool get_level1_table_address(CPUARMState *env, ARMMMUIdx mmu_idx,
     return true;
 }
 
+/* Translate a S1 pagetable walk through S2 if needed.  */
+static hwaddr S1_ptw_translate(CPUARMState *env, ARMMMUIdx mmu_idx,
+                               hwaddr addr, MemTxAttrs txattrs,
+                               uint32_t *fsr,
+                               ARMMMUFaultInfo *fi)
+{
+    if ((mmu_idx == ARMMMUIdx_S1NSE0 || mmu_idx == ARMMMUIdx_S1NSE1) &&
+        !regime_translation_disabled(env, ARMMMUIdx_S2NS)) {
+        target_ulong s2size;
+        hwaddr s2pa;
+        int s2prot;
+        int ret;
+
+        ret = get_phys_addr_lpae(env, addr, 0, ARMMMUIdx_S2NS, &s2pa,
+                                 &txattrs, &s2prot, &s2size, fsr, fi);
+        if (ret) {
+            fi->s2addr = addr;
+            fi->stage2 = true;
+            fi->s1ptw = true;
+            return ~0;
+        }
+        addr = s2pa;
+    }
+    return addr;
+}
+
 /* All loads done in the course of a page table walk go through here.
  * TODO: rather than ignoring errors from physical memory reads (which
  * are external aborts in ARM terminology) we should propagate this
@@ -6158,11 +6190,19 @@ static uint32_t arm_ldl_ptw(CPUState *cs, hwaddr addr, bool is_secure)
     return address_space_ldl(cs->as, addr, attrs, NULL);
 }
 
-static uint64_t arm_ldq_ptw(CPUState *cs, hwaddr addr, bool is_secure)
+static uint64_t arm_ldq_ptw(CPUState *cs, hwaddr addr, bool is_secure,
+                            ARMMMUIdx mmu_idx, uint32_t *fsr,
+                            ARMMMUFaultInfo *fi)
 {
+    ARMCPU *cpu = ARM_CPU(cs);
+    CPUARMState *env = &cpu->env;
     MemTxAttrs attrs = {};
 
     attrs.secure = is_secure;
+    addr = S1_ptw_translate(env, mmu_idx, addr, attrs, fsr, fi);
+    if (fi->s1ptw) {
+        return 0;
+    }
     return address_space_ldq(cs->as, addr, attrs, NULL);
 }
 
@@ -6718,7 +6758,11 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
         descaddr |= (address >> (stride * (4 - level))) & descmask;
         descaddr &= ~7ULL;
         nstable = extract32(tableattrs, 4, 1);
-        descriptor = arm_ldq_ptw(cs, descaddr, !nstable);
+        descriptor = arm_ldq_ptw(cs, descaddr, !nstable, mmu_idx, fsr, fi);
+        if (fi->s1ptw) {
+            goto do_fault;
+        }
+
         if (!(descriptor & 1) ||
             (!(descriptor & 2) && (level == 3))) {
             /* Invalid, or the Reserved level 3 encoding */
@@ -6802,6 +6846,8 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
 do_fault:
     /* Long-descriptor format IFSR/DFSR value */
     *fsr = (1 << 9) | (fault_type << 2) | level;
+    /* Tag the error as S2 for failed S1 PTW at S2 or ordinary S2.  */
+    fi->stage2 = fi->s1ptw || (mmu_idx == ARMMMUIdx_S2NS);
     return true;
 }
 
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index 7ff3c61..d4715f4 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -104,10 +104,10 @@ void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
          * information; this is always true for exceptions reported to EL1.
          */
         if (is_write == 2) {
-            syn = syn_insn_abort(same_el, 0, 0, syn);
+            syn = syn_insn_abort(same_el, 0, fi.s1ptw, syn);
             exc = EXCP_PREFETCH_ABORT;
         } else {
-            syn = syn_data_abort(same_el, 0, 0, 0, is_write == 1, syn);
+            syn = syn_data_abort(same_el, 0, 0, fi.s1ptw, is_write == 1, syn);
             if (is_write == 1 && arm_feature(env, ARM_FEATURE_V6)) {
                 fsr |= (1 << 11);
             }
-- 
1.9.1

Re: [Qemu-devel] [PATCH v4 10/13] target-arm: Add S2 translation to 64bit S1 PTWs

[Peter Maydell]

On 14 October 2015 at 23:55, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Add support for applying S2 translation to 64bit S1
> page-table walks.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---

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

thanks
-- PMM

[Qemu-devel] [PATCH v4 11/13] target-arm: Add S2 translation to 32bit S1 PTWs

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Add support for applying S2 translation to 32bit S1
page-table walks.

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

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 784cd0b..69e24e1 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -6182,11 +6182,19 @@ static hwaddr S1_ptw_translate(CPUARMState *env, ARMMMUIdx mmu_idx,
  * was being done for a CPU load/store or an address translation instruction
  * (but not if it was for a debug access).
  */
-static uint32_t arm_ldl_ptw(CPUState *cs, hwaddr addr, bool is_secure)
+static uint32_t arm_ldl_ptw(CPUState *cs, hwaddr addr, bool is_secure,
+                            ARMMMUIdx mmu_idx, uint32_t *fsr,
+                            ARMMMUFaultInfo *fi)
 {
+    ARMCPU *cpu = ARM_CPU(cs);
+    CPUARMState *env = &cpu->env;
     MemTxAttrs attrs = {};
 
     attrs.secure = is_secure;
+    addr = S1_ptw_translate(env, mmu_idx, addr, attrs, fsr, fi);
+    if (fi->s1ptw) {
+        return 0;
+    }
     return address_space_ldl(cs->as, addr, attrs, NULL);
 }
 
@@ -6230,7 +6238,8 @@ static bool get_phys_addr_v5(CPUARMState *env, uint32_t address,
         code = 5;
         goto do_fault;
     }
-    desc = arm_ldl_ptw(cs, table, regime_is_secure(env, mmu_idx));
+    desc = arm_ldl_ptw(cs, table, regime_is_secure(env, mmu_idx),
+                       mmu_idx, fsr, fi);
     type = (desc & 3);
     domain = (desc >> 5) & 0x0f;
     if (regime_el(env, mmu_idx) == 1) {
@@ -6266,7 +6275,8 @@ static bool get_phys_addr_v5(CPUARMState *env, uint32_t address,
             /* Fine pagetable.  */
             table = (desc & 0xfffff000) | ((address >> 8) & 0xffc);
         }
-        desc = arm_ldl_ptw(cs, table, regime_is_secure(env, mmu_idx));
+        desc = arm_ldl_ptw(cs, table, regime_is_secure(env, mmu_idx),
+                           mmu_idx, fsr, fi);
         switch (desc & 3) {
         case 0: /* Page translation fault.  */
             code = 7;
@@ -6347,7 +6357,8 @@ static bool get_phys_addr_v6(CPUARMState *env, uint32_t address,
         code = 5;
         goto do_fault;
     }
-    desc = arm_ldl_ptw(cs, table, regime_is_secure(env, mmu_idx));
+    desc = arm_ldl_ptw(cs, table, regime_is_secure(env, mmu_idx),
+                       mmu_idx, fsr, fi);
     type = (desc & 3);
     if (type == 0 || (type == 3 && !arm_feature(env, ARM_FEATURE_PXN))) {
         /* Section translation fault, or attempt to use the encoding
@@ -6398,7 +6409,8 @@ static bool get_phys_addr_v6(CPUARMState *env, uint32_t address,
         ns = extract32(desc, 3, 1);
         /* Lookup l2 entry.  */
         table = (desc & 0xfffffc00) | ((address >> 10) & 0x3fc);
-        desc = arm_ldl_ptw(cs, table, regime_is_secure(env, mmu_idx));
+        desc = arm_ldl_ptw(cs, table, regime_is_secure(env, mmu_idx),
+                           mmu_idx, fsr, fi);
         ap = ((desc >> 4) & 3) | ((desc >> 7) & 4);
         switch (desc & 3) {
         case 0: /* Page translation fault.  */
-- 
1.9.1

Re: [Qemu-devel] [PATCH v4 11/13] target-arm: Add S2 translation to 32bit S1 PTWs

[Peter Maydell]

On 14 October 2015 at 23:55, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Add support for applying S2 translation to 32bit S1
> page-table walks.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---

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

thanks
-- PMM

[Qemu-devel] [PATCH v4 12/13] target-arm: Route S2 MMU faults to EL2

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/op_helper.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index d4715f4..2ccd1c9 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -90,13 +90,19 @@ void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
         ARMCPU *cpu = ARM_CPU(cs);
         CPUARMState *env = &cpu->env;
         uint32_t syn, exc;
-        bool same_el = (arm_current_el(env) != 0);
+        unsigned int target_el;
+        bool same_el;
 
         if (retaddr) {
             /* now we have a real cpu fault */
             cpu_restore_state(cs, retaddr);
         }
 
+        target_el = exception_target_el(env);
+        if (fi.stage2) {
+            target_el = 2;
+        }
+        same_el = arm_current_el(env) == target_el;
         /* AArch64 syndrome does not have an LPAE bit */
         syn = fsr & ~(1 << 9);
 
@@ -116,7 +122,7 @@ void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
 
         env->exception.vaddress = addr;
         env->exception.fsr = fsr;
-        raise_exception(env, exc, syn, exception_target_el(env));
+        raise_exception(env, exc, syn, target_el);
     }
 }
 #endif
-- 
1.9.1

Re: [Qemu-devel] [PATCH v4 12/13] target-arm: Route S2 MMU faults to EL2

[Peter Maydell]

On 14 October 2015 at 23:55, 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/op_helper.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index d4715f4..2ccd1c9 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -90,13 +90,19 @@ void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
>          ARMCPU *cpu = ARM_CPU(cs);
>          CPUARMState *env = &cpu->env;
>          uint32_t syn, exc;
> -        bool same_el = (arm_current_el(env) != 0);
> +        unsigned int target_el;
> +        bool same_el;
>
>          if (retaddr) {
>              /* now we have a real cpu fault */
>              cpu_restore_state(cs, retaddr);
>          }
>
> +        target_el = exception_target_el(env);
> +        if (fi.stage2) {
> +            target_el = 2;
> +        }
> +        same_el = arm_current_el(env) == target_el;
>          /* AArch64 syndrome does not have an LPAE bit */
>          syn = fsr & ~(1 << 9);
>
> @@ -116,7 +122,7 @@ void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
>
>          env->exception.vaddress = addr;
>          env->exception.fsr = fsr;
> -        raise_exception(env, exc, syn, exception_target_el(env));
> +        raise_exception(env, exc, syn, target_el);
>      }
>  }

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

thanks
-- PMM

[Qemu-devel] [PATCH v4 13/13] target-arm: Add support for S1 + S2 MMU translations

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

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

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 69e24e1..9d70ef2 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -7129,14 +7129,44 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,
                           ARMMMUFaultInfo *fi)
 {
     if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
-        /* TODO: when we support EL2 we should here call ourselves recursively
-         * to do the stage 1 and then stage 2 translations. The arm_ld*_ptw
-         * functions will also need changing to perform ARMMMUIdx_S2NS loads
-         * rather than direct physical memory loads when appropriate.
-         * For non-EL2 CPUs a stage1+stage2 translation is just stage 1.
+        /* Call ourselves recursively to do the stage 1 and then stage 2
+         * translations.
          */
-        assert(!arm_feature(env, ARM_FEATURE_EL2));
-        mmu_idx += ARMMMUIdx_S1NSE0;
+        if (arm_feature(env, ARM_FEATURE_EL2)) {
+            hwaddr ipa;
+            int s2_prot;
+            int ret;
+
+            ret = get_phys_addr(env, address, access_type,
+                                mmu_idx + ARMMMUIdx_S1NSE0, &ipa, attrs,
+                                prot, page_size, fsr, fi);
+
+            /* If S1 fails or S2 is disabled, return early.  */
+            if (ret || regime_translation_disabled(env, ARMMMUIdx_S2NS)) {
+                if (ret && fi->stage2) {
+                    /* This is a S2 error while doing S1 PTW.  */
+                    env->cp15.hpfar_el2 = extract64(fi->s2addr, 12, 47) << 4;
+                }
+                *phys_ptr = ipa;
+                return ret;
+            }
+
+            /* S1 is done. Now do S2 translation.  */
+            ret = get_phys_addr_lpae(env, ipa, access_type, ARMMMUIdx_S2NS,
+                                     phys_ptr, attrs, &s2_prot,
+                                     page_size, fsr, fi);
+            if (ret) {
+                env->cp15.hpfar_el2 = extract64(ipa, 12, 47) << 4;
+            }
+            /* Combine the S1 and S2 perms.  */
+            *prot &= s2_prot;
+            return ret;
+        } else {
+            /*
+             * For non-EL2 CPUs a stage1+stage2 translation is just stage 1.
+             */
+            mmu_idx += ARMMMUIdx_S1NSE0;
+        }
     }
 
     /* The page table entries may downgrade secure to non-secure, but
-- 
1.9.1

Re: [Qemu-devel] [PATCH v4 13/13] target-arm: Add support for S1 + S2 MMU translations

[Peter Maydell]

On 14 October 2015 at 23:55, 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 | 44 +++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 37 insertions(+), 7 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 69e24e1..9d70ef2 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -7129,14 +7129,44 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,
>                            ARMMMUFaultInfo *fi)
>  {
>      if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
> -        /* TODO: when we support EL2 we should here call ourselves recursively
> -         * to do the stage 1 and then stage 2 translations. The arm_ld*_ptw
> -         * functions will also need changing to perform ARMMMUIdx_S2NS loads
> -         * rather than direct physical memory loads when appropriate.
> -         * For non-EL2 CPUs a stage1+stage2 translation is just stage 1.
> +        /* Call ourselves recursively to do the stage 1 and then stage 2
> +         * translations.
>           */
> -        assert(!arm_feature(env, ARM_FEATURE_EL2));
> -        mmu_idx += ARMMMUIdx_S1NSE0;
> +        if (arm_feature(env, ARM_FEATURE_EL2)) {
> +            hwaddr ipa;
> +            int s2_prot;
> +            int ret;
> +
> +            ret = get_phys_addr(env, address, access_type,
> +                                mmu_idx + ARMMMUIdx_S1NSE0, &ipa, attrs,
> +                                prot, page_size, fsr, fi);
> +
> +            /* If S1 fails or S2 is disabled, return early.  */
> +            if (ret || regime_translation_disabled(env, ARMMMUIdx_S2NS)) {
> +                if (ret && fi->stage2) {
> +                    /* This is a S2 error while doing S1 PTW.  */
> +                    env->cp15.hpfar_el2 = extract64(fi->s2addr, 12, 47) << 4;

Might be worth a note that it's OK to set the HPFAR here because
this always results in a fault (even if from an AT instruction),
whereas we can't set the FAR registers here because that doesn't
happen for stage 1 faults from AT instructions.

...I think we still need to add the code to cause the exception
if a stage 1 AT instruction results in a stage 2 fault, right?
If the caller has to look into the FaultInfo struct anyway, maybe
we should just let the caller set the HPFAR_EL2 from the s2addr
if it's going to send the exception to EL2.

> +                }
> +                *phys_ptr = ipa;
> +                return ret;
> +            }
> +
> +            /* S1 is done. Now do S2 translation.  */
> +            ret = get_phys_addr_lpae(env, ipa, access_type, ARMMMUIdx_S2NS,
> +                                     phys_ptr, attrs, &s2_prot,
> +                                     page_size, fsr, fi);
> +            if (ret) {
> +                env->cp15.hpfar_el2 = extract64(ipa, 12, 47) << 4;
> +            }
> +            /* Combine the S1 and S2 perms.  */
> +            *prot &= s2_prot;
> +            return ret;
> +        } else {
> +            /*
> +             * For non-EL2 CPUs a stage1+stage2 translation is just stage 1.
> +             */
> +            mmu_idx += ARMMMUIdx_S1NSE0;
> +        }
>      }
>
>      /* The page table entries may downgrade secure to non-secure, but
> --
> 1.9.1
>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 13/13] target-arm: Add support for S1 + S2 MMU translations

[Edgar E. Iglesias]

On Fri, Oct 23, 2015 at 06:09:24PM +0100, Peter Maydell wrote:
> On 14 October 2015 at 23:55, 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 | 44 +++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 37 insertions(+), 7 deletions(-)
> >
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index 69e24e1..9d70ef2 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -7129,14 +7129,44 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,
> >                            ARMMMUFaultInfo *fi)
> >  {
> >      if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
> > -        /* TODO: when we support EL2 we should here call ourselves recursively
> > -         * to do the stage 1 and then stage 2 translations. The arm_ld*_ptw
> > -         * functions will also need changing to perform ARMMMUIdx_S2NS loads
> > -         * rather than direct physical memory loads when appropriate.
> > -         * For non-EL2 CPUs a stage1+stage2 translation is just stage 1.
> > +        /* Call ourselves recursively to do the stage 1 and then stage 2
> > +         * translations.
> >           */
> > -        assert(!arm_feature(env, ARM_FEATURE_EL2));
> > -        mmu_idx += ARMMMUIdx_S1NSE0;
> > +        if (arm_feature(env, ARM_FEATURE_EL2)) {
> > +            hwaddr ipa;
> > +            int s2_prot;
> > +            int ret;
> > +
> > +            ret = get_phys_addr(env, address, access_type,
> > +                                mmu_idx + ARMMMUIdx_S1NSE0, &ipa, attrs,
> > +                                prot, page_size, fsr, fi);
> > +
> > +            /* If S1 fails or S2 is disabled, return early.  */
> > +            if (ret || regime_translation_disabled(env, ARMMMUIdx_S2NS)) {
> > +                if (ret && fi->stage2) {
> > +                    /* This is a S2 error while doing S1 PTW.  */
> > +                    env->cp15.hpfar_el2 = extract64(fi->s2addr, 12, 47) << 4;
> 
> Might be worth a note that it's OK to set the HPFAR here because
> this always results in a fault (even if from an AT instruction),
> whereas we can't set the FAR registers here because that doesn't
> happen for stage 1 faults from AT instructions.
> 
> ...I think we still need to add the code to cause the exception
> if a stage 1 AT instruction results in a stage 2 fault, right?

Yes, those faults are still missing... I can try to add them
in the next round/series together with the detailed error reporting.


> If the caller has to look into the FaultInfo struct anyway, maybe
> we should just let the caller set the HPFAR_EL2 from the s2addr
> if it's going to send the exception to EL2.


Agreed, I've moved the setting of HPFAR_EL2 to the callers.

Thanks!
Edgar



> 
> > +                }
> > +                *phys_ptr = ipa;
> > +                return ret;
> > +            }
> > +
> > +            /* S1 is done. Now do S2 translation.  */
> > +            ret = get_phys_addr_lpae(env, ipa, access_type, ARMMMUIdx_S2NS,
> > +                                     phys_ptr, attrs, &s2_prot,
> > +                                     page_size, fsr, fi);
> > +            if (ret) {
> > +                env->cp15.hpfar_el2 = extract64(ipa, 12, 47) << 4;
> > +            }
> > +            /* Combine the S1 and S2 perms.  */
> > +            *prot &= s2_prot;
> > +            return ret;
> > +        } else {
> > +            /*
> > +             * For non-EL2 CPUs a stage1+stage2 translation is just stage 1.
> > +             */
> > +            mmu_idx += ARMMMUIdx_S1NSE0;
> > +        }
> >      }
> >
> >      /* The page table entries may downgrade secure to non-secure, but
> > --
> > 1.9.1
> >
> 
> thanks
> -- PMM