[PATCH] arm64: fix VTTBR_BADDR_MASK

[PATCH] arm64: fix VTTBR_BADDR_MASK

[Joel Schopp]

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)
 

[PATCH] arm64: fix VTTBR_BADDR_MASK

[Christoffer Dall]

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.

So while all the MSB in the physical address range of your system should
be used, we still need to check that the pgd we allocate is actually
aligned properly, because writing any bits in [x-1:0} as non-zero can
potentially cause unpredicted behavior and the translation walk results
can be corrupted.

So, you do need to use a proper shift, I'm fine with fixing it for the
assumed 40-bit physical address space for now, but you need to figure
out the right value of 'x', for the BADDR[47:x], and you should change
the code in update_vttbr() as part of this patch, something like
BUG_ON(pgd_phys & ~VTTBR_BADDR_MASK) and make sure the pgd is allocated
at the right alignment, or ensure that the PGD is large enough that we
can just mask off the lower bits and still contain the required number
of entries.

Thanks,
-Christoffer

[PATCH] arm64: fix VTTBR_BADDR_MASK

[Joel Schopp]

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 ;)

[PATCH] arm64: fix VTTBR_BADDR_MASK

[Joel Schopp]

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.

[PATCH] arm64: fix VTTBR_BADDR_MASK

[Christoffer Dall]

On Thu, Jul 10, 2014 at 04:51:06PM -0500, Joel Schopp wrote:
> 
> 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.

Thanks,
-Christoffer