[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [Qemu-arm] [PATCH] target-arm: Fix descriptor address m

From: Sergey Sorokin
Subject: Re: [Qemu-devel] [Qemu-arm] [PATCH] target-arm: Fix descriptor address masking in ARM address translation
Date: Wed, 27 Apr 2016 00:39:36 +0300

26.04.2016, 19:35, "Tom Hanson" <address@hidden>:

On 03/21/2016 09:56 AM, Sergey Sorokin wrote:

 17.03.2016, 18:24, "Peter Maydell" <address@hidden>:
   On 17 March 2016 at 15:21, Sergey Sorokin <address@hidden> wrote:
    17.03.2016, 14:40, "Peter Maydell" <address@hidden>:
    On 13 March 2016 at 18:28, Sergey Sorokin <address@hidden> 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.

   -- PMM

 I mean in the patch.
 We need to fix lower bits in descaddrmask anyway.

 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.

There is a newer version of the patch. I'm sorry, I forgot to report here about it.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]