KVM: Extend Eager Page Splitting to the shadow MMU

Commit fb58a9c345f6 ("KVM: x86/mmu: Optimize MMU page cache lookup for fully direct MMUs") skipped the unsync checks and write flood clearing for full direct MMUs. We can extend this further to skip the checks for all direct shadow pages. Direct shadow pages in indirect MMUs (i.e. shadow paging) are used when shadowing a guest huge page with smaller pages. Such direct shadow pages, like their counterparts in fully direct MMUs, are never marked unsynced or have a non-zero write-flooding count.

Checking sp->role.direct also generates better code than checking direct_map because, due to register pressure, direct_map has to get shoved onto the stack and then pulled back off.

No functional change intended.

Reviewed-by Sean Christopherson <[email protected]>

Reviewed-by Peter Xu <[email protected]>

Signed-off-by David Matlack <[email protected]>

The parameter "direct" can either be true or false, and all of the callers pass in a bool variable or true/false literal, so just use the type bool.

No functional change intended.

Reviewed-by Sean Christopherson <[email protected]>

Signed-off-by David Matlack <[email protected]>

---

Instead of computing the shadow page role from scratch for every new page, derive most of the information from the parent shadow page. This avoids redundant calculations and reduces the number of parameters to kvm_mmu_get_page().

Preemptively split out the role calculation to a separate function for use in a following commit.

No functional change intended.

Reviewed-by Peter Xu <[email protected]>

Signed-off-by David Matlack <[email protected]>

---
 arch/x86/kvm/mmu/mmu.c         | 96 +++++++++++++++++++++++-----------
 arch/x86/kvm/mmu/paging_tmpl.h |  9 ++--
 2 files changed, 71 insertions(+), 34 deletions(-)

arch/x86/kvm/mmu/mmu.c +2 -8

index dc20eccd6a77..4249a771818b 100644
@@ -2021,31 +2021,15 @@ static void clear_sp_write_flooding_count(u64 *spte)
 	__clear_sp_write_flooding_count(sptep_to_sp(spte));
 }
 
