KVM: MMU: reduce the size of shadow page structure and some cleanup

PATCH Applied Xiao Guangrong 4 participants 24 messages Dec 16, 2011 – Jan 10, 2012

Subsystems (4)

Documentation x86 KVM KVM x86

lore

Share & tools

lore

Xiao Guangrong

author

2011-12-16 10:13 UTC #0 lore raw

cover KVM: MMU: reduce the size of shadow page structure and some cleanup

In this patchset, we observably reduce the size of struct kvm_mmu_page and let the code more simple.

And this patchset have already tested by unittest and RHEL.6.1 setup/boot/reboot/shutdown.

Xiao Guangrong

author

2011-12-16 10:14 UTC #1 lore raw

PATCH 1/8 KVM: MMU: combine unsync and unsync_children

unsync is only used for the page of level == 1 and unsync_children is only used in the upper page, so combine them to reduce the size of shadow page structure

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

---
 Documentation/virtual/kvm/mmu.txt |    5 ++-
 arch/x86/include/asm/kvm_host.h   |   14 +++++++++++-
 arch/x86/kvm/mmu.c                |   39 +++++++++++++++++++++++++-----------
 arch/x86/kvm/mmu_audit.c          |    6 ++--
 arch/x86/kvm/mmutrace.h           |    3 +-
 arch/x86/kvm/paging_tmpl.h        |    2 +-
 6 files changed, 48 insertions(+), 21 deletions(-)

Documentation/virtual/kvm/mmu.txt +3 -2

index 5dc972c..4a5fedd 100644
@@ -205,14 +205,15 @@ Shadow pages contain the following information:
     this page's spt.  Otherwise, parent_ptes points at a data structure
     with a list of parent_ptes.
   unsync:
+    It is used when role.level == 1, only level 1 shadow pages can be unsync.
     If true, then the translations in this page may not match the guest's
     translation.  This is equivalent to the state of the tlb when a pte is
     changed but before the tlb entry is flushed.  Accordingly, unsync ptes
     are synchronized when the guest executes invlpg or flushes its tlb by
     other means.  Valid for leaf pages.
   unsync_children:
-    How many sptes in the page point at pages that are unsync (or have
-    unsynchronized children).
+    It is used when role.level > 1 and indicates how many sptes in the page
+    point at unsync pages or unsynchronized children.
   unsync_child_bitmap:
     A bitmap indicating which sptes in spt point (directly or indirectly) at
     pages that may be unsynchronized.  Used to quickly locate all unsychronized

arch/x86/include/asm/kvm_host.h +12 -2

