11.03.2016, 11:41, "Peter Maydell" <peter.mayd...@linaro.org>:
>On 4 March 2016 at 23:04, Sergey Sorokin <afaral...@yandex.ru> 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 <afaral...@yandex.ru>
>> ---
>> 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
