Skip to contentSkip to conversation

KVM: fast page fault

PATCH Superseded Xiao Guangrong 2 participants 17 messages May 23, 2012 – May 24, 2012

Subsystems (4)

v5 of 7 Superseded Latest is v7 →

lore
Share & tools
lore
#0 lore raw

v5 cover KVM: fast page fault

Changlog:
This are some changes from Marcelo's review:
- drop SPTE_WRITE_PROTECT bit, now, only one bit is needed to do lockless update.

- always atomicly update spte if it can be updated out of mmu-lock.

- flod the judgement of tlb flush into mmu_spte_update(), make the code easilyer audited.

Performance result:
- autoest migration test (smp2.Fedora.16.64.migrate.with_autotest.dbench.unix):

                                                   before     after
smp2.Fedora.16.64.migrate.unix                        93      91    +2.1%
smp2.Fedora.16.64.migrate.with_autotest.dbench.unix   218     188   +13.7%

- the benchmark attached is used to measure the resuming time after dirty-log
before after
Run 10 times, Avg time: 512466818 ns. 269231261 ns +47.5%

#1 lore raw

PATCH v5 1/9 KVM: MMU: return bool in __rmap_write_protect

The reture value of __rmap_write_protect is either 1 or 0, use true/false instead of these

---
 arch/x86/kvm/mmu.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)
arch/x86/kvm/mmu.c +7 -6
index 72102e0..ddbf89f 100644
@@ -1051,11 +1051,12 @@ static void drop_spte(struct kvm *kvm, u64 *sptep)
 		rmap_remove(kvm, sptep);
 }
 
-static int __rmap_write_protect(struct kvm *kvm, unsigned long *rmapp, int level)
+static bool
+__rmap_write_protect(struct kvm *kvm, unsigned long *rmapp, int level)
 {
 	u64 *sptep;
 	struct rmap_iterator iter;
-	int write_protected = 0;
+	bool write_protected = false;
 
 	for (sptep = rmap_get_first(*rmapp, &iter); sptep;) {
 		BUG_ON(!(*sptep & PT_PRESENT_MASK));
@@ -1076,7 +1077,7 @@ static int __rmap_write_protect(struct kvm *kvm, unsigned long *rmapp, int level
 			sptep = rmap_get_first(*rmapp, &iter);
 		}
 
-		write_protected = 1;
+		write_protected = true;
 	}
 
 	return write_protected;
@@ -1107,12 +1108,12 @@ void kvm_mmu_write_protect_pt_masked(struct kvm *kvm,
 	}
 }
 
-static int rmap_write_protect(struct kvm *kvm, u64 gfn)
+static bool rmap_write_protect(struct kvm *kvm, u64 gfn)
 {
 	struct kvm_memory_slot *slot;
 	unsigned long *rmapp;
 	int i;
-	int write_protected = 0;
+	bool write_protected = false;
 
 	slot = gfn_to_memslot(kvm, gfn);
 
@@ -1701,7 +1702,7 @@ static void mmu_sync_children(struct kvm_vcpu *vcpu,
 
 	kvm_mmu_pages_init(parent, &parents, &pages);
 	while (mmu_unsync_walk(parent, &pages)) {
-		int protected = 0;
+		bool protected = false;
 
 		for_each_sp(pages, sp, parents, i)
 			protected |= rmap_write_protect(vcpu->kvm, sp->gfn);
Signature · 1 line
1.7.7.6
#2 lore raw

PATCH v5 2/9 KVM: MMU: abstract spte write-protect

In reply to Xiao Guangrong

Introduce a common function to abstract spte write-protect to cleanup the code

---
 arch/x86/kvm/mmu.c |   58 +++++++++++++++++++++++++++------------------------
 1 files changed, 31 insertions(+), 27 deletions(-)
arch/x86/kvm/mmu.c +31 -27
index ddbf89f..337ff0a 100644
@@ -1051,36 +1051,48 @@ static void drop_spte(struct kvm *kvm, u64 *sptep)
 		rmap_remove(kvm, sptep);
 }
 
+/* Return true if the spte is dropped. */
+static bool spte_write_protect(struct kvm *kvm, u64 *sptep, bool *flush)
+{
+	u64 spte = *sptep;
+
+	if (!is_writable_pte(spte))
+		return false;
+
+	rmap_printk("rmap_write_protect: spte %p %llx\n", sptep, *sptep);
+
+	*flush |= true;
+	if (is_large_pte(spte)) {
+		WARN_ON(page_header(__pa(sptep))->role.level ==
+		       PT_PAGE_TABLE_LEVEL);
+		drop_spte(kvm, sptep);
+		--kvm->stat.lpages;
+		return true;
+	}
+
+	spte = spte & ~PT_WRITABLE_MASK;
+	mmu_spte_update(sptep, spte);
+	return false;
+}
+
 static bool
 __rmap_write_protect(struct kvm *kvm, unsigned long *rmapp, int level)
 {
 	u64 *sptep;
 	struct rmap_iterator iter;
-	bool write_protected = false;
+	bool flush = false;
 
 	for (sptep = rmap_get_first(*rmapp, &iter); sptep;) {
 		BUG_ON(!(*sptep & PT_PRESENT_MASK));
-		rmap_printk("rmap_write_protect: spte %p %llx\n", sptep, *sptep);
-
-		if (!is_writable_pte(*sptep)) {
-			sptep = rmap_get_next(&iter);
-			continue;
-		}
-
-		if (level == PT_PAGE_TABLE_LEVEL) {
-			mmu_spte_update(sptep, *sptep & ~PT_WRITABLE_MASK);
-			sptep = rmap_get_next(&iter);
-		} else {
-			BUG_ON(!is_large_pte(*sptep));
-			drop_spte(kvm, sptep);
-			--kvm->stat.lpages;
+		if (spte_write_protect(kvm, sptep, &flush)) {
 			sptep = rmap_get_first(*rmapp, &iter);
+			continue;
 		}
 
-		write_protected = true;
+		sptep = rmap_get_next(&iter);
 	}
 
-	return write_protected;
+	return flush;
 }
 
 /**
@@ -3887,6 +3899,7 @@ int kvm_mmu_setup(struct kvm_vcpu *vcpu)
 void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int slot)
 {
 	struct kvm_mmu_page *sp;
+	bool flush = false;
 
 	list_for_each_entry(sp, &kvm->arch.active_mmu_pages, link) {
 		int i;
@@ -3901,16 +3914,7 @@ void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int slot)
 			      !is_last_spte(pt[i], sp->role.level))
 				continue;
 
-			if (is_large_pte(pt[i])) {
-				drop_spte(kvm, &pt[i]);
-				--kvm->stat.lpages;
-				continue;
-			}
-
-			/* avoid RMW */
-			if (is_writable_pte(pt[i]))
-				mmu_spte_update(&pt[i],
-						pt[i] & ~PT_WRITABLE_MASK);
+			spte_write_protect(kvm, &pt[i], &flush);
 		}
 	}
 	kvm_flush_remote_tlbs(kvm);
Signature · 1 line
1.7.7.6
#3 lore raw

PATCH v5 3/9 KVM: VMX: export PFEC.P bit on ept

In reply to Xiao Guangrong

Export the present bit of page fault error code, the later patch will use it

---
 arch/x86/kvm/vmx.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)
