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

[Qemu-devel] [PATCH v2 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

Changes v1->v2:
 * use the correct FCSEIDR for the translation regime
 * fix typo in patch 1 for MEMSUFFIX to use for new index 6
 * a few new comments and other minor nits as per review of v1

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           | 121 +++++++---
 target-arm/helper.c        | 548 +++++++++++++++++++++++++++++++--------------
 target-arm/translate-a64.c |  24 +-
 target-arm/translate.c     |  31 ++-
 target-arm/translate.h     |   3 +-
 6 files changed, 557 insertions(+), 198 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v2 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>
Reviewed-by: Greg Bellows <greg.bellows@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..1673287 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_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

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

[Richard Henderson]

On 01/29/2015 10:55 AM, 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>
> Reviewed-by: Greg Bellows <greg.bellows@linaro.org>
> ---
>  include/exec/cpu_ldst.h | 28 +++++++++++++++++++++++++---
>  1 file changed, 25 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~

[Qemu-devel] [PATCH v2 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>
Reviewed-by: Greg Bellows <greg.bellows@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 v2 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>
Reviewed-by: Greg Bellows <greg.bellows@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

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

[Edgar E. Iglesias]

On Thu, Jan 29, 2015 at 06:55:09PM +0000, Peter Maydell 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>
> Reviewed-by: Greg Bellows <greg.bellows@linaro.org>


Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
> 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 v2 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>
Reviewed-by: Greg Bellows <greg.bellows@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           | 115 ++++++++++++++++++++++++++++++++++++---------
 target-arm/helper.c        |   3 +-
 target-arm/translate-a64.c |   5 +-
 target-arm/translate.c     |   5 +-
 target-arm/translate.h     |   3 +-
 5 files changed, 102 insertions(+), 29 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 3eb00f4..cf47952 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
-static inline int cpu_mmu_index (CPUARMState *env)
+
+/* 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)
 {
-    return arm_current_el(env);
+    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)
+{
+    int el = arm_current_el(env);
+
+    if (el < 2 && 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 v2 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>
Reviewed-by: Greg Bellows <greg.bellows@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 v2 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>
Reviewed-by: Greg Bellows <greg.bellows@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 cf47952..b45280d 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

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

[Edgar E. Iglesias]

On Thu, Jan 29, 2015 at 06:55:12PM +0000, Peter Maydell 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>
> Reviewed-by: Greg Bellows <greg.bellows@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

> ---
>  target-arm/cpu.h | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index cf47952..b45280d 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 v2 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>
Reviewed-by: Greg Bellows <greg.bellows@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

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

[Edgar E. Iglesias]

On Thu, Jan 29, 2015 at 06:55:13PM +0000, Peter Maydell 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>
> Reviewed-by: Greg Bellows <greg.bellows@linaro.org>

Sounds good:

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  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 v2 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>
Reviewed-by: Greg Bellows <greg.bellows@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..589a074 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: ATS1CPR, ATS1CPW */
+        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: ATS1CUR, ATS1CUW */
+        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: ATS12NSOPR, ATS12NSOPW */
+        mmu_idx = ARMMMUIdx_S12NSE1;
+        break;
+    case 6:
+        /* stage 1+2 NonSecure PL0: ATS12NSOUR, ATS12NSOUW */
+        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: /* AT S1E1R, AT S1E1W */
+            mmu_idx = secure ? ARMMMUIdx_S1SE1 : ARMMMUIdx_S1NSE1;
+            break;
+        case 4: /* AT S1E2R, AT S1E2W */
+            mmu_idx = ARMMMUIdx_S1E2;
+            break;
+        case 6: /* AT S1E3R, AT S1E3W */
+            mmu_idx = ARMMMUIdx_S1E3;
+            break;
+        default:
+            g_assert_not_reached();
+        }
+        break;
+    case 2: /* AT S1E0R, AT S1E0W */
+        mmu_idx = secure ? ARMMMUIdx_S1SE0 : ARMMMUIdx_S1NSE0;
+        break;
+    case 4: /* AT S12E1R, AT S12E1W */
+        mmu_idx = ARMMMUIdx_S12NSE1;
+        break;
+    case 6: /* AT S12E0R, AT S12E0W */
+        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

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

[Edgar E. Iglesias]

On Thu, Jan 29, 2015 at 06:55:14PM +0000, Peter Maydell 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>
> Reviewed-by: Greg Bellows <greg.bellows@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  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..589a074 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: ATS1CPR, ATS1CPW */
> +        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: ATS1CUR, ATS1CUW */
> +        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: ATS12NSOPR, ATS12NSOPW */
> +        mmu_idx = ARMMMUIdx_S12NSE1;
> +        break;
> +    case 6:
> +        /* stage 1+2 NonSecure PL0: ATS12NSOUR, ATS12NSOUW */
> +        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: /* AT S1E1R, AT S1E1W */
> +            mmu_idx = secure ? ARMMMUIdx_S1SE1 : ARMMMUIdx_S1NSE1;
> +            break;
> +        case 4: /* AT S1E2R, AT S1E2W */
> +            mmu_idx = ARMMMUIdx_S1E2;
> +            break;
> +        case 6: /* AT S1E3R, AT S1E3W */
> +            mmu_idx = ARMMMUIdx_S1E3;
> +            break;
> +        default:
> +            g_assert_not_reached();
> +        }
> +        break;
> +    case 2: /* AT S1E0R, AT S1E0W */
> +        mmu_idx = secure ? ARMMMUIdx_S1SE0 : ARMMMUIdx_S1NSE0;
> +        break;
> +    case 4: /* AT S12E1R, AT S12E1W */
> +        mmu_idx = ARMMMUIdx_S12NSE1;
> +        break;
> +    case 6: /* AT S12E0R, AT S12E0W */
> +        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 v2 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 | 214 +++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 163 insertions(+), 51 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 589a074..042ee7a 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -4556,13 +4556,91 @@ 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;
+    case ARMMMUIdx_S12NSE0:
+    case ARMMMUIdx_S12NSE1:
+        g_assert_not_reached();
+    }
+}
+
 /* 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 +4658,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 +4690,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 +4727,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 +4739,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 +4808,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 +4821,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 +4835,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 +4857,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 +4916,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 +4954,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 +4974,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 +4999,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 +5059,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 +5148,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 +5183,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 +5272,50 @@ 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) {
-        address += A32_BANKED_CURRENT_REG_GET(env, fcseidr);
+    /* Fast Context Switch Extension. This doesn't exist at all in v8.
+     * In v7 and earlier it affects all stage 1 translations.
+     */
+    if (address < 0x02000000 && mmu_idx != ARMMMUIdx_S2NS
+        && !arm_feature(env, ARM_FEATURE_V8)) {
+        if (regime_el(env, mmu_idx) == 3) {
+            address += env->cp15.fcseidr_s;
+        } else {
+            address += env->cp15.fcseidr_ns;
+        }
     }
 
