Skip to contentSkip to conversation

oos shadow optimizations

PATCH In review Marcelo Tosatti 2 participants 20 messages Oct 25, 2008 – Nov 2, 2008

Subsystems (3)
lore
Share & tools
lore
#1 lore raw

PATCH 1/3 KVM: MMU: collapse remote TLB flushes on root sync

Instead of flushing remote TLB's at every page resync, do an initial pass to write protect the sptes, collapsing the flushes on a single remote TLB invalidation.

kernbench is 2.3% faster on 4-way guest. Improvements have been seen with other loads such as AIM7.

Avi: feel free to change this if you dislike the style (I do, but can't think of anything nicer).

arch/x86/kvm/mmu.c +38 -6
Index: kvm/arch/x86/kvm/mmu.c
===================================================================
@@ -152,6 +152,7 @@ struct kvm_shadow_walk {
 
 struct kvm_unsync_walk {
 	int (*entry) (struct kvm_mmu_page *sp, struct kvm_unsync_walk *walk);
+	bool clear_unsync;
 };
 
 typedef int (*mmu_parent_walk_fn) (struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp);
@@ -615,7 +616,7 @@ static u64 *rmap_next(struct kvm *kvm, u
 	return NULL;
 }
 
-static void rmap_write_protect(struct kvm *kvm, u64 gfn)
+static int __rmap_write_protect(struct kvm *kvm, u64 gfn)
 {
 	unsigned long *rmapp;
 	u64 *spte;
@@ -661,7 +662,12 @@ static void rmap_write_protect(struct kv
 		spte = rmap_next(kvm, rmapp, spte);
 	}
 
-	if (write_protected)
+	return write_protected;
+}
+
+static void rmap_write_protect(struct kvm *kvm, u64 gfn)
+{
+	if (__rmap_write_protect(kvm, gfn))
 		kvm_flush_remote_tlbs(kvm);
 }
 
@@ -985,12 +991,14 @@ static int mmu_unsync_walk(struct kvm_mm
 				ret = mmu_unsync_walk(child, walker);
 				if (ret)
 					return ret;
-				__clear_bit(i, sp->unsync_child_bitmap);
+				if (walker->clear_unsync)
+					__clear_bit(i, sp->unsync_child_bitmap);
 			}
 
 			if (child->unsync) {
 				ret = walker->entry(child, walker);
-				__clear_bit(i, sp->unsync_child_bitmap);
+				if (walker->clear_unsync)
+					__clear_bit(i, sp->unsync_child_bitmap);
 				if (ret)
 					return ret;
 			}
@@ -1053,6 +1061,7 @@ static int kvm_sync_page(struct kvm_vcpu
 struct sync_walker {
 	struct kvm_vcpu *vcpu;
 	struct kvm_unsync_walk walker;
+	bool write_protected;
 };
 
 static int mmu_sync_fn(struct kvm_mmu_page *sp, struct kvm_unsync_walk *walk)
@@ -1065,13 +1074,35 @@ static int mmu_sync_fn(struct kvm_mmu_pa
 	return (need_resched() || spin_needbreak(&vcpu->kvm->mmu_lock));
 }
 
+static int mmu_wprotect_fn(struct kvm_mmu_page *sp,
+			   struct kvm_unsync_walk *walk)
+{
+	struct sync_walker *sync_walk = container_of(walk, struct sync_walker,
+						     walker);
+	struct kvm_vcpu *vcpu = sync_walk->vcpu;
+
+	if (__rmap_write_protect(vcpu->kvm, sp->gfn))
+		sync_walk->write_protected = true;
+	return need_resched();
+}
+
 static void mmu_sync_children(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)
 {
 	struct sync_walker walker = {
-		.walker = { .entry = mmu_sync_fn, },
+		.walker = { .entry = mmu_wprotect_fn,
+			    .clear_unsync = false, },
 		.vcpu = vcpu,
+		.write_protected = false
 	};
 
+	/* collapse the TLB flushes as an optimization */
+	mmu_unsync_walk(sp, &walker.walker);
+	if (walker.write_protected)
+		kvm_flush_remote_tlbs(vcpu->kvm);
+
+	walker.walker.entry = mmu_sync_fn;
+	walker.walker.clear_unsync = true;
+
 	while (mmu_unsync_walk(sp, &walker.walker))
 		cond_resched_lock(&vcpu->kvm->mmu_lock);
 }
@@ -1257,7 +1288,8 @@ static int mmu_zap_fn(struct kvm_mmu_pag
 static int mmu_zap_unsync_children(struct kvm *kvm, struct kvm_mmu_page *sp)
 {
 	struct zap_walker walker = {
-		.walker = { .entry = mmu_zap_fn, },
+		.walker = { .entry = mmu_zap_fn,
+			    .clear_unsync = true, },
 		.kvm = kvm,
 		.zapped = 0,
 	};
#2 lore raw

Re: [patch 1/3] KVM: MMU: collapse remote TLB flushes on root sync

Quoted 21 lines · Marcelo Tosatti

Instead of flushing remote TLB's at every page resync, do an initial pass to write protect the sptes, collapsing the flushes on a single remote TLB invalidation.

kernbench is 2.3% faster on 4-way guest. Improvements have been seen with other loads such as AIM7.

Avi: feel free to change this if you dislike the style (I do, but can't think of anything nicer).

static void mmu_sync_children(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp) {

arch/x86/kvm/mmu.c +3 -1
 	struct sync_walker walker = {
-		.walker = { .entry = mmu_sync_fn, },
+		.walker = { .entry = mmu_wprotect_fn,
+			    .clear_unsync = false, },
 		.vcpu = vcpu,
+		.write_protected = false
 	};
 

arch/x86/kvm/mmu.c static int mmu_sync_fn(struct kvm_mmu_pa · line 1107

Quoted 10 lines
arch/x86/kvm/mmu.c +8 -0
+	/* collapse the TLB flushes as an optimization */
+	mmu_unsync_walk(sp, &walker.walker);
+	if (walker.write_protected)
+		kvm_flush_remote_tlbs(vcpu->kvm);
+
+	walker.walker.entry = mmu_sync_fn;
+	walker.walker.clear_unsync = true;
+
 	while (mmu_unsync_walk(sp, &walker.walker))
 		cond_resched_lock(&vcpu->kvm->mmu_lock);

We're always doing two passes here, which is a bit sad. How about having a single pass which:

- collects unsync pages into an array
- exits on no more unsync pages or max array size reached

Then, iterate over the array:

- write protect all pages
- flush tlb
- sync pages

Loop until the root is synced.

If the number of pages to sync is typically small, and the array is sized to be larger than this, then we only walk the pagetables once.

btw, our walkers are a bit awkward (though still better than what we had before). If we rewrite them into for_each style iterators, the code could become cleaner and shorter.

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

Re: [patch 1/3] KVM: MMU: collapse remote TLB flushes on root sync

Quoted 40 lines · Avi Kivity
Quoted 20 lines · Marcelo Tosatti

Instead of flushing remote TLB's at every page resync, do an initial pass to write protect the sptes, collapsing the flushes on a single remote TLB invalidation.

kernbench is 2.3% faster on 4-way guest. Improvements have been seen with other loads such as AIM7.

Avi: feel free to change this if you dislike the style (I do, but can't think of anything nicer).

static void mmu_sync_children(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp) {

arch/x86/kvm/mmu.c +3 -1
 	struct sync_walker walker = {
-		.walker = { .entry = mmu_sync_fn, },
+		.walker = { .entry = mmu_wprotect_fn,
+			    .clear_unsync = false, },
 		.vcpu = vcpu,
+		.write_protected = false
 	};

arch/x86/kvm/mmu.c static int mmu_sync_fn(struct kvm_mmu_pa · line 1107

Quoted 10 lines
arch/x86/kvm/mmu.c +7 -0
 +	/* collapse the TLB flushes as an optimization */
+	mmu_unsync_walk(sp, &walker.walker);
+	if (walker.write_protected)
+		kvm_flush_remote_tlbs(vcpu->kvm);
+
+	walker.walker.entry = mmu_sync_fn;
+	walker.walker.clear_unsync = true;
+
 	while (mmu_unsync_walk(sp, &walker.walker))
 		cond_resched_lock(&vcpu->kvm->mmu_lock);

We're always doing two passes here, which is a bit sad. How about having a single pass which:

- collects unsync pages into an array
- exits on no more unsync pages or max array size reached

Then, iterate over the array:

Quoted 8 lines

- write protect all pages
- flush tlb
- sync pages

Loop until the root is synced.

If the number of pages to sync is typically small, and the array is sized to be larger than this, then we only walk the pagetables once.

There is significant overhead now in comparison to the early indexing scheme with a list per root. It must be optimized.

A problem with your suggestion is how to clean the unsync bitmap bit in the upper pagetables. The advantage however is that the bitmaps and spte entries can be cached in L1, while currently the cache is blown at every page resync.

What i'm testing now is:

#define KVM_PAGE_ARRAY_NR 16

struct kvm_mmu_pages {
        struct kvm_mmu_page *pages[KVM_PAGE_ARRAY_NR];
        struct kvm_mmu_page *parent_pages[KVM_PAGE_ARRAY_NR];
        unsigned int offset[KVM_PAGE_ARRAY_NR];
        unsigned int nr;
};

static void mmu_sync_children(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)

{
        int i;
        struct kvm_mmu_pages pages;

        kvm_mmu_pages_init(&pages);
        while (mmu_unsync_walk(sp, &pages)) {
                for_each_sp(pages, i) {
                        struct kvm_mmu_page *parent = pages.parent_pages[i];

                        kvm_sync_page(vcpu, pages.pages[i]);
                        __clear_bit(pages.offset[i],
                                    parent->unsync_child_bitmap);
                }
                kvm_mmu_pages_init(&pages);
                cond_resched_lock(&vcpu->kvm->mmu_lock);
        }
}

But a second pass is still needed for levels 3 and 4 (the second pass could be postponed for the next CR3 switch, but i'm not sure its worthwhile).

Also, the array method poorly handles cases with a large number of unsync pages, which are common with 2.4/kscand for example.

Hum, Chris suggests a list_head per level instead of the bitmap.

Quoted 6 lines

btw, our walkers are a bit awkward (though still better than what we had before). If we rewrite them into for_each style iterators, the code could become cleaner and shorter.

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

Re: [patch 1/3] KVM: MMU: collapse remote TLB flushes on root sync

Quoted 3 lines · Marcelo Tosatti

There is significant overhead now in comparison to the early indexing scheme with a list per root. It must be optimized.

What's the typical number of (1) unsynced pages and (2) unsynced pages belonging to next cr3 when switching cr3?

I'm guessing (1) and (2) are almost equal, and both fairly large?

(one nice thing is that pages which are no longer used as pagetables will not be resynced, so our forky workload preformance should be good)

Quoted 2 lines

A problem with your suggestion is how to clean the unsync bitmap bit in the upper pagetables.

True.

Quoted 4 lines

The advantage however is that the bitmaps and spte entries can be cached in L1, while currently the cache is blown at every page resync.

Quoted 9 lines

What i'm testing now is:

#define KVM_PAGE_ARRAY_NR 16

struct kvm_mmu_pages {
        struct kvm_mmu_page *pages[KVM_PAGE_ARRAY_NR];
        struct kvm_mmu_page *parent_pages[KVM_PAGE_ARRAY_NR];
        unsigned int offset[KVM_PAGE_ARRAY_NR];

struct { ... } ...[KVM_PAGE_ARRAY_NR]; // SCNR

Quoted 11 lines
        unsigned int nr;
};

static void mmu_sync_children(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)

{
        int i;
        struct kvm_mmu_pages pages;

        kvm_mmu_pages_init(&pages);
        while (mmu_unsync_walk(sp, &pages)) {

So mmu_unsync_walk() collects unsynced pages, write protects them, flushes the tlb, returns 0 if none found?

Perhaps it can simply collect the pages, then do a write-protect pass, tlb flush, and resync pass. The two passes are now over L1 cached data so they're not too expensive.

Quoted 15 lines
                for_each_sp(pages, i) {
                        struct kvm_mmu_page *parent = pages.parent_pages[i];

                        kvm_sync_page(vcpu, pages.pages[i]);
                        __clear_bit(pages.offset[i],
                                    parent->unsync_child_bitmap);
  


                }
                kvm_mmu_pages_init(&pages);
                cond_resched_lock(&vcpu->kvm->mmu_lock);
        }
}
Quoted 5 lines

But a second pass is still needed for levels 3 and 4 (the second pass could be postponed for the next CR3 switch, but i'm not sure its worthwhile).

We could:
- keep all three parents in the array
- for the bitmap, keep a count of how many bits are set
- when we clear a bit, we dec the count, and if zero, we clear the bit
in the parent's parent.

Quoted 3 lines

Also, the array method poorly handles cases with a large number of unsync pages, which are common with 2.4/kscand for example.

Well, depends how large the array is. If it's large enough, the tlb flush cost is overwhelmed by the cost of the actual resync.

Quoted 3 lines

Hum, Chris suggests a list_head per level instead of the bitmap.

Doesn't work, a page can have multiple parents, so it would need to be linked to multiple lists.

We can have

union {
bitmap;
array;
}

and use the array for sparse bitmaps (or use the bitmap for overflowed arrays)

Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
#5 lore raw

Re: [patch 1/3] KVM: MMU: collapse remote TLB flushes on root sync

Quoted 9 lines · Avi Kivity
Quoted 3 lines · Marcelo Tosatti

There is significant overhead now in comparison to the early indexing scheme with a list per root. It must be optimized.

What's the typical number of (1) unsynced pages and (2) unsynced pages belonging to next cr3 when switching cr3?

I'm guessing (1) and (2) are almost equal, and both fairly large?

About 1% of memory unsynced (including pages that are not pagetables anymore, so the value is not exact).

Not equal, (2) a hundred or so.

Quoted 3 lines

Perhaps it can simply collect the pages, then do a write-protect pass, tlb flush, and resync pass. The two passes are now over L1 cached data so they're not too expensive.

Thats what its doing.

Quoted 5 lines

We could:
- keep all three parents in the array
- for the bitmap, keep a count of how many bits are set
- when we clear a bit, we dec the count, and if zero, we clear the bit
in the parent's parent.

Yep.

#6 lore raw

PATCH 2/3 KVM: MMU: skip global pgtables on sync due to cr3 switch

In reply to Marcelo Tosatti

Skip syncing global pages on cr3 switch (but not on cr4/cr0). This is important for Linux 32-bit guests with PAE, where the kmap page is marked as global.

arch/x86/kvm/mmu.c +50 -8
Index: kvm/arch/x86/kvm/mmu.c
===================================================================
@@ -794,9 +794,11 @@ static struct kvm_mmu_page *kvm_mmu_allo
 	sp->gfns = mmu_memory_cache_alloc(&vcpu->arch.mmu_page_cache, PAGE_SIZE);
 	set_page_private(virt_to_page(sp->spt), (unsigned long)sp);
 	list_add(&sp->link, &vcpu->kvm->arch.active_mmu_pages);
+	INIT_LIST_HEAD(&sp->oos_link);
 	ASSERT(is_empty_shadow_page(sp->spt));
 	sp->slot_bitmap = 0;
 	sp->multimapped = 0;
+	sp->global = 1;
 	sp->parent_pte = parent_pte;
 	--vcpu->kvm->arch.n_free_mmu_pages;
 	return sp;
@@ -1031,10 +1033,18 @@ static struct kvm_mmu_page *kvm_mmu_look
 	return NULL;
 }
 
+static void kvm_unlink_unsync_global(struct kvm *kvm, struct kvm_mmu_page *sp)
+{
+	list_del(&sp->oos_link);
+	--kvm->stat.mmu_unsync_global;
+}
+
 static void kvm_unlink_unsync_page(struct kvm *kvm, struct kvm_mmu_page *sp)
 {
 	WARN_ON(!sp->unsync);
 	sp->unsync = 0;
+	if (sp->global)
+		kvm_unlink_unsync_global(kvm, sp);
 	--kvm->stat.mmu_unsync;
 }
 
@@ -1070,7 +1080,8 @@ static int mmu_sync_fn(struct kvm_mmu_pa
 						     walker);
 	struct kvm_vcpu *vcpu = sync_walk->vcpu;
 
-	kvm_sync_page(vcpu, sp);
+	if (!sp->global)
+		kvm_sync_page(vcpu, sp);
 	return (need_resched() || spin_needbreak(&vcpu->kvm->mmu_lock));
 }
 
@@ -1081,7 +1092,7 @@ static int mmu_wprotect_fn(struct kvm_mm
 						     walker);
 	struct kvm_vcpu *vcpu = sync_walk->vcpu;
 
-	if (__rmap_write_protect(vcpu->kvm, sp->gfn))
+	if (!sp->global && __rmap_write_protect(vcpu->kvm, sp->gfn))
 		sync_walk->write_protected = true;
 	return need_resched();
 }
@@ -1547,9 +1558,15 @@ static int kvm_unsync_page(struct kvm_vc
 		if (s->role.word != sp->role.word)
 			return 1;
 	}
-	kvm_mmu_mark_parents_unsync(vcpu, sp);
 	++vcpu->kvm->stat.mmu_unsync;
 	sp->unsync = 1;
+
+	if (sp->global) {
+		list_add(&sp->oos_link, &vcpu->kvm->arch.oos_global_pages);
+		++vcpu->kvm->stat.mmu_unsync_global;
+	} else
+		kvm_mmu_mark_parents_unsync(vcpu, sp);
+
 	mmu_convert_notrap(sp);
 	return 0;
 }
@@ -1575,12 +1592,21 @@ static int mmu_need_write_protect(struct
 static int set_spte(struct kvm_vcpu *vcpu, u64 *shadow_pte,
 		    unsigned pte_access, int user_fault,
 		    int write_fault, int dirty, int largepage,
-		    gfn_t gfn, pfn_t pfn, bool speculative,
+		    int global, gfn_t gfn, pfn_t pfn, bool speculative,
 		    bool can_unsync)
 {
 	u64 spte;
 	int ret = 0;
 	u64 mt_mask = shadow_mt_mask;
+	struct kvm_mmu_page *sp = page_header(__pa(shadow_pte));
+
+	if (!global && sp->global) {
+		sp->global = 0;
+		if (sp->unsync) {
+			kvm_unlink_unsync_global(vcpu->kvm, sp);
+			kvm_mmu_mark_parents_unsync(vcpu, sp);
+		}
+	}
 
 	/*
 	 * We don't set the accessed bit, since we sometimes want to see
@@ -1640,8 +1666,8 @@ set_pte:
 static void mmu_set_spte(struct kvm_vcpu *vcpu, u64 *shadow_pte,
 			 unsigned pt_access, unsigned pte_access,
 			 int user_fault, int write_fault, int dirty,
-			 int *ptwrite, int largepage, gfn_t gfn,
-			 pfn_t pfn, bool speculative)
+			 int *ptwrite, int largepage, int global,
+			 gfn_t gfn, pfn_t pfn, bool speculative)
 {
 	int was_rmapped = 0;
 	int was_writeble = is_writeble_pte(*shadow_pte);
@@ -1674,7 +1700,7 @@ static void mmu_set_spte(struct kvm_vcpu
 		}
 	}
 	if (set_spte(vcpu, shadow_pte, pte_access, user_fault, write_fault,
-		      dirty, largepage, gfn, pfn, speculative, true)) {
+		      dirty, largepage, global, gfn, pfn, speculative, true)) {
 		if (write_fault)
 			*ptwrite = 1;
 		kvm_x86_ops->tlb_flush(vcpu);
@@ -1731,7 +1757,7 @@ static int direct_map_entry(struct kvm_s
 	    || (walk->largepage && level == PT_DIRECTORY_LEVEL)) {
 		mmu_set_spte(vcpu, sptep, ACC_ALL, ACC_ALL,
 			     0, walk->write, 1, &walk->pt_write,
-			     walk->largepage, gfn, walk->pfn, false);
+			     walk->largepage, 0, gfn, walk->pfn, false);
 		++vcpu->stat.pf_fixed;
 		return 1;
 	}
@@ -1918,6 +1944,15 @@ static void mmu_sync_roots(struct kvm_vc
 	}
 }
 
+static void mmu_sync_global(struct kvm_vcpu *vcpu)
+{
+	struct kvm *kvm = vcpu->kvm;
+	struct kvm_mmu_page *sp, *n;
+
+	list_for_each_entry_safe(sp, n, &kvm->arch.oos_global_pages, oos_link)
+		kvm_sync_page(vcpu, sp);
+}
+
 void kvm_mmu_sync_roots(struct kvm_vcpu *vcpu)
 {
 	spin_lock(&vcpu->kvm->mmu_lock);
@@ -1925,6 +1960,13 @@ void kvm_mmu_sync_roots(struct kvm_vcpu 
 	spin_unlock(&vcpu->kvm->mmu_lock);
 }
 
+void kvm_mmu_sync_global(struct kvm_vcpu *vcpu)
+{
+	spin_lock(&vcpu->kvm->mmu_lock);
+	mmu_sync_global(vcpu);
+	spin_unlock(&vcpu->kvm->mmu_lock);
+}
+
 static gpa_t nonpaging_gva_to_gpa(struct kvm_vcpu *vcpu, gva_t vaddr)
 {
 	return vaddr;
arch/x86/kvm/paging_tmpl.h +6 -4
Index: kvm/arch/x86/kvm/paging_tmpl.h
===================================================================
@@ -274,7 +274,8 @@ static void FNAME(update_pte)(struct kvm
 		return;
 	kvm_get_pfn(pfn);
 	mmu_set_spte(vcpu, spte, page->role.access, pte_access, 0, 0,
-		     gpte & PT_DIRTY_MASK, NULL, largepage, gpte_to_gfn(gpte),
+		     gpte & PT_DIRTY_MASK, NULL, largepage,
+		     gpte & PT_GLOBAL_MASK, gpte_to_gfn(gpte),
 		     pfn, true);
 }
 
@@ -301,8 +302,9 @@ static int FNAME(shadow_walk_entry)(stru
 		mmu_set_spte(vcpu, sptep, access, gw->pte_access & access,
 			     sw->user_fault, sw->write_fault,
 			     gw->ptes[gw->level-1] & PT_DIRTY_MASK,
-			     sw->ptwrite, sw->largepage, gw->gfn, sw->pfn,
-			     false);
+			     sw->ptwrite, sw->largepage,
+			     gw->ptes[gw->level-1] & PT_GLOBAL_MASK,
+			     gw->gfn, sw->pfn, false);
 		sw->sptep = sptep;
 		return 1;
 	}
@@ -579,7 +581,7 @@ static int FNAME(sync_page)(struct kvm_v
 		nr_present++;
 		pte_access = sp->role.access & FNAME(gpte_access)(vcpu, gpte);
 		set_spte(vcpu, &sp->spt[i], pte_access, 0, 0,
-			 is_dirty_pte(gpte), 0, gfn,
+			 is_dirty_pte(gpte), 0, gpte & PT_GLOBAL_MASK, gfn,
 			 spte_to_pfn(sp->spt[i]), true, false);
 	}
 
arch/x86/kvm/x86.c +4 -0
Index: kvm/arch/x86/kvm/x86.c
===================================================================
@@ -104,6 +104,7 @@ struct kvm_stats_debugfs_item debugfs_en
 	{ "mmu_recycled", VM_STAT(mmu_recycled) },
 	{ "mmu_cache_miss", VM_STAT(mmu_cache_miss) },
 	{ "mmu_unsync", VM_STAT(mmu_unsync) },
+	{ "mmu_unsync_global", VM_STAT(mmu_unsync_global) },
 	{ "remote_tlb_flush", VM_STAT(remote_tlb_flush) },
 	{ "largepages", VM_STAT(lpages) },
 	{ NULL }
@@ -315,6 +316,7 @@ void kvm_set_cr0(struct kvm_vcpu *vcpu, 
 	kvm_x86_ops->set_cr0(vcpu, cr0);
 	vcpu->arch.cr0 = cr0;
 
+	kvm_mmu_sync_global(vcpu);
 	kvm_mmu_reset_context(vcpu);
 	return;
 }
@@ -358,6 +360,7 @@ void kvm_set_cr4(struct kvm_vcpu *vcpu, 
 	}
 	kvm_x86_ops->set_cr4(vcpu, cr4);
 	vcpu->arch.cr4 = cr4;
+	kvm_mmu_sync_global(vcpu);
 	kvm_mmu_reset_context(vcpu);
 }
 EXPORT_SYMBOL_GPL(kvm_set_cr4);
@@ -4113,6 +4116,7 @@ struct  kvm *kvm_arch_create_vm(void)
 		return ERR_PTR(-ENOMEM);
 
 	INIT_LIST_HEAD(&kvm->arch.active_mmu_pages);
+	INIT_LIST_HEAD(&kvm->arch.oos_global_pages);
 	INIT_LIST_HEAD(&kvm->arch.assigned_dev_head);
 
 	return kvm;
include/asm-x86/kvm_host.h +6 -0
Index: kvm/include/asm-x86/kvm_host.h
===================================================================
@@ -182,6 +182,8 @@ struct kvm_mmu_page {
 	struct list_head link;
 	struct hlist_node hash_link;
 
+	struct list_head oos_link;
+
 	/*
 	 * The following two entries are used to key the shadow page in the
 	 * hash table.
@@ -198,6 +200,7 @@ struct kvm_mmu_page {
 	int multimapped;         /* More than one parent_pte? */
 	int root_count;          /* Currently serving as active root */
 	bool unsync;
+	bool global;
 	bool unsync_children;
 	union {
 		u64 *parent_pte;               /* !multimapped */
@@ -354,6 +357,7 @@ struct kvm_arch{
 	 */
 	struct list_head active_mmu_pages;
 	struct list_head assigned_dev_head;
+	struct list_head oos_global_pages;
 	struct dmar_domain *intel_iommu_domain;
 	struct kvm_pic *vpic;
 	struct kvm_ioapic *vioapic;
@@ -379,6 +383,7 @@ struct kvm_vm_stat {
 	u32 mmu_recycled;
 	u32 mmu_cache_miss;
 	u32 mmu_unsync;
+	u32 mmu_unsync_global;
 	u32 remote_tlb_flush;
 	u32 lpages;
 };
@@ -597,6 +602,7 @@ void __kvm_mmu_free_some_pages(struct kv
 int kvm_mmu_load(struct kvm_vcpu *vcpu);
 void kvm_mmu_unload(struct kvm_vcpu *vcpu);
 void kvm_mmu_sync_roots(struct kvm_vcpu *vcpu);
+void kvm_mmu_sync_global(struct kvm_vcpu *vcpu);
 
 int kvm_emulate_hypercall(struct kvm_vcpu *vcpu);
 
#7 lore raw

Re: [patch 2/3] KVM: MMU: skip global pgtables on sync due to cr3 switch

Quoted 5 lines · Marcelo Tosatti

Skip syncing global pages on cr3 switch (but not on cr4/cr0). This is important for Linux 32-bit guests with PAE, where the kmap page is marked as global.

Patch is good, but won't apply without the first.

arch/x86/kvm/mmu.c static int mmu_need_write_protect(struct · line 1604

Quoted 9 lines

{

arch/x86/kvm/mmu.c +4 -0
 	u64 spte;
 	int ret = 0;
 	u64 mt_mask = shadow_mt_mask;
+	struct kvm_mmu_page *sp = page_header(__pa(shadow_pte));
+
+	if (!global && sp->global) {
+		sp->global = 0;

A slight deficiency in this approach is that a page can't transition from !global to global. I don't think this is frequent, so we don't need to deal with it. But a more accurate approach is to keep a count of non-global present mappings, and to activate the global logic when this count is nonzero.

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

Re: [patch 2/3] KVM: MMU: skip global pgtables on sync due to cr3 switch

Quoted 9 lines · Avi Kivity
Quoted 5 lines · Marcelo Tosatti

Skip syncing global pages on cr3 switch (but not on cr4/cr0). This is important for Linux 32-bit guests with PAE, where the kmap page is marked as global.

Patch is good, but won't apply without the first.

Quoted 13 lines

arch/x86/kvm/mmu.c static int mmu_need_write_protect(struct · line 1604

Quoted 9 lines

{

arch/x86/kvm/mmu.c +4 -0
 	u64 spte;
 	int ret = 0;
 	u64 mt_mask = shadow_mt_mask;
+	struct kvm_mmu_page *sp = page_header(__pa(shadow_pte));
+
+	if (!global && sp->global) {
+		sp->global = 0;

A slight deficiency in this approach is that a page can't transition from !global to global. I don't think this is frequent, so we don't need to deal with it.

Yep.

Quoted 3 lines

But a more accurate approach is to keep a count
of non-global present mappings, and to activate the global logic when this count is nonzero.

I don't think the overhead it adds is significant for the added complexity?

#9 lore raw

Re: [patch 2/3] KVM: MMU: skip global pgtables on sync due to cr3 switch

Quoted 6 lines · Marcelo Tosatti
Quoted 2 lines

But a more accurate approach is to keep a count of non-global present mappings, and to activate the global logic when this count is nonzero.

I don't think the overhead it adds is significant for the added complexity?

Not sure what you're asking exactly (-ENOPARSE). I don't think it's worthwhile to account for !global -> global transitions.

Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
#10 lore raw

Re: [patch 2/3] KVM: MMU: skip global pgtables on sync due to cr3 switch

Quoted 11 lines · Avi Kivity
Quoted 7 lines · Marcelo Tosatti
Quoted 4 lines

But a more accurate approach is to keep a count of non-global present mappings, and to activate the global logic when this count is nonzero.

I don't think the overhead it adds is significant for the added complexity?

Not sure what you're asking exactly (-ENOPARSE). I don't think it's worthwhile to account for !global -> global transitions.

Was referring to "active the global logic when this count is nonzero" (ie don't see the point of keeping non-global count).

#11 lore raw

Re: [patch 2/3] KVM: MMU: skip global pgtables on sync due to cr3 switch

Quoted 6 lines · Marcelo Tosatti
Quoted 2 lines

Not sure what you're asking exactly (-ENOPARSE). I don't think it's worthwhile to account for !global -> global transitions.

Was referring to "active the global logic when this count is nonzero" (ie don't see the point of keeping non-global count).

We agree then.

Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
#12 lore raw

PATCH 3/3 KVM: MMU: prepopulate the shadow on invlpg

In reply to Marcelo Tosatti

If the guest executes invlpg on a non present entry, peek into th pagetable and attempt to prepopulate the shadow entry.

Also stop dirty fault updates from interfering with the fork dete

For RHEL3 and 32-bit Vista guests the success rate is high. With Win2003 there is a 1:2 success/fail ratio, but even then compilation benchmarks are slightly faster. 2000 and XP rarely execute invlpg on non present entries.

2% improvement on RHEL3/AIM7.

arch/x86/kvm/mmu.c +13 -12
Index: kvm/arch/x86/kvm/mmu.c
===================================================================
@@ -2365,7 +2365,8 @@ static void kvm_mmu_access_page(struct k
 }
 
 void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
-		       const u8 *new, int bytes)
+		       const u8 *new, int bytes,
+		       bool speculative)
 {
 	gfn_t gfn = gpa >> PAGE_SHIFT;
 	struct kvm_mmu_page *sp;
@@ -2391,15 +2392,17 @@ void kvm_mmu_pte_write(struct kvm_vcpu *
 	kvm_mmu_free_some_pages(vcpu);
 	++vcpu->kvm->stat.mmu_pte_write;
 	kvm_mmu_audit(vcpu, "pre pte write");
-	if (gfn == vcpu->arch.last_pt_write_gfn
-	    && !last_updated_pte_accessed(vcpu)) {
-		++vcpu->arch.last_pt_write_count;
-		if (vcpu->arch.last_pt_write_count >= 3)
-			flooded = 1;
-	} else {
-		vcpu->arch.last_pt_write_gfn = gfn;
-		vcpu->arch.last_pt_write_count = 1;
-		vcpu->arch.last_pte_updated = NULL;
+	if (!speculative) {
+		if (gfn == vcpu->arch.last_pt_write_gfn
+		    && !last_updated_pte_accessed(vcpu)) {
+			++vcpu->arch.last_pt_write_count;
+			if (vcpu->arch.last_pt_write_count >= 3)
+				flooded = 1;
+		} else {
+			vcpu->arch.last_pt_write_gfn = gfn;
+			vcpu->arch.last_pt_write_count = 1;
+			vcpu->arch.last_pte_updated = NULL;
+		}
 	}
 	index = kvm_page_table_hashfn(gfn);
 	bucket = &vcpu->kvm->arch.mmu_page_hash[index];
@@ -2539,9 +2542,7 @@ EXPORT_SYMBOL_GPL(kvm_mmu_page_fault);
 
 void kvm_mmu_invlpg(struct kvm_vcpu *vcpu, gva_t gva)
 {
-	spin_lock(&vcpu->kvm->mmu_lock);
 	vcpu->arch.mmu.invlpg(vcpu, gva);
-	spin_unlock(&vcpu->kvm->mmu_lock);
 	kvm_mmu_flush_tlb(vcpu);
 	++vcpu->stat.invlpg;
 }
arch/x86/kvm/paging_tmpl.h +26 -2
Index: kvm/arch/x86/kvm/paging_tmpl.h
===================================================================
@@ -82,6 +82,7 @@ struct shadow_walker {
 	int *ptwrite;
 	pfn_t pfn;
 	u64 *sptep;
+	gpa_t pte_gpa;
 };
 
 static gfn_t gpte_to_gfn(pt_element_t gpte)
@@ -222,7 +223,7 @@ walk:
 		if (ret)
 			goto walk;
 		pte |= PT_DIRTY_MASK;
-		kvm_mmu_pte_write(vcpu, pte_gpa, (u8 *)&pte, sizeof(pte));
+		kvm_mmu_pte_write(vcpu, pte_gpa, (u8 *)&pte, sizeof(pte), 1);
 		walker->ptes[walker->level - 1] = pte;
 	}
 
@@ -467,10 +468,18 @@ static int FNAME(shadow_invlpg_entry)(st
 				      struct kvm_vcpu *vcpu, u64 addr,
 				      u64 *sptep, int level)
 {
+	struct shadow_walker *sw =
+		container_of(_sw, struct shadow_walker, walker);
 
 	if (level == PT_PAGE_TABLE_LEVEL) {
-		if (is_shadow_present_pte(*sptep))
+		struct kvm_mmu_page *sp = page_header(__pa(sptep));
+		sw->pte_gpa = (sp->gfn << PAGE_SHIFT);
+		sw->pte_gpa += (sptep - sp->spt) * sizeof(pt_element_t);
+
+		if (is_shadow_present_pte(*sptep)) {
 			rmap_remove(vcpu->kvm, sptep);
+			sw->pte_gpa = -1;
+		}
 		set_shadow_pte(sptep, shadow_trap_nonpresent_pte);
 		return 1;
 	}
@@ -481,11 +490,26 @@ static int FNAME(shadow_invlpg_entry)(st
 
 static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
 {
+	pt_element_t gpte;
 	struct shadow_walker walker = {
 		.walker = { .entry = FNAME(shadow_invlpg_entry), },
+		.pte_gpa = -1,
 	};
 
+	spin_lock(&vcpu->kvm->mmu_lock);
 	walk_shadow(&walker.walker, vcpu, gva);
+	spin_unlock(&vcpu->kvm->mmu_lock);
+	if (walker.pte_gpa == -1)
+		return;
+	if (kvm_read_guest_atomic(vcpu->kvm, walker.pte_gpa, &gpte,
+				  sizeof(pt_element_t)))
+		return;
+	if (is_present_pte(gpte) && (gpte & PT_ACCESSED_MASK)) {
+		if (mmu_topup_memory_caches(vcpu))
+			return;
+		kvm_mmu_pte_write(vcpu, walker.pte_gpa, (const u8 *)&gpte,
+				  sizeof(pt_element_t), 1);
+	}
 }
 
 static gpa_t FNAME(gva_to_gpa)(struct kvm_vcpu *vcpu, gva_t vaddr)
arch/x86/kvm/x86.c +1 -1
Index: kvm/arch/x86/kvm/x86.c
===================================================================
@@ -2044,7 +2044,7 @@ int emulator_write_phys(struct kvm_vcpu 
 	ret = kvm_write_guest(vcpu->kvm, gpa, val, bytes);
 	if (ret < 0)
 		return 0;
-	kvm_mmu_pte_write(vcpu, gpa, val, bytes);
+	kvm_mmu_pte_write(vcpu, gpa, val, bytes, 0);
 	return 1;
 }
 
include/asm-x86/kvm_host.h +2 -1
Index: kvm/include/asm-x86/kvm_host.h
===================================================================
@@ -596,7 +596,8 @@ unsigned long segment_base(u16 selector)
 
 void kvm_mmu_flush_tlb(struct kvm_vcpu *vcpu);
 void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
-		       const u8 *new, int bytes);
+		       const u8 *new, int bytes,
+		       bool speculative);
 int kvm_mmu_unprotect_page_virt(struct kvm_vcpu *vcpu, gva_t gva);
 void __kvm_mmu_free_some_pages(struct kvm_vcpu *vcpu);
 int kvm_mmu_load(struct kvm_vcpu *vcpu);
#13 lore raw

Re: [patch 3/3] KVM: MMU: prepopulate the shadow on invlpg

Quoted 19 lines · Marcelo Tosatti

If the guest executes invlpg on a non present entry, peek into th pagetable and attempt to prepopulate the shadow entry.

Also stop dirty fault updates from interfering with the fork dete

For RHEL3 and 32-bit Vista guests the success rate is high. With Win2003 there is a 1:2 success/fail ratio, but even then compilation benchmarks are slightly faster. 2000 and XP rarely execute invlpg on non present entries.

2% improvement on RHEL3/AIM7.

Index: kvm/arch/x86/kvm/mmu.c
===================================================================

arch/x86/kvm/mmu.c static void kvm_mmu_access_page(struct k · line 2370

Quoted 7 lines
arch/x86/kvm/mmu.c +2 -1
@@ -2365,7 +2365,8 @@ static void kvm_mmu_access_page(struct k
 }
 
 void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
-		       const u8 *new, int bytes)
+		       const u8 *new, int bytes,
+		       bool speculative)
 

kvm_mmu_pte_write()s are always speculative. Maybe this is misnamed?

Perhaps 'guest_initiated' (with the opposite meaning).

arch/x86/kvm/mmu.c walk: · line 227

Quoted 7 lines
arch/x86/kvm/mmu.c +1 -1
@@ -222,7 +223,7 @@ walk:
 		if (ret)
 			goto walk;
 		pte |= PT_DIRTY_MASK;
-		kvm_mmu_pte_write(vcpu, pte_gpa, (u8 *)&pte, sizeof(pte));
+		kvm_mmu_pte_write(vcpu, pte_gpa, (u8 *)&pte, sizeof(pte), 1);
 

This is definitely not a speculative write. But it is !guest_initiated.

arch/x86/kvm/mmu.c static int FNAME(shadow_invlpg_entry)(st · line 476

Quoted 11 lines
arch/x86/kvm/mmu.c +3 -1
@@ -467,10 +468,18 @@ static int FNAME(shadow_invlpg_entry)(st
 				      struct kvm_vcpu *vcpu, u64 addr,
 				      u64 *sptep, int level)
 {
+	struct shadow_walker *sw =
+		container_of(_sw, struct shadow_walker, walker);
 
 	if (level == PT_PAGE_TABLE_LEVEL) {
-		if (is_shadow_present_pte(*sptep))
+		struct kvm_mmu_page *sp = page_header(__pa(sptep));
 

blank line after declarations. Or move to head of function.

arch/x86/kvm/mmu.c static int FNAME(shadow_invlpg_entry)(st · line 483

Quoted 7 lines
arch/x86/kvm/mmu.c +5 -0
+		sw->pte_gpa = (sp->gfn << PAGE_SHIFT);
+		sw->pte_gpa += (sptep - sp->spt) * sizeof(pt_element_t);
+
+		if (is_shadow_present_pte(*sptep)) {
 			rmap_remove(vcpu->kvm, sptep);
+			sw->pte_gpa = -1;
 

Why? The pte could have heen replaced (for example, a write access to a cow page).

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

Re: [patch 3/3] KVM: MMU: prepopulate the shadow on invlpg

Quoted 5 lines · Avi Kivity
Quoted 5 lines

Index: kvm/arch/x86/kvm/mmu.c
===================================================================

arch/x86/kvm/mmu.c +0 -0
@@ -2365,7 +2365,8 @@ static void kvm_mmu_access_page(struct k
Quoted 8 lines

arch/x86/kvm/mmu.c static void kvm_mmu_access_page(struct k

Quoted 6 lines
arch/x86/kvm/mmu.c +2 -1
 }
  void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
-		       const u8 *new, int bytes)
+		       const u8 *new, int bytes,
+		       bool speculative)
 

kvm_mmu_pte_write()s are always speculative. Maybe this is misnamed?

Yep.

Quoted 10 lines

arch/x86/kvm/mmu.c static void kvm_mmu_access_page(struct k

Quoted 7 lines
arch/x86/kvm/mmu.c +5 -0
+		sw->pte_gpa = (sp->gfn << PAGE_SHIFT);
+		sw->pte_gpa += (sptep - sp->spt) * sizeof(pt_element_t);
+
+		if (is_shadow_present_pte(*sptep)) {
 			rmap_remove(vcpu->kvm, sptep);
+			sw->pte_gpa = -1;

Why? The pte could have heen replaced (for example, a write access to a cow page).

Well look-aheads on address space teardown will be useless. OTOH the guest pte read cost is minimal compared to an exit.

Whatever you prefer. Learning guest behaviour as suggested earlier would be optimal, but simple is good.

#15 lore raw

Re: [patch 3/3] KVM: MMU: prepopulate the shadow on invlpg

Quoted 11 lines · Marcelo Tosatti
Quoted 10 lines

arch/x86/kvm/paging_tmpl.h static int FNAME(shadow_invlpg_entry)(st · line 481

Quoted 8 lines
arch/x86/kvm/paging_tmpl.h +5 -0
+		sw->pte_gpa = (sp->gfn << PAGE_SHIFT);
+		sw->pte_gpa += (sptep - sp->spt) * sizeof(pt_element_t);
+
+		if (is_shadow_present_pte(*sptep)) {
 			rmap_remove(vcpu->kvm, sptep);
+			sw->pte_gpa = -1;

Why? The pte could have heen replaced (for example, a write access to a cow page).

Quoted 3 lines

Well look-aheads on address space teardown will be useless. OTOH the guest pte read cost is minimal compared to an exit.

Don't understand. We will incur an exit if a pte is replaced and invlpg'ed due to a copy-on-write (do guests actually execute invlpg after a cow? I don't think they have to).

What is the downside? A pagetable teardown that does not involve zeroing the page? I don't think we'll see invlpg on that path, more likely a complete tlb flush.

Quoted 2 lines

Whatever you prefer. Learning guest behaviour as suggested earlier would be optimal, but simple is good.

We're way past simple. We can reclaim some of the complexity by always doing unsync, and dropping emulation and kvm_mmu_set_pte(), but need to make sure we don't regress on performance. I think Windows does a pde write on context switch, which will add a vmexit, but Windows applications are not too context switch intensive AFAIK.

Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
#16 lore raw

Re: [patch 3/3] KVM: MMU: prepopulate the shadow on invlpg

Quoted 16 lines · Avi Kivity
Quoted 11 lines · Marcelo Tosatti
Quoted 10 lines

arch/x86/kvm/paging_tmpl.h static int FNAME(shadow_invlpg_entry)(st · line 481

Quoted 7 lines
arch/x86/kvm/paging_tmpl.h +5 -0
+		sw->pte_gpa = (sp->gfn << PAGE_SHIFT);
+		sw->pte_gpa += (sptep - sp->spt) * sizeof(pt_element_t);
+
+		if (is_shadow_present_pte(*sptep)) {
 			rmap_remove(vcpu->kvm, sptep);
+			sw->pte_gpa = -1;

Why? The pte could have heen replaced (for example, a write access to a cow page).

Quoted 3 lines

Well look-aheads on address space teardown will be useless. OTOH the guest pte read cost is minimal compared to an exit.

Quoted 7 lines

Don't understand. We will incur an exit if a pte is replaced and invlpg'ed due to a copy-on-write (do guests actually execute invlpg after a cow? I don't think they have to).

What is the downside? A pagetable teardown that does not involve zeroing the page? I don't think we'll see invlpg on that path, more likely a complete tlb flush.

Err, I'm on crack. The assumption is that the common case is pte invalidation + invlpg: kunmap_atomic, page aging clearing the accessed bit, page reclaim.

Linux COW will invalidate + invlpg (do_wp_page) first:

entry = mk_pte(new_page, vma->vm_page_prot); entry = maybe_mkwrite(pte_mkdirty(entry), vma); /*
* Clear the pte entry and flush it first, before
* updating the
* pte with the new entry. This will avoid a race
* condition
* seen in the presence of one thread doing SMC and
* another
* thread doing COW.
*/
ptep_clear_flush_notify(vma, address, page_table);

Not sure about Windows.

Quoted 9 lines
Quoted 3 lines

Whatever you prefer. Learning guest behaviour as suggested earlier would be optimal, but simple is good.

We're way past simple. We can reclaim some of the complexity by always doing unsync, and dropping emulation and kvm_mmu_set_pte(), but need to make sure we don't regress on performance. I think Windows does a pde write on context switch, which will add a vmexit, but Windows applications are not too context switch intensive AFAIK.

#17 lore raw

Re: [patch 3/3] KVM: MMU: prepopulate the shadow on invlpg

Quoted 18 lines · Marcelo Tosatti

Err, I'm on crack. The assumption is that the common case is pte invalidation + invlpg: kunmap_atomic, page aging clearing the accessed bit, page reclaim.

Linux COW will invalidate + invlpg (do_wp_page) first:

entry = mk_pte(new_page, vma->vm_page_prot); entry = maybe_mkwrite(pte_mkdirty(entry), vma); /*
* Clear the pte entry and flush it first, before
* updating the
* pte with the new entry. This will avoid a race
* condition
* seen in the presence of one thread doing SMC and
* another
* thread doing COW.
*/
ptep_clear_flush_notify(vma, address, page_table);

Quoted 3 lines

Not sure about Windows.

Okay, so Linux won't win on this. But is there any downside, apart from fetching the pte and reestablishing the spte?

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

Re: [patch 3/3] KVM: MMU: prepopulate the shadow on invlpg

Quoted 20 lines · Avi Kivity
Quoted 19 lines · Marcelo Tosatti

Err, I'm on crack. The assumption is that the common case is pte invalidation + invlpg: kunmap_atomic, page aging clearing the accessed bit, page reclaim.

Linux COW will invalidate + invlpg (do_wp_page) first:

entry = mk_pte(new_page, vma->vm_page_prot); entry = maybe_mkwrite(pte_mkdirty(entry), vma); /*
* Clear the pte entry and flush it first, before
* updating the
* pte with the new entry. This will avoid a race
* condition
* seen in the presence of one thread doing SMC and
* another
* thread doing COW.
*/
ptep_clear_flush_notify(vma, address, page_table);

Quoted 6 lines
Quoted 3 lines

Not sure about Windows.

Okay, so Linux won't win on this. But is there any downside, apart from fetching the pte and reestablishing the spte?

No, the downside is fetching the pte without being able to establish a valid spte.

#19 lore raw

Re: [patch 3/3] KVM: MMU: prepopulate the shadow on invlpg

Quoted 6 lines · Marcelo Tosatti
Quoted 2 lines

Okay, so Linux won't win on this. But is there any downside, apart from fetching the pte and reestablishing the spte?

No, the downside is fetching the pte without being able to establish a valid spte.

That doesn't seem so expensive. Just a kvm_read_guest_atomic() with a couple of bit tests for present/accessed.

(we do need to optimize copy_from_user_inatomic() to use a single mov insn)

Signature · 1 line
error compiling committee.c: too many arguments to function