arch/x86/kvm/vmx.c +8 -1
index 32eb588..fe18fa3 100644
@@ -4769,6 +4769,7 @@ static int handle_ept_violation(struct kvm_vcpu *vcpu)
 {
 	unsigned long exit_qualification;
 	gpa_t gpa;
+	u32 error_code;
 	int gla_validity;
 
 	exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
@@ -4793,7 +4794,13 @@ static int handle_ept_violation(struct kvm_vcpu *vcpu)
 
 	gpa = vmcs_read64(GUEST_PHYSICAL_ADDRESS);
 	trace_kvm_page_fault(gpa, exit_qualification);
-	return kvm_mmu_page_fault(vcpu, gpa, exit_qualification & 0x3, NULL, 0);
+
+	/* It is a write fault? */
+	error_code = exit_qualification & (1U << 1);
+	/* ept page table is present? */
+	error_code |= (exit_qualification >> 3) & 0x1;
+
+	return kvm_mmu_page_fault(vcpu, gpa, error_code, NULL, 0);
 }
 
 static u64 ept_rsvd_mask(u64 spte, int level)
Signature · 1 line
1.7.7.6
#4 lore raw

PATCH v5 4/9 KVM: MMU: fold tlb flush judgement into mmu_spte_update

In reply to Xiao Guangrong

mmu_spte_update() is the common function, we can easily audit the path

---
 arch/x86/kvm/mmu.c |   33 ++++++++++++++++++++-------------
 1 files changed, 20 insertions(+), 13 deletions(-)
arch/x86/kvm/mmu.c +20 -13
index 337ff0a..4810992 100644
@@ -478,15 +478,24 @@ static void mmu_spte_set(u64 *sptep, u64 new_spte)
 
 /* Rules for using mmu_spte_update:
  * Update the state bits, it means the mapped pfn is not changged.
+ *
+ * Whenever we overwrite a writable spte with a read-only one we
+ * should 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, the return value indicates this
+ * case.
  */
-static void mmu_spte_update(u64 *sptep, u64 new_spte)
+static bool mmu_spte_update(u64 *sptep, u64 new_spte)
 {
 	u64 mask, old_spte = *sptep;
+	bool ret = false;
 
 	WARN_ON(!is_rmap_spte(new_spte));
 
-	if (!is_shadow_present_pte(old_spte))
-		return mmu_spte_set(sptep, new_spte);
+	if (!is_shadow_present_pte(old_spte)) {
+		mmu_spte_set(sptep, new_spte);
+		return ret;
+	}
 
 	new_spte |= old_spte & shadow_dirty_mask;
 
@@ -499,13 +508,18 @@ static void mmu_spte_update(u64 *sptep, u64 new_spte)
 	else
 		old_spte = __update_clear_spte_slow(sptep, new_spte);
 
+	if (is_writable_pte(old_spte) && !is_writable_pte(new_spte))
+		ret = true;
+
 	if (!shadow_accessed_mask)
-		return;
+		return ret;
 
 	if (spte_is_bit_cleared(old_spte, new_spte, shadow_accessed_mask))
 		kvm_set_pfn_accessed(spte_to_pfn(old_spte));
 	if (spte_is_bit_cleared(old_spte, new_spte, shadow_dirty_mask))
 		kvm_set_pfn_dirty(spte_to_pfn(old_spte));
+
+	return ret;
 }
 
 /*
@@ -2256,7 +2270,7 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 		    gfn_t gfn, pfn_t pfn, bool speculative,
 		    bool can_unsync, bool host_writable)
 {
-	u64 spte, entry = *sptep;
+	u64 spte;
 	int ret = 0;
 
 	if (set_mmio_spte(sptep, gfn, pfn, pte_access))
@@ -2334,14 +2348,7 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 		mark_page_dirty(vcpu->kvm, gfn);
 
 set_pte:
-	mmu_spte_update(sptep, spte);
-	/*
-	 * If we overwrite a writable spte with a read-only one we
-	 * should 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.
-	 */
-	if (is_writable_pte(entry) && !is_writable_pte(*sptep))
+	if (mmu_spte_update(sptep, spte))
 		kvm_flush_remote_tlbs(vcpu->kvm);
 done:
 	return ret;
Signature · 1 line
1.7.7.6
#5 lore raw

PATCH v5 5/9 KVM: MMU: introduce SPTE_MMU_WRITEABLE bit

In reply to Xiao Guangrong

This bit indicates whether the spte can be writable on MMU, that means the corresponding gpte is writable and the corresponding gfn is not protected by shadow page protection

---
 arch/x86/kvm/mmu.c |   43 +++++++++++++++++++++++++++++--------------
 1 files changed, 29 insertions(+), 14 deletions(-)
arch/x86/kvm/mmu.c +29 -14
index 4810992..bd837f3 100644
@@ -145,7 +145,8 @@ module_param(dbg, 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_MMU_WRITEABLE	(1ULL << (PT_FIRST_AVAIL_BITS_SHIFT + 1))
 
 #define SHADOW_PT_INDEX(addr, level) PT64_INDEX(addr, level)
 
@@ -1065,32 +1066,45 @@ static void drop_spte(struct kvm *kvm, u64 *sptep)
 		rmap_remove(kvm, sptep);
 }
 
+static bool spte_can_be_writable(u64 spte)
+{
+	u64 mask = SPTE_HOST_WRITEABLE | SPTE_MMU_WRITEABLE;
+
+	return (spte & mask) == mask;
+}
+
 /* Return true if the spte is dropped. */
-static bool spte_write_protect(struct kvm *kvm, u64 *sptep, bool *flush)
+static bool
+spte_write_protect(struct kvm *kvm, u64 *sptep, bool *flush, bool pt_protect)
 {
 	u64 spte = *sptep;
 
-	if (!is_writable_pte(spte))
+	if (!is_writable_pte(spte) &&
+	      !(pt_protect && spte_can_be_writable(spte)))
 		return false;
 
 	rmap_printk("rmap_write_protect: spte %p %llx\n", sptep, *sptep);
 
-	*flush |= true;
 	if (is_large_pte(spte)) {
 		WARN_ON(page_header(__pa(sptep))->role.level ==
 		       PT_PAGE_TABLE_LEVEL);
+
+		*flush |= true;
 		drop_spte(kvm, sptep);
 		--kvm->stat.lpages;
 		return true;
 	}
 
+	if (pt_protect)
+		spte &= ~SPTE_MMU_WRITEABLE;
 	spte = spte & ~PT_WRITABLE_MASK;
-	mmu_spte_update(sptep, spte);
+
+	*flush = mmu_spte_update(sptep, spte);
 	return false;
 }
 