-    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

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

[Edgar E. Iglesias]

On Thu, Jan 29, 2015 at 06:55:15PM +0000, Peter Maydell 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 | 214 +++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 163 insertions(+), 51 deletions(-)
> 
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 589a074..042ee7a 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4556,13 +4556,91 @@ 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;
> +    case ARMMMUIdx_S12NSE0:
> +    case ARMMMUIdx_S12NSE1:
> +        g_assert_not_reached();
> +    }
> +}
> +
>  /* 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 +4658,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 +4690,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 +4727,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 +4739,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 +4808,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 +4821,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 +4835,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 +4857,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 +4916,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 +4954,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 +4974,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 +4999,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 +5059,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 +5148,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 +5183,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 +5272,50 @@ 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;

I'm not sure I understand this. Did you mean the following?
mmu_idx = ARMMMUIdx_S1NSE0;

Maybe you can relax the assert to check for FEATURE_EL2 and hcr_el2 & HCR_VM ?
And not change the mmu_idx.

Cheers,
Edgar



> +    }
>  
> -    /* Fast Context Switch Extension.  */
> -    if (address < 0x02000000) {
> -        address += A32_BANKED_CURRENT_REG_GET(env, fcseidr);
> +    /* Fast Context Switch Extension. This doesn't exist at all in v8.
> +     * In v7 and earlier it affects all stage 1 translations.
> +     */
> +    if (address < 0x02000000 && mmu_idx != ARMMMUIdx_S2NS
> +        && !arm_feature(env, ARM_FEATURE_V8)) {
> +        if (regime_el(env, mmu_idx) == 3) {
> +            address += env->cp15.fcseidr_s;
> +        } else {
> +            address += env->cp15.fcseidr_ns;
> +        }
>      }
>  
> -    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
> 

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

[Peter Maydell]

On 30 January 2015 at 02:03, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> On Thu, Jan 29, 2015 at 06:55:15PM +0000, Peter Maydell 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.
>> -    /* 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;
>
> I'm not sure I understand this. Did you mean the following?
> mmu_idx = ARMMMUIdx_S1NSE0;

No. This code is handling "we asked for a stage1+2 EL0 or EL1
lookup but we don't have EL2". In this case these degrade
to the equivalent stage-1-only lookups:
 S12NSE0 -> S1NSE0
 S12NSE1 -> S1NSE1
We're relying on S12NSE0 being zero and the E0/E1 indexes
being consecutive on both sides.

> Maybe you can relax the assert to check for FEATURE_EL2 and hcr_el2 & HCR_VM ?
> And not change the mmu_idx.

The assert is here to say "if you want to implement EL2 there
is work to do here". For EL2, this is going to look
something like:
    if (arm_feature(env, ARM_FEATURE_EL2 && (hcr_el2 & HCR_VM)) {
        /* stage 2 exists and is enabled */
        hwaddr ipa;
        get_phys_addr(env, addr, &ipa, ..., stage 1 mmuidx, ...);
        handle stage 1 faults;
        get_phys_addr(env, ipa, &physaddr, ...., stage 2 mmuidx, ...);
        handle stage 2 faults;
        combine protection etc info from stage 1 and stage 2;
        return final physaddr for combined lookup;
    }

