| Message ID | 20230719144131.29052-2-binbin.wu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Linear Address Masking (LAM) KVM Enabling | expand |
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 >
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 >>
On Mon, Aug 28, 2023, Binbin Wu wrote: > > > 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" ? It would be technically wrong for PAE paging, as the PTEs themselves are 64 bits, i.e. PT_BASE_ADDR_MASK would be 51:12, but CR3 is still only 32 bits. Wouldn't matter in practice, but I think it's worth calling out that going out of our way to use PT_BASE_ADDR_MASK wouldn't actually "fix" anything.
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)
Use GENMASK_ULL() to define __PT_BASE_ADDR_MASK. 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(-)