-static bool
-__rmap_write_protect(struct kvm *kvm, unsigned long *rmapp, int level)
+static bool __rmap_write_protect(struct kvm *kvm, unsigned long *rmapp,
+				 int level, bool pt_protect)
 {
 	u64 *sptep;
 	struct rmap_iterator iter;
@@ -1098,7 +1112,7 @@ __rmap_write_protect(struct kvm *kvm, unsigned long *rmapp, int level)
 
 	for (sptep = rmap_get_first(*rmapp, &iter); sptep;) {
 		BUG_ON(!(*sptep & PT_PRESENT_MASK));
-		if (spte_write_protect(kvm, sptep, &flush)) {
+		if (spte_write_protect(kvm, sptep, &flush, pt_protect)) {
 			sptep = rmap_get_first(*rmapp, &iter);
 			continue;
 		}
@@ -1127,7 +1141,7 @@ void kvm_mmu_write_protect_pt_masked(struct kvm *kvm,
 
 	while (mask) {
 		rmapp = &slot->rmap[gfn_offset + __ffs(mask)];
-		__rmap_write_protect(kvm, rmapp, PT_PAGE_TABLE_LEVEL);
+		__rmap_write_protect(kvm, rmapp, PT_PAGE_TABLE_LEVEL, false);
 
 		/* clear the first set bit */
 		mask &= mask - 1;
@@ -1146,7 +1160,7 @@ static bool rmap_write_protect(struct kvm *kvm, u64 gfn)
 	for (i = PT_PAGE_TABLE_LEVEL;
 	     i < PT_PAGE_TABLE_LEVEL + KVM_NR_PAGE_SIZES; ++i) {
 		rmapp = __gfn_to_rmap(gfn, i, slot);
-		write_protected |= __rmap_write_protect(kvm, rmapp, i);
+		write_protected |= __rmap_write_protect(kvm, rmapp, i, true);
 	}
 
 	return write_protected;
@@ -2284,8 +2298,10 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 		spte |= shadow_x_mask;
 	else
 		spte |= shadow_nx_mask;
+
 	if (pte_access & ACC_USER_MASK)
 		spte |= shadow_user_mask;
+
 	if (level > PT_PAGE_TABLE_LEVEL)
 		spte |= PT_PAGE_SIZE_MASK;
 	if (tdp_enabled)
@@ -2310,7 +2326,7 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 			goto done;
 		}
 
-		spte |= PT_WRITABLE_MASK;
+		spte |= PT_WRITABLE_MASK | SPTE_MMU_WRITEABLE;
 
 		if (!vcpu->arch.mmu.direct_map
 		    && !(pte_access & ACC_WRITE_MASK)) {
@@ -2339,8 +2355,7 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 				 __func__, gfn);
 			ret = 1;
 			pte_access &= ~ACC_WRITE_MASK;
-			if (is_writable_pte(spte))
-				spte &= ~PT_WRITABLE_MASK;
+			spte &= ~(PT_WRITABLE_MASK | SPTE_MMU_WRITEABLE);
 		}
 	}
 
@@ -3921,7 +3936,7 @@ void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int slot)
 			      !is_last_spte(pt[i], sp->role.level))
 				continue;
 
-			spte_write_protect(kvm, &pt[i], &flush);
+			spte_write_protect(kvm, &pt[i], &flush, false);
 		}
 	}
 	kvm_flush_remote_tlbs(kvm);
Signature · 1 line
1.7.7.6
#6 lore raw

PATCH v5 6/9 KVM: MMU: fast path of handling guest page fault

In reply to Xiao Guangrong

If the the present bit of page fault error code is set, it indicates the shadow page is populated on all levels, it means what we do is only modify the access bit which can be done out of mmu-lock

Currently, in order to simplify the code, we only fix the page fault caused by write-protect on the fast path

---
 arch/x86/kvm/mmu.c         |  189 +++++++++++++++++++++++++++++++++++++++-----
 arch/x86/kvm/paging_tmpl.h |    3 +
 2 files changed, 173 insertions(+), 19 deletions(-)
arch/x86/kvm/mmu.c +170 -19
index bd837f3..aded056 100644
@@ -445,8 +445,27 @@ static bool __check_direct_spte_mmio_pf(u64 spte)
 }
 #endif
 
+static bool spte_can_be_writable(u64 spte)
+{
+	u64 mask = SPTE_HOST_WRITEABLE | SPTE_MMU_WRITEABLE;
+
+	return (spte & mask) == mask;
+}
+
+static bool spte_can_lockless_update(u64 spte)
+{
+	return  !is_writable_pte(spte) && spte_can_be_writable(spte);
+}
+
 static bool spte_has_volatile_bits(u64 spte)
 {
+	/*
+	 * Always atomicly update spte if it can be updated
+	 * out of mmu-lock.
+	 */
+	if (spte_can_lockless_update(spte))
+		return true;
+
 	if (!shadow_accessed_mask)
 		return false;
 
@@ -488,7 +507,7 @@ static void mmu_spte_set(u64 *sptep, u64 new_spte)
  */
 static bool mmu_spte_update(u64 *sptep, u64 new_spte)
 {
-	u64 mask, old_spte = *sptep;
+	u64 old_spte = *sptep;
 	bool ret = false;
 
 	WARN_ON(!is_rmap_spte(new_spte));
@@ -498,13 +517,7 @@ static bool mmu_spte_update(u64 *sptep, u64 new_spte)
 		return ret;
 	}
 
-	new_spte |= old_spte & shadow_dirty_mask;
-
-	mask = shadow_accessed_mask;
-	if (is_writable_pte(old_spte))
-		mask |= shadow_dirty_mask;
-
-	if (!spte_has_volatile_bits(old_spte) || (new_spte & mask) == mask)
+	if (!spte_has_volatile_bits(old_spte))
 		__update_clear_spte_fast(sptep, new_spte);
 	else
 		old_spte = __update_clear_spte_slow(sptep, new_spte);
@@ -1066,13 +1079,6 @@ static void drop_spte(struct kvm *kvm, u64 *sptep)
 		rmap_remove(kvm, sptep);
 }
 
-static bool spte_can_be_writable(u64 spte)
-{
-	u64 mask = SPTE_HOST_WRITEABLE | SPTE_MMU_WRITEABLE;
-
-	return (spte & mask) == mask;
-}
-
 /* Return true if the spte is dropped. */
 static bool
 spte_write_protect(struct kvm *kvm, u64 *sptep, bool *flush, bool pt_protect)
@@ -2661,18 +2667,157 @@ exit:
 	return ret;
 }
 
