KVM: MMU: fix direct sp's access corruptted

In no-direct mapping, we mark sp is 'direct' when we mapping the guest's larger page, but its access is encoded form upper page-struct entire not include the last mapping, it will cause access conflict.

For example, have this mapping:

         [W]
       / PDE1 ->  |---|
   P[W]          |   | LPA
       \ PDE2 ->  |---|
         [R]

P have two children, PDE1 and PDE2, both PDE1 and PDE2 mapping the same lage page(LPA). The P's access is WR, PDE1's access is WR, PDE2's access is RO(just consider read-write permissions here)

When guest access PDE1, we will create a direct sp for LPA, the sp's access is from P, is W, then we will mark the ptes is W in this sp.

Then, guest access PDE2, we will find LPA's shadow page, is the same as PDE's, and mark the ptes is RO.

So, if guest access PDE1, the incorrect #PF is occured.

Fixed by encode the last mapping access into direct shadow page

And, it also cleanup the code that directly get the last level's dirty flag

Quoted 1 line

Looks good, but please split the cleanup from the fix (we'll want to backport the fix but not the cleanup).

Consider using small page to fit guest's large page mapping:

If the mapping is writable but the dirty flag is not set, we will find the read-only direct sp and setup the mapping, then if the write #PF occur, we will mark this mapping writable in the read-only direct sp, now, other real read-only mapping will happily write it without #PF.

It may hurt guest's COW

Fixed by re-install the mapping when write #PF occur.

Signed-off-by Xiao Guangrong<[email protected]>

---
  arch/x86/kvm/mmu.c         |    3 ++-
  arch/x86/kvm/paging_tmpl.h |   18 ++++++++++++++++++
  2 files changed, 20 insertions(+), 1 deletions(-)

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

index 556a798..0412ba4 100644
@@ -153,7 +153,8 @@ module_param(oos_shadow, bool, 0644);
  #define CREATE_TRACE_POINTS
  #include "mmutrace.h"
 
-#define SPTE_HOST_WRITEABLE (1ULL<<  PT_FIRST_AVAIL_BITS_SHIFT)
+#define SPTE_HOST_WRITEABLE	(1ULL<<  PT_FIRST_AVAIL_BITS_SHIFT)
+#define SPTE_NO_DIRTY		(2ULL<<  PT_FIRST_AVAIL_BITS_SHIFT)
 
  #define SHADOW_PT_INDEX(addr, level) PT64_INDEX(addr, level)
 

arch/x86/kvm/paging_tmpl.h +5 -0

index e46eb8a..fdba751 100644
@@ -325,6 +325,20 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
  			break;
  		}
 
