[Qemu-devel] [PATCH RFC 0/8] arm: Steps towards EL2 support round 5

[Qemu-devel] [PATCH RFC 0/8] 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 for AArch64. I've marked it RFC because I
expect a few iterations. Once we can settle on the approach I'll
add the AArch32 support (changes for arm_ldl_ptw etc). I've probably
missed alot of details aswell.

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).

Some of the patches at the start of the series might be good already,
please pick them up if you agree Peter!

Comments welcome!

Best regards,
Edgar

Edgar E. Iglesias (8):
  target-arm: Add HPFAR_EL2
  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 support for S1 PTW
  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    | 211 ++++++++++++++++++++++++++++++++++++++++---------
 target-arm/internals.h |  10 ++-
 target-arm/op_helper.c |  17 ++--
 4 files changed, 197 insertions(+), 42 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH RFC 1/8] target-arm: Add HPFAR_EL2

[Edgar E. Iglesias]

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

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/cpu.h    | 1 +
 target-arm/helper.c | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 1b80516..e7694a5 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -280,6 +280,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 12ea88f..b709582 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -3221,6 +3221,9 @@ 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, .type = ARM_CP_CONST, .resetvalue = 0 },
     REGINFO_SENTINEL
 };
 
@@ -3442,6 +3445,10 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
       .resetvalue = 0,
       .writefn = gt_hyp_ctl_write, .raw_writefn = raw_write },
 #endif
+    { .name = "HPFAR_EL2", .state = ARM_CP_STATE_BOTH,
+      .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 RFC 2/8] 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 | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index b709582..33be8c2 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -6542,18 +6542,26 @@ 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 (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
-     * 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;
-     */
-    level = 4 - (va_size - tsz - 4) / granule_sz;
+    if (mmu_idx == ARMMMUIdx_S2NS) {
+        unsigned int startlevel = extract32(tcr->raw_tcr, 6, 2);
+        level = 3 - startlevel;
+        if (granule_sz == 9) {
+            level = 2 - startlevel;
+        }
+    } else {
+        /* 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 the input address. In the pseudocode this is:
+         *  level = 4 - RoundUp((inputsize - grainsize) / stride)
+         * where their 'inputsize' is our 'va_size - tsz', '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;
+         */
+        level = 4 - (va_size - tsz - 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
-- 
1.9.1

[Qemu-devel] [PATCH RFC 3/8] 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 | 48 +++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 45 insertions(+), 3 deletions(-)

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

[Qemu-devel] [PATCH RFC 4/8] 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.

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

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 6f0ed51..7e7f29d 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -13,7 +13,7 @@
 #include "exec/semihost.h"
 
 #ifndef CONFIG_USER_ONLY
-static inline bool get_phys_addr(CPUARMState *env, target_ulong 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);
@@ -6967,10 +6967,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 RFC 5/8] 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 State-2 translation.

No functional changes.

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

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 7e7f29d..6eb903b 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -16,7 +16,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
@@ -1772,9 +1773,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.
@@ -6434,7 +6436,8 @@ typedef enum {
 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)
 {
     CPUState *cs = CPU(arm_env_get_cpu(env));
     /* Read an LPAE long-descriptor translation table. */
@@ -6970,7 +6973,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
@@ -7029,7 +7033,7 @@ 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);
@@ -7044,7 +7048,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;
@@ -7055,7 +7060,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;
@@ -7078,9 +7083,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 36a56aa..61dfc97 100644
--- a/target-arm/internals.h
+++ b/target-arm/internals.h
@@ -389,8 +389,16 @@ bool arm_is_psci_call(ARMCPU *cpu, int excp_type);
 void arm_handle_psci_call(ARMCPU *cpu);
 #endif
 
+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

[Qemu-devel] [PATCH RFC 6/8] target-arm: Add S2 translation support for S1 PTW

[Edgar E. Iglesias]

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

Add support for applying S2 translation to 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 6eb903b..eac1a25 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -19,6 +19,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
@@ -6146,6 +6152,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
@@ -6161,11 +6193,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);
 }
 