+static bool page_fault_can_be_fast(struct kvm_vcpu *vcpu, gfn_t gfn,
+				   u32 error_code)
+{
+	/*
+	 * #PF can be fast only if the shadow page table is present and it
+	 * is caused by write-protect, that means we just need change the
+	 * W bit of the spte which can be done out of mmu-lock.
+	 */
+	if (!(error_code & PFERR_PRESENT_MASK) ||
+	      !(error_code & PFERR_WRITE_MASK))
+		return false;
+
+	return true;
+}
+
+static bool
+fast_pf_fix_indirect_spte(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
+			  u64 *sptep, u64 spte, gfn_t gfn)
+{
+	pfn_t pfn;
+	bool ret = false;
+
+	/*
+	 * For the indirect spte, it is hard to get a stable gfn since
+	 * we just use a cmpxchg to avoid all the races which is not
+	 * enough to avoid the ABA problem: the host can arbitrarily
+	 * change spte and the mapping from gfn to pfn.
+	 *
+	 * What we do is call gfn_to_pfn_atomic to bind the gfn and the
+	 * pfn because after the call:
+	 * - we have held the refcount of pfn that means the pfn can not
+	 *   be freed and be reused for another gfn.
+	 * - the pfn is writable that means it can not be shared by different
+	 *   gfn.
+	 */
+	pfn = gfn_to_pfn_atomic(vcpu->kvm, gfn);
+
+	/* The host page is swapped out or merged. */
+	if (mmu_invalid_pfn(pfn))
+		goto exit;
+
+	ret = true;
+
+	if (pfn != spte_to_pfn(spte))
+		goto exit;
+
+	if (cmpxchg64(sptep, spte, spte | PT_WRITABLE_MASK) == spte)
+		mark_page_dirty(vcpu->kvm, gfn);
+
+exit:
+	kvm_release_pfn_clean(pfn);
+	return ret;
+}
+
+static bool
+fast_pf_fix_direct_spte(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
+			u64 *sptep, u64 spte)
+{
+	gfn_t gfn;
+
+	WARN_ON(!sp->role.direct);
+
+	/*
+	 * The gfn of direct spte is stable since it is calculated
+	 * by sp->gfn.
+	 */
+	gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->spt);
+
+	if (cmpxchg64(sptep, spte, spte | PT_WRITABLE_MASK) == spte)
+		mark_page_dirty(vcpu->kvm, gfn);
+
+	return true;
+}
+
+/*
+ * Return value:
+ * - true: let the vcpu to access on the same address again.
+ * - false: let the real page fault path to fix it.
+ */
+static bool fast_page_fault(struct kvm_vcpu *vcpu, gva_t gva, gfn_t gfn,
+			    int level, u32 error_code)
+{
+	struct kvm_shadow_walk_iterator iterator;
+	struct kvm_mmu_page *sp;
+	bool ret = false;
+	u64 spte = 0ull;
+
+	if (!page_fault_can_be_fast(vcpu, gfn, error_code))
+		return false;
+
+	walk_shadow_page_lockless_begin(vcpu);
+	for_each_shadow_entry_lockless(vcpu, gva, iterator, spte)
+		if (!is_shadow_present_pte(spte) || iterator.level < level)
+			break;
+
+	/*
+	 * If the mapping has been changed, let the vcpu fault on the
+	 * same address again.
+	 */
+	if (!is_rmap_spte(spte)) {
+		ret = true;
+		goto exit;
+	}
+
+	if (!is_last_spte(spte, level))
+		goto exit;
+
+	/*
+	 * Check if it is a spurious fault caused by TLB lazily flushed.
+	 *
+	 * Need not check the access of upper level table entries since
+	 * they are always ACC_ALL.
+	 */
+	 if (is_writable_pte(spte)) {
+		ret = true;
+		goto exit;
+	}
+
+	/*
+	 * Currently, to simplify the code, only the spte write-protected
+	 * by dirty-log can be fast fixed.
+	 */
+	if (!spte_can_be_writable(spte))
+		goto exit;
+
+	sp = page_header(__pa(iterator.sptep));
+
+	if (sp->role.direct)
+		ret = fast_pf_fix_direct_spte(vcpu, sp, iterator.sptep, spte);
+	else
+		ret = fast_pf_fix_indirect_spte(vcpu, sp, iterator.sptep,
+						spte, gfn);
+
+exit:
+	walk_shadow_page_lockless_end(vcpu);
+
+	return ret;
+}
+
 static bool try_async_pf(struct kvm_vcpu *vcpu, bool prefault, gfn_t gfn,
 			 gva_t gva, pfn_t *pfn, bool write, bool *writable);
 
-static int nonpaging_map(struct kvm_vcpu *vcpu, gva_t v, int write, gfn_t gfn,
-			 bool prefault)
+static int nonpaging_map(struct kvm_vcpu *vcpu, gva_t v, u32 error_code,
+			 gfn_t gfn, bool prefault)
 {
 	int r;
 	int level;
 	int force_pt_level;
 	pfn_t pfn;
 	unsigned long mmu_seq;
-	bool map_writable;
+	bool map_writable, write = error_code & PFERR_WRITE_MASK;
 
 	force_pt_level = mapping_level_dirty_bitmap(vcpu, gfn);
 	if (likely(!force_pt_level)) {
@@ -2689,6 +2834,9 @@ static int nonpaging_map(struct kvm_vcpu *vcpu, gva_t v, int write, gfn_t gfn,
 	} else
 		level = PT_PAGE_TABLE_LEVEL;
 
+	if (fast_page_fault(vcpu, v, gfn, level, error_code))
+		return 0;
+
 	mmu_seq = vcpu->kvm->mmu_notifier_seq;
 	smp_rmb();
 
@@ -3077,7 +3225,7 @@ static int nonpaging_page_fault(struct kvm_vcpu *vcpu, gva_t gva,
 	gfn = gva >> PAGE_SHIFT;
 
 	return nonpaging_map(vcpu, gva & PAGE_MASK,
-			     error_code & PFERR_WRITE_MASK, gfn, prefault);
+			     error_code, gfn, prefault);
 }
 
 static int kvm_arch_setup_async_pf(struct kvm_vcpu *vcpu, gva_t gva, gfn_t gfn)
@@ -3157,6 +3305,9 @@ static int tdp_page_fault(struct kvm_vcpu *vcpu, gva_t gpa, u32 error_code,
 	} else
 		level = PT_PAGE_TABLE_LEVEL;
 
+	if (fast_page_fault(vcpu, gpa, gfn, level, error_code))
+		return 0;
+
 	mmu_seq = vcpu->kvm->mmu_notifier_seq;
 	smp_rmb();
 
arch/x86/kvm/paging_tmpl.h +3 -0
index 34f9709..6127326 100644
@@ -617,6 +617,9 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gva_t addr, u32 error_code,
 		walker.gfn = walker.gfn & ~(KVM_PAGES_PER_HPAGE(level) - 1);
 	}
 
+	if (fast_page_fault(vcpu, addr, walker.gfn, level, error_code))
+		return 0;
+
 	mmu_seq = vcpu->kvm->mmu_notifier_seq;
 	smp_rmb();
 
Signature · 1 line
1.7.7.6
#7 lore raw

Re: [PATCH v5 6/9] KVM: MMU: fast path of handling guest page fault

Quoted 7 lines · Xiao Guangrong

If the the present bit of page fault error code is set, it indicates the shadow page is populated on all levels, it means what we do is only modify the access bit which can be done out of mmu-lock

Currently, in order to simplify the code, we only fix the page fault caused by write-protect on the fast path

arch/x86/kvm/mmu.c static bool __check_direct_spte_mmio_pf(u64 spte) · line 467

Quoted 9 lines
arch/x86/kvm/mmu.c +7 -0
+
 static bool spte_has_volatile_bits(u64 spte)
 {
+	/*
+	 * Always atomicly update spte if it can be updated
+	 * out of mmu-lock.
+	 */
+	if (spte_can_lockless_update(spte))
+		return true;

This is a really subtle point, but is it really needed?

Lockless spte updates should always set the dirty and accessed bits, so we won't be overwriting any volatile bits there.

Quoted 9 lines
arch/x86/kvm/mmu.c +1 -1
+
 	if (!shadow_accessed_mask)
 		return false;
@@ -498,13 +517,7 @@ static bool mmu_spte_update(u64 *sptep, u64 new_spte)
 		return ret;
 	}
 
