On Wed, Jan 28, 2015 at 4:30 PM, Peter Maydell wrote: > On 28 January 2015 at 21:37, Greg Bellows wrote: > > > >> +/* Return true if the translation regime is using LPAE format page > tables > >> */ > >> +static inline bool regime_using_lpae_format(CPUARMState *env, > >> + ARMMMUIdx mmu_idx) > >> +{ > >> + int el = regime_el(env, mmu_idx); > >> + if (el == 2 || arm_el_is_aa64(env, el)) { > > > > > > For the life of me, I can not figure out why EL2 is wired to always use > > LPAE. Is this stated in the spec somewhere? I found places where EL2 > > registers can vary depending on TTBCR.EAE bit settings which implies it > is > > not always true. > > The only translation regimes controlled by EL2 are: > (1) EL2's own stage 1 translations > (2) the stage 2 translations > > These must both be LPAE format: see the v8 ARM ARM section G4.4: > "the translation tables for the Non-secure PL2 stage 1 translations, > and for the Non-secure PL1&0 stage 2 translations, must use the > Long-descriptor translation table format." v7 ARM ARM B3.3 has > similar text. > ​I see that now, thanks for pointing this out​ > > (Basically, short-descriptors are obsolete and are only supported in > the pre-Virtualization translation regimes, ie AArch32 EL1/3.) > > > I realize EL2 is not supported yet, but wouldn't this break AArch32 if it > > were and the LPAE feature was not enabled? > > Implementations with Virtualization must include LPAE. > > >> -static bool get_level1_table_address(CPUARMState *env, uint32_t *table, > >> - uint32_t address) > >> +static bool get_level1_table_address(CPUARMState *env, ARMMMUIdx > mmu_idx, > >> + uint32_t *table, uint32_t address) > >> { > >> - /* Get the TCR bank based on our security state */ > >> - TCR *tcr = &env->cp15.tcr_el[arm_is_secure(env) ? 3 : 1]; > >> + /* Note that we can only get here for an AArch32 PL0/PL1 lookup */ > >> + int el = regime_el(env, mmu_idx); > >> + TCR *tcr = regime_tcr(env, mmu_idx); > >> > >> - /* We only get here if EL1 is running in AArch32. If EL3 is running > >> in > >> - * AArch32 there is a secure and non-secure instance of the > >> translation > >> - * table registers. > >> - */ > >> if (address & tcr->mask) { > >> if (tcr->raw_tcr & TTBCR_PD1) { > >> /* Translation table walk disabled for TTBR1 */ > >> return false; > >> } > >> - *table = A32_BANKED_CURRENT_REG_GET(env, ttbr1) & 0xffffc000; > >> + *table = env->cp15.ttbr1_el[el] & 0xffffc000; > > > > > > Perhaps you plan to address this in a separate patch, but I believe > TTBR1 is > > only applicable to EL1 and EL0 in AArch64. > > It's true that TTBR1 is only for EL0/EL1, but see the comment at the > start of the function -- we can't get here except for EL0 and EL1, > because this function is only used for some kinds of short-descriptor > tables. > ​I saw that comment, but was not sure whether it was stale given we were adding EL3. I now see how AArch64 is routed away from this function. > > >> @@ -4663,7 +4736,12 @@ static int get_phys_addr_v5(CPUARMState *env, > >> uint32_t address, int access_type, > >> desc = ldl_phys(cs->as, table); > >> type = (desc & 3); > >> domain = (desc >> 5) & 0x0f; > >> - domain_prot = (A32_BANKED_CURRENT_REG_GET(env, dacr) >> (domain * > 2)) > >> & 3; > >> + if (regime_el(env, mmu_idx) == 1) { > >> + dacr = env->cp15.dacr_ns; > >> + } else { > >> + dacr = env->cp15.dacr_s; > >> + } > >> + domain_prot = (dacr >> (domain * 2)) & 3; > > > > > > Is there a reason that you did not add a regime_dacr() here like you did > for > > SCTLR and TCR? > > Didn't seem necessary, since we know we only need to deal with S vs NS, > and the concept isn't generally applicable to most regimes. If the > dacr in env->cp15 was stored as dacr[4] I'd have used dacr[el] as > I do above for the ttbr1_el[], but it isn't, hence the conditional. > > ​Fair enough, was more a question of consistency since you do the same thing in both the v5 and v6 code.​ > The TCR and SCTLR are used in LPAE format page tables so they apply > for the whole set of translation regimes. > > > Also, the ARMv8 ARM makes it sound like DACR has no value in AArch64. > > Well, the DACR is only relevant to short-descriptor format page tables, > so it's only consulted for AArch32 translations, and there's no > equivalent register in AArch64. (There is a DACR32_EL2 64 bit register, > but that is only there so a hypervisor can save and restore the state > of a 32 bit VM (at EL1) that is using short-descriptor page tables.) > > > However, if it did have meaning in AArch64, then for S1SE1 would we be > > accessing the wrong bank as regime_el returns 1? This working off the > > understanding that an address reference from an instruction executed in > > S/EL1 and AArch64 would generate such an index. > ​So this comment is moot given my misunderstanding that we would not be in this code if AArch64. ​ > > We can only get here for regime S1SE1 if: > * EL3 is AArch64 > * EL1 is AArch32 > > Since EL3 is 64 bit, there is no banking of registers and regardless > of whether EL1 is Secure or NonSecure we want the one and only > register (which is in dacr_ns). (Compare the way we use > ttbr1_el[regime_el(env, mmu_idx)] and so get TTBR1_EL1 whether > this is Secure EL1 or NonSecure EL1.) > > If EL3 is 32 bit then there is banking of registers, but it's > not possible to get here for S1SE1 in that case (only for S EL3 > and NS EL1). > ​Right, that all makes sense. now. > > >> + /* TODO: > >> + * This code assumes we're either a 64-bit EL1 or a 32-bit PL1; > >> + * it doesn't handle the different format TCR for TCR_EL2, TCR_EL3, > >> + * and VTCR_EL2, or the fact that those regimes don't have a split > >> + * TTBR0/TTBR1. Attribute and permission bit handling should also > >> + * be checked when adding support for those page table walks. > >> + */ > > > > > > Maybe copy this comment up above in get_level1_table_address(). > > This is the correct location; see remarks above. > > >> @@ -5171,39 +5269,43 @@ static inline int get_phys_addr(CPUARMState > *env, > >> target_ulong address, > >> hwaddr *phys_ptr, int *prot, > >> target_ulong *page_size) > >> { > >> - /* This is not entirely correct as get_phys_addr() can also be > called > >> - * from ats_write() for an address translation of a specific > regime. > >> - */ > >> - uint32_t sctlr = A32_BANKED_CURRENT_REG_GET(env, sctlr); > >> - > >> - /* This will go away when we handle mmu_idx properly here */ > >> - int is_user = (mmu_idx == ARMMMUIdx_S12NSE0 || > >> - mmu_idx == ARMMMUIdx_S1SE0 || > >> - mmu_idx == ARMMMUIdx_S1NSE0); > >> + if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) { > >> + /* TODO: when we support EL2 we should here call ourselves > >> recursively > >> + * to do the stage 1 and then stage 2 translations. The > ldl_phys > >> + * calls for stage 1 will also need changing. > >> + * For non-EL2 CPUs a stage1+stage2 translation is just stage > 1. > >> + */ > >> + assert(!arm_feature(env, ARM_FEATURE_EL2)); > >> + mmu_idx += ARMMMUIdx_S1NSE0; > >> + } > >> > >> /* Fast Context Switch Extension. */ > >> - if (address < 0x02000000) { > >> + if (address < 0x02000000 && mmu_idx != ARMMMUIdx_S2NS) { > >> address += A32_BANKED_CURRENT_REG_GET(env, fcseidr); > > > > > > Now that the MMU index includes security state info we use in in certain > > circumstances to determine the security state. However, we don't seem to > > consistently use it. For example, earlier changes used the mmu_index to > > choose certain register banks but here we still rely on the BANKED > macro. I > > see this inconsistency being prone to errors. Maybe we have just not > gotten > > to change all of the cases over, but I thought I'd highlight it. > > Yes, you're right: if the Secure world does an NS ATS operation > then we should be using the NS copy of the register. I think > I mentally skipped over this requirement because the whole bit > of code is irrelevant for v8 CPUs (where FSCEIDR is mandatorily > RAZ/WI and so it doesn't matter which one we use). > > It would also I think be safer to explicitly guard this with > a not-if-v8 check, because we don't actually implement that > RAZ/WI behaviour. So something like: > > if (address < 0x02000000 && mmu_idx != ARMMMUIdx_S2NS > && !arm_feature(env, ARM_FEATURE_V8)) { > uint32_t fcseidr; > if (regime_el(env, mmu_idx) == 3) { > fcseidr = env->cp15.fcseidr_s; > } else { > fcseidr = env->cp15.fcseidr_ns; > } > address += fcseidr; > } > > (note that stage 1 PL2 lookups need to use the NS FCSEIDR.) > > ​Yeah, this approach makes sense.​ > thanks > -- PMM >