@@ -6625,7 +6665,11 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
         descaddr |= (address >> (granule_sz * (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 */
@@ -6708,6 +6752,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

[Qemu-devel] [PATCH RFC 7/8] 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

[Qemu-devel] [PATCH RFC 8/8] 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 eac1a25..5710dfc 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -7023,14 +7023,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 RFC 0/8] arm: Steps towards EL2 support round 5

[Edgar E. Iglesias]

On Sat, Sep 19, 2015 at 07:15:19AM -0700, Edgar E. Iglesias wrote:
> 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 for AArch64. I've marked it RFC because I
> expect a few iterations. Once we can settle on the approach I'll
> add the AArch32 support (changes for arm_ldl_ptw etc). I've probably
> missed alot of details aswell.

Another note, the mem attributes handling is also not considered yet.
For example the user attribute when doing S2 and so one.
It wasn't super clear to me howto reorg the code and how the specs
works here so I will have to revisit that aswell. If you have comments
on how it should work those are greatly appreciated :-)

Cheers,
Edgar



> 
> 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).
> 
> Some of the patches at the start of the series might be good already,
> please pick them up if you agree Peter!
> 
> Comments welcome!
> 
> Best regards,
> Edgar
> 
> Edgar E. Iglesias (8):
>   target-arm: Add HPFAR_EL2
>   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 support for S1 PTW
>   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    | 211 ++++++++++++++++++++++++++++++++++++++++---------
>  target-arm/internals.h |  10 ++-
>  target-arm/op_helper.c |  17 ++--
>  4 files changed, 197 insertions(+), 42 deletions(-)
> 
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH RFC 1/8] target-arm: Add HPFAR_EL2

[Peter Maydell]

On 19 September 2015 at 07:15, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target-arm/cpu.h    | 1 +
>  target-arm/helper.c | 7 +++++++
>  2 files changed, 8 insertions(+)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 1b80516..e7694a5 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -280,6 +280,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 12ea88f..b709582 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3221,6 +3221,9 @@ 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, .type = ARM_CP_CONST, .resetvalue = 0 },
>      REGINFO_SENTINEL
>  };
>
> @@ -3442,6 +3445,10 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
>        .resetvalue = 0,
>        .writefn = gt_hyp_ctl_write, .raw_writefn = raw_write },
>  #endif
> +    { .name = "HPFAR_EL2", .state = ARM_CP_STATE_BOTH,
> +      .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
> +      .access = PL2_RW,
> +      .fieldoffset = offsetof(CPUARMState, cp15.hpfar_el2) },
>      REGINFO_SENTINEL
>  }

Shouldn't these have the accessfn for "no access from 32-bit EL3
if SCR.NS is 0" ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH RFC 2/8] target-arm: Add computation of starting level for S2 PTW

[Peter Maydell]

On 19 September 2015 at 07:15, 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 | 32 ++++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 12 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index b709582..33be8c2 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -6542,18 +6542,26 @@ 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 (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
> -     * 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;
> -     */
> -    level = 4 - (va_size - tsz - 4) / granule_sz;
> +    if (mmu_idx == ARMMMUIdx_S2NS) {
> +        unsigned int startlevel = extract32(tcr->raw_tcr, 6, 2);
> +        level = 3 - startlevel;
> +        if (granule_sz == 9) {
> +            level = 2 - startlevel;
> +        }

I think this is right code-wise but we could make it read a little more
nicely: if you make the condition be "if (mmu_idx != ARMMUIdx_S2NS)" then
the common case comes first and its long comment works as a description
of what we're doing here. Then the else clause can just say
 /* For stage 2 translations the starting level is specified by the
  * VCTR_EL2.SL0 field (whose interpretation depends on the page size)
  */

I was pondering whether writing it as
     if (granule_sz == 9) {
         /* 4K pages */
         level = 2 - startlevel;
     } else {
         /* 16K or 64K pages */
         level = 3 - startlevel;
     }

would be slightly better, but it's marginal. Do add a "4K pages"
comment in somewhere, though.

> +    } else {
> +        /* 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 the input address. In the pseudocode this is:
> +         *  level = 4 - RoundUp((inputsize - grainsize) / stride)
> +         * where their 'inputsize' is our 'va_size - tsz', '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;
> +         */
> +        level = 4 - (va_size - tsz - 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
> --
> 1.9.1
>

thanks
-- PMM

Re: [Qemu-devel] [PATCH RFC 3/8] target-arm: Add support for S2 page-table protection bits

[Peter Maydell]

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

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

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

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

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


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

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

> +            break;
> +        }

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

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

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH RFC 4/8] target-arm: Avoid inline for get_phys_addr

[Peter Maydell]

On 19 September 2015 at 07:15, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Avoid inline for get_phys_addr() to prepare for future recursive use.

Does the compiler actually complain?

In any case this function is a lot more complex than it used to be so
we might as well just rely on the compiler's discretion about whether
to bother inlining it or not.

> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target-arm/helper.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 6f0ed51..7e7f29d 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -13,7 +13,7 @@
>  #include "exec/semihost.h"
>
>  #ifndef CONFIG_USER_ONLY
> -static inline bool get_phys_addr(CPUARMState *env, target_ulong 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);

Might as well reindent these lines.

> @@ -6967,10 +6967,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
>

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH RFC 5/8] target-arm: Add ARMMMUFaultInfo