-	new_spte |= old_spte & shadow_dirty_mask;

arch/x86/kvm/mmu.c static bool mmu_spte_update(u64 *sptep, u64 new_spte) · line 523

Quoted 10 lines
arch/x86/kvm/mmu.c +1 -6
-
-	mask = shadow_accessed_mask;
-	if (is_writable_pte(old_spte))
-		mask |= shadow_dirty_mask;
-
-	if (!spte_has_volatile_bits(old_spte) || (new_spte & mask) == mask)
+	if (!spte_has_volatile_bits(old_spte))
 		__update_clear_spte_fast(sptep, new_spte);
 	else
 		old_spte = __update_clear_spte_slow(sptep, new_spte);

It looks like the old code is bad.. why can we ignore volatile bits in the old spte? Suppose pfn is changing?

Quoted 24 lines
arch/x86/kvm/mmu.c +24 -0
+
+static bool
+fast_pf_fix_indirect_spte(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
+			  u64 *sptep, u64 spte, gfn_t gfn)
+{
+	pfn_t pfn;
+	bool ret = false;
+
+	/*
+	 * For the indirect spte, it is hard to get a stable gfn since
+	 * we just use a cmpxchg to avoid all the races which is not
+	 * enough to avoid the ABA problem: the host can arbitrarily
+	 * change spte and the mapping from gfn to pfn.
+	 *
+	 * What we do is call gfn_to_pfn_atomic to bind the gfn and the
+	 * pfn because after the call:
+	 * - we have held the refcount of pfn that means the pfn can not
+	 *   be freed and be reused for another gfn.
+	 * - the pfn is writable that means it can not be shared by different
+	 *   gfn.
+	 */
+	pfn = gfn_to_pfn_atomic(vcpu->kvm, gfn);
+
+	/* The host page is swapped out or merged. */

arch/x86/kvm/mmu.c exit: · line 2717

Quoted 10 lines
arch/x86/kvm/mmu.c +10 -0
+	if (mmu_invalid_pfn(pfn))
+		goto exit;
+
+	ret = true;
+
+	if (pfn != spte_to_pfn(spte))
+		goto exit;
+
+	if (cmpxchg64(sptep, spte, spte | PT_WRITABLE_MASK) == spte)
+		mark_page_dirty(vcpu->kvm, gfn);

Isn't it better to kvm_release_pfn_dirty() here?

Quoted 13 lines
arch/x86/kvm/mmu.c +13 -0
+
+exit:
+	kvm_release_pfn_clean(pfn);
+	return ret;
+}
+
+> +
+/*
+ * Return value:
+ * - true: let the vcpu to access on the same address again.
+ * - false: let the real page fault path to fix it.
+ */
+static bool fast_page_fault(struct kvm_vcpu *vcpu, gva_t gva, gfn_t gfn,

arch/x86/kvm/mmu.c exit: · line 2759

Quoted 10 lines
arch/x86/kvm/mmu.c +10 -0
+			    int level, u32 error_code)
+{
+	struct kvm_shadow_walk_iterator iterator;
+	struct kvm_mmu_page *sp;
+	bool ret = false;
+	u64 spte = 0ull;
+
+	if (!page_fault_can_be_fast(vcpu, gfn, error_code))
+		return false;
+

No need to pass gfn here.

Quoted 49 lines
arch/x86/kvm/mmu.c +49 -0
+	walk_shadow_page_lockless_begin(vcpu);
+	for_each_shadow_entry_lockless(vcpu, gva, iterator, spte)
+		if (!is_shadow_present_pte(spte) || iterator.level < level)
+			break;
+
+	/*
+	 * If the mapping has been changed, let the vcpu fault on the
+	 * same address again.
+	 */
+	if (!is_rmap_spte(spte)) {
+		ret = true;
+		goto exit;
+	}
+
+	if (!is_last_spte(spte, level))
+		goto exit;
+
+	/*
+	 * Check if it is a spurious fault caused by TLB lazily flushed.
+	 *
+	 * Need not check the access of upper level table entries since
+	 * they are always ACC_ALL.
+	 */
+	 if (is_writable_pte(spte)) {
+		ret = true;
+		goto exit;
+	}
+
+	/*
+	 * Currently, to simplify the code, only the spte write-protected
+	 * by dirty-log can be fast fixed.
+	 */
+	if (!spte_can_be_writable(spte))
+		goto exit;
+
+	sp = page_header(__pa(iterator.sptep));
+
+	if (sp->role.direct)
+		ret = fast_pf_fix_direct_spte(vcpu, sp, iterator.sptep, spte);
+	else
+		ret = fast_pf_fix_indirect_spte(vcpu, sp, iterator.sptep,
+						spte, gfn);
+
+exit:
+	walk_shadow_page_lockless_end(vcpu);
+
+	return ret;
+}
+
Signature · 1 line
error compiling committee.c: too many arguments to function
#8 lore raw

Re: [PATCH v5 6/9] KVM: MMU: fast path of handling guest page fault

Quoted 12 lines · Avi Kivity

arch/x86/kvm/mmu.c static bool __check_direct_spte_mmio_pf(u64 spte) · line 467

Quoted 8 lines

static bool spte_has_volatile_bits(u64 spte)
{

arch/x86/kvm/mmu.c +6 -0
+	/*
+	 * Always atomicly update spte if it can be updated
+	 * out of mmu-lock.
+	 */
+	if (spte_can_lockless_update(spte))
+		return true;

This is a really subtle point, but is it really needed?

Quoted 3 lines

Lockless spte updates should always set the dirty and accessed bits, so we won't be overwriting any volatile bits there.

Avi,

Currently, The spte update/clear paths in mmu-lock think the "Dirty bit" is not volatile if the spte is readonly. Then the "Dirty bit" caused by lockless update can be lost.

And, for tlb flush:

|        * If we overwrite a writable spte with a read-only one we
|        * should 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.
|        */
|       if (is_writable_pte(entry) && !is_writable_pte(*sptep))
|                kvm_flush_remote_tlbs(vcpu->kvm);

Atomically update spte can help us to get a stable is_writable_pte().

Quoted 8 lines
Quoted 8 lines
arch/x86/kvm/mmu.c +1 -0
+
 	if (!shadow_accessed_mask)
 		return false;
@@ -498,13 +517,7 @@ static bool mmu_spte_update(u64 *sptep, u64 new_spte)
 		return ret;
 	}
 
Quoted 16 lines

arch/x86/kvm/mmu.c static bool mmu_spte_update(u64 *sptep, u64 new_spte) · line 523

Quoted 11 lines
arch/x86/kvm/mmu.c +1 -7
-	new_spte |= old_spte & shadow_dirty_mask;
-
-	mask = shadow_accessed_mask;
-	if (is_writable_pte(old_spte))
-		mask |= shadow_dirty_mask;
-
-	if (!spte_has_volatile_bits(old_spte) || (new_spte & mask) == mask)
+	if (!spte_has_volatile_bits(old_spte))
 		__update_clear_spte_fast(sptep, new_spte);
 	else
 		old_spte = __update_clear_spte_slow(sptep, new_spte);

It looks like the old code is bad.. why can we ignore volatile bits in the old spte? Suppose pfn is changing?

