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

From: Sergey Sorokin <afarallax@yandex.ru>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm <qemu-arm@nongnu.org>, QEMU Developers <qemu-devel@nongnu.org>
Subject: Re: [Qemu-devel] [PATCH] target-arm: Fix descriptor address masking in ARM address translation
Date: Mon, 21 Mar 2016 18:56:19 +0300	[thread overview]
Message-ID: <1362381458575779@web6j.yandex.ru> (raw)
In-Reply-To: <CAFEAcA_+znhWyJFWC3eMi8JiZw-nb2MyK_+uD6Fww76reinoeQ@mail.gmail.com>

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.