+		if (*sptep&  SPTE_NO_DIRTY) {
+			struct kvm_mmu_page *child;
+
+			WARN_ON(level !=  gw->level);
+			WARN_ON(!is_shadow_present_pte(*sptep));

Instead of adding a new bit, can you encode the protection in the direct sp's access bits? So we'll have one sp for read-only or writeable-but-not-dirty small pages, and another sp for writeable-and-dirty small pages.

Quoted 6 lines · Avi Kivity

Instead of adding a new bit, can you encode the protection in the direct sp's access bits? So we'll have one sp for read-only or writeable-but-not-dirty small pages, and another sp for writeable-and-dirty small pages.

It looks like it can't solve all problems, it fix the access corrupted, but will cause D bit losed:

mapping A and mapping B both are writable-and-dirty, when mapping A write #PF occurs, the mapping is writable, then we can't set B's D bit anymore.

Anyway, i think we should re-intall the mapping when the state is changed. :-(

Quoted 10 lines · Xiao Guangrong

Quoted 6 lines · Avi Kivity

Instead of adding a new bit, can you encode the protection in the direct sp's access bits? So we'll have one sp for read-only or writeable-but-not-dirty small pages, and another sp for writeable-and-dirty small pages.

Quoted 6 lines

It looks like it can't solve all problems, it fix the access corrupted, but will cause D bit losed:

mapping A and mapping B both are writable-and-dirty, when mapping A write #PF occurs, the mapping is writable, then we can't set B's D bit anymore.

If B is writeable-and-dirty, then it's D bit is already set, and we don't need to do anything.

If B is writeable-and-clean, then we'll have an spte pointing to a read-only sp, so we'll get a write fault on access and an opportunity to set the D bit.

Quoted 3 lines

Anyway, i think we should re-intall the mapping when the state is changed. :-(

When the gpte is changed from read-only to writeable or from clean to dirty, we need to update the spte, yes. But that's true for other sptes as well, not just large gptes.

Quoted 9 lines

If B is writeable-and-dirty, then it's D bit is already set, and we don't need to do anything.

If B is writeable-and-clean, then we'll have an spte pointing to a read-only sp, so we'll get a write fault on access and an opportunity to set the D bit.

Sorry, a typo in my reply, i mean mapping A and B both are writable-and-clean, while A occurs write-#PF, we should change A's spte map to writable sp, if we only update the spte in writable-and-clean sp(form readonly to writable), the B's D bit will miss set.

Quoted 9 lines

Quoted 4 lines

Anyway, i think we should re-intall the mapping when the state is changed. :-(

When the gpte is changed from read-only to writeable or from clean to dirty, we need to update the spte, yes. But that's true for other sptes as well, not just large gptes.

I think the indirect sp is not hurt by this bug since for the indirect sp, the access just form its upper-level, and the D bit is only in the last level, when we change the pte's access, is not affect its sp's access.

But for direct sp, the sp's access is form all level. and different mapping that not share the last mapping page will have the same last sp.

Quoted 9 lines · Xiao Guangrong

Quoted 9 lines

If B is writeable-and-dirty, then it's D bit is already set, and we don't need to do anything.

If B is writeable-and-clean, then we'll have an spte pointing to a read-only sp, so we'll get a write fault on access and an opportunity to set the D bit.

Quoted 8 lines

Sorry, a typo in my reply, i mean mapping A and B both are writable-and-clean,
while A occurs write-#PF, we should change A's spte map to writable sp, if we
only update the spte in writable-and-clean sp(form readonly to writable), the B's
D bit will miss set.

Right.

We need to update something to notice this:

- FNAME(fetch)() to replace the spte
- FNAME(walk_addr)() to invalidate the spte

I think FNAME(walk_addr) is the right place, we're updating the gpte, so we should update the spte at the same time, just like a guest write. But that will be expensive (there could be many sptes, so we have to call kvm_mmu_pte_write()), so perhaps FNAME(fetch) is easier.

We have now

        if (is_shadow_present_pte(*sptep) && !is_large_pte(*sptep))
            continue;

So we need to add a check, if sp->role.access doesn't match pt_access & pte_access, we need to get a new sp with the correct access (can only change read->write).

Quoted 11 lines

We have now

         if (is_shadow_present_pte(*sptep)&&  !is_large_pte(*sptep))
             continue;

So we need to add a check, if sp->role.access doesn't match pt_access& pte_access, we need to get a new sp with the correct access (can only change read->write).

Umm, we should update the spte at the gw->level, so we need get the child sp, and compare its access at this point, just like this:

if (level == gw->level&&  is_shadow_present_pte(*sptep)) {
	child_sp = page_header(__pa(*sptep&  PT64_BASE_ADDR_MASK));

	if (child_sp->access != pt_access&  pte_access&  (diry ? 1 : ~ACC_WRITE_MASK )) {
		/* Zap sptep */
		......
	}

}

So, why not use the new spte flag (SPTE_NO_DIRTY in my patch) to mark this spte then we can see this spte whether need updated directly? i think it more simpler ;-)

(quoted diff) +6 -0

+		if (*sptep&  SPTE_NO_DIRTY) {
+			struct kvm_mmu_page *child;
+
+			WARN_ON(level !=  gw->level);
+			WARN_ON(!is_shadow_present_pte(*sptep));
+			if (dirty) {

Quoted 23 lines · Xiao Guangrong

Quoted 11 lines

We have now

         if (is_shadow_present_pte(*sptep)&&  !is_large_pte(*sptep))
             continue;

So we need to add a check, if sp->role.access doesn't match pt_access& pte_access, we need to get a new sp with the correct access (can only change read->write).

Umm, we should update the spte at the gw->level, so we need get the child sp, and compare its access at this point, just like this:

if (level == gw->level&&  is_shadow_present_pte(*sptep)) {
    child_sp = page_header(__pa(*sptep&  PT64_BASE_ADDR_MASK));

    if (child_sp->access != pt_access&  pte_access&  (diry ? 1 :
~ACC_WRITE_MASK )) {
        /* Zap sptep */
        ......
    }

Quoted 6 lines

}

So, why not use the new spte flag (SPTE_NO_DIRTY in my patch) to mark this spte then we can see
this spte whether need updated directly? i think it more simpler ;-)

It's new state, and new state means more maintenance of that state and the need to consider the state in all relevant code paths.

In terms of maintainability, changing walk_addr() is best, since it maintains the tight invariant that PT_PAGE_DIRECTORY_LEVEL sptes are always consistent with their sptes. Updating fetch() to allow for a relaxed invariant (spte may be read-only while gpte is write-dirty) is more complicated, but performs better. This is also consistent with what we do with PT_PAGE_TABLE_LEVEL gptes/sptes and with unsync pages.

btw, how can the patch work?

Review

Quoted 9 lines

(quoted diff) +4 -0

+        if (level == gw->level&&  !dirty&&
+              access&  gw->pte_access&  ACC_WRITE_MASK)
+            spte |= SPTE_NO_DIRTY;
+
          spte = __pa(sp->spt)
              | PT_PRESENT_MASK | PT_ACCESSED_MASK
              | PT_WRITABLE_MASK | PT_USER_MASK;

spte is immediately overwritten by the following assignment.

However, the other half of the patch can be adapted:

Quoted 13 lines

(quoted diff) +12 -0

+        if (*sptep&  SPTE_NO_DIRTY) {
+            struct kvm_mmu_page *child;
+
+            WARN_ON(level !=  gw->level);
+            WARN_ON(!is_shadow_present_pte(*sptep));
+            if (dirty) {
+                child = page_header(*sptep&
+                              PT64_BASE_ADDR_MASK);
+                mmu_page_remove_parent_pte(child, sptep);
+                __set_spte(sptep, shadow_trap_nonpresent_pte);
+                kvm_flush_remote_tlbs(vcpu->kvm);
+            }

Review

Quoted 5 lines

(quoted diff) +2 -0

+        }
+
          if (is_shadow_present_pte(*sptep)&&  !is_large_pte(*sptep))
              continue;

Simply replace (*spte & SPTE_NO_DIRTY) with a condition that checks whether sp->access is consistent with gw->pt(e)_access.

Can you write a test case for qemu-kvm.git/kvm/test that demonstrates the problem and the fix? It will help ensure we don't regress in this area.

Quoted 5 lines

Simply replace (*spte& SPTE_NO_DIRTY) with a condition that checks whether sp->access is consistent with gw->pt(e)_access.

If the guest mapping is writable and it !dirty, we mark SPTE_NO_DIRTY flag in the spte, when the next #PF occurs, we just need check this flag and see whether gpte's D bit is set, if it's true, we zap this spte and map to the correct sp.

Quoted 5 lines

Can you write a test case for qemu-kvm.git/kvm/test that demonstrates the problem and the fix? It will help ensure we don't regress in this area.

OK, but allow me do it later :-)

Quoted 5 lines · Xiao Guangrong

Quoted 5 lines

Simply replace (*spte& SPTE_NO_DIRTY) with a condition that checks whether sp->access is consistent with gw->pt(e)_access.

Quoted 7 lines

If the guest mapping is writable and it !dirty, we mark SPTE_NO_DIRTY flag in
the spte, when the next #PF occurs, we just need check this flag and see whether
gpte's D bit is set, if it's true, we zap this spte and map to the correct sp.

My point is, SPTE_NO_DIRTY is equivalent to an sp->role.access check (the access check is a bit slower, but that shouldn't matter).

Quoted 9 lines

Quoted 6 lines

Can you write a test case for qemu-kvm.git/kvm/test that demonstrates the problem and the fix? It will help ensure we don't regress in this area.

OK, but allow me do it later :-)

Sure, but please do it soon.

Quoted 4 lines · Xiao Guangrong

Quoted 4 lines

If B is writeable-and-dirty, then it's D bit is already set, and we don't need to do anything.

Quoted 8 lines

Quoted 4 lines

If B is writeable-and-clean, then we'll have an spte pointing to a read-only sp, so we'll get a write fault on access and an opportunity to set the D bit.

Sorry, a typo in my reply, i mean mapping A and B both are writable-and-clean, while A occurs write-#PF, we should change A's spte map to writable sp, if we only update the spte in writable-and-clean sp(form readonly to writable), the B's D bit will miss set.

Right.

We need to update something to notice this:

- FNAME(fetch)() to replace the spte
- FNAME(walk_addr)() to invalidate the spte

I think FNAME(walk_addr) is the right place, we're updating the gpte, so we should update the spte at the same time, just like a guest write. But that will be expensive (there could be many sptes, so we have to call kvm_mmu_pte_write()), so perhaps FNAME(fetch) is easier.

We have now

        if (is_shadow_present_pte(*sptep) && !is_large_pte(*sptep))
            continue;

So we need to add a check, if sp->role.access doesn't match pt_access & pte_access, we need to get a new sp with the correct access (can only change read->write).

Note:

- modifying walk_addr() to call kvm_mmu_pte_write() is probably not so bad. It's rare that a large pte walk sets the dirty bit, and it's probably rare to share those large ptes. Still, I think the fetch() change is better since it's more local.

- there was once talk that instead of folding pt_access and pte_access together into the leaf sp->role.access, each sp level would have its own access permissions. In this case we don't even have to get a new direct sp, only change the PT_DIRECTORY_LEVEL spte to add write permissions (all direct sp's would be writeable and permissions would be controlled at their parent_pte level). Of course that's a much bigger change than this bug fix.

Quoted 10 lines

- there was once talk that instead of folding pt_access and pte_access together into the leaf sp->role.access, each sp level would have its own access permissions. In this case we don't even have to get a new direct sp, only change the PT_DIRECTORY_LEVEL spte to add write permissions (all direct sp's would be writeable and permissions would be controlled at their parent_pte level). Of course that's a much bigger change than this bug fix.

Yeah, i have considered this way, but it will change the shadow page's mapping
way: it control the access at the upper level, but in the current code, we allow the upper level have the ALL_ACCESS and control the access right at the last level. It will break many things, such as write-protected...

Quoted 10 lines · Xiao Guangrong

Quoted 10 lines

- there was once talk that instead of folding pt_access and pte_access together into the leaf sp->role.access, each sp level would have its own access permissions. In this case we don't even have to get a new direct sp, only change the PT_DIRECTORY_LEVEL spte to add write permissions (all direct sp's would be writeable and permissions would be controlled at their parent_pte level). Of course that's a much bigger change than this bug fix.