That's quite a bit of extra code, so it's deferred til we
actually implement EL2, and in the meantime we assert as a
marker for "if you hit this you need to implement all that".

-- PMM

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

[Greg Bellows]

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

On Thu, Jan 29, 2015 at 12:55 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 | 214
> +++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 163 insertions(+), 51 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 589a074..042ee7a 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4556,13 +4556,91 @@ 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;
> +    case ARMMMUIdx_S12NSE0:
> +    case ARMMMUIdx_S12NSE1:
> +        g_assert_not_reached();
> +    }
> +}
> +
>  /* 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 +4658,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 +4690,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 +4727,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 +4739,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 +4808,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 +4821,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 +4835,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 +4857,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 +4916,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 +4954,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 +4974,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 +4999,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 +5059,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 +5148,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 +5183,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 +5272,50 @@ 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) {
> -        address += A32_BANKED_CURRENT_REG_GET(env, fcseidr);
> +    /* Fast Context Switch Extension. This doesn't exist at all in v8.
> +     * In v7 and earlier it affects all stage 1 translations.
> +     */
> +    if (address < 0x02000000 && mmu_idx != ARMMMUIdx_S2NS
> +        && !arm_feature(env, ARM_FEATURE_V8)) {
> +        if (regime_el(env, mmu_idx) == 3) {
> +            address += env->cp15.fcseidr_s;
> +        } else {
> +            address += env->cp15.fcseidr_ns;
> +        }
>      }
>
> -    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
>
>
​Reviewed-by: Greg Bellows <greg.bellows@linaro.org>​

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

[Qemu-devel] [PATCH v2 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>
Reviewed-by: Greg Bellows <greg.bellows@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 042ee7a..c6f76b4 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -4639,55 +4639,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,
@@ -4765,13 +4765,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.  */
@@ -4788,17 +4788,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;
@@ -5193,18 +5193,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;
@@ -5214,31 +5214,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

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

[Edgar E. Iglesias]

On Thu, Jan 29, 2015 at 06:55:16PM +0000, Peter Maydell 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>
> Reviewed-by: Greg Bellows <greg.bellows@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

> ---
>  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 042ee7a..c6f76b4 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4639,55 +4639,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,
> @@ -4765,13 +4765,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.  */
> @@ -4788,17 +4788,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;
> @@ -5193,18 +5193,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;
> @@ -5214,31 +5214,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 v2 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>
Reviewed-by: Greg Bellows <greg.bellows@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 c6f76b4..e09af89 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -4646,18 +4646,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;
@@ -4669,10 +4671,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.  */
@@ -4682,8 +4685,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();
@@ -5194,17 +5198,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;
@@ -5216,21 +5223,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 v2 11/11] target-arm: Fix brace style in reindented code