-static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
-					     gfn_t gfn,
-					     gva_t gaddr,
-					     unsigned level,
-					     bool direct,
-					     unsigned int access)
+static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, gfn_t gfn,
+					     union kvm_mmu_page_role role)
 {
-	union kvm_mmu_page_role role;
 	struct hlist_head *sp_list;
-	unsigned quadrant;
 	struct kvm_mmu_page *sp;
 	int ret;
 	int collisions = 0;
 	LIST_HEAD(invalid_list);
 

arch/x86/kvm/mmu/mmu.c +1 -1

 	sp_list = &vcpu->kvm->arch.mmu_page_hash[kvm_page_table_hashfn(gfn)];
 	for_each_valid_sp(vcpu->kvm, sp, sp_list) {
 		if (sp->gfn != gfn) {

@@ -2063,7 +2047,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
 			 * Unsync pages must not be left as is, because the new
 			 * upper-level page will be write-protected.
 			 */
-			if (level > PG_LEVEL_4K && sp->unsync)
+			if (role.level > PG_LEVEL_4K && sp->unsync)
 				kvm_mmu_prepare_zap_page(vcpu->kvm, sp,
 							 &invalid_list);
 			continue;

arch/x86/kvm/mmu/mmu.c +2 -1

@@ -3310,12 +3338,21 @@ static int mmu_check_root(struct kvm_vcpu *vcpu, gfn_t root_gfn)
 	return ret;
 }
 
-static hpa_t mmu_alloc_root(struct kvm_vcpu *vcpu, gfn_t gfn, gva_t gva,
+static hpa_t mmu_alloc_root(struct kvm_vcpu *vcpu, gfn_t gfn, int quadrant,
 			    u8 level, bool direct)
 {
+	union kvm_mmu_page_role role;

Quoted 42 lines · David Matlack

Instead of computing the shadow page role from scratch for every new page, derive most of the information from the parent shadow page. This avoids redundant calculations and reduces the number of parameters to kvm_mmu_get_page().

Preemptively split out the role calculation to a separate function for use in a following commit.

No functional change intended.

Reviewed-by Peter Xu <[email protected]>

Signed-off-by David Matlack <[email protected]>

---
 arch/x86/kvm/mmu/mmu.c         | 96 +++++++++++++++++++++++-----------
 arch/x86/kvm/mmu/paging_tmpl.h |  9 ++--
 2 files changed, 71 insertions(+), 34 deletions(-)

arch/x86/kvm/mmu/mmu.c +2 -8

index dc20eccd6a77..4249a771818b 100644
@@ -2021,31 +2021,15 @@ static void clear_sp_write_flooding_count(u64 *spte)
      __clear_sp_write_flooding_count(sptep_to_sp(spte));
 }
 
-static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
-                                          gfn_t gfn,
-                                          gva_t gaddr,
-                                          unsigned level,
-                                          bool direct,
-                                          unsigned int access)
+static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, gfn_t gfn,
+                                          union kvm_mmu_page_role role)
 {
-     union kvm_mmu_page_role role;
      struct hlist_head *sp_list;
-     unsigned quadrant;
      struct kvm_mmu_page *sp;
      int ret;
      int collisions = 0;
      LIST_HEAD(invalid_list);
 

arch/x86/kvm/mmu/mmu.c static void clear_sp_write_flooding_count(u64 *spte) · line 2048

Quoted 10 lines

arch/x86/kvm/mmu/mmu.c +0 -10

-     role = vcpu->arch.mmu->root_role;
-     role.level = level;
-     role.direct = direct;
-     role.access = access;
-     if (role.has_4_byte_gpte) {
-             quadrant = gaddr >> (PAGE_SHIFT + (PT64_PT_BITS * level));
-             quadrant &= (1 << ((PT32_PT_BITS - PT64_PT_BITS) * level)) - 1;
-             role.quadrant = quadrant;
-     }
-

When you rebase to kvm/queue, the helper will need to deal with

        if (level <= vcpu->arch.mmu->cpu_role.base.level)
                role.passthrough = 0;

KVM should never create a passthrough huge page, so I believe it's just a matter of adding yet another boolean param to kvm_mmu_child_role().

Quoted 14 lines

arch/x86/kvm/mmu/mmu.c +1 -1

      sp_list = &vcpu->kvm->arch.mmu_page_hash[kvm_page_table_hashfn(gfn)];
      for_each_valid_sp(vcpu->kvm, sp, sp_list) {
              if (sp->gfn != gfn) {

@@ -2063,7 +2047,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
                       * Unsync pages must not be left as is, because the new
                       * upper-level page will be write-protected.
                       */
-                     if (level > PG_LEVEL_4K && sp->unsync)
+                     if (role.level > PG_LEVEL_4K && sp->unsync)
                              kvm_mmu_prepare_zap_page(vcpu->kvm, sp,
                                                       &invalid_list);
                      continue;

...

Quoted 9 lines

arch/x86/kvm/mmu/mmu.c +2 -1

@@ -3310,12 +3338,21 @@ static int mmu_check_root(struct kvm_vcpu *vcpu, gfn_t root_gfn)
      return ret;
 }
 
-static hpa_t mmu_alloc_root(struct kvm_vcpu *vcpu, gfn_t gfn, gva_t gva,
+static hpa_t mmu_alloc_root(struct kvm_vcpu *vcpu, gfn_t gfn, int quadrant,
                          u8 level, bool direct)
 {
+     union kvm_mmu_page_role role;

arch/x86/kvm/mmu/mmu.c static int mmu_check_root(struct kvm_vcpu *vcpu, gfn_t root_gfn) · line 3353

Quoted 10 lines

arch/x86/kvm/mmu/mmu.c +7 -1

      struct kvm_mmu_page *sp;
 
-     sp = kvm_mmu_get_page(vcpu, gfn, gva, level, direct, ACC_ALL);
+     role = vcpu->arch.mmu->root_role;
+     role.level = level;
+     role.direct = direct;
+     role.access = ACC_ALL;
+
+     if (role.has_4_byte_gpte)
+             role.quadrant = quadrant;

Maybe add a comment explaining the PAE and 32-bit paging paths share a call for allocating PDPTEs? Otherwise it looks like passing a non-zero quadrant when the guest doesn't have 4-byte PTEs should be a bug.

Hmm, even better, if the check is moved to the caller, then this can be:

        role.level = level;
        role.direct = direct;
        role.access = ACC_ALL;
        role.quadrant = quadrant;

        WARN_ON_ONCE(quadrant && !role.has_4_byte_gpte));
        WARN_ON_ONCE(direct && role.has_4_byte_gpte));

and no comment is necessary.

arch/x86/kvm/mmu/mmu.c static int mmu_alloc_direct_roots(struct kvm_vcpu *vcpu) · line 3389

Quoted 13 lines

arch/x86/kvm/mmu/mmu.c +3 -2

+
+     sp = kvm_mmu_get_page(vcpu, gfn, role);
      ++sp->root_count;
 
      return __pa(sp->spt);

@@ -3349,8 +3386,8 @@ static int mmu_alloc_direct_roots(struct kvm_vcpu *vcpu)
              for (i = 0; i < 4; ++i) {
                      WARN_ON_ONCE(IS_VALID_PAE_ROOT(mmu->pae_root[i]));
 
-                     root = mmu_alloc_root(vcpu, i << (30 - PAGE_SHIFT),
-                                           i << 30, PT32_ROOT_LEVEL, true);
+                     root = mmu_alloc_root(vcpu, i << (30 - PAGE_SHIFT), i,

The @quadrant here can be hardcoded to '0', has_4_byte_gpte is guaranteed to be false if the MMU is direct. And then in the indirect path, set gva (and then quadrant) based on 'i' iff the guest is using 32-bit paging.

Probably worth making it a separate patch just in case I'm forgetting something. Lightly tested...

--
From: Sean Christopherson <[email protected]>
Date: Thu, 5 May 2022 14:19:35 -0700
Subject: [PATCH] KVM: x86/mmu: Pass '0' for @gva when allocating root with
8-byte gpte

Pass '0' instead of the "real" gva when allocating a direct PAE root, a.k.a. a direct PDPTE, and when allocating indirect roots that shadow 64-bit / 8-byte GPTEs.

Thee @gva is only needed if the root is shadowing 32-bit paging in the guest, in which case KVM needs to use different shadow pages for each of the two 4-byte GPTEs covered by KVM's 8-byte PAE SPTE.

For direct MMUs, there's obviously no shadowing, and for indirect MMU

In anticipation of moving the quadrant logic into mmu_alloc_root(), WARN if a non-zero @gva is passed for !4-byte GPTEs, and WARN if 4-byte GPTEs are ever combined with a direct root (there's no shadowing, so TDP roots should ignore the GPTE size).

Signed-off-by Sean Christopherson <[email protected]>

---
 arch/x86/kvm/mmu/mmu.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

arch/x86/kvm/mmu/mmu.c +11 -3

index dc20eccd6a77..6dfa3cfa8394 100644
@@ -3313,8 +3313,12 @@ static int mmu_check_root(struct kvm_vcpu *vcpu, gfn_t root_gfn)
 static hpa_t mmu_alloc_root(struct kvm_vcpu *vcpu, gfn_t gfn, gva_t gva,
                            u8 level, bool direct)
 {
+       union kvm_mmu_page_role role = vcpu->arch.mmu->root_role;
        struct kvm_mmu_page *sp;
 
+       WARN_ON_ONCE(gva && !role.has_4_byte_gpte);
+       WARN_ON_ONCE(direct && role.has_4_byte_gpte);
+
        sp = kvm_mmu_get_page(vcpu, gfn, gva, level, direct, ACC_ALL);
        ++sp->root_count;
 

@@ -3349,8 +3353,8 @@ static int mmu_alloc_direct_roots(struct kvm_vcpu *vcpu)
                for (i = 0; i < 4; ++i) {
                        WARN_ON_ONCE(IS_VALID_PAE_ROOT(mmu->pae_root[i]));
 
-                       root = mmu_alloc_root(vcpu, i << (30 - PAGE_SHIFT),
-                                             i << 30, PT32_ROOT_LEVEL, true);
+                       root = mmu_alloc_root(vcpu, i << (30 - PAGE_SHIFT), 0,
+                                             PT32_ROOT_LEVEL, true);
                        mmu->pae_root[i] = root | PT_PRESENT_MASK |
                                           shadow_me_mask;
                }

@@ -3435,6 +3439,7 @@ static int mmu_alloc_shadow_roots(struct kvm_vcpu *vcpu)
        u64 pdptrs[4], pm_mask;
        gfn_t root_gfn, root_pgd;
        hpa_t root;
+       gva_t gva;
        unsigned i;
        int r;
 

@@ -3508,6 +3513,7 @@ static int mmu_alloc_shadow_roots(struct kvm_vcpu *vcpu)
                }
        }
 
+       gva = 0;
        for (i = 0; i < 4; ++i) {
                WARN_ON_ONCE(IS_VALID_PAE_ROOT(mmu->pae_root[i]));
 

@@ -3517,9 +3523,11 @@ static int mmu_alloc_shadow_roots(struct kvm_vcpu *vcpu)
                                continue;
                        }
                        root_gfn = pdptrs[i] >> PAGE_SHIFT;
+               } else if (mmu->cpu_role.base.level == PT32_ROOT_LEVEL) {
+                       gva = i << 30;
                }
 
-               root = mmu_alloc_root(vcpu, root_gfn, i << 30,
+               root = mmu_alloc_root(vcpu, root_gfn, gva,
                                      PT32_ROOT_LEVEL, false);
                mmu->pae_root[i] = root | pm_mask;
        }

base-commit: 8bae380ad7dd3c31266d3685841ea4ce574d462d --

+Lai Jiangshan

It looks like only root pages can be passthrough, so kvm_mmu_child_role() can hard-code passthrough to 0. Lai do you agree?

Instead of computing the shadow page role from scratch for every new page, derive most of the information from the parent shadow page. This avoids redundant calculations and reduces the number of parameters to kvm_mmu_get_page().

Preemptively split out the role calculation to a separate function for use in a following commit.

No functional change intended.

Reviewed-by Peter Xu <[email protected]>

Signed-off-by David Matlack <[email protected]>

---
  arch/x86/kvm/mmu/mmu.c         | 96 +++++++++++++++++++++++-----------
  arch/x86/kvm/mmu/paging_tmpl.h |  9 ++--
  2 files changed, 71 insertions(+), 34 deletions(-)

arch/x86/kvm/mmu/mmu.c +2 -8

index dc20eccd6a77..4249a771818b 100644
@@ -2021,31 +2021,15 @@ static void clear_sp_write_flooding_count(u64 *spte)
  	__clear_sp_write_flooding_count(sptep_to_sp(spte));
  }
 
-static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
-					     gfn_t gfn,
-					     gva_t gaddr,
-					     unsigned level,
-					     bool direct,
-					     unsigned int access)
+static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, gfn_t gfn,
+					     union kvm_mmu_page_role role)
  {
-	union kvm_mmu_page_role role;
  	struct hlist_head *sp_list;
-	unsigned quadrant;
  	struct kvm_mmu_page *sp;
  	int ret;
  	int collisions = 0;
  	LIST_HEAD(invalid_list);

arch/x86/kvm/mmu/mmu.c +10 -5

-
  	sp_list = &vcpu->kvm->arch.mmu_page_hash[kvm_page_table_hashfn(gfn)];
  	for_each_valid_sp(vcpu->kvm, sp, sp_list) {
  		if (sp->gfn != gfn) {

@@ -2063,7 +2047,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
  			 * Unsync pages must not be left as is, because the new
  			 * upper-level page will be write-protected.
  			 */
-			if (level > PG_LEVEL_4K && sp->unsync)
+			if (role.level > PG_LEVEL_4K && sp->unsync)
  				kvm_mmu_prepare_zap_page(vcpu->kvm, sp,
  							 &invalid_list);
  			continue;

@@ -2104,14 +2088,14 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
 
  	++vcpu->kvm->stat.mmu_cache_miss;
 
-	sp = kvm_mmu_alloc_page(vcpu, direct);
+	sp = kvm_mmu_alloc_page(vcpu, role.direct);
 
  	sp->gfn = gfn;
  	sp->role = role;
  	hlist_add_head(&sp->hash_link, sp_list);
-	if (!direct) {
+	if (!role.direct) {
  		account_shadowed(vcpu->kvm, sp);
-		if (level == PG_LEVEL_4K && kvm_vcpu_write_protect_gfn(vcpu, gfn))
+		if (role.level == PG_LEVEL_4K && kvm_vcpu_write_protect_gfn(vcpu, gfn))
  			kvm_flush_remote_tlbs_with_address(vcpu->kvm, gfn, 1);
  	}
  	trace_kvm_mmu_get_page(sp, true);

@@ -2123,6 +2107,51 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
  	return sp;
  }
 
+static union kvm_mmu_page_role kvm_mmu_child_role(u64 *sptep, bool direct, u32 access)
+{
+	struct kvm_mmu_page *parent_sp = sptep_to_sp(sptep);
+	union kvm_mmu_page_role role;
+
+	role = parent_sp->role;

Quoted 41 lines · David Matlack

Instead of computing the shadow page role from scratch for every new page, derive most of the information from the parent shadow page. This avoids redundant calculations and reduces the number of parameters to kvm_mmu_get_page().

Preemptively split out the role calculation to a separate function for use in a following commit.

No functional change intended.

Reviewed-by Peter Xu <[email protected]>

Signed-off-by David Matlack <[email protected]>

---
  arch/x86/kvm/mmu/mmu.c         | 96 +++++++++++++++++++++++-----------
  arch/x86/kvm/mmu/paging_tmpl.h |  9 ++--
  2 files changed, 71 insertions(+), 34 deletions(-)

arch/x86/kvm/mmu/mmu.c +2 -8

index dc20eccd6a77..4249a771818b 100644
@@ -2021,31 +2021,15 @@ static void clear_sp_write_flooding_count(u64 *spte)
      __clear_sp_write_flooding_count(sptep_to_sp(spte));
  }
 
-static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
-                                          gfn_t gfn,
-                                          gva_t gaddr,
-                                          unsigned level,
-                                          bool direct,
-                                          unsigned int access)
+static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, gfn_t gfn,
+                                          union kvm_mmu_page_role role)
  {
-     union kvm_mmu_page_role role;
      struct hlist_head *sp_list;
-     unsigned quadrant;
      struct kvm_mmu_page *sp;
      int ret;
      int collisions = 0;
      LIST_HEAD(invalid_list);

arch/x86/kvm/mmu/mmu.c static void clear_sp_write_flooding_count(u64 *spte) · line 2047

Quoted 10 lines

arch/x86/kvm/mmu/mmu.c +0 -9

 
-     role = vcpu->arch.mmu->root_role;
-     role.level = level;
-     role.direct = direct;
-     role.access = access;
-     if (role.has_4_byte_gpte) {
-             quadrant = gaddr >> (PAGE_SHIFT + (PT64_PT_BITS * level));
-             quadrant &= (1 << ((PT32_PT_BITS - PT64_PT_BITS) * level)) - 1;
-             role.quadrant = quadrant;
-     }

I don't think replacing it with kvm_mmu_child_role() can reduce any calculations.

role.level, role.direct, role.access and role.quadrant still need to be calculated. And the old code is still in mmu_alloc_root().

I think kvm_mmu_get_shadow_page() can keep the those parameters and kvm_mmu_child_role() is only introduced for nested_mmu_get_sp_for_split().

Both kvm_mmu_get_shadow_page() and nested_mmu_get_sp_for_split() call __kvm_mmu_get_shadow_page() which uses role as a parameter.

Quoted 44 lines

arch/x86/kvm/mmu/mmu.c +10 -5

-
      sp_list = &vcpu->kvm->arch.mmu_page_hash[kvm_page_table_hashfn(gfn)];
      for_each_valid_sp(vcpu->kvm, sp, sp_list) {
              if (sp->gfn != gfn) {

@@ -2063,7 +2047,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
                       * Unsync pages must not be left as is, because the new
                       * upper-level page will be write-protected.
                       */
-                     if (level > PG_LEVEL_4K && sp->unsync)
+                     if (role.level > PG_LEVEL_4K && sp->unsync)
                              kvm_mmu_prepare_zap_page(vcpu->kvm, sp,
                                                       &invalid_list);
                      continue;

@@ -2104,14 +2088,14 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
 
      ++vcpu->kvm->stat.mmu_cache_miss;
 
-     sp = kvm_mmu_alloc_page(vcpu, direct);
+     sp = kvm_mmu_alloc_page(vcpu, role.direct);
 
      sp->gfn = gfn;
      sp->role = role;
      hlist_add_head(&sp->hash_link, sp_list);
-     if (!direct) {
+     if (!role.direct) {
              account_shadowed(vcpu->kvm, sp);
-             if (level == PG_LEVEL_4K && kvm_vcpu_write_protect_gfn(vcpu, gfn))
+             if (role.level == PG_LEVEL_4K && kvm_vcpu_write_protect_gfn(vcpu, gfn))
                      kvm_flush_remote_tlbs_with_address(vcpu->kvm, gfn, 1);
      }
      trace_kvm_mmu_get_page(sp, true);

@@ -2123,6 +2107,51 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
      return sp;
  }
 
+static union kvm_mmu_page_role kvm_mmu_child_role(u64 *sptep, bool direct, u32 access)
+{
+     struct kvm_mmu_page *parent_sp = sptep_to_sp(sptep);
+     union kvm_mmu_page_role role;
+
+     role = parent_sp->role;

arch/x86/kvm/mmu/mmu.c static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, · line 2125

Quoted 10 lines

arch/x86/kvm/mmu/mmu.c +10 -0

+     role.level--;
+     role.access = access;
+     role.direct = direct;
+
+     /*
+      * If the guest has 4-byte PTEs then that means it's using 32-bit,
+      * 2-level, non-PAE paging. KVM shadows such guests using 4 PAE page
+      * directories, each mapping 1/4 of the guest's linear address space
+      * (1GiB). The shadow pages for those 4 page directories are
+      * pre-allocated and assigned a separate quadrant in their role.

It is not going to be true in patchset:
[PATCH V2 0/7] KVM: X86/MMU: Use one-off special shadow page for special roots

https://lore.kernel.org/lkml/20220503150735.32723-1-jiangshanlai at gmail.com/

The shadow pages for those 4 page directories are also allocated on demand.

Quoted 59 lines · Lai Jiangshan

Quoted 41 lines · David Matlack

Instead of computing the shadow page role from scratch for every new page, derive most of the information from the parent shadow page. This avoids redundant calculations and reduces the number of parameters to kvm_mmu_get_page().

Preemptively split out the role calculation to a separate function for use in a following commit.

No functional change intended.

Reviewed-by Peter Xu <[email protected]>

Signed-off-by David Matlack <[email protected]>

---
  arch/x86/kvm/mmu/mmu.c         | 96 +++++++++++++++++++++++-----------
  arch/x86/kvm/mmu/paging_tmpl.h |  9 ++--
  2 files changed, 71 insertions(+), 34 deletions(-)

arch/x86/kvm/mmu/mmu.c +2 -8

index dc20eccd6a77..4249a771818b 100644
@@ -2021,31 +2021,15 @@ static void clear_sp_write_flooding_count(u64 *spte)
      __clear_sp_write_flooding_count(sptep_to_sp(spte));
  }
 
-static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
-                                          gfn_t gfn,
-                                          gva_t gaddr,
-                                          unsigned level,
-                                          bool direct,
-                                          unsigned int access)
+static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, gfn_t gfn,
+                                          union kvm_mmu_page_role role)
  {
-     union kvm_mmu_page_role role;
      struct hlist_head *sp_list;
-     unsigned quadrant;
      struct kvm_mmu_page *sp;
      int ret;
      int collisions = 0;
      LIST_HEAD(invalid_list);

arch/x86/kvm/mmu/mmu.c static void clear_sp_write_flooding_count(u64 *spte) · line 2047

Quoted 10 lines

arch/x86/kvm/mmu/mmu.c +0 -9

 
-     role = vcpu->arch.mmu->root_role;
-     role.level = level;
-     role.direct = direct;
-     role.access = access;
-     if (role.has_4_byte_gpte) {
-             quadrant = gaddr >> (PAGE_SHIFT + (PT64_PT_BITS * level));
-             quadrant &= (1 << ((PT32_PT_BITS - PT64_PT_BITS) * level)) - 1;
-             role.quadrant = quadrant;
-     }

I don't think replacing it with kvm_mmu_child_role() can reduce any calculations.

Quoted 2 lines

role.level, role.direct, role.access and role.quadrant still need to be calculated. And the old code is still in mmu_alloc_root().

You are correct. Instead of saying "less calculations" I should have said "eliminates the dependency on vcpu->arch.mmu->root_role".

Quoted 6 lines

I think kvm_mmu_get_shadow_page() can keep the those parameters and kvm_mmu_child_role() is only introduced for nested_mmu_get_sp_for_split().

Both kvm_mmu_get_shadow_page() and nested_mmu_get_sp_for_split() call __kvm_mmu_get_shadow_page() which uses role as a parameter.

I agree this would work, but I think the end result would be more difficult to read.

The way I've implemented it there are two ways the SP roles are calculated:

1. For root SPs: Derive the role from vCPU root_role and caller-provided inputs.
2. For child SPs: Derive the role from parent SP and caller-provided inputs.

Your proposal would still have two ways to calculate the SP role, but split along a different dimension:

1. For vCPUs creating SPs: Derive the role from vCPU root_role and caller-provided inputs.
2. For Eager Page Splitting creating SPs: Derive the role from parent SP and caller-provided inputs.

With your proposal, it is less obvious that eager page splitting is going to end up with the correct role. Whereas if we use the same calculation for all child SPs, it is immediately obvious.

Quoted 60 lines

Quoted 45 lines

arch/x86/kvm/mmu/mmu.c +10 -5

-
      sp_list = &vcpu->kvm->arch.mmu_page_hash[kvm_page_table_hashfn(gfn)];
      for_each_valid_sp(vcpu->kvm, sp, sp_list) {
              if (sp->gfn != gfn) {

@@ -2063,7 +2047,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
                       * Unsync pages must not be left as is, because the new
                       * upper-level page will be write-protected.
                       */
-                     if (level > PG_LEVEL_4K && sp->unsync)
+                     if (role.level > PG_LEVEL_4K && sp->unsync)
                              kvm_mmu_prepare_zap_page(vcpu->kvm, sp,
                                                       &invalid_list);
                      continue;

@@ -2104,14 +2088,14 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
 
      ++vcpu->kvm->stat.mmu_cache_miss;
 
-     sp = kvm_mmu_alloc_page(vcpu, direct);
+     sp = kvm_mmu_alloc_page(vcpu, role.direct);
 
      sp->gfn = gfn;
      sp->role = role;
      hlist_add_head(&sp->hash_link, sp_list);
-     if (!direct) {
+     if (!role.direct) {
              account_shadowed(vcpu->kvm, sp);
-             if (level == PG_LEVEL_4K && kvm_vcpu_write_protect_gfn(vcpu, gfn))
+             if (role.level == PG_LEVEL_4K && kvm_vcpu_write_protect_gfn(vcpu, gfn))
                      kvm_flush_remote_tlbs_with_address(vcpu->kvm, gfn, 1);
      }
      trace_kvm_mmu_get_page(sp, true);

@@ -2123,6 +2107,51 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
      return sp;
  }
 
+static union kvm_mmu_page_role kvm_mmu_child_role(u64 *sptep, bool direct, u32 access)
+{
+     struct kvm_mmu_page *parent_sp = sptep_to_sp(sptep);
+     union kvm_mmu_page_role role;
+
+     role = parent_sp->role;

arch/x86/kvm/mmu/mmu.c static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, · line 2125

Quoted 10 lines

arch/x86/kvm/mmu/mmu.c +10 -0

+     role.level--;
+     role.access = access;
+     role.direct = direct;
+
+     /*
+      * If the guest has 4-byte PTEs then that means it's using 32-bit,
+      * 2-level, non-PAE paging. KVM shadows such guests using 4 PAE page
+      * directories, each mapping 1/4 of the guest's linear address space
+      * (1GiB). The shadow pages for those 4 page directories are
+      * pre-allocated and assigned a separate quadrant in their role.

It is not going to be true in patchset:
[PATCH V2 0/7] KVM: X86/MMU: Use one-off special shadow page for special roots

Quoted 3 lines

https://lore.kernel.org/lkml/20220503150735.32723-1-jiangshanlai at gmail.com/

The shadow pages for those 4 page directories are also allocated on demand.

Ack. I can even just drop this sentence in v5, it's just background information.

()

Quoted 23 lines · David Matlack

Quoted 21 lines · Lai Jiangshan

Quoted 6 lines

arch/x86/kvm/mmu/mmu.c +6 -0

+static union kvm_mmu_page_role kvm_mmu_child_role(u64 *sptep, bool direct, u32 access)
+{
+     struct kvm_mmu_page *parent_sp = sptep_to_sp(sptep);
+     union kvm_mmu_page_role role;
+
+     role = parent_sp->role;

arch/x86/kvm/mmu/mmu.c static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, · line 2125

Quoted 10 lines

arch/x86/kvm/mmu/mmu.c +10 -0

+     role.level--;
+     role.access = access;
+     role.direct = direct;
+
+     /*
+      * If the guest has 4-byte PTEs then that means it's using 32-bit,
+      * 2-level, non-PAE paging. KVM shadows such guests using 4 PAE page
+      * directories, each mapping 1/4 of the guest's linear address space
+      * (1GiB). The shadow pages for those 4 page directories are
+      * pre-allocated and assigned a separate quadrant in their role.

It is not going to be true in patchset:
[PATCH V2 0/7] KVM: X86/MMU: Use one-off special shadow page for special roots

Quoted 5 lines

Quoted 3 lines

https://lore.kernel.org/lkml/20220503150735.32723-1-jiangshanlai at gmail.com/

The shadow pages for those 4 page directories are also allocated on demand.

Ack. I can even just drop this sentence in v5, it's just background information.

No, if one-off special shadow pages are used.

kvm_mmu_child_role() should be:

()

Quoted 90 lines · David Matlack

Quoted 59 lines · Lai Jiangshan

Quoted 41 lines · David Matlack

Instead of computing the shadow page role from scratch for every new page, derive most of the information from the parent shadow page. This avoids redundant calculations and reduces the number of parameters to kvm_mmu_get_page().

Preemptively split out the role calculation to a separate function for use in a following commit.

No functional change intended.

Reviewed-by Peter Xu <[email protected]>

Signed-off-by David Matlack <[email protected]>

---
  arch/x86/kvm/mmu/mmu.c         | 96 +++++++++++++++++++++++-----------
  arch/x86/kvm/mmu/paging_tmpl.h |  9 ++--
  2 files changed, 71 insertions(+), 34 deletions(-)

arch/x86/kvm/mmu/mmu.c +2 -8

index dc20eccd6a77..4249a771818b 100644
@@ -2021,31 +2021,15 @@ static void clear_sp_write_flooding_count(u64 *spte)
      __clear_sp_write_flooding_count(sptep_to_sp(spte));
  }
 
-static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
-                                          gfn_t gfn,
-                                          gva_t gaddr,
-                                          unsigned level,
-                                          bool direct,
-                                          unsigned int access)
+static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, gfn_t gfn,
+                                          union kvm_mmu_page_role role)
  {
-     union kvm_mmu_page_role role;
      struct hlist_head *sp_list;
-     unsigned quadrant;
      struct kvm_mmu_page *sp;
      int ret;
      int collisions = 0;
      LIST_HEAD(invalid_list);

arch/x86/kvm/mmu/mmu.c static void clear_sp_write_flooding_count(u64 *spte) · line 2047

Quoted 10 lines

arch/x86/kvm/mmu/mmu.c +0 -9

 
-     role = vcpu->arch.mmu->root_role;
-     role.level = level;
-     role.direct = direct;
-     role.access = access;
-     if (role.has_4_byte_gpte) {
-             quadrant = gaddr >> (PAGE_SHIFT + (PT64_PT_BITS * level));
-             quadrant &= (1 << ((PT32_PT_BITS - PT64_PT_BITS) * level)) - 1;
-             role.quadrant = quadrant;
-     }

I don't think replacing it with kvm_mmu_child_role() can reduce any calculations.

Quoted 2 lines

role.level, role.direct, role.access and role.quadrant still need to be calculated. And the old code is still in mmu_alloc_root().

You are correct. Instead of saying "less calculations" I should have said "eliminates the dependency on vcpu->arch.mmu->root_role".

Quoted 6 lines

I think kvm_mmu_get_shadow_page() can keep the those parameters and kvm_mmu_child_role() is only introduced for nested_mmu_get_sp_for_split().

Both kvm_mmu_get_shadow_page() and nested_mmu_get_sp_for_split() call __kvm_mmu_get_shadow_page() which uses role as a parameter.

I agree this would work, but I think the end result would be more difficult to read.

The way I've implemented it there are two ways the SP roles are calculated:

1. For root SPs: Derive the role from vCPU root_role and caller-provided inputs.
2. For child SPs: Derive the role from parent SP and caller-provided inputs.

Your proposal would still have two ways to calculate the SP role, but split along a different dimension:

1. For vCPUs creating SPs: Derive the role from vCPU root_role and caller-provided inputs.
2. For Eager Page Splitting creating SPs: Derive the role from parent SP and caller-provided inputs.

Quoted 3 lines

With your proposal, it is less obvious that eager page splitting is going to end up with the correct role. Whereas if we use the same calculation for all child SPs, it is immediately obvious.

In this patchset, there are still two ways to calculate the SP role including the one in mmu_alloc_root() which I dislike.

The old code is just moved into mmu_alloc_root() even kvm_mmu_child_role() is introduced.

Quoted 60 lines

Quoted 60 lines

Quoted 45 lines

arch/x86/kvm/mmu/mmu.c +10 -5

-
      sp_list = &vcpu->kvm->arch.mmu_page_hash[kvm_page_table_hashfn(gfn)];
      for_each_valid_sp(vcpu->kvm, sp, sp_list) {
              if (sp->gfn != gfn) {

@@ -2063,7 +2047,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
                       * Unsync pages must not be left as is, because the new
                       * upper-level page will be write-protected.
                       */
-                     if (level > PG_LEVEL_4K && sp->unsync)
+                     if (role.level > PG_LEVEL_4K && sp->unsync)
                              kvm_mmu_prepare_zap_page(vcpu->kvm, sp,
                                                       &invalid_list);
                      continue;

@@ -2104,14 +2088,14 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
 
      ++vcpu->kvm->stat.mmu_cache_miss;
 
-     sp = kvm_mmu_alloc_page(vcpu, direct);
+     sp = kvm_mmu_alloc_page(vcpu, role.direct);
 
      sp->gfn = gfn;
      sp->role = role;
      hlist_add_head(&sp->hash_link, sp_list);
-     if (!direct) {
+     if (!role.direct) {
              account_shadowed(vcpu->kvm, sp);
-             if (level == PG_LEVEL_4K && kvm_vcpu_write_protect_gfn(vcpu, gfn))
+             if (role.level == PG_LEVEL_4K && kvm_vcpu_write_protect_gfn(vcpu, gfn))
                      kvm_flush_remote_tlbs_with_address(vcpu->kvm, gfn, 1);
      }
      trace_kvm_mmu_get_page(sp, true);

@@ -2123,6 +2107,51 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
      return sp;
  }
 
+static union kvm_mmu_page_role kvm_mmu_child_role(u64 *sptep, bool direct, u32 access)
+{
+     struct kvm_mmu_page *parent_sp = sptep_to_sp(sptep);
+     union kvm_mmu_page_role role;
+
+     role = parent_sp->role;

arch/x86/kvm/mmu/mmu.c static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, · line 2125

Quoted 10 lines

arch/x86/kvm/mmu/mmu.c +10 -0

+     role.level--;
+     role.access = access;
+     role.direct = direct;
+
+     /*
+      * If the guest has 4-byte PTEs then that means it's using 32-bit,
+      * 2-level, non-PAE paging. KVM shadows such guests using 4 PAE page
+      * directories, each mapping 1/4 of the guest's linear address space
+      * (1GiB). The shadow pages for those 4 page directories are
+      * pre-allocated and assigned a separate quadrant in their role.

It is not going to be true in patchset:
[PATCH V2 0/7] KVM: X86/MMU: Use one-off special shadow page for special roots

Quoted 5 lines

Quoted 3 lines

https://lore.kernel.org/lkml/20220503150735.32723-1-jiangshanlai at gmail.com/

The shadow pages for those 4 page directories are also allocated on demand.

Ack. I can even just drop this sentence in v5, it's just background information.

No, if one-off special shadow pages are used.

kvm_mmu_child_role() should be:

arch/x86/kvm/mmu/mmu.c +6 -0

+       if (role.has_4_byte_gpte) {
+               if (role.level == PG_LEVEL_4K)
+                       role.quadrant = (sptep - parent_sp->spt) % 2;
+               if (role.level == PG_LEVEL_2M)
+                       role.quadrant = (sptep - parent_sp->spt) % 4;
+       }

And if one-off special shadow pages are merged first. You don't need any calculation in mmu_alloc_root(), you can just directly use

    sp = kvm_mmu_get_page(vcpu, gfn, vcpu->arch.mmu->root_role);

because vcpu->arch.mmu->root_role is always the real role of the root sp no matter if it is a normall root sp or an one-off special sp.

I hope you will pardon me for my touting my patchset and asking people to review them in your threads.

Thanks
Lai

Quoted 38 lines · Lai Jiangshan

Quoted 10 lines · David Matlack

Quoted 9 lines · Lai Jiangshan

Quoted 6 lines · David Matlack

(quoted diff) +6 -0

+     /*
+      * If the guest has 4-byte PTEs then that means it's using 32-bit,
+      * 2-level, non-PAE paging. KVM shadows such guests using 4 PAE page
+      * directories, each mapping 1/4 of the guest's linear address space
+      * (1GiB). The shadow pages for those 4 page directories are
+      * pre-allocated and assigned a separate quadrant in their role.

Quoted 8 lines

Quoted 6 lines

It is not going to be true in patchset:
[PATCH V2 0/7] KVM: X86/MMU: Use one-off special shadow page for special roots

https://lore.kernel.org/lkml/20220503150735.32723-1-jiangshanlai at gmail.com/

The shadow pages for those 4 page directories are also allocated on demand.

Ack. I can even just drop this sentence in v5, it's just background information.

No, if one-off special shadow pages are used.

kvm_mmu_child_role() should be:

(quoted diff) +6 -0

+       if (role.has_4_byte_gpte) {
+               if (role.level == PG_LEVEL_4K)
+                       role.quadrant = (sptep - parent_sp->spt) % 2;
+               if (role.level == PG_LEVEL_2M)
+                       role.quadrant = (sptep - parent_sp->spt) % 4;
+       }

And if one-off special shadow pages are merged first. You don't need any calculation in mmu_alloc_root(), you can just directly use

    sp = kvm_mmu_get_page(vcpu, gfn, vcpu->arch.mmu->root_role);

because vcpu->arch.mmu->root_role is always the real role of the root sp no matter if it is a normall root sp or an one-off special sp.

Quoted 2 lines

I hope you will pardon me for my touting my patchset and asking people to review them in your threads.

I see what you mean now. If your series is queued I will rebase on top with the appropriate changes. But for now I will continue to code against kvm/queue.

Decompose kvm_mmu_get_page() into separate helper functions to increase readability and prepare for allocating shadow pages without a vcpu pointer.

Specifically, pull the guts of kvm_mmu_get_page() into 2 helper functions:

kvm_mmu_find_shadow_page() -
Walks the page hash checking for any existing mmu pages that match the given gfn and role.

kvm_mmu_alloc_shadow_page()
Allocates and initializes an entirely new kvm_mmu_page. This currently requries a vcpu pointer for allocation and looking up the memslot but that will be removed in a future commit.

No functional change intended.

Signed-off-by David Matlack <[email protected]>

---

Consolidate kvm_mmu_alloc_page() and kvm_mmu_alloc_shadow_page() under the latter so that all shadow page allocation and initialization happens in one place.

No functional change intended.

Signed-off-by David Matlack <[email protected]>

---
 arch/x86/kvm/mmu/mmu.c | 39 +++++++++++++++++----------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

arch/x86/kvm/mmu/mmu.c +7 -22

index 5582badf4947..7d03320f6e08 100644
@@ -1703,27 +1703,6 @@ static void drop_parent_pte(struct kvm_mmu_page *sp,
 	mmu_spte_clear_no_track(parent_pte);
 }
 
-static struct kvm_mmu_page *kvm_mmu_alloc_page(struct kvm_vcpu *vcpu, bool direct)
-{
-	struct kvm_mmu_page *sp;
-
-	sp = kvm_mmu_memory_cache_alloc(&vcpu->arch.mmu_page_header_cache);
-	sp->spt = kvm_mmu_memory_cache_alloc(&vcpu->arch.mmu_shadow_page_cache);
-	if (!direct)
-		sp->gfns = kvm_mmu_memory_cache_alloc(&vcpu->arch.mmu_gfn_array_cache);
-	set_page_private(virt_to_page(sp->spt), (unsigned long)sp);
-
-	/*
-	 * active_mmu_pages must be a FIFO list, as kvm_zap_obsolete_pages()
-	 * depends on valid pages being added to the head of the list.  See
-	 * comments in kvm_zap_obsolete_pages().
-	 */
-	sp->mmu_valid_gen = vcpu->kvm->arch.mmu_valid_gen;
-	list_add(&sp->link, &vcpu->kvm->arch.active_mmu_pages);
-	kvm_mod_used_mmu_pages(vcpu->kvm, +1);
-	return sp;
-}
-
 static void mark_unsync(u64 *spte);
 static void kvm_mmu_mark_parents_unsync(struct kvm_mmu_page *sp)
 {

@@ -2100,7 +2079,23 @@ static struct kvm_mmu_page *kvm_mmu_alloc_shadow_page(struct kvm_vcpu *vcpu,
 						      struct hlist_head *sp_list,
 						      union kvm_mmu_page_role role)
 {
-	struct kvm_mmu_page *sp = kvm_mmu_alloc_page(vcpu, role.direct);
+	struct kvm_mmu_page *sp;
+
+	sp = kvm_mmu_memory_cache_alloc(&vcpu->arch.mmu_page_header_cache);
+	sp->spt = kvm_mmu_memory_cache_alloc(&vcpu->arch.mmu_shadow_page_cache);
+	if (!role.direct)
+		sp->gfns = kvm_mmu_memory_cache_alloc(&vcpu->arch.mmu_gfn_array_cache);
+

arch/x86/kvm/mmu/mmu.c +0 -0

 	sp->gfn = gfn;
 	sp->role = role;

Signature · 1 line

2.36.0.rc2.479.g8af0fa9b8e-goog

Quoted 55 lines · David Matlack

Consolidate kvm_mmu_alloc_page() and kvm_mmu_alloc_shadow_page() under the latter so that all shadow page allocation and initialization happens in one place.

No functional change intended.

Signed-off-by David Matlack <[email protected]>

---
 arch/x86/kvm/mmu/mmu.c | 39 +++++++++++++++++----------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

arch/x86/kvm/mmu/mmu.c +7 -22

index 5582badf4947..7d03320f6e08 100644
@@ -1703,27 +1703,6 @@ static void drop_parent_pte(struct kvm_mmu_page *sp,
      mmu_spte_clear_no_track(parent_pte);
 }
 
-static struct kvm_mmu_page *kvm_mmu_alloc_page(struct kvm_vcpu *vcpu, bool direct)
-{
-     struct kvm_mmu_page *sp;
-
-     sp = kvm_mmu_memory_cache_alloc(&vcpu->arch.mmu_page_header_cache);
-     sp->spt = kvm_mmu_memory_cache_alloc(&vcpu->arch.mmu_shadow_page_cache);
-     if (!direct)
-             sp->gfns = kvm_mmu_memory_cache_alloc(&vcpu->arch.mmu_gfn_array_cache);
-     set_page_private(virt_to_page(sp->spt), (unsigned long)sp);
-
-     /*
-      * active_mmu_pages must be a FIFO list, as kvm_zap_obsolete_pages()
-      * depends on valid pages being added to the head of the list.  See
-      * comments in kvm_zap_obsolete_pages().
-      */
-     sp->mmu_valid_gen = vcpu->kvm->arch.mmu_valid_gen;
-     list_add(&sp->link, &vcpu->kvm->arch.active_mmu_pages);
-     kvm_mod_used_mmu_pages(vcpu->kvm, +1);
-     return sp;
-}
-
 static void mark_unsync(u64 *spte);
 static void kvm_mmu_mark_parents_unsync(struct kvm_mmu_page *sp)
 {

@@ -2100,7 +2079,23 @@ static struct kvm_mmu_page *kvm_mmu_alloc_shadow_page(struct kvm_vcpu *vcpu,
                                                    struct hlist_head *sp_list,
                                                    union kvm_mmu_page_role role)
 {
-     struct kvm_mmu_page *sp = kvm_mmu_alloc_page(vcpu, role.direct);
+     struct kvm_mmu_page *sp;
+
+     sp = kvm_mmu_memory_cache_alloc(&vcpu->arch.mmu_page_header_cache);
+     sp->spt = kvm_mmu_memory_cache_alloc(&vcpu->arch.mmu_shadow_page_cache);
+     if (!role.direct)
+             sp->gfns = kvm_mmu_memory_cache_alloc(&vcpu->arch.mmu_gfn_array_cache);
+

arch/x86/kvm/mmu/mmu.c static struct kvm_mmu_page *kvm_mmu_alloc_shadow_page(struct kvm_vcpu *vcpu, · line 2098

Quoted 10 lines

arch/x86/kvm/mmu/mmu.c +10 -0

+     set_page_private(virt_to_page(sp->spt), (unsigned long)sp);
+
+     /*
+      * active_mmu_pages must be a FIFO list, as kvm_zap_obsolete_pages()
+      * depends on valid pages being added to the head of the list.  See
+      * comments in kvm_zap_obsolete_pages().
+      */
+     sp->mmu_valid_gen = vcpu->kvm->arch.mmu_valid_gen;
+     list_add(&sp->link, &vcpu->kvm->arch.active_mmu_pages);
+     kvm_mod_used_mmu_pages(vcpu->kvm, +1);

To reduce churn later on, what about opportunistically grabbing vcpu->kvm in a local variable in this patch?

Actually, at that point, it's a super trivial change, so you can probably just drop

KVM: x86/mmu: Replace vcpu with kvm in kvm_mmu_alloc_shadow_page()

and do the vcpu/kvm swap as part of

KVM: x86/mmu: Pass memory caches to allocate SPs separately

Quoted 6 lines

arch/x86/kvm/mmu/mmu.c +0 -1

      sp->gfn = gfn;
      sp->role = role;
--

2.36.0.rc2.479.g8af0fa9b8e-goog

Rename 2 functions:

kvm_mmu_get_page() -> kvm_mmu_get_shadow_page() kvm_mmu_free_page() -> kvm_mmu_free_shadow_page()

This change makes it clear that these functions deal with shadow pages rather than struct pages. It also aligns these functions with the naming scheme for kvm_mmu_find_shadow_page() and kvm_mmu_alloc_shadow_page().

Prefer "shadow_page" over the shorter "sp" since these are core functions and the line lengths aren't terrible.

No functional change intended.

Signed-off-by David Matlack <[email protected]>

---

Move the code that write-protects newly-shadowed guest page tables into account_shadowed(). This avoids a extra gfn-to-memslot lookup and is a more logical place for this code to live. But most importantly, this reduces kvm_mmu_alloc_shadow_page()'s reliance on having a struct kvm_vcpu pointer, which will be necessary when creating new shadow pages during VM ioctls for eager page splitting.

Note, it is safe to drop the role.level == PG_LEVEL_4K check since account_shadowed() returns early if role.level > PG_LEVEL_4K.

No functional change intended.

Signed-off-by David Matlack <[email protected]>

---

 arch/x86/kvm/mmu/mmu.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

arch/x86/kvm/mmu/mmu.c +3 -0

index fa7846760887..4f894db88bbf 100644
@@ -807,6 +807,9 @@ static void account_shadowed(struct kvm *kvm, struct kvm_mmu_page *sp)
 						    KVM_PAGE_TRACK_WRITE);
 
 	kvm_mmu_gfn_disallow_lpage(slot, gfn);
+
+	if (kvm_mmu_slot_gfn_write_protect(kvm, slot, gfn, PG_LEVEL_4K))
+		kvm_flush_remote_tlbs_with_address(kvm, gfn, 1);
 }
 
 void account_huge_nx_page(struct kvm *kvm, struct kvm_mmu_page *sp)

arch/x86/kvm/mmu/mmu.c +0 -2

-			kvm_flush_remote_tlbs_with_address(vcpu->kvm, gfn, 1);
-	}
 
 	return sp;
 }

Signature · 1 line

2.36.0.rc2.479.g8af0fa9b8e-goog

Quoted 7 lines · David Matlack

Move the code that write-protects newly-shadowed guest page tables into account_shadowed(). This avoids a extra gfn-to-memslot lookup and is a more logical place for this code to live. But most importantly, this reduces kvm_mmu_alloc_shadow_page()'s reliance on having a struct kvm_vcpu pointer, which will be necessary when creating new shadow pages during VM ioctls for eager page splitting.

Quoted 7 lines

Note, it is safe to drop the role.level == PG_LEVEL_4K check since account_shadowed() returns early if role.level > PG_LEVEL_4K.

No functional change intended.

Signed-off-by David Matlack <[email protected]>

---

Reviewed-by Sean Christopherson <[email protected]>

Quoted 18 lines

 arch/x86/kvm/mmu/mmu.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

arch/x86/kvm/mmu/mmu.c +3 -0

index fa7846760887..4f894db88bbf 100644
@@ -807,6 +807,9 @@ static void account_shadowed(struct kvm *kvm, struct kvm_mmu_page *sp)
                                                  KVM_PAGE_TRACK_WRITE);
 
      kvm_mmu_gfn_disallow_lpage(slot, gfn);
+
+     if (kvm_mmu_slot_gfn_write_protect(kvm, slot, gfn, PG_LEVEL_4K))
+             kvm_flush_remote_tlbs_with_address(kvm, gfn, 1);
 }
 
 void account_huge_nx_page(struct kvm *kvm, struct kvm_mmu_page *sp)

arch/x86/kvm/mmu/mmu.c static struct kvm_mmu_page *kvm_mmu_alloc_shadow_page(struct kvm_vcpu *vcpu, · line 2105

Quoted 8 lines

arch/x86/kvm/mmu/mmu.c +2 -2

@@ -2100,11 +2103,9 @@ static struct kvm_mmu_page *kvm_mmu_alloc_shadow_page(struct kvm_vcpu *vcpu,
      sp->gfn = gfn;
      sp->role = role;
      hlist_add_head(&sp->hash_link, sp_list);
-     if (!role.direct) {
+
+     if (!role.direct)
              account_shadowed(vcpu->kvm, sp);
-             if (role.level == PG_LEVEL_4K && kvm_vcpu_write_protect_gfn(vcpu, gfn))

Huh. Two thoughts.

1. Can you add a patch to drop kvm_vcpu_write_protect_gfn() entirely, i.e. convert mmu_sync_children() to use kvm_mmu_slot_gfn_write_protect? It's largely a moot point, but only because mmu_sync_children() only operates on shadow pages that are relevant to the current non-SMM/SMM role. And _that_ holds true only because KVM does kvm_mmu_reset_context() and drops roots for the vCPU on SMM transitions.

   That'd be a good oppurtunity to move this pair into a helper:

        slots = kvm_memslots_for_spte_role(kvm, sp->role);
        slot = __gfn_to_memslot(slots, gfn);

2. This got me thinking... Write-protecting for shadow paging should NOT be associated with the vCPU or even the role. The SMM memslots conceptually operate on the same guest physical memory, SMM is just given access to memory that is not present in the non-SMM memslots.

If KVM creates SPTEs for non-SMM, then it needs to write-protect _all_ memslots that contain the relevant gfn, e.g. if the guest takes an SMI and modifies the non-SMM page tables, then KVM needs trap and emulate (or unsync) those writes.

The mess "works" because no sane SMM handler (kind of a contradiction in terms) will modify non-SMM page tables, and vice versa.

The entire duplicate memslots approach is flawed. Better would have been to make SMM a flag and hide SMM-only memslots, not duplicated everything...

Quoted 8 lines

arch/x86/kvm/mmu/mmu.c +0 -3

-                     kvm_flush_remote_tlbs_with_address(vcpu->kvm, gfn, 1);
-     }
 
      return sp;
 }
--

2.36.0.rc2.479.g8af0fa9b8e-goog

Refactor kvm_mmu_alloc_shadow_page() to receive the caches from which it will allocate the various pieces of memory for shadow pages as a parameter, rather than deriving them from the vcpu pointer. This will be useful in a future commit where shadow pages are allocated during VM ioctls for eager page splitting, and thus will use a different set of caches.

Preemptively pull the caches out all the way to kvm_mmu_get_shadow_page() since eager page splitting will not be calling kvm_mmu_alloc_shadow_page() directly.

No functional change intended.

Signed-off-by David Matlack <[email protected]>

---

Allow the vcpu pointer in __kvm_mmu_get_shadow_page() to be NULL. Rename it to vcpu_or_null to prevent future commits from accidentally taking dependency on it without first considering the NULL case.

The vcpu pointer is only used for syncing indirect shadow pages in kvm_mmu_find_shadow_page(). A vcpu pointer it not required for correctness since unsync pages can simply be zapped. But this should never occur in practice, since the only use-case for passing a NULL vCPU pointer is eager page splitting which will only request direct shadow pages (which can never be unsync).

Even though __kvm_mmu_get_shadow_page() can gracefully handle a NULL vcpu, add a WARN() that will fire if __kvm_mmu_get_shadow_page() is ever called to get an indirect shadow page with a NULL vCPU pointer, since zapping unsync SPs is a performance overhead that should be considered.

Signed-off-by David Matlack <[email protected]>

---
 arch/x86/kvm/mmu/mmu.c | 40 ++++++++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 8 deletions(-)

arch/x86/kvm/mmu/mmu.c +0 -0

index 04029c01aebd..21407bd4435a 100644
@@ -1845,16 +1845,27 @@ static void kvm_mmu_commit_zap_page(struct kvm *kvm,
 	  &(_kvm)->arch.mmu_page_hash[kvm_page_table_hashfn(_gfn)])	\
 		if ((_sp)->gfn != (_gfn) || (_sp)->role.direct) {} else
 

Quoted 29 lines · David Matlack

Allow the vcpu pointer in __kvm_mmu_get_shadow_page() to be NULL. Rename it to vcpu_or_null to prevent future commits from accidentally taking dependency on it without first considering the NULL case.

The vcpu pointer is only used for syncing indirect shadow pages in kvm_mmu_find_shadow_page(). A vcpu pointer it not required for correctness since unsync pages can simply be zapped. But this should never occur in practice, since the only use-case for passing a NULL vCPU pointer is eager page splitting which will only request direct shadow pages (which can never be unsync).

Even though __kvm_mmu_get_shadow_page() can gracefully handle a NULL vcpu, add a WARN() that will fire if __kvm_mmu_get_shadow_page() is ever called to get an indirect shadow page with a NULL vCPU pointer, since zapping unsync SPs is a performance overhead that should be considered.

Signed-off-by David Matlack <[email protected]>

---
 arch/x86/kvm/mmu/mmu.c | 40 ++++++++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 8 deletions(-)

arch/x86/kvm/mmu/mmu.c +0 -0

index 04029c01aebd..21407bd4435a 100644
@@ -1845,16 +1845,27 @@ static void kvm_mmu_commit_zap_page(struct kvm *kvm,
        &(_kvm)->arch.mmu_page_hash[kvm_page_table_hashfn(_gfn)])     \
              if ((_sp)->gfn != (_gfn) || (_sp)->role.direct) {} else
 

arch/x86/kvm/mmu/mmu.c static void kvm_mmu_commit_zap_page(struct kvm *kvm, · line 1854

Quoted 10 lines

arch/x86/kvm/mmu/mmu.c +6 -3

-static int kvm_sync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
-                      struct list_head *invalid_list)
+static int __kvm_sync_page(struct kvm *kvm, struct kvm_vcpu *vcpu_or_null,
+                        struct kvm_mmu_page *sp,
+                        struct list_head *invalid_list)
 {
-     int ret = vcpu->arch.mmu->sync_page(vcpu, sp);
+     int ret = -1;
+
+     if (vcpu_or_null)

This should never happen. I like the idea of warning early, but I really don't like that the WARN is far removed from the code that actually depends on @vcpu being non-NULL. Case in point, KVM should have bailed on the WARN and never reached this point. And the inner __kvm_sync_page() is completely unnecessary.

I also don't love the vcpu_or_null terminology; I get the intent, but it doesn't really help because understand why/when it's NULL.

I played around with casting, e.g. to/from an unsigned long or void *, to prevent usage, but that doesn't work very well because 'unsigned long' ends up being awkward/confusing, and 'void *' is easily lost on a function call. And both lose type safety :-(

All in all, I think I'd prefer this patch to simply be a KVM_BUG_ON() if kvm_mmu_find_shadow_page() encounters an unsync page. Less churn, and IMO there's no real loss in robustness, e.g. we'd really have to screw up code review and testing to introduce a null vCPU pointer dereference in this code.

arch/x86/kvm/mmu/mmu.c +7 -0

index 3d102522804a..5aed9265f592 100644
@@ -2041,6 +2041,13 @@ static struct kvm_mmu_page *kvm_mmu_find_shadow_page(struct kvm *kvm,
                        goto out;
 
                if (sp->unsync) {
+                       /*
+                        * Getting indirect shadow pages without a vCPU pointer
+                        * is not supported, i.e. this should never happen.
+                        */
+                       if (KVM_BUG_ON(!vcpu, kvm))
+                               break;
+
                        /*
                         * The page is good, but is stale.  kvm_sync_page does
                         * get the latest guest state, but (unlike mmu_unsync_children)

Quoted 31 lines · Sean Christopherson

Quoted 29 lines · David Matlack

Allow the vcpu pointer in __kvm_mmu_get_shadow_page() to be NULL. Rename it to vcpu_or_null to prevent future commits from accidentally taking dependency on it without first considering the NULL case.

The vcpu pointer is only used for syncing indirect shadow pages in kvm_mmu_find_shadow_page(). A vcpu pointer it not required for correctness since unsync pages can simply be zapped. But this should never occur in practice, since the only use-case for passing a NULL vCPU pointer is eager page splitting which will only request direct shadow pages (which can never be unsync).

Even though __kvm_mmu_get_shadow_page() can gracefully handle a NULL vcpu, add a WARN() that will fire if __kvm_mmu_get_shadow_page() is ever called to get an indirect shadow page with a NULL vCPU pointer, since zapping unsync SPs is a performance overhead that should be considered.

Signed-off-by David Matlack <[email protected]>

---
 arch/x86/kvm/mmu/mmu.c | 40 ++++++++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 8 deletions(-)

arch/x86/kvm/mmu/mmu.c +0 -0

index 04029c01aebd..21407bd4435a 100644
@@ -1845,16 +1845,27 @@ static void kvm_mmu_commit_zap_page(struct kvm *kvm,
        &(_kvm)->arch.mmu_page_hash[kvm_page_table_hashfn(_gfn)])     \
              if ((_sp)->gfn != (_gfn) || (_sp)->role.direct) {} else
 

Quoted 15 lines

arch/x86/kvm/mmu/mmu.c static void kvm_mmu_commit_zap_page(struct kvm *kvm, · line 1854

Quoted 10 lines

arch/x86/kvm/mmu/mmu.c +6 -3

-static int kvm_sync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
-                      struct list_head *invalid_list)
+static int __kvm_sync_page(struct kvm *kvm, struct kvm_vcpu *vcpu_or_null,
+                        struct kvm_mmu_page *sp,
+                        struct list_head *invalid_list)
 {
-     int ret = vcpu->arch.mmu->sync_page(vcpu, sp);
+     int ret = -1;
+
+     if (vcpu_or_null)

This should never happen. I like the idea of warning early, but I really don't like that the WARN is far removed from the code that actually depends on @vcpu being non-NULL. Case in point, KVM should have bailed on the WARN and never reached this point. And the inner __kvm_sync_page() is completely unnecessary.

Yeah that's fair.

Quoted 1 line

Quoted 2 lines

I also don't love the vcpu_or_null terminology; I get the intent, but it doesn't really help because understand why/when it's NULL.

Eh, I don't think it needs to encode why or when. It just needs to flag to the reader (and future code authors) that this vcpu pointer (unlike all other vcpu pointers in KVM) is NULL in certain cases.

Quoted 4 lines

I played around with casting, e.g. to/from an unsigned long or void *, to prevent usage, but that doesn't work very well because 'unsigned long' ends up being awkward/confusing, and 'void *' is easily lost on a function call. And both lose type safety :-(

Yet another shortcoming of C :(

(The other being our other discussion about the RET_PF* return codes getting easily misinterpreted as KVM's magic return-to-user / continue-running-guest return codes.)

Makes me miss Rust!

Quoted 5 lines

All in all, I think I'd prefer this patch to simply be a KVM_BUG_ON() if kvm_mmu_find_shadow_page() encounters an unsync page. Less churn, and IMO there's no real loss in robustness, e.g. we'd really have to screw up code review and testing to introduce a null vCPU pointer dereference in this code.

Agreed about moving the check here and dropping __kvm_sync_page(). But I would prefer to retain the vcpu_or_null name (or at least something other than "vcpu" to indicate there's something non-standard about this pointer).

Quoted 51 lines · David Matlack

Quoted 31 lines · Sean Christopherson

Quoted 29 lines · David Matlack

Allow the vcpu pointer in __kvm_mmu_get_shadow_page() to be NULL. Rename it to vcpu_or_null to prevent future commits from accidentally taking dependency on it without first considering the NULL case.

The vcpu pointer is only used for syncing indirect shadow pages in kvm_mmu_find_shadow_page(). A vcpu pointer it not required for correctness since unsync pages can simply be zapped. But this should never occur in practice, since the only use-case for passing a NULL vCPU pointer is eager page splitting which will only request direct shadow pages (which can never be unsync).

Even though __kvm_mmu_get_shadow_page() can gracefully handle a NULL vcpu, add a WARN() that will fire if __kvm_mmu_get_shadow_page() is ever called to get an indirect shadow page with a NULL vCPU pointer, since zapping unsync SPs is a performance overhead that should be considered.

Signed-off-by David Matlack <[email protected]>

---
 arch/x86/kvm/mmu/mmu.c | 40 ++++++++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 8 deletions(-)

arch/x86/kvm/mmu/mmu.c +0 -0

index 04029c01aebd..21407bd4435a 100644
@@ -1845,16 +1845,27 @@ static void kvm_mmu_commit_zap_page(struct kvm *kvm,
        &(_kvm)->arch.mmu_page_hash[kvm_page_table_hashfn(_gfn)])     \
              if ((_sp)->gfn != (_gfn) || (_sp)->role.direct) {} else
 

Quoted 15 lines

arch/x86/kvm/mmu/mmu.c static void kvm_mmu_commit_zap_page(struct kvm *kvm, · line 1854

Quoted 10 lines

arch/x86/kvm/mmu/mmu.c +6 -3

-static int kvm_sync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
-                      struct list_head *invalid_list)
+static int __kvm_sync_page(struct kvm *kvm, struct kvm_vcpu *vcpu_or_null,
+                        struct kvm_mmu_page *sp,
+                        struct list_head *invalid_list)
 {
-     int ret = vcpu->arch.mmu->sync_page(vcpu, sp);
+     int ret = -1;
+
+     if (vcpu_or_null)

This should never happen. I like the idea of warning early, but I really don't like that the WARN is far removed from the code that actually depends on @vcpu being non-NULL. Case in point, KVM should have bailed on the WARN and never reached this point. And the inner __kvm_sync_page() is completely unnecessary.

Yeah that's fair.

Quoted 1 line

Quoted 6 lines

Quoted 2 lines

I also don't love the vcpu_or_null terminology; I get the intent, but it doesn't really help because understand why/when it's NULL.

Eh, I don't think it needs to encode why or when. It just needs to flag to the reader (and future code authors) that this vcpu pointer (unlike all other vcpu pointers in KVM) is NULL in certain cases.

My objection is that without the why/when, developers that aren't familiar with this code won't know the rules for using vcpu_or_null. E.g. I don't want to end up with

        if (vcpu_or_null)
                do x;
        else
                do y;

because inevitably it'll become unclear whether or not that code is actually _correct_. It might not #GP on a NULL pointer, but it doesn't mean it's correct.

Quoted 7 lines

Quoted 5 lines

I played around with casting, e.g. to/from an unsigned long or void *, to prevent usage, but that doesn't work very well because 'unsigned long' ends up being awkward/confusing, and 'void *' is easily lost on a function call. And both lose type safety :-(

Yet another shortcoming of C :(

And lack of closures, which would work very well here.

Quoted 12 lines

(The other being our other discussion about the RET_PF* return codes getting easily misinterpreted as KVM's magic return-to-user / continue-running-guest return codes.)

Makes me miss Rust!

Quoted 5 lines

All in all, I think I'd prefer this patch to simply be a KVM_BUG_ON() if kvm_mmu_find_shadow_page() encounters an unsync page. Less churn, and IMO there's no real loss in robustness, e.g. we'd really have to screw up code review and testing to introduce a null vCPU pointer dereference in this code.

Quoted 4 lines

Agreed about moving the check here and dropping __kvm_sync_page(). But I would prefer to retain the vcpu_or_null name (or at least something other than "vcpu" to indicate there's something non-standard about this pointer).

The least awful idea I've come up with is wrapping the vCPU in a struct, e.g.

        struct sync_page_info {
                void *vcpu;
        }

That provides the contextual information I want, and also provides the hint that something is odd about the vcpu, which you want. It's like a very poor man's closure :-)

The struct could even be passed by value to avoid the miniscule overhead, and to make readers look extra hard because it's that much more wierd.

arch/x86/kvm/mmu/mmu.c +21 -5

index 3d102522804a..068be77a4fff 100644
@@ -2003,8 +2003,13 @@ static void clear_sp_write_flooding_count(u64 *spte)
        __clear_sp_write_flooding_count(sptep_to_sp(spte));
 }
 
+/* Wrapper to make it difficult to dereference a potentially NULL @vcpu. */
+struct sync_page_info {
+       void *vcpu;
+};
+
 static struct kvm_mmu_page *kvm_mmu_find_shadow_page(struct kvm *kvm,
-                                                    struct kvm_vcpu *vcpu,
+                                                    struct sync_page_info spi,
                                                     gfn_t gfn,
                                                     struct hlist_head *sp_list,
                                                     union kvm_mmu_page_role role)

@@ -2041,6 +2046,13 @@ static struct kvm_mmu_page *kvm_mmu_find_shadow_page(struct kvm *kvm,
                        goto out;
 
                if (sp->unsync) {
+                       /*
+                        * Getting indirect shadow pages without a valid @spi
+                        * is not supported, i.e. this should never happen.
+                        */
+                       if (KVM_BUG_ON(!spi.vcpu, kvm))
+                               break;
+
                        /*
                         * The page is good, but is stale.  kvm_sync_page does
                         * get the latest guest state, but (unlike mmu_unsync_children)

@@ -2053,7 +2065,7 @@ static struct kvm_mmu_page *kvm_mmu_find_shadow_page(struct kvm *kvm,
                         * If the sync fails, the page is zapped.  If so, break
                         * in order to rebuild it.
                         */
-                       ret = kvm_sync_page(vcpu, sp, &invalid_list);
+                       ret = kvm_sync_page(spi.vcpu, sp, &invalid_list);
                        if (ret < 0)
                                break;
 

@@ -2120,7 +2132,7 @@ static struct kvm_mmu_page *kvm_mmu_alloc_shadow_page(struct kvm *kvm,
 }
 
 static struct kvm_mmu_page *__kvm_mmu_get_shadow_page(struct kvm *kvm,
-                                                     struct kvm_vcpu *vcpu,
+                                                     struct sync_page_info spi,
                                                      struct shadow_page_caches *caches,
                                                      gfn_t gfn,
                                                      union kvm_mmu_page_role role)

@@ -2131,7 +2143,7 @@ static struct kvm_mmu_page *__kvm_mmu_get_shadow_page(struct kvm *kvm,
 
        sp_list = &kvm->arch.mmu_page_hash[kvm_page_table_hashfn(gfn)];
 
-       sp = kvm_mmu_find_shadow_page(kvm, vcpu, gfn, sp_list, role);
+       sp = kvm_mmu_find_shadow_page(kvm, spi, gfn, sp_list, role);
        if (!sp) {
                created = true;
                sp = kvm_mmu_alloc_shadow_page(kvm, caches, gfn, sp_list, role);

@@ -2151,7 +2163,11 @@ static struct kvm_mmu_page *kvm_mmu_get_shadow_page(struct kvm_vcpu *vcpu,
                .gfn_array_cache = &vcpu->arch.mmu_gfn_array_cache,
        };
 
-       return __kvm_mmu_get_shadow_page(vcpu->kvm, vcpu, &caches, gfn, role);
+       struct sync_page_info spi = {
+               .vcpu = vcpu,
+       };
+
+       return __kvm_mmu_get_shadow_page(vcpu->kvm, spi, &caches, gfn, role);
 }
 
 static union kvm_mmu_page_role kvm_mmu_child_role(u64 *sptep, bool direct, u32 access)

arch/x86/kvm/mmu/mmu.c +2 -2

-static void link_shadow_page(struct kvm_vcpu *vcpu, u64 *sptep,
-			     struct kvm_mmu_page *sp)
+static void __link_shadow_page(struct kvm_mmu_memory_cache *cache, u64 *sptep,
+			       struct kvm_mmu_page *sp)
 {
 	u64 spte;

@@ -2297,12 +2300,17 @@ static void link_shadow_page(struct kvm_vcpu *vcpu, u64 *sptep,
 

arch/x86/kvm/mmu/mmu.c +4 -0

+{
+	__link_shadow_page(&vcpu->arch.mmu_pte_list_desc_cache, sptep, sp);
+}
+
 static void validate_direct_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 				   unsigned direct_access)
 {

Signature · 1 line

2.36.0.rc2.479.g8af0fa9b8e-goog

Quoted 14 lines · David Matlack

arch/x86/kvm/mmu/mmu.c +3 -3

-static void link_shadow_page(struct kvm_vcpu *vcpu, u64 *sptep,
-                          struct kvm_mmu_page *sp)
+static void __link_shadow_page(struct kvm_mmu_memory_cache *cache, u64 *sptep,
+                            struct kvm_mmu_page *sp)
 {
      u64 spte;

@@ -2297,12 +2300,17 @@ static void link_shadow_page(struct kvm_vcpu *vcpu, u64 *sptep,
 
      mmu_spte_set(sptep, spte);
 
-     mmu_page_add_parent_pte(vcpu, sp, sptep);
+     mmu_page_add_parent_pte(cache, sp, sptep);
 

arch/x86/kvm/mmu/mmu.c static void link_shadow_page(struct kvm_vcpu *vcpu, u64 *sptep, · line 2309

Quoted 5 lines

arch/x86/kvm/mmu/mmu.c +1 -0

      if (sp->unsync_children || sp->unsync)
              mark_unsync(sptep);
 }
 
+static void link_shadow_page(struct kvm_vcpu *vcpu, u64 *sptep, struct kvm_mmu_page *sp)

Nit, would prefer to wrap here, especially since __link_shadow_page() wraps.

Quoted 11 lines

arch/x86/kvm/mmu/mmu.c +4 -1

+{
+     __link_shadow_page(&vcpu->arch.mmu_pte_list_desc_cache, sptep, sp);
+}
+
 static void validate_direct_spte(struct kvm_vcpu *vcpu, u64 *sptep,
                                 unsigned direct_access)
 {
--

2.36.0.rc2.479.g8af0fa9b8e-goog

arch/x86/kvm/mmu/paging_tmpl.h +2 -1

index a8a755e1561d..97bf53b29b88 100644
@@ -978,7 +978,8 @@ static gpa_t FNAME(gva_to_gpa)(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
 }
 
 /*
- * Using the cached information from sp->gfns is safe because:
+ * Using the information in sp->shadowed_translation (kvm_mmu_page_get_gfn()
+ * and kvm_mmu_page_get_access()) is safe because:
  * - The spte has a reference to the struct page, so the pfn for a given gfn
  *   can't change unless all sptes pointing to it are nuked first.
  *

@@ -1052,12 +1053,15 @@ static int FNAME(sync_page)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)
 		if (sync_mmio_spte(vcpu, &sp->spt[i], gfn, pte_access))
 			continue;

arch/x86/kvm/mmu/paging_tmpl.h +1 -0

+
 		sptep = &sp->spt[i];
 		spte = *sptep;
 		host_writable = spte & shadow_host_writable_mask;

Signature · 1 line

2.36.0.rc2.479.g8af0fa9b8e-goog

arch/x86/kvm/mmu/mmu.c +4 -1

@@ -2820,7 +2861,10 @@ static int mmu_set_spte(struct kvm_vcpu *vcpu, struct kvm_memory_slot *slot,
 
 	if (!was_rmapped) {
 		WARN_ON_ONCE(ret == RET_PF_SPURIOUS);
-		rmap_add(vcpu, slot, sptep, gfn);
+		rmap_add(vcpu, slot, sptep, gfn, pte_access);
+	} else {
+		/* Already rmapped but the pte_access bits may have changed. */
+		kvm_mmu_page_set_access(sp, sptep - sp->spt, pte_access);
 	}
 
 	return ret;

arch/x86/kvm/mmu/paging_tmpl.h +2 -1

index a8a755e1561d..97bf53b29b88 100644
@@ -978,7 +978,8 @@ static gpa_t FNAME(gva_to_gpa)(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
 }
 
 /*
- * Using the cached information from sp->gfns is safe because:
+ * Using the information in sp->shadowed_translation (kvm_mmu_page_get_gfn()
+ * and kvm_mmu_page_get_access()) is safe because:
  * - The spte has a reference to the struct page, so the pfn for a given gfn
  *   can't change unless all sptes pointing to it are nuked first.
  *

@@ -1052,12 +1053,15 @@ static int FNAME(sync_page)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)
 		if (sync_mmio_spte(vcpu, &sp->spt[i], gfn, pte_access))

arch/x86/kvm/mmu/paging_tmpl.h +2 -0

+			kvm_mmu_page_set_access(sp, i, pte_access);
+
 		sptep = &sp->spt[i];
 		spte = *sptep;
 		host_writable = spte & shadow_host_writable_mask;

Signature · 1 line

2.36.0.rc2.479.g8af0fa9b8e-goog

Quoted 12 lines · David Matlack

arch/x86/kvm/mmu/mmu.c +4 -1

@@ -2820,7 +2861,10 @@ static int mmu_set_spte(struct kvm_vcpu *vcpu, struct kvm_memory_slot *slot,
 
      if (!was_rmapped) {
              WARN_ON_ONCE(ret == RET_PF_SPURIOUS);
-             rmap_add(vcpu, slot, sptep, gfn);
+             rmap_add(vcpu, slot, sptep, gfn, pte_access);
+     } else {
+             /* Already rmapped but the pte_access bits may have changed. */
+             kvm_mmu_page_set_access(sp, sptep - sp->spt, pte_access);
      }
 
      return ret;

...

Quoted 25 lines

arch/x86/kvm/mmu/paging_tmpl.h +3 -2

index a8a755e1561d..97bf53b29b88 100644
@@ -978,7 +978,8 @@ static gpa_t FNAME(gva_to_gpa)(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
 }
 
 /*
- * Using the cached information from sp->gfns is safe because:
+ * Using the information in sp->shadowed_translation (kvm_mmu_page_get_gfn()
+ * and kvm_mmu_page_get_access()) is safe because:
  * - The spte has a reference to the struct page, so the pfn for a given gfn
  *   can't change unless all sptes pointing to it are nuked first.
  *

@@ -1052,12 +1053,15 @@ static int FNAME(sync_page)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)
              if (sync_mmio_spte(vcpu, &sp->spt[i], gfn, pte_access))
                      continue;
 
-             if (gfn != sp->gfns[i]) {
+             if (gfn != kvm_mmu_page_get_gfn(sp, i)) {
                      drop_spte(vcpu->kvm, &sp->spt[i]);
                      flush = true;
                      continue;
              }
 

arch/x86/kvm/mmu/paging_tmpl.h static int FNAME(sync_page)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp) · line 1062

Quoted 1 line

arch/x86/kvm/mmu/paging_tmpl.h +1 -0

+             if (pte_access != kvm_mmu_page_get_access(sp, i))

I think it makes sense to do this unconditionally, same as mmu_set_spte(). Or make the mmu_set_spte() case conditional. I don't have a strong preference either way, but the two callers should be consistent with each other.

Quoted 9 lines

arch/x86/kvm/mmu/paging_tmpl.h +2 -1

+                     kvm_mmu_page_set_access(sp, i, pte_access);
+
              sptep = &sp->spt[i];
              spte = *sptep;
              host_writable = spte & shadow_host_writable_mask;
--

2.36.0.rc2.479.g8af0fa9b8e-goog