From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52140) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YEm22-0005l6-2H for qemu-devel@nongnu.org; Fri, 23 Jan 2015 16:44:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YEm1w-0000L3-7r for qemu-devel@nongnu.org; Fri, 23 Jan 2015 16:44:45 -0500 Received: from mail-qg0-f50.google.com ([209.85.192.50]:56816) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YEm1w-0000Ks-2o for qemu-devel@nongnu.org; Fri, 23 Jan 2015 16:44:40 -0500 Received: by mail-qg0-f50.google.com with SMTP id f51so8141200qge.9 for ; Fri, 23 Jan 2015 13:44:39 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <1422037228-5363-5-git-send-email-peter.maydell@linaro.org> References: <1422037228-5363-1-git-send-email-peter.maydell@linaro.org> <1422037228-5363-5-git-send-email-peter.maydell@linaro.org> Date: Fri, 23 Jan 2015 15:44:39 -0600 Message-ID: From: Greg Bellows Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH 04/11] target-arm: Define correct mmu_idx values and pass them in TB flags List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: "Edgar E. Iglesias" , Andrew Jones , =?UTF-8?B?QWxleCBCZW5uw6ll?= , QEMU Developers , Patch Tracking On Fri, Jan 23, 2015 at 12:20 PM, Peter Maydell wrote: > We currently claim that for ARM the mmu_idx should simply be the current > exception level. However this isn't actually correct -- secure EL0 and EL1 > should have separate indexes from non-secure EL0 and EL1 since their > VA->PA mappings may differ. We also will want an index for stage 2 > translations when we properly support EL2. > > Define and document all seven mmu index values that we require, and > pass the mmu index in the TB flags rather than exception level or > priv/user bit. > > This change doesn't update the get_phys_addr() code, so our page > table walking still assumes a simplistic "user or priv?" model for > the moment. > > Signed-off-by: Peter Maydell > --- > This leaves some odd gaps in the TB flags usage. I will circle > back and clean this up later (including moving the other common > flags like the singlestep ones to the top of the flags word), > but I didn't want to bloat this patchseries further. > --- > target-arm/cpu.h | 113 ++++++++++++++++++++++++++++++++++++--------- > target-arm/helper.c | 3 +- > target-arm/translate-a64.c | 5 +- > target-arm/translate.c | 5 +- > target-arm/translate.h | 3 +- > 5 files changed, 101 insertions(+), 28 deletions(-) > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > index 3eb00f4..cf7b9ab 100644 > --- a/target-arm/cpu.h > +++ b/target-arm/cpu.h > @@ -98,7 +98,7 @@ typedef uint32_t ARMReadCPFunc(void *opaque, int cp_info, > > struct arm_boot_info; > > -#define NB_MMU_MODES 4 > +#define NB_MMU_MODES 7 > > /* We currently assume float and double are IEEE single and double > precision respectively. > @@ -1572,13 +1572,92 @@ static inline CPUARMState *cpu_init(const char *cpu_model) > #define cpu_signal_handler cpu_arm_signal_handler > #define cpu_list arm_cpu_list > > -/* MMU modes definitions */ > +/* ARM has the following "translation regimes" (as the ARM ARM calls them): > + * > + * If EL3 is 64-bit: > + * + NonSecure EL1 & 0 stage 1 > + * + NonSecure EL1 & 0 stage 2 > + * + NonSecure EL2 > + * + Secure EL1 & EL0 > + * + Secure EL3 > + * If EL3 is 32-bit: > + * + NonSecure PL1 & 0 stage 1 > + * + NonSecure PL1 & 0 stage 2 > + * + NonSecure PL2 > + * + Secure PL0 & PL1 > + * (reminder: for 32 bit EL3, Secure PL1 is *EL3*, not EL1.) > + * > + * For QEMU, an mmu_idx is not quite the same as a translation regime because: > + * 1. we need to split the "EL1 & 0" regimes into two mmu_idxes, because they > + * may differ in access permissions even if the VA->PA map is the same > + * 2. we want to cache in our TLB the full VA->IPA->PA lookup for a stage 1+2 > + * translation, which means that we have one mmu_idx that deals with two > + * concatenated translation regimes [this sort of combined s1+2 TLB is > + * architecturally permitted] > + * 3. we don't need to allocate an mmu_idx to translations that we won't be > + * handling via the TLB. The only way to do a stage 1 translation without > + * the immediate stage 2 translation is via the ATS or AT system insns, > + * which can be slow-pathed and always do a page table walk. > + * 4. we can also safely fold together the "32 bit EL3" and "64 bit EL3" > + * translation regimes, because they map reasonably well to each other > + * and they can't both be active at the same time. > + * This gives us the following list of mmu_idx values: > + * > + * NS EL0 (aka NS PL0) stage 1+2 > + * NS EL1 (aka NS PL1) stage 1+2 > + * NS EL2 (aka NS PL2) > + * S EL3 (aka S PL1) > + * S EL0 (aka S PL0) > + * S EL1 (not used if EL3 is 32 bit) > + * NS EL0+1 stage 2 > + * > + * (The last of these is an mmu_idx because we want to be able to use the TLB > + * for the accesses done as part of a stage 1 page table walk, rather than > + * having to walk the stage 2 page table over and over.) > + * > + * Our enumeration includes at the end some entries which are not "true" > + * mmu_idx values in that they don't have corresponding TLBs and are only > + * valid for doing slow path page table walks. > + * > + * The constant names here are patterned after the general style of the names > + * of the AT/ATS operations. > + * The values used are carefully arranged to make mmu_idx => EL lookup easy. > + */ > +typedef enum ARMMMUIdx { > + ARMMMUIdx_S12NSE0 = 0, > + ARMMMUIdx_S12NSE1 = 1, > + ARMMMUIdx_S1E2 = 2, > + ARMMMUIdx_S1E3 = 3, > + ARMMMUIdx_S1SE0 = 4, > + ARMMMUIdx_S1SE1 = 5, > + ARMMMUIdx_S2NS = 6, > + /* Indexes below here don't have TLBs and are used only for AT system > + * instructions or for the first stage of an S12 page table walk. > + */ > + ARMMMUIdx_S1NSE0 = 7, > + ARMMMUIdx_S1NSE1 = 8, > +} ARMMMUIdx; > + > #define MMU_MODE0_SUFFIX _user > #define MMU_MODE1_SUFFIX _kernel > #define MMU_USER_IDX 0 > + > +/* Return the exception level we're running at if this is our mmu_idx */ > +static inline int arm_mmu_idx_to_el(ARMMMUIdx mmu_idx) > +{ > + assert(mmu_idx < ARMMMUIdx_S2NS); > + return mmu_idx & 3; > +} > + > +/* Determine the current mmu_idx to use for normal loads/stores */ > static inline int cpu_mmu_index (CPUARMState *env) > { > - return arm_current_el(env); > + int el = arm_current_el(env); > + > + if (el < 3 && arm_is_secure_below_el3(env)) { We bypass the secure check for EL3 but not EL2. We should either circumvent both EL2 & 3 or neither. > + return ARMMMUIdx_S1SE0 + el; > + } > + return el; > } > > /* Return the Exception Level targeted by debug exceptions; > @@ -1645,9 +1724,13 @@ static inline bool arm_singlestep_active(CPUARMState *env) > > /* Bit usage in the TB flags field: bit 31 indicates whether we are > * in 32 or 64 bit mode. The meaning of the other bits depends on that. > + * We put flags which are shared between 32 and 64 bit mode at the top > + * of the word, and flags which apply to only one mode at the bottom. > */ > #define ARM_TBFLAG_AARCH64_STATE_SHIFT 31 > #define ARM_TBFLAG_AARCH64_STATE_MASK (1U << ARM_TBFLAG_AARCH64_STATE_SHIFT) > +#define ARM_TBFLAG_MMUIDX_SHIFT 28 > +#define ARM_TBFLAG_MMUIDX_MASK (0x7 << ARM_TBFLAG_MMUIDX_SHIFT) > > /* Bit usage when in AArch32 state: */ > #define ARM_TBFLAG_THUMB_SHIFT 0 > @@ -1656,8 +1739,6 @@ static inline bool arm_singlestep_active(CPUARMState *env) > #define ARM_TBFLAG_VECLEN_MASK (0x7 << ARM_TBFLAG_VECLEN_SHIFT) > #define ARM_TBFLAG_VECSTRIDE_SHIFT 4 > #define ARM_TBFLAG_VECSTRIDE_MASK (0x3 << ARM_TBFLAG_VECSTRIDE_SHIFT) > -#define ARM_TBFLAG_PRIV_SHIFT 6 > -#define ARM_TBFLAG_PRIV_MASK (1 << ARM_TBFLAG_PRIV_SHIFT) > #define ARM_TBFLAG_VFPEN_SHIFT 7 > #define ARM_TBFLAG_VFPEN_MASK (1 << ARM_TBFLAG_VFPEN_SHIFT) > #define ARM_TBFLAG_CONDEXEC_SHIFT 8 > @@ -1683,8 +1764,6 @@ static inline bool arm_singlestep_active(CPUARMState *env) > #define ARM_TBFLAG_NS_MASK (1 << ARM_TBFLAG_NS_SHIFT) > > /* Bit usage when in AArch64 state */ > -#define ARM_TBFLAG_AA64_EL_SHIFT 0 > -#define ARM_TBFLAG_AA64_EL_MASK (0x3 << ARM_TBFLAG_AA64_EL_SHIFT) > #define ARM_TBFLAG_AA64_FPEN_SHIFT 2 > #define ARM_TBFLAG_AA64_FPEN_MASK (1 << ARM_TBFLAG_AA64_FPEN_SHIFT) > #define ARM_TBFLAG_AA64_SS_ACTIVE_SHIFT 3 > @@ -1695,14 +1774,14 @@ static inline bool arm_singlestep_active(CPUARMState *env) > /* some convenience accessor macros */ > #define ARM_TBFLAG_AARCH64_STATE(F) \ > (((F) & ARM_TBFLAG_AARCH64_STATE_MASK) >> ARM_TBFLAG_AARCH64_STATE_SHIFT) > +#define ARM_TBFLAG_MMUIDX(F) \ > + (((F) & ARM_TBFLAG_MMUIDX_MASK) >> ARM_TBFLAG_MMUIDX_SHIFT) > #define ARM_TBFLAG_THUMB(F) \ > (((F) & ARM_TBFLAG_THUMB_MASK) >> ARM_TBFLAG_THUMB_SHIFT) > #define ARM_TBFLAG_VECLEN(F) \ > (((F) & ARM_TBFLAG_VECLEN_MASK) >> ARM_TBFLAG_VECLEN_SHIFT) > #define ARM_TBFLAG_VECSTRIDE(F) \ > (((F) & ARM_TBFLAG_VECSTRIDE_MASK) >> ARM_TBFLAG_VECSTRIDE_SHIFT) > -#define ARM_TBFLAG_PRIV(F) \ > - (((F) & ARM_TBFLAG_PRIV_MASK) >> ARM_TBFLAG_PRIV_SHIFT) > #define ARM_TBFLAG_VFPEN(F) \ > (((F) & ARM_TBFLAG_VFPEN_MASK) >> ARM_TBFLAG_VFPEN_SHIFT) > #define ARM_TBFLAG_CONDEXEC(F) \ > @@ -1717,8 +1796,6 @@ static inline bool arm_singlestep_active(CPUARMState *env) > (((F) & ARM_TBFLAG_PSTATE_SS_MASK) >> ARM_TBFLAG_PSTATE_SS_SHIFT) > #define ARM_TBFLAG_XSCALE_CPAR(F) \ > (((F) & ARM_TBFLAG_XSCALE_CPAR_MASK) >> ARM_TBFLAG_XSCALE_CPAR_SHIFT) > -#define ARM_TBFLAG_AA64_EL(F) \ > - (((F) & ARM_TBFLAG_AA64_EL_MASK) >> ARM_TBFLAG_AA64_EL_SHIFT) > #define ARM_TBFLAG_AA64_FPEN(F) \ > (((F) & ARM_TBFLAG_AA64_FPEN_MASK) >> ARM_TBFLAG_AA64_FPEN_SHIFT) > #define ARM_TBFLAG_AA64_SS_ACTIVE(F) \ > @@ -1742,8 +1819,7 @@ static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc, > > if (is_a64(env)) { > *pc = env->pc; > - *flags = ARM_TBFLAG_AARCH64_STATE_MASK > - | (arm_current_el(env) << ARM_TBFLAG_AA64_EL_SHIFT); > + *flags = ARM_TBFLAG_AARCH64_STATE_MASK; > if (fpen == 3 || (fpen == 1 && arm_current_el(env) != 0)) { > *flags |= ARM_TBFLAG_AA64_FPEN_MASK; > } > @@ -1761,21 +1837,12 @@ static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc, > } > } > } else { > - int privmode; > *pc = env->regs[15]; > *flags = (env->thumb << ARM_TBFLAG_THUMB_SHIFT) > | (env->vfp.vec_len << ARM_TBFLAG_VECLEN_SHIFT) > | (env->vfp.vec_stride << ARM_TBFLAG_VECSTRIDE_SHIFT) > | (env->condexec_bits << ARM_TBFLAG_CONDEXEC_SHIFT) > | (env->bswap_code << ARM_TBFLAG_BSWAP_CODE_SHIFT); > - if (arm_feature(env, ARM_FEATURE_M)) { > - privmode = !((env->v7m.exception == 0) && (env->v7m.control & 1)); > - } else { > - privmode = (env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_USR; > - } > - if (privmode) { > - *flags |= ARM_TBFLAG_PRIV_MASK; > - } > if (!(access_secure_reg(env))) { > *flags |= ARM_TBFLAG_NS_MASK; > } > @@ -1803,6 +1870,8 @@ static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc, > << ARM_TBFLAG_XSCALE_CPAR_SHIFT); > } > > + *flags |= (cpu_mmu_index(env) << ARM_TBFLAG_MMUIDX_SHIFT); > + > *cs_base = 0; > } > > diff --git a/target-arm/helper.c b/target-arm/helper.c > index 1a5e067..06478d8 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -5119,7 +5119,8 @@ int arm_cpu_handle_mmu_fault(CPUState *cs, vaddr address, > uint32_t syn; > bool same_el = (arm_current_el(env) != 0); > > - is_user = mmu_idx == MMU_USER_IDX; > + /* TODO: pass the translation regime to get_phys_addr */ > + is_user = (arm_mmu_idx_to_el(mmu_idx) == 0); > ret = get_phys_addr(env, address, access_type, is_user, &phys_addr, &prot, > &page_size); > if (ret == 0) { > diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c > index dac2f63..96f14ff 100644 > --- a/target-arm/translate-a64.c > +++ b/target-arm/translate-a64.c > @@ -10922,14 +10922,15 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu, > dc->bswap_code = 0; > dc->condexec_mask = 0; > dc->condexec_cond = 0; > + dc->mmu_idx = ARM_TBFLAG_MMUIDX(tb->flags); > + dc->current_el = arm_mmu_idx_to_el(dc->mmu_idx); > #if !defined(CONFIG_USER_ONLY) > - dc->user = (ARM_TBFLAG_AA64_EL(tb->flags) == 0); > + dc->user = (dc->current_el == 0); > #endif > dc->cpacr_fpen = ARM_TBFLAG_AA64_FPEN(tb->flags); > dc->vec_len = 0; > dc->vec_stride = 0; > dc->cp_regs = cpu->cp_regs; > - dc->current_el = arm_current_el(env); > dc->features = env->features; > > /* Single step state. The code-generation logic here is: > diff --git a/target-arm/translate.c b/target-arm/translate.c > index bdfcdf1..7163649 100644 > --- a/target-arm/translate.c > +++ b/target-arm/translate.c > @@ -11032,8 +11032,10 @@ static inline void gen_intermediate_code_internal(ARMCPU *cpu, > dc->bswap_code = ARM_TBFLAG_BSWAP_CODE(tb->flags); > dc->condexec_mask = (ARM_TBFLAG_CONDEXEC(tb->flags) & 0xf) << 1; > dc->condexec_cond = ARM_TBFLAG_CONDEXEC(tb->flags) >> 4; > + dc->mmu_idx = ARM_TBFLAG_MMUIDX(tb->flags); > + dc->current_el = arm_mmu_idx_to_el(dc->mmu_idx); > #if !defined(CONFIG_USER_ONLY) > - dc->user = (ARM_TBFLAG_PRIV(tb->flags) == 0); > + dc->user = (dc->current_el == 0); > #endif > dc->ns = ARM_TBFLAG_NS(tb->flags); > dc->cpacr_fpen = ARM_TBFLAG_CPACR_FPEN(tb->flags); > @@ -11042,7 +11044,6 @@ static inline void gen_intermediate_code_internal(ARMCPU *cpu, > dc->vec_stride = ARM_TBFLAG_VECSTRIDE(tb->flags); > dc->c15_cpar = ARM_TBFLAG_XSCALE_CPAR(tb->flags); > dc->cp_regs = cpu->cp_regs; > - dc->current_el = arm_current_el(env); > dc->features = env->features; > > /* Single step state. The code-generation logic here is: > diff --git a/target-arm/translate.h b/target-arm/translate.h > index f6ee789..a1eb5b5 100644 > --- a/target-arm/translate.h > +++ b/target-arm/translate.h > @@ -20,6 +20,7 @@ typedef struct DisasContext { > #if !defined(CONFIG_USER_ONLY) > int user; > #endif > + ARMMMUIdx mmu_idx; /* MMU index to use for normal loads/stores */ > bool ns; /* Use non-secure CPREG bank on access */ > bool cpacr_fpen; /* FP enabled via CPACR.FPEN */ > bool vfp_enabled; /* FP enabled via FPSCR.EN */ > @@ -69,7 +70,7 @@ static inline int arm_dc_feature(DisasContext *dc, int feature) > > static inline int get_mem_index(DisasContext *s) > { > - return s->current_el; > + return s->mmu_idx; > } > > /* target-specific extra values for is_jmp */ > -- > 1.9.1 > Otherwise, Reviewed-by: Greg Bellows