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

[Tom Hanson <thomas.hanson@linaro.org>]

On 03/21/2016 09:56 AM, Sergey Sorokin wrote:
> 17.03.2016, 18:24, "Peter Maydell" <peter.maydell@linaro.org>:
>>   On 17 March 2016 at 15:21, Sergey Sorokin <afarallax@yandex.ru> wrote:
>>>    17.03.2016, 14:40, "Peter Maydell" <peter.maydell@linaro.org>:
>>>>    On 13 March 2016 at 18:28, Sergey Sorokin <afarallax@yandex.ru> wrote:
>>>>>>    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.
>>>>
>>>>    The way the code in QEMU is structured is that we extract the
>>>>    descriptor field in one go and then will operate on it
>>>>    (checking for need to AddressSize fault, etc) as a second
>>>>    action. The field descriptors themselves are the sizes I said.
>>>
>>>    Well, may be it's enough just to change this comment as you intend:
>>>
>>>>>    - /* 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.
>>>>>          */
>>
>>   The comment is correct as it stands.
>>
>>   thanks
>>   -- PMM
>
> I mean in the patch.
> We need to fix lower bits in descaddrmask anyway.
> So:
>
> I could describe in the comment, that the descriptor field is up to bit 47 for ARMv8 (as long as you want it),
> but we use the descaddrmask up to bit 39 for AArch32,
> because we don't need other bits in that case to construct next descriptor address.
> It is clearly described in the ARM pseudo-code.
> Why should we keep in the mask bits from 40 up to 47 if we don't need them? Even if they are all zeroes.
> It is a bit obfuscating, as I said.
>
  I agree with Peter.  The original comment is correct.

Looking at the TLBRecord AArch32.TranslationTableWalkLD pseudocode, it is treating the AArch32 address as 48 bits long.  For example:
     if !IsZero(baseregister<47:40>) then
         level = 0;
         result.addrdesc.fault = AArch32.AddressSizeFault(ipaddress, domain, level, acctype, iswrite,
                                                          secondstage, s2fs1walk);
     return result;

This requires that an AArch32 address have specific values up through bit 47.