Quoted 8 lines

Yeah, i have considered this way, but it will change the shadow page's mapping
way: it control the access at the upper level, but in the current code, we allow
the upper level have the ALL_ACCESS and control the access right at the last level.
It will break many things, such as write-protected...

spte's access bits have dual purpose, both to map guest protection and for host protection (like for shadowed pages, or ksm pages). So the last level sptes still need to consider host write protection.

After remove a rmap, we should flush all vcpu's tlb

Signed-off-by Xiao Guangrong<[email protected]>

---
  arch/x86/kvm/mmu.c |    2 ++
  1 files changed, 2 insertions(+), 0 deletions(-)

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

index 0412ba4..f151540 100644

Introduce mmu_topup_memory_cache_atomic(), it support topup memory cache in atomic context

Quoted 4 lines · Xiao Guangrong

Introduce mmu_topup_memory_cache_atomic(), it support topup memory cache in atomic context

Can instead preallocate enough for all prefetches, isn't it simpler?

Support prefetch ptes when intercept guest #PF, avoid to #PF by later access

If we meet any failure in the prefetch path, we will exit it and not try other ptes to avoid become heavy path

Note: this speculative will mark page become dirty but it not really accessed, the same issue is in other speculative paths like invlpg, pte write, fortunately, it just affect host memory management. After Avi's patchset named "[PATCH v2 1/4] KVM: MMU: Introduce drop_spte()" merged, we will easily fix it. Will do it in the future.

