Re: [Qemu-devel] [PATCH] target-arm: Fix descriptor address masking in ARM address translation

[Sergey Sorokin]

11.03.2016, 11:41, "Peter Maydell" <address@hidden>:
>On 4 March 2016 at 23:04, Sergey Sorokin <address@hidden> wrote:
>> There is a bug in ARM address translation regime with a long-descriptor
>> format. On the descriptor reading its address is formed from an index
>> which is a part of the input address. And on the first iteration this index
>> is incorrectly masked with 'grainsize' mask. But it can be wider according
>> to pseudo-code.
>> On the other hand on the iterations other than first the descriptor address
>> is formed from the previous level descriptor by masking with 'descaddrmask'
>> value. It always clears just 12 lower bits, but it must clear 'grainsize'
>> lower bits instead according to pseudo-code.
>> The patch fixes both cases.
>
>This is pretty confusing to understand -- it might help if you
>could give an example.

According to documentation (ARMv8 ARM DDI 0487A.i J1.1.5:
aarch64/translation/walk/AArch64.TranslationTableWalk):

bits(48) index = ZeroExtend(inputaddr<addrselecttop:addrselectbottom>:'000');
descaddr.paddress.physicaladdress = baseaddress OR index;

For a first iteration of the descriptor reading:

addrselecttop = inputsize - 1;
addrselectbottom = (3-level)*stride + grainsize;

Let's assume grainsize == 12 (so stride == 9), level == 1, inputsize == 43.
Then index is
inputaddr<42:30>:'000';

But currently QEMU incorrecly masks an input address with descmask value:

descmask = (1ULL << (stride + 3)) - 1;
...
descaddr |= (address >> (stride * (4 - level))) & descmask;
descaddr &= ~7ULL;

Then the index in my example in QEMU is:
address<38:30>:'000';

And my patch fixes this bug.

>
>Is this something that only causes problems for the "concatenated
>translation tables at the initial level" case (which is only
>possible for S2 tables) ?
>
>> Signed-off-by: Sergey Sorokin <address@hidden>
>> ---
>>  target-arm/helper.c | 29 ++++++++++-------------------
>>  1 file changed, 10 insertions(+), 19 deletions(-)
>>
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index dec8e8b..b5f289c 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -7243,7 +7243,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
>> target_ulong address,
>>      uint32_t tg;
>>      uint64_t ttbr;
>>      int ttbr_select;
>> -    hwaddr descaddr, descmask;
>> +    hwaddr descaddr, indexmask, indexmask_grainsize;
>>      uint32_t tableattrs;
>>      target_ulong page_size;
>>      uint32_t attrs;
>> @@ -7431,28 +7431,18 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
>> target_ulong address,
>>          }
>>      }
>>
>> -    /* Clear the vaddr bits which aren't part of the within-region address,
>> -     * so that we don't have to special case things when calculating the
>> -     * first descriptor address.
>> -     */
>> -    if (va_size != inputsize) {
>> -        address &= (1ULL << inputsize) - 1;
>> -    }
>> -
>> -    descmask = (1ULL << (stride + 3)) - 1;
>> +    indexmask_grainsize = (1ULL << (stride + 3)) - 1;
>> +    indexmask = (1ULL << (inputsize - (stride * (4 - level)))) - 1;
>>
>>      /* Now we can extract the actual base address from the TTBR */
>>      descaddr = extract64(ttbr, 0, 48);
>> -    descaddr &= ~((1ULL << (inputsize - (stride * (4 - level)))) - 1);
>> +    descaddr &= ~indexmask;
>>
>> -    /* The address field in the descriptor goes up to bit 39 for ARMv7
>> -     * but up to bit 47 for ARMv8.
>> +    /* The address field in the descriptor goes up to bit 39 for AArch32
>> +     * but up to bit 47 for AArch64.
>>       */
>
>This is not correct -- the descriptor field widths are as the comment
>states before your patch:
> * up to bit 39 for ARMv7
> * up to bit 47 for ARMv8 (whether AArch32 or AArch64)
>
>See the v8 ARM ARM AArch32.TranslationTableWalkLD pseudocode and in
>particular note the width which it uses for AddressSizeFault checks.

I see in ARMv8 ARM DDI 0487A.i J1.2.4
aarch32/translation/walk/AArch32.TranslationTableWalkLD:

Before 'repeat' cycle:
baseaddress = baseregister<39:baselowerbound>:Zeros(baselowerbound);

Inside the cycle:
baseaddress = desc<39:grainsize>:Zeros(grainsize);

We use 'descaddrmask' in QEMU to get this 'baseaddress' from a descriptor.
So my patch is correct and fixes the bug.

>
>> -    if (arm_feature(env, ARM_FEATURE_V8)) {
>> -        descaddrmask = 0xfffffffff000ULL;
>> -    } else {
>> -        descaddrmask = 0xfffffff000ULL;
>> -    }
>> +    descaddrmask = ((1ull << (va_size == 64 ? 48 : 40)) - 1) &
>> +                   ~indexmask_grainsize;
>
>...so this part of the patch is wrong.
>
>>
>>      /* Secure accesses start with the page table in secure memory and
>>       * can be downgraded to non-secure at any step. Non-secure accesses
>> @@ -7464,7 +7454,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
>> target_ulong address,
>>          uint64_t descriptor;
>>          bool nstable;
>>
>> -        descaddr |= (address >> (stride * (4 - level))) & descmask;
>> +        descaddr |= (address >> (stride * (4 - level))) & indexmask;
>>          descaddr &= ~7ULL;
>>          nstable = extract32(tableattrs, 4, 1);
>>          descriptor = arm_ldq_ptw(cs, descaddr, !nstable, mmu_idx, fsr, fi);
>> @@ -7487,6 +7477,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
>> target_ulong address,
>>               */
>>              tableattrs |= extract64(descriptor, 59, 5);
>>              level++;
>> +            indexmask = indexmask_grainsize;
>>              continue;
>>          }
>>          /* Block entry at level 1 or 2, or page entry at level 3.
>> --
>> 1.9.3
>>
>
>thanks
>-- PMM