KVM: MMU: don't drop spte if overwrite it from W to RO

From: Lai Jiangshan <[email protected]>

Rename it to fix the sense better

We can past the page fault to guest directly if gpte's reserved is set

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

---
  arch/x86/kvm/paging_tmpl.h |   10 ++++++----
  1 files changed, 6 insertions(+), 4 deletions(-)

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

index 291342d..952357a 100644
@@ -760,6 +760,7 @@ static int FNAME(sync_page)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
  		pt_element_t gpte;
  		gpa_t pte_gpa;
  		gfn_t gfn;
+		bool gpte_invalid;
 
  		if (!is_shadow_present_pte(sp->spt[i]))
  			continue;

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

+		if (gpte_invalid || gfn != sp->gfns[i] ||
+		      !(gpte&  PT_ACCESSED_MASK)) {
  			u64 nonpresent;
 
-			if (is_present_gpte(gpte) || !clear_unsync)
+			if (gpte_invalid || !clear_unsync)
  				nonpresent = shadow_trap_nonpresent_pte;
  			else
  				nonpresent = shadow_notrap_nonpresent_pte;

Quoted 3 lines · Xiao Guangrong

We can past the page fault to guest directly if gpte's reserved is set

How can that work? shadow_notrap_nonpresent_pte causes a fault with PFEC.P=PFEC.RSVD=0, while we need PFEC.P=PFEC.RSVD=1.

Quoted 5 lines · Avi Kivity

Quoted 3 lines · Xiao Guangrong

We can past the page fault to guest directly if gpte's reserved is set

Quoted 3 lines

How can that work? shadow_notrap_nonpresent_pte causes a fault with PFEC.P=PFEC.RSVD=0, while we need PFEC.P=PFEC.RSVD=1.

Ah, i missed it for a long time, thanks for you point it out.

The same mistake is in 'prefetch' path, i'll fix it in the v2 version.

Quoted 10 lines · Xiao Guangrong

Quoted 8 lines · Avi Kivity

Quoted 3 lines · Xiao Guangrong

We can past the page fault to guest directly if gpte's reserved is set

How can that work? shadow_notrap_nonpresent_pte causes a fault with PFEC.P=PFEC.RSVD=0, while we need PFEC.P=PFEC.RSVD=1.

Quoted 3 lines

Ah, i missed it for a long time, thanks for you point it out.

The same mistake is in 'prefetch' path, i'll fix it in the v2 version.

Doesn't access.flat catch this?

Ideally we'd have a test case to catch this, but it may be hard to write.

Some operation of these functions is very similar, so introduce a common function to cleanup them

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

---
 arch/x86/kvm/mmu.c         |    3 -
 arch/x86/kvm/paging_tmpl.h |  191 ++++++++++++++++++++++++-------------------
 2 files changed, 107 insertions(+), 87 deletions(-)

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

index 94d157f..d0bcca2 100644
@@ -3108,9 +3108,6 @@ static void mmu_pte_write_new_pte(struct kvm_vcpu *vcpu,
 		return;
         }
 
-	if (is_rsvd_bits_set(&vcpu->arch.mmu, *(u64 *)new, PT_PAGE_TABLE_LEVEL))
-		return;
-
 	++vcpu->kvm->stat.mmu_pte_updated;
 	if (!sp->role.cr4_pae)
 		paging32_update_pte(vcpu, sp, spte, new);

arch/x86/kvm/paging_tmpl.h +25 -15

index 952357a..1a1a0b9 100644
@@ -299,42 +299,90 @@ static int FNAME(walk_addr_nested)(struct guest_walker *walker,
 					addr, access);
 }
 
