Re: [PATCH v10 1/9] KVM: x86/mmu: Use GENMASK_ULL() to define __PT_BASE_ADDR_MASK

[Binbin Wu <binbin.wu@linux.intel.com>]

On 8/17/2023 5:00 AM, Sean Christopherson wrote:
> On Wed, Jul 19, 2023, Binbin Wu wrote:
>> Use GENMASK_ULL() to define __PT_BASE_ADDR_MASK.
> Using GENMASK_ULL() is an opportunistic cleanup, it is not the main purpose for
> this patch.  The main purpose is to extract the maximum theoretical mask for guest
> MAXPHYADDR so that it can be used to strip bits from CR3.
>
> And rather than bury the actual use in "KVM: x86: Virtualize CR3.LAM_{U48,U57}",
> I think it makes sense to do the masking in this patch.  That change only becomes
> _necessary_ when LAM comes along, but it's completely valid without LAM.
>
> That will also provide a place to explain why we decided to unconditionally mask
> the pgd (it's harmless for 32-bit guests, querying 64-bit mode would be more
> expensive, and for EPT the mask isn't tied to guest mode).
OK.

> And it should also
> explain that using PT_BASE_ADDR_MASK would actually be wrong (PAE has 64-bit
> elements _except_ for CR3).
Hi Sean, I am not sure if I understand it correctly.
Do you mean when KVM shadows the page table of guest using 32-bit paging 
or PAE paging,
guest CR3 is or can be 32 bits for 32-bit paging or PAE paging, so that 
apply the mask to a 32-bit
value CR3 "would actually be wrong" ?


>
> E.g. end up with a shortlog for this patch along the lines of:
>
>    KVM: x86/mmu: Drop non-PA bits when getting GFN for guest's PGD
>
> and write the changelog accordingly.
>
>> No functional change intended.
>>
>> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
>> ---
>>   arch/x86/kvm/mmu/mmu_internal.h | 1 +
>>   arch/x86/kvm/mmu/paging_tmpl.h  | 2 +-
>>   2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/mmu/mmu_internal.h b/arch/x86/kvm/mmu/mmu_internal.h
>> index d39af5639ce9..7d2105432d66 100644
>> --- a/arch/x86/kvm/mmu/mmu_internal.h
>> +++ b/arch/x86/kvm/mmu/mmu_internal.h
>> @@ -21,6 +21,7 @@ extern bool dbg;
>>   #endif
>>   
>>   /* Page table builder macros common to shadow (host) PTEs and guest PTEs. */
>> +#define __PT_BASE_ADDR_MASK GENMASK_ULL(51, 12)
>>   #define __PT_LEVEL_SHIFT(level, bits_per_level)	\
>>   	(PAGE_SHIFT + ((level) - 1) * (bits_per_level))
>>   #define __PT_INDEX(address, level, bits_per_level) \
>> diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h
>> index 0662e0278e70..00c8193f5991 100644
>> --- a/arch/x86/kvm/mmu/paging_tmpl.h
>> +++ b/arch/x86/kvm/mmu/paging_tmpl.h
>> @@ -62,7 +62,7 @@
>>   #endif
>>   
>>   /* Common logic, but per-type values.  These also need to be undefined. */
>> -#define PT_BASE_ADDR_MASK	((pt_element_t)(((1ULL << 52) - 1) & ~(u64)(PAGE_SIZE-1)))
>> +#define PT_BASE_ADDR_MASK	((pt_element_t)__PT_BASE_ADDR_MASK)
>>   #define PT_LVL_ADDR_MASK(lvl)	__PT_LVL_ADDR_MASK(PT_BASE_ADDR_MASK, lvl, PT_LEVEL_BITS)
>>   #define PT_LVL_OFFSET_MASK(lvl)	__PT_LVL_OFFSET_MASK(PT_BASE_ADDR_MASK, lvl, PT_LEVEL_BITS)
>>   #define PT_INDEX(addr, lvl)	__PT_INDEX(addr, lvl, PT_LEVEL_BITS)
>> -- 
>> 2.25.1
>>