Signed-off-by Xiao Guangrong <[email protected]>

---
 arch/x86/kvm/mmu.c         |   69 +++++++++++++++++++++++++++++++++++++++++
 arch/x86/kvm/paging_tmpl.h |   74 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 143 insertions(+), 0 deletions(-)

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

index 6c06666..b2ad723 100644
@@ -89,6 +89,8 @@ module_param(oos_shadow, bool, 0644);
 	}
 #endif
 
+#define PTE_PREFETCH_NUM	16
+
 #define PT_FIRST_AVAIL_BITS_SHIFT 9
 #define PT64_SECOND_AVAIL_BITS_SHIFT 52
 

@@ -1998,6 +2000,72 @@ static void nonpaging_new_cr3(struct kvm_vcpu *vcpu)
 {
 }
 
+static int direct_pte_prefetch_many(struct kvm_vcpu *vcpu,
+				    struct kvm_mmu_page *sp,
+				    u64 *start, u64 *end)
+{
+	gfn_t gfn;
+	struct page *pages[PTE_PREFETCH_NUM];
+
+	if (pte_prefetch_topup_memory_cache(vcpu, end - start))
+		return -1;
+
+	gfn = sp->gfn + start - sp->spt;
+	while (start < end) {
+		unsigned long addr;
+		int entry, j, ret;
+
+		addr = gfn_to_hva_many(vcpu->kvm, gfn, &entry);
+		if (kvm_is_error_hva(addr))
+			return -1;
+
+		entry = min(entry, (int)(end - start));
+		ret = __get_user_pages_fast(addr, entry, 1, pages);
+		if (ret <= 0)
+			return -1;
+
+		for (j = 0; j < ret; j++, gfn++, start++)
+			mmu_set_spte(vcpu, start, ACC_ALL,
+				     sp->role.access, 0, 0, 1, NULL,
+				     sp->role.level, gfn,
+				     page_to_pfn(pages[j]), true, false);
+
+		if (ret < entry)
+			return -1;
+	}
+	return 0;
+}
+
+static void direct_pte_prefetch(struct kvm_vcpu *vcpu, u64 *sptep)
+{
+	struct kvm_mmu_page *sp;
+	u64 *start = NULL;
+	int index, i, max;
+
+	sp = page_header(__pa(sptep));
+	WARN_ON(!sp->role.direct);
+
+	if (sp->role.level > PT_PAGE_TABLE_LEVEL)
+		return;
+
+	index = sptep - sp->spt;
+	i = index & ~(PTE_PREFETCH_NUM - 1);
+	max = index | (PTE_PREFETCH_NUM - 1);
+
+	for (; i < max; i++) {
+		u64 *spte = sp->spt + i;
+
+		if (*spte != shadow_trap_nonpresent_pte || spte == sptep) {
+			if (!start)
+				continue;
+			if (direct_pte_prefetch_many(vcpu, sp, start, spte) < 0)
+				break;
+			start = NULL;
+		} else if (!start)
+			start = spte;
+	}
+}
+
 static int __direct_map(struct kvm_vcpu *vcpu, gpa_t v, int write,
 			int level, gfn_t gfn, pfn_t pfn)
 {

@@ -2012,6 +2080,7 @@ static int __direct_map(struct kvm_vcpu *vcpu, gpa_t v, int write,
 				     0, write, 1, &pt_write,
 				     level, gfn, pfn, false, true);
 			++vcpu->stat.pf_fixed;
+			direct_pte_prefetch(vcpu, iterator.sptep);
 			break;
 		}
 

