[Qemu-devel] [PATCH 00/11] target-arm: handle mmu_idx/translation regimes properly

[Qemu-devel] [PATCH 00/11] target-arm: handle mmu_idx/translation regimes properly

[Peter Maydell]

This patchseries fixes up our somewhat broken handling of mmu_idx values:
 * implement the full set of 7 mmu_idxes we need for supporting EL2 and EL3
 * pass the mmu_idx in the TB flags rather than EL or a priv flag,
   so we can generate code with the correct kind of access
 * identify the correct mmu_idx to use for AT/ATS system insns
 * pass mmu_idx into get_phys_addr() and use it within that family
   of functions as an indication of which translation regime to do
   a v-to-p lookup for, instead of relying on an is_user flag plus the
   current CPU state
 * some minor indent stuff on the end

It does not contain:
 * complete support for EL2 or 64-bit EL3; in some places I have added
   the code where it was obvious and easy; in others I have just left
   TODO marker comments
 * the 'tlb_flush_for_mmuidx' functionality I proposed in a previous mail;
   I preferred to get the semantics right in this patchset first before
   improving the efficiency later

Peter Maydell (11):
  cpu_ldst.h: Allow NB_MMU_MODES to be 7
  target-arm: Make arm_current_el() return sensible values for M profile
  target-arm/translate-a64: Fix wrong mmu_idx usage for LDT/STT
  target-arm: Define correct mmu_idx values and pass them in TB flags
  target-arm: Use correct mmu_idx for unprivileged loads and stores
  target-arm: Don't define any MMU_MODE*_SUFFIXes
  target-arm: Split AArch64 cases out of ats_write()
  target-arm: Pass mmu_idx to get_phys_addr()
  target-arm: Use mmu_idx in get_phys_addr()
  target-arm: Reindent ancient page-table-walk code
  target-arm: Fix brace style in reindented code

 include/exec/cpu_ldst.h    |  28 ++-
 target-arm/cpu.h           | 119 ++++++++--
 target-arm/helper.c        | 534 +++++++++++++++++++++++++++++++--------------
 target-arm/translate-a64.c |  24 +-
 target-arm/translate.c     |  31 ++-
 target-arm/translate.h     |   3 +-
 6 files changed, 544 insertions(+), 195 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 01/11] cpu_ldst.h: Allow NB_MMU_MODES to be 7

[Peter Maydell]

Support guest CPUs which need 7 MMU index values.
Add a comment about what would be required to raise the limit
further (trivial for 8, TCG backend rework for 9 or more).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/exec/cpu_ldst.h | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
index 0e825ea..fa5ea63 100644
--- a/include/exec/cpu_ldst.h
+++ b/include/exec/cpu_ldst.h
@@ -244,9 +244,31 @@ uint64_t helper_ldq_cmmu(CPUArchState *env, target_ulong addr, int mmu_idx);
 #undef MEMSUFFIX
 #endif /* (NB_MMU_MODES >= 6) */
 
-#if (NB_MMU_MODES > 6)
-#error "NB_MMU_MODES > 6 is not supported for now"
-#endif /* (NB_MMU_MODES > 6) */
+#if (NB_MMU_MODES >= 7) && defined(MMU_MODE6_SUFFIX)
+
+#define CPU_MMU_INDEX 6
+#define MEMSUFFIX MMU_MODE5_SUFFIX
+#define DATA_SIZE 1
+#include "exec/cpu_ldst_template.h"
+
+#define DATA_SIZE 2
+#include "exec/cpu_ldst_template.h"
+
+#define DATA_SIZE 4
+#include "exec/cpu_ldst_template.h"
+
+#define DATA_SIZE 8
+#include "exec/cpu_ldst_template.h"
+#undef CPU_MMU_INDEX
+#undef MEMSUFFIX
+#endif /* (NB_MMU_MODES >= 7) */
+
+#if (NB_MMU_MODES > 7)
+/* Note that supporting NB_MMU_MODES == 9 would require
+ * changes to at least the ARM TCG backend.
+ */
+#error "NB_MMU_MODES > 7 is not supported for now"
+#endif /* (NB_MMU_MODES > 7) */
 
 /* these access are slower, they must be as rare as possible */
 #define CPU_MMU_INDEX (cpu_mmu_index(env))
-- 
1.9.1

[Qemu-devel] [PATCH 02/11] target-arm: Make arm_current_el() return sensible values for M profile

[Peter Maydell]

Although M profile doesn't have the same concept of exception level
as A profile, it does have a notion of privileged versus not, which
we currently track in the privmode TB flag. Support returning this
information if arm_current_el() is called on an M profile core, so
that we can identify the correct MMU index to use (and put the MMU
index in the TB flags) without having to special-case M profile.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/cpu.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index cd7a9e8..3eb00f4 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -1211,6 +1211,10 @@ static inline bool cptype_valid(int cptype)
  */
 static inline int arm_current_el(CPUARMState *env)
 {
+    if (arm_feature(env, ARM_FEATURE_M)) {
+        return !((env->v7m.exception == 0) && (env->v7m.control & 1));
+    }
+
     if (is_a64(env)) {
         return extract32(env->pstate, 2, 2);
     }
-- 
1.9.1

[Qemu-devel] [PATCH 03/11] target-arm/translate-a64: Fix wrong mmu_idx usage for LDT/STT

[Peter Maydell]

The LDT/STT (load/store unprivileged) instruction decode was using
the wrong MMU index value. This meant that instead of these insns
being "always access as if user-mode regardless of current privilege"
they were "always access as if kernel-mode regardless of current
privilege". This went unnoticed because AArch64 Linux doesn't use
these instructions.

Cc: qemu-stable@nongnu.org

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I'm not counting this as a security issue because I'm assuming
nobody treats TCG guests as a security boundary (certainly I
would not recommend doing so...)
---
 target-arm/translate-a64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index 80d2359..dac2f63 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -2107,7 +2107,7 @@ static void disas_ldst_reg_imm9(DisasContext *s, uint32_t insn)
         }
     } else {
         TCGv_i64 tcg_rt = cpu_reg(s, rt);
-        int memidx = is_unpriv ? 1 : get_mem_index(s);
+        int memidx = is_unpriv ? MMU_USER_IDX : get_mem_index(s);
 
         if (is_store) {
             do_gpr_st_memidx(s, tcg_rt, tcg_addr, size, memidx);
-- 
1.9.1

[Qemu-devel] [PATCH 04/11] target-arm: Define correct mmu_idx values and pass them in TB flags

[Peter Maydell]

We currently claim that for ARM the mmu_idx should simply be the current
exception level. However this isn't actually correct -- secure EL0 and EL1
should have separate indexes from non-secure EL0 and EL1 since their
VA->PA mappings may differ. We also will want an index for stage 2
translations when we properly support EL2.

Define and document all seven mmu index values that we require, and
pass the mmu index in the TB flags rather than exception level or
priv/user bit.

This change doesn't update the get_phys_addr() code, so our page
table walking still assumes a simplistic "user or priv?" model for
the moment.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
This leaves some odd gaps in the TB flags usage. I will circle
back and clean this up later (including moving the other common
flags like the singlestep ones to the top of the flags word),
but I didn't want to bloat this patchseries further.
---
 target-arm/cpu.h           | 113 ++++++++++++++++++++++++++++++++++++---------
 target-arm/helper.c        |   3 +-
 target-arm/translate-a64.c |   5 +-
 target-arm/translate.c     |   5 +-
 target-arm/translate.h     |   3 +-
 5 files changed, 101 insertions(+), 28 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 3eb00f4..cf7b9ab 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -98,7 +98,7 @@ typedef uint32_t ARMReadCPFunc(void *opaque, int cp_info,
 
 struct arm_boot_info;
 
-#define NB_MMU_MODES 4
+#define NB_MMU_MODES 7
 
 /* We currently assume float and double are IEEE single and double
    precision respectively.
@@ -1572,13 +1572,92 @@ static inline CPUARMState *cpu_init(const char *cpu_model)
 #define cpu_signal_handler cpu_arm_signal_handler
 #define cpu_list arm_cpu_list
 
-/* MMU modes definitions */
+/* ARM has the following "translation regimes" (as the ARM ARM calls them):
+ *
+ * If EL3 is 64-bit:
+ *  + NonSecure EL1 & 0 stage 1
+ *  + NonSecure EL1 & 0 stage 2
+ *  + NonSecure EL2
+ *  + Secure EL1 & EL0
+ *  + Secure EL3
+ * If EL3 is 32-bit:
+ *  + NonSecure PL1 & 0 stage 1
+ *  + NonSecure PL1 & 0 stage 2
+ *  + NonSecure PL2
+ *  + Secure PL0 & PL1
+ * (reminder: for 32 bit EL3, Secure PL1 is *EL3*, not EL1.)
+ *
+ * For QEMU, an mmu_idx is not quite the same as a translation regime because:
+ *  1. we need to split the "EL1 & 0" regimes into two mmu_idxes, because they
+ *     may differ in access permissions even if the VA->PA map is the same
+ *  2. we want to cache in our TLB the full VA->IPA->PA lookup for a stage 1+2
+ *     translation, which means that we have one mmu_idx that deals with two
+ *     concatenated translation regimes [this sort of combined s1+2 TLB is
+ *     architecturally permitted]
+ *  3. we don't need to allocate an mmu_idx to translations that we won't be
+ *     handling via the TLB. The only way to do a stage 1 translation without
+ *     the immediate stage 2 translation is via the ATS or AT system insns,
+ *     which can be slow-pathed and always do a page table walk.
+ *  4. we can also safely fold together the "32 bit EL3" and "64 bit EL3"
+ *     translation regimes, because they map reasonably well to each other
+ *     and they can't both be active at the same time.
+ * This gives us the following list of mmu_idx values:
+ *
+ * NS EL0 (aka NS PL0) stage 1+2
+ * NS EL1 (aka NS PL1) stage 1+2
+ * NS EL2 (aka NS PL2)
+ * S EL3 (aka S PL1)
+ * S EL0 (aka S PL0)
+ * S EL1 (not used if EL3 is 32 bit)
+ * NS EL0+1 stage 2
+ *
+ * (The last of these is an mmu_idx because we want to be able to use the TLB
+ * for the accesses done as part of a stage 1 page table walk, rather than
+ * having to walk the stage 2 page table over and over.)
+ *
+ * Our enumeration includes at the end some entries which are not "true"
+ * mmu_idx values in that they don't have corresponding TLBs and are only
+ * valid for doing slow path page table walks.
+ *
+ * The constant names here are patterned after the general style of the names
+ * of the AT/ATS operations.
+ * The values used are carefully arranged to make mmu_idx => EL lookup easy.
+ */
+typedef enum ARMMMUIdx {
+    ARMMMUIdx_S12NSE0 = 0,
+    ARMMMUIdx_S12NSE1 = 1,
+    ARMMMUIdx_S1E2 = 2,
+    ARMMMUIdx_S1E3 = 3,
+    ARMMMUIdx_S1SE0 = 4,
+    ARMMMUIdx_S1SE1 = 5,
+    ARMMMUIdx_S2NS = 6,
+    /* Indexes below here don't have TLBs and are used only for AT system
+     * instructions or for the first stage of an S12 page table walk.
+     */
+    ARMMMUIdx_S1NSE0 = 7,
+    ARMMMUIdx_S1NSE1 = 8,
+} ARMMMUIdx;
+
 #define MMU_MODE0_SUFFIX _user
 #define MMU_MODE1_SUFFIX _kernel
 #define MMU_USER_IDX 0
+
+/* Return the exception level we're running at if this is our mmu_idx */
+static inline int arm_mmu_idx_to_el(ARMMMUIdx mmu_idx)
+{
+    assert(mmu_idx < ARMMMUIdx_S2NS);
+    return mmu_idx & 3;
+}
+
+/* Determine the current mmu_idx to use for normal loads/stores */
 static inline int cpu_mmu_index (CPUARMState *env)
 {
-    return arm_current_el(env);
+    int el = arm_current_el(env);
+
+    if (el < 3 && arm_is_secure_below_el3(env)) {
+        return ARMMMUIdx_S1SE0 + el;
+    }
+    return el;
 }
 
 /* Return the Exception Level targeted by debug exceptions;
@@ -1645,9 +1724,13 @@ static inline bool arm_singlestep_active(CPUARMState *env)
 
 /* Bit usage in the TB flags field: bit 31 indicates whether we are
  * in 32 or 64 bit mode. The meaning of the other bits depends on that.
+ * We put flags which are shared between 32 and 64 bit mode at the top
+ * of the word, and flags which apply to only one mode at the bottom.
  */
 #define ARM_TBFLAG_AARCH64_STATE_SHIFT 31
 #define ARM_TBFLAG_AARCH64_STATE_MASK  (1U << ARM_TBFLAG_AARCH64_STATE_SHIFT)
+#define ARM_TBFLAG_MMUIDX_SHIFT 28
+#define ARM_TBFLAG_MMUIDX_MASK (0x7 << ARM_TBFLAG_MMUIDX_SHIFT)
 
 /* Bit usage when in AArch32 state: */
 #define ARM_TBFLAG_THUMB_SHIFT      0
@@ -1656,8 +1739,6 @@ static inline bool arm_singlestep_active(CPUARMState *env)
 #define ARM_TBFLAG_VECLEN_MASK      (0x7 << ARM_TBFLAG_VECLEN_SHIFT)
 #define ARM_TBFLAG_VECSTRIDE_SHIFT  4
 #define ARM_TBFLAG_VECSTRIDE_MASK   (0x3 << ARM_TBFLAG_VECSTRIDE_SHIFT)
-#define ARM_TBFLAG_PRIV_SHIFT       6
-#define ARM_TBFLAG_PRIV_MASK        (1 << ARM_TBFLAG_PRIV_SHIFT)
 #define ARM_TBFLAG_VFPEN_SHIFT      7
 #define ARM_TBFLAG_VFPEN_MASK       (1 << ARM_TBFLAG_VFPEN_SHIFT)
 #define ARM_TBFLAG_CONDEXEC_SHIFT   8
@@ -1683,8 +1764,6 @@ static inline bool arm_singlestep_active(CPUARMState *env)
 #define ARM_TBFLAG_NS_MASK          (1 << ARM_TBFLAG_NS_SHIFT)
 
 /* Bit usage when in AArch64 state */
-#define ARM_TBFLAG_AA64_EL_SHIFT    0
-#define ARM_TBFLAG_AA64_EL_MASK     (0x3 << ARM_TBFLAG_AA64_EL_SHIFT)
 #define ARM_TBFLAG_AA64_FPEN_SHIFT  2
 #define ARM_TBFLAG_AA64_FPEN_MASK   (1 << ARM_TBFLAG_AA64_FPEN_SHIFT)
 #define ARM_TBFLAG_AA64_SS_ACTIVE_SHIFT 3
@@ -1695,14 +1774,14 @@ static inline bool arm_singlestep_active(CPUARMState *env)
 /* some convenience accessor macros */
 #define ARM_TBFLAG_AARCH64_STATE(F) \
     (((F) & ARM_TBFLAG_AARCH64_STATE_MASK) >> ARM_TBFLAG_AARCH64_STATE_SHIFT)
+#define ARM_TBFLAG_MMUIDX(F) \
+    (((F) & ARM_TBFLAG_MMUIDX_MASK) >> ARM_TBFLAG_MMUIDX_SHIFT)
 #define ARM_TBFLAG_THUMB(F) \
     (((F) & ARM_TBFLAG_THUMB_MASK) >> ARM_TBFLAG_THUMB_SHIFT)
 #define ARM_TBFLAG_VECLEN(F) \
     (((F) & ARM_TBFLAG_VECLEN_MASK) >> ARM_TBFLAG_VECLEN_SHIFT)
 #define ARM_TBFLAG_VECSTRIDE(F) \
     (((F) & ARM_TBFLAG_VECSTRIDE_MASK) >> ARM_TBFLAG_VECSTRIDE_SHIFT)
-#define ARM_TBFLAG_PRIV(F) \
-    (((F) & ARM_TBFLAG_PRIV_MASK) >> ARM_TBFLAG_PRIV_SHIFT)
 #define ARM_TBFLAG_VFPEN(F) \
     (((F) & ARM_TBFLAG_VFPEN_MASK) >> ARM_TBFLAG_VFPEN_SHIFT)
 #define ARM_TBFLAG_CONDEXEC(F) \
@@ -1717,8 +1796,6 @@ static inline bool arm_singlestep_active(CPUARMState *env)
     (((F) & ARM_TBFLAG_PSTATE_SS_MASK) >> ARM_TBFLAG_PSTATE_SS_SHIFT)
 #define ARM_TBFLAG_XSCALE_CPAR(F) \
     (((F) & ARM_TBFLAG_XSCALE_CPAR_MASK) >> ARM_TBFLAG_XSCALE_CPAR_SHIFT)
-#define ARM_TBFLAG_AA64_EL(F) \
-    (((F) & ARM_TBFLAG_AA64_EL_MASK) >> ARM_TBFLAG_AA64_EL_SHIFT)
 #define ARM_TBFLAG_AA64_FPEN(F) \
     (((F) & ARM_TBFLAG_AA64_FPEN_MASK) >> ARM_TBFLAG_AA64_FPEN_SHIFT)
 #define ARM_TBFLAG_AA64_SS_ACTIVE(F) \
@@ -1742,8 +1819,7 @@ static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
 
     if (is_a64(env)) {
         *pc = env->pc;
-        *flags = ARM_TBFLAG_AARCH64_STATE_MASK
-            | (arm_current_el(env) << ARM_TBFLAG_AA64_EL_SHIFT);
+        *flags = ARM_TBFLAG_AARCH64_STATE_MASK;
         if (fpen == 3 || (fpen == 1 && arm_current_el(env) != 0)) {
             *flags |= ARM_TBFLAG_AA64_FPEN_MASK;
         }
@@ -1761,21 +1837,12 @@ static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
             }
         }
     } else {
-        int privmode;
         *pc = env->regs[15];
         *flags = (env->thumb << ARM_TBFLAG_THUMB_SHIFT)
             | (env->vfp.vec_len << ARM_TBFLAG_VECLEN_SHIFT)
             | (env->vfp.vec_stride << ARM_TBFLAG_VECSTRIDE_SHIFT)
             | (env->condexec_bits << ARM_TBFLAG_CONDEXEC_SHIFT)
             | (env->bswap_code << ARM_TBFLAG_BSWAP_CODE_SHIFT);
-        if (arm_feature(env, ARM_FEATURE_M)) {
-            privmode = !((env->v7m.exception == 0) && (env->v7m.control & 1));
-        } else {
-            privmode = (env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_USR;
-        }
-        if (privmode) {
-            *flags |= ARM_TBFLAG_PRIV_MASK;
-        }
         if (!(access_secure_reg(env))) {
             *flags |= ARM_TBFLAG_NS_MASK;
         }
@@ -1803,6 +1870,8 @@ static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
                    << ARM_TBFLAG_XSCALE_CPAR_SHIFT);
     }
 
+    *flags |= (cpu_mmu_index(env) << ARM_TBFLAG_MMUIDX_SHIFT);
+
     *cs_base = 0;
 }
 
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 1a5e067..06478d8 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -5119,7 +5119,8 @@ int arm_cpu_handle_mmu_fault(CPUState *cs, vaddr address,
     uint32_t syn;
     bool same_el = (arm_current_el(env) != 0);
 
-    is_user = mmu_idx == MMU_USER_IDX;
+    /* TODO: pass the translation regime to get_phys_addr */
+    is_user = (arm_mmu_idx_to_el(mmu_idx) == 0);
     ret = get_phys_addr(env, address, access_type, is_user, &phys_addr, &prot,
                         &page_size);
     if (ret == 0) {
diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index dac2f63..96f14ff 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -10922,14 +10922,15 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
     dc->bswap_code = 0;
     dc->condexec_mask = 0;
     dc->condexec_cond = 0;
+    dc->mmu_idx = ARM_TBFLAG_MMUIDX(tb->flags);
+    dc->current_el = arm_mmu_idx_to_el(dc->mmu_idx);
 #if !defined(CONFIG_USER_ONLY)
-    dc->user = (ARM_TBFLAG_AA64_EL(tb->flags) == 0);
+    dc->user = (dc->current_el == 0);
 #endif
     dc->cpacr_fpen = ARM_TBFLAG_AA64_FPEN(tb->flags);
     dc->vec_len = 0;
     dc->vec_stride = 0;
     dc->cp_regs = cpu->cp_regs;
-    dc->current_el = arm_current_el(env);
     dc->features = env->features;
 
     /* Single step state. The code-generation logic here is:
diff --git a/target-arm/translate.c b/target-arm/translate.c
index bdfcdf1..7163649 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -11032,8 +11032,10 @@ static inline void gen_intermediate_code_internal(ARMCPU *cpu,
     dc->bswap_code = ARM_TBFLAG_BSWAP_CODE(tb->flags);
     dc->condexec_mask = (ARM_TBFLAG_CONDEXEC(tb->flags) & 0xf) << 1;
     dc->condexec_cond = ARM_TBFLAG_CONDEXEC(tb->flags) >> 4;
+    dc->mmu_idx = ARM_TBFLAG_MMUIDX(tb->flags);
+    dc->current_el = arm_mmu_idx_to_el(dc->mmu_idx);
 #if !defined(CONFIG_USER_ONLY)
-    dc->user = (ARM_TBFLAG_PRIV(tb->flags) == 0);
+    dc->user = (dc->current_el == 0);
 #endif
     dc->ns = ARM_TBFLAG_NS(tb->flags);
     dc->cpacr_fpen = ARM_TBFLAG_CPACR_FPEN(tb->flags);
@@ -11042,7 +11044,6 @@ static inline void gen_intermediate_code_internal(ARMCPU *cpu,
     dc->vec_stride = ARM_TBFLAG_VECSTRIDE(tb->flags);
     dc->c15_cpar = ARM_TBFLAG_XSCALE_CPAR(tb->flags);
     dc->cp_regs = cpu->cp_regs;
-    dc->current_el = arm_current_el(env);
     dc->features = env->features;
 
     /* Single step state. The code-generation logic here is:
diff --git a/target-arm/translate.h b/target-arm/translate.h
index f6ee789..a1eb5b5 100644
--- a/target-arm/translate.h
+++ b/target-arm/translate.h
@@ -20,6 +20,7 @@ typedef struct DisasContext {
 #if !defined(CONFIG_USER_ONLY)
     int user;
 #endif
+    ARMMMUIdx mmu_idx; /* MMU index to use for normal loads/stores */
     bool ns;        /* Use non-secure CPREG bank on access */
     bool cpacr_fpen; /* FP enabled via CPACR.FPEN */
     bool vfp_enabled; /* FP enabled via FPSCR.EN */
@@ -69,7 +70,7 @@ static inline int arm_dc_feature(DisasContext *dc, int feature)
 
 static inline int get_mem_index(DisasContext *s)
 {
-    return s->current_el;
+    return s->mmu_idx;
 }
 
 /* target-specific extra values for is_jmp */
-- 
1.9.1

[Qemu-devel] [PATCH 05/11] target-arm: Use correct mmu_idx for unprivileged loads and stores

[Peter Maydell]

The MMU index to use for unprivileged loads and stores is more
complicated than we currently implement:
 * for A64, it should be "if at EL1, access as if EL0; otherwise
   access at current EL"
 * for A32/T32, it should be "if EL2, UNPREDICTABLE; otherwise
   access as if at EL0".

In both cases, if we want to make the access for Secure EL0
this is not the same mmu_idx as for Non-Secure EL0.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/translate-a64.c | 19 ++++++++++++++++++-
 target-arm/translate.c     | 26 ++++++++++++++++++++++++--
 2 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index 96f14ff..acf4b16 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -123,6 +123,23 @@ void a64_translate_init(void)
 #endif
 }
 
+static inline ARMMMUIdx get_a64_user_mem_index(DisasContext *s)
+{
+    /* Return the mmu_idx to use for A64 "unprivileged load/store" insns:
+     *  if EL1, access as if EL0; otherwise access at current EL
+     */
+    switch (s->mmu_idx) {
+    case ARMMMUIdx_S12NSE1:
+        return ARMMMUIdx_S12NSE0;
+    case ARMMMUIdx_S1SE1:
+        return ARMMMUIdx_S1SE0;
+    case ARMMMUIdx_S2NS:
+        g_assert_not_reached();
+    default:
+        return s->mmu_idx;
+    }
+}
+
 void aarch64_cpu_dump_state(CPUState *cs, FILE *f,
                             fprintf_function cpu_fprintf, int flags)
 {
@@ -2107,7 +2124,7 @@ static void disas_ldst_reg_imm9(DisasContext *s, uint32_t insn)
         }
     } else {
         TCGv_i64 tcg_rt = cpu_reg(s, rt);
-        int memidx = is_unpriv ? MMU_USER_IDX : get_mem_index(s);
+        int memidx = is_unpriv ? get_a64_user_mem_index(s) : get_mem_index(s);
 
         if (is_store) {
             do_gpr_st_memidx(s, tcg_rt, tcg_addr, size, memidx);
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 7163649..715f65d 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -113,6 +113,28 @@ void arm_translate_init(void)
     a64_translate_init();
 }
 
+static inline ARMMMUIdx get_a32_user_mem_index(DisasContext *s)
+{
+    /* Return the mmu_idx to use for A32/T32 "unprivileged load/store"
+     * insns:
+     *  if PL2, UNPREDICTABLE (we choose to implement as if PL0)
+     *  otherwise, access as if at PL0.
+     */
+    switch (s->mmu_idx) {
+    case ARMMMUIdx_S1E2:        /* this one is UNPREDICTABLE */
+    case ARMMMUIdx_S12NSE0:
+    case ARMMMUIdx_S12NSE1:
+        return ARMMMUIdx_S12NSE0;
+    case ARMMMUIdx_S1E3:
+    case ARMMMUIdx_S1SE0:
+    case ARMMMUIdx_S1SE1:
+        return ARMMMUIdx_S1SE0;
+    case ARMMMUIdx_S2NS:
+    default:
+        g_assert_not_reached();
+    }
+}
+
 static inline TCGv_i32 load_cpu_offset(int offset)
 {
     TCGv_i32 tmp = tcg_temp_new_i32();
@@ -8793,7 +8815,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
             tmp2 = load_reg(s, rn);
             if ((insn & 0x01200000) == 0x00200000) {
                 /* ldrt/strt */
-                i = MMU_USER_IDX;
+                i = get_a32_user_mem_index(s);
             } else {
                 i = get_mem_index(s);
             }
@@ -10173,7 +10195,7 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
                     break;
                 case 0xe: /* User privilege.  */
                     tcg_gen_addi_i32(addr, addr, imm);
-                    memidx = MMU_USER_IDX;
+                    memidx = get_a32_user_mem_index(s);
                     break;
                 case 0x9: /* Post-decrement.  */
                     imm = -imm;
-- 
1.9.1

[Qemu-devel] [PATCH 06/11] target-arm: Don't define any MMU_MODE*_SUFFIXes

[Peter Maydell]

target-arm doesn't use any of the MMU-mode specific cpu ldst
accessor functions. Suppress their generation by not defining
any of the MMU_MODE*_SUFFIX macros. ("user" and "kernel" are
too simplistic as descriptions of indexes 0 and 1 anyway.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/cpu.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index cf7b9ab..d18df8f 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -1638,8 +1638,6 @@ typedef enum ARMMMUIdx {
     ARMMMUIdx_S1NSE1 = 8,
 } ARMMMUIdx;
 
-#define MMU_MODE0_SUFFIX _user
-#define MMU_MODE1_SUFFIX _kernel
 #define MMU_USER_IDX 0
 
 /* Return the exception level we're running at if this is our mmu_idx */
-- 
1.9.1

[Qemu-devel] [PATCH 07/11] target-arm: Split AArch64 cases out of ats_write()

[Peter Maydell]

Instead of simply reusing ats_write() as the handler for both AArch32
and AArch64 address translation operations, use a different function
for each with the common code in a third function. This is necessary
because the semantics for selecting the right translation regime are
different; we are only getting away with sharing currently because
we don't support EL2 and only support EL3 in AArch32.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 06478d8..04bc0a1 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -1435,13 +1435,13 @@ static CPAccessResult ats_access(CPUARMState *env, const ARMCPRegInfo *ri)
     return CP_ACCESS_OK;
 }
 
-static void ats_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
+static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
+                             int access_type, int is_user)
 {
     hwaddr phys_addr;
     target_ulong page_size;
     int prot;
-    int ret, is_user = ri->opc2 & 2;
-    int access_type = ri->opc2 & 1;
+    int ret;
     uint64_t par64;
 
     ret = get_phys_addr(env, value, access_type, is_user,
@@ -1481,9 +1481,28 @@ static void ats_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
                     ((ret & 0xf) << 1) | 1;
         }
     }
+    return par64;
+}
+
+static void ats_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
+{
+    int is_user = ri->opc2 & 2;
+    int access_type = ri->opc2 & 1;
+    uint64_t par64;
+
+    par64 = do_ats_write(env, value, access_type, is_user);
 
     A32_BANKED_CURRENT_REG_SET(env, par, par64);
 }