[Edgar E. Iglesias]

On Thu, Jan 29, 2015 at 06:55:17PM +0000, Peter Maydell wrote:
> This patch fixes the brace style in the code reindented in the
> previous commit.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Greg Bellows <greg.bellows@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

> ---
>  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 c6f76b4..e09af89 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4646,18 +4646,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;
> @@ -4669,10 +4671,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.  */
> @@ -4682,8 +4685,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();
> @@ -5194,17 +5198,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;
> @@ -5216,21 +5223,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 v2 00/11] target-arm: handle mmu_idx/translation regimes properly

[Edgar E. Iglesias]

On Thu, Jan 29, 2015 at 06:55:06PM +0000, Peter Maydell wrote:
> 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


Hi Peter,

A little bit of general feedback.

IIRC, last time the dedicated S-EL0 and S-EL1 MMU idx came up the
discussion went around flushing the qemu tlbs when switching between
S/NS. Having the dedicated MMU-idx is faster but for Aarch64 I think
we would need logic in at least the TTBRx access handlers to make use
of the dedicated secure MMU idx as Aarch64 secure monitors need to
reprogram the MMU when world switching.

Another thing around the ARMMMUIdx_S2NS index.
>From what I've seen, what would really help is having a fast
way to go from VM mode to non-vm mode. In particular for KVM.
For example when a guest writes to a virtio console there is alot
of ping-ponging between NS-S12(Guest) and NS-S1(Linux/KVM).

Similary for XEN, it would really help to have that ASID/VMID indexed TLB I
think you suggested at some point. In XEN's case the ping-ponging
goes between two guests, domUs and dom0.

I'm not try to indicate that you should add any of that now,
I'm just not sure sure it's worth adding the ARMMMUIdx_S2NS without
trying if it will actually give any real life improvements in
QEMU.

Cheers,
Edgar


> 
> 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
> 
> Changes v1->v2:
>  * use the correct FCSEIDR for the translation regime
>  * fix typo in patch 1 for MEMSUFFIX to use for new index 6
>  * a few new comments and other minor nits as per review of v1
> 
> 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           | 121 +++++++---
>  target-arm/helper.c        | 548 +++++++++++++++++++++++++++++++--------------
>  target-arm/translate-a64.c |  24 +-
>  target-arm/translate.c     |  31 ++-
>  target-arm/translate.h     |   3 +-
>  6 files changed, 557 insertions(+), 198 deletions(-)
> 
> -- 
> 1.9.1
> 

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

[Peter Maydell]

On 30 January 2015 at 01:36, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> IIRC, last time the dedicated S-EL0 and S-EL1 MMU idx came up the
> discussion went around flushing the qemu tlbs when switching between
> S/NS. Having the dedicated MMU-idx is faster but for Aarch64 I think
> we would need logic in at least the TTBRx access handlers to make use
> of the dedicated secure MMU idx as Aarch64 secure monitors need to
> reprogram the MMU when world switching.
>
> Another thing around the ARMMMUIdx_S2NS index.
> From what I've seen, what would really help is having a fast
> way to go from VM mode to non-vm mode. In particular for KVM.
> For example when a guest writes to a virtio console there is alot
> of ping-ponging between NS-S12(Guest) and NS-S1(Linux/KVM).
>
> Similary for XEN, it would really help to have that ASID/VMID indexed TLB I
> think you suggested at some point. In XEN's case the ping-ponging
> goes between two guests, domUs and dom0.

Yes, the lack of ASID/VMID restrictions on the TLB hurts us.
It's tricky to implement in s/w without it being slower than
what we have now, though.

> I'm not try to indicate that you should add any of that now,
> I'm just not sure sure it's worth adding the ARMMMUIdx_S2NS without
> trying if it will actually give any real life improvements in
> QEMU.

Mostly my aim here was to make sure we were actually treating
separately the various virt-to-phys mappings which the architecture
says are separate, so we don't have nasty "hit something stale
in the TLB" bugs to track down.

-- PMM

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

[Peter Maydell]

On 29 January 2015 at 18:55, Peter Maydell <peter.maydell@linaro.org> wrote:
> 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

I'm planning to put this series into my next target-arm pull,
sometime tail end of the week.

-- PMM