| Message ID | 20230509135143.1721-1-yan.y.zhao@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86/mmu: refine memtype related mmu zap | expand |
On Tue, May 09, 2023 at 09:51:43PM +0800, Yan Zhao wrote: >Call new helper kvm_zap_gfn_for_memtype() to skip zap mmu if EPT is not >enabled. > >When guest MTRR changes and it's desired to zap TDP entries to remove >stale mappings, only do it when EPT is enabled, because only memory type >of EPT leaf is affected by guest MTRR with noncoherent DMA present. > >Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> >--- > arch/x86/kvm/mtrr.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > >diff --git a/arch/x86/kvm/mtrr.c b/arch/x86/kvm/mtrr.c >index 9fac1ec03463..62ebb9978156 100644 >--- a/arch/x86/kvm/mtrr.c >+++ b/arch/x86/kvm/mtrr.c >@@ -330,7 +330,7 @@ static void update_mtrr(struct kvm_vcpu *vcpu, u32 msr) > var_mtrr_range(&mtrr_state->var_ranges[index], &start, &end); > } > >- kvm_zap_gfn_range(vcpu->kvm, gpa_to_gfn(start), gpa_to_gfn(end)); >+ kvm_zap_gfn_for_memtype(vcpu->kvm, gpa_to_gfn(start), gpa_to_gfn(end)); I am wondering if the check of shadow_memtype_mask (now inside the kvm_zap_gfn_for_memtype()) should be moved to the beginning of update_mtrr(). Because if EPT isn't enabled, computing @start/@end is useless and can be skipped. > } > > static bool var_mtrr_range_is_valid(struct kvm_mtrr_range *range) >-- >2.17.1 >
On Wed, May 10, 2023 at 01:39:25PM +0800, Chao Gao wrote: > On Tue, May 09, 2023 at 09:51:43PM +0800, Yan Zhao wrote: > >Call new helper kvm_zap_gfn_for_memtype() to skip zap mmu if EPT is not > >enabled. > > > >When guest MTRR changes and it's desired to zap TDP entries to remove > >stale mappings, only do it when EPT is enabled, because only memory type > >of EPT leaf is affected by guest MTRR with noncoherent DMA present. > > > >Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> > >--- > > arch/x86/kvm/mtrr.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > >diff --git a/arch/x86/kvm/mtrr.c b/arch/x86/kvm/mtrr.c > >index 9fac1ec03463..62ebb9978156 100644 > >--- a/arch/x86/kvm/mtrr.c > >+++ b/arch/x86/kvm/mtrr.c > >@@ -330,7 +330,7 @@ static void update_mtrr(struct kvm_vcpu *vcpu, u32 msr) > > var_mtrr_range(&mtrr_state->var_ranges[index], &start, &end); > > } > > > >- kvm_zap_gfn_range(vcpu->kvm, gpa_to_gfn(start), gpa_to_gfn(end)); > >+ kvm_zap_gfn_for_memtype(vcpu->kvm, gpa_to_gfn(start), gpa_to_gfn(end)); > > I am wondering if the check of shadow_memtype_mask (now inside the > kvm_zap_gfn_for_memtype()) should be moved to the beginning of update_mtrr(). > Because if EPT isn't enabled, computing @start/@end is useless and can be > skipped. No, shadow_memtype_mask is not accessible in mtrr.c. It's better to confine it in mmu related files, e.g. mmu/mmu.c, mmu/spte.c > > > } > > > > static bool var_mtrr_range_is_valid(struct kvm_mtrr_range *range) > >-- > >2.17.1 > >
On Wed, 2023-05-10 at 16:00 +0800, Yan Zhao wrote: > On Wed, May 10, 2023 at 01:39:25PM +0800, Chao Gao wrote: > > On Tue, May 09, 2023 at 09:51:43PM +0800, Yan Zhao wrote: > > > Call new helper kvm_zap_gfn_for_memtype() to skip zap mmu if EPT is not > > > enabled. > > > > > > When guest MTRR changes and it's desired to zap TDP entries to remove > > > stale mappings, only do it when EPT is enabled, because only memory type > > > of EPT leaf is affected by guest MTRR with noncoherent DMA present. > > > > > > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> > > > --- > > > arch/x86/kvm/mtrr.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/arch/x86/kvm/mtrr.c b/arch/x86/kvm/mtrr.c > > > index 9fac1ec03463..62ebb9978156 100644 > > > --- a/arch/x86/kvm/mtrr.c > > > +++ b/arch/x86/kvm/mtrr.c > > > @@ -330,7 +330,7 @@ static void update_mtrr(struct kvm_vcpu *vcpu, u32 msr) > > > var_mtrr_range(&mtrr_state->var_ranges[index], &start, &end); > > > } > > > > > > - kvm_zap_gfn_range(vcpu->kvm, gpa_to_gfn(start), gpa_to_gfn(end)); > > > + kvm_zap_gfn_for_memtype(vcpu->kvm, gpa_to_gfn(start), gpa_to_gfn(end)); > > > > I am wondering if the check of shadow_memtype_mask (now inside the > > kvm_zap_gfn_for_memtype()) should be moved to the beginning of update_mtrr(). > > Because if EPT isn't enabled, computing @start/@end is useless and can be > > skipped. > > No, shadow_memtype_mask is not accessible in mtrr.c. > It's better to confine it in mmu related files, e.g. mmu/mmu.c, > mmu/spte.c > No, I think it should be shadow_memtype_mask. Conceptually, after commit 38bf9d7bf277 ("KVM: x86/mmu: Add shadow mask for effective host MTRR memtype"), I believe in update_mtrr() the check: if (msr == MSR_IA32_CR_PAT || !tdp_enabled || !kvm_arch_has_noncoherent_dma(vcpu->kvm)) return; should be: if (msr == MSR_IA32_CR_PAT || !shadow_memtype_mask || !kvm_arch_has_noncoherent_dma(vcpu->kvm)) return; Because the intention of 'shadow_memtype_mask' is to use it as a dedicated variable to represent hardware has capability to override the host MTRRs (which is the case for EPT), instead of relying on TDP being enabled. That being said, to me it's more reasonable to have a separate patch to replace the '!tdp_enabled' check with '!shadow_memtype_mask' in update_mtrr(), perhaps with a Fixes tag for commit 38bf9d7bf277. The kvm_zap_gfn_range() should be remain unchanged. For the problem that shadow_memtype_mask isn't accessible in mtrr.c I think you can include "mmu/spte.h"? >
On Wed, May 10, 2023 at 06:54:51PM +0800, Huang, Kai wrote: > On Wed, 2023-05-10 at 16:00 +0800, Yan Zhao wrote: > > On Wed, May 10, 2023 at 01:39:25PM +0800, Chao Gao wrote: > > > On Tue, May 09, 2023 at 09:51:43PM +0800, Yan Zhao wrote: > > > > Call new helper kvm_zap_gfn_for_memtype() to skip zap mmu if EPT is not > > > > enabled. > > > > > > > > When guest MTRR changes and it's desired to zap TDP entries to remove > > > > stale mappings, only do it when EPT is enabled, because only memory type > > > > of EPT leaf is affected by guest MTRR with noncoherent DMA present. > > > > > > > > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> > > > > --- > > > > arch/x86/kvm/mtrr.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/arch/x86/kvm/mtrr.c b/arch/x86/kvm/mtrr.c > > > > index 9fac1ec03463..62ebb9978156 100644 > > > > --- a/arch/x86/kvm/mtrr.c > > > > +++ b/arch/x86/kvm/mtrr.c > > > > @@ -330,7 +330,7 @@ static void update_mtrr(struct kvm_vcpu *vcpu, u32 msr) > > > > var_mtrr_range(&mtrr_state->var_ranges[index], &start, &end); > > > > } > > > > > > > > - kvm_zap_gfn_range(vcpu->kvm, gpa_to_gfn(start), gpa_to_gfn(end)); > > > > + kvm_zap_gfn_for_memtype(vcpu->kvm, gpa_to_gfn(start), gpa_to_gfn(end)); > > > > > > I am wondering if the check of shadow_memtype_mask (now inside the > > > kvm_zap_gfn_for_memtype()) should be moved to the beginning of update_mtrr(). > > > Because if EPT isn't enabled, computing @start/@end is useless and can be > > > skipped. > > > > No, shadow_memtype_mask is not accessible in mtrr.c. > > It's better to confine it in mmu related files, e.g. mmu/mmu.c, > > mmu/spte.c > > > > No, I think it should be shadow_memtype_mask. > > Conceptually, after commit 38bf9d7bf277 ("KVM: x86/mmu: Add shadow mask for > effective host MTRR memtype"), I believe in update_mtrr() the check: > > if (msr == MSR_IA32_CR_PAT || !tdp_enabled || > !kvm_arch_has_noncoherent_dma(vcpu->kvm)) > return; > > should be: > > if (msr == MSR_IA32_CR_PAT || !shadow_memtype_mask || > !kvm_arch_has_noncoherent_dma(vcpu->kvm)) > return; > > Because the intention of 'shadow_memtype_mask' is to use it as a dedicated > variable to represent hardware has capability to override the host MTRRs (which > is the case for EPT), instead of relying on TDP being enabled. > > That being said, to me it's more reasonable to have a separate patch to replace > the '!tdp_enabled' check with '!shadow_memtype_mask' in update_mtrr(), perhaps > with a Fixes tag for commit 38bf9d7bf277. > > The kvm_zap_gfn_range() should be remain unchanged. > > For the problem that shadow_memtype_mask isn't accessible in mtrr.c I think you > can include "mmu/spte.h"? > I agree shadow_memtype_mask is the right value to check but it's indeed internal to kvm mmu. Including "mmu/spte.h" will further include "mmu/mmu_internal.h" What about introduce kvm_mmu_memtye_effective() instead as below? diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h index a04577afbc71..173960b1827d 100644 --- a/arch/x86/kvm/mmu.h +++ b/arch/x86/kvm/mmu.h @@ -254,6 +254,8 @@ static inline bool kvm_shadow_root_allocated(struct kvm *kvm) return smp_load_acquire(&kvm->arch.shadow_root_allocated); } +bool kvm_mmu_memtye_effective(struct kvm *kvm); + #ifdef CONFIG_X86_64 extern bool tdp_mmu_enabled; #else diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index 2706754794d1..ff7752d158d7 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -6288,6 +6288,10 @@ void kvm_zap_gfn_for_memtype(struct kvm *kvm, gfn_t gfn_start, gfn_t gfn_end) kvm_zap_gfn_range(kvm, gpa_to_gfn(0), gpa_to_gfn(~0ULL)); } +bool kvm_mmu_memtye_effective(struct kvm *kvm) +{ + return shadow_memtype_mask; +} /* * Invalidate (zap) SPTEs that cover GFNs from gfn_start and up to gfn_end * (not including it) diff --git a/arch/x86/kvm/mtrr.c b/arch/x86/kvm/mtrr.c index 5ce5bc0c4971..b6bd147d22a0 100644 --- a/arch/x86/kvm/mtrr.c +++ b/arch/x86/kvm/mtrr.c @@ -313,7 +313,7 @@ static void update_mtrr(struct kvm_vcpu *vcpu, u32 msr) int cnt; unsigned long long all; - if (msr == MSR_IA32_CR_PAT || !tdp_enabled || + if (msr == MSR_IA32_CR_PAT || !kvm_mmu_memtye_effective(vcpu->kvm) || !kvm_arch_has_noncoherent_dma(vcpu->kvm)) return;
On Thu, May 11, 2023 at 10:42:13AM +0800, Huang, Kai wrote: > > > > > > > I agree shadow_memtype_mask is the right value to check but it's indeed > > internal to kvm mmu. > > Including "mmu/spte.h" will further include "mmu/mmu_internal.h" > > > > What about introduce kvm_mmu_memtye_effective() instead as below? > > > > [...] > > > > > +bool kvm_mmu_memtye_effective(struct kvm *kvm) > > +{ > > + return shadow_memtype_mask; > > +} > > I don't think it's a good name. shadow_memtype_mask doesn't mean any actual > effective memtype, but just represent those bits that KVM can manipulate to get > an effective memtype for the guest access. > > Instead, it should represent the hardware capability to be able to emulate > guest's MTRR independent from host's MTRR. So, perhaps something like: > > bool kvm_mmu_guest_mtrr_emulatable(); What about kvm_mmu_cap_effective_guest_mtrr()? Guest MTRR is always emulated with vMTRR. > > It's better if Sean can provide input here. > > (Btw, off this topic, I think it's even more reasonable to make > shadow_memtype_mask only be meaningful when tdp_enabled is true, because the > capability to override host MTRR and emulate guest MTRR should be only available > when secondary MMU is turned on). >
> > > > I agree shadow_memtype_mask is the right value to check but it's indeed > internal to kvm mmu. > Including "mmu/spte.h" will further include "mmu/mmu_internal.h" > > What about introduce kvm_mmu_memtye_effective() instead as below? > [...] > > +bool kvm_mmu_memtye_effective(struct kvm *kvm) > +{ > + return shadow_memtype_mask; > +} I don't think it's a good name. shadow_memtype_mask doesn't mean any actual effective memtype, but just represent those bits that KVM can manipulate to get an effective memtype for the guest access. Instead, it should represent the hardware capability to be able to emulate guest's MTRR independent from host's MTRR. So, perhaps something like: bool kvm_mmu_guest_mtrr_emulatable(); It's better if Sean can provide input here. (Btw, off this topic, I think it's even more reasonable to make shadow_memtype_mask only be meaningful when tdp_enabled is true, because the capability to override host MTRR and emulate guest MTRR should be only available when secondary MMU is turned on).
On Thu, 2023-05-11 at 10:31 +0800, Zhao, Yan Y wrote: > On Thu, May 11, 2023 at 10:42:13AM +0800, Huang, Kai wrote: > > > > > > > > > > I agree shadow_memtype_mask is the right value to check but it's indeed > > > internal to kvm mmu. > > > Including "mmu/spte.h" will further include "mmu/mmu_internal.h" > > > > > > What about introduce kvm_mmu_memtye_effective() instead as below? > > > > > > > [...] > > > > > > > > +bool kvm_mmu_memtye_effective(struct kvm *kvm) > > > +{ > > > + return shadow_memtype_mask; > > > +} > > > > I don't think it's a good name. shadow_memtype_mask doesn't mean any actual > > effective memtype, but just represent those bits that KVM can manipulate to get > > an effective memtype for the guest access. > > > > Instead, it should represent the hardware capability to be able to emulate > > guest's MTRR independent from host's MTRR. So, perhaps something like: > > > > bool kvm_mmu_guest_mtrr_emulatable(); > > What about kvm_mmu_cap_effective_guest_mtrr()? > Guest MTRR is always emulated with vMTRR. Fine to me, but again it's better if Sean can provide input. > > > > > It's better if Sean can provide input here. > > > > (Btw, off this topic, I think it's even more reasonable to make > > shadow_memtype_mask only be meaningful when tdp_enabled is true, because the > > capability to override host MTRR and emulate guest MTRR should be only available > > when secondary MMU is turned on). > >
diff --git a/arch/x86/kvm/mtrr.c b/arch/x86/kvm/mtrr.c index 9fac1ec03463..62ebb9978156 100644 --- a/arch/x86/kvm/mtrr.c +++ b/arch/x86/kvm/mtrr.c @@ -330,7 +330,7 @@ static void update_mtrr(struct kvm_vcpu *vcpu, u32 msr) var_mtrr_range(&mtrr_state->var_ranges[index], &start, &end); } - kvm_zap_gfn_range(vcpu->kvm, gpa_to_gfn(start), gpa_to_gfn(end)); + kvm_zap_gfn_for_memtype(vcpu->kvm, gpa_to_gfn(start), gpa_to_gfn(end)); } static bool var_mtrr_range_is_valid(struct kvm_mtrr_range *range)
Call new helper kvm_zap_gfn_for_memtype() to skip zap mmu if EPT is not enabled. When guest MTRR changes and it's desired to zap TDP entries to remove stale mappings, only do it when EPT is enabled, because only memory type of EPT leaf is affected by guest MTRR with noncoherent DMA present. Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> --- arch/x86/kvm/mtrr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)