-static void FNAME(update_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
-			      u64 *spte, const void *pte)
+static bool FNAME(fetch_guest_pte)(struct kvm_vcpu *vcpu,
+				   struct kvm_mmu_page *sp, u64 *spte,
+				   bool clear_unsync, pt_element_t gpte,
+				   pfn_t (get_pfn)(struct kvm_vcpu *, u64 *,
+					  pt_element_t, unsigned, bool *))
 {
-	pt_element_t gpte;
 	unsigned pte_access;
+	u64 nonpresent = shadow_trap_nonpresent_pte;
+	gfn_t gfn;
 	pfn_t pfn;
-	u64 new_spte;
+	bool dirty, host_writeable;
 
-	gpte = *(const pt_element_t *)pte;
-	if (~gpte & (PT_PRESENT_MASK | PT_ACCESSED_MASK)) {
-		if (!is_present_gpte(gpte)) {
-			if (sp->unsync)
-				new_spte = shadow_trap_nonpresent_pte;
-			else
-				new_spte = shadow_notrap_nonpresent_pte;
-			__set_spte(spte, new_spte);
-		}
-		return;
+	if (!is_present_gpte(gpte) ||
+	      is_rsvd_bits_set(&vcpu->arch.mmu, gpte, PT_PAGE_TABLE_LEVEL)) {
+		if (!sp->unsync && !clear_unsync)
+			nonpresent = shadow_notrap_nonpresent_pte;
+		goto no_present;
 	}
-	pgprintk("%s: gpte %llx spte %p\n", __func__, (u64)gpte, spte);
+
+	if (!(gpte & PT_ACCESSED_MASK))
+		goto no_present;
+
 	pte_access = sp->role.access & FNAME(gpte_access)(vcpu, gpte);
+	gfn = gpte_to_gfn(gpte);
+	dirty = is_dirty_gpte(gpte);
+	pfn = get_pfn(vcpu, spte, gpte, pte_access, &host_writeable);
+
+	if (is_error_pfn(pfn))
+		goto no_present;
+
+	if (!host_writeable)

arch/x86/kvm/paging_tmpl.h static int FNAME(walk_addr_nested)(struct guest_walker *walker, · line 342

Quoted 4 lines

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

+	else
+		mmu_set_spte(vcpu, spte, sp->role.access, pte_access, 0, 0,
+			     dirty, NULL, PT_PAGE_TABLE_LEVEL, gfn,
+			     pfn, true, host_writeable);

For example, the update path should always go through mmu_set_spte to update last_pte_updated, last_pte_gfn.

Also the callbacks make it harder to read the code. Maybe the unification works if you use common functions for common parts.

We just need flush tlb if overwrite a writable spte with a read-only one

Quoted 2 lines · Xiao Guangrong

We just need flush tlb if overwrite a writable spte with a read-only one

What are the advantages? Avoid playing with rmap, and avoid a window where the spte is missing?

We just need flush tlb if overwrite a writable spte with a read-only one

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

---
 arch/x86/kvm/mmu.c |   19 +++++++++----------
 1 files changed, 9 insertions(+), 10 deletions(-)

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

index 4b6d54c..1a93ab4 100644
@@ -2044,6 +2044,15 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 	if (pte_access & ACC_WRITE_MASK)
 		mark_page_dirty(vcpu->kvm, gfn);

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

 set_pte:
 	update_spte(sptep, spte);
 done:

@@ -2084,16 +2093,6 @@ static void mmu_set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 				 spte_to_pfn(*sptep), pfn);
 			drop_spte(vcpu->kvm, sptep, shadow_trap_nonpresent_pte);
 			kvm_flush_remote_tlbs(vcpu->kvm);
-		/*
-		 * If we overwrite a writable spte with a read-only one,

Quoted 15 lines · Xiao Guangrong

We just need flush tlb if overwrite a writable spte with a read-only one

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

---
 arch/x86/kvm/mmu.c |   19 +++++++++----------
 1 files changed, 9 insertions(+), 10 deletions(-)

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

index 4b6d54c..1a93ab4 100644
@@ -2044,6 +2044,15 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 	if (pte_access & ACC_WRITE_MASK)
 		mark_page_dirty(vcpu->kvm, gfn);
 

arch/x86/kvm/mmu.c static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep, · line 2055

Quoted 9 lines

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

+	/*
+	 * If we overwrite a writable spte with a read-only one,
+	 * flush remote TLBs. Otherwise rmap_write_protect will
+	 * find a read-only spte, even though the writable spte
+	 * might be cached on a CPU's TLB.
+	 */
+	else if (is_writable_pte(*sptep))
+		ret = 1;
+

The return value of set_spte indicates whether the gfn being mapped to was write protected, not if a TLB flush is necessary.