[Peter Maydell]

On 19 September 2015 at 07:15, 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 State-2 translation.
>
> No functional changes.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

OK I think, but maybe a comment on the definition of the
ARMMMUFaultInfo struct describing its purpose and maybe the meaning
of the fields would be useful?

thanks
-- PMM

Re: [Qemu-devel] [PATCH RFC 0/8] arm: Steps towards EL2 support round 5

[Peter Maydell]

On 19 September 2015 at 07:15, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> 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 for AArch64. I've marked it RFC because I
> expect a few iterations. Once we can settle on the approach I'll
> add the AArch32 support (changes for arm_ldl_ptw etc). I've probably
> missed alot of details aswell.
>
> 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).
>
> Some of the patches at the start of the series might be good already,
> please pick them up if you agree Peter!
>
> Comments welcome!
>
> Best regards,
> Edgar
>
> Edgar E. Iglesias (8):
>   target-arm: Add HPFAR_EL2
>   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 support for S1 PTW
>   target-arm: Route S2 MMU faults to EL2
>   target-arm: Add support for S1 + S2 MMU translations

I've reviewed the easy patches at the start of this series.
IIRC you said you'd found some issues with it anyway and were
planning to do a v2 in a bit, so I'll wait for that rather
than trying to tackle the last few patches now.

thanks
-- PMM

Re: [Qemu-devel] [PATCH RFC 0/8] arm: Steps towards EL2 support round 5

[Edgar E. Iglesias]

On Wed, Sep 23, 2015 at 10:11:19AM -0700, Peter Maydell wrote:
> On 19 September 2015 at 07:15, Edgar E. Iglesias
> <edgar.iglesias@gmail.com> wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > Hi,
> >
> > Another round of patches towards EL2 support. This one adds partial
> > support for 2-stage MMU for AArch64. I've marked it RFC because I
> > expect a few iterations. Once we can settle on the approach I'll
> > add the AArch32 support (changes for arm_ldl_ptw etc). I've probably
> > missed alot of details aswell.
> >
> > 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).
> >
> > Some of the patches at the start of the series might be good already,
> > please pick them up if you agree Peter!
> >
> > Comments welcome!
> >
> > Best regards,
> > Edgar
> >
> > Edgar E. Iglesias (8):
> >   target-arm: Add HPFAR_EL2
> >   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 support for S1 PTW
> >   target-arm: Route S2 MMU faults to EL2
> >   target-arm: Add support for S1 + S2 MMU translations
> 
> I've reviewed the easy patches at the start of this series.
> IIRC you said you'd found some issues with it anyway and were
> planning to do a v2 in a bit, so I'll wait for that rather
> than trying to tackle the last few patches now.
>

Thanks Peter,

I'll fix up the stuff you commented on and post a v2.

Cheers,
Edgar 

Re: [Qemu-devel] [PATCH RFC 4/8] target-arm: Avoid inline for get_phys_addr

[Edgar E. Iglesias]

On Wed, Sep 23, 2015 at 09:58:10AM -0700, Peter Maydell wrote:
> On 19 September 2015 at 07:15, Edgar E. Iglesias
> <edgar.iglesias@gmail.com> wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > Avoid inline for get_phys_addr() to prepare for future recursive use.
> 
> Does the compiler actually complain?

Sorry for the late replies Peter...
Yes the compiler complains if we keep the inline. I'll fix the indentation.

Thanks,
Edgar