arch/x86/kvm/paging_tmpl.h +5 -0

index fdba751..134f031 100644
@@ -291,6 +291,79 @@ static void FNAME(update_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
 		     gpte_to_gfn(gpte), pfn, true, true);
 }
 
+static void FNAME(pte_prefetch)(struct kvm_vcpu *vcpu, u64 *sptep)
+{
+	struct kvm_mmu_page *sp;
+	pt_element_t gptep[PTE_PREFETCH_NUM];
+	gpa_t first_pte_gpa;

arch/x86/kvm/paging_tmpl.h +58 -0

+
+	index = sptep - sp->spt;
+	i = index & ~(PTE_PREFETCH_NUM - 1);
+	max = index | (PTE_PREFETCH_NUM - 1);
+
+	if (PTTYPE == 32)
+		offset = sp->role.quadrant << PT64_LEVEL_BITS;
+
+	first_pte_gpa = gfn_to_gpa(sp->gfn) +
+				(offset + i) * sizeof(pt_element_t);
+
+	if (kvm_read_guest_atomic(vcpu->kvm, first_pte_gpa, gptep,
+					sizeof(gptep)) < 0)
+		return;
+
+	for (j = 0; i < max; i++, j++) {
+		pt_element_t gpte;
+		unsigned pte_access;
+		u64 *spte = sp->spt + i;
+		gfn_t gfn;
+		pfn_t pfn;
+
+		if (spte == sptep)
+			continue;
+
+		if (*spte != shadow_trap_nonpresent_pte)
+			continue;
+
+		gpte = gptep[j];
+
+		if (is_rsvd_bits_set(vcpu, gpte, PT_PAGE_TABLE_LEVEL))
+			break;
+
+		if (!(gpte & PT_ACCESSED_MASK))
+			continue;
+
+		if (!is_present_gpte(gpte)) {
+			if (!sp->unsync)
+				__set_spte(spte, shadow_notrap_nonpresent_pte);
+			continue;
+		}
+
+		gfn = gpte_to_gfn(gpte);
+
+		pfn = gfn_to_pfn_atomic(vcpu->kvm, gfn);
+		if (is_error_pfn(pfn) ||
+			pte_prefetch_topup_memory_cache(vcpu, 1)) {
+			kvm_release_pfn_clean(pfn);
+			break;
+		}
+
+		pte_access = sp->role.access & FNAME(gpte_access)(vcpu, gpte);
+		mmu_set_spte(vcpu, spte, sp->role.access, pte_access, 0, 0,
+			     is_dirty_gpte(gpte), NULL, sp->role.level, gfn,
+			     pfn, true, false);
+	}
+}
+
 /*
  * Fetch a shadow pte for a specific level in the paging hierarchy.
  */

