[v2,3/6] KVM: x86/mmu: only zap EPT when guest MTRR changes

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

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