All of lore.kernel.org
 help / color / mirror / Atom feed
From: joel.schopp@amd.com (Joel Schopp)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] arm64: fix VTTBR_BADDR_MASK
Date: Thu, 10 Jul 2014 16:51:06 -0500	[thread overview]
Message-ID: <53BF0ACA.70403@amd.com> (raw)
In-Reply-To: <53BEFF7E.5010907@amd.com>


On 07/10/2014 04:02 PM, Joel Schopp wrote:
> On 07/10/2014 03:25 PM, Christoffer Dall wrote:
>> On Wed, Jul 09, 2014 at 11:17:04AM -0500, Joel Schopp wrote:
>>> The current calculation for VTTBR_BADDR_MASK masks only 39 bits and not
>>> all 40 bits.  That last bit is important as some systems allocate
>>> from near the top of the available address space.
>>>
>>> This patch is necessary to run KVM on an aarch64 SOC I have been testing.
>>>
>>> Signed-off-by: Joel Schopp <joel.schopp@amd.com>
>>> ---
>>>  arch/arm64/include/asm/kvm_arm.h |    2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
>>> index 3d69030..b39e93f 100644
>>> --- a/arch/arm64/include/asm/kvm_arm.h
>>> +++ b/arch/arm64/include/asm/kvm_arm.h
>>> @@ -148,7 +148,7 @@
>>>  #endif
>>>  
>>>  #define VTTBR_BADDR_SHIFT (VTTBR_X - 1)
>>> -#define VTTBR_BADDR_MASK  (((1LLU << (40 - VTTBR_X)) - 1) << VTTBR_BADDR_SHIFT)
>>> +#define VTTBR_BADDR_MASK  (0xffffffffffLLU)              /* bits 0-39 */
>>>  #define VTTBR_VMID_SHIFT  (48LLU)
>>>  #define VTTBR_VMID_MASK	  (0xffLLU << VTTBR_VMID_SHIFT)
>>>  
>>>
>> While this is obviously fixing a bug, it doesn't feel like the right
>> short-term fix.  I'll have to go back and read the definitions of x in
>> BADDR[47:x] for VTTBR_EL2 exactly again, but the intended use of
>> VTTBR_BADDR_MASK (and the only occurence of it in C-code) is to deal
>> with alignment of the allocated pgd.
> I think there is some confusion.  Before VTTBR_BADDR_MASK always
> evaluated to 0x7fffffffffLLU, after the change it always evaluates to
> 0xffffffffffLLU
>
> Neither before nor after the patch is it dealing with alignment.  Any
> bits it throws away (bits 40-47) are most significant not least significant.
>
> I could have rewritten the macro like:
>
> #define VTTBR_BADDR_MASK  (((1LLU << (40 - VTTBR_X + 1)) - 1) << VTTBR_BADDR_SHIFT)
>
> to correct the bug but it's my opinion that the existing code is quite
> obfuscated which is how the bug happened in the first place.  It seemed
> easier to just actually mask the bits in a straightforward and easy to
> understand manner.  I even added a comment so nobody has to count the fs ;)
>
I hate to reply to my own email correcting myself.  But you were
correct.  I will fix and resend a v2.

WARNING: multiple messages have this Message-ID (diff)
From: Joel Schopp <joel.schopp@amd.com>
To: Christoffer Dall <christoffer.dall@linaro.org>
Cc: kvmarm@lists.cs.columbia.edu,
	linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org
Subject: Re: [PATCH] arm64: fix VTTBR_BADDR_MASK
Date: Thu, 10 Jul 2014 16:51:06 -0500	[thread overview]
Message-ID: <53BF0ACA.70403@amd.com> (raw)
In-Reply-To: <53BEFF7E.5010907@amd.com>


On 07/10/2014 04:02 PM, Joel Schopp wrote:
> On 07/10/2014 03:25 PM, Christoffer Dall wrote:
>> On Wed, Jul 09, 2014 at 11:17:04AM -0500, Joel Schopp wrote:
>>> The current calculation for VTTBR_BADDR_MASK masks only 39 bits and not
>>> all 40 bits.  That last bit is important as some systems allocate
>>> from near the top of the available address space.
>>>
>>> This patch is necessary to run KVM on an aarch64 SOC I have been testing.
>>>
>>> Signed-off-by: Joel Schopp <joel.schopp@amd.com>
>>> ---
>>>  arch/arm64/include/asm/kvm_arm.h |    2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
>>> index 3d69030..b39e93f 100644
>>> --- a/arch/arm64/include/asm/kvm_arm.h
>>> +++ b/arch/arm64/include/asm/kvm_arm.h
>>> @@ -148,7 +148,7 @@
>>>  #endif
>>>  
>>>  #define VTTBR_BADDR_SHIFT (VTTBR_X - 1)
>>> -#define VTTBR_BADDR_MASK  (((1LLU << (40 - VTTBR_X)) - 1) << VTTBR_BADDR_SHIFT)
>>> +#define VTTBR_BADDR_MASK  (0xffffffffffLLU)              /* bits 0-39 */
>>>  #define VTTBR_VMID_SHIFT  (48LLU)
>>>  #define VTTBR_VMID_MASK	  (0xffLLU << VTTBR_VMID_SHIFT)
>>>  
>>>
>> While this is obviously fixing a bug, it doesn't feel like the right
>> short-term fix.  I'll have to go back and read the definitions of x in
>> BADDR[47:x] for VTTBR_EL2 exactly again, but the intended use of
>> VTTBR_BADDR_MASK (and the only occurence of it in C-code) is to deal
>> with alignment of the allocated pgd.
> I think there is some confusion.  Before VTTBR_BADDR_MASK always
> evaluated to 0x7fffffffffLLU, after the change it always evaluates to
> 0xffffffffffLLU
>
> Neither before nor after the patch is it dealing with alignment.  Any
> bits it throws away (bits 40-47) are most significant not least significant.
>
> I could have rewritten the macro like:
>
> #define VTTBR_BADDR_MASK  (((1LLU << (40 - VTTBR_X + 1)) - 1) << VTTBR_BADDR_SHIFT)
>
> to correct the bug but it's my opinion that the existing code is quite
> obfuscated which is how the bug happened in the first place.  It seemed
> easier to just actually mask the bits in a straightforward and easy to
> understand manner.  I even added a comment so nobody has to count the fs ;)
>
I hate to reply to my own email correcting myself.  But you were
correct.  I will fix and resend a v2.

  reply	other threads:[~2014-07-10 21:51 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-07-09 16:17 [PATCH] arm64: fix VTTBR_BADDR_MASK Joel Schopp
2014-07-09 16:17 ` Joel Schopp
2014-07-10 20:25 ` Christoffer Dall
2014-07-10 20:25   ` Christoffer Dall
2014-07-10 21:02   ` Joel Schopp
2014-07-10 21:02     ` Joel Schopp
2014-07-10 21:51     ` Joel Schopp [this message]
2014-07-10 21:51       ` Joel Schopp
2014-07-11 10:38       ` Christoffer Dall
2014-07-11 10:38         ` Christoffer Dall

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=53BF0ACA.70403@amd.com \
    --to=joel.schopp@amd.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.