index 52d6640..c0a89cd 100644
@@ -233,9 +233,19 @@ struct kvm_mmu_page {
 	 * in this shadow page.
 	 */
 	DECLARE_BITMAP(slot_bitmap, KVM_MEM_SLOTS_NUM);
-	bool unsync;
 	int root_count;          /* Currently serving as active root */
-	unsigned int unsync_children;
+
+	/*
+	 * If role.level == 1, unsync indicates whether the sp is
+	 * unsync, otherwise unsync_children indicates the number
+	 * of sptes which point at unsync sp or unsychronized children.
+	 * See sp_is_unsync() / sp_unsync_children_num.
+	 */
+	union {
+		bool unsync;
+		unsigned int unsync_children;
+	};
+
 	unsigned long parent_ptes;	/* Reverse mapping for parent_pte */
 	DECLARE_BITMAP(unsync_child_bitmap, 512);

arch/x86/kvm/mmu.c +27 ~~-12~~

index 2a2a9b4..6913a16 100644
@@ -151,6 +151,21 @@ module_param(dbg, bool, 0644);
 
 #define SHADOW_PT_INDEX(addr, level) PT64_INDEX(addr, level)
 
+static bool sp_is_unsync(struct kvm_mmu_page *sp)
+{
+	return sp->role.level == PT_PAGE_TABLE_LEVEL && sp->unsync;
+}
+
+static unsigned int sp_unsync_children_num(struct kvm_mmu_page *sp)
+{
+	unsigned int num = 0;
+
+	if (sp->role.level != PT_PAGE_TABLE_LEVEL)
+		num = sp->unsync_children;
+
+	return num;
+}
+
 struct pte_list_desc {
 	u64 *sptes[PTE_LIST_EXT];
 	struct pte_list_desc *more;

@@ -1401,7 +1416,7 @@ static int mmu_pages_add(struct kvm_mmu_pages *pvec, struct kvm_mmu_page *sp,
 {
 	int i;
 
-	if (sp->unsync)
+	if (sp_is_unsync(sp))
 		for (i=0; i < pvec->nr; i++)
 			if (pvec->page[i].sp == sp)
 				return 0;

@@ -1426,7 +1441,7 @@ static int __mmu_unsync_walk(struct kvm_mmu_page *sp,
 
 		child = page_header(ent & PT64_BASE_ADDR_MASK);
 
-		if (child->unsync_children) {
+		if (sp_unsync_children_num(child)) {
 			if (mmu_pages_add(pvec, child, i))
 				return -ENOSPC;

@@ -1437,7 +1452,7 @@ static int __mmu_unsync_walk(struct kvm_mmu_page *sp,
 				nr_unsync_leaf += ret;
 			else
 				return ret;
-		} else if (child->unsync) {
+		} else if (sp_is_unsync(child)) {
 			nr_unsync_leaf++;
 			if (mmu_pages_add(pvec, child, i))
 				return -ENOSPC;

@@ -1468,7 +1483,7 @@ static int mmu_unsync_walk(struct kvm_mmu_page *sp,
 
 static void kvm_unlink_unsync_page(struct kvm *kvm, struct kvm_mmu_page *sp)
 {
-	WARN_ON(!sp->unsync);
+	WARN_ON(!sp_is_unsync(sp));
 	trace_kvm_mmu_sync_page(sp);
 	sp->unsync = 0;
 	--kvm->stat.mmu_unsync;

@@ -1546,7 +1561,7 @@ static void kvm_sync_pages(struct kvm_vcpu *vcpu,  gfn_t gfn)
 	bool flush = false;
 
 	for_each_gfn_indirect_valid_sp(vcpu->kvm, s, gfn, node) {
-		if (!s->unsync)
+		if (!sp_is_unsync(s))
 			continue;
 
 		WARN_ON(s->role.level != PT_PAGE_TABLE_LEVEL);

@@ -1699,20 +1714,20 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
 		role.quadrant = quadrant;
 	}
 	for_each_gfn_sp(vcpu->kvm, sp, gfn, node) {
-		if (!need_sync && sp->unsync)
+		if (!need_sync && sp_is_unsync(sp))
 			need_sync = true;
 
 		if (sp->role.word != role.word)
 			continue;
 
-		if (sp->unsync && kvm_sync_page_transient(vcpu, sp))
+		if (sp_is_unsync(sp) && kvm_sync_page_transient(vcpu, sp))
 			break;
 
 		mmu_page_add_parent_pte(vcpu, sp, parent_pte);
-		if (sp->unsync_children) {
+		if (sp_unsync_children_num(sp)) {
 			kvm_make_request(KVM_REQ_MMU_SYNC, vcpu);
 			kvm_mmu_mark_parents_unsync(sp);
-		} else if (sp->unsync)
+		} else if (sp_is_unsync(sp))
 			kvm_mmu_mark_parents_unsync(sp);
 
 		__clear_sp_write_flooding_count(sp);

@@ -1914,7 +1929,7 @@ static int kvm_mmu_prepare_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp,
 	kvm_mmu_unlink_parents(kvm, sp);
 	if (!sp->role.invalid && !sp->role.direct)
 		unaccount_shadowed(kvm, sp->gfn);
-	if (sp->unsync)
+	if (sp_is_unsync(sp))
 		kvm_unlink_unsync_page(kvm, sp);
 	if (!sp->root_count) {
 		/* Count self */

@@ -2163,7 +2178,7 @@ static void kvm_unsync_pages(struct kvm_vcpu *vcpu,  gfn_t gfn)
 	struct hlist_node *node;
 
 	for_each_gfn_indirect_valid_sp(vcpu->kvm, s, gfn, node) {
-		if (s->unsync)
+		if (sp_is_unsync(s))
 			continue;
 		WARN_ON(s->role.level != PT_PAGE_TABLE_LEVEL);
 		__kvm_unsync_page(vcpu, s);

@@ -2184,7 +2199,7 @@ static int mmu_need_write_protect(struct kvm_vcpu *vcpu, gfn_t gfn,
 		if (s->role.level != PT_PAGE_TABLE_LEVEL)
 			return 1;
 
-		if (!need_unsync && !s->unsync) {
+		if (!need_unsync && !sp_is_unsync(s)) {
 			need_unsync = true;
 		}
 	}

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

index fe15dcc..2106910 100644
@@ -102,7 +102,7 @@ static void audit_mappings(struct kvm_vcpu *vcpu, u64 *sptep, int level)
 
 	sp = page_header(__pa(sptep));
 
-	if (sp->unsync) {
+	if (sp_is_unsync(sp)) {
 		if (level != PT_PAGE_TABLE_LEVEL) {
 			audit_printk(vcpu->kvm, "unsync sp: %p "
 				     "level = %d\n", sp, level);

@@ -168,7 +168,7 @@ static void audit_spte_after_sync(struct kvm_vcpu *vcpu, u64 *sptep, int level)
 {
 	struct kvm_mmu_page *sp = page_header(__pa(sptep));
 
-	if (vcpu->kvm->arch.audit_point == AUDIT_POST_SYNC && sp->unsync)
+	if (vcpu->kvm->arch.audit_point == AUDIT_POST_SYNC && sp_is_unsync(sp))
 		audit_printk(vcpu->kvm, "meet unsync sp(%p) after sync "
 			     "root.\n", sp);
 }

@@ -194,7 +194,7 @@ static void audit_write_protection(struct kvm *kvm, struct kvm_mmu_page *sp)
 	unsigned long *rmapp;
 	u64 *spte;
 
-	if (sp->role.direct || sp->unsync || sp->role.invalid)
+	if (sp->role.direct || sp_is_unsync(sp) || sp->role.invalid)
 		return;
 
 	slot = gfn_to_memslot(kvm, sp->gfn);

arch/x86/kvm/mmutrace.h +2 -1

index 89fb0e8..7fe9562 100644
@@ -38,7 +38,8 @@
 			 role.invalid ? " invalid" : "",		\
 			 role.nxe ? "" : "!",				\
 			 __entry->root_count,				\
-			 __entry->unsync ? "unsync" : "sync", 0);	\
+			 role.level == 1 && __entry->unsync ?		\
+			 "unsync" : "sync", 0);				\
 	ret;								\
 		})

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

index 1561028..7dacc80 100644
@@ -691,7 +691,7 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
 			pt_element_t gpte;
 			gpa_t pte_gpa;
 
-			if (!sp->unsync)
+			if (!sp_is_unsync(sp))
 				break;
 
 			pte_gpa = FNAME(get_level1_sp_gpa)(sp);

Signature · 1 line

1.7.7.4

Xiao Guangrong

author

2011-12-16 10:14 UTC #7 lore raw

PATCH 2/8 KVM: MMU: set the dirty bit for the upper shadow page

In reply to Xiao Guangrong

Upper page do not need to be tracked the status bit, it is safe to set the dirty and let cpu to happily prefetch it

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

---
 arch/x86/kvm/mmu.c |   13 +++++--------
 1 files changed, 5 insertions(+), 8 deletions(-)

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

index 6913a16..a2d28aa 100644
@@ -150,6 +150,9 @@ module_param(dbg, bool, 0644);
 #define SPTE_HOST_WRITEABLE (1ULL << PT_FIRST_AVAIL_BITS_SHIFT)
 
 #define SHADOW_PT_INDEX(addr, level) PT64_INDEX(addr, level)
+#define SHADOW_PAGE_TABLE						\
+	(PT_PRESENT_MASK | PT_WRITABLE_MASK | shadow_user_mask |	\
+	 shadow_x_mask | shadow_accessed_mask | shadow_dirty_mask)
 
 static bool sp_is_unsync(struct kvm_mmu_page *sp)
 {

@@ -1808,9 +1811,7 @@ static void link_shadow_page(u64 *sptep, struct kvm_mmu_page *sp)
 {
 	u64 spte;
 
-	spte = __pa(sp->spt)
-		| PT_PRESENT_MASK | PT_ACCESSED_MASK
-		| PT_WRITABLE_MASK | PT_USER_MASK;
+	spte = __pa(sp->spt) | SHADOW_PAGE_TABLE;
 	mmu_spte_set(sptep, spte);
 }

@@ -2497,11 +2498,7 @@ static int __direct_map(struct kvm_vcpu *vcpu, gpa_t v, int write,
 				return -ENOMEM;
 			}
 
-			mmu_spte_set(iterator.sptep,
-				     __pa(sp->spt)
-				     | PT_PRESENT_MASK | PT_WRITABLE_MASK
-				     | shadow_user_mask | shadow_x_mask
-				     | shadow_accessed_mask);
+			link_shadow_page(iterator.sptep, sp);
 		}
 	}
 	return emulate;

Signature · 1 line

1.7.7.4

Xiao Guangrong

author

2011-12-16 10:15 UTC #10 lore raw

PATCH 3/8 KVM: MMU: do not add a nonpresent spte to rmaps of its child

In reply to Xiao Guangrong

Set the spte before adding it to the rmap of its child so that all parent spte are valid when propagate unsync bit from a usnync page / children page

And this feature is needed by the later patch

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

---
 arch/x86/kvm/mmu.c         |   74 +++++++++++++++----------------------------
 arch/x86/kvm/mmutrace.h    |    2 +-
 arch/x86/kvm/paging_tmpl.h |   14 +++-----
 3 files changed, 32 insertions(+), 58 deletions(-)

arch/x86/kvm/mmu.c +26 ~~-48~~

index a2d28aa..89202f4 100644
@@ -1321,12 +1321,14 @@ static unsigned kvm_page_table_hashfn(gfn_t gfn)
 	return gfn & ((1 << KVM_MMU_HASH_SHIFT) - 1);
 }
 
-static void mmu_page_add_parent_pte(struct kvm_vcpu *vcpu,
-				    struct kvm_mmu_page *sp, u64 *parent_pte)
+static void mmu_page_add_set_parent_pte(struct kvm_vcpu *vcpu,
+					struct kvm_mmu_page *sp,
+					u64 *parent_pte)
 {
 	if (!parent_pte)
 		return;
 
+	mmu_spte_set(parent_pte, __pa(sp->spt) | SHADOW_PAGE_TABLE);
 	pte_list_add(vcpu, parent_pte, &sp->parent_ptes);
 }

@@ -1357,7 +1359,7 @@ static struct kvm_mmu_page *kvm_mmu_alloc_page(struct kvm_vcpu *vcpu,
 	list_add(&sp->link, &vcpu->kvm->arch.active_mmu_pages);
 	bitmap_zero(sp->slot_bitmap, KVM_MEM_SLOTS_NUM);
 	sp->parent_ptes = 0;
-	mmu_page_add_parent_pte(vcpu, sp, parent_pte);
+	mmu_page_add_set_parent_pte(vcpu, sp, parent_pte);
 	kvm_mod_used_mmu_pages(vcpu->kvm, +1);
 	return sp;
 }

@@ -1690,13 +1692,10 @@ static void clear_sp_write_flooding_count(u64 *spte)
 	__clear_sp_write_flooding_count(sp);
 }
 
-static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
-					     gfn_t gfn,
-					     gva_t gaddr,
-					     unsigned level,
-					     int direct,
-					     unsigned access,
-					     u64 *parent_pte)
+static struct kvm_mmu_page *
+kvm_mmu_get_set_page(struct kvm_vcpu *vcpu, gfn_t gfn, gva_t gaddr,
+		     unsigned level, int direct, unsigned access,
+		     u64 *parent_pte)
 {
 	union kvm_mmu_page_role role;
 	unsigned quadrant;

@@ -1726,7 +1725,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
 		if (sp_is_unsync(sp) && kvm_sync_page_transient(vcpu, sp))
 			break;
 
-		mmu_page_add_parent_pte(vcpu, sp, parent_pte);
+		mmu_page_add_set_parent_pte(vcpu, sp, parent_pte);
 		if (sp_unsync_children_num(sp)) {
 			kvm_make_request(KVM_REQ_MMU_SYNC, vcpu);
 			kvm_mmu_mark_parents_unsync(sp);

@@ -1734,7 +1733,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
 			kvm_mmu_mark_parents_unsync(sp);
 
 		__clear_sp_write_flooding_count(sp);
-		trace_kvm_mmu_get_page(sp, false);
+		trace_kvm_mmu_get_set_page(sp, false);
 		return sp;
 	}
 	++vcpu->kvm->stat.mmu_cache_miss;

@@ -1754,7 +1753,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
 		account_shadowed(vcpu->kvm, gfn);
 	}
 	init_shadow_page_table(sp);
-	trace_kvm_mmu_get_page(sp, true);
+	trace_kvm_mmu_get_set_page(sp, true);
 	return sp;
 }

@@ -1807,14 +1806,6 @@ static void shadow_walk_next(struct kvm_shadow_walk_iterator *iterator)
 	return __shadow_walk_next(iterator, *iterator->sptep);
 }
 
-static void link_shadow_page(u64 *sptep, struct kvm_mmu_page *sp)
-{
-	u64 spte;
-
-	spte = __pa(sp->spt) | SHADOW_PAGE_TABLE;
-	mmu_spte_set(sptep, spte);
-}
-
 static void drop_large_spte(struct kvm_vcpu *vcpu, u64 *sptep)
 {
 	if (is_large_pte(*sptep)) {

@@ -1879,11 +1870,6 @@ static void kvm_mmu_page_unlink_children(struct kvm *kvm,
 		mmu_page_zap_pte(kvm, sp, sp->spt + i);
 }
 
-static void kvm_mmu_put_page(struct kvm_mmu_page *sp, u64 *parent_pte)
-{
-	mmu_page_remove_parent_pte(sp, parent_pte);
-}
-
 static void kvm_mmu_unlink_parents(struct kvm *kvm, struct kvm_mmu_page *sp)
 {
 	u64 *parent_pte;

@@ -2468,7 +2454,6 @@ static int __direct_map(struct kvm_vcpu *vcpu, gpa_t v, int write,
 			bool prefault)
 {
 	struct kvm_shadow_walk_iterator iterator;
-	struct kvm_mmu_page *sp;
 	int emulate = 0;
 	gfn_t pseudo_gfn;

@@ -2489,16 +2474,9 @@ static int __direct_map(struct kvm_vcpu *vcpu, gpa_t v, int write,
 
 			base_addr &= PT64_LVL_ADDR_MASK(iterator.level);
 			pseudo_gfn = base_addr >> PAGE_SHIFT;
-			sp = kvm_mmu_get_page(vcpu, pseudo_gfn, iterator.addr,
-					      iterator.level - 1,
-					      1, ACC_ALL, iterator.sptep);
-			if (!sp) {
-				pgprintk("nonpaging_map: ENOMEM\n");
-				kvm_release_pfn_clean(pfn);
-				return -ENOMEM;
-			}
-
-			link_shadow_page(iterator.sptep, sp);
+			kvm_mmu_get_set_page(vcpu, pseudo_gfn, iterator.addr,
+					     iterator.level - 1,
+					     1, ACC_ALL, iterator.sptep);
 		}
 	}
 	return emulate;

@@ -2713,8 +2691,8 @@ static int mmu_alloc_direct_roots(struct kvm_vcpu *vcpu)
 	if (vcpu->arch.mmu.shadow_root_level == PT64_ROOT_LEVEL) {
 		spin_lock(&vcpu->kvm->mmu_lock);
 		kvm_mmu_free_some_pages(vcpu);
-		sp = kvm_mmu_get_page(vcpu, 0, 0, PT64_ROOT_LEVEL,
-				      1, ACC_ALL, NULL);
+		sp = kvm_mmu_get_set_page(vcpu, 0, 0, PT64_ROOT_LEVEL,
+					  1, ACC_ALL, NULL);
 		++sp->root_count;
 		spin_unlock(&vcpu->kvm->mmu_lock);
 		vcpu->arch.mmu.root_hpa = __pa(sp->spt);

@@ -2725,10 +2703,10 @@ static int mmu_alloc_direct_roots(struct kvm_vcpu *vcpu)
 			ASSERT(!VALID_PAGE(root));
 			spin_lock(&vcpu->kvm->mmu_lock);
 			kvm_mmu_free_some_pages(vcpu);
-			sp = kvm_mmu_get_page(vcpu, i << (30 - PAGE_SHIFT),
-					      i << 30,
-					      PT32_ROOT_LEVEL, 1, ACC_ALL,
-					      NULL);
+			sp = kvm_mmu_get_set_page(vcpu, i << (30 - PAGE_SHIFT),
+						  i << 30,
+						  PT32_ROOT_LEVEL, 1, ACC_ALL,
+						  NULL);
 			root = __pa(sp->spt);
 			++sp->root_count;
 			spin_unlock(&vcpu->kvm->mmu_lock);

@@ -2764,8 +2742,8 @@ static int mmu_alloc_shadow_roots(struct kvm_vcpu *vcpu)
 
 		spin_lock(&vcpu->kvm->mmu_lock);
 		kvm_mmu_free_some_pages(vcpu);
-		sp = kvm_mmu_get_page(vcpu, root_gfn, 0, PT64_ROOT_LEVEL,
-				      0, ACC_ALL, NULL);
+		sp = kvm_mmu_get_set_page(vcpu, root_gfn, 0, PT64_ROOT_LEVEL,
+					  0, ACC_ALL, NULL);
 		root = __pa(sp->spt);
 		++sp->root_count;
 		spin_unlock(&vcpu->kvm->mmu_lock);

@@ -2798,9 +2776,9 @@ static int mmu_alloc_shadow_roots(struct kvm_vcpu *vcpu)
 		}
 		spin_lock(&vcpu->kvm->mmu_lock);
 		kvm_mmu_free_some_pages(vcpu);
-		sp = kvm_mmu_get_page(vcpu, root_gfn, i << 30,
-				      PT32_ROOT_LEVEL, 0,
-				      ACC_ALL, NULL);
+		sp = kvm_mmu_get_set_page(vcpu, root_gfn, i << 30,
+					  PT32_ROOT_LEVEL, 0,
+					  ACC_ALL, NULL);
 		root = __pa(sp->spt);
 		++sp->root_count;
 		spin_unlock(&vcpu->kvm->mmu_lock);

arch/x86/kvm/mmutrace.h +1 -1

index 7fe9562..f100078 100644
@@ -145,7 +145,7 @@ TRACE_EVENT(
 );
 
 TRACE_EVENT(
-	kvm_mmu_get_page,
+	kvm_mmu_get_set_page,
 	TP_PROTO(struct kvm_mmu_page *sp, bool created),
 	TP_ARGS(sp, created),

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

index 7dacc80..c79c503 100644
@@ -503,8 +503,8 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 		sp = NULL;
 		if (!is_shadow_present_pte(*it.sptep)) {
 			table_gfn = gw->table_gfn[it.level - 2];
-			sp = kvm_mmu_get_page(vcpu, table_gfn, addr, it.level-1,
-					      false, access, it.sptep);
+			sp = kvm_mmu_get_set_page(vcpu, table_gfn, addr,
+					it.level - 1, false, access, it.sptep);
 		}
 
 		/*

@@ -513,9 +513,6 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 		 */
 		if (FNAME(gpte_changed)(vcpu, gw, it.level - 1))
 			goto out_gpte_changed;
-
-		if (sp)
-			link_shadow_page(it.sptep, sp);
 	}
 
 	for (;

@@ -533,9 +530,8 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 
 		direct_gfn = gw->gfn & ~(KVM_PAGES_PER_HPAGE(it.level) - 1);
 
-		sp = kvm_mmu_get_page(vcpu, direct_gfn, addr, it.level-1,
-				      true, direct_access, it.sptep);
-		link_shadow_page(it.sptep, sp);
+		kvm_mmu_get_set_page(vcpu, direct_gfn, addr, it.level - 1,
+				     true, direct_access, it.sptep);
 	}
 
 	clear_sp_write_flooding_count(it.sptep);

@@ -548,7 +544,7 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 
 out_gpte_changed:
 	if (sp)
-		kvm_mmu_put_page(sp, it.sptep);
+		drop_parent_pte(sp, it.sptep);
 	kvm_release_pfn_clean(pfn);
 	return NULL;
 }

Signature · 1 line

1.7.7.4

Xiao Guangrong

author

2011-12-16 10:16 UTC #13 lore raw

PATCH 4/8 KVM: MMU: drop unsync_child_bitmap

In reply to Xiao Guangrong

unsync_child_bitmap is used to record which spte has unsync page or unsync children, we can set a free bit in the spte instead of it

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

---
 Documentation/virtual/kvm/mmu.txt |    4 --
 arch/x86/include/asm/kvm_host.h   |    1 -
 arch/x86/kvm/mmu.c                |   77 +++++++++++++++++++++++++------------
 3 files changed, 52 insertions(+), 30 deletions(-)

Documentation/virtual/kvm/mmu.txt +0 -4

index 4a5fedd..6d70a6e 100644
@@ -214,10 +214,6 @@ Shadow pages contain the following information:
   unsync_children:
     It is used when role.level > 1 and indicates how many sptes in the page
     point at unsync pages or unsynchronized children.
-  unsync_child_bitmap:
-    A bitmap indicating which sptes in spt point (directly or indirectly) at
-    pages that may be unsynchronized.  Used to quickly locate all unsychronized
-    pages reachable from a given page.
 
 Reverse map
 ===========

arch/x86/include/asm/kvm_host.h +0 -1

index c0a89cd..601c7f6 100644
@@ -247,7 +247,6 @@ struct kvm_mmu_page {
 	};
 
 	unsigned long parent_ptes;	/* Reverse mapping for parent_pte */
-	DECLARE_BITMAP(unsync_child_bitmap, 512);
 
 #ifdef CONFIG_X86_32
 	int clear_spte_count;

arch/x86/kvm/mmu.c +52 ~~-25~~

132 lines not on this page · raw message

Signature · 1 line

1.7.7.4

Xiao Guangrong

author

2011-12-16 10:17 UTC #16 lore raw

PATCH 5/8 KVM: MMU: optimize walking unsync shadow page

In reply to Xiao Guangrong

Exactly, unysnc_children is the number of unsync sptes, we can use to avoid unnecessary spte fetching

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

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

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

index 9bd2084..16e0642 100644
@@ -1468,12 +1468,15 @@ static int __mmu_unsync_walk(struct kvm_mmu_page *sp,
 			   struct kvm_mmu_pages *pvec)
 {
 	u64 *spte;
-	int i, ret, nr_unsync_leaf = 0;
+	int i, ret, nr_unsync_leaf = 0, unysnc_children = sp->unsync_children;
 
 	for_each_unsync_children(sp, spte, i) {
 		struct kvm_mmu_page *child;
 		u64 ent = *spte;
 
+		if (!unysnc_children--)
+			break;
+
 		WARN_ON(!is_shadow_present_pte(ent) || is_large_pte(ent));
 
 		child = page_header(ent & PT64_BASE_ADDR_MASK);

Signature · 1 line

1.7.7.4

Xiao Guangrong

author

2011-12-16 10:17 UTC #17 lore raw

PATCH 6/8 KVM: MMU: optimize handing invlpg

In reply to Xiao Guangrong

Use unsync bit to see if the spte is point to unsync child

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

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

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

index c79c503..5333256 100644
@@ -706,7 +706,7 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
 			FNAME(update_pte)(vcpu, sp, sptep, &gpte);
 		}
 
-		if (!is_shadow_present_pte(*sptep) || !sp->unsync_children)
+		if (!mmu_spte_is_unsync_child(sptep))
 			break;
 	}
 	spin_unlock(&vcpu->kvm->mmu_lock);

Signature · 1 line

1.7.7.4

Xiao Guangrong

author

2011-12-16 10:18 UTC #18 lore raw

PATCH 7/8 KVM: MMU: remove the redundant get_written_sptes

In reply to Xiao Guangrong

get_written_sptes is called twice in kvm_mmu_pte_write, one of them can be removed

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

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

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

index 16e0642..5d0f0e3 100644
@@ -3575,7 +3575,7 @@ static u64 mmu_pte_write_fetch_gpte(struct kvm_vcpu *vcpu, gpa_t *gpa,
  * If we're seeing too many writes to a page, it may no longer be a page table,
  * or we may be forking, in which case it is better to unmap the page.
  */
-static bool detect_write_flooding(struct kvm_mmu_page *sp, u64 *spte)
+static bool detect_write_flooding(struct kvm_mmu_page *sp)
 {
 	/*
 	 * Skip write-flooding detected for the sp whose level is 1, because

@@ -3684,10 +3684,8 @@ void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 
 	mask.cr0_wp = mask.cr4_pae = mask.nxe = 1;
 	for_each_gfn_indirect_valid_sp(vcpu->kvm, sp, gfn, node) {
-		spte = get_written_sptes(sp, gpa, &npte);
-
 		if (detect_write_misaligned(sp, gpa, bytes) ||
-		      detect_write_flooding(sp, spte)) {
+		      detect_write_flooding(sp)) {
 			zap_page |= !!kvm_mmu_prepare_zap_page(vcpu->kvm, sp,
 						     &invalid_list);
 			++vcpu->kvm->stat.mmu_flooded;

Signature · 1 line

1.7.7.4

Xiao Guangrong

author

2011-12-16 10:18 UTC #20 lore raw

PATCH 8/8 KVM: MMU: remove PT64_SECOND_AVAIL_BITS_SHIFT

In reply to Xiao Guangrong

It is not used, remove it

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

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

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

index 5d0f0e3..234a32e 100644
@@ -91,7 +91,6 @@ module_param(dbg, bool, 0644);
 #define PTE_PREFETCH_NUM		8
 
 #define PT_FIRST_AVAIL_BITS_SHIFT 9
-#define PT64_SECOND_AVAIL_BITS_SHIFT 52
 
 #define PT64_LEVEL_BITS 9

Signature · 1 line

1.7.7.4

Avi Kivity

2011-12-18 08:59 UTC #14 lore raw

Re: [PATCH 4/8] KVM: MMU: drop unsync_child_bitmap

In reply to Xiao Guangrong

Quoted 3 lines · Xiao Guangrong

unsync_child_bitmap is used to record which spte has unsync page or unsync children, we can set a free bit in the spte instead of it

unsync_child_bitmap takes one cacheline; the shadow page table takes
64. This will make unsync/resync much more expensive.

I suggest to put unsync_child_bitmap at the end (together with unsync_children) and simply not allocate it when unneeded (have two kmem_caches for the two cases).

Signature · 1 line

error compiling committee.c: too many arguments to function

Avi Kivity

2011-12-18 10:42 UTC #21 lore raw

Re: [PATCH 8/8] KVM: MMU: remove PT64_SECOND_AVAIL_BITS_SHIFT

In reply to Xiao Guangrong

Quoted 12 lines · Xiao Guangrong

It is not used, remove it
Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/mmu.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)
arch/x86/kvm/mmu.c +0 -0
index 5d0f0e3..234a32e 100644

arch/x86/kvm/mmu.c module_param(dbg, bool, 0644); · line 95

Quoted 6 lines

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

@@ -91,7 +91,6 @@ module_param(dbg, bool, 0644);
 #define PTE_PREFETCH_NUM		8
 
 #define PT_FIRST_AVAIL_BITS_SHIFT 9
-#define PT64_SECOND_AVAIL_BITS_SHIFT 52

Don't see a reason to drop it, we may use it some day.

Signature · 1 line

error compiling committee.c: too many arguments to function

Takuya Yoshikawa

2011-12-19 02:24 UTC #2 lore raw

Re: [PATCH 1/8] KVM: MMU: combine unsync and unsync_children

In reply to Xiao Guangrong

About naming issues in the kvm mmu code.

Not restricted to your patch series, so please take as a suggestion for the future.

arch/x86/kvm/mmu.c module_param(dbg, bool, 0644); · line 157

Quoted 4 lines · Xiao Guangrong

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

+static bool sp_is_unsync(struct kvm_mmu_page *sp)
+{
+	return sp->role.level == PT_PAGE_TABLE_LEVEL&&  sp->unsync;
+}

is_unsync_sp() is more consistent with others?

	e.g. is_large_pte(), is_writable_pte(), is_last_spte()


	Takuya

Quoted 11 lines

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

+
+static unsigned int sp_unsync_children_num(struct kvm_mmu_page *sp)
+{
+	unsigned int num = 0;
+
+	if (sp->role.level != PT_PAGE_TABLE_LEVEL)
+		num = sp->unsync_children;
+
+	return num;
+}
+

Takuya Yoshikawa

2011-12-19 02:38 UTC #11 lore raw

Re: [PATCH 3/8] KVM: MMU: do not add a nonpresent spte to rmaps of its child

In reply to Xiao Guangrong

arch/x86/kvm/mmu.c static unsigned kvm_page_table_hashfn(gfn_t gfn) · line 1333

Quoted 12 lines · Xiao Guangrong

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

-static void mmu_page_add_parent_pte(struct kvm_vcpu *vcpu,
-				    struct kvm_mmu_page *sp, u64 *parent_pte)
+static void mmu_page_add_set_parent_pte(struct kvm_vcpu *vcpu,
+					struct kvm_mmu_page *sp,
+					u64 *parent_pte)
  {
  	if (!parent_pte)
  		return;
 
+	mmu_spte_set(parent_pte, __pa(sp->spt) | SHADOW_PAGE_TABLE);
  	pte_list_add(vcpu, parent_pte,&sp->parent_ptes);
  }

There are a few prefixes in the kvm mmu code.

	e.g. mmu_page_, kvm_mmu_, kvm_mmu_page_, ...

Sometimes we also use "sp".

How about deciding a consistent way from now on?

E.g.
if the function is static and for local use only, such a prefix can be eliminated,

if it is used outside of mmu.c, kvm_mmu_ is needed,

we use sp for kvm_mmu_page,
...

(just an example)

Takuya

Avi Kivity

2011-12-19 08:32 UTC #12 lore raw

Re: [PATCH 3/8] KVM: MMU: do not add a nonpresent spte to rmaps of its child

Quoted 17 lines · Takuya Yoshikawa

arch/x86/kvm/mmu.c static unsigned kvm_page_table_hashfn(gfn_t gfn) · line 1333

Quoted 12 lines · Xiao Guangrong

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

-static void mmu_page_add_parent_pte(struct kvm_vcpu *vcpu,
-                    struct kvm_mmu_page *sp, u64 *parent_pte)
+static void mmu_page_add_set_parent_pte(struct kvm_vcpu *vcpu,
+                    struct kvm_mmu_page *sp,
+                    u64 *parent_pte)
  {
      if (!parent_pte)
          return;
 
+    mmu_spte_set(parent_pte, __pa(sp->spt) | SHADOW_PAGE_TABLE);
      pte_list_add(vcpu, parent_pte,&sp->parent_ptes);
  }

There are a few prefixes in the kvm mmu code.

Quoted 6 lines

    e.g. mmu_page_, kvm_mmu_, kvm_mmu_page_, ...
Sometimes we also use "sp".
How about deciding a consistent way from now on?

In general I am in favour of consistency, but I'm not inviting mass-renaming patches. Let's try to be consistent in new patches (like your suggestions on this patchset).

Quoted 8 lines

E.g.
if the function is static and for local use only, such a prefix can be eliminated,
if it is used outside of mmu.c, kvm_mmu_ is needed,
we use sp for kvm_mmu_page,
...

Signature · 1 line

error compiling committee.c: too many arguments to function

Marcelo Tosatti

2011-12-22 16:06 UTC #3 lore raw

Re: [PATCH 1/8] KVM: MMU: combine unsync and unsync_children

In reply to Xiao Guangrong

Quoted 74 lines · Xiao Guangrong

unsync is only used for the page of level == 1 and unsync_children is only used in the upper page, so combine them to reduce the size of shadow page structure

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

---
 Documentation/virtual/kvm/mmu.txt |    5 ++-
 arch/x86/include/asm/kvm_host.h   |   14 +++++++++++-
 arch/x86/kvm/mmu.c                |   39 +++++++++++++++++++++++++-----------
 arch/x86/kvm/mmu_audit.c          |    6 ++--
 arch/x86/kvm/mmutrace.h           |    3 +-
 arch/x86/kvm/paging_tmpl.h        |    2 +-
 6 files changed, 48 insertions(+), 21 deletions(-)

Documentation/virtual/kvm/mmu.txt +3 -2

index 5dc972c..4a5fedd 100644
@@ -205,14 +205,15 @@ Shadow pages contain the following information:
     this page's spt.  Otherwise, parent_ptes points at a data structure
     with a list of parent_ptes.
   unsync:
+    It is used when role.level == 1, only level 1 shadow pages can be unsync.
     If true, then the translations in this page may not match the guest's
     translation.  This is equivalent to the state of the tlb when a pte is
     changed but before the tlb entry is flushed.  Accordingly, unsync ptes
     are synchronized when the guest executes invlpg or flushes its tlb by
     other means.  Valid for leaf pages.
   unsync_children:
-    How many sptes in the page point at pages that are unsync (or have
-    unsynchronized children).
+    It is used when role.level > 1 and indicates how many sptes in the page
+    point at unsync pages or unsynchronized children.
   unsync_child_bitmap:
     A bitmap indicating which sptes in spt point (directly or indirectly) at
     pages that may be unsynchronized.  Used to quickly locate all unsychronized

arch/x86/include/asm/kvm_host.h +12 -2

index 52d6640..c0a89cd 100644
@@ -233,9 +233,19 @@ struct kvm_mmu_page {
 	 * in this shadow page.
 	 */
 	DECLARE_BITMAP(slot_bitmap, KVM_MEM_SLOTS_NUM);
-	bool unsync;
 	int root_count;          /* Currently serving as active root */
-	unsigned int unsync_children;
+
+	/*
+	 * If role.level == 1, unsync indicates whether the sp is
+	 * unsync, otherwise unsync_children indicates the number
+	 * of sptes which point at unsync sp or unsychronized children.
+	 * See sp_is_unsync() / sp_unsync_children_num.
+	 */
+	union {
+		bool unsync;
+		unsigned int unsync_children;
+	};
+
 	unsigned long parent_ptes;	/* Reverse mapping for parent_pte */
 	DECLARE_BITMAP(unsync_child_bitmap, 512);

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

index 2a2a9b4..6913a16 100644
@@ -151,6 +151,21 @@ module_param(dbg, bool, 0644);
 
 #define SHADOW_PT_INDEX(addr, level) PT64_INDEX(addr, level)
 
+static bool sp_is_unsync(struct kvm_mmu_page *sp)
+{
+	return sp->role.level == PT_PAGE_TABLE_LEVEL && sp->unsync;
+}

arch/x86/kvm/mmu.c module_param(dbg, bool, 0644); · line 167

Quoted 10 lines

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

+
+static unsigned int sp_unsync_children_num(struct kvm_mmu_page *sp)
+{
+	unsigned int num = 0;
+
+	if (sp->role.level != PT_PAGE_TABLE_LEVEL)
+		num = sp->unsync_children;
+
+	return num;
+}

IIRC there are windows where unsync_children is not accurate. Have you verified this function is not called inside one of those windows?

Xiao Guangrong

author

2011-12-23 04:04 UTC #15 lore raw

Re: [PATCH 4/8] KVM: MMU: drop unsync_child_bitmap

In reply to Avi Kivity

Quoted 8 lines · Avi Kivity

Quoted 3 lines · Xiao Guangrong
unsync_child_bitmap is used to record which spte has unsync page or unsync children, we can set a free bit in the spte instead of it
unsync_child_bitmap takes one cacheline; the shadow page table takes
64. This will make unsync/resync much more expensive.

Indeed, need more thing about this patch...

Quoted 5 lines

I suggest to put unsync_child_bitmap at the end (together with unsync_children) and simply not allocate it when unneeded (have two kmem_caches for the two cases).

Good point, thanks Avi.

Xiao Guangrong

author

2011-12-23 04:07 UTC #22 lore raw

Re: [PATCH 8/8] KVM: MMU: remove PT64_SECOND_AVAIL_BITS_SHIFT

In reply to Avi Kivity

Quoted 15 lines · Avi Kivity

Quoted 14 lines · Xiao Guangrong
It is not used, remove it
Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/mmu.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)
arch/x86/kvm/mmu.c +0 -0
index 5d0f0e3..234a32e 100644
@@ -91,7 +91,6 @@ module_param(dbg, bool, 0644);
 #define PTE_PREFETCH_NUM		8
 

Quoted 7 lines

arch/x86/kvm/mmu.c module_param(dbg, bool, 0644); · line 96
Quoted 4 lines
arch/x86/kvm/mmu.c +0 -1
 #define PT_FIRST_AVAIL_BITS_SHIFT 9
-#define PT64_SECOND_AVAIL_BITS_SHIFT 52
 
 
Don't see a reason to drop it, we may use it some day.

Anyway, it is not used in currently code, I do not have strong opinion on it :) Please ignore it if you do not like.

Xiao Guangrong

author

2011-12-23 04:11 UTC #4 lore raw

Re: [PATCH 1/8] KVM: MMU: combine unsync and unsync_children

In reply to Marcelo Tosatti

Quoted 80 lines · Marcelo Tosatti

Quoted 79 lines · Xiao Guangrong

unsync is only used for the page of level == 1 and unsync_children is only used in the upper page, so combine them to reduce the size of shadow page structure

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

---
 Documentation/virtual/kvm/mmu.txt |    5 ++-
 arch/x86/include/asm/kvm_host.h   |   14 +++++++++++-
 arch/x86/kvm/mmu.c                |   39 +++++++++++++++++++++++++-----------
 arch/x86/kvm/mmu_audit.c          |    6 ++--
 arch/x86/kvm/mmutrace.h           |    3 +-
 arch/x86/kvm/paging_tmpl.h        |    2 +-
 6 files changed, 48 insertions(+), 21 deletions(-)

Documentation/virtual/kvm/mmu.txt +3 -2

index 5dc972c..4a5fedd 100644
@@ -205,14 +205,15 @@ Shadow pages contain the following information:
     this page's spt.  Otherwise, parent_ptes points at a data structure
     with a list of parent_ptes.
   unsync:
+    It is used when role.level == 1, only level 1 shadow pages can be unsync.
     If true, then the translations in this page may not match the guest's
     translation.  This is equivalent to the state of the tlb when a pte is
     changed but before the tlb entry is flushed.  Accordingly, unsync ptes
     are synchronized when the guest executes invlpg or flushes its tlb by
     other means.  Valid for leaf pages.
   unsync_children:
-    How many sptes in the page point at pages that are unsync (or have
-    unsynchronized children).
+    It is used when role.level > 1 and indicates how many sptes in the page
+    point at unsync pages or unsynchronized children.
   unsync_child_bitmap:
     A bitmap indicating which sptes in spt point (directly or indirectly) at
     pages that may be unsynchronized.  Used to quickly locate all unsychronized

arch/x86/include/asm/kvm_host.h +12 -2

index 52d6640..c0a89cd 100644
@@ -233,9 +233,19 @@ struct kvm_mmu_page {
 	 * in this shadow page.
 	 */
 	DECLARE_BITMAP(slot_bitmap, KVM_MEM_SLOTS_NUM);
-	bool unsync;
 	int root_count;          /* Currently serving as active root */
-	unsigned int unsync_children;
+
+	/*
+	 * If role.level == 1, unsync indicates whether the sp is
+	 * unsync, otherwise unsync_children indicates the number
+	 * of sptes which point at unsync sp or unsychronized children.
+	 * See sp_is_unsync() / sp_unsync_children_num.
+	 */
+	union {
+		bool unsync;
+		unsigned int unsync_children;
+	};
+
 	unsigned long parent_ptes;	/* Reverse mapping for parent_pte */
 	DECLARE_BITMAP(unsync_child_bitmap, 512);

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

index 2a2a9b4..6913a16 100644
@@ -151,6 +151,21 @@ module_param(dbg, bool, 0644);
 
 #define SHADOW_PT_INDEX(addr, level) PT64_INDEX(addr, level)
 
+static bool sp_is_unsync(struct kvm_mmu_page *sp)
+{
+	return sp->role.level == PT_PAGE_TABLE_LEVEL && sp->unsync;
+}
+
+static unsigned int sp_unsync_children_num(struct kvm_mmu_page *sp)
+{
+	unsigned int num = 0;
+

Quoted 9 lines

arch/x86/kvm/mmu.c module_param(dbg, bool, 0644); · line 167
Quoted 5 lines
arch/x86/kvm/mmu.c +5 -0
+	if (sp->role.level != PT_PAGE_TABLE_LEVEL)
+		num = sp->unsync_children;
+
+	return num;
+}
IIRC there are windows where unsync_children is not accurate. Have you verified this function is not called inside one of those windows?

Actually, this function is only used to see whether the sp has unsync children in current code.

And this patch did not change the logic, it just use sp_unsync_children_num instead of using sp->unsync_children directly.

Xiao Guangrong

author

2012-01-09 05:04 UTC #23 lore raw

In reply to Xiao Guangrong

Quoted 5 lines · Xiao Guangrong

In this patchset, we observably reduce the size of struct kvm_mmu_page and let the code more simple.
And this patchset have already tested by unittest and RHEL.6.1 setup/boot/reboot/shutdown.

Hi Avi, Marcelo,

Could you please apply patch 1, 2, 7 if they are good for you? :)

Marcelo Tosatti

2012-01-09 11:16 UTC #5 lore raw

Re: [PATCH 1/8] KVM: MMU: combine unsync and unsync_children

In reply to Xiao Guangrong

Quoted 74 lines · Xiao Guangrong

unsync is only used for the page of level == 1 and unsync_children is only used in the upper page, so combine them to reduce the size of shadow page structure

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

---
 Documentation/virtual/kvm/mmu.txt |    5 ++-
 arch/x86/include/asm/kvm_host.h   |   14 +++++++++++-
 arch/x86/kvm/mmu.c                |   39 +++++++++++++++++++++++++-----------
 arch/x86/kvm/mmu_audit.c          |    6 ++--
 arch/x86/kvm/mmutrace.h           |    3 +-
 arch/x86/kvm/paging_tmpl.h        |    2 +-
 6 files changed, 48 insertions(+), 21 deletions(-)

Documentation/virtual/kvm/mmu.txt +3 -2

index 5dc972c..4a5fedd 100644
@@ -205,14 +205,15 @@ Shadow pages contain the following information:
     this page's spt.  Otherwise, parent_ptes points at a data structure
     with a list of parent_ptes.
   unsync:
+    It is used when role.level == 1, only level 1 shadow pages can be unsync.
     If true, then the translations in this page may not match the guest's
     translation.  This is equivalent to the state of the tlb when a pte is
     changed but before the tlb entry is flushed.  Accordingly, unsync ptes
     are synchronized when the guest executes invlpg or flushes its tlb by
     other means.  Valid for leaf pages.
   unsync_children:
-    How many sptes in the page point at pages that are unsync (or have
-    unsynchronized children).
+    It is used when role.level > 1 and indicates how many sptes in the page
+    point at unsync pages or unsynchronized children.
   unsync_child_bitmap:
     A bitmap indicating which sptes in spt point (directly or indirectly) at
     pages that may be unsynchronized.  Used to quickly locate all unsychronized

arch/x86/include/asm/kvm_host.h +12 -2

index 52d6640..c0a89cd 100644
@@ -233,9 +233,19 @@ struct kvm_mmu_page {
 	 * in this shadow page.
 	 */
 	DECLARE_BITMAP(slot_bitmap, KVM_MEM_SLOTS_NUM);
-	bool unsync;
 	int root_count;          /* Currently serving as active root */
-	unsigned int unsync_children;
+
+	/*
+	 * If role.level == 1, unsync indicates whether the sp is
+	 * unsync, otherwise unsync_children indicates the number
+	 * of sptes which point at unsync sp or unsychronized children.
+	 * See sp_is_unsync() / sp_unsync_children_num.
+	 */
+	union {
+		bool unsync;
+		unsigned int unsync_children;
+	};
+
 	unsigned long parent_ptes;	/* Reverse mapping for parent_pte */
 	DECLARE_BITMAP(unsync_child_bitmap, 512);

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

index 2a2a9b4..6913a16 100644
@@ -151,6 +151,21 @@ module_param(dbg, bool, 0644);
 
 #define SHADOW_PT_INDEX(addr, level) PT64_INDEX(addr, level)
 
+static bool sp_is_unsync(struct kvm_mmu_page *sp)
+{
+	return sp->role.level == PT_PAGE_TABLE_LEVEL && sp->unsync;
+}

arch/x86/kvm/mmu.c module_param(dbg, bool, 0644); · line 167

Quoted 10 lines

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

+
+static unsigned int sp_unsync_children_num(struct kvm_mmu_page *sp)
+{
+	unsigned int num = 0;
+
+	if (sp->role.level != PT_PAGE_TABLE_LEVEL)
+		num = sp->unsync_children;
+
+	return num;
+}

This adds significant complexity/trickery to the code for little gain. For example, in kvm_mmu_page_get, you use sp_is_unsync() which depends on sp->role.level, but that is not set yet.

In general, given the number of optimizations made to the shadow mmu code, and the fact that over time NPT will dominate, i'd suggest time should be spent improving other areas.

Marcelo Tosatti

2012-01-09 11:30 UTC #8 lore raw

Re: [PATCH 2/8] KVM: MMU: set the dirty bit for the upper shadow page

In reply to Xiao Guangrong

Quoted 7 lines · Xiao Guangrong

Upper page do not need to be tracked the status bit, it is safe to set the dirty and let cpu to happily prefetch it
Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/mmu.c |   13 +++++--------
 1 files changed, 5 insertions(+), 8 deletions(-)

Dirty bit is ignored in non-leaf pagetable entries.

Marcelo Tosatti

Applied

2012-01-09 11:33 UTC #19 lore raw

Re: [PATCH 7/8] KVM: MMU: remove the redundant get_written_sptes

In reply to Xiao Guangrong

Quoted 7 lines · Xiao Guangrong

get_written_sptes is called twice in kvm_mmu_pte_write, one of them can be removed
Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/mmu.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

Applied, thanks.

Xiao Guangrong

author

2012-01-10 04:45 UTC #6 lore raw

Re: [PATCH 1/8] KVM: MMU: combine unsync and unsync_children

In reply to Marcelo Tosatti

Quoted 4 lines · Marcelo Tosatti

In general, given the number of optimizations made to the shadow mmu code, and the fact that over time NPT will dominate, i'd suggest time should be spent improving other areas.

I totally agree with you, thanks Marcelo!

Xiao Guangrong

author

2012-01-10 04:46 UTC #9 lore raw

Re: [PATCH 2/8] KVM: MMU: set the dirty bit for the upper shadow page

In reply to Marcelo Tosatti

Quoted 11 lines · Marcelo Tosatti

Quoted 7 lines · Xiao Guangrong
Upper page do not need to be tracked the status bit, it is safe to set the dirty and let cpu to happily prefetch it
Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/mmu.c |   13 +++++--------
 1 files changed, 5 insertions(+), 8 deletions(-)
Dirty bit is ignored in non-leaf pagetable entries.

Sorry, i forgot it!