/* Rules for using mmu_spte_update:
* Update the state bits, it means the mapped pfn is not changged.

If pfn is changed, we should clear spte first, then set the spte to the new pfn, in kvm_set_pte_rmapp(), we have:

| mmu_spte_clear_track_bits(sptep);
| mmu_spte_set(sptep, new_spte);

Quoted 28 lines
Quoted 28 lines
arch/x86/kvm/mmu.c +28 -0
+
+static bool
+fast_pf_fix_indirect_spte(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
+			  u64 *sptep, u64 spte, gfn_t gfn)
+{
+	pfn_t pfn;
+	bool ret = false;
+
+	/*
+	 * For the indirect spte, it is hard to get a stable gfn since
+	 * we just use a cmpxchg to avoid all the races which is not
+	 * enough to avoid the ABA problem: the host can arbitrarily
+	 * change spte and the mapping from gfn to pfn.
+	 *
+	 * What we do is call gfn_to_pfn_atomic to bind the gfn and the
+	 * pfn because after the call:
+	 * - we have held the refcount of pfn that means the pfn can not
+	 *   be freed and be reused for another gfn.
+	 * - the pfn is writable that means it can not be shared by different
+	 *   gfn.
+	 */
+	pfn = gfn_to_pfn_atomic(vcpu->kvm, gfn);
+
+	/* The host page is swapped out or merged. */
+	if (mmu_invalid_pfn(pfn))
+		goto exit;
+
+	ret = true;
Quoted 9 lines

arch/x86/kvm/mmu.c exit: · line 2717

Quoted 6 lines
arch/x86/kvm/mmu.c +6 -0
+
+	if (pfn != spte_to_pfn(spte))
+		goto exit;
+
+	if (cmpxchg64(sptep, spte, spte | PT_WRITABLE_MASK) == spte)
+		mark_page_dirty(vcpu->kvm, gfn);

Isn't it better to kvm_release_pfn_dirty() here?

Right, kvm_release_pfn_dirty is better.

