From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53992) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YGb83-0004XF-5N for qemu-devel@nongnu.org; Wed, 28 Jan 2015 17:30:32 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YGb7x-0001nR-LS for qemu-devel@nongnu.org; Wed, 28 Jan 2015 17:30:31 -0500 Received: from mail-lb0-f169.google.com ([209.85.217.169]:38677) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YGb7x-0001n9-71 for qemu-devel@nongnu.org; Wed, 28 Jan 2015 17:30:25 -0500 Received: by mail-lb0-f169.google.com with SMTP id f15so22157775lbj.0 for ; Wed, 28 Jan 2015 14:30:24 -0800 (PST) MIME-Version: 1.0 In-Reply-To: References: <1422037228-5363-1-git-send-email-peter.maydell@linaro.org> <1422037228-5363-10-git-send-email-peter.maydell@linaro.org> From: Peter Maydell Date: Wed, 28 Jan 2015 22:30:03 +0000 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH 09/11] target-arm: Use mmu_idx in get_phys_addr() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Bellows Cc: "Edgar E. Iglesias" , Andrew Jones , =?UTF-8?B?QWxleCBCZW5uw6ll?= , QEMU Developers , Patch Tracking 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. (Basically, short-descriptors are obsolete and are only supported in the pre-Virtualization translation regimes, ie AArch32 EL1/3.) > I realize EL2 is not supported yet, but wouldn't this break AArch32 if it > were and the LPAE feature was not enabled? Implementations with Virtualization must include LPAE. >> -static bool get_level1_table_address(CPUARMState *env, uint32_t *table, >> - uint32_t address) >> +static bool get_level1_table_address(CPUARMState *env, ARMMMUIdx mmu_idx, >> + uint32_t *table, uint32_t address) >> { >> - /* Get the TCR bank based on our security state */ >> - TCR *tcr = &env->cp15.tcr_el[arm_is_secure(env) ? 3 : 1]; >> + /* Note that we can only get here for an AArch32 PL0/PL1 lookup */ >> + int el = regime_el(env, mmu_idx); >> + TCR *tcr = regime_tcr(env, mmu_idx); >> >> - /* We only get here if EL1 is running in AArch32. If EL3 is running >> in >> - * AArch32 there is a secure and non-secure instance of the >> translation >> - * table registers. >> - */ >> if (address & tcr->mask) { >> if (tcr->raw_tcr & TTBCR_PD1) { >> /* Translation table walk disabled for TTBR1 */ >> return false; >> } >> - *table = A32_BANKED_CURRENT_REG_GET(env, ttbr1) & 0xffffc000; >> + *table = env->cp15.ttbr1_el[el] & 0xffffc000; > > > Perhaps you plan to address this in a separate patch, but I believe TTBR1 is > only applicable to EL1 and EL0 in AArch64. It's true that TTBR1 is only for EL0/EL1, but see the comment at the start of the function -- we can't get here except for EL0 and EL1, because this function is only used for some kinds of short-descriptor tables. >> @@ -4663,7 +4736,12 @@ static int get_phys_addr_v5(CPUARMState *env, >> uint32_t address, int access_type, >> desc = ldl_phys(cs->as, table); >> type = (desc & 3); >> domain = (desc >> 5) & 0x0f; >> - domain_prot = (A32_BANKED_CURRENT_REG_GET(env, dacr) >> (domain * 2)) >> & 3; >> + if (regime_el(env, mmu_idx) == 1) { >> + dacr = env->cp15.dacr_ns; >> + } else { >> + dacr = env->cp15.dacr_s; >> + } >> + domain_prot = (dacr >> (domain * 2)) & 3; > > > Is there a reason that you did not add a regime_dacr() here like you did for > SCTLR and TCR? Didn't seem necessary, since we know we only need to deal with S vs NS, and the concept isn't generally applicable to most regimes. If the dacr in env->cp15 was stored as dacr[4] I'd have used dacr[el] as I do above for the ttbr1_el[], but it isn't, hence the conditional. The TCR and SCTLR are used in LPAE format page tables so they apply for the whole set of translation regimes. > Also, the ARMv8 ARM makes it sound like DACR has no value in AArch64. Well, the DACR is only relevant to short-descriptor format page tables, so it's only consulted for AArch32 translations, and there's no equivalent register in AArch64. (There is a DACR32_EL2 64 bit register, but that is only there so a hypervisor can save and restore the state of a 32 bit VM (at EL1) that is using short-descriptor page tables.) > However, if it did have meaning in AArch64, then for S1SE1 would we be > accessing the wrong bank as regime_el returns 1? This working off the > understanding that an address reference from an instruction executed in > S/EL1 and AArch64 would generate such an index. We can only get here for regime S1SE1 if: * EL3 is AArch64 * EL1 is AArch32 Since EL3 is 64 bit, there is no banking of registers and regardless of whether EL1 is Secure or NonSecure we want the one and only register (which is in dacr_ns). (Compare the way we use ttbr1_el[regime_el(env, mmu_idx)] and so get TTBR1_EL1 whether this is Secure EL1 or NonSecure EL1.) If EL3 is 32 bit then there is banking of registers, but it's not possible to get here for S1SE1 in that case (only for S EL3 and NS EL1). >> + /* TODO: >> + * This code assumes we're either a 64-bit EL1 or a 32-bit PL1; >> + * it doesn't handle the different format TCR for TCR_EL2, TCR_EL3, >> + * and VTCR_EL2, or the fact that those regimes don't have a split >> + * TTBR0/TTBR1. Attribute and permission bit handling should also >> + * be checked when adding support for those page table walks. >> + */ > > > Maybe copy this comment up above in get_level1_table_address(). This is the correct location; see remarks above. >> @@ -5171,39 +5269,43 @@ static inline int get_phys_addr(CPUARMState *env, >> target_ulong address, >> hwaddr *phys_ptr, int *prot, >> target_ulong *page_size) >> { >> - /* This is not entirely correct as get_phys_addr() can also be called >> - * from ats_write() for an address translation of a specific regime. >> - */ >> - uint32_t sctlr = A32_BANKED_CURRENT_REG_GET(env, sctlr); >> - >> - /* This will go away when we handle mmu_idx properly here */ >> - int is_user = (mmu_idx == ARMMMUIdx_S12NSE0 || >> - mmu_idx == ARMMMUIdx_S1SE0 || >> - mmu_idx == ARMMMUIdx_S1NSE0); >> + if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) { >> + /* TODO: when we support EL2 we should here call ourselves >> recursively >> + * to do the stage 1 and then stage 2 translations. The ldl_phys >> + * calls for stage 1 will also need changing. >> + * For non-EL2 CPUs a stage1+stage2 translation is just stage 1. >> + */ >> + assert(!arm_feature(env, ARM_FEATURE_EL2)); >> + mmu_idx += ARMMMUIdx_S1NSE0; >> + } >> >> /* Fast Context Switch Extension. */ >> - if (address < 0x02000000) { >> + if (address < 0x02000000 && mmu_idx != ARMMMUIdx_S2NS) { >> address += A32_BANKED_CURRENT_REG_GET(env, fcseidr); > > > Now that the MMU index includes security state info we use in in certain > circumstances to determine the security state. However, we don't seem to > consistently use it. For example, earlier changes used the mmu_index to > choose certain register banks but here we still rely on the BANKED macro. I > see this inconsistency being prone to errors. Maybe we have just not gotten > to change all of the cases over, but I thought I'd highlight it. Yes, you're right: if the Secure world does an NS ATS operation then we should be using the NS copy of the register. I think I mentally skipped over this requirement because the whole bit of code is irrelevant for v8 CPUs (where FSCEIDR is mandatorily RAZ/WI and so it doesn't matter which one we use). It would also I think be safer to explicitly guard this with a not-if-v8 check, because we don't actually implement that RAZ/WI behaviour. So something like: if (address < 0x02000000 && mmu_idx != ARMMMUIdx_S2NS && !arm_feature(env, ARM_FEATURE_V8)) { uint32_t fcseidr; if (regime_el(env, mmu_idx) == 3) { fcseidr = env->cp15.fcseidr_s; } else { fcseidr = env->cp15.fcseidr_ns; } address += fcseidr; } (note that stage 1 PL2 lookups need to use the NS FCSEIDR.) thanks -- PMM