Re: [PATCH] arm64: fix VTTBR_BADDR_MASK

[Joel Schopp <joel.schopp@amd.com>]

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