Quoted 17 lines
Quoted 17 lines
arch/x86/kvm/mmu.c +17 -0
+
+exit:
+	kvm_release_pfn_clean(pfn);
+	return ret;
+}
+
+> +
+/*
+ * Return value:
+ * - true: let the vcpu to access on the same address again.
+ * - false: let the real page fault path to fix it.
+ */
+static bool fast_page_fault(struct kvm_vcpu *vcpu, gva_t gva, gfn_t gfn,
+			    int level, u32 error_code)
+{
+	struct kvm_shadow_walk_iterator iterator;
+	struct kvm_mmu_page *sp;
Quoted 8 lines

arch/x86/kvm/mmu.c exit: · line 2759

Quoted 6 lines
arch/x86/kvm/mmu.c +6 -0
+	bool ret = false;
+	u64 spte = 0ull;
+
+	if (!page_fault_can_be_fast(vcpu, gfn, error_code))
+		return false;
+

No need to pass gfn here.

Right, will fix it.

#9 lore raw

Re: [PATCH v5 6/9] KVM: MMU: fast path of handling guest page fault

Quoted 20 lines · Xiao Guangrong
Quoted 12 lines · Avi Kivity

arch/x86/kvm/mmu.c static bool __check_direct_spte_mmio_pf(u64 spte) · line 467

Quoted 8 lines

static bool spte_has_volatile_bits(u64 spte)
{

arch/x86/kvm/mmu.c +6 -0
+	/*
+	 * Always atomicly update spte if it can be updated
+	 * out of mmu-lock.
+	 */
+	if (spte_can_lockless_update(spte))
+		return true;

This is a really subtle point, but is it really needed?

Quoted 3 lines

Lockless spte updates should always set the dirty and accessed bits, so we won't be overwriting any volatile bits there.

Quoted 6 lines

Avi,

Currently, The spte update/clear paths in mmu-lock think the "Dirty bit" is not volatile if the spte is readonly. Then the "Dirty bit" caused by lockless update can be lost.

Maybe it's better to change that. In fact, changing

	if ((spte & shadow_accessed_mask) &&
	      (!is_writable_pte(spte) || (spte & shadow_dirty_mask)))
		return false;

to

	if (~spte & (shadow_accessed_mask | shadow_dirty_mask))
		return false;

is almost the same thing - we miss the case where the page is COW or shadowed though.

If we release the page as dirty, as below, perhaps the whole thing doesn't matter; the mm must drop spte.w (or spte.d) before it needs to access spte.d again.

Quoted 11 lines

And, for tlb flush:

|        * If we overwrite a writable spte with a read-only one we
|        * should 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.
|        */
|       if (is_writable_pte(entry) && !is_writable_pte(*sptep))
|                kvm_flush_remote_tlbs(vcpu->kvm);

Atomically update spte can help us to get a stable is_writable_pte().

Why is it unstable? mmu_set_spte() before cleared SPTE_MMU_WRITEABLE, so the lockless path will keep its hands off *spte.

Quoted 28 lines
Quoted 10 lines
Quoted 10 lines
arch/x86/kvm/mmu.c +1 -0
+
 	if (!shadow_accessed_mask)
 		return false;
@@ -498,13 +517,7 @@ static bool mmu_spte_update(u64 *sptep, u64 new_spte)
 		return ret;
 	}
 
Quoted 16 lines

arch/x86/kvm/mmu.c static bool mmu_spte_update(u64 *sptep, u64 new_spte) · line 523

Quoted 11 lines
arch/x86/kvm/mmu.c +1 -7
-	new_spte |= old_spte & shadow_dirty_mask;
-
-	mask = shadow_accessed_mask;
-	if (is_writable_pte(old_spte))
-		mask |= shadow_dirty_mask;
-
-	if (!spte_has_volatile_bits(old_spte) || (new_spte & mask) == mask)
+	if (!spte_has_volatile_bits(old_spte))
 		__update_clear_spte_fast(sptep, new_spte);
 	else
 		old_spte = __update_clear_spte_slow(sptep, new_spte);

It looks like the old code is bad.. why can we ignore volatile bits in the old spte? Suppose pfn is changing?

Quoted 8 lines

/* Rules for using mmu_spte_update:
* Update the state bits, it means the mapped pfn is not changged.

If pfn is changed, we should clear spte first, then set the spte to the new pfn, in kvm_set_pte_rmapp(), we have:

| mmu_spte_clear_track_bits(sptep);
| mmu_spte_set(sptep, new_spte);

Okay, thanks.

Signature · 1 line
error compiling committee.c: too many arguments to function
#10 lore raw

Re: [PATCH v5 6/9] KVM: MMU: fast path of handling guest page fault

Quoted 30 lines · Avi Kivity
Quoted 20 lines · Xiao Guangrong
Quoted 12 lines · Avi Kivity

arch/x86/kvm/mmu.c static bool __check_direct_spte_mmio_pf(u64 spte) · line 467

Quoted 8 lines

static bool spte_has_volatile_bits(u64 spte)
{

arch/x86/kvm/mmu.c +6 -0
+	/*
+	 * Always atomicly update spte if it can be updated
+	 * out of mmu-lock.
+	 */
+	if (spte_can_lockless_update(spte))
+		return true;

This is a really subtle point, but is it really needed?

Quoted 3 lines

Lockless spte updates should always set the dirty and accessed bits, so we won't be overwriting any volatile bits there.

Quoted 6 lines

Avi,

Currently, The spte update/clear paths in mmu-lock think the "Dirty bit" is not volatile if the spte is readonly. Then the "Dirty bit" caused by lockless update can be lost.

Maybe it's better to change that. In fact, changing

Quoted 9 lines
	if ((spte & shadow_accessed_mask) &&
	      (!is_writable_pte(spte) || (spte & shadow_dirty_mask)))
		return false;

to

	if (~spte & (shadow_accessed_mask | shadow_dirty_mask))
		return false;

Okay, i like this way.

Quoted 18 lines

is almost the same thing - we miss the case where the page is COW or shadowed though.

If we release the page as dirty, as below, perhaps the whole thing doesn't matter; the mm must drop spte.w (or spte.d) before it needs to access spte.d again.

Quoted 10 lines

And, for tlb flush:

|        * If we overwrite a writable spte with a read-only one we
|        * should 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.
|        */
|       if (is_writable_pte(entry) && !is_writable_pte(*sptep))
|                kvm_flush_remote_tlbs(vcpu->kvm);
Quoted 5 lines
Quoted 1 line

Atomically update spte can help us to get a stable is_writable_pte().

Why is it unstable? mmu_set_spte() before cleared SPTE_MMU_WRITEABLE, so the lockless path will keep its hands off *spte.

Since dirty-log path does not clear SPTE_MMU_WRITEABLE bit, so in the system, we have this kind of spte which is readonly but SPTE_MMU_WRITEABLE is set.

In mmu_set_spte(), we may read a read-only spte (which indicates the TLb need not be flushed), but it can be marked writeable by fast page fault, then the TLB is dirty.

If you do not like the way in this patch, we can change it to:

       if (spte_can_be_wriable(entry) && !is_writable_pte(*sptep))
                kvm_flush_remote_tlbs(vcpu->kvm);

And actually, this kind of TLB flush can be delay until page table protection happen, we can simply mask tlb dirty after your patchset of "flush tlb out of mmu lock"

#11 lore raw

PATCH v5 7/9 KVM: MMU: trace fast page fault

In reply to Xiao Guangrong

To see what happen on this path and help us to optimize it

---
 arch/x86/kvm/mmu.c      |    2 ++
 arch/x86/kvm/mmutrace.h |   41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 0 deletions(-)
arch/x86/kvm/mmu.c +2 -0
index aded056..ab4dfeb 100644
@@ -2801,6 +2801,8 @@ static bool fast_page_fault(struct kvm_vcpu *vcpu, gva_t gva, gfn_t gfn,
 						spte, gfn);
 
 exit:
+	trace_fast_page_fault(vcpu, gva, gfn, error_code, iterator.sptep,
+			      spte, ret);
 	walk_shadow_page_lockless_end(vcpu);
 
 	return ret;
arch/x86/kvm/mmutrace.h +41 -0

47 lines not on this page · raw message

Signature · 1 line
1.7.7.6
#12 lore raw

PATCH v5 8/9 KVM: MMU: fix kvm_mmu_pagetable_walk tracepoint

In reply to Xiao Guangrong

The P bit of page fault error code is missed in this tracepoint, fix it by passing the full error code

---
 arch/x86/kvm/mmutrace.h    |    7 +++----
 arch/x86/kvm/paging_tmpl.h |    3 +--
 2 files changed, 4 insertions(+), 6 deletions(-)
arch/x86/kvm/mmutrace.h +3 -4
index 84da94f..e762d35 100644
@@ -54,8 +54,8 @@
  */
 TRACE_EVENT(
 	kvm_mmu_pagetable_walk,
-	TP_PROTO(u64 addr, int write_fault, int user_fault, int fetch_fault),
-	TP_ARGS(addr, write_fault, user_fault, fetch_fault),
+	TP_PROTO(u64 addr, u32 pferr),
+	TP_ARGS(addr, pferr),
 
 	TP_STRUCT__entry(
 		__field(__u64, addr)
@@ -64,8 +64,7 @@ TRACE_EVENT(
 
 	TP_fast_assign(
 		__entry->addr = addr;
-		__entry->pferr = (!!write_fault << 1) | (!!user_fault << 2)
-		                 | (!!fetch_fault << 4);
+		__entry->pferr = pferr;
 	),
 
 	TP_printk("addr %llx pferr %x %s", __entry->addr, __entry->pferr,
arch/x86/kvm/paging_tmpl.h +1 -2
index 6127326..df546fd 100644
@@ -154,8 +154,7 @@ static int FNAME(walk_addr_generic)(struct guest_walker *walker,
 	const int fetch_fault = access & PFERR_FETCH_MASK;
 	u16 errcode = 0;
 
-	trace_kvm_mmu_pagetable_walk(addr, write_fault, user_fault,
-				     fetch_fault);
+	trace_kvm_mmu_pagetable_walk(addr, access);
 retry_walk:
 	eperm = false;
 	walker->level = mmu->root_level;
Signature · 1 line
1.7.7.6
#13 lore raw
In reply to Xiao Guangrong
Quoted 12 lines · Xiao Guangrong

Changlog:
This are some changes from Marcelo's review:
- drop SPTE_WRITE_PROTECT bit, now, only one bit is needed to do lockless update.

- always atomicly update spte if it can be updated out of mmu-lock.

- flod the judgement of tlb flush into mmu_spte_update(), make the code easilyer audited.

Performance result:
- autoest migration test (smp2.Fedora.16.64.migrate.with_autotest.dbench.unix):

Quoted 8 lines
                                                   before     after
smp2.Fedora.16.64.migrate.unix                        93      91    +2.1%
smp2.Fedora.16.64.migrate.with_autotest.dbench.unix   218     188   +13.7%

- the benchmark attached is used to measure the resuming time after dirty-log
before after
Run 10 times, Avg time: 512466818 ns. 269231261 ns +47.5%

Still scary (esp. indirect sptes), but looks pretty good.

Signature · 1 line
error compiling committee.c: too many arguments to function
#14 lore raw
Quoted 17 lines · Avi Kivity
Quoted 16 lines · Xiao Guangrong

Changlog:
This are some changes from Marcelo's review:
- drop SPTE_WRITE_PROTECT bit, now, only one bit is needed to do lockless update.

- always atomicly update spte if it can be updated out of mmu-lock.

- flod the judgement of tlb flush into mmu_spte_update(), make the code easilyer audited.

Performance result:
- autoest migration test (smp2.Fedora.16.64.migrate.with_autotest.dbench.unix):

                                                   before     after
smp2.Fedora.16.64.migrate.unix                        93      91    +2.1%
smp2.Fedora.16.64.migrate.with_autotest.dbench.unix   218     188   +13.7%
Quoted 6 lines
Quoted 4 lines

- the benchmark attached is used to measure the resuming time after dirty-log
before after
Run 10 times, Avg time: 512466818 ns. 269231261 ns +47.5%

Still scary (esp. indirect sptes), but looks pretty good.

Hmm, i do not have a better way to solve the ABA problem on indirect spte now. :( How about only allow fast page fault to work for direct spte?

#15 lore raw
Quoted 24 lines · Xiao Guangrong
Quoted 22 lines · Avi Kivity
Quoted 20 lines · Xiao Guangrong

Changlog:
This are some changes from Marcelo's review:
- drop SPTE_WRITE_PROTECT bit, now, only one bit is needed to do lockless update.

- always atomicly update spte if it can be updated out of mmu-lock.

- flod the judgement of tlb flush into mmu_spte_update(), make the code easilyer audited.

Performance result:
- autoest migration test (smp2.Fedora.16.64.migrate.with_autotest.dbench.unix):

                                                   before     after
smp2.Fedora.16.64.migrate.unix                        93      91    +2.1%
smp2.Fedora.16.64.migrate.with_autotest.dbench.unix   218     188   +13.7%

- the benchmark attached is used to measure the resuming time after dirty-log
before after
Run 10 times, Avg time: 512466818 ns. 269231261 ns +47.5%

Quoted 6 lines
Quoted 1 line

Still scary (esp. indirect sptes), but looks pretty good.

Hmm, i do not have a better way to solve the ABA problem on indirect spte now. :( How about only allow fast page fault to work for direct spte?

I'll certainly be more comfortable with that, at least to start with.

Signature · 1 line
error compiling committee.c: too many arguments to function
#16 lore raw

PATCH v5 9/9 KVM: MMU: document mmu-lock and fast page fault

In reply to Xiao Guangrong

Document fast page fault and mmu-lock in locking.txt

---
 Documentation/virtual/kvm/locking.txt |  136 ++++++++++++++++++++++++++++++++-
 1 files changed, 135 insertions(+), 1 deletions(-)
Documentation/virtual/kvm/locking.txt +135 -1
index 3b4cd3b..a4a485d 100644
@@ -6,7 +6,135 @@ KVM Lock Overview
 
 (to be written)
 
-2. Reference
+2: Exception
+------------
+
+Fast page fault:
+
+Fast page fault is the fast path which fixes the guest page fault out of
+the mmu-lock on x86. Currently, the page fault can be fast only if the
+shadow page table is present and it is caused by write-protect, that means
+we just need change the W bit of the spte.
+
+What we use to avoid all the race is the SPTE_HOST_WRITEABLE bit,
+SPTE_MMU_WRITEABLE bit on the spte:
+- SPTE_HOST_WRITEABLE means the gfn is writable on host.
+- SPTE_MMU_WRITEABLE means the gfn is writable on mmu. The bit is set when
+  the gfn is writable on guest mmu and it is not write-protected by shadow
+  page write-protection.
+
+On fast page fault path, we will use cmpxchg to atomically set the spte W
+bit if spte.SPTE_HOST_WRITEABLE = 1 and spte.SPTE_WRITE_PROTECT = 1, this
+is safe because whenever changing these bits can be detected by cmpxchg.
+
+But we need carefully check these cases:
+1): The mapping from gfn to pfn
+The mapping from gfn to pfn may be changed since we can only ensure the pfn
+is not changed during cmpxchg. This is a ABA problem, for example, below case
+will happen:
+
+At the beginning:
+gpte = gfn1
+gfn1 is mapped to pfn1 on host
+spte is the shadow page table entry corresponding with gpte and
+spte = pfn1
+
+   VCPU 0                           VCPU0
+on fast page fault path:
+
+   old_spte = *spte;
+                                 pfn1 is swapped out:
+                                    spte = 0;
+
+                                 pfn1 is re-alloced for gfn2.
+
+                                 gpte is changed to point to
+                                 gfn2 by the guest:
+                                    spte = pfn1;
+
+   if (cmpxchg(spte, old_spte, old_spte+W)
+	mark_page_dirty(vcpu->kvm, gfn1)
+             OOPS!!!
+
+We dirty-log for gfn1, that means gfn2 is lost in dirty-bitmap.
+
+For direct sp, we can easily avoid it since the spte of direct sp is fixed
+to gfn. For indirect sp, before we do cmpxchg, we call gfn_to_pfn_atomic()
+to pin gfn to pfn, because after gfn_to_pfn_atomic():
+- We have held the refcount of pfn that means the pfn can not be freed and
+  be reused for another gfn.
+- The pfn is writable that means it can not be shared between different gfns
+  by KSM.
+
+Then, we can ensure the dirty bitmaps is correctly set for a gfn.
+
+2): Dirty bit tracking
+In the origin code, the spte can be fast updated (non-atomically) if the
+spte is read-only and the Accessed bit has already been set since the
+Accessed bit and Dirty bit can not be lost.
+
+But it is not true after fast page fault since the spte can be marked
+writable between reading spte and updating spte. Like below case:
+
+At the beginning:
+spte.W = 0
+spte.Accessed = 1
+
+   VCPU 0                                       VCPU0
+In mmu_spte_clear_track_bits():
+
+   old_spte = *spte;
+
+   /* 'if' condition is satisfied. */
+   if (old_spte.Accssed == 1 &&
+        old_spte.W == 0)
+      spte = 0ull;
+                                         on fast page fault path:
+                                             spte.W = 1
+                                         memory write on the spte:
+                                             spte.Dirty = 1
+
+
+   else
+      old_spte = xchg(spte, 0ull)
+
+
+   if (old_spte.Accssed == 1)
+      kvm_set_pfn_accessed(spte.pfn);
+   if (old_spte.Dirty == 1)
+      kvm_set_pfn_dirty(spte.pfn);
+      OOPS!!!
+
+The Dirty bit is lost in this case.
+
+In order to avoid this kind of issue, we always treat the spte as "volatile"
+if it can be updated out of mmu-lock, see spte_has_volatile_bits(), it means,
+the spte is always atomicly updated in this case.
+
+3): flush tlbs due to spte updated
+If the spte is updated from writable to readonly, we should flush all TLBs,
+otherwise rmap_write_protect will find a read-only spte, even though the
+writable spte might be cached on a CPU's TLB.
+
+As mentioned before, the spte can be updated to writeable out of mmu-lock on
+fast page fault path, in order to easily audit the path, we see if TLBs need
+be flushed caused by this reason in mmu_spte_update() since this is a common
+function to update spte (present -> present).
+
+See mmu_spte_update():
+
+|	if (!spte_has_volatile_bits(old_spte))
+|		__update_clear_spte_fast(sptep, new_spte);
+|	else
+|		old_spte = __update_clear_spte_slow(sptep, new_spte);
+|
+|	if (is_writable_pte(old_spte) && !is_writable_pte(new_spte))
+|		ret = true;
+
+Since the spte is "volatile" if it can be updated out of mmu-lock, we always
+atomicly update the spte, the race caused by fast page fault can be avoided.
+
+3. Reference
 ------------
 
 Name:		kvm_lock
@@ -23,3 +151,9 @@ Arch:		x86
 Protects:	- kvm_arch::{last_tsc_write,last_tsc_nsec,last_tsc_offset}
 		- tsc offset in vmcb
 Comment:	'raw' because updating the tsc offsets must not be preempted.
+
+Name:		kvm->mmu_lock
+Type:		spinlock_t
+Arch:		any
+Protects:	-shadow page/shadow tlb entry
+Comment:	it is a spinlock since it is used in mmu notifier.
Signature · 1 line
1.7.7.6