+
+static void ats_write64(CPUARMState *env, const ARMCPRegInfo *ri,
+                        uint64_t value)
+{
+    int is_user = ri->opc2 & 2;
+    int access_type = ri->opc2 & 1;
+
+    env->cp15.par_el[1] = do_ats_write(env, value, access_type, is_user);
+}
 #endif
 
 static const ARMCPRegInfo vapa_cp_reginfo[] = {
@@ -2257,16 +2276,16 @@ static const ARMCPRegInfo v8_cp_reginfo[] = {
     /* 64 bit address translation operations */
     { .name = "AT_S1E1R", .state = ARM_CP_STATE_AA64,
       .opc0 = 1, .opc1 = 0, .crn = 7, .crm = 8, .opc2 = 0,
-      .access = PL1_W, .type = ARM_CP_NO_MIGRATE, .writefn = ats_write },
+      .access = PL1_W, .type = ARM_CP_NO_MIGRATE, .writefn = ats_write64 },
     { .name = "AT_S1E1W", .state = ARM_CP_STATE_AA64,
       .opc0 = 1, .opc1 = 0, .crn = 7, .crm = 8, .opc2 = 1,
-      .access = PL1_W, .type = ARM_CP_NO_MIGRATE, .writefn = ats_write },
+      .access = PL1_W, .type = ARM_CP_NO_MIGRATE, .writefn = ats_write64 },
     { .name = "AT_S1E0R", .state = ARM_CP_STATE_AA64,
       .opc0 = 1, .opc1 = 0, .crn = 7, .crm = 8, .opc2 = 2,
-      .access = PL1_W, .type = ARM_CP_NO_MIGRATE, .writefn = ats_write },
+      .access = PL1_W, .type = ARM_CP_NO_MIGRATE, .writefn = ats_write64 },
     { .name = "AT_S1E0W", .state = ARM_CP_STATE_AA64,
       .opc0 = 1, .opc1 = 0, .crn = 7, .crm = 8, .opc2 = 3,
-      .access = PL1_W, .type = ARM_CP_NO_MIGRATE, .writefn = ats_write },
+      .access = PL1_W, .type = ARM_CP_NO_MIGRATE, .writefn = ats_write64 },
 #endif
     /* TLB invalidate last level of translation table walk */
     { .name = "TLBIMVALIS", .cp = 15, .opc1 = 0, .crn = 8, .crm = 3, .opc2 = 5,
-- 
1.9.1

[Qemu-devel] [PATCH 08/11] target-arm: Pass mmu_idx to get_phys_addr()

[Peter Maydell]

Make all the callers of get_phys_addr() pass it the correct
mmu_idx rather than just a simple "is_user" flag. This includes
properly decoding the AT/ATS system instructions; we include the
logic for handling all the opc1/opc2 cases because we'll need
them later for supporting EL2/EL3, even if we don't have the
regdef stanzas yet.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c | 110 +++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 96 insertions(+), 14 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 04bc0a1..0ae04eb 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -13,7 +13,7 @@
 
 #ifndef CONFIG_USER_ONLY
 static inline int get_phys_addr(CPUARMState *env, target_ulong address,
-                                int access_type, int is_user,
+                                int access_type, ARMMMUIdx mmu_idx,
                                 hwaddr *phys_ptr, int *prot,
                                 target_ulong *page_size);
 
@@ -1436,7 +1436,7 @@ static CPAccessResult ats_access(CPUARMState *env, const ARMCPRegInfo *ri)
 }
 
 static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
