diff mbox series

[22/22] kvm: mmu: Don't clear write flooding count for direct roots

Message ID 20200925212302.3979661-23-bgardon@google.com (mailing list archive)
State New, archived
Headers show
Series Introduce the TDP MMU | expand

Commit Message

Ben Gardon Sept. 25, 2020, 9:23 p.m. UTC 
Direct roots don't have a write flooding count because the guest can't
affect that paging structure. Thus there's no need to clear the write
flooding count on a fast CR3 switch for direct roots.

Tested by running kvm-unit-tests and KVM selftests on an Intel Haswell
machine. This series introduced no new failures.

This series can be viewed in Gerrit at:
	https://linux-review.googlesource.com/c/virt/kvm/kvm/+/2538

Signed-off-by: Ben Gardon <bgardon@google.com>
---
 arch/x86/kvm/mmu/mmu.c     | 15 +++++++++++----
 arch/x86/kvm/mmu/tdp_mmu.c | 12 ++++++++++++
 arch/x86/kvm/mmu/tdp_mmu.h |  2 ++
 3 files changed, 25 insertions(+), 4 deletions(-)

Comments

Paolo Bonzini Sept. 26, 2020, 1:25 a.m. UTC | #1 
On 25/09/20 23:23, Ben Gardon wrote:
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index 42dde27decd75..c07831b0c73e1 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -124,6 +124,18 @@ static struct kvm_mmu_page *find_tdp_mmu_root_with_role(
>  	return NULL;
>  }
>  
> +hpa_t kvm_tdp_mmu_root_hpa_for_role(struct kvm *kvm,
> +				    union kvm_mmu_page_role role)
> +{
> +	struct kvm_mmu_page *root;
> +
> +	root = find_tdp_mmu_root_with_role(kvm, role);
> +	if (root)
> +		return __pa(root->spt);
> +
> +	return INVALID_PAGE;
> +}
> +
>  static union kvm_mmu_page_role page_role_for_level(struct kvm_vcpu *vcpu,
>  						   int level)
>  {
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.h b/arch/x86/kvm/mmu/tdp_mmu.h
> index cc0b7241975aa..2395ffa71bb05 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.h
> +++ b/arch/x86/kvm/mmu/tdp_mmu.h
> @@ -9,6 +9,8 @@ void kvm_mmu_init_tdp_mmu(struct kvm *kvm);
>  void kvm_mmu_uninit_tdp_mmu(struct kvm *kvm);
>  
>  bool is_tdp_mmu_root(struct kvm *kvm, hpa_t root);
> +hpa_t kvm_tdp_mmu_root_hpa_for_role(struct kvm *kvm,
> +				    union kvm_mmu_page_role role);
>  hpa_t kvm_tdp_mmu_get_vcpu_root_hpa(struct kvm_vcpu *vcpu);
>  void kvm_tdp_mmu_put_root_hpa(struct kvm *kvm, hpa_t root_hpa);
>  

Probably missing a piece since this code is not used and neither is the
new argument to is_root_usable.

I'm a bit confused by is_root_usable since there should be only one PGD
for the TDP MMU (the one for the root_mmu).

Paolo
Ben Gardon Oct. 5, 2020, 10:48 p.m. UTC | #2 
On Fri, Sep 25, 2020 at 6:25 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 25/09/20 23:23, Ben Gardon wrote:
> > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> > index 42dde27decd75..c07831b0c73e1 100644
> > --- a/arch/x86/kvm/mmu/tdp_mmu.c
> > +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> > @@ -124,6 +124,18 @@ static struct kvm_mmu_page *find_tdp_mmu_root_with_role(
> >       return NULL;
> >  }
> >
> > +hpa_t kvm_tdp_mmu_root_hpa_for_role(struct kvm *kvm,
> > +                                 union kvm_mmu_page_role role)
> > +{
> > +     struct kvm_mmu_page *root;
> > +
> > +     root = find_tdp_mmu_root_with_role(kvm, role);
> > +     if (root)
> > +             return __pa(root->spt);
> > +
> > +     return INVALID_PAGE;
> > +}
> > +
> >  static union kvm_mmu_page_role page_role_for_level(struct kvm_vcpu *vcpu,
> >                                                  int level)
> >  {
> > diff --git a/arch/x86/kvm/mmu/tdp_mmu.h b/arch/x86/kvm/mmu/tdp_mmu.h
> > index cc0b7241975aa..2395ffa71bb05 100644
> > --- a/arch/x86/kvm/mmu/tdp_mmu.h
> > +++ b/arch/x86/kvm/mmu/tdp_mmu.h
> > @@ -9,6 +9,8 @@ void kvm_mmu_init_tdp_mmu(struct kvm *kvm);
> >  void kvm_mmu_uninit_tdp_mmu(struct kvm *kvm);
> >
> >  bool is_tdp_mmu_root(struct kvm *kvm, hpa_t root);
> > +hpa_t kvm_tdp_mmu_root_hpa_for_role(struct kvm *kvm,
> > +                                 union kvm_mmu_page_role role);
> >  hpa_t kvm_tdp_mmu_get_vcpu_root_hpa(struct kvm_vcpu *vcpu);
> >  void kvm_tdp_mmu_put_root_hpa(struct kvm *kvm, hpa_t root_hpa);
> >
>
> Probably missing a piece since this code is not used and neither is the
> new argument to is_root_usable.
>
> I'm a bit confused by is_root_usable since there should be only one PGD
> for the TDP MMU (the one for the root_mmu).

*facepalm* sorry about that. This commit used to be titled "Implement
fast CR3 switching for the TDP MMU" but several refactors later most
of it was not useful. The only change that should be part of this
patch is the one to avoid clearing the write flooding counts. I must
have failed to revert the other changes.

>
> Paolo
>
Sean Christopherson Oct. 5, 2020, 11:44 p.m. UTC | #3 
On Mon, Oct 05, 2020 at 03:48:09PM -0700, Ben Gardon wrote:
> On Fri, Sep 25, 2020 at 6:25 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> > On 25/09/20 23:23, Ben Gardon wrote:
> > > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> > > index 42dde27decd75..c07831b0c73e1 100644
> > > --- a/arch/x86/kvm/mmu/tdp_mmu.c
> > > +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> > > @@ -124,6 +124,18 @@ static struct kvm_mmu_page *find_tdp_mmu_root_with_role(
> > >       return NULL;
> > >  }
> > >
> > > +hpa_t kvm_tdp_mmu_root_hpa_for_role(struct kvm *kvm,
> > > +                                 union kvm_mmu_page_role role)
> > > +{
> > > +     struct kvm_mmu_page *root;
> > > +
> > > +     root = find_tdp_mmu_root_with_role(kvm, role);
> > > +     if (root)
> > > +             return __pa(root->spt);
> > > +
> > > +     return INVALID_PAGE;
> > > +}
> > > +
> > >  static union kvm_mmu_page_role page_role_for_level(struct kvm_vcpu *vcpu,
> > >                                                  int level)
> > >  {
> > > diff --git a/arch/x86/kvm/mmu/tdp_mmu.h b/arch/x86/kvm/mmu/tdp_mmu.h
> > > index cc0b7241975aa..2395ffa71bb05 100644
> > > --- a/arch/x86/kvm/mmu/tdp_mmu.h
> > > +++ b/arch/x86/kvm/mmu/tdp_mmu.h
> > > @@ -9,6 +9,8 @@ void kvm_mmu_init_tdp_mmu(struct kvm *kvm);
> > >  void kvm_mmu_uninit_tdp_mmu(struct kvm *kvm);
> > >
> > >  bool is_tdp_mmu_root(struct kvm *kvm, hpa_t root);
> > > +hpa_t kvm_tdp_mmu_root_hpa_for_role(struct kvm *kvm,
> > > +                                 union kvm_mmu_page_role role);
> > >  hpa_t kvm_tdp_mmu_get_vcpu_root_hpa(struct kvm_vcpu *vcpu);
> > >  void kvm_tdp_mmu_put_root_hpa(struct kvm *kvm, hpa_t root_hpa);
> > >
> >
> > Probably missing a piece since this code is not used and neither is the
> > new argument to is_root_usable.
> >
> > I'm a bit confused by is_root_usable since there should be only one PGD
> > for the TDP MMU (the one for the root_mmu).
> 
> *facepalm* sorry about that. This commit used to be titled "Implement
> fast CR3 switching for the TDP MMU" but several refactors later most
> of it was not useful. The only change that should be part of this
> patch is the one to avoid clearing the write flooding counts. I must
> have failed to revert the other changes.

Tangentially related, isn't it possible to end up with multiple roots if the
MAXPHYSADDR is different between vCPUs?  I.e. if userspace coerces KVM into
using a mix of 4-level and 5-level EPT?

Not saying that's a remotely valid config...
Ben Gardon Oct. 6, 2020, 4:19 p.m. UTC | #4 
On Mon, Oct 5, 2020 at 5:07 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Mon, Oct 05, 2020 at 03:48:09PM -0700, Ben Gardon wrote:
> > On Fri, Sep 25, 2020 at 6:25 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
> > >
> > > On 25/09/20 23:23, Ben Gardon wrote:
> > > > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> > > > index 42dde27decd75..c07831b0c73e1 100644
> > > > --- a/arch/x86/kvm/mmu/tdp_mmu.c
> > > > +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> > > > @@ -124,6 +124,18 @@ static struct kvm_mmu_page *find_tdp_mmu_root_with_role(
> > > >       return NULL;
> > > >  }
> > > >
> > > > +hpa_t kvm_tdp_mmu_root_hpa_for_role(struct kvm *kvm,
> > > > +                                 union kvm_mmu_page_role role)
> > > > +{
> > > > +     struct kvm_mmu_page *root;
> > > > +
> > > > +     root = find_tdp_mmu_root_with_role(kvm, role);
> > > > +     if (root)
> > > > +             return __pa(root->spt);
> > > > +
> > > > +     return INVALID_PAGE;
> > > > +}
> > > > +
> > > >  static union kvm_mmu_page_role page_role_for_level(struct kvm_vcpu *vcpu,
> > > >                                                  int level)
> > > >  {
> > > > diff --git a/arch/x86/kvm/mmu/tdp_mmu.h b/arch/x86/kvm/mmu/tdp_mmu.h
> > > > index cc0b7241975aa..2395ffa71bb05 100644
> > > > --- a/arch/x86/kvm/mmu/tdp_mmu.h
> > > > +++ b/arch/x86/kvm/mmu/tdp_mmu.h
> > > > @@ -9,6 +9,8 @@ void kvm_mmu_init_tdp_mmu(struct kvm *kvm);
> > > >  void kvm_mmu_uninit_tdp_mmu(struct kvm *kvm);
> > > >
> > > >  bool is_tdp_mmu_root(struct kvm *kvm, hpa_t root);
> > > > +hpa_t kvm_tdp_mmu_root_hpa_for_role(struct kvm *kvm,
> > > > +                                 union kvm_mmu_page_role role);
> > > >  hpa_t kvm_tdp_mmu_get_vcpu_root_hpa(struct kvm_vcpu *vcpu);
> > > >  void kvm_tdp_mmu_put_root_hpa(struct kvm *kvm, hpa_t root_hpa);
> > > >
> > >
> > > Probably missing a piece since this code is not used and neither is the
> > > new argument to is_root_usable.
> > >
> > > I'm a bit confused by is_root_usable since there should be only one PGD
> > > for the TDP MMU (the one for the root_mmu).
> >
> > *facepalm* sorry about that. This commit used to be titled "Implement
> > fast CR3 switching for the TDP MMU" but several refactors later most
> > of it was not useful. The only change that should be part of this
> > patch is the one to avoid clearing the write flooding counts. I must
> > have failed to revert the other changes.
>
> Tangentially related, isn't it possible to end up with multiple roots if the
> MAXPHYSADDR is different between vCPUs?  I.e. if userspace coerces KVM into
> using a mix of 4-level and 5-level EPT?
>
> Not saying that's a remotely valid config...

We'll also end up with multiple TDP MMU roots if using SMM, and being
able to switch back and forth between "legacy/shadow MMU" roots and
TDP MMU roots improves nested performance since we can use the TDP MMU
for L1.
Since the TDP MMU associates struct kvm_mmu_pages with all its roots,
no special casing should be needed for root switching.
At one point in this patch set I was using some alternative data
structure to replace struct kvm_mmu_page for the TDP MMU, but I
abandoned that approach.
diff mbox series

Patch

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 0ce7720a72d4e..345c934fabf4c 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -4267,7 +4267,8 @@  static void nonpaging_init_context(struct kvm_vcpu *vcpu,
 	context->nx = false;
 }
 
-static inline bool is_root_usable(struct kvm_mmu_root_info *root, gpa_t pgd,
+static inline bool is_root_usable(struct kvm *kvm,
+				  struct kvm_mmu_root_info *root, gpa_t pgd,
 				  union kvm_mmu_page_role role)
 {
 	return (role.direct || pgd == root->pgd) &&
@@ -4293,13 +4294,13 @@  static bool cached_root_available(struct kvm_vcpu *vcpu, gpa_t new_pgd,
 	root.pgd = mmu->root_pgd;
 	root.hpa = mmu->root_hpa;
 
-	if (is_root_usable(&root, new_pgd, new_role))
+	if (is_root_usable(vcpu->kvm, &root, new_pgd, new_role))
 		return true;
 
 	for (i = 0; i < KVM_MMU_NUM_PREV_ROOTS; i++) {
 		swap(root, mmu->prev_roots[i]);
 
-		if (is_root_usable(&root, new_pgd, new_role))
+		if (is_root_usable(vcpu->kvm, &root, new_pgd, new_role))
 			break;
 	}
 
@@ -4356,7 +4357,13 @@  static void __kvm_mmu_new_pgd(struct kvm_vcpu *vcpu, gpa_t new_pgd,
 	 */
 	vcpu_clear_mmio_info(vcpu, MMIO_GVA_ANY);
 
-	__clear_sp_write_flooding_count(to_shadow_page(vcpu->arch.mmu->root_hpa));
+	/*
+	 * If this is a direct root page, it doesn't have a write flooding
+	 * count. Otherwise, clear the write flooding count.
+	 */
+	if (!new_role.direct)
+		__clear_sp_write_flooding_count(
+				to_shadow_page(vcpu->arch.mmu->root_hpa));
 }
 
 void kvm_mmu_new_pgd(struct kvm_vcpu *vcpu, gpa_t new_pgd, bool skip_tlb_flush,
diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index 42dde27decd75..c07831b0c73e1 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -124,6 +124,18 @@  static struct kvm_mmu_page *find_tdp_mmu_root_with_role(
 	return NULL;
 }
 
+hpa_t kvm_tdp_mmu_root_hpa_for_role(struct kvm *kvm,
+				    union kvm_mmu_page_role role)
+{
+	struct kvm_mmu_page *root;
+
+	root = find_tdp_mmu_root_with_role(kvm, role);
+	if (root)
+		return __pa(root->spt);
+
+	return INVALID_PAGE;
+}
+
 static union kvm_mmu_page_role page_role_for_level(struct kvm_vcpu *vcpu,
 						   int level)
 {
diff --git a/arch/x86/kvm/mmu/tdp_mmu.h b/arch/x86/kvm/mmu/tdp_mmu.h
index cc0b7241975aa..2395ffa71bb05 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.h
+++ b/arch/x86/kvm/mmu/tdp_mmu.h
@@ -9,6 +9,8 @@  void kvm_mmu_init_tdp_mmu(struct kvm *kvm);
 void kvm_mmu_uninit_tdp_mmu(struct kvm *kvm);
 
 bool is_tdp_mmu_root(struct kvm *kvm, hpa_t root);
+hpa_t kvm_tdp_mmu_root_hpa_for_role(struct kvm *kvm,
+				    union kvm_mmu_page_role role);
 hpa_t kvm_tdp_mmu_get_vcpu_root_hpa(struct kvm_vcpu *vcpu);
 void kvm_tdp_mmu_put_root_hpa(struct kvm *kvm, hpa_t root_hpa);