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

[Sergey Sorokin]

12.03.2016, 03:18, "Peter Maydell" <address@hidden>:
>On 12 March 2016 at 06:44, Sergey Sorokin <address@hidden> wrote:
>> 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';
>
>...which is more than 9 bits, so when does this happen?
>I think this can only happen for the Stage-2 only
>concatenated translation-tables case...
>
>(I agree we have a bug here, I'm just trying to work out when it
>can trigger; if it's only possible for S2 page tables then it's
>not a visible bug yet because no CPUs have EL2 support enabled.)

I can not anwer you to this question. The bug was found by our internal
corporate test suite with EL2 enabled.

>
>>>> -    /* 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);
>
>Yes, but this happens only after we have done the check:
>
>  if !IsZero(desc<47:40>) then
>     [take the AddressSize fault]
>
>which tells us that the descriptor field really is up to bit 48.
>We just haven't yet implemented the check in QEMU which will
>generate the AddressSize fault if the top bits are nonzero.
>(In contrast, in ARMv7 there really are only 40 bits there.)
>
>If you want to implement the AddressSize checks that's fine,
>but otherwise please leave this bit of the code alone.

You said me that my code is not correct, I have proved that it conforms
to the documentation.
It's a bit obfuscating when the doc explicitly says to take bits up to 39
from the descriptor, but in QEMU we take bits up to 47 relying on the check in
another part of the code, even if both ways are correct.

Nevertheless there is another bug in descaddrmask in QEMU.
>From ARM ARM:
baseaddress = desc<39:grainsize>:Zeros(grainsize);

But currently QEMU does:
descaddrmask = 0xfffffffff000ULL;

It assumes that grainsize is always 12, but it can be greater
in AArch64 translation regime.
The patch fixes the bug,
and completely conforms to the doc, doesn't it?