arch/x86/kvm/paging_tmpl.h static void FNAME(update_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp, · line 308

Quoted 6 lines

arch/x86/kvm/paging_tmpl.h +6 -0

+
+	if (sp->role.level > PT_PAGE_TABLE_LEVEL)
+		return;
+
+	if (sp->role.direct)
+		return direct_pte_prefetch(vcpu, sptep);

Can never happen.

Quoted 5 lines

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

@@ -322,6 +395,7 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 				     user_fault, write_fault,
 				     dirty, ptwrite, level,
 				     gw->gfn, pfn, false, true);
+			FNAME(pte_prefetch)(vcpu, sptep);

Review static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr, · line 400

Quoted 2 lines

(quoted diff) +0 -0

 			break;
 		}

I'm afraid this can introduce regressions since it increases mmu_lock contention. Can you get some numbers with 4-vcpu or 8-vcpu guest and many threads benchmarks, such as kernbench and apachebench? (on non-EPT).

Also prefetch should be disabled for EPT, due to lack of accessed bit.

Quoted 9 lines · Marcelo Tosatti

arch/x86/kvm/paging_tmpl.h static void FNAME(update_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp, · line 308

Quoted 6 lines

arch/x86/kvm/paging_tmpl.h +6 -0

+
+	if (sp->role.level > PT_PAGE_TABLE_LEVEL)
+		return;
+
+	if (sp->role.direct)
+		return direct_pte_prefetch(vcpu, sptep);

Can never happen.

Marcelo,

Thanks for your comment. You mean that we can't meet sp->role.direct here? could you please tell me why? During my test, it can be triggered.

Quoted 5 lines

Quoted 5 lines

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

@@ -322,6 +395,7 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 				     user_fault, write_fault,
 				     dirty, ptwrite, level,
 				     gw->gfn, pfn, false, true);
+			FNAME(pte_prefetch)(vcpu, sptep);

Quoted 9 lines

Review static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr, · line 400

Quoted 2 lines

(quoted diff) +0 -0

 			break;
 		}

I'm afraid this can introduce regressions since it increases mmu_lock contention. Can you get some numbers with 4-vcpu or 8-vcpu guest and many threads benchmarks, such as kernbench and apachebench? (on non-EPT).

The pte prefetch is the fast path, it only occupies little time, for the worst case, only need read 128 byte form the guest pte, and if it prefetched success, the #PF cause by later access will avoid, then we avoid to exit form the guest, and walk guest pte, walk shadow pages, flush local tlb... a lots of work can be reduced.

Before i post this patchset firstly, i do the performance test by using unixbench, it improved ~3.6% under EPT disable case.
(it's in the first version's chagelog)

Today, i do the kernbench test with 4 vcpu and 1G memory, the result shows it improved ~1.6% :-)

Quoted 2 lines

Also prefetch should be disabled for EPT, due to lack of accessed bit.

But we call mmu_set_spte() with speculative == false, it not touch the accessed bit.

Quoted 3 lines

Quoted 2 lines

Also prefetch should be disabled for EPT, due to lack of accessed bit.

But we call mmu_set_spte() with speculative == false, it not touch the accessed bit.

There is no accessed bit on EPT. So the aging code (kvm_age_rmapp) considers any present translation as accessed. There is no way to distinguish between actually accessed translations and prefetched (but unused) ones.