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

[Peter Maydell]

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