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

[Greg Bellows <greg.bellows@linaro.org>]

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

On Wed, Jan 28, 2015 at 4:30 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 28 January 2015 at 21:37, Greg Bellows <greg.bellows@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
>

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