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

From: Sean Christopherson <seanjc@google.com>
To: Binbin Wu <binbin.wu@linux.intel.com>
Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
	pbonzini@redhat.com, chao.gao@intel.com, kai.huang@intel.com,
	David.Laight@aculab.com, robert.hu@linux.intel.com,
	guang.zeng@intel.com
Subject: Re: [PATCH v10 1/9] KVM: x86/mmu: Use GENMASK_ULL() to define __PT_BASE_ADDR_MASK
Date: Wed, 16 Aug 2023 14:00:23 -0700	[thread overview]
Message-ID: <ZN0454peMb3z/0Bg@google.com> (raw)
In-Reply-To: <20230719144131.29052-2-binbin.wu@linux.intel.com>

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).  And it should also
explain that using PT_BASE_ADDR_MASK would actually be wrong (PAE has 64-bit
elements _except_ for CR3).

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
> 