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