-                             int access_type, int is_user)
+                             int access_type, ARMMMUIdx mmu_idx)
 {
     hwaddr phys_addr;
     target_ulong page_size;
@@ -1444,7 +1444,7 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
     int ret;
     uint64_t par64;
 
-    ret = get_phys_addr(env, value, access_type, is_user,
+    ret = get_phys_addr(env, value, access_type, mmu_idx,
                         &phys_addr, &prot, &page_size);
     if (extended_addresses_enabled(env)) {
         /* ret is a DFSR/IFSR value for the long descriptor
@@ -1486,11 +1486,58 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
 
 static void ats_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
 {
-    int is_user = ri->opc2 & 2;
     int access_type = ri->opc2 & 1;
     uint64_t par64;
+    ARMMMUIdx mmu_idx;
+    int el = arm_current_el(env);
+    bool secure = arm_is_secure_below_el3(env);
 
-    par64 = do_ats_write(env, value, access_type, is_user);
+    switch (ri->opc2 & 6) {
+    case 0:
+        /* stage 1 current state PL1 */
+        switch (el) {
+        case 3:
+            mmu_idx = ARMMMUIdx_S1E3;
+            break;
+        case 2:
+            mmu_idx = ARMMMUIdx_S1NSE1;
+            break;
+        case 1:
+            mmu_idx = secure ? ARMMMUIdx_S1SE1 : ARMMMUIdx_S1NSE1;
+            break;
+        default:
+            g_assert_not_reached();
+        }
+        break;
+    case 2:
+        /* stage 1 current state PL0 */
+        switch (el) {
+        case 3:
+            mmu_idx = ARMMMUIdx_S1SE0;
+            break;
+        case 2:
+            mmu_idx = ARMMMUIdx_S1NSE0;
+            break;
+        case 1:
+            mmu_idx = secure ? ARMMMUIdx_S1SE0 : ARMMMUIdx_S1NSE0;
+            break;
+        default:
+            g_assert_not_reached();
+        }
+        break;
+    case 4:
+        /* stage 1+2 NonSecure PL1 */
+        mmu_idx = ARMMMUIdx_S12NSE1;
+        break;
+    case 6:
+        /* stage 1+2 NonSecure PL0 */
+        mmu_idx = ARMMMUIdx_S12NSE0;
+        break;
+    default:
+        g_assert_not_reached();
+    }
+
+    par64 = do_ats_write(env, value, access_type, mmu_idx);
 
     A32_BANKED_CURRENT_REG_SET(env, par, par64);
 }
@@ -1498,10 +1545,40 @@ static void ats_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
 static void ats_write64(CPUARMState *env, const ARMCPRegInfo *ri,
                         uint64_t value)
 {
-    int is_user = ri->opc2 & 2;
     int access_type = ri->opc2 & 1;
+    ARMMMUIdx mmu_idx;
+    int secure = arm_is_secure_below_el3(env);
+
+    switch (ri->opc2 & 6) {
+    case 0:
+        switch (ri->opc1) {
+        case 0:
+            mmu_idx = secure ? ARMMMUIdx_S1SE1 : ARMMMUIdx_S1NSE1;
+            break;
+        case 4:
+            mmu_idx = ARMMMUIdx_S1E2;
+            break;
+        case 6:
+            mmu_idx = ARMMMUIdx_S1E3;
+            break;
+        default:
+            g_assert_not_reached();
+        }
+        break;
+    case 2:
+        mmu_idx = secure ? ARMMMUIdx_S1SE0 : ARMMMUIdx_S1NSE0;
+        break;
+    case 4:
+        mmu_idx = ARMMMUIdx_S12NSE1;
+        break;
+    case 6:
+        mmu_idx = ARMMMUIdx_S12NSE0;
+        break;
+    default:
+        g_assert_not_reached();
+    }
 
-    env->cp15.par_el[1] = do_ats_write(env, value, access_type, is_user);
+    env->cp15.par_el[1] = do_ats_write(env, value, access_type, mmu_idx);
 }
 #endif
 
@@ -5084,13 +5161,13 @@ static int get_phys_addr_mpu(CPUARMState *env, uint32_t address,
  * @env: CPUARMState
  * @address: virtual address to get physical address for
  * @access_type: 0 for read, 1 for write, 2 for execute
- * @is_user: 0 for privileged access, 1 for user
+ * @mmu_idx: MMU index indicating required translation regime
  * @phys_ptr: set to the physical address corresponding to the virtual address
  * @prot: set to the permissions for the page containing phys_ptr
  * @page_size: set to the size of the page containing phys_ptr
  */
 static inline int get_phys_addr(CPUARMState *env, target_ulong address,
-                                int access_type, int is_user,
+                                int access_type, ARMMMUIdx mmu_idx,
                                 hwaddr *phys_ptr, int *prot,
                                 target_ulong *page_size)
 {
@@ -5099,6 +5176,11 @@ static inline int get_phys_addr(CPUARMState *env, target_ulong address,
      */
     uint32_t sctlr = A32_BANKED_CURRENT_REG_GET(env, sctlr);
 
+    /* This will go away when we handle mmu_idx properly here */
+    int is_user = (mmu_idx == ARMMMUIdx_S12NSE0 ||
+                   mmu_idx == ARMMMUIdx_S1SE0 ||
+                   mmu_idx == ARMMMUIdx_S1NSE0);
+
     /* Fast Context Switch Extension.  */
     if (address < 0x02000000) {
         address += A32_BANKED_CURRENT_REG_GET(env, fcseidr);
@@ -5134,13 +5216,11 @@ int arm_cpu_handle_mmu_fault(CPUState *cs, vaddr address,
     hwaddr phys_addr;
     target_ulong page_size;
     int prot;
-    int ret, is_user;
+    int ret;
     uint32_t syn;
     bool same_el = (arm_current_el(env) != 0);
 
-    /* TODO: pass the translation regime to get_phys_addr */
-    is_user = (arm_mmu_idx_to_el(mmu_idx) == 0);
-    ret = get_phys_addr(env, address, access_type, is_user, &phys_addr, &prot,
+    ret = get_phys_addr(env, address, access_type, mmu_idx, &phys_addr, &prot,
                         &page_size);
     if (ret == 0) {
         /* Map a single [sub]page.  */
@@ -5176,12 +5256,14 @@ int arm_cpu_handle_mmu_fault(CPUState *cs, vaddr address,
 hwaddr arm_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
 {
     ARMCPU *cpu = ARM_CPU(cs);
+    CPUARMState *env = &cpu->env;
     hwaddr phys_addr;
     target_ulong page_size;
     int prot;
     int ret;
 
-    ret = get_phys_addr(&cpu->env, addr, 0, 0, &phys_addr, &prot, &page_size);
+    ret = get_phys_addr(env, addr, 0, cpu_mmu_index(env), &phys_addr,
+                        &prot, &page_size);
 
     if (ret != 0) {
         return -1;
-- 
1.9.1

[Qemu-devel] [PATCH 09/11] target-arm: Use mmu_idx in get_phys_addr()

[Peter Maydell]

Now we have the mmu_idx in get_phys_addr(), use it correctly to
determine the behaviour of virtual to physical address translations,
rather than using just an is_user flag and the current CPU state.

Some TODO comments have been added to indicate where changes will
need to be made to add EL2 and 64-bit EL3 support.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c | 200 +++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 151 insertions(+), 49 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 0ae04eb..0a06bbe 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -4556,13 +4556,88 @@ void arm_cpu_do_interrupt(CPUState *cs)
     cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
 }
 
+
+/* Return the exception level which controls this address translation regime */
+static inline uint32_t regime_el(CPUARMState *env, ARMMMUIdx mmu_idx)
+{
+    switch (mmu_idx) {
+    case ARMMMUIdx_S2NS:
+    case ARMMMUIdx_S1E2:
+        return 2;
+    case ARMMMUIdx_S1E3:
+        return 3;
+    case ARMMMUIdx_S1SE0:
+        return arm_el_is_aa64(env, 3) ? 1 : 3;
+    case ARMMMUIdx_S1SE1:
+    case ARMMMUIdx_S1NSE0:
+    case ARMMMUIdx_S1NSE1:
+        return 1;
+    default:
+        g_assert_not_reached();
+    }
+}
+
+/* Return the SCTLR value which controls this address translation regime */
+static inline uint32_t regime_sctlr(CPUARMState *env, ARMMMUIdx mmu_idx)
+{
+    return env->cp15.sctlr_el[regime_el(env, mmu_idx)];
+}
+
+/* Return true if the specified stage of address translation is disabled */
+static inline bool regime_translation_disabled(CPUARMState *env,
+                                               ARMMMUIdx mmu_idx)
+{
+    if (mmu_idx == ARMMMUIdx_S2NS) {
+        return (env->cp15.hcr_el2 & HCR_VM) == 0;
+    }
+    return (regime_sctlr(env, mmu_idx) & SCTLR_M) == 0;
+}
+
+/* Return the TCR controlling this translation regime */
+static inline TCR *regime_tcr(CPUARMState *env, ARMMMUIdx mmu_idx)
+{
+    if (mmu_idx == ARMMMUIdx_S2NS) {
+        /* TODO: return VTCR_EL2 */
+        g_assert_not_reached();
+    }
+    return &env->cp15.tcr_el[regime_el(env, mmu_idx)];
+}
+
+/* Return true if the translation regime is using LPAE format page tables */
+static inline bool regime_using_lpae_format(CPUARMState *env,
+                                            ARMMMUIdx mmu_idx)
+{
+    int el = regime_el(env, mmu_idx);
+    if (el == 2 || arm_el_is_aa64(env, el)) {
+        return true;
+    }
+    if (arm_feature(env, ARM_FEATURE_LPAE)
+        && (regime_tcr(env, mmu_idx)->raw_tcr & TTBCR_EAE)) {
+        return true;
+    }
+    return false;
+}
+
+static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx)
+{
+    switch (mmu_idx) {
+    case ARMMMUIdx_S1SE0:
+    case ARMMMUIdx_S1NSE0:
+        return true;
+    default:
+        return false;
+    }
+}
+
 /* Check section/page access permissions.
    Returns the page protection flags, or zero if the access is not
    permitted.  */
-static inline int check_ap(CPUARMState *env, int ap, int domain_prot,
-                           int access_type, int is_user)
+static inline int check_ap(CPUARMState *env, ARMMMUIdx mmu_idx,
+                           int ap, int domain_prot,
+                           int access_type)
 {
   int prot_ro;
+  bool is_user = regime_is_user(env, mmu_idx);
 
   if (domain_prot == 3) {
     return PAGE_READ | PAGE_WRITE;
@@ -4580,7 +4655,7 @@ static inline int check_ap(CPUARMState *env, int ap, int domain_prot,
       }
       if (access_type == 1)
           return 0;
-      switch (A32_BANKED_CURRENT_REG_GET(env, sctlr) & (SCTLR_S | SCTLR_R)) {
+      switch (regime_sctlr(env, mmu_idx) & (SCTLR_S | SCTLR_R)) {
       case SCTLR_S:
           return is_user ? 0 : PAGE_READ;
       case SCTLR_R:
@@ -4612,35 +4687,32 @@ static inline int check_ap(CPUARMState *env, int ap, int domain_prot,
   }
 }
 
-static bool get_level1_table_address(CPUARMState *env, uint32_t *table,
-                                         uint32_t address)
+static bool get_level1_table_address(CPUARMState *env, ARMMMUIdx mmu_idx,
+                                     uint32_t *table, uint32_t address)
 {
-    /* Get the TCR bank based on our security state */
-    TCR *tcr = &env->cp15.tcr_el[arm_is_secure(env) ? 3 : 1];
+    /* Note that we can only get here for an AArch32 PL0/PL1 lookup */
+    int el = regime_el(env, mmu_idx);
+    TCR *tcr = regime_tcr(env, mmu_idx);
 
-    /* We only get here if EL1 is running in AArch32. If EL3 is running in
-     * AArch32 there is a secure and non-secure instance of the translation
-     * table registers.
-     */
     if (address & tcr->mask) {
         if (tcr->raw_tcr & TTBCR_PD1) {
             /* Translation table walk disabled for TTBR1 */
             return false;
         }
-        *table = A32_BANKED_CURRENT_REG_GET(env, ttbr1) & 0xffffc000;
+        *table = env->cp15.ttbr1_el[el] & 0xffffc000;
     } else {
         if (tcr->raw_tcr & TTBCR_PD0) {
             /* Translation table walk disabled for TTBR0 */
             return false;
         }
-        *table = A32_BANKED_CURRENT_REG_GET(env, ttbr0) & tcr->base_mask;
+        *table = env->cp15.ttbr0_el[el] & tcr->base_mask;
     }
     *table |= (address >> 18) & 0x3ffc;
     return true;
 }
 
 static int get_phys_addr_v5(CPUARMState *env, uint32_t address, int access_type,
-                            int is_user, hwaddr *phys_ptr,
+                            ARMMMUIdx mmu_idx, hwaddr *phys_ptr,
                             int *prot, target_ulong *page_size)
 {
     CPUState *cs = CPU(arm_env_get_cpu(env));
@@ -4652,10 +4724,11 @@ static int get_phys_addr_v5(CPUARMState *env, uint32_t address, int access_type,
     int domain = 0;
     int domain_prot;
     hwaddr phys_addr;
+    uint32_t dacr;
 
     /* Pagetable walk.  */
     /* Lookup l1 descriptor.  */
-    if (!get_level1_table_address(env, &table, address)) {
+    if (!get_level1_table_address(env, mmu_idx, &table, address)) {
         /* Section translation fault if page walk is disabled by PD0 or PD1 */
         code = 5;
         goto do_fault;
@@ -4663,7 +4736,12 @@ static int get_phys_addr_v5(CPUARMState *env, uint32_t address, int access_type,
     desc = ldl_phys(cs->as, table);
     type = (desc & 3);
     domain = (desc >> 5) & 0x0f;
-    domain_prot = (A32_BANKED_CURRENT_REG_GET(env, dacr) >> (domain * 2)) & 3;
+    if (regime_el(env, mmu_idx) == 1) {
+        dacr = env->cp15.dacr_ns;
+    } else {
+        dacr = env->cp15.dacr_s;
+    }
+    domain_prot = (dacr >> (domain * 2)) & 3;
     if (type == 0) {
         /* Section translation fault.  */
         code = 5;
@@ -4727,7 +4805,7 @@ static int get_phys_addr_v5(CPUARMState *env, uint32_t address, int access_type,
         }
         code = 15;
     }
-    *prot = check_ap(env, ap, domain_prot, access_type, is_user);
+    *prot = check_ap(env, mmu_idx, ap, domain_prot, access_type);
     if (!*prot) {
         /* Access permission fault.  */
         goto do_fault;
@@ -4740,7 +4818,7 @@ do_fault:
 }
 
 static int get_phys_addr_v6(CPUARMState *env, uint32_t address, int access_type,
-                            int is_user, hwaddr *phys_ptr,
+                            ARMMMUIdx mmu_idx, hwaddr *phys_ptr,
                             int *prot, target_ulong *page_size)
 {
     CPUState *cs = CPU(arm_env_get_cpu(env));
@@ -4754,10 +4832,11 @@ static int get_phys_addr_v6(CPUARMState *env, uint32_t address, int access_type,
     int domain = 0;
     int domain_prot;
     hwaddr phys_addr;
+    uint32_t dacr;
 
     /* Pagetable walk.  */
     /* Lookup l1 descriptor.  */
-    if (!get_level1_table_address(env, &table, address)) {
+    if (!get_level1_table_address(env, mmu_idx, &table, address)) {
         /* Section translation fault if page walk is disabled by PD0 or PD1 */
         code = 5;
         goto do_fault;
@@ -4775,7 +4854,12 @@ static int get_phys_addr_v6(CPUARMState *env, uint32_t address, int access_type,
         /* Page or Section.  */
         domain = (desc >> 5) & 0x0f;
     }
-    domain_prot = (A32_BANKED_CURRENT_REG_GET(env, dacr) >> (domain * 2)) & 3;
+    if (regime_el(env, mmu_idx) == 1) {
+        dacr = env->cp15.dacr_ns;
+    } else {
+        dacr = env->cp15.dacr_s;
+    }
+    domain_prot = (dacr >> (domain * 2)) & 3;
     if (domain_prot == 0 || domain_prot == 2) {
         if (type != 1) {
             code = 9; /* Section domain fault.  */
@@ -4829,20 +4913,20 @@ static int get_phys_addr_v6(CPUARMState *env, uint32_t address, int access_type,
     if (domain_prot == 3) {
         *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
     } else {
-        if (pxn && !is_user) {
+        if (pxn && !regime_is_user(env, mmu_idx)) {
             xn = 1;
         }
         if (xn && access_type == 2)
             goto do_fault;
 
         /* The simplified model uses AP[0] as an access control bit.  */
-        if ((A32_BANKED_CURRENT_REG_GET(env, sctlr) & SCTLR_AFE)
+        if ((regime_sctlr(env, mmu_idx) & SCTLR_AFE)
                 && (ap & 1) == 0) {
             /* Access flag fault.  */
             code = (code == 15) ? 6 : 3;
             goto do_fault;
         }
-        *prot = check_ap(env, ap, domain_prot, access_type, is_user);
+        *prot = check_ap(env, mmu_idx, ap, domain_prot, access_type);
         if (!*prot) {
             /* Access permission fault.  */
             goto do_fault;
@@ -4867,7 +4951,7 @@ typedef enum {
 } MMUFaultType;
 
 static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
-                              int access_type, int is_user,
+                              int access_type, ARMMMUIdx mmu_idx,
                               hwaddr *phys_ptr, int *prot,
                               target_ulong *page_size_ptr)
 {
@@ -4887,9 +4971,17 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
     int32_t granule_sz = 9;
     int32_t va_size = 32;
     int32_t tbi = 0;
-    TCR *tcr = &env->cp15.tcr_el[arm_is_secure(env) ? 3 : 1];
-
-    if (arm_el_is_aa64(env, 1)) {
+    bool is_user;
+    TCR *tcr = regime_tcr(env, mmu_idx);
+
+    /* TODO:
+     * This code assumes we're either a 64-bit EL1 or a 32-bit PL1;
+     * it doesn't handle the different format TCR for TCR_EL2, TCR_EL3,
+     * and VTCR_EL2, or the fact that those regimes don't have a split
+     * TTBR0/TTBR1. Attribute and permission bit handling should also
+     * be checked when adding support for those page table walks.
+     */
+    if (arm_el_is_aa64(env, regime_el(env, mmu_idx))) {
         va_size = 64;
         if (extract64(address, 55, 1))
             tbi = extract64(tcr->raw_tcr, 38, 1);
@@ -4904,12 +4996,12 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
      * TTBCR/TTBR0/TTBR1 in accordance with ARM ARM DDI0406C table B-32:
      */
     uint32_t t0sz = extract32(tcr->raw_tcr, 0, 6);
-    if (arm_el_is_aa64(env, 1)) {
+    if (va_size == 64) {
         t0sz = MIN(t0sz, 39);
         t0sz = MAX(t0sz, 16);
     }
     uint32_t t1sz = extract32(tcr->raw_tcr, 16, 6);
-    if (arm_el_is_aa64(env, 1)) {
+    if (va_size == 64) {
         t1sz = MIN(t1sz, 39);
         t1sz = MAX(t1sz, 16);
     }
@@ -4964,6 +5056,10 @@ static int 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
+     */
+
     if (epd) {
         /* Translation table walk disabled => Translation fault on TLB miss */
         goto do_fault;
@@ -5049,6 +5145,7 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
         goto do_fault;
     }
     fault_type = permission_fault;
+    is_user = regime_is_user(env, mmu_idx);
     if (is_user && !(attrs & (1 << 4))) {
         /* Unprivileged access not enabled */
         goto do_fault;
@@ -5083,12 +5180,13 @@ do_fault:
 }
 
 static int get_phys_addr_mpu(CPUARMState *env, uint32_t address,
-                             int access_type, int is_user,
+                             int access_type, ARMMMUIdx mmu_idx,
                              hwaddr *phys_ptr, int *prot)
 {
     int n;
     uint32_t mask;
     uint32_t base;
+    bool is_user = regime_is_user(env, mmu_idx);
 
     *phys_ptr = address;
     for (n = 7; n >= 0; n--) {
@@ -5171,39 +5269,43 @@ static inline int get_phys_addr(CPUARMState *env, target_ulong address,
                                 hwaddr *phys_ptr, int *prot,
                                 target_ulong *page_size)
 {
-    /* This is not entirely correct as get_phys_addr() can also be called
-     * from ats_write() for an address translation of a specific regime.
-     */
-    uint32_t sctlr = A32_BANKED_CURRENT_REG_GET(env, sctlr);
-
-    /* This will go away when we handle mmu_idx properly here */
-    int is_user = (mmu_idx == ARMMMUIdx_S12NSE0 ||
-                   mmu_idx == ARMMMUIdx_S1SE0 ||
-                   mmu_idx == ARMMMUIdx_S1NSE0);
+    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 ldl_phys
+         * calls for stage 1 will also need changing.
+         * For non-EL2 CPUs a stage1+stage2 translation is just stage 1.
+         */
+        assert(!arm_feature(env, ARM_FEATURE_EL2));
+        mmu_idx += ARMMMUIdx_S1NSE0;
+    }
 
     /* Fast Context Switch Extension.  */
-    if (address < 0x02000000) {
+    if (address < 0x02000000 && mmu_idx != ARMMMUIdx_S2NS) {
         address += A32_BANKED_CURRENT_REG_GET(env, fcseidr);
     }
 
-    if ((sctlr & SCTLR_M) == 0) {
+    if (regime_translation_disabled(env, mmu_idx)) {
         /* MMU/MPU disabled.  */
         *phys_ptr = address;
         *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
         *page_size = TARGET_PAGE_SIZE;
         return 0;
-    } else if (arm_feature(env, ARM_FEATURE_MPU)) {
+    }
+
+    if (arm_feature(env, ARM_FEATURE_MPU)) {
         *page_size = TARGET_PAGE_SIZE;
-	return get_phys_addr_mpu(env, address, access_type, is_user, phys_ptr,
-				 prot);
-    } else if (extended_addresses_enabled(env)) {
-        return get_phys_addr_lpae(env, address, access_type, is_user, phys_ptr,
+        return get_phys_addr_mpu(env, address, access_type, mmu_idx, phys_ptr,
+                                 prot);
+    }
+
+    if (regime_using_lpae_format(env, mmu_idx)) {
+        return get_phys_addr_lpae(env, address, access_type, mmu_idx, phys_ptr,
                                   prot, page_size);
-    } else if (sctlr & SCTLR_XP) {
-        return get_phys_addr_v6(env, address, access_type, is_user, phys_ptr,
+    } else if (regime_sctlr(env, mmu_idx) & SCTLR_XP) {
+        return get_phys_addr_v6(env, address, access_type, mmu_idx, phys_ptr,
                                 prot, page_size);
     } else {
-        return get_phys_addr_v5(env, address, access_type, is_user, phys_ptr,
+        return get_phys_addr_v5(env, address, access_type, mmu_idx, phys_ptr,
                                 prot, page_size);
     }
 }
-- 
1.9.1

[Qemu-devel] [PATCH 10/11] target-arm: Reindent ancient page-table-walk code

[Peter Maydell]

A few of the oldest parts of the page-table-walk code have broken indent
(either hardcoded tabs or two-spaces). Reindent these sections.

For ease of review, this patch does not touch the brace style and
so is a whitespace-only change.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c | 192 ++++++++++++++++++++++++++--------------------------
 1 file changed, 96 insertions(+), 96 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 0a06bbe..3a23af8 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -4636,55 +4636,55 @@ static inline int check_ap(CPUARMState *env, ARMMMUIdx mmu_idx,
                            int ap, int domain_prot,
                            int access_type)
 {
-  int prot_ro;
-  bool is_user = regime_is_user(env, mmu_idx);
-
-  if (domain_prot == 3) {
-    return PAGE_READ | PAGE_WRITE;
-  }
-
-  if (access_type == 1)
-      prot_ro = 0;
-  else
-      prot_ro = PAGE_READ;
-
-  switch (ap) {
-  case 0:
-      if (arm_feature(env, ARM_FEATURE_V7)) {
-          return 0;
-      }
-      if (access_type == 1)
-          return 0;
-      switch (regime_sctlr(env, mmu_idx) & (SCTLR_S | SCTLR_R)) {
-      case SCTLR_S:
-          return is_user ? 0 : PAGE_READ;
-      case SCTLR_R:
-          return PAGE_READ;
-      default:
-          return 0;
-      }
-  case 1:
-      return is_user ? 0 : PAGE_READ | PAGE_WRITE;
-  case 2:
-      if (is_user)
-          return prot_ro;
-      else
-          return PAGE_READ | PAGE_WRITE;
-  case 3:
-      return PAGE_READ | PAGE_WRITE;
-  case 4: /* Reserved.  */
-      return 0;
-  case 5:
-      return is_user ? 0 : prot_ro;
-  case 6:
-      return prot_ro;
-  case 7:
-      if (!arm_feature (env, ARM_FEATURE_V6K))
-          return 0;
-      return prot_ro;
-  default:
-      abort();
-  }
+    int prot_ro;
+    bool is_user = regime_is_user(env, mmu_idx);
+
+    if (domain_prot == 3) {
+        return PAGE_READ | PAGE_WRITE;
+    }
+
+    if (access_type == 1)
+        prot_ro = 0;
+    else
+        prot_ro = PAGE_READ;
+
+    switch (ap) {
+    case 0:
+        if (arm_feature(env, ARM_FEATURE_V7)) {
+            return 0;
+        }
+        if (access_type == 1)
+            return 0;
+        switch (regime_sctlr(env, mmu_idx) & (SCTLR_S | SCTLR_R)) {
+        case SCTLR_S:
+            return is_user ? 0 : PAGE_READ;
+        case SCTLR_R:
+            return PAGE_READ;
+        default:
+            return 0;
+        }
+    case 1:
+        return is_user ? 0 : PAGE_READ | PAGE_WRITE;
+    case 2:
+        if (is_user)
+            return prot_ro;
+        else
+            return PAGE_READ | PAGE_WRITE;
+    case 3:
+        return PAGE_READ | PAGE_WRITE;
+    case 4: /* Reserved.  */
+        return 0;
+    case 5:
+        return is_user ? 0 : prot_ro;
+    case 6:
+        return prot_ro;
+    case 7:
+        if (!arm_feature (env, ARM_FEATURE_V6K))
+            return 0;
+        return prot_ro;
+    default:
+        abort();
+    }
 }
 
 static bool get_level1_table_address(CPUARMState *env, ARMMMUIdx mmu_idx,
@@ -4762,13 +4762,13 @@ static int get_phys_addr_v5(CPUARMState *env, uint32_t address, int access_type,
         *page_size = 1024 * 1024;
     } else {
         /* Lookup l2 entry.  */
-	if (type == 1) {
-	    /* Coarse pagetable.  */
-	    table = (desc & 0xfffffc00) | ((address >> 10) & 0x3fc);
-	} else {
-	    /* Fine pagetable.  */
-	    table = (desc & 0xfffff000) | ((address >> 8) & 0xffc);
-	}
+        if (type == 1) {
+            /* Coarse pagetable.  */
+            table = (desc & 0xfffffc00) | ((address >> 10) & 0x3fc);
+        } else {
+            /* Fine pagetable.  */
+            table = (desc & 0xfffff000) | ((address >> 8) & 0xffc);
+        }
         desc = ldl_phys(cs->as, table);
         switch (desc & 3) {
         case 0: /* Page translation fault.  */
@@ -4785,17 +4785,17 @@ static int get_phys_addr_v5(CPUARMState *env, uint32_t address, int access_type,
             *page_size = 0x1000;
             break;
         case 3: /* 1k page.  */
-	    if (type == 1) {
-		if (arm_feature(env, ARM_FEATURE_XSCALE)) {
-		    phys_addr = (desc & 0xfffff000) | (address & 0xfff);
-		} else {
-		    /* Page translation fault.  */
-		    code = 7;
-		    goto do_fault;
-		}
-	    } else {
-		phys_addr = (desc & 0xfffffc00) | (address & 0x3ff);
-	    }
+            if (type == 1) {
+                if (arm_feature(env, ARM_FEATURE_XSCALE)) {
+                    phys_addr = (desc & 0xfffff000) | (address & 0xfff);
+                } else {
+                    /* Page translation fault.  */
+                    code = 7;
+                    goto do_fault;
+                }
+            } else {
+                phys_addr = (desc & 0xfffffc00) | (address & 0x3ff);
+            }
             ap = (desc >> 4) & 3;
             *page_size = 0x400;
             break;
@@ -5190,18 +5190,18 @@ static int get_phys_addr_mpu(CPUARMState *env, uint32_t address,
 
     *phys_ptr = address;
     for (n = 7; n >= 0; n--) {
-	base = env->cp15.c6_region[n];
-	if ((base & 1) == 0)
-	    continue;
-	mask = 1 << ((base >> 1) & 0x1f);
-	/* Keep this shift separate from the above to avoid an
-	   (undefined) << 32.  */
-	mask = (mask << 1) - 1;
-	if (((base ^ address) & ~mask) == 0)
-	    break;
+        base = env->cp15.c6_region[n];
+        if ((base & 1) == 0)
+            continue;
+        mask = 1 << ((base >> 1) & 0x1f);
+        /* Keep this shift separate from the above to avoid an
+           (undefined) << 32.  */
+        mask = (mask << 1) - 1;
+        if (((base ^ address) & ~mask) == 0)
+            break;
     }
     if (n < 0)
-	return 2;
+        return 2;
 
     if (access_type == 2) {
         mask = env->cp15.pmsav5_insn_ap;
@@ -5211,31 +5211,31 @@ static int get_phys_addr_mpu(CPUARMState *env, uint32_t address,
     mask = (mask >> (n * 4)) & 0xf;
     switch (mask) {
     case 0:
-	return 1;
+        return 1;
     case 1:
-	if (is_user)
-	  return 1;
-	*prot = PAGE_READ | PAGE_WRITE;
-	break;
+        if (is_user)
+          return 1;
+        *prot = PAGE_READ | PAGE_WRITE;
+        break;
     case 2:
-	*prot = PAGE_READ;
-	if (!is_user)
-	    *prot |= PAGE_WRITE;
-	break;
+        *prot = PAGE_READ;
+        if (!is_user)
+            *prot |= PAGE_WRITE;
+        break;
     case 3:
-	*prot = PAGE_READ | PAGE_WRITE;
-	break;
+        *prot = PAGE_READ | PAGE_WRITE;
+        break;
     case 5:
-	if (is_user)
-	    return 1;
-	*prot = PAGE_READ;
-	break;
+        if (is_user)
+            return 1;
+        *prot = PAGE_READ;
+        break;
     case 6:
-	*prot = PAGE_READ;
-	break;
+        *prot = PAGE_READ;
+        break;
     default:
-	/* Bad permission.  */
-	return 1;
+        /* Bad permission.  */
+        return 1;
     }
     *prot |= PAGE_EXEC;
     return 0;
-- 
1.9.1

[Qemu-devel] [PATCH 11/11] target-arm: Fix brace style in reindented code

[Peter Maydell]

This patch fixes the brace style in the code reindented in the
previous commit.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c | 36 +++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 3a23af8..cc80829 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -4643,18 +4643,20 @@ static inline int check_ap(CPUARMState *env, ARMMMUIdx mmu_idx,
         return PAGE_READ | PAGE_WRITE;
     }
 
-    if (access_type == 1)
+    if (access_type == 1) {
         prot_ro = 0;
-    else
+    } else {
         prot_ro = PAGE_READ;
+    }
 
     switch (ap) {
     case 0:
         if (arm_feature(env, ARM_FEATURE_V7)) {
             return 0;
         }
-        if (access_type == 1)
+        if (access_type == 1) {
             return 0;
+        }
         switch (regime_sctlr(env, mmu_idx) & (SCTLR_S | SCTLR_R)) {
         case SCTLR_S:
             return is_user ? 0 : PAGE_READ;
@@ -4666,10 +4668,11 @@ static inline int check_ap(CPUARMState *env, ARMMMUIdx mmu_idx,
     case 1:
         return is_user ? 0 : PAGE_READ | PAGE_WRITE;
     case 2:
-        if (is_user)
+        if (is_user) {
             return prot_ro;
-        else
+        } else {
             return PAGE_READ | PAGE_WRITE;
+        }
     case 3:
         return PAGE_READ | PAGE_WRITE;
     case 4: /* Reserved.  */
@@ -4679,8 +4682,9 @@ static inline int check_ap(CPUARMState *env, ARMMMUIdx mmu_idx,
     case 6:
         return prot_ro;
     case 7:
-        if (!arm_feature (env, ARM_FEATURE_V6K))
+        if (!arm_feature(env, ARM_FEATURE_V6K)) {
             return 0;
+        }
         return prot_ro;
     default:
         abort();
@@ -5191,17 +5195,20 @@ static int get_phys_addr_mpu(CPUARMState *env, uint32_t address,
     *phys_ptr = address;
     for (n = 7; n >= 0; n--) {
         base = env->cp15.c6_region[n];
-        if ((base & 1) == 0)
+        if ((base & 1) == 0) {
             continue;
+        }
         mask = 1 << ((base >> 1) & 0x1f);
         /* Keep this shift separate from the above to avoid an
            (undefined) << 32.  */
         mask = (mask << 1) - 1;
-        if (((base ^ address) & ~mask) == 0)
+        if (((base ^ address) & ~mask) == 0) {
             break;
+        }
     }
-    if (n < 0)
+    if (n < 0) {
         return 2;
+    }
 
     if (access_type == 2) {
         mask = env->cp15.pmsav5_insn_ap;
@@ -5213,21 +5220,24 @@ static int get_phys_addr_mpu(CPUARMState *env, uint32_t address,
     case 0:
         return 1;
     case 1:
-        if (is_user)
-          return 1;
+        if (is_user) {
+            return 1;
+        }
         *prot = PAGE_READ | PAGE_WRITE;
         break;
     case 2:
         *prot = PAGE_READ;
-        if (!is_user)
+        if (!is_user) {
             *prot |= PAGE_WRITE;
+        }
         break;
     case 3:
         *prot = PAGE_READ | PAGE_WRITE;
         break;
     case 5:
-        if (is_user)
+        if (is_user) {
             return 1;
+        }
         *prot = PAGE_READ;
         break;
     case 6:
-- 
1.9.1

Re: [Qemu-devel] [PATCH 01/11] cpu_ldst.h: Allow NB_MMU_MODES to be 7

[Greg Bellows]

On Fri, Jan 23, 2015 at 12:20 PM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> Support guest CPUs which need 7 MMU index values.
> Add a comment about what would be required to raise the limit
> further (trivial for 8, TCG backend rework for 9 or more).
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/exec/cpu_ldst.h | 28 +++++++++++++++++++++++++---
>  1 file changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
> index 0e825ea..fa5ea63 100644
> --- a/include/exec/cpu_ldst.h
> +++ b/include/exec/cpu_ldst.h
> @@ -244,9 +244,31 @@ uint64_t helper_ldq_cmmu(CPUArchState *env, target_ulong addr, int mmu_idx);
>  #undef MEMSUFFIX
>  #endif /* (NB_MMU_MODES >= 6) */
>
> -#if (NB_MMU_MODES > 6)
> -#error "NB_MMU_MODES > 6 is not supported for now"
> -#endif /* (NB_MMU_MODES > 6) */
> +#if (NB_MMU_MODES >= 7) && defined(MMU_MODE6_SUFFIX)
> +
> +#define CPU_MMU_INDEX 6
> +#define MEMSUFFIX MMU_MODE5_SUFFIX

Should this be MMU_MODE6_SUFFIX?

> +#define DATA_SIZE 1
> +#include "exec/cpu_ldst_template.h"
> +
> +#define DATA_SIZE 2
> +#include "exec/cpu_ldst_template.h"
> +
> +#define DATA_SIZE 4
> +#include "exec/cpu_ldst_template.h"
> +
> +#define DATA_SIZE 8
> +#include "exec/cpu_ldst_template.h"
> +#undef CPU_MMU_INDEX
> +#undef MEMSUFFIX
> +#endif /* (NB_MMU_MODES >= 7) */
> +
> +#if (NB_MMU_MODES > 7)
> +/* Note that supporting NB_MMU_MODES == 9 would require
> + * changes to at least the ARM TCG backend.
> + */
> +#error "NB_MMU_MODES > 7 is not supported for now"
> +#endif /* (NB_MMU_MODES > 7) */
>
>  /* these access are slower, they must be as rare as possible */
>  #define CPU_MMU_INDEX (cpu_mmu_index(env))
> --
> 1.9.1
>

Otherwise,
Reviewed-by: Greg Bellows <greg.bellows@linaro.org>

Re: [Qemu-devel] [PATCH 01/11] cpu_ldst.h: Allow NB_MMU_MODES to be 7

[Paolo Bonzini]

On 23/01/2015 19:20, Peter Maydell wrote:
> Support guest CPUs which need 7 MMU index values.
> Add a comment about what would be required to raise the limit
> further (trivial for 8, TCG backend rework for 9 or more).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

I'll send a patch for 16 next Monday.

Paolo

Re: [Qemu-devel] [PATCH 03/11] target-arm/translate-a64: Fix wrong mmu_idx usage for LDT/STT

[Greg Bellows]

On Fri, Jan 23, 2015 at 12:20 PM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> The LDT/STT (load/store unprivileged) instruction decode was using
> the wrong MMU index value. This meant that instead of these insns
> being "always access as if user-mode regardless of current privilege"
> they were "always access as if kernel-mode regardless of current
> privilege". This went unnoticed because AArch64 Linux doesn't use
> these instructions.
>
> Cc: qemu-stable@nongnu.org
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I'm not counting this as a security issue because I'm assuming
> nobody treats TCG guests as a security boundary (certainly I
> would not recommend doing so...)
> ---
>  target-arm/translate-a64.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index 80d2359..dac2f63 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -2107,7 +2107,7 @@ static void disas_ldst_reg_imm9(DisasContext *s, uint32_t insn)
>          }
>      } else {
>          TCGv_i64 tcg_rt = cpu_reg(s, rt);
> -        int memidx = is_unpriv ? 1 : get_mem_index(s);
> +        int memidx = is_unpriv ? MMU_USER_IDX : get_mem_index(s);
>
>          if (is_store) {
>              do_gpr_st_memidx(s, tcg_rt, tcg_addr, size, memidx);
> --
> 1.9.1
>

Reviewed-by: Greg Bellows <greg.bellows@linaro.org>

Re: [Qemu-devel] [PATCH 02/11] target-arm: Make arm_current_el() return sensible values for M profile

[Greg Bellows]

On Fri, Jan 23, 2015 at 12:20 PM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> Although M profile doesn't have the same concept of exception level
> as A profile, it does have a notion of privileged versus not, which
> we currently track in the privmode TB flag. Support returning this
> information if arm_current_el() is called on an M profile core, so
> that we can identify the correct MMU index to use (and put the MMU
> index in the TB flags) without having to special-case M profile.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/cpu.h | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index cd7a9e8..3eb00f4 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1211,6 +1211,10 @@ static inline bool cptype_valid(int cptype)
>   */
>  static inline int arm_current_el(CPUARMState *env)
>  {
> +    if (arm_feature(env, ARM_FEATURE_M)) {
> +        return !((env->v7m.exception == 0) && (env->v7m.control & 1));
> +    }
> +
>      if (is_a64(env)) {
>          return extract32(env->pstate, 2, 2);
>      }
> --
> 1.9.1
>

Reviewed-by: Greg Bellows <greg.bellows@linaro.org>

Re: [Qemu-devel] [PATCH 04/11] target-arm: Define correct mmu_idx values and pass them in TB flags

[Greg Bellows]

On Fri, Jan 23, 2015 at 12:20 PM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> We currently claim that for ARM the mmu_idx should simply be the current
> exception level. However this isn't actually correct -- secure EL0 and EL1
> should have separate indexes from non-secure EL0 and EL1 since their
> VA->PA mappings may differ. We also will want an index for stage 2
> translations when we properly support EL2.
>
> Define and document all seven mmu index values that we require, and
> pass the mmu index in the TB flags rather than exception level or
> priv/user bit.
>
> This change doesn't update the get_phys_addr() code, so our page
> table walking still assumes a simplistic "user or priv?" model for
> the moment.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> This leaves some odd gaps in the TB flags usage. I will circle
> back and clean this up later (including moving the other common
> flags like the singlestep ones to the top of the flags word),
> but I didn't want to bloat this patchseries further.
> ---
>  target-arm/cpu.h           | 113 ++++++++++++++++++++++++++++++++++++---------
>  target-arm/helper.c        |   3 +-
>  target-arm/translate-a64.c |   5 +-
>  target-arm/translate.c     |   5 +-
>  target-arm/translate.h     |   3 +-
>  5 files changed, 101 insertions(+), 28 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 3eb00f4..cf7b9ab 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -98,7 +98,7 @@ typedef uint32_t ARMReadCPFunc(void *opaque, int cp_info,
>
>  struct arm_boot_info;
>
> -#define NB_MMU_MODES 4
> +#define NB_MMU_MODES 7
>
>  /* We currently assume float and double are IEEE single and double
>     precision respectively.
> @@ -1572,13 +1572,92 @@ static inline CPUARMState *cpu_init(const char *cpu_model)
>  #define cpu_signal_handler cpu_arm_signal_handler
>  #define cpu_list arm_cpu_list
>
> -/* MMU modes definitions */
> +/* ARM has the following "translation regimes" (as the ARM ARM calls them):
> + *
> + * If EL3 is 64-bit:
> + *  + NonSecure EL1 & 0 stage 1
> + *  + NonSecure EL1 & 0 stage 2
> + *  + NonSecure EL2
> + *  + Secure EL1 & EL0
> + *  + Secure EL3
> + * If EL3 is 32-bit:
> + *  + NonSecure PL1 & 0 stage 1
> + *  + NonSecure PL1 & 0 stage 2
> + *  + NonSecure PL2
> + *  + Secure PL0 & PL1
> + * (reminder: for 32 bit EL3, Secure PL1 is *EL3*, not EL1.)
> + *
> + * For QEMU, an mmu_idx is not quite the same as a translation regime because:
> + *  1. we need to split the "EL1 & 0" regimes into two mmu_idxes, because they
> + *     may differ in access permissions even if the VA->PA map is the same
> + *  2. we want to cache in our TLB the full VA->IPA->PA lookup for a stage 1+2
> + *     translation, which means that we have one mmu_idx that deals with two
> + *     concatenated translation regimes [this sort of combined s1+2 TLB is
> + *     architecturally permitted]
> + *  3. we don't need to allocate an mmu_idx to translations that we won't be
> + *     handling via the TLB. The only way to do a stage 1 translation without
> + *     the immediate stage 2 translation is via the ATS or AT system insns,
> + *     which can be slow-pathed and always do a page table walk.
> + *  4. we can also safely fold together the "32 bit EL3" and "64 bit EL3"
> + *     translation regimes, because they map reasonably well to each other
> + *     and they can't both be active at the same time.
> + * This gives us the following list of mmu_idx values:
> + *
> + * NS EL0 (aka NS PL0) stage 1+2
> + * NS EL1 (aka NS PL1) stage 1+2
> + * NS EL2 (aka NS PL2)
> + * S EL3 (aka S PL1)
> + * S EL0 (aka S PL0)
> + * S EL1 (not used if EL3 is 32 bit)
> + * NS EL0+1 stage 2
> + *
> + * (The last of these is an mmu_idx because we want to be able to use the TLB
> + * for the accesses done as part of a stage 1 page table walk, rather than
> + * having to walk the stage 2 page table over and over.)
> + *
> + * Our enumeration includes at the end some entries which are not "true"
> + * mmu_idx values in that they don't have corresponding TLBs and are only
> + * valid for doing slow path page table walks.
> + *
> + * The constant names here are patterned after the general style of the names
> + * of the AT/ATS operations.
> + * The values used are carefully arranged to make mmu_idx => EL lookup easy.
> + */
> +typedef enum ARMMMUIdx {
> +    ARMMMUIdx_S12NSE0 = 0,
> +    ARMMMUIdx_S12NSE1 = 1,
> +    ARMMMUIdx_S1E2 = 2,
> +    ARMMMUIdx_S1E3 = 3,
> +    ARMMMUIdx_S1SE0 = 4,
> +    ARMMMUIdx_S1SE1 = 5,
> +    ARMMMUIdx_S2NS = 6,
> +    /* Indexes below here don't have TLBs and are used only for AT system
> +     * instructions or for the first stage of an S12 page table walk.
> +     */
> +    ARMMMUIdx_S1NSE0 = 7,
> +    ARMMMUIdx_S1NSE1 = 8,
> +} ARMMMUIdx;
> +
>  #define MMU_MODE0_SUFFIX _user
>  #define MMU_MODE1_SUFFIX _kernel
>  #define MMU_USER_IDX 0
> +
> +/* Return the exception level we're running at if this is our mmu_idx */
> +static inline int arm_mmu_idx_to_el(ARMMMUIdx mmu_idx)
> +{
> +    assert(mmu_idx < ARMMMUIdx_S2NS);
> +    return mmu_idx & 3;
> +}
> +
> +/* Determine the current mmu_idx to use for normal loads/stores */
>  static inline int cpu_mmu_index (CPUARMState *env)
>  {
> -    return arm_current_el(env);
> +    int el = arm_current_el(env);
> +
> +    if (el < 3 && arm_is_secure_below_el3(env)) {

We bypass the secure check for EL3 but not EL2.  We should either
circumvent both EL2 & 3 or neither.

> +        return ARMMMUIdx_S1SE0 + el;
> +    }
> +    return el;
>  }
>
>  /* Return the Exception Level targeted by debug exceptions;
> @@ -1645,9 +1724,13 @@ static inline bool arm_singlestep_active(CPUARMState *env)
>
>  /* Bit usage in the TB flags field: bit 31 indicates whether we are
>   * in 32 or 64 bit mode. The meaning of the other bits depends on that.
> + * We put flags which are shared between 32 and 64 bit mode at the top
> + * of the word, and flags which apply to only one mode at the bottom.
>   */
>  #define ARM_TBFLAG_AARCH64_STATE_SHIFT 31
>  #define ARM_TBFLAG_AARCH64_STATE_MASK  (1U << ARM_TBFLAG_AARCH64_STATE_SHIFT)
> +#define ARM_TBFLAG_MMUIDX_SHIFT 28
> +#define ARM_TBFLAG_MMUIDX_MASK (0x7 << ARM_TBFLAG_MMUIDX_SHIFT)
>
>  /* Bit usage when in AArch32 state: */
>  #define ARM_TBFLAG_THUMB_SHIFT      0
> @@ -1656,8 +1739,6 @@ static inline bool arm_singlestep_active(CPUARMState *env)
>  #define ARM_TBFLAG_VECLEN_MASK      (0x7 << ARM_TBFLAG_VECLEN_SHIFT)
>  #define ARM_TBFLAG_VECSTRIDE_SHIFT  4
>  #define ARM_TBFLAG_VECSTRIDE_MASK   (0x3 << ARM_TBFLAG_VECSTRIDE_SHIFT)
> -#define ARM_TBFLAG_PRIV_SHIFT       6
> -#define ARM_TBFLAG_PRIV_MASK        (1 << ARM_TBFLAG_PRIV_SHIFT)
>  #define ARM_TBFLAG_VFPEN_SHIFT      7
>  #define ARM_TBFLAG_VFPEN_MASK       (1 << ARM_TBFLAG_VFPEN_SHIFT)
>  #define ARM_TBFLAG_CONDEXEC_SHIFT   8
> @@ -1683,8 +1764,6 @@ static inline bool arm_singlestep_active(CPUARMState *env)
>  #define ARM_TBFLAG_NS_MASK          (1 << ARM_TBFLAG_NS_SHIFT)
>
>  /* Bit usage when in AArch64 state */
> -#define ARM_TBFLAG_AA64_EL_SHIFT    0
> -#define ARM_TBFLAG_AA64_EL_MASK     (0x3 << ARM_TBFLAG_AA64_EL_SHIFT)
>  #define ARM_TBFLAG_AA64_FPEN_SHIFT  2
>  #define ARM_TBFLAG_AA64_FPEN_MASK   (1 << ARM_TBFLAG_AA64_FPEN_SHIFT)
>  #define ARM_TBFLAG_AA64_SS_ACTIVE_SHIFT 3
> @@ -1695,14 +1774,14 @@ static inline bool arm_singlestep_active(CPUARMState *env)
>  /* some convenience accessor macros */
>  #define ARM_TBFLAG_AARCH64_STATE(F) \
>      (((F) & ARM_TBFLAG_AARCH64_STATE_MASK) >> ARM_TBFLAG_AARCH64_STATE_SHIFT)
> +#define ARM_TBFLAG_MMUIDX(F) \
> +    (((F) & ARM_TBFLAG_MMUIDX_MASK) >> ARM_TBFLAG_MMUIDX_SHIFT)
>  #define ARM_TBFLAG_THUMB(F) \
>      (((F) & ARM_TBFLAG_THUMB_MASK) >> ARM_TBFLAG_THUMB_SHIFT)
>  #define ARM_TBFLAG_VECLEN(F) \
>      (((F) & ARM_TBFLAG_VECLEN_MASK) >> ARM_TBFLAG_VECLEN_SHIFT)
>  #define ARM_TBFLAG_VECSTRIDE(F) \
>      (((F) & ARM_TBFLAG_VECSTRIDE_MASK) >> ARM_TBFLAG_VECSTRIDE_SHIFT)
> -#define ARM_TBFLAG_PRIV(F) \
> -    (((F) & ARM_TBFLAG_PRIV_MASK) >> ARM_TBFLAG_PRIV_SHIFT)
>  #define ARM_TBFLAG_VFPEN(F) \
>      (((F) & ARM_TBFLAG_VFPEN_MASK) >> ARM_TBFLAG_VFPEN_SHIFT)
>  #define ARM_TBFLAG_CONDEXEC(F) \
> @@ -1717,8 +1796,6 @@ static inline bool arm_singlestep_active(CPUARMState *env)
>      (((F) & ARM_TBFLAG_PSTATE_SS_MASK) >> ARM_TBFLAG_PSTATE_SS_SHIFT)
>  #define ARM_TBFLAG_XSCALE_CPAR(F) \
>      (((F) & ARM_TBFLAG_XSCALE_CPAR_MASK) >> ARM_TBFLAG_XSCALE_CPAR_SHIFT)
> -#define ARM_TBFLAG_AA64_EL(F) \
> -    (((F) & ARM_TBFLAG_AA64_EL_MASK) >> ARM_TBFLAG_AA64_EL_SHIFT)
>  #define ARM_TBFLAG_AA64_FPEN(F) \
>      (((F) & ARM_TBFLAG_AA64_FPEN_MASK) >> ARM_TBFLAG_AA64_FPEN_SHIFT)
>  #define ARM_TBFLAG_AA64_SS_ACTIVE(F) \
> @@ -1742,8 +1819,7 @@ static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
>
>      if (is_a64(env)) {
>          *pc = env->pc;
> -        *flags = ARM_TBFLAG_AARCH64_STATE_MASK
> -            | (arm_current_el(env) << ARM_TBFLAG_AA64_EL_SHIFT);
> +        *flags = ARM_TBFLAG_AARCH64_STATE_MASK;
>          if (fpen == 3 || (fpen == 1 && arm_current_el(env) != 0)) {
>              *flags |= ARM_TBFLAG_AA64_FPEN_MASK;
>          }
> @@ -1761,21 +1837,12 @@ static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
>              }
>          }
>      } else {
> -        int privmode;
>          *pc = env->regs[15];
>          *flags = (env->thumb << ARM_TBFLAG_THUMB_SHIFT)
>              | (env->vfp.vec_len << ARM_TBFLAG_VECLEN_SHIFT)
>              | (env->vfp.vec_stride << ARM_TBFLAG_VECSTRIDE_SHIFT)
>              | (env->condexec_bits << ARM_TBFLAG_CONDEXEC_SHIFT)
>              | (env->bswap_code << ARM_TBFLAG_BSWAP_CODE_SHIFT);
> -        if (arm_feature(env, ARM_FEATURE_M)) {
> -            privmode = !((env->v7m.exception == 0) && (env->v7m.control & 1));
> -        } else {
> -            privmode = (env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_USR;
> -        }
> -        if (privmode) {
> -            *flags |= ARM_TBFLAG_PRIV_MASK;
> -        }
>          if (!(access_secure_reg(env))) {
>              *flags |= ARM_TBFLAG_NS_MASK;
>          }
> @@ -1803,6 +1870,8 @@ static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
>                     << ARM_TBFLAG_XSCALE_CPAR_SHIFT);
>      }
>
> +    *flags |= (cpu_mmu_index(env) << ARM_TBFLAG_MMUIDX_SHIFT);
> +
>      *cs_base = 0;
>  }
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 1a5e067..06478d8 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -5119,7 +5119,8 @@ int arm_cpu_handle_mmu_fault(CPUState *cs, vaddr address,
>      uint32_t syn;
>      bool same_el = (arm_current_el(env) != 0);
>
> -    is_user = mmu_idx == MMU_USER_IDX;
> +    /* TODO: pass the translation regime to get_phys_addr */
> +    is_user = (arm_mmu_idx_to_el(mmu_idx) == 0);
>      ret = get_phys_addr(env, address, access_type, is_user, &phys_addr, &prot,
>                          &page_size);
>      if (ret == 0) {
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index dac2f63..96f14ff 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -10922,14 +10922,15 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
>      dc->bswap_code = 0;
>      dc->condexec_mask = 0;
>      dc->condexec_cond = 0;
> +    dc->mmu_idx = ARM_TBFLAG_MMUIDX(tb->flags);
> +    dc->current_el = arm_mmu_idx_to_el(dc->mmu_idx);
>  #if !defined(CONFIG_USER_ONLY)
> -    dc->user = (ARM_TBFLAG_AA64_EL(tb->flags) == 0);
> +    dc->user = (dc->current_el == 0);
>  #endif
>      dc->cpacr_fpen = ARM_TBFLAG_AA64_FPEN(tb->flags);
>      dc->vec_len = 0;
>      dc->vec_stride = 0;
>      dc->cp_regs = cpu->cp_regs;
> -    dc->current_el = arm_current_el(env);
>      dc->features = env->features;
>
>      /* Single step state. The code-generation logic here is:
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index bdfcdf1..7163649 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -11032,8 +11032,10 @@ static inline void gen_intermediate_code_internal(ARMCPU *cpu,
>      dc->bswap_code = ARM_TBFLAG_BSWAP_CODE(tb->flags);
>      dc->condexec_mask = (ARM_TBFLAG_CONDEXEC(tb->flags) & 0xf) << 1;
>      dc->condexec_cond = ARM_TBFLAG_CONDEXEC(tb->flags) >> 4;
> +    dc->mmu_idx = ARM_TBFLAG_MMUIDX(tb->flags);
> +    dc->current_el = arm_mmu_idx_to_el(dc->mmu_idx);
>  #if !defined(CONFIG_USER_ONLY)
> -    dc->user = (ARM_TBFLAG_PRIV(tb->flags) == 0);
> +    dc->user = (dc->current_el == 0);
>  #endif
>      dc->ns = ARM_TBFLAG_NS(tb->flags);
>      dc->cpacr_fpen = ARM_TBFLAG_CPACR_FPEN(tb->flags);
> @@ -11042,7 +11044,6 @@ static inline void gen_intermediate_code_internal(ARMCPU *cpu,
>      dc->vec_stride = ARM_TBFLAG_VECSTRIDE(tb->flags);
>      dc->c15_cpar = ARM_TBFLAG_XSCALE_CPAR(tb->flags);
>      dc->cp_regs = cpu->cp_regs;
> -    dc->current_el = arm_current_el(env);
>      dc->features = env->features;
>
>      /* Single step state. The code-generation logic here is:
> diff --git a/target-arm/translate.h b/target-arm/translate.h
> index f6ee789..a1eb5b5 100644
> --- a/target-arm/translate.h
> +++ b/target-arm/translate.h
> @@ -20,6 +20,7 @@ typedef struct DisasContext {
>  #if !defined(CONFIG_USER_ONLY)
>      int user;
>  #endif
> +    ARMMMUIdx mmu_idx; /* MMU index to use for normal loads/stores */
>      bool ns;        /* Use non-secure CPREG bank on access */
>      bool cpacr_fpen; /* FP enabled via CPACR.FPEN */
>      bool vfp_enabled; /* FP enabled via FPSCR.EN */
> @@ -69,7 +70,7 @@ static inline int arm_dc_feature(DisasContext *dc, int feature)
>
>  static inline int get_mem_index(DisasContext *s)
>  {
> -    return s->current_el;
> +    return s->mmu_idx;
>  }
>
>  /* target-specific extra values for is_jmp */
> --
> 1.9.1
>

Otherwise,

Reviewed-by: Greg Bellows <greg.bellows@linaro.org>

Re: [Qemu-devel] [PATCH 01/11] cpu_ldst.h: Allow NB_MMU_MODES to be 7

[Peter Maydell]

On 23 January 2015 at 20:16, Greg Bellows <greg.bellows@linaro.org> wrote:
> On Fri, Jan 23, 2015 at 12:20 PM, Peter Maydell
> <peter.maydell@linaro.org> wrote:
>> Support guest CPUs which need 7 MMU index values.
>> Add a comment about what would be required to raise the limit
>> further (trivial for 8, TCG backend rework for 9 or more).
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  include/exec/cpu_ldst.h | 28 +++++++++++++++++++++++++---
>>  1 file changed, 25 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
>> index 0e825ea..fa5ea63 100644
>> --- a/include/exec/cpu_ldst.h
>> +++ b/include/exec/cpu_ldst.h
>> @@ -244,9 +244,31 @@ uint64_t helper_ldq_cmmu(CPUArchState *env, target_ulong addr, int mmu_idx);
>>  #undef MEMSUFFIX
>>  #endif /* (NB_MMU_MODES >= 6) */
>>
>> -#if (NB_MMU_MODES > 6)
>> -#error "NB_MMU_MODES > 6 is not supported for now"
>> -#endif /* (NB_MMU_MODES > 6) */
>> +#if (NB_MMU_MODES >= 7) && defined(MMU_MODE6_SUFFIX)
>> +
>> +#define CPU_MMU_INDEX 6
>> +#define MEMSUFFIX MMU_MODE5_SUFFIX
>
> Should this be MMU_MODE6_SUFFIX?

Yes, good catch (we don't notice since we don't define
the MMU_MODE*_SUFFIX anyway).

-- PMM

Re: [Qemu-devel] [PATCH 04/11] target-arm: Define correct mmu_idx values and pass them in TB flags

[Peter Maydell]

On 23 January 2015 at 21:44, Greg Bellows <greg.bellows@linaro.org> wrote:
> On Fri, Jan 23, 2015 at 12:20 PM, Peter Maydell
> <peter.maydell@linaro.org> wrote:
>> +typedef enum ARMMMUIdx {
>> +    ARMMMUIdx_S12NSE0 = 0,
>> +    ARMMMUIdx_S12NSE1 = 1,
>> +    ARMMMUIdx_S1E2 = 2,
>> +    ARMMMUIdx_S1E3 = 3,
>> +    ARMMMUIdx_S1SE0 = 4,
>> +    ARMMMUIdx_S1SE1 = 5,
>> +    ARMMMUIdx_S2NS = 6,
>> +    /* Indexes below here don't have TLBs and are used only for AT system
>> +     * instructions or for the first stage of an S12 page table walk.
>> +     */
>> +    ARMMMUIdx_S1NSE0 = 7,
>> +    ARMMMUIdx_S1NSE1 = 8,
>> +} ARMMMUIdx;
>> +
>>  #define MMU_MODE0_SUFFIX _user
>>  #define MMU_MODE1_SUFFIX _kernel
>>  #define MMU_USER_IDX 0
>> +
>> +/* Return the exception level we're running at if this is our mmu_idx */
>> +static inline int arm_mmu_idx_to_el(ARMMMUIdx mmu_idx)
>> +{
>> +    assert(mmu_idx < ARMMMUIdx_S2NS);
>> +    return mmu_idx & 3;
>> +}
>> +
>> +/* Determine the current mmu_idx to use for normal loads/stores */
>>  static inline int cpu_mmu_index (CPUARMState *env)
>>  {
>> -    return arm_current_el(env);
>> +    int el = arm_current_el(env);
>> +
>> +    if (el < 3 && arm_is_secure_below_el3(env)) {
>
> We bypass the secure check for EL3 but not EL2.  We should either
> circumvent both EL2 & 3 or neither.

Not sure what you mean here. The point of this code is to return
a suitable MMU idx for the current CPU state. If we are in
EL2 or EL3 then just "el" is correct (being ARMMMUIdx_S1E2 and
ARMMMUIdx_S1E3). We can't be in this condition with el == 2
because if we're in EL2 then the NS bit must be set (there's
no such thing as Secure EL2) and so arm_is_secure_below_el3()
will have returned false. So we know here that el is 0 or 1,
and this addition below:

>> +        return ARMMMUIdx_S1SE0 + el;

means we end up with either _S1SE0 or _S1SE1, as required.

(the condition could equally well be written
   if (el < 2 && arm_is_secure_below_el3(env))
 as the two are true in exactly the same set of cases. < 3 seemed
 marginally better to me, since it's expressing "if we're secure
and not in EL3" which is the set of cases where we need to
change the mmu_idx from the 0/1/2/3 values.)

>> +    }
>> +    return el;

Anything else (_S12NSE0, _S12NSE1, _S1E2, _S1E3) is this return.

>>  }

thanks
-- PMM

Re: [Qemu-devel] [PATCH 04/11] target-arm: Define correct mmu_idx values and pass them in TB flags

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 2738 bytes --]

On Jan 23, 2015 7:12 PM, "Peter Maydell" <peter.maydell@linaro.org> wrote:
>
> On 23 January 2015 at 21:44, Greg Bellows <greg.bellows@linaro.org> wrote:
> > On Fri, Jan 23, 2015 at 12:20 PM, Peter Maydell
> > <peter.maydell@linaro.org> wrote:
> >> +typedef enum ARMMMUIdx {
> >> +    ARMMMUIdx_S12NSE0 = 0,
> >> +    ARMMMUIdx_S12NSE1 = 1,
> >> +    ARMMMUIdx_S1E2 = 2,
> >> +    ARMMMUIdx_S1E3 = 3,
> >> +    ARMMMUIdx_S1SE0 = 4,
> >> +    ARMMMUIdx_S1SE1 = 5,
> >> +    ARMMMUIdx_S2NS = 6,
> >> +    /* Indexes below here don't have TLBs and are used only for AT
system
> >> +     * instructions or for the first stage of an S12 page table walk.
> >> +     */
> >> +    ARMMMUIdx_S1NSE0 = 7,
> >> +    ARMMMUIdx_S1NSE1 = 8,
> >> +} ARMMMUIdx;
> >> +
> >>  #define MMU_MODE0_SUFFIX _user
> >>  #define MMU_MODE1_SUFFIX _kernel
> >>  #define MMU_USER_IDX 0
> >> +
> >> +/* Return the exception level we're running at if this is our mmu_idx
*/
> >> +static inline int arm_mmu_idx_to_el(ARMMMUIdx mmu_idx)
> >> +{
> >> +    assert(mmu_idx < ARMMMUIdx_S2NS);
> >> +    return mmu_idx & 3;
> >> +}
> >> +
> >> +/* Determine the current mmu_idx to use for normal loads/stores */
> >>  static inline int cpu_mmu_index (CPUARMState *env)
> >>  {
> >> -    return arm_current_el(env);
> >> +    int el = arm_current_el(env);
> >> +
> >> +    if (el < 3 && arm_is_secure_below_el3(env)) {
> >
> > We bypass the secure check for EL3 but not EL2.  We should either
> > circumvent both EL2 & 3 or neither.
>
> Not sure what you mean here. The point of this code is to return
> a suitable MMU idx for the current CPU state. If we are in
> EL2 or EL3 then just "el" is correct (being ARMMMUIdx_S1E2 and
> ARMMMUIdx_S1E3). We can't be in this condition with el == 2

I understand what the code is doing, my point is that you rely on
arm_is_secure_below_el3 to deal with EL2 when you could have just as easily
checked for el<2.  Not a big deal though.

> because if we're in EL2 then the NS bit must be set (there's
> no such thing as Secure EL2) and so arm_is_secure_below_el3()
> will have returned false. So we know here that el is 0 or 1,
> and this addition below:
>
> >> +        return ARMMMUIdx_S1SE0 + el;
>
> means we end up with either _S1SE0 or _S1SE1, as required.
>
> (the condition could equally well be written
>    if (el < 2 && arm_is_secure_below_el3(env))
>  as the two are true in exactly the same set of cases. < 3 seemed
>  marginally better to me, since it's expressing "if we're secure
> and not in EL3" which is the set of cases where we need to
> change the mmu_idx from the 0/1/2/3 values.)
>
> >> +    }
> >> +    return el;
>
> Anything else (_S12NSE0, _S12NSE1, _S1E2, _S1E3) is this return.
>
> >>  }
>
> thanks
> -- PMM

[-- Attachment #2: Type: text/html, Size: 3896 bytes --]

Re: [Qemu-devel] [PATCH 04/11] target-arm: Define correct mmu_idx values and pass them in TB flags

[Peter Maydell]

On 24 January 2015 at 16:36, Greg Bellows <greg.bellows@linaro.org> wrote:
> I understand what the code is doing, my point is that you rely on
> arm_is_secure_below_el3 to deal with EL2 when you could have just as easily
> checked for el<2.  Not a big deal though.

Well, I rely on the architecture to tell me that it isn't
possible for arm_is_secure_below_el3 to be true if we're
in EL2. Otherwise there'd be a bug in that function...

-- PMM

Re: [Qemu-devel] [PATCH 04/11] target-arm: Define correct mmu_idx values and pass them in TB flags

[Peter Maydell]

On 24 January 2015 at 19:31, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 24 January 2015 at 16:36, Greg Bellows <greg.bellows@linaro.org> wrote:
>> I understand what the code is doing, my point is that you rely on
>> arm_is_secure_below_el3 to deal with EL2 when you could have just as easily
>> checked for el<2.  Not a big deal though.
>
> Well, I rely on the architecture to tell me that it isn't
> possible for arm_is_secure_below_el3 to be true if we're
> in EL2. Otherwise there'd be a bug in that function...

On reflection, I see your point, so have changed to 'el < 2 && ...'
for v2.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 05/11] target-arm: Use correct mmu_idx for unprivileged loads and stores

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 4263 bytes --]

On Fri, Jan 23, 2015 at 12:20 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> The MMU index to use for unprivileged loads and stores is more
> complicated than we currently implement:
>  * for A64, it should be "if at EL1, access as if EL0; otherwise
>    access at current EL"
>  * for A32/T32, it should be "if EL2, UNPREDICTABLE; otherwise
>    access as if at EL0".
>
>
​The wording between the specs appears to be almost identical, curious why
the handling is different?​



> In both cases, if we want to make the access for Secure EL0
> this is not the same mmu_idx as for Non-Secure EL0.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/translate-a64.c | 19 ++++++++++++++++++-
>  target-arm/translate.c     | 26 ++++++++++++++++++++++++--
>  2 files changed, 42 insertions(+), 3 deletions(-)
>
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index 96f14ff..acf4b16 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -123,6 +123,23 @@ void a64_translate_init(void)
>  #endif
>  }
>
> +static inline ARMMMUIdx get_a64_user_mem_index(DisasContext *s)
> +{
> +    /* Return the mmu_idx to use for A64 "unprivileged load/store" insns:
> +     *  if EL1, access as if EL0; otherwise access at current EL
> +     */
> +    switch (s->mmu_idx) {
> +    case ARMMMUIdx_S12NSE1:
> +        return ARMMMUIdx_S12NSE0;
> +    case ARMMMUIdx_S1SE1:
> +        return ARMMMUIdx_S1SE0;
> +    case ARMMMUIdx_S2NS:
> +        g_assert_not_reached();
> +    default:
> +        return s->mmu_idx;
> +    }
> +}
> +
>  void aarch64_cpu_dump_state(CPUState *cs, FILE *f,
>                              fprintf_function cpu_fprintf, int flags)
>  {
> @@ -2107,7 +2124,7 @@ static void disas_ldst_reg_imm9(DisasContext *s,
> uint32_t insn)
>          }
>      } else {
>          TCGv_i64 tcg_rt = cpu_reg(s, rt);
> -        int memidx = is_unpriv ? MMU_USER_IDX : get_mem_index(s);
> +        int memidx = is_unpriv ? get_a64_user_mem_index(s) :
> get_mem_index(s);
>
>          if (is_store) {
>              do_gpr_st_memidx(s, tcg_rt, tcg_addr, size, memidx);
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 7163649..715f65d 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -113,6 +113,28 @@ void arm_translate_init(void)
>      a64_translate_init();
>  }
>
> +static inline ARMMMUIdx get_a32_user_mem_index(DisasContext *s)
> +{
> +    /* Return the mmu_idx to use for A32/T32 "unprivileged load/store"
> +     * insns:
> +     *  if PL2, UNPREDICTABLE (we choose to implement as if PL0)
> +     *  otherwise, access as if at PL0.
> +     */
> +    switch (s->mmu_idx) {
> +    case ARMMMUIdx_S1E2:        /* this one is UNPREDICTABLE */
> +    case ARMMMUIdx_S12NSE0:
> +    case ARMMMUIdx_S12NSE1:
> +        return ARMMMUIdx_S12NSE0;
> +    case ARMMMUIdx_S1E3:
> +    case ARMMMUIdx_S1SE0:
> +    case ARMMMUIdx_S1SE1:
> +        return ARMMMUIdx_S1SE0;
> +    case ARMMMUIdx_S2NS:
> +    default:
> +        g_assert_not_reached();
> +    }
> +}
> +
>  static inline TCGv_i32 load_cpu_offset(int offset)
>  {
>      TCGv_i32 tmp = tcg_temp_new_i32();
> @@ -8793,7 +8815,7 @@ static void disas_arm_insn(DisasContext *s, unsigned
> int insn)
>              tmp2 = load_reg(s, rn);
>              if ((insn & 0x01200000) == 0x00200000) {
>                  /* ldrt/strt */
> -                i = MMU_USER_IDX;
> +                i = get_a32_user_mem_index(s);
>              } else {
>                  i = get_mem_index(s);
>              }
> @@ -10173,7 +10195,7 @@ static int disas_thumb2_insn(CPUARMState *env,
> DisasContext *s, uint16_t insn_hw
>                      break;
>                  case 0xe: /* User privilege.  */
>                      tcg_gen_addi_i32(addr, addr, imm);
> -                    memidx = MMU_USER_IDX;
> +                    memidx = get_a32_user_mem_index(s);
>                      break;
>                  case 0x9: /* Post-decrement.  */
>                      imm = -imm;
> --
> 1.9.1
>
> ​Otherwise,

Reviewed-by: Greg Bellows <greg.bellows@linaro.org>​

[-- Attachment #2: Type: text/html, Size: 5839 bytes --]

Re: [Qemu-devel] [PATCH 05/11] target-arm: Use correct mmu_idx for unprivileged loads and stores

[Peter Maydell]

On 26 January 2015 at 14:40, Greg Bellows <greg.bellows@linaro.org> wrote:
> On Fri, Jan 23, 2015 at 12:20 PM, Peter Maydell <peter.maydell@linaro.org>
> wrote:
>>
>> The MMU index to use for unprivileged loads and stores is more
>> complicated than we currently implement:
>>  * for A64, it should be "if at EL1, access as if EL0; otherwise
>>    access at current EL"
>>  * for A32/T32, it should be "if EL2, UNPREDICTABLE; otherwise
>>    access as if at EL0".
>>
>
> The wording between the specs appears to be almost identical, curious why
> the handling is different?

Because that's what the ARM ARM specifies. Compare C3.2.5 (A64 LDT &c)
with F7.1.95 (A32/T32 LDRT).

-- PMM

Re: [Qemu-devel] [PATCH 05/11] target-arm: Use correct mmu_idx for unprivileged loads and stores

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 1293 bytes --]

On Mon, Jan 26, 2015 at 8:56 AM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 26 January 2015 at 14:40, Greg Bellows <greg.bellows@linaro.org> wrote:
> > On Fri, Jan 23, 2015 at 12:20 PM, Peter Maydell <
> peter.maydell@linaro.org>
> > wrote:
> >>
> >> The MMU index to use for unprivileged loads and stores is more
> >> complicated than we currently implement:
> >>  * for A64, it should be "if at EL1, access as if EL0; otherwise
> >>    access at current EL"
> >>  * for A32/T32, it should be "if EL2, UNPREDICTABLE; otherwise
> >>    access as if at EL0".
> >>
> >
> > The wording between the specs appears to be almost identical, curious why
> > the handling is different?
>
> Because that's what the ARM ARM specifies. Compare C3.2.5 (A64 LDT &c)
> with F7.1.95 (A32/T32 LDRT).
>
​​

​I had been comparing the wording of ARMv8 - F1.6.3 and ​ARMv7 - A4.6.3.
After comparing the LDRT instructions between A64 (C6.6.97) and A32
(F7.1.95), I am still missing the distinction that warrants the following
different behavior:

- EL2 is unpredictable in both A64 and A32, but in one case we treat it as
such and the other we demote it to NS/EL0 to allow it.
- EL3 is demoted to S/EL0 in one case but remains EL3 in the other.


>
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 2569 bytes --]

Re: [Qemu-devel] [PATCH 06/11] target-arm: Don't define any MMU_MODE*_SUFFIXes

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 972 bytes --]

On Fri, Jan 23, 2015 at 12:20 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> target-arm doesn't use any of the MMU-mode specific cpu ldst
> accessor functions. Suppress their generation by not defining
> any of the MMU_MODE*_SUFFIX macros. ("user" and "kernel" are
> too simplistic as descriptions of indexes 0 and 1 anyway.)
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/cpu.h | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index cf7b9ab..d18df8f 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1638,8 +1638,6 @@ typedef enum ARMMMUIdx {
>      ARMMMUIdx_S1NSE1 = 8,
>  } ARMMMUIdx;
>
> -#define MMU_MODE0_SUFFIX _user
> -#define MMU_MODE1_SUFFIX _kernel
>  #define MMU_USER_IDX 0
>
>  /* Return the exception level we're running at if this is our mmu_idx */
> --
> 1.9.1
>
>
​Reviewed-by: Greg Bellows <greg.bellows@linaro.org>​

[-- Attachment #2: Type: text/html, Size: 1717 bytes --]

Re: [Qemu-devel] [PATCH 07/11] target-arm: Split AArch64 cases out of ats_write()

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 3859 bytes --]

On Fri, Jan 23, 2015 at 12:20 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> Instead of simply reusing ats_write() as the handler for both AArch32
> and AArch64 address translation operations, use a different function
> for each with the common code in a third function. This is necessary
> because the semantics for selecting the right translation regime are
> different; we are only getting away with sharing currently because
> we don't support EL2 and only support EL3 in AArch32.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/helper.c | 33 ++++++++++++++++++++++++++-------
>  1 file changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 06478d8..04bc0a1 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -1435,13 +1435,13 @@ static CPAccessResult ats_access(CPUARMState *env,
> const ARMCPRegInfo *ri)
>      return CP_ACCESS_OK;
>  }
>
> -static void ats_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t
> value)
> +static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
> +                             int access_type, int is_user)
>  {
>      hwaddr phys_addr;
>      target_ulong page_size;
>      int prot;
> -    int ret, is_user = ri->opc2 & 2;
> -    int access_type = ri->opc2 & 1;
> +    int ret;
>      uint64_t par64;
>
>      ret = get_phys_addr(env, value, access_type, is_user,
> @@ -1481,9 +1481,28 @@ static void ats_write(CPUARMState *env, const
> ARMCPRegInfo *ri, uint64_t value)
>                      ((ret & 0xf) << 1) | 1;
>          }
>      }
> +    return par64;
> +}
> +
> +static void ats_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t
> value)
> +{
> +    int is_user = ri->opc2 & 2;
> +    int access_type = ri->opc2 & 1;
> +    uint64_t par64;
> +
> +    par64 = do_ats_write(env, value, access_type, is_user);
>
>      A32_BANKED_CURRENT_REG_SET(env, par, par64);
>  }
> +
> +static void ats_write64(CPUARMState *env, const ARMCPRegInfo *ri,
> +                        uint64_t value)
> +{
> +    int is_user = ri->opc2 & 2;
> +    int access_type = ri->opc2 & 1;
> +
> +    env->cp15.par_el[1] = do_ats_write(env, value, access_type, is_user);
> +}
>  #endif
>
>  static const ARMCPRegInfo vapa_cp_reginfo[] = {
> @@ -2257,16 +2276,16 @@ static const ARMCPRegInfo v8_cp_reginfo[] = {
>      /* 64 bit address translation operations */
>      { .name = "AT_S1E1R", .state = ARM_CP_STATE_AA64,
>        .opc0 = 1, .opc1 = 0, .crn = 7, .crm = 8, .opc2 = 0,
> -      .access = PL1_W, .type = ARM_CP_NO_MIGRATE, .writefn = ats_write },
> +      .access = PL1_W, .type = ARM_CP_NO_MIGRATE, .writefn = ats_write64
> },
>      { .name = "AT_S1E1W", .state = ARM_CP_STATE_AA64,
>        .opc0 = 1, .opc1 = 0, .crn = 7, .crm = 8, .opc2 = 1,
> -      .access = PL1_W, .type = ARM_CP_NO_MIGRATE, .writefn = ats_write },
> +      .access = PL1_W, .type = ARM_CP_NO_MIGRATE, .writefn = ats_write64
> },
>      { .name = "AT_S1E0R", .state = ARM_CP_STATE_AA64,
>        .opc0 = 1, .opc1 = 0, .crn = 7, .crm = 8, .opc2 = 2,
> -      .access = PL1_W, .type = ARM_CP_NO_MIGRATE, .writefn = ats_write },
> +      .access = PL1_W, .type = ARM_CP_NO_MIGRATE, .writefn = ats_write64
> },
>      { .name = "AT_S1E0W", .state = ARM_CP_STATE_AA64,
>        .opc0 = 1, .opc1 = 0, .crn = 7, .crm = 8, .opc2 = 3,
> -      .access = PL1_W, .type = ARM_CP_NO_MIGRATE, .writefn = ats_write },
> +      .access = PL1_W, .type = ARM_CP_NO_MIGRATE, .writefn = ats_write64
> },
>  #endif
>      /* TLB invalidate last level of translation table walk */
>      { .name = "TLBIMVALIS", .cp = 15, .opc1 = 0, .crn = 8, .crm = 3,
> .opc2 = 5,
> --
> 1.9.1
>
>
​Reviewed-by: Greg Bellows <greg.bellows@linaro.org>​

[-- Attachment #2: Type: text/html, Size: 4930 bytes --]

Re: [Qemu-devel] [PATCH 05/11] target-arm: Use correct mmu_idx for unprivileged loads and stores

[Peter Maydell]

On 26 January 2015 at 19:34, Greg Bellows <greg.bellows@linaro.org> wrote:
> On Mon, Jan 26, 2015 at 8:56 AM, Peter Maydell <peter.maydell@linaro.org>
> wrote:
>> Because that's what the ARM ARM specifies. Compare C3.2.5 (A64 LDT &c)
>> with F7.1.95 (A32/T32 LDRT).
>
>
> I had been comparing the wording of ARMv8 - F1.6.3 and ARMv7 - A4.6.3.
> After comparing the LDRT instructions between A64 (C6.6.97) and A32
> (F7.1.95), I am still missing the distinction that warrants the following
> different behavior:
>
> - EL2 is unpredictable in both A64 and A32, but in one case we treat it as
> such and the other we demote it to NS/EL0 to allow it.

No, it's not unpredictable in A64. It behaves as if a normal
(EL2) access [C3.2.5 "if the PE is executing in any other Exception
level, then a normal memory access for that level is performed"].
It is only unpredictable at EL2 in A32/T32 [F7.1.95 "UNPREDICTABLE
in Hyp mode"; in the v7 ARM ARM, A4.6.3 "UNPREDICTABLE if executed
at PL2"]. You'll see that the pseudocode for A32/T32 LDRT has
an UNPREDICTABLE check for PL2, but the pseudocode for A64
LDTR does not have any equivalent check.

> - EL3 is demoted to S/EL0 in one case but remains EL3 in the
> other.

Remains EL3 for AArch64 (by the same C3.2.5 requirement quoted above);
must act as if EL0 for AArch32 (F7.1.95 "as if the PE were running
in User mode").

This is because an EL3 A32/T32 insn is PL1, and AArch32 accesses
from PL1 must behave as if from PL0 (otherwise pre-v8 software
would break). An EL3 A64 insn, on the other hand, is definitely
not EL1 and there's no back-compatibility behaviour required.

Both these differences are required by the spec.

-- PMM

Re: [Qemu-devel] [PATCH 08/11] target-arm: Pass mmu_idx to get_phys_addr()

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 8238 bytes --]

On Fri, Jan 23, 2015 at 12:20 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> Make all the callers of get_phys_addr() pass it the correct
> mmu_idx rather than just a simple "is_user" flag. This includes
> properly decoding the AT/ATS system instructions; we include the
> logic for handling all the opc1/opc2 cases because we'll need
> them later for supporting EL2/EL3, even if we don't have the
> regdef stanzas yet.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/helper.c | 110
> +++++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 96 insertions(+), 14 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 04bc0a1..0ae04eb 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -13,7 +13,7 @@
>
>  #ifndef CONFIG_USER_ONLY
>  static inline int get_phys_addr(CPUARMState *env, target_ulong address,
> -                                int access_type, int is_user,
> +                                int access_type, ARMMMUIdx mmu_idx,
>                                  hwaddr *phys_ptr, int *prot,
>                                  target_ulong *page_size);
>
> @@ -1436,7 +1436,7 @@ static CPAccessResult ats_access(CPUARMState *env,
> const ARMCPRegInfo *ri)
>  }
>
>  static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
> -                             int access_type, int is_user)
> +                             int access_type, ARMMMUIdx mmu_idx)
>  {
>      hwaddr phys_addr;
>      target_ulong page_size;
> @@ -1444,7 +1444,7 @@ static uint64_t do_ats_write(CPUARMState *env,
> uint64_t value,
>      int ret;
>      uint64_t par64;
>
> -    ret = get_phys_addr(env, value, access_type, is_user,
> +    ret = get_phys_addr(env, value, access_type, mmu_idx,
>                          &phys_addr, &prot, &page_size);
>      if (extended_addresses_enabled(env)) {
>          /* ret is a DFSR/IFSR value for the long descriptor
> @@ -1486,11 +1486,58 @@ static uint64_t do_ats_write(CPUARMState *env,
> uint64_t value,
>
>  static void ats_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t
> value)
>  {
> -    int is_user = ri->opc2 & 2;
>      int access_type = ri->opc2 & 1;
>      uint64_t par64;
> +    ARMMMUIdx mmu_idx;
> +    int el = arm_current_el(env);
> +    bool secure = arm_is_secure_below_el3(env);
>
> -    par64 = do_ats_write(env, value, access_type, is_user);
> +    switch (ri->opc2 & 6) {
> +    case 0:
> +        /* stage 1 current state PL1 */
> +        switch (el) {
> +        case 3:
> +            mmu_idx = ARMMMUIdx_S1E3;
> +            break;
> +        case 2:
> +            mmu_idx = ARMMMUIdx_S1NSE1;
> +            break;
> +        case 1:
> +            mmu_idx = secure ? ARMMMUIdx_S1SE1 : ARMMMUIdx_S1NSE1;
> +            break;
> +        default:
> +            g_assert_not_reached();
> +        }
> +        break;
> +    case 2:
> +        /* stage 1 current state PL0 */
> +        switch (el) {
> +        case 3:
> +            mmu_idx = ARMMMUIdx_S1SE0;
> +            break;
> +        case 2:
> +            mmu_idx = ARMMMUIdx_S1NSE0;
> +            break;
> +        case 1:
> +            mmu_idx = secure ? ARMMMUIdx_S1SE0 : ARMMMUIdx_S1NSE0;
> +            break;
> +        default:
> +            g_assert_not_reached();
> +        }
> +        break;
> +    case 4:
> +        /* stage 1+2 NonSecure PL1 */
> +        mmu_idx = ARMMMUIdx_S12NSE1;
> +        break;
> +    case 6:
> +        /* stage 1+2 NonSecure PL0 */
> +        mmu_idx = ARMMMUIdx_S12NSE0;
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
> +
> +    par64 = do_ats_write(env, value, access_type, mmu_idx);
>
>      A32_BANKED_CURRENT_REG_SET(env, par, par64);
>  }
> @@ -1498,10 +1545,40 @@ static void ats_write(CPUARMState *env, const
> ARMCPRegInfo *ri, uint64_t value)
>  static void ats_write64(CPUARMState *env, const ARMCPRegInfo *ri,
>                          uint64_t value)
>  {
> -    int is_user = ri->opc2 & 2;
>      int access_type = ri->opc2 & 1;
> +    ARMMMUIdx mmu_idx;
> +    int secure = arm_is_secure_below_el3(env);
> +
> +    switch (ri->opc2 & 6) {
> +    case 0:
> +        switch (ri->opc1) {
> +        case 0:
> +            mmu_idx = secure ? ARMMMUIdx_S1SE1 : ARMMMUIdx_S1NSE1;
> +            break;
> +        case 4:
> +            mmu_idx = ARMMMUIdx_S1E2;
> +            break;
> +        case 6:
> +            mmu_idx = ARMMMUIdx_S1E3;
> +            break;
> +        default:
> +            g_assert_not_reached();
> +        }
> +        break;
> +    case 2:
> +        mmu_idx = secure ? ARMMMUIdx_S1SE0 : ARMMMUIdx_S1NSE0;
> +        break;
> +    case 4:
> +        mmu_idx = ARMMMUIdx_S12NSE1;
> +        break;
> +    case 6:
> +        mmu_idx = ARMMMUIdx_S12NSE0;
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
>

​The above cases would be more readable if each case had a comment
identifying the corresponding AT instruction.  Just faster for reference
purposes.


>
> -    env->cp15.par_el[1] = do_ats_write(env, value, access_type, is_user);
> +    env->cp15.par_el[1] = do_ats_write(env, value, access_type, mmu_idx);
>  }
>  #endif
>
> @@ -5084,13 +5161,13 @@ static int get_phys_addr_mpu(CPUARMState *env,
> uint32_t address,
>   * @env: CPUARMState
>   * @address: virtual address to get physical address for
>   * @access_type: 0 for read, 1 for write, 2 for execute
> - * @is_user: 0 for privileged access, 1 for user
> + * @mmu_idx: MMU index indicating required translation regime
>   * @phys_ptr: set to the physical address corresponding to the virtual
> address
>   * @prot: set to the permissions for the page containing phys_ptr
>   * @page_size: set to the size of the page containing phys_ptr
>   */
>  static inline int get_phys_addr(CPUARMState *env, target_ulong address,
> -                                int access_type, int is_user,
> +                                int access_type, ARMMMUIdx mmu_idx,
>                                  hwaddr *phys_ptr, int *prot,
>                                  target_ulong *page_size)
>  {
> @@ -5099,6 +5176,11 @@ static inline int get_phys_addr(CPUARMState *env,
> target_ulong address,
>       */
>      uint32_t sctlr = A32_BANKED_CURRENT_REG_GET(env, sctlr);
>
> +    /* This will go away when we handle mmu_idx properly here */
> +    int is_user = (mmu_idx == ARMMMUIdx_S12NSE0 ||
> +                   mmu_idx == ARMMMUIdx_S1SE0 ||
> +                   mmu_idx == ARMMMUIdx_S1NSE0);
> +
>      /* Fast Context Switch Extension.  */
>      if (address < 0x02000000) {
>          address += A32_BANKED_CURRENT_REG_GET(env, fcseidr);
> @@ -5134,13 +5216,11 @@ int arm_cpu_handle_mmu_fault(CPUState *cs, vaddr
> address,
>      hwaddr phys_addr;
>      target_ulong page_size;
>      int prot;
> -    int ret, is_user;
> +    int ret;
>      uint32_t syn;
>      bool same_el = (arm_current_el(env) != 0);
>
> -    /* TODO: pass the translation regime to get_phys_addr */
> -    is_user = (arm_mmu_idx_to_el(mmu_idx) == 0);
> -    ret = get_phys_addr(env, address, access_type, is_user, &phys_addr,
> &prot,
> +    ret = get_phys_addr(env, address, access_type, mmu_idx, &phys_addr,
> &prot,
>                          &page_size);
>      if (ret == 0) {
>          /* Map a single [sub]page.  */
> @@ -5176,12 +5256,14 @@ int arm_cpu_handle_mmu_fault(CPUState *cs, vaddr
> address,
>  hwaddr arm_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
>  {
>      ARMCPU *cpu = ARM_CPU(cs);
> +    CPUARMState *env = &cpu->env;
>      hwaddr phys_addr;
>      target_ulong page_size;
>      int prot;
>      int ret;
>
> -    ret = get_phys_addr(&cpu->env, addr, 0, 0, &phys_addr, &prot,
> &page_size);
> +    ret = get_phys_addr(env, addr, 0, cpu_mmu_index(env), &phys_addr,
> +                        &prot, &page_size);
>
>      if (ret != 0) {
>          return -1;
> --
> 1.9.1
>
>
>
​Otherwise,

Reviewed-by: Greg Bellows <greg.bellows@linaro.org>​

[-- Attachment #2: Type: text/html, Size: 10551 bytes --]

Re: [Qemu-devel] [PATCH 08/11] target-arm: Pass mmu_idx to get_phys_addr()

[Peter Maydell]

On 26 January 2015 at 21:41, Greg Bellows <greg.bellows@linaro.org> wrote:
> The above cases would be more readable if each case had a comment
> identifying the corresponding AT instruction.  Just faster for reference
> purposes.

Makes sense; will add them in v2.

-- PMM

Re: [Qemu-devel] [PATCH 05/11] target-arm: Use correct mmu_idx for unprivileged loads and stores

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 2059 bytes --]

On Mon, Jan 26, 2015 at 2:37 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 26 January 2015 at 19:34, Greg Bellows <greg.bellows@linaro.org> wrote:
> > On Mon, Jan 26, 2015 at 8:56 AM, Peter Maydell <peter.maydell@linaro.org
> >
> > wrote:
> >> Because that's what the ARM ARM specifies. Compare C3.2.5 (A64 LDT &c)
> >> with F7.1.95 (A32/T32 LDRT).
> >
> >
> > I had been comparing the wording of ARMv8 - F1.6.3 and ARMv7 - A4.6.3.
> > After comparing the LDRT instructions between A64 (C6.6.97) and A32
> > (F7.1.95), I am still missing the distinction that warrants the following
> > different behavior:
> >
> > - EL2 is unpredictable in both A64 and A32, but in one case we treat it
> as
> > such and the other we demote it to NS/EL0 to allow it.
>
> No, it's not unpredictable in A64. It behaves as if a normal
> (EL2) access [C3.2.5 "if the PE is executing in any other Exception
> level, then a normal memory access for that level is performed"].
> It is only unpredictable at EL2 in A32/T32 [F7.1.95 "UNPREDICTABLE
> in Hyp mode"; in the v7 ARM ARM, A4.6.3 "UNPREDICTABLE if executed
> at PL2"]. You'll see that the pseudocode for A32/T32 LDRT has
> an UNPREDICTABLE check for PL2, but the pseudocode for A64
> LDTR does not have any equivalent check.
>

​My bad, you are correct, I read S2NS too fast.​


>
> > - EL3 is demoted to S/EL0 in one case but remains EL3 in the
> > other.
>
> Remains EL3 for AArch64 (by the same C3.2.5 requirement quoted above);
> must act as if EL0 for AArch32 (F7.1.95 "as if the PE were running
> in User mode").
>
> ​Ah yes.. EL3 is PL1 on AArch32.



> This is because an EL3 A32/T32 insn is PL1, and AArch32 accesses
> from PL1 must behave as if from PL0 (otherwise pre-v8 software
> would break). An EL3 A64 insn, on the other hand, is definitely
> not EL1 and there's no back-compatibility behaviour required.
>
> Both these differences are required by the spec.
>
> -- PMM
>

​I see the differences now, thanks for the clarification.​

[-- Attachment #2: Type: text/html, Size: 3342 bytes --]

Re: [Qemu-devel] [PATCH 10/11] target-arm: Reindent ancient page-table-walk code

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 8135 bytes --]

On Fri, Jan 23, 2015 at 12:20 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> A few of the oldest parts of the page-table-walk code have broken indent
> (either hardcoded tabs or two-spaces). Reindent these sections.
>
> For ease of review, this patch does not touch the brace style and
> so is a whitespace-only change.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/helper.c | 192
> ++++++++++++++++++++++++++--------------------------
>  1 file changed, 96 insertions(+), 96 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 0a06bbe..3a23af8 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4636,55 +4636,55 @@ static inline int check_ap(CPUARMState *env,
> ARMMMUIdx mmu_idx,
>                             int ap, int domain_prot,
>                             int access_type)
>  {
> -  int prot_ro;
> -  bool is_user = regime_is_user(env, mmu_idx);
> -
> -  if (domain_prot == 3) {
> -    return PAGE_READ | PAGE_WRITE;
> -  }
> -
> -  if (access_type == 1)
> -      prot_ro = 0;
> -  else
> -      prot_ro = PAGE_READ;
> -
> -  switch (ap) {
> -  case 0:
> -      if (arm_feature(env, ARM_FEATURE_V7)) {
> -          return 0;
> -      }
> -      if (access_type == 1)
> -          return 0;
> -      switch (regime_sctlr(env, mmu_idx) & (SCTLR_S | SCTLR_R)) {
> -      case SCTLR_S:
> -          return is_user ? 0 : PAGE_READ;
> -      case SCTLR_R:
> -          return PAGE_READ;
> -      default:
> -          return 0;
> -      }
> -  case 1:
> -      return is_user ? 0 : PAGE_READ | PAGE_WRITE;
> -  case 2:
> -      if (is_user)
> -          return prot_ro;
> -      else
> -          return PAGE_READ | PAGE_WRITE;
> -  case 3:
> -      return PAGE_READ | PAGE_WRITE;
> -  case 4: /* Reserved.  */
> -      return 0;
> -  case 5:
> -      return is_user ? 0 : prot_ro;
> -  case 6:
> -      return prot_ro;
> -  case 7:
> -      if (!arm_feature (env, ARM_FEATURE_V6K))
> -          return 0;
> -      return prot_ro;
> -  default:
> -      abort();
> -  }
> +    int prot_ro;
> +    bool is_user = regime_is_user(env, mmu_idx);
> +
> +    if (domain_prot == 3) {
> +        return PAGE_READ | PAGE_WRITE;
> +    }
> +
> +    if (access_type == 1)
> +        prot_ro = 0;
> +    else
> +        prot_ro = PAGE_READ;
> +
> +    switch (ap) {
> +    case 0:
> +        if (arm_feature(env, ARM_FEATURE_V7)) {
> +            return 0;
> +        }
> +        if (access_type == 1)
> +            return 0;
> +        switch (regime_sctlr(env, mmu_idx) & (SCTLR_S | SCTLR_R)) {
> +        case SCTLR_S:
> +            return is_user ? 0 : PAGE_READ;
> +        case SCTLR_R:
> +            return PAGE_READ;
> +        default:
> +            return 0;
> +        }
> +    case 1:
> +        return is_user ? 0 : PAGE_READ | PAGE_WRITE;
> +    case 2:
> +        if (is_user)
> +            return prot_ro;
> +        else
> +            return PAGE_READ | PAGE_WRITE;
> +    case 3:
> +        return PAGE_READ | PAGE_WRITE;
> +    case 4: /* Reserved.  */
> +        return 0;
> +    case 5:
> +        return is_user ? 0 : prot_ro;
> +    case 6:
> +        return prot_ro;
> +    case 7:
> +        if (!arm_feature (env, ARM_FEATURE_V6K))
> +            return 0;
> +        return prot_ro;
> +    default:
> +        abort();
> +    }
>  }
>
>  static bool get_level1_table_address(CPUARMState *env, ARMMMUIdx mmu_idx,
> @@ -4762,13 +4762,13 @@ static int get_phys_addr_v5(CPUARMState *env,
> uint32_t address, int access_type,
>          *page_size = 1024 * 1024;
>      } else {
>          /* Lookup l2 entry.  */
> -       if (type == 1) {
> -           /* Coarse pagetable.  */
> -           table = (desc & 0xfffffc00) | ((address >> 10) & 0x3fc);
> -       } else {
> -           /* Fine pagetable.  */
> -           table = (desc & 0xfffff000) | ((address >> 8) & 0xffc);
> -       }
> +        if (type == 1) {
> +            /* Coarse pagetable.  */
> +            table = (desc & 0xfffffc00) | ((address >> 10) & 0x3fc);
> +        } else {
> +            /* Fine pagetable.  */
> +            table = (desc & 0xfffff000) | ((address >> 8) & 0xffc);
> +        }
>          desc = ldl_phys(cs->as, table);
>          switch (desc & 3) {
>          case 0: /* Page translation fault.  */
> @@ -4785,17 +4785,17 @@ static int get_phys_addr_v5(CPUARMState *env,
> uint32_t address, int access_type,
>              *page_size = 0x1000;
>              break;
>          case 3: /* 1k page.  */
> -           if (type == 1) {
> -               if (arm_feature(env, ARM_FEATURE_XSCALE)) {
> -                   phys_addr = (desc & 0xfffff000) | (address & 0xfff);
> -               } else {
> -                   /* Page translation fault.  */
> -                   code = 7;
> -                   goto do_fault;
> -               }
> -           } else {
> -               phys_addr = (desc & 0xfffffc00) | (address & 0x3ff);
> -           }
> +            if (type == 1) {
> +                if (arm_feature(env, ARM_FEATURE_XSCALE)) {
> +                    phys_addr = (desc & 0xfffff000) | (address & 0xfff);
> +                } else {
> +                    /* Page translation fault.  */
> +                    code = 7;
> +                    goto do_fault;
> +                }
> +            } else {
> +                phys_addr = (desc & 0xfffffc00) | (address & 0x3ff);
> +            }
>              ap = (desc >> 4) & 3;
>              *page_size = 0x400;
>              break;
> @@ -5190,18 +5190,18 @@ static int get_phys_addr_mpu(CPUARMState *env,
> uint32_t address,
>
>      *phys_ptr = address;
>      for (n = 7; n >= 0; n--) {
> -       base = env->cp15.c6_region[n];
> -       if ((base & 1) == 0)
> -           continue;
> -       mask = 1 << ((base >> 1) & 0x1f);
> -       /* Keep this shift separate from the above to avoid an
> -          (undefined) << 32.  */
> -       mask = (mask << 1) - 1;
> -       if (((base ^ address) & ~mask) == 0)
> -           break;
> +        base = env->cp15.c6_region[n];
> +        if ((base & 1) == 0)
> +            continue;
> +        mask = 1 << ((base >> 1) & 0x1f);
> +        /* Keep this shift separate from the above to avoid an
> +           (undefined) << 32.  */
> +        mask = (mask << 1) - 1;
> +        if (((base ^ address) & ~mask) == 0)
> +            break;
>      }
>      if (n < 0)
> -       return 2;
> +        return 2;
>
>      if (access_type == 2) {
>          mask = env->cp15.pmsav5_insn_ap;
> @@ -5211,31 +5211,31 @@ static int get_phys_addr_mpu(CPUARMState *env,
> uint32_t address,
>      mask = (mask >> (n * 4)) & 0xf;
>      switch (mask) {
>      case 0:
> -       return 1;
> +        return 1;
>      case 1:
> -       if (is_user)
> -         return 1;
> -       *prot = PAGE_READ | PAGE_WRITE;
> -       break;
> +        if (is_user)
> +          return 1;
> +        *prot = PAGE_READ | PAGE_WRITE;
> +        break;
>      case 2:
> -       *prot = PAGE_READ;
> -       if (!is_user)
> -           *prot |= PAGE_WRITE;
> -       break;
> +        *prot = PAGE_READ;
> +        if (!is_user)
> +            *prot |= PAGE_WRITE;
> +        break;
>      case 3:
> -       *prot = PAGE_READ | PAGE_WRITE;
> -       break;
> +        *prot = PAGE_READ | PAGE_WRITE;
> +        break;
>      case 5:
> -       if (is_user)
> -           return 1;
> -       *prot = PAGE_READ;
> -       break;
> +        if (is_user)
> +            return 1;
> +        *prot = PAGE_READ;
> +        break;
>      case 6:
> -       *prot = PAGE_READ;
> -       break;
> +        *prot = PAGE_READ;
> +        break;
>      default:
> -       /* Bad permission.  */
> -       return 1;
> +        /* Bad permission.  */
> +        return 1;
>      }
>      *prot |= PAGE_EXEC;
>      return 0;
> --
> 1.9.1
>
>
​Reviewed-by: Greg Bellows <greg.bellows@linaro.org>​

[-- Attachment #2: Type: text/html, Size: 10520 bytes --]

Re: [Qemu-devel] [PATCH 11/11] target-arm: Fix brace style in reindented code

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 3663 bytes --]

On Fri, Jan 23, 2015 at 12:20 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> This patch fixes the brace style in the code reindented in the
> previous commit.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/helper.c | 36 +++++++++++++++++++++++-------------
>  1 file changed, 23 insertions(+), 13 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 3a23af8..cc80829 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4643,18 +4643,20 @@ static inline int check_ap(CPUARMState *env,
> ARMMMUIdx mmu_idx,
>          return PAGE_READ | PAGE_WRITE;
>      }
>
> -    if (access_type == 1)
> +    if (access_type == 1) {
>          prot_ro = 0;
> -    else
> +    } else {
>          prot_ro = PAGE_READ;
> +    }
>
>      switch (ap) {
>      case 0:
>          if (arm_feature(env, ARM_FEATURE_V7)) {
>              return 0;
>          }
> -        if (access_type == 1)
> +        if (access_type == 1) {
>              return 0;
> +        }
>          switch (regime_sctlr(env, mmu_idx) & (SCTLR_S | SCTLR_R)) {
>          case SCTLR_S:
>              return is_user ? 0 : PAGE_READ;
> @@ -4666,10 +4668,11 @@ static inline int check_ap(CPUARMState *env,
> ARMMMUIdx mmu_idx,
>      case 1:
>          return is_user ? 0 : PAGE_READ | PAGE_WRITE;
>      case 2:
> -        if (is_user)
> +        if (is_user) {
>              return prot_ro;
> -        else
> +        } else {
>              return PAGE_READ | PAGE_WRITE;
> +        }
>      case 3:
>          return PAGE_READ | PAGE_WRITE;
>      case 4: /* Reserved.  */
> @@ -4679,8 +4682,9 @@ static inline int check_ap(CPUARMState *env,
> ARMMMUIdx mmu_idx,
>      case 6:
>          return prot_ro;
>      case 7:
> -        if (!arm_feature (env, ARM_FEATURE_V6K))
> +        if (!arm_feature(env, ARM_FEATURE_V6K)) {
>              return 0;
> +        }
>          return prot_ro;
>      default:
>          abort();
> @@ -5191,17 +5195,20 @@ static int get_phys_addr_mpu(CPUARMState *env,
> uint32_t address,
>      *phys_ptr = address;
>      for (n = 7; n >= 0; n--) {
>          base = env->cp15.c6_region[n];
> -        if ((base & 1) == 0)
> +        if ((base & 1) == 0) {
>              continue;
> +        }
>          mask = 1 << ((base >> 1) & 0x1f);
>          /* Keep this shift separate from the above to avoid an
>             (undefined) << 32.  */
>          mask = (mask << 1) - 1;
> -        if (((base ^ address) & ~mask) == 0)
> +        if (((base ^ address) & ~mask) == 0) {
>              break;
> +        }
>      }
> -    if (n < 0)
> +    if (n < 0) {
>          return 2;
> +    }
>
>      if (access_type == 2) {
>          mask = env->cp15.pmsav5_insn_ap;
> @@ -5213,21 +5220,24 @@ static int get_phys_addr_mpu(CPUARMState *env,
> uint32_t address,
>      case 0:
>          return 1;
>      case 1:
> -        if (is_user)
> -          return 1;
> +        if (is_user) {
> +            return 1;
> +        }
>          *prot = PAGE_READ | PAGE_WRITE;
>          break;
>      case 2:
>          *prot = PAGE_READ;
> -        if (!is_user)
> +        if (!is_user) {
>              *prot |= PAGE_WRITE;
> +        }
>          break;
>      case 3:
>          *prot = PAGE_READ | PAGE_WRITE;
>          break;
>      case 5:
> -        if (is_user)
> +        if (is_user) {
>              return 1;
> +        }
>          *prot = PAGE_READ;
>          break;
>      case 6:
> --
> 1.9.1
>
>
​Reviewed-by: Greg Bellows <greg.bellows@linaro.org>​

[-- Attachment #2: Type: text/html, Size: 5009 bytes --]

Re: [Qemu-devel] [PATCH 09/11] target-arm: Use mmu_idx in get_phys_addr()

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 17142 bytes --]

On Fri, Jan 23, 2015 at 12:20 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> Now we have the mmu_idx in get_phys_addr(), use it correctly to
> determine the behaviour of virtual to physical address translations,
> rather than using just an is_user flag and the current CPU state.
>
> Some TODO comments have been added to indicate where changes will
> need to be made to add EL2 and 64-bit EL3 support.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/helper.c | 200
> +++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 151 insertions(+), 49 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 0ae04eb..0a06bbe 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4556,13 +4556,88 @@ void arm_cpu_do_interrupt(CPUState *cs)
>      cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
>  }
>
> +
> +/* Return the exception level which controls this address translation
> regime */
> +static inline uint32_t regime_el(CPUARMState *env, ARMMMUIdx mmu_idx)
> +{
> +    switch (mmu_idx) {
> +    case ARMMMUIdx_S2NS:
> +    case ARMMMUIdx_S1E2:
> +        return 2;
> +    case ARMMMUIdx_S1E3:
> +        return 3;
> +    case ARMMMUIdx_S1SE0:
> +        return arm_el_is_aa64(env, 3) ? 1 : 3;
> +    case ARMMMUIdx_S1SE1:
>

​I think this should be handled the same way as S1SE0 as Secure EL1 maps to
EL3 when EL3 is AArch32.


> +    case ARMMMUIdx_S1NSE0:
> +    case ARMMMUIdx_S1NSE1:
> +        return 1;
> +    default:
> +        g_assert_not_reached();
> +    }
> +}
> +
> +/* Return the SCTLR value which controls this address translation regime
> */
> +static inline uint32_t regime_sctlr(CPUARMState *env, ARMMMUIdx mmu_idx)
> +{
> +    return env->cp15.sctlr_el[regime_el(env, mmu_idx)];
>

​Given the above regime_el(), for S1SE1, this would return the non-secure
SCTLR bank on a secure translation.  Same below for TCR and all uses
thereafter.


> +}
> +
> +/* Return true if the specified stage of address translation is disabled
> */
> +static inline bool regime_translation_disabled(CPUARMState *env,
> +                                               ARMMMUIdx mmu_idx)
> +{
> +    if (mmu_idx == ARMMMUIdx_S2NS) {
> +        return (env->cp15.hcr_el2 & HCR_VM) == 0;
> +    }
> +    return (regime_sctlr(env, mmu_idx) & SCTLR_M) == 0;
> +}
> +
> +/* Return the TCR controlling this translation regime */
> +static inline TCR *regime_tcr(CPUARMState *env, ARMMMUIdx mmu_idx)
> +{
> +    if (mmu_idx == ARMMMUIdx_S2NS) {
> +        /* TODO: return VTCR_EL2 */
> +        g_assert_not_reached();
> +    }
> +    return &env->cp15.tcr_el[regime_el(env, mmu_idx)];
> +}
> +
> +/* Return true if the translation regime is using LPAE format page tables
> */
> +static inline bool regime_using_lpae_format(CPUARMState *env,
> +                                            ARMMMUIdx mmu_idx)
> +{
> +    int el = regime_el(env, mmu_idx);
> +    if (el == 2 || arm_el_is_aa64(env, el)) {
> +        return true;
> +    }
> +    if (arm_feature(env, ARM_FEATURE_LPAE)
> +        && (regime_tcr(env, mmu_idx)->raw_tcr & TTBCR_EAE)) {
> +        return true;
> +    }
> +    return false;
> +}
> +
> +static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx)
> +{
> +    switch (mmu_idx) {
> +    case ARMMMUIdx_S1SE0:
> +    case ARMMMUIdx_S1NSE0:
>

The get_phys_addr path filters out ARMMMUIdx_S12NSE0 so calls to this
function in this context don't matter, but what if it is called outside
this context?  How should it handle this index?


> +        return true;
> +    default:
> +        return false;
> +    }
> +}
> +
>  /* Check section/page access permissions.
>     Returns the page protection flags, or zero if the access is not
>     permitted.  */
> -static inline int check_ap(CPUARMState *env, int ap, int domain_prot,
> -                           int access_type, int is_user)
> +static inline int check_ap(CPUARMState *env, ARMMMUIdx mmu_idx,
> +                           int ap, int domain_prot,
> +                           int access_type)
>  {
>    int prot_ro;
> +  bool is_user = regime_is_user(env, mmu_idx);
>
>    if (domain_prot == 3) {
>      return PAGE_READ | PAGE_WRITE;
> @@ -4580,7 +4655,7 @@ static inline int check_ap(CPUARMState *env, int ap,
> int domain_prot,
>        }
>        if (access_type == 1)
>            return 0;
> -      switch (A32_BANKED_CURRENT_REG_GET(env, sctlr) & (SCTLR_S |
> SCTLR_R)) {
> +      switch (regime_sctlr(env, mmu_idx) & (SCTLR_S | SCTLR_R)) {
>        case SCTLR_S:
>            return is_user ? 0 : PAGE_READ;
>        case SCTLR_R:
> @@ -4612,35 +4687,32 @@ static inline int check_ap(CPUARMState *env, int
> ap, int domain_prot,
>    }
>  }
>
> -static bool get_level1_table_address(CPUARMState *env, uint32_t *table,
> -                                         uint32_t address)
> +static bool get_level1_table_address(CPUARMState *env, ARMMMUIdx mmu_idx,
> +                                     uint32_t *table, uint32_t address)
>  {
> -    /* Get the TCR bank based on our security state */
> -    TCR *tcr = &env->cp15.tcr_el[arm_is_secure(env) ? 3 : 1];
> +    /* Note that we can only get here for an AArch32 PL0/PL1 lookup */
> +    int el = regime_el(env, mmu_idx);
> +    TCR *tcr = regime_tcr(env, mmu_idx);
>
> -    /* We only get here if EL1 is running in AArch32. If EL3 is running in
> -     * AArch32 there is a secure and non-secure instance of the
> translation
> -     * table registers.
> -     */
>      if (address & tcr->mask) {
>          if (tcr->raw_tcr & TTBCR_PD1) {
>              /* Translation table walk disabled for TTBR1 */
>              return false;
>          }
> -        *table = A32_BANKED_CURRENT_REG_GET(env, ttbr1) & 0xffffc000;
> +        *table = env->cp15.ttbr1_el[el] & 0xffffc000;
>      } else {
>          if (tcr->raw_tcr & TTBCR_PD0) {
>              /* Translation table walk disabled for TTBR0 */
>              return false;
>          }
> -        *table = A32_BANKED_CURRENT_REG_GET(env, ttbr0) & tcr->base_mask;
> +        *table = env->cp15.ttbr0_el[el] & tcr->base_mask;
>      }
>      *table |= (address >> 18) & 0x3ffc;
>      return true;
>  }
>
>  static int get_phys_addr_v5(CPUARMState *env, uint32_t address, int
> access_type,
> -                            int is_user, hwaddr *phys_ptr,
> +                            ARMMMUIdx mmu_idx, hwaddr *phys_ptr,
>                              int *prot, target_ulong *page_size)
>  {
>      CPUState *cs = CPU(arm_env_get_cpu(env));
> @@ -4652,10 +4724,11 @@ static int get_phys_addr_v5(CPUARMState *env,
> uint32_t address, int access_type,
>      int domain = 0;
>      int domain_prot;
>      hwaddr phys_addr;
> +    uint32_t dacr;
>
>      /* Pagetable walk.  */
>      /* Lookup l1 descriptor.  */
> -    if (!get_level1_table_address(env, &table, address)) {
> +    if (!get_level1_table_address(env, mmu_idx, &table, address)) {
>          /* Section translation fault if page walk is disabled by PD0 or
> PD1 */
>          code = 5;
>          goto do_fault;
> @@ -4663,7 +4736,12 @@ static int get_phys_addr_v5(CPUARMState *env,
> uint32_t address, int access_type,
>      desc = ldl_phys(cs->as, table);
>      type = (desc & 3);
>      domain = (desc >> 5) & 0x0f;
> -    domain_prot = (A32_BANKED_CURRENT_REG_GET(env, dacr) >> (domain * 2))
> & 3;
> +    if (regime_el(env, mmu_idx) == 1) {
> +        dacr = env->cp15.dacr_ns;
> +    } else {
> +        dacr = env->cp15.dacr_s;
> +    }
> +    domain_prot = (dacr >> (domain * 2)) & 3;
>      if (type == 0) {
>          /* Section translation fault.


>  */
>          code = 5;
> @@ -4727,7 +4805,7 @@ static int get_phys_addr_v5(CPUARMState *env,
> uint32_t address, int access_type,
>          }
>          code = 15;
>      }
> -    *prot = check_ap(env, ap, domain_prot, access_type, is_user);
> +    *prot = check_ap(env, mmu_idx, ap, domain_prot, access_type);
>      if (!*prot) {
>          /* Access permission fault.  */
>          goto do_fault;
> @@ -4740,7 +4818,7 @@ do_fault:
>  }
>
>  static int get_phys_addr_v6(CPUARMState *env, uint32_t address, int
> access_type,
> -                            int is_user, hwaddr *phys_ptr,
> +                            ARMMMUIdx mmu_idx, hwaddr *phys_ptr,
>                              int *prot, target_ulong *page_size)
>  {
>      CPUState *cs = CPU(arm_env_get_cpu(env));
> @@ -4754,10 +4832,11 @@ static int get_phys_addr_v6(CPUARMState *env,
> uint32_t address, int access_type,
>      int domain = 0;
>      int domain_prot;
>      hwaddr phys_addr;
> +    uint32_t dacr;
>
>      /* Pagetable walk.  */
>      /* Lookup l1 descriptor.  */
> -    if (!get_level1_table_address(env, &table, address)) {
> +    if (!get_level1_table_address(env, mmu_idx, &table, address)) {
>          /* Section translation fault if page walk is disabled by PD0 or
> PD1 */
>          code = 5;
>          goto do_fault;
> @@ -4775,7 +4854,12 @@ static int get_phys_addr_v6(CPUARMState *env,
> uint32_t address, int access_type,
>          /* Page or Section.  */
>          domain = (desc >> 5) & 0x0f;
>      }
> -    domain_prot = (A32_BANKED_CURRENT_REG_GET(env, dacr) >> (domain * 2))
> & 3;
> +    if (regime_el(env, mmu_idx) == 1) {
> +        dacr = env->cp15.dacr_ns;
> +    } else {
> +        dacr = env->cp15.dacr_s;
> +    }
> +    domain_prot = (dacr >> (domain * 2)) & 3;
>      if (domain_prot == 0 || domain_prot == 2) {
>          if (type != 1) {
>              code = 9; /* Section domain fault.  */
> @@ -4829,20 +4913,20 @@ static int get_phys_addr_v6(CPUARMState *env,
> uint32_t address, int access_type,
>      if (domain_prot == 3) {
>          *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
>      } else {
> -        if (pxn && !is_user) {
> +        if (pxn && !regime_is_user(env, mmu_idx)) {
>              xn = 1;
>          }
>          if (xn && access_type == 2)
>              goto do_fault;
>
>          /* The simplified model uses AP[0] as an access control bit.  */
> -        if ((A32_BANKED_CURRENT_REG_GET(env, sctlr) & SCTLR_AFE)
> +        if ((regime_sctlr(env, mmu_idx) & SCTLR_AFE)
>                  && (ap & 1) == 0) {
>              /* Access flag fault.  */
>              code = (code == 15) ? 6 : 3;
>              goto do_fault;
>          }
> -        *prot = check_ap(env, ap, domain_prot, access_type, is_user);
> +        *prot = check_ap(env, mmu_idx, ap, domain_prot, access_type);
>          if (!*prot) {
>              /* Access permission fault.  */
>              goto do_fault;
> @@ -4867,7 +4951,7 @@ typedef enum {
>  } MMUFaultType;
>
>  static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
> -                              int access_type, int is_user,
> +                              int access_type, ARMMMUIdx mmu_idx,
>                                hwaddr *phys_ptr, int *prot,
>                                target_ulong *page_size_ptr)
>  {
> @@ -4887,9 +4971,17 @@ static int get_phys_addr_lpae(CPUARMState *env,
> target_ulong address,
>      int32_t granule_sz = 9;
>      int32_t va_size = 32;
>      int32_t tbi = 0;
> -    TCR *tcr = &env->cp15.tcr_el[arm_is_secure(env) ? 3 : 1];
> -
> -    if (arm_el_is_aa64(env, 1)) {
> +    bool is_user;
> +    TCR *tcr = regime_tcr(env, mmu_idx);
> +
> +    /* TODO:
> +     * This code assumes we're either a 64-bit EL1 or a 32-bit PL1;
> +     * it doesn't handle the different format TCR for TCR_EL2, TCR_EL3,
> +     * and VTCR_EL2, or the fact that those regimes don't have a split
> +     * TTBR0/TTBR1. Attribute and permission bit handling should also
> +     * be checked when adding support for those page table walks.
> +     */
> +    if (arm_el_is_aa64(env, regime_el(env, mmu_idx))) {
>          va_size = 64;
>          if (extract64(address, 55, 1))
>              tbi = extract64(tcr->raw_tcr, 38, 1);
> @@ -4904,12 +4996,12 @@ static int get_phys_addr_lpae(CPUARMState *env,
> target_ulong address,
>       * TTBCR/TTBR0/TTBR1 in accordance with ARM ARM DDI0406C table B-32:
>       */
>      uint32_t t0sz = extract32(tcr->raw_tcr, 0, 6);
> -    if (arm_el_is_aa64(env, 1)) {
> +    if (va_size == 64) {
>          t0sz = MIN(t0sz, 39);
>          t0sz = MAX(t0sz, 16);
>      }
>      uint32_t t1sz = extract32(tcr->raw_tcr, 16, 6);
> -    if (arm_el_is_aa64(env, 1)) {
> +    if (va_size == 64) {
>          t1sz = MIN(t1sz, 39);
>          t1sz = MAX(t1sz, 16);
>      }
> @@ -4964,6 +5056,10 @@ static int 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
> +     */
> +
>      if (epd) {
>          /* Translation table walk disabled => Translation fault on TLB
> miss */
>          goto do_fault;
> @@ -5049,6 +5145,7 @@ static int get_phys_addr_lpae(CPUARMState *env,
> target_ulong address,
>          goto do_fault;
>      }
>      fault_type = permission_fault;
> +    is_user = regime_is_user(env, mmu_idx);
>      if (is_user && !(attrs & (1 << 4))) {
>          /* Unprivileged access not enabled */
>          goto do_fault;
> @@ -5083,12 +5180,13 @@ do_fault:
>  }
>
>  static int get_phys_addr_mpu(CPUARMState *env, uint32_t address,
> -                             int access_type, int is_user,
> +                             int access_type, ARMMMUIdx mmu_idx,
>                               hwaddr *phys_ptr, int *prot)
>  {
>      int n;
>      uint32_t mask;
>      uint32_t base;
> +    bool is_user = regime_is_user(env, mmu_idx);
>
>      *phys_ptr = address;
>      for (n = 7; n >= 0; n--) {
> @@ -5171,39 +5269,43 @@ static inline int get_phys_addr(CPUARMState *env,
> target_ulong address,
>                                  hwaddr *phys_ptr, int *prot,
>                                  target_ulong *page_size)
>  {
> -    /* This is not entirely correct as get_phys_addr() can also be called
> -     * from ats_write() for an address translation of a specific regime.
> -     */
> -    uint32_t sctlr = A32_BANKED_CURRENT_REG_GET(env, sctlr);
> -
> -    /* This will go away when we handle mmu_idx properly here */
> -    int is_user = (mmu_idx == ARMMMUIdx_S12NSE0 ||
> -                   mmu_idx == ARMMMUIdx_S1SE0 ||
> -                   mmu_idx == ARMMMUIdx_S1NSE0);
> +    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 ldl_phys
> +         * calls for stage 1 will also need changing.
> +         * For non-EL2 CPUs a stage1+stage2 translation is just stage 1.
> +         */
> +        assert(!arm_feature(env, ARM_FEATURE_EL2));
> +        mmu_idx += ARMMMUIdx_S1NSE0;
> +    }
>
>      /* Fast Context Switch Extension.  */
> -    if (address < 0x02000000) {
> +    if (address < 0x02000000 && mmu_idx != ARMMMUIdx_S2NS) {
>          address += A32_BANKED_CURRENT_REG_GET(env, fcseidr);
>      }
>
> -    if ((sctlr & SCTLR_M) == 0) {
> +    if (regime_translation_disabled(env, mmu_idx)) {
>          /* MMU/MPU disabled.  */
>          *phys_ptr = address;
>          *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
>          *page_size = TARGET_PAGE_SIZE;
>          return 0;
> -    } else if (arm_feature(env, ARM_FEATURE_MPU)) {
> +    }
> +
> +    if (arm_feature(env, ARM_FEATURE_MPU)) {
>          *page_size = TARGET_PAGE_SIZE;
> -       return get_phys_addr_mpu(env, address, access_type, is_user,
> phys_ptr,
> -                                prot);
> -    } else if (extended_addresses_enabled(env)) {
> -        return get_phys_addr_lpae(env, address, access_type, is_user,
> phys_ptr,
> +        return get_phys_addr_mpu(env, address, access_type, mmu_idx,
> phys_ptr,
> +                                 prot);
> +    }
> +
> +    if (regime_using_lpae_format(env, mmu_idx)) {
> +        return get_phys_addr_lpae(env, address, access_type, mmu_idx,
> phys_ptr,
>                                    prot, page_size);
> -    } else if (sctlr & SCTLR_XP) {
> -        return get_phys_addr_v6(env, address, access_type, is_user,
> phys_ptr,
> +    } else if (regime_sctlr(env, mmu_idx) & SCTLR_XP) {
> +        return get_phys_addr_v6(env, address, access_type, mmu_idx,
> phys_ptr,
>                                  prot, page_size);
>      } else {
> -        return get_phys_addr_v5(env, address, access_type, is_user,
> phys_ptr,
> +        return get_phys_addr_v5(env, address, access_type, mmu_idx,
> phys_ptr,
>                                  prot, page_size);
>      }
>  }
> --
> 1.9.1
>
>

[-- Attachment #2: Type: text/html, Size: 21334 bytes --]

Re: [Qemu-devel] [PATCH 09/11] target-arm: Use mmu_idx in get_phys_addr()

[Peter Maydell]

On 27 January 2015 at 17:57, Greg Bellows <greg.bellows@linaro.org> wrote:
>
>
> On Fri, Jan 23, 2015 at 12:20 PM, Peter Maydell <peter.maydell@linaro.org>
> wrote:
>> +/* Return the exception level which controls this address translation
>> regime */
>> +static inline uint32_t regime_el(CPUARMState *env, ARMMMUIdx mmu_idx)
>> +{
>> +    switch (mmu_idx) {
>> +    case ARMMMUIdx_S2NS:
>> +    case ARMMMUIdx_S1E2:
>> +        return 2;
>> +    case ARMMMUIdx_S1E3:
>> +        return 3;
>> +    case ARMMMUIdx_S1SE0:
>> +        return arm_el_is_aa64(env, 3) ? 1 : 3;
>> +    case ARMMMUIdx_S1SE1:
>
>
> I think this should be handled the same way as S1SE0 as Secure EL1 maps to
> EL3 when EL3 is AArch32.

If EL3 is AArch32 then you'll never be using this MMU index.
By definition the S1SE1 index is for code executing at
Secure EL1, and there isn't any of that unless EL3 is 64 bit.
(Secure EL1 doesn't "map to" anything, it just doesn't
exist/have any code running in it.)

>> +    case ARMMMUIdx_S1NSE0:
>> +    case ARMMMUIdx_S1NSE1:
>> +        return 1;
>> +    default:
>> +        g_assert_not_reached();
>> +    }
>> +}
>> +
>> +/* Return the SCTLR value which controls this address translation regime
>> */
>> +static inline uint32_t regime_sctlr(CPUARMState *env, ARMMMUIdx mmu_idx)
>> +{
>> +    return env->cp15.sctlr_el[regime_el(env, mmu_idx)];
>
>
> Given the above regime_el(), for S1SE1, this would return the non-secure
> SCTLR bank on a secure translation.  Same below for TCR and all uses
> thereafter.

That's correct, because S1SE1 implies "secure EL1 under a 64 bit
EL3", in which case there is no system register banking and
both Secure and NonSecure use the same underlying register.
Compare the way that A32_BANKED_CURRENT_REG_GET/SET always
use the NS version if arm_el_is_aa64(env, 3).

>> +static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx)
>> +{
>> +    switch (mmu_idx) {
>> +    case ARMMMUIdx_S1SE0:
>> +    case ARMMMUIdx_S1NSE0:
>
>
> The get_phys_addr path filters out ARMMMUIdx_S12NSE0 so calls to this
> function in this context don't matter, but what if it is called outside this
> context?  How should it handle this index?

g_assert_not_reached(),  but it didn't seem worth cluttering
the switch with a bunch of extra labels just to assert that
they weren't reachable.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 04/11] target-arm: Define correct mmu_idx values and pass them in TB flags

[Peter Maydell]

On 23 January 2015 at 18:20, Peter Maydell <peter.maydell@linaro.org> wrote:
> +/* Determine the current mmu_idx to use for normal loads/stores */
>  static inline int cpu_mmu_index (CPUARMState *env)
>  {
> -    return arm_current_el(env);
> +    int el = arm_current_el(env);
> +
> +    if (el < 3 && arm_is_secure_below_el3(env)) {
> +        return ARMMMUIdx_S1SE0 + el;
> +    }
> +    return el;
>  }

Just noticed the stray extra space between function name and '('
here; will delete the space in v2, since we're rewriting the whole
function body anyhow...

-- PMM

Re: [Qemu-devel] [PATCH 09/11] target-arm: Use mmu_idx in get_phys_addr()

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 2943 bytes --]

On Tue, Jan 27, 2015 at 12:12 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 27 January 2015 at 17:57, Greg Bellows <greg.bellows@linaro.org> wrote:
> >
> >
> > On Fri, Jan 23, 2015 at 12:20 PM, Peter Maydell <
> peter.maydell@linaro.org>
> > wrote:
> >> +/* Return the exception level which controls this address translation
> >> regime */
> >> +static inline uint32_t regime_el(CPUARMState *env, ARMMMUIdx mmu_idx)
> >> +{
> >> +    switch (mmu_idx) {
> >> +    case ARMMMUIdx_S2NS:
> >> +    case ARMMMUIdx_S1E2:
> >> +        return 2;
> >> +    case ARMMMUIdx_S1E3:
> >> +        return 3;
> >> +    case ARMMMUIdx_S1SE0:
> >> +        return arm_el_is_aa64(env, 3) ? 1 : 3;
> >> +    case ARMMMUIdx_S1SE1:
> >
> >
> > I think this should be handled the same way as S1SE0 as Secure EL1 maps
> to
> > EL3 when EL3 is AArch32.
>
> If EL3 is AArch32 then you'll never be using this MMU index.
> By definition the S1SE1 index is for code executing at
> Secure EL1, and there isn't any of that unless EL3 is 64 bit.
> (Secure EL1 doesn't "map to" anything, it just doesn't
> exist/have any code running in it.)
>

​Ah yes and thanks for the pointer to the origin of mmu index I have now
connected where we prevent that index with this code.


>
> >> +    case ARMMMUIdx_S1NSE0:
> >> +    case ARMMMUIdx_S1NSE1:
> >> +        return 1;
> >> +    default:
> >> +        g_assert_not_reached();
> >> +    }
> >> +}
> >> +
> >> +/* Return the SCTLR value which controls this address translation
> regime
> >> */
> >> +static inline uint32_t regime_sctlr(CPUARMState *env, ARMMMUIdx
> mmu_idx)
> >> +{
> >> +    return env->cp15.sctlr_el[regime_el(env, mmu_idx)];
> >
> >
> > Given the above regime_el(), for S1SE1, this would return the non-secure
> > SCTLR bank on a secure translation.  Same below for TCR and all uses
> > thereafter.
>
> That's correct, because S1SE1 implies "secure EL1 under a 64 bit
> EL3", in which case there is no system register banking and
> both Secure and NonSecure use the same underlying register.
> Compare the way that A32_BANKED_CURRENT_REG_GET/SET always
> use the NS version if arm_el_is_aa64(env, 3).
>
> >> +static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx)
> >> +{
> >> +    switch (mmu_idx) {
> >> +    case ARMMMUIdx_S1SE0:
> >> +    case ARMMMUIdx_S1NSE0:
> >
> >
> > The get_phys_addr path filters out ARMMMUIdx_S12NSE0 so calls to this
> > function in this context don't matter, but what if it is called outside
> this
> > context?  How should it handle this index?
>
> g_assert_not_reached(),  but it didn't seem worth cluttering
> the switch with a bunch of extra labels just to assert that
> they weren't reachable.
>
>
​I see how it could clutter things, but given that the routine is generic
we probably should just like we do in regime_el().​



> thanks
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 4439 bytes --]

Re: [Qemu-devel] [PATCH 09/11] target-arm: Use mmu_idx in get_phys_addr()

[Peter Maydell]

On 27 January 2015 at 19:49, Greg Bellows <greg.bellows@linaro.org> wrote:
> On Tue, Jan 27, 2015 at 12:12 PM, Peter Maydell <peter.maydell@linaro.org>
> wrote:
>> g_assert_not_reached(),  but it didn't seem worth cluttering
>> the switch with a bunch of extra labels just to assert that
>> they weren't reachable.
>>
>
> I see how it could clutter things, but given that the routine is generic we
> probably should just like we do in regime_el().

Not a big deal, so I'll add them, but the routine isn't generic --
it's purely a local utility routine for the benefit of the
get_phys_addr family of functions and not intended to be called
from elsewhere.

-- PMM

Re: [Qemu-devel] [PATCH 09/11] target-arm: Use mmu_idx in get_phys_addr()

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 18194 bytes --]

On Fri, Jan 23, 2015 at
​​
12:20 PM, Peter Maydell <peter.maydell@linaro.org> wrote:

> Now we have the mmu_idx in get_phys_addr(), use it correctly to
> determine the behaviour of virtual to physical address translations,
> rather than using just an is_user flag and the current CPU state.
>
> Some TODO comments have been added to indicate where changes will
> need to be made to add EL2 and 64-bit EL3 support.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/helper.c | 200
> +++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 151 insertions(+), 49 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 0ae04eb..0a06bbe 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4556,13 +4556,88 @@ void arm_cpu_do_interrupt(CPUState *cs)
>      cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
>  }
>
> +
> +/* Return the exception level which controls this address translation
> regime */
> +static inline uint32_t regime_el(CPUARMState *env, ARMMMUIdx mmu_idx)
> +{
> +    switch (mmu_idx) {
> +    case ARMMMUIdx_S2NS:
> +    case ARMMMUIdx_S1E2:
> +        return 2;
> +    case ARMMMUIdx_S1E3:
> +        return 3;
> +    case ARMMMUIdx_S1SE0:
> +        return arm_el_is_aa64(env, 3) ? 1 : 3;
> +    case ARMMMUIdx_S1SE1:
> +    case ARMMMUIdx_S1NSE0:
> +    case ARMMMUIdx_S1NSE1:
> +        return 1;
> +    default:
> +        g_assert_not_reached();
> +    }
> +}
> +
> +/* Return the SCTLR value which controls this address translation regime
> */
> +static inline uint32_t regime_sctlr(CPUARMState *env, ARMMMUIdx mmu_idx)
> +{
> +    return env->cp15.sctlr_el[regime_el(env, mmu_idx)];
> +}
> +
> +/* Return true if the specified stage of address translation is disabled
> */
> +static inline bool regime_translation_disabled(CPUARMState *env,
> +                                               ARMMMUIdx mmu_idx)
> +{
> +    if (mmu_idx == ARMMMUIdx_S2NS) {
> +        return (env->cp15.hcr_el2 & HCR_VM) == 0;
> +    }
> +    return (regime_sctlr(env, mmu_idx) & SCTLR_M) == 0;
> +}
> +
> +/* Return the TCR controlling this translation regime */
> +static inline TCR *regime_tcr(CPUARMState *env, ARMMMUIdx mmu_idx)
> +{
> +    if (mmu_idx == ARMMMUIdx_S2NS) {
> +        /* TODO: return VTCR_EL2 */
> +        g_assert_not_reached();
> +    }
> +    return &env->cp15.tcr_el[regime_el(env, mmu_idx)];
> +}
> +
> +/* Return true if the translation regime is using LPAE format page tables
> */
> +static inline bool regime_using_lpae_format(CPUARMState *env,
> +                                            ARMMMUIdx mmu_idx)
> +{
> +    int el = regime_el(env, mmu_idx);
> +    if (el == 2 || arm_el_is_aa64(env, el)) {
>

​For the life of me, I can not figure out why EL2 is wired to always use
LPAE.   Is this stated in the spec somewhere?​  I found places where EL2
registers can vary depending on TTBCR.EAE bit settings which implies it is
not always true.

I realize EL2 is not supported yet, but wouldn't this break AArch32 if it
were and the LPAE feature was not enabled?


> +        return true;
> +    }
> +    if (arm_feature(env, ARM_FEATURE_LPAE)
> +        && (regime_tcr(env, mmu_idx)->raw_tcr & TTBCR_EAE)) {
> +        return true;
> +    }
> +    return false;
> +}
> +
> +static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx)
> +{
> +    switch (mmu_idx) {
> +    case ARMMMUIdx_S1SE0:
> +    case ARMMMUIdx_S1NSE0:
> +        return true;
> +    default:
> +        return false;
> +    }
> +}
> +
>  /* Check section/page access permissions.
>     Returns the page protection flags, or zero if the access is not
>     permitted.  */
> -static inline int check_ap(CPUARMState *env, int ap, int domain_prot,
> -                           int access_type, int is_user)
> +static inline int check_ap(CPUARMState *env, ARMMMUIdx mmu_idx,
> +                           int ap, int domain_prot,
> +                           int access_type)
>  {
>    int prot_ro;
> +  bool is_user = regime_is_user(env, mmu_idx);
>
>    if (domain_prot == 3) {
>      return PAGE_READ | PAGE_WRITE;
> @@ -4580,7 +4655,7 @@ static inline int check_ap(CPUARMState *env, int ap,
> int domain_prot,
>        }
>        if (access_type == 1)
>            return 0;
> -      switch (A32_BANKED_CURRENT_REG_GET(env, sctlr) & (SCTLR_S |
> SCTLR_R)) {
> +      switch (regime_sctlr(env, mmu_idx) & (SCTLR_S | SCTLR_R)) {
>        case SCTLR_S:
>            return is_user ? 0 : PAGE_READ;
>        case SCTLR_R:
> @@ -4612,35 +4687,32 @@ static inline int check_ap(CPUARMState *env, int
> ap, int domain_prot,
>    }
>  }
>
> -static bool get_level1_table_address(CPUARMState *env, uint32_t *table,
> -                                         uint32_t address)
> +static bool get_level1_table_address(CPUARMState *env, ARMMMUIdx mmu_idx,
> +                                     uint32_t *table, uint32_t address)
>  {
> -    /* Get the TCR bank based on our security state */
> -    TCR *tcr = &env->cp15.tcr_el[arm_is_secure(env) ? 3 : 1];
> +    /* Note that we can only get here for an AArch32 PL0/PL1 lookup */
> +    int el = regime_el(env, mmu_idx);
> +    TCR *tcr = regime_tcr(env, mmu_idx);
>
> -    /* We only get here if EL1 is running in AArch32. If EL3 is running in
> -     * AArch32 there is a secure and non-secure instance of the
> translation
> -     * table registers.
> -     */
>      if (address & tcr->mask) {
>          if (tcr->raw_tcr & TTBCR_PD1) {
>              /* Translation table walk disabled for TTBR1 */
>              return false;
>          }
> -        *table = A32_BANKED_CURRENT_REG_GET(env, ttbr1) & 0xffffc000;
> +        *table = env->cp15.ttbr1_el[el] & 0xffffc000;
>

​Perhaps you plan to address this in a separate patch, but I believe TTBR1
is only applicable to ​EL1 and EL0 in AArch64.


>      } else {
>          if (tcr->raw_tcr & TTBCR_PD0) {
>              /* Translation table walk disabled for TTBR0 */
>              return false;
>          }
> -        *table = A32_BANKED_CURRENT_REG_GET(env, ttbr0) & tcr->base_mask;
> +        *table = env->cp15.ttbr0_el[el] & tcr->base_mask;
>      }
>      *table |= (address >> 18) & 0x3ffc;
>      return true;
>  }
>
>  static int get_phys_addr_v5(CPUARMState *env, uint32_t address, int
> access_type,
> -                            int is_user, hwaddr *phys_ptr,
> +                            ARMMMUIdx mmu_idx, hwaddr *phys_ptr,
>                              int *prot, target_ulong *page_size)
>  {
>      CPUState *cs = CPU(arm_env_get_cpu(env));
> @@ -4652,10 +4724,11 @@ static int get_phys_addr_v5(CPUARMState *env,
> uint32_t address, int access_type,
>      int domain = 0;
>      int domain_prot;
>      hwaddr phys_addr;
> +    uint32_t dacr;
>
>      /* Pagetable walk.  */
>      /* Lookup l1 descriptor.  */
> -    if (!get_level1_table_address(env, &table, address)) {
> +    if (!get_level1_table_address(env, mmu_idx, &table, address)) {
>          /* Section translation fault if page walk is disabled by PD0 or
> PD1 */
>          code = 5;
>          goto do_fault;
> @@ -4663,7 +4736,12 @@ static int get_phys_addr_v5(CPUARMState *env,
> uint32_t address, int access_type,
>      desc = ldl_phys(cs->as, table);
>      type = (desc & 3);
>      domain = (desc >> 5) & 0x0f;
> -    domain_prot = (A32_BANKED_CURRENT_REG_GET(env, dacr) >> (domain * 2))
> & 3;
> +    if (regime_el(env, mmu_idx) == 1) {
> +        dacr = env->cp15.dacr_ns;
> +    } else {
> +        dacr = env->cp15.dacr_s;
> +    }
> +    domain_prot = (dacr >> (domain * 2)) & 3;
>

​Is there a reason that you did not add a regime​_dacr() here like you did
for SCTLR and TCR?

Also, the ARMv8 ARM makes it sound like DACR has no value in AArch64.
However, if it did have meaning in AArch64, then for S1SE1 would we be
accessing the wrong bank as regime_el returns 1?  This working off the
understanding that an address reference from an instruction executed in
S/EL1 and AArch64 would generate such an index.

Maybe we need a regime_is_secure()?


>      if (type == 0) {
>          /* Section translation fault.  */
>          code = 5;
> @@ -4727,7 +4805,7 @@ static int get_phys_addr_v5(CPUARMState *env,
> uint32_t address, int access_type,
>          }
>          code = 15;
>      }
> -    *prot = check_ap(env, ap, domain_prot, access_type, is_user);
> +    *prot = check_ap(env, mmu_idx, ap, domain_prot, access_type);
>      if (!*prot) {
>          /* Access permission fault.  */
>          goto do_fault;
> @@ -4740,7 +4818,7 @@ do_fault:
>  }
>
>  static int get_phys_addr_v6(CPUARMState *env, uint32_t address, int
> access_type,
> -                            int is_user, hwaddr *phys_ptr,
> +                            ARMMMUIdx mmu_idx, hwaddr *phys_ptr,
>                              int *prot, target_ulong *page_size)
>  {
>      CPUState *cs = CPU(arm_env_get_cpu(env));
> @@ -4754,10 +4832,11 @@ static int get_phys_addr_v6(CPUARMState *env,
> uint32_t address, int access_type,
>      int domain = 0;
>      int domain_prot;
>      hwaddr phys_addr;
> +    uint32_t dacr;
>
>      /* Pagetable walk.  */
>      /* Lookup l1 descriptor.  */
> -    if (!get_level1_table_address(env, &table, address)) {
> +    if (!get_level1_table_address(env, mmu_idx, &table, address)) {
>          /* Section translation fault if page walk is disabled by PD0 or
> PD1 */
>          code = 5;
>          goto do_fault;
> @@ -4775,7 +4854,12 @@ static int get_phys_addr_v6(CPUARMState *env,
> uint32_t address, int access_type,
>          /* Page or Section.  */
>          domain = (desc >> 5) & 0x0f;
>      }
> -    domain_prot = (A32_BANKED_CURRENT_REG_GET(env, dacr) >> (domain * 2))
> & 3;
> +    if (regime_el(env, mmu_idx) == 1) {
> +        dacr = env->cp15.dacr_ns;
> +    } else {
> +        dacr = env->cp15.dacr_s;
> +    }
> +    domain_prot = (dacr >> (domain * 2)) & 3;
>      if (domain_prot == 0 || domain_prot == 2) {
>          if (type != 1) {
>              code = 9; /* Section domain fault.  */
> @@ -4829,20 +4913,20 @@ static int get_phys_addr_v6(CPUARMState *env,
> uint32_t address, int access_type,
>      if (domain_prot == 3) {
>          *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
>      } else {
> -        if (pxn && !is_user) {
> +        if (pxn && !regime_is_user(env, mmu_idx)) {
>              xn = 1;
>          }
>          if (xn && access_type == 2)
>              goto do_fault;
>
>          /* The simplified model uses AP[0] as an access control bit.  */
> -        if ((A32_BANKED_CURRENT_REG_GET(env, sctlr) & SCTLR_AFE)
> +        if ((regime_sctlr(env, mmu_idx) & SCTLR_AFE)
>                  && (ap & 1) == 0) {
>              /* Access flag fault.  */
>              code = (code == 15) ? 6 : 3;
>              goto do_fault;
>          }
> -        *prot = check_ap(env, ap, domain_prot, access_type, is_user);
> +        *prot = check_ap(env, mmu_idx, ap, domain_prot, access_type);
>          if (!*prot) {
>              /* Access permission fault.  */
>              goto do_fault;
> @@ -4867,7 +4951,7 @@ typedef enum {
>  } MMUFaultType;
>
>  static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
> -                              int access_type, int is_user,
> +                              int access_type, ARMMMUIdx mmu_idx,
>                                hwaddr *phys_ptr, int *prot,
>                                target_ulong *page_size_ptr)
>  {
> @@ -4887,9 +4971,17 @@ static int get_phys_addr_lpae(CPUARMState *env,
> target_ulong address,
>      int32_t granule_sz = 9;
>      int32_t va_size = 32;
>      int32_t tbi = 0;
> -    TCR *tcr = &env->cp15.tcr_el[arm_is_secure(env) ? 3 : 1];
> -
> -    if (arm_el_is_aa64(env, 1)) {
> +    bool is_user;
> +    TCR *tcr = regime_tcr(env, mmu_idx);
> +
> +    /* TODO:
> +     * This code assumes we're either a 64-bit EL1 or a 32-bit PL1;
> +     * it doesn't handle the different format TCR for TCR_EL2, TCR_EL3,
> +     * and VTCR_EL2, or the fact that those regimes don't have a split
> +     * TTBR0/TTBR1. Attribute and permission bit handling should also
> +     * be checked when adding support for those page table walks.
> +     */
>

​Maybe copy this comment up above​ in get_level1_table_address().


> +    if (arm_el_is_aa64(env, regime_el(env, mmu_idx))) {
>          va_size = 64;
>          if (extract64(address, 55, 1))
>              tbi = extract64(tcr->raw_tcr, 38, 1);
> @@ -4904,12 +4996,12 @@ static int get_phys_addr_lpae(CPUARMState *env,
> target_ulong address,
>       * TTBCR/TTBR0/TTBR1 in accordance with ARM ARM DDI0406C table B-32:
>       */
>      uint32_t t0sz = extract32(tcr->raw_tcr, 0, 6);
> -    if (arm_el_is_aa64(env, 1)) {
> +    if (va_size == 64) {
>          t0sz = MIN(t0sz, 39);
>          t0sz = MAX(t0sz, 16);
>      }
>      uint32_t t1sz = extract32(tcr->raw_tcr, 16, 6);
> -    if (arm_el_is_aa64(env, 1)) {
> +    if (va_size == 64) {
>          t1sz = MIN(t1sz, 39);
>          t1sz = MAX(t1sz, 16);
>      }
> @@ -4964,6 +5056,10 @@ static int 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
> +     */
> +
>      if (epd) {
>          /* Translation table walk disabled => Translation fault on TLB
> miss */
>          goto do_fault;
> @@ -5049,6 +5145,7 @@ static int get_phys_addr_lpae(CPUARMState *env,
> target_ulong address,
>          goto do_fault;
>      }
>      fault_type = permission_fault;
> +    is_user = regime_is_user(env, mmu_idx);
>      if (is_user && !(attrs & (1 << 4))) {
>          /* Unprivileged access not enabled */
>          goto do_fault;
> @@ -5083,12 +5180,13 @@ do_fault:
>  }
>
>  static int get_phys_addr_mpu(CPUARMState *env, uint32_t address,
> -                             int access_type, int is_user,
> +                             int access_type, ARMMMUIdx mmu_idx,
>                               hwaddr *phys_ptr, int *prot)
>  {
>      int n;
>      uint32_t mask;
>      uint32_t base;
> +    bool is_user = regime_is_user(env, mmu_idx);
>
>      *phys_ptr = address;
>      for (n = 7; n >= 0; n--) {
> @@ -5171,39 +5269,43 @@ static inline int get_phys_addr(CPUARMState *env,
> target_ulong address,
>                                  hwaddr *phys_ptr, int *prot,
>                                  target_ulong *page_size)
>  {
> -    /* This is not entirely correct as get_phys_addr() can also be called
> -     * from ats_write() for an address translation of a specific regime.
> -     */
> -    uint32_t sctlr = A32_BANKED_CURRENT_REG_GET(env, sctlr);
> -
> -    /* This will go away when we handle mmu_idx properly here */
> -    int is_user = (mmu_idx == ARMMMUIdx_S12NSE0 ||
> -                   mmu_idx == ARMMMUIdx_S1SE0 ||
> -                   mmu_idx == ARMMMUIdx_S1NSE0);
> +    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 ldl_phys
> +         * calls for stage 1 will also need changing.
> +         * For non-EL2 CPUs a stage1+stage2 translation is just stage 1.
> +         */
> +        assert(!arm_feature(env, ARM_FEATURE_EL2));
> +        mmu_idx += ARMMMUIdx_S1NSE0;
> +    }
>
>      /* Fast Context Switch Extension.  */
> -    if (address < 0x02000000) {
> +    if (address < 0x02000000 && mmu_idx != ARMMMUIdx_S2NS) {
>          address += A32_BANKED_CURRENT_REG_GET(env, fcseidr);
>

​Now that the MMU index includes security state info we use in in certain
circumstances to determine the security state.  However, we don't seem to
consistently use it.  For example, earlier changes used the mmu_index to
choose certain register banks but here we still rely on the BANKED macro. I
see this inconsistency being prone to errors.  Maybe we have just not
gotten to change all of the cases over, but I thought I'd highlight it.


>      }
>
> -    if ((sctlr & SCTLR_M) == 0) {
> +    if (regime_translation_disabled(env, mmu_idx)) {
>          /* MMU/MPU disabled.  */
>          *phys_ptr = address;
>          *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
>          *page_size = TARGET_PAGE_SIZE;
>          return 0;
> -    } else if (arm_feature(env, ARM_FEATURE_MPU)) {
> +    }
> +
> +    if (arm_feature(env, ARM_FEATURE_MPU)) {
>          *page_size = TARGET_PAGE_SIZE;
> -       return get_phys_addr_mpu(env, address, access_type, is_user,
> phys_ptr,
> -                                prot);
> -    } else if (extended_addresses_enabled(env)) {
> -        return get_phys_addr_lpae(env, address, access_type, is_user,
> phys_ptr,
> +        return get_phys_addr_mpu(env, address, access_type, mmu_idx,
> phys_ptr,
> +                                 prot);
> +    }
> +
> +    if (regime_using_lpae_format(env, mmu_idx)) {
> +        return get_phys_addr_lpae(env, address, access_type, mmu_idx,
> phys_ptr,
>                                    prot, page_size);
> -    } else if (sctlr & SCTLR_XP) {
> -        return get_phys_addr_v6(env, address, access_type, is_user,
> phys_ptr,
> +    } else if (regime_sctlr(env, mmu_idx) & SCTLR_XP) {
> +        return get_phys_addr_v6(env, address, access_type, mmu_idx,
> phys_ptr,
>                                  prot, page_size);
>      } else {
> -        return get_phys_addr_v5(env, address, access_type, is_user,
> phys_ptr,
> +        return get_phys_addr_v5(env, address, access_type, mmu_idx,
> phys_ptr,
>                                  prot, page_size);
>      }
>  }
> --
> 1.9.1
>
>

[-- Attachment #2: Type: text/html, Size: 23242 bytes --]

Re: [Qemu-devel] [PATCH 04/11] target-arm: Define correct mmu_idx values and pass them in TB flags

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 14317 bytes --]

On Fri, Jan 23, 2015 at 12:20 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> We currently claim that for ARM the mmu_idx should simply be the current
> exception level. However this isn't actually correct -- secure EL0 and EL1
> should have separate indexes from non-secure EL0 and EL1 since their
> VA->PA mappings may differ. We also will want an index for stage 2
> translations when we properly support EL2.
>
> Define and document all seven mmu index values that we require, and
> pass the mmu index in the TB flags rather than exception level or
> priv/user bit.
>
> This change doesn't update the get_phys_addr() code, so our page
> table walking still assumes a simplistic "user or priv?" model for
> the moment.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> This leaves some odd gaps in the TB flags usage. I will circle
> back and clean this up later (including moving the other common
> flags like the singlestep ones to the top of the flags word),
> but I didn't want to bloat this patchseries further.
> ---
>  target-arm/cpu.h           | 113
> ++++++++++++++++++++++++++++++++++++---------
>  target-arm/helper.c        |   3 +-
>  target-arm/translate-a64.c |   5 +-
>  target-arm/translate.c     |   5 +-
>  target-arm/translate.h     |   3 +-
>  5 files changed, 101 insertions(+), 28 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 3eb00f4..cf7b9ab 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -98,7 +98,7 @@ typedef uint32_t ARMReadCPFunc(void *opaque, int cp_info,
>
>  struct arm_boot_info;
>
> -#define NB_MMU_MODES 4
> +#define NB_MMU_MODES 7
>
>  /* We currently assume float and double are IEEE single and double
>     precision respectively.
> @@ -1572,13 +1572,92 @@ static inline CPUARMState *cpu_init(const char
> *cpu_model)
>  #define cpu_signal_handler cpu_arm_signal_handler
>  #define cpu_list arm_cpu_list
>
> -/* MMU modes definitions */
> +/* ARM has the following "translation regimes" (as the ARM ARM calls
> them):
> + *
> + * If EL3 is 64-bit:
> + *  + NonSecure EL1 & 0 stage 1
> + *  + NonSecure EL1 & 0 stage 2
> + *  + NonSecure EL2
> + *  + Secure EL1 & EL0
> + *  + Secure EL3
> + * If EL3 is 32-bit:
> + *  + NonSecure PL1 & 0 stage 1
> + *  + NonSecure PL1 & 0 stage 2
> + *  + NonSecure PL2
> + *  + Secure PL0 & PL1
> + * (reminder: for 32 bit EL3, Secure PL1 is *EL3*, not EL1.)
> + *
> + * For QEMU, an mmu_idx is not quite the same as a translation regime
> because:
> + *  1. we need to split the "EL1 & 0" regimes into two mmu_idxes, because
> they
> + *     may differ in access permissions even if the VA->PA map is the same
> + *  2. we want to cache in our TLB the full VA->IPA->PA lookup for a
> stage 1+2
> + *     translation, which means that we have one mmu_idx that deals with
> two
> + *     concatenated translation regimes [this sort of combined s1+2 TLB is
> + *     architecturally permitted]
> + *  3. we don't need to allocate an mmu_idx to translations that we won't
> be
> + *     handling via the TLB. The only way to do a stage 1 translation
> without
> + *     the immediate stage 2 translation is via the ATS or AT system
> insns,
> + *     which can be slow-pathed and always do a page table walk.
> + *  4. we can also safely fold together the "32 bit EL3" and "64 bit EL3"
> + *     translation regimes, because they map reasonably well to each other
> + *     and they can't both be active at the same time.
> + * This gives us the following list of mmu_idx values:
> + *
> + * NS EL0 (aka NS PL0) stage 1+2
> + * NS EL1 (aka NS PL1) stage 1+2
> + * NS EL2 (aka NS PL2)
> + * S EL3 (aka S PL1)
> + * S EL0 (aka S PL0)
> + * S EL1 (not used if EL3 is 32 bit)
> + * NS EL0+1 stage 2
> + *
> + * (The last of these is an mmu_idx because we want to be able to use the
> TLB
> + * for the accesses done as part of a stage 1 page table walk, rather than
> + * having to walk the stage 2 page table over and over.)
> + *
> + * Our enumeration includes at the end some entries which are not "true"
> + * mmu_idx values in that they don't have corresponding TLBs and are only
> + * valid for doing slow path page table walks.
> + *
> + * The constant names here are patterned after the general style of the
> names
> + * of the AT/ATS operations.
> + * The values used are carefully arranged to make mmu_idx => EL lookup
> easy.
> + */
> +typedef enum ARMMMUIdx {
> +    ARMMMUIdx_S12NSE0 = 0,
> +    ARMMMUIdx_S12NSE1 = 1,
> +    ARMMMUIdx_S1E2 = 2,
> +    ARMMMUIdx_S1E3 = 3,
> +    ARMMMUIdx_S1SE0 = 4,
> +    ARMMMUIdx_S1SE1 = 5,
> +    ARMMMUIdx_S2NS = 6,
> +    /* Indexes below here don't have TLBs and are used only for AT system
> +     * instructions or for the first stage of an S12 page table walk.
> +     */
> +    ARMMMUIdx_S1NSE0 = 7,
> +    ARMMMUIdx_S1NSE1 = 8,
> +} ARMMMUIdx;
> +
>  #define MMU_MODE0_SUFFIX _user
>  #define MMU_MODE1_SUFFIX _kernel
>  #define MMU_USER_IDX 0
> +
> +/* Return the exception level we're running at if this is our mmu_idx */
> +static inline int arm_mmu_idx_to_el(ARMMMUIdx mmu_idx)
> +{
> +    assert(mmu_idx < ARMMMUIdx_S2NS);
> +    return mmu_idx & 3;
> +}
> +
> +/* Determine the current mmu_idx to use for normal loads/stores */
>  static inline int cpu_mmu_index (CPUARMState *env)
>  {
> -    return arm_current_el(env);
> +    int el = arm_current_el(env);
> +
> +    if (el < 3 && arm_is_secure_below_el3(env)) {
> +        return ARMMMUIdx_S1SE0 + el;
> +    }
> +    return el;
>  }
>
>  /* Return the Exception Level targeted by debug exceptions;
> @@ -1645,9 +1724,13 @@ static inline bool
> arm_singlestep_active(CPUARMState *env)
>
>  /* Bit usage in the TB flags field: bit 31 indicates whether we are
>   * in 32 or 64 bit mode. The meaning of the other bits depends on that.
> + * We put flags which are shared between 32 and 64 bit mode at the top
> + * of the word, and flags which apply to only one mode at the bottom.
>   */
>  #define ARM_TBFLAG_AARCH64_STATE_SHIFT 31
>  #define ARM_TBFLAG_AARCH64_STATE_MASK  (1U <<
> ARM_TBFLAG_AARCH64_STATE_SHIFT)
> +#define ARM_TBFLAG_MMUIDX_SHIFT 28
> +#define ARM_TBFLAG_MMUIDX_MASK (0x7 << ARM_TBFLAG_MMUIDX_SHIFT)
>
>  /* Bit usage when in AArch32 state: */
>  #define ARM_TBFLAG_THUMB_SHIFT      0
> @@ -1656,8 +1739,6 @@ static inline bool arm_singlestep_active(CPUARMState
> *env)
>  #define ARM_TBFLAG_VECLEN_MASK      (0x7 << ARM_TBFLAG_VECLEN_SHIFT)
>  #define ARM_TBFLAG_VECSTRIDE_SHIFT  4
>  #define ARM_TBFLAG_VECSTRIDE_MASK   (0x3 << ARM_TBFLAG_VECSTRIDE_SHIFT)
> -#define ARM_TBFLAG_PRIV_SHIFT       6
> -#define ARM_TBFLAG_PRIV_MASK        (1 << ARM_TBFLAG_PRIV_SHIFT)
>  #define ARM_TBFLAG_VFPEN_SHIFT      7
>  #define ARM_TBFLAG_VFPEN_MASK       (1 << ARM_TBFLAG_VFPEN_SHIFT)
>  #define ARM_TBFLAG_CONDEXEC_SHIFT   8
> @@ -1683,8 +1764,6 @@ static inline bool arm_singlestep_active(CPUARMState
> *env)
>  #define ARM_TBFLAG_NS_MASK          (1 << ARM_TBFLAG_NS_SHIFT)
>
>  /* Bit usage when in AArch64 state */
> -#define ARM_TBFLAG_AA64_EL_SHIFT    0
> -#define ARM_TBFLAG_AA64_EL_MASK     (0x3 << ARM_TBFLAG_AA64_EL_SHIFT)
>  #define ARM_TBFLAG_AA64_FPEN_SHIFT  2
>  #define ARM_TBFLAG_AA64_FPEN_MASK   (1 << ARM_TBFLAG_AA64_FPEN_SHIFT)
>  #define ARM_TBFLAG_AA64_SS_ACTIVE_SHIFT 3
> @@ -1695,14 +1774,14 @@ static inline bool
> arm_singlestep_active(CPUARMState *env)
>  /* some convenience accessor macros */
>  #define ARM_TBFLAG_AARCH64_STATE(F) \
>      (((F) & ARM_TBFLAG_AARCH64_STATE_MASK) >>
> ARM_TBFLAG_AARCH64_STATE_SHIFT)
> +#define ARM_TBFLAG_MMUIDX(F) \
> +    (((F) & ARM_TBFLAG_MMUIDX_MASK) >> ARM_TBFLAG_MMUIDX_SHIFT)
>  #define ARM_TBFLAG_THUMB(F) \
>      (((F) & ARM_TBFLAG_THUMB_MASK) >> ARM_TBFLAG_THUMB_SHIFT)
>  #define ARM_TBFLAG_VECLEN(F) \
>      (((F) & ARM_TBFLAG_VECLEN_MASK) >> ARM_TBFLAG_VECLEN_SHIFT)
>  #define ARM_TBFLAG_VECSTRIDE(F) \
>      (((F) & ARM_TBFLAG_VECSTRIDE_MASK) >> ARM_TBFLAG_VECSTRIDE_SHIFT)
> -#define ARM_TBFLAG_PRIV(F) \
> -    (((F) & ARM_TBFLAG_PRIV_MASK) >> ARM_TBFLAG_PRIV_SHIFT)
>  #define ARM_TBFLAG_VFPEN(F) \
>      (((F) & ARM_TBFLAG_VFPEN_MASK) >> ARM_TBFLAG_VFPEN_SHIFT)
>  #define ARM_TBFLAG_CONDEXEC(F) \
> @@ -1717,8 +1796,6 @@ static inline bool arm_singlestep_active(CPUARMState
> *env)
>      (((F) & ARM_TBFLAG_PSTATE_SS_MASK) >> ARM_TBFLAG_PSTATE_SS_SHIFT)
>  #define ARM_TBFLAG_XSCALE_CPAR(F) \
>      (((F) & ARM_TBFLAG_XSCALE_CPAR_MASK) >> ARM_TBFLAG_XSCALE_CPAR_SHIFT)
> -#define ARM_TBFLAG_AA64_EL(F) \
> -    (((F) & ARM_TBFLAG_AA64_EL_MASK) >> ARM_TBFLAG_AA64_EL_SHIFT)
>  #define ARM_TBFLAG_AA64_FPEN(F) \
>      (((F) & ARM_TBFLAG_AA64_FPEN_MASK) >> ARM_TBFLAG_AA64_FPEN_SHIFT)
>  #define ARM_TBFLAG_AA64_SS_ACTIVE(F) \
> @@ -1742,8 +1819,7 @@ static inline void cpu_get_tb_cpu_state(CPUARMState
> *env, target_ulong *pc,
>
>      if (is_a64(env)) {
>          *pc = env->pc;
> -        *flags = ARM_TBFLAG_AARCH64_STATE_MASK
> -            | (arm_current_el(env) << ARM_TBFLAG_AA64_EL_SHIFT);
> +        *flags = ARM_TBFLAG_AARCH64_STATE_MASK;
>          if (fpen == 3 || (fpen == 1 && arm_current_el(env) != 0)) {
>              *flags |= ARM_TBFLAG_AA64_FPEN_MASK;
>          }
> @@ -1761,21 +1837,12 @@ static inline void
> cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
>              }
>          }
>      } else {
> -        int privmode;
>          *pc = env->regs[15];
>          *flags = (env->thumb << ARM_TBFLAG_THUMB_SHIFT)
>              | (env->vfp.vec_len << ARM_TBFLAG_VECLEN_SHIFT)
>              | (env->vfp.vec_stride << ARM_TBFLAG_VECSTRIDE_SHIFT)
>              | (env->condexec_bits << ARM_TBFLAG_CONDEXEC_SHIFT)
>              | (env->bswap_code << ARM_TBFLAG_BSWAP_CODE_SHIFT);
> -        if (arm_feature(env, ARM_FEATURE_M)) {
> -            privmode = !((env->v7m.exception == 0) && (env->v7m.control &
> 1));
> -        } else {
> -            privmode = (env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_USR;
> -        }
> -        if (privmode) {
> -            *flags |= ARM_TBFLAG_PRIV_MASK;
> -        }
>          if (!(access_secure_reg(env))) {
>              *flags |= ARM_TBFLAG_NS_MASK;
>          }
> @@ -1803,6 +1870,8 @@ static inline void cpu_get_tb_cpu_state(CPUARMState
> *env, target_ulong *pc,
>                     << ARM_TBFLAG_XSCALE_CPAR_SHIFT);
>      }
>
> +    *flags |= (cpu_mmu_index(env) << ARM_TBFLAG_MMUIDX_SHIFT);
> +
>

​After getting through patch 9, I wonder if the TB NS bit can also be
removed as it is implied in the MMU index.​


>      *cs_base = 0;
>  }
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 1a5e067..06478d8 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -5119,7 +5119,8 @@ int arm_cpu_handle_mmu_fault(CPUState *cs, vaddr
> address,
>      uint32_t syn;
>      bool same_el = (arm_current_el(env) != 0);
>
> -    is_user = mmu_idx == MMU_USER_IDX;
> +    /* TODO: pass the translation regime to get_phys_addr */
> +    is_user = (arm_mmu_idx_to_el(mmu_idx) == 0);
>      ret = get_phys_addr(env, address, access_type, is_user, &phys_addr,
> &prot,
>                          &page_size);
>      if (ret == 0) {
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index dac2f63..96f14ff 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -10922,14 +10922,15 @@ void gen_intermediate_code_internal_a64(ARMCPU
> *cpu,
>      dc->bswap_code = 0;
>      dc->condexec_mask = 0;
>      dc->condexec_cond = 0;
> +    dc->mmu_idx = ARM_TBFLAG_MMUIDX(tb->flags);
> +    dc->current_el = arm_mmu_idx_to_el(dc->mmu_idx);
>  #if !defined(CONFIG_USER_ONLY)
> -    dc->user = (ARM_TBFLAG_AA64_EL(tb->flags) == 0);
> +    dc->user = (dc->current_el == 0);
>  #endif
>      dc->cpacr_fpen = ARM_TBFLAG_AA64_FPEN(tb->flags);
>      dc->vec_len = 0;
>      dc->vec_stride = 0;
>      dc->cp_regs = cpu->cp_regs;
> -    dc->current_el = arm_current_el(env);
>      dc->features = env->features;
>
>      /* Single step state. The code-generation logic here is:
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index bdfcdf1..7163649 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -11032,8 +11032,10 @@ static inline void
> gen_intermediate_code_internal(ARMCPU *cpu,
>      dc->bswap_code = ARM_TBFLAG_BSWAP_CODE(tb->flags);
>      dc->condexec_mask = (ARM_TBFLAG_CONDEXEC(tb->flags) & 0xf) << 1;
>      dc->condexec_cond = ARM_TBFLAG_CONDEXEC(tb->flags) >> 4;
> +    dc->mmu_idx = ARM_TBFLAG_MMUIDX(tb->flags);
> +    dc->current_el = arm_mmu_idx_to_el(dc->mmu_idx);
>  #if !defined(CONFIG_USER_ONLY)
> -    dc->user = (ARM_TBFLAG_PRIV(tb->flags) == 0);
> +    dc->user = (dc->current_el == 0);
>  #endif
>      dc->ns = ARM_TBFLAG_NS(tb->flags);
>      dc->cpacr_fpen = ARM_TBFLAG_CPACR_FPEN(tb->flags);
> @@ -11042,7 +11044,6 @@ static inline void
> gen_intermediate_code_internal(ARMCPU *cpu,
>      dc->vec_stride = ARM_TBFLAG_VECSTRIDE(tb->flags);
>      dc->c15_cpar = ARM_TBFLAG_XSCALE_CPAR(tb->flags);
>      dc->cp_regs = cpu->cp_regs;
> -    dc->current_el = arm_current_el(env);
>      dc->features = env->features;
>
>      /* Single step state. The code-generation logic here is:
> diff --git a/target-arm/translate.h b/target-arm/translate.h
> index f6ee789..a1eb5b5 100644
> --- a/target-arm/translate.h
> +++ b/target-arm/translate.h
> @@ -20,6 +20,7 @@ typedef struct DisasContext {
>  #if !defined(CONFIG_USER_ONLY)
>      int user;
>  #endif
> +    ARMMMUIdx mmu_idx; /* MMU index to use for normal loads/stores */
>      bool ns;        /* Use non-secure CPREG bank on access */
>      bool cpacr_fpen; /* FP enabled via CPACR.FPEN */
>      bool vfp_enabled; /* FP enabled via FPSCR.EN */
> @@ -69,7 +70,7 @@ static inline int arm_dc_feature(DisasContext *dc, int
> feature)
>
>  static inline int get_mem_index(DisasContext *s)
>  {
> -    return s->current_el;
> +    return s->mmu_idx;
>  }
>
>  /* target-specific extra values for is_jmp */
> --
> 1.9.1
>
>

[-- Attachment #2: Type: text/html, Size: 16729 bytes --]

Re: [Qemu-devel] [PATCH 09/11] target-arm: Use mmu_idx in get_phys_addr()

[Peter Maydell]

On 28 January 2015 at 21:37, Greg Bellows <greg.bellows@linaro.org> wrote:
>
>> +/* Return true if the translation regime is using LPAE format page tables
>> */
>> +static inline bool regime_using_lpae_format(CPUARMState *env,
>> +                                            ARMMMUIdx mmu_idx)
>> +{
>> +    int el = regime_el(env, mmu_idx);
>> +    if (el == 2 || arm_el_is_aa64(env, el)) {
>
>
> For the life of me, I can not figure out why EL2 is wired to always use
> LPAE.   Is this stated in the spec somewhere?  I found places where EL2
> registers can vary depending on TTBCR.EAE bit settings which implies it is
> not always true.

The only translation regimes controlled by EL2 are:
 (1) EL2's own stage 1 translations
 (2) the stage 2 translations

These must both be LPAE format: see the v8 ARM ARM section G4.4:
"the translation tables for the Non-secure PL2 stage 1 translations,
and for the Non-secure PL1&0 stage 2 translations, must use the
Long-descriptor translation table format." v7 ARM ARM B3.3 has
similar text.

(Basically, short-descriptors are obsolete and are only supported in
the pre-Virtualization translation regimes, ie AArch32 EL1/3.)

> I realize EL2 is not supported yet, but wouldn't this break AArch32 if it
> were and the LPAE feature was not enabled?

Implementations with Virtualization must include LPAE.

>> -static bool get_level1_table_address(CPUARMState *env, uint32_t *table,
>> -                                         uint32_t address)
>> +static bool get_level1_table_address(CPUARMState *env, ARMMMUIdx mmu_idx,
>> +                                     uint32_t *table, uint32_t address)
>>  {
>> -    /* Get the TCR bank based on our security state */
>> -    TCR *tcr = &env->cp15.tcr_el[arm_is_secure(env) ? 3 : 1];
>> +    /* Note that we can only get here for an AArch32 PL0/PL1 lookup */
>> +    int el = regime_el(env, mmu_idx);
>> +    TCR *tcr = regime_tcr(env, mmu_idx);
>>
>> -    /* We only get here if EL1 is running in AArch32. If EL3 is running
>> in
>> -     * AArch32 there is a secure and non-secure instance of the
>> translation
>> -     * table registers.
>> -     */
>>      if (address & tcr->mask) {
>>          if (tcr->raw_tcr & TTBCR_PD1) {
>>              /* Translation table walk disabled for TTBR1 */
>>              return false;
>>          }
>> -        *table = A32_BANKED_CURRENT_REG_GET(env, ttbr1) & 0xffffc000;
>> +        *table = env->cp15.ttbr1_el[el] & 0xffffc000;
>
>
> Perhaps you plan to address this in a separate patch, but I believe TTBR1 is
> only applicable to EL1 and EL0 in AArch64.

It's true that TTBR1 is only for EL0/EL1, but see the comment at the
start of the function -- we can't get here except for EL0 and EL1,
because this function is only used for some kinds of short-descriptor
tables.

>> @@ -4663,7 +4736,12 @@ static int get_phys_addr_v5(CPUARMState *env,
>> uint32_t address, int access_type,
>>      desc = ldl_phys(cs->as, table);
>>      type = (desc & 3);
>>      domain = (desc >> 5) & 0x0f;
>> -    domain_prot = (A32_BANKED_CURRENT_REG_GET(env, dacr) >> (domain * 2))
>> & 3;
>> +    if (regime_el(env, mmu_idx) == 1) {
>> +        dacr = env->cp15.dacr_ns;
>> +    } else {
>> +        dacr = env->cp15.dacr_s;
>> +    }
>> +    domain_prot = (dacr >> (domain * 2)) & 3;
>
>
> Is there a reason that you did not add a regime_dacr() here like you did for
> SCTLR and TCR?

Didn't seem necessary, since we know we only need to deal with S vs NS,
and the concept isn't generally applicable to most regimes. If the
dacr in env->cp15 was stored as dacr[4] I'd have used dacr[el] as
I do above for the ttbr1_el[], but it isn't, hence the conditional.

The TCR and SCTLR are used in LPAE format page tables so they apply
for the whole set of translation regimes.

> Also, the ARMv8 ARM makes it sound like DACR has no value in AArch64.

Well, the DACR is only relevant to short-descriptor format page tables,
so it's only consulted for AArch32 translations, and there's no
equivalent register in AArch64. (There is a DACR32_EL2 64 bit register,
but that is only there so a hypervisor can save and restore the state
of a 32 bit VM (at EL1) that is using short-descriptor page tables.)

> However, if it did have meaning in AArch64, then for S1SE1 would we be
> accessing the wrong bank as regime_el returns 1?  This working off the
> understanding that an address reference from an instruction executed in
> S/EL1 and AArch64 would generate such an index.

We can only get here for regime S1SE1 if:
 * EL3 is AArch64
 * EL1 is AArch32

Since EL3 is 64 bit, there is no banking of registers and regardless
of whether EL1 is Secure or NonSecure we want the one and only
register (which is in dacr_ns). (Compare the way we use
ttbr1_el[regime_el(env, mmu_idx)] and so get TTBR1_EL1 whether
this is Secure EL1 or NonSecure EL1.)

If EL3 is 32 bit then there is banking of registers, but it's
not possible to get here for S1SE1 in that case (only for S EL3
and NS EL1).

>> +    /* TODO:
>> +     * This code assumes we're either a 64-bit EL1 or a 32-bit PL1;
>> +     * it doesn't handle the different format TCR for TCR_EL2, TCR_EL3,
>> +     * and VTCR_EL2, or the fact that those regimes don't have a split
>> +     * TTBR0/TTBR1. Attribute and permission bit handling should also
>> +     * be checked when adding support for those page table walks.
>> +     */
>
>
> Maybe copy this comment up above in get_level1_table_address().

This is the correct location; see remarks above.

>> @@ -5171,39 +5269,43 @@ static inline int get_phys_addr(CPUARMState *env,
>> target_ulong address,
>>                                  hwaddr *phys_ptr, int *prot,
>>                                  target_ulong *page_size)
>>  {
>> -    /* This is not entirely correct as get_phys_addr() can also be called
>> -     * from ats_write() for an address translation of a specific regime.
>> -     */
>> -    uint32_t sctlr = A32_BANKED_CURRENT_REG_GET(env, sctlr);
>> -
>> -    /* This will go away when we handle mmu_idx properly here */
>> -    int is_user = (mmu_idx == ARMMMUIdx_S12NSE0 ||
>> -                   mmu_idx == ARMMMUIdx_S1SE0 ||
>> -                   mmu_idx == ARMMMUIdx_S1NSE0);
>> +    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 ldl_phys
>> +         * calls for stage 1 will also need changing.
>> +         * For non-EL2 CPUs a stage1+stage2 translation is just stage 1.
>> +         */
>> +        assert(!arm_feature(env, ARM_FEATURE_EL2));
>> +        mmu_idx += ARMMMUIdx_S1NSE0;
>> +    }
>>
>>      /* Fast Context Switch Extension.  */
>> -    if (address < 0x02000000) {
>> +    if (address < 0x02000000 && mmu_idx != ARMMMUIdx_S2NS) {
>>          address += A32_BANKED_CURRENT_REG_GET(env, fcseidr);
>
>
> Now that the MMU index includes security state info we use in in certain
> circumstances to determine the security state.  However, we don't seem to
> consistently use it.  For example, earlier changes used the mmu_index to
> choose certain register banks but here we still rely on the BANKED macro. I
> see this inconsistency being prone to errors.  Maybe we have just not gotten
> to change all of the cases over, but I thought I'd highlight it.

Yes, you're right: if the Secure world does an NS ATS operation
then we should be using the NS copy of the register. I think
I mentally skipped over this requirement because the whole bit
of code is irrelevant for v8 CPUs (where FSCEIDR is mandatorily
RAZ/WI and so it doesn't matter which one we use).

It would also I think be safer to explicitly guard this with
a not-if-v8 check, because we don't actually implement that
RAZ/WI behaviour. So something like:

  if (address < 0x02000000 && mmu_idx != ARMMMUIdx_S2NS
      && !arm_feature(env, ARM_FEATURE_V8)) {
      uint32_t fcseidr;
      if (regime_el(env, mmu_idx) == 3) {
          fcseidr = env->cp15.fcseidr_s;
      } else {
          fcseidr = env->cp15.fcseidr_ns;
      }
      address += fcseidr;
  }

(note that stage 1 PL2 lookups need to use the NS FCSEIDR.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 04/11] target-arm: Define correct mmu_idx values and pass them in TB flags

[Peter Maydell]

On 28 January 2015 at 21:57, Greg Bellows <greg.bellows@linaro.org> wrote:
> After getting through patch 9, I wonder if the TB NS bit can also be removed
> as it is implied in the MMU index.

No, because for a 32-bit EL3 we are always running under a Secure
translation regime (S1E3) but the TBFLAG_NS bit may be either 0 or
1 depending on the value of the SCR.NS bit.

-- PMM

Re: [Qemu-devel] [PATCH 09/11] target-arm: Use mmu_idx in get_phys_addr()

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 9453 bytes --]

On Wed, Jan 28, 2015 at 4:30 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 28 January 2015 at 21:37, Greg Bellows <greg.bellows@linaro.org> wrote:
> >
> >> +/* Return true if the translation regime is using LPAE format page
> tables
> >> */
> >> +static inline bool regime_using_lpae_format(CPUARMState *env,
> >> +                                            ARMMMUIdx mmu_idx)
> >> +{
> >> +    int el = regime_el(env, mmu_idx);
> >> +    if (el == 2 || arm_el_is_aa64(env, el)) {
> >
> >
> > For the life of me, I can not figure out why EL2 is wired to always use
> > LPAE.   Is this stated in the spec somewhere?  I found places where EL2
> > registers can vary depending on TTBCR.EAE bit settings which implies it
> is
> > not always true.
>
> The only translation regimes controlled by EL2 are:
>  (1) EL2's own stage 1 translations
>  (2) the stage 2 translations
>
> These must both be LPAE format: see the v8 ARM ARM section G4.4:
> "the translation tables for the Non-secure PL2 stage 1 translations,
> and for the Non-secure PL1&0 stage 2 translations, must use the
> Long-descriptor translation table format." v7 ARM ARM B3.3 has
> similar text.
>

​I see that now, thanks for pointing this out​


>
> (Basically, short-descriptors are obsolete and are only supported in
> the pre-Virtualization translation regimes, ie AArch32 EL1/3.)
>
> > I realize EL2 is not supported yet, but wouldn't this break AArch32 if it
> > were and the LPAE feature was not enabled?
>
> Implementations with Virtualization must include LPAE.
>
> >> -static bool get_level1_table_address(CPUARMState *env, uint32_t *table,
> >> -                                         uint32_t address)
> >> +static bool get_level1_table_address(CPUARMState *env, ARMMMUIdx
> mmu_idx,
> >> +                                     uint32_t *table, uint32_t address)
> >>  {
> >> -    /* Get the TCR bank based on our security state */
> >> -    TCR *tcr = &env->cp15.tcr_el[arm_is_secure(env) ? 3 : 1];
> >> +    /* Note that we can only get here for an AArch32 PL0/PL1 lookup */
> >> +    int el = regime_el(env, mmu_idx);
> >> +    TCR *tcr = regime_tcr(env, mmu_idx);
> >>
> >> -    /* We only get here if EL1 is running in AArch32. If EL3 is running
> >> in
> >> -     * AArch32 there is a secure and non-secure instance of the
> >> translation
> >> -     * table registers.
> >> -     */
> >>      if (address & tcr->mask) {
> >>          if (tcr->raw_tcr & TTBCR_PD1) {
> >>              /* Translation table walk disabled for TTBR1 */
> >>              return false;
> >>          }
> >> -        *table = A32_BANKED_CURRENT_REG_GET(env, ttbr1) & 0xffffc000;
> >> +        *table = env->cp15.ttbr1_el[el] & 0xffffc000;
> >
> >
> > Perhaps you plan to address this in a separate patch, but I believe
> TTBR1 is
> > only applicable to EL1 and EL0 in AArch64.
>
> It's true that TTBR1 is only for EL0/EL1, but see the comment at the
> start of the function -- we can't get here except for EL0 and EL1,
> because this function is only used for some kinds of short-descriptor
> tables.
>

​I saw that comment, but was not sure whether it was stale given we were
adding EL3.  I now see how AArch64 is routed away from this function.


>
> >> @@ -4663,7 +4736,12 @@ static int get_phys_addr_v5(CPUARMState *env,
> >> uint32_t address, int access_type,
> >>      desc = ldl_phys(cs->as, table);
> >>      type = (desc & 3);
> >>      domain = (desc >> 5) & 0x0f;
> >> -    domain_prot = (A32_BANKED_CURRENT_REG_GET(env, dacr) >> (domain *
> 2))
> >> & 3;
> >> +    if (regime_el(env, mmu_idx) == 1) {
> >> +        dacr = env->cp15.dacr_ns;
> >> +    } else {
> >> +        dacr = env->cp15.dacr_s;
> >> +    }
> >> +    domain_prot = (dacr >> (domain * 2)) & 3;
> >
> >
> > Is there a reason that you did not add a regime_dacr() here like you did
> for
> > SCTLR and TCR?
>
> Didn't seem necessary, since we know we only need to deal with S vs NS,
> and the concept isn't generally applicable to most regimes. If the
> dacr in env->cp15 was stored as dacr[4] I'd have used dacr[el] as
> I do above for the ttbr1_el[], but it isn't, hence the conditional.
>
> ​Fair enough, was more a question of consistency since you do the same
thing in both the v5 and v6 code.​


> The TCR and SCTLR are used in LPAE format page tables so they apply
> for the whole set of translation regimes.
>
> > Also, the ARMv8 ARM makes it sound like DACR has no value in AArch64.
>
> Well, the DACR is only relevant to short-descriptor format page tables,
> so it's only consulted for AArch32 translations, and there's no
> equivalent register in AArch64. (There is a DACR32_EL2 64 bit register,
> but that is only there so a hypervisor can save and restore the state
> of a 32 bit VM (at EL1) that is using short-descriptor page tables.)
>
> > However, if it did have meaning in AArch64, then for S1SE1 would we be
> > accessing the wrong bank as regime_el returns 1?  This working off the
> > understanding that an address reference from an instruction executed in
> > S/EL1 and AArch64 would generate such an index.
>

​So this comment is moot given my misunderstanding that we would not be in
this code if AArch64.
​


>
> We can only get here for regime S1SE1 if:
>  * EL3 is AArch64
>  * EL1 is AArch32
>
> Since EL3 is 64 bit, there is no banking of registers and regardless
> of whether EL1 is Secure or NonSecure we want the one and only
> register (which is in dacr_ns). (Compare the way we use
> ttbr1_el[regime_el(env, mmu_idx)] and so get TTBR1_EL1 whether
> this is Secure EL1 or NonSecure EL1.)
>
> If EL3 is 32 bit then there is banking of registers, but it's
> not possible to get here for S1SE1 in that case (only for S EL3
> and NS EL1).
>

​Right, that all makes sense. now.


>
> >> +    /* TODO:
> >> +     * This code assumes we're either a 64-bit EL1 or a 32-bit PL1;
> >> +     * it doesn't handle the different format TCR for TCR_EL2, TCR_EL3,
> >> +     * and VTCR_EL2, or the fact that those regimes don't have a split
> >> +     * TTBR0/TTBR1. Attribute and permission bit handling should also
> >> +     * be checked when adding support for those page table walks.
> >> +     */
> >
> >
> > Maybe copy this comment up above in get_level1_table_address().
>
> This is the correct location; see remarks above.
>
> >> @@ -5171,39 +5269,43 @@ static inline int get_phys_addr(CPUARMState
> *env,
> >> target_ulong address,
> >>                                  hwaddr *phys_ptr, int *prot,
> >>                                  target_ulong *page_size)
> >>  {
> >> -    /* This is not entirely correct as get_phys_addr() can also be
> called
> >> -     * from ats_write() for an address translation of a specific
> regime.
> >> -     */
> >> -    uint32_t sctlr = A32_BANKED_CURRENT_REG_GET(env, sctlr);
> >> -
> >> -    /* This will go away when we handle mmu_idx properly here */
> >> -    int is_user = (mmu_idx == ARMMMUIdx_S12NSE0 ||
> >> -                   mmu_idx == ARMMMUIdx_S1SE0 ||
> >> -                   mmu_idx == ARMMMUIdx_S1NSE0);
> >> +    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
> ldl_phys
> >> +         * calls for stage 1 will also need changing.
> >> +         * For non-EL2 CPUs a stage1+stage2 translation is just stage
> 1.
> >> +         */
> >> +        assert(!arm_feature(env, ARM_FEATURE_EL2));
> >> +        mmu_idx += ARMMMUIdx_S1NSE0;
> >> +    }
> >>
> >>      /* Fast Context Switch Extension.  */
> >> -    if (address < 0x02000000) {
> >> +    if (address < 0x02000000 && mmu_idx != ARMMMUIdx_S2NS) {
> >>          address += A32_BANKED_CURRENT_REG_GET(env, fcseidr);
> >
> >
> > Now that the MMU index includes security state info we use in in certain
> > circumstances to determine the security state.  However, we don't seem to
> > consistently use it.  For example, earlier changes used the mmu_index to
> > choose certain register banks but here we still rely on the BANKED
> macro. I
> > see this inconsistency being prone to errors.  Maybe we have just not
> gotten
> > to change all of the cases over, but I thought I'd highlight it.
>
> Yes, you're right: if the Secure world does an NS ATS operation
> then we should be using the NS copy of the register. I think
> I mentally skipped over this requirement because the whole bit
> of code is irrelevant for v8 CPUs (where FSCEIDR is mandatorily
> RAZ/WI and so it doesn't matter which one we use).
>
> It would also I think be safer to explicitly guard this with
> a not-if-v8 check, because we don't actually implement that
> RAZ/WI behaviour. So something like:
>
>   if (address < 0x02000000 && mmu_idx != ARMMMUIdx_S2NS
>       && !arm_feature(env, ARM_FEATURE_V8)) {
>       uint32_t fcseidr;
>       if (regime_el(env, mmu_idx) == 3) {
>           fcseidr = env->cp15.fcseidr_s;
>       } else {
>           fcseidr = env->cp15.fcseidr_ns;
>       }
>       address += fcseidr;
>   }
>
> (note that stage 1 PL2 lookups need to use the NS FCSEIDR.)
>
>
​Yeah, this approach makes sense.​


> thanks
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 12942 bytes --]

Re: [Qemu-devel] [PATCH 04/11] target-arm: Define correct mmu_idx values and pass them in TB flags

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 526 bytes --]

On Wed, Jan 28, 2015 at 4:34 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 28 January 2015 at 21:57, Greg Bellows <greg.bellows@linaro.org> wrote:
> > After getting through patch 9, I wonder if the TB NS bit can also be
> removed
> > as it is implied in the MMU index.
>
> No, because for a 32-bit EL3 we are always running under a Secure
> translation regime (S1E3) but the TBFLAG_NS bit may be either 0 or
> 1 depending on the value of the SCR.NS bit.
>
>
​That is correct.​



> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 1228 bytes --]