> 
> In any case this function is a lot more complex than it used to be so
> we might as well just rely on the compiler's discretion about whether
> to bother inlining it or not.
> 
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  target-arm/helper.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index 6f0ed51..7e7f29d 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -13,7 +13,7 @@
> >  #include "exec/semihost.h"
> >
> >  #ifndef CONFIG_USER_ONLY
> > -static inline bool get_phys_addr(CPUARMState *env, target_ulong 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);
> 
> Might as well reindent these lines.
> 
> > @@ -6967,10 +6967,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
> >
> 
> Otherwise:
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH RFC 3/8] target-arm: Add support for S2 page-table protection bits

[Edgar E. Iglesias]

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


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

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

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


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

Thanks,
Edgar


> 
> 
> > +    return prot_rw;
> > +}
> > +
> >  /* Translate section/page access permissions to protection flags
> >   *
> >   * @env:     CPUARMState
> > @@ -6617,6 +6649,11 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
> >          /* Extract attributes from the descriptor and merge with table attrs */
> >          attrs = extract64(descriptor, 2, 10)
> >              | (extract64(descriptor, 52, 12) << 10);
> > +
> > +        if (mmu_idx == ARMMMUIdx_S2NS) {
> > +            /* The following extractions do not apply to S2.  */
> 
> I think it would be clearer to say
>   /* Stage 2 table descriptors do not include any attribute fields. */
> 
> > +            break;
> > +        }
> 
> and then here put
>       /* Merge in attributes from table descriptors */
> 
> and delete the now-redundant "and merge with table attrs" part from the
> comment at the start of this hunk.
> 
> >          attrs |= extract32(tableattrs, 0, 2) << 11; /* XN, PXN */
> >          attrs |= extract32(tableattrs, 3, 1) << 5; /* APTable[1] => AP[2] */
> >          /* The sense of AP[1] vs APTable[0] is reversed, as APTable[0] == 1
> > @@ -6638,11 +6675,16 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
> >      }
> >
> >      ap = extract32(attrs, 4, 2);
> > -    ns = extract32(attrs, 3, 1);
> >      xn = extract32(attrs, 12, 1);
> > -    pxn = extract32(attrs, 11, 1);
> >
> > -    *prot = get_S1prot(env, mmu_idx, va_size == 64, ap, ns, xn, pxn);
> > +    if (mmu_idx == ARMMMUIdx_S2NS) {
> > +        ns = true;
> > +        *prot = get_S2prot(env, ap, xn);
> > +    } else {
> > +        ns = extract32(attrs, 3, 1);
> > +        pxn = extract32(attrs, 11, 1);
> > +        *prot = get_S1prot(env, mmu_idx, va_size == 64, ap, ns, xn, pxn);
> > +    }
> >
> >      fault_type = permission_fault;
> >      if (!(*prot & (1 << access_type))) {
> > --
> > 1.9.1
> >
> 
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH RFC 3/8] target-arm: Add support for S2 page-table protection bits

[Peter Maydell]

On 1 October 2015 at 19:44, Edgar E. Iglesias <edgar.iglesias@xilinx.com> wrote:
> On Wed, Sep 23, 2015 at 09:55:05AM -0700, Peter Maydell wrote:
>> This isn't right -- the XN bit controls executability and the
>> S2AP bits don't affect it at all. I think you want:
>>
>>     int prot_rw = 0;
>>     if (s2ap & 1) {
>>         prot_rw |= PAGE_READ;
>>     }
>>     if (s2ap & 2) {
>>         prot_rw |= PAGE_WRITE;
>>     }
>>     if (!xn) {
>>         prot_rw |= PAGE_EXEC;
>>     }
>
>
> Thanks, this was the stuff I was worried about when we talked about it
> last time. I've got the following now which seems to be the same as
> you suggest:
>
> static int get_S2prot(CPUARMState *env, int s2ap, int xn)
> {
>     int prot;
>
>     prot = s2ap & 1 ? PAGE_READ : 0;
>     prot |= s2ap & 2 ? PAGE_WRITE : 0;
>     if (!xn) {
>         prot |= PAGE_EXEC;
>     }
>     return prot;
> }

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH RFC 3/8] target-arm: Add support for S2 page-table protection bits

[Edgar E. Iglesias]

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

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

Thanks,
Edgar