KVM: Defer remote tlb flushes on invlpg (v3)

KVM currently flushes the tlbs on all cpus when emulating invlpg. This is because at the time of invlpg we lose track of the page, and leaving stale tlb entries could cause the guest to access the page when it is later freed (say after being swapped out).

However we have a second change to flush the tlbs, when an mmu notifier is called to let us know the host pte has been invalidated. We can safely defer the flush to this point, which occurs much less frequently. Of course, we still do a local tlb flush when emulating invlpg.

Signed-off-by Avi Kivity <[email protected]>

---

Changes from v2:
- dropped remote flushes from guest pagetable write protect paths
- fixed up memory barriers
- use existing local tlb flush in invlpg, no need to add another one

 arch/x86/kvm/mmu.c         |    3 +--
 arch/x86/kvm/paging_tmpl.h |    5 +----
 include/linux/kvm_host.h   |    2 ++
 virt/kvm/kvm_main.c        |   17 +++++++++++------
 4 files changed, 15 insertions(+), 12 deletions(-)

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

index 2a36f7f..f0ea56c 100644
@@ -1184,8 +1184,7 @@ static void mmu_sync_children(struct kvm_vcpu *vcpu,
 		for_each_sp(pages, sp, parents, i)
 			protected |= rmap_write_protect(vcpu->kvm, sp->gfn);
 
-		if (protected)
-			kvm_flush_remote_tlbs(vcpu->kvm);
+		kvm_flush_remote_tlbs_cond(vcpu->kvm, protected);
 
 		for_each_sp(pages, sp, parents, i) {
 			kvm_sync_page(vcpu, sp);

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

index 855eb71..2273b26 100644
@@ -445,7 +445,6 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
 	gpa_t pte_gpa = -1;
 	int level;
 	u64 *sptep;
-	int need_flush = 0;
 
 	spin_lock(&vcpu->kvm->mmu_lock);
 

@@ -465,7 +464,7 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
 				rmap_remove(vcpu->kvm, sptep);
 				if (is_large_pte(*sptep))
 					--vcpu->kvm->stat.lpages;
-				need_flush = 1;
+				vcpu->kvm->remote_tlbs_dirty = true;
 			}
 			set_shadow_pte(sptep, shadow_trap_nonpresent_pte);
 			break;

@@ -475,8 +474,6 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
 			break;
 	}
 
-	if (need_flush)
-		kvm_flush_remote_tlbs(vcpu->kvm);
 	spin_unlock(&vcpu->kvm->mmu_lock);
 
 	if (pte_gpa == -1)

include/linux/kvm_host.h +2 -0

index 11eb702..b779c57 100644
@@ -125,6 +125,7 @@ struct kvm_kernel_irq_routing_entry {
 struct kvm {
 	struct mutex lock; /* protects the vcpus array and APIC accesses */
 	spinlock_t mmu_lock;
+	bool remote_tlbs_dirty;
 	struct rw_semaphore slots_lock;
 	struct mm_struct *mm; /* userspace tied to this vm */
 	int nmemslots;

@@ -235,6 +236,7 @@ void kvm_resched(struct kvm_vcpu *vcpu);
 void kvm_load_guest_fpu(struct kvm_vcpu *vcpu);
 void kvm_put_guest_fpu(struct kvm_vcpu *vcpu);
 void kvm_flush_remote_tlbs(struct kvm *kvm);
+void kvm_flush_remote_tlbs_cond(struct kvm *kvm, bool cond);
 void kvm_reload_remote_mmus(struct kvm *kvm);
 
 long kvm_arch_dev_ioctl(struct file *filp,

virt/kvm/kvm_main.c +11 -6

index 68b217e..12afa50 100644
@@ -758,10 +758,18 @@ static bool make_all_cpus_request(struct kvm *kvm, unsigned int req)
 
 void kvm_flush_remote_tlbs(struct kvm *kvm)
 {
+	kvm->remote_tlbs_dirty = false;
+	smp_wmb();
 	if (make_all_cpus_request(kvm, KVM_REQ_TLB_FLUSH))
 		++kvm->stat.remote_tlb_flush;
 }
 
+void kvm_flush_remote_tlbs_cond(struct kvm *kvm, bool cond)
+{
+	if (cond || kvm->remote_tlbs_dirty)
+		kvm_flush_remote_tlbs(kvm);
+}
+
 void kvm_reload_remote_mmus(struct kvm *kvm)
 {
 	make_all_cpus_request(kvm, KVM_REQ_MMU_RELOAD);

@@ -841,8 +849,7 @@ static void kvm_mmu_notifier_invalidate_page(struct mmu_notifier *mn,
 	spin_unlock(&kvm->mmu_lock);
 
 	/* we've to flush the tlb before the pages can be freed */
-	if (need_tlb_flush)
-		kvm_flush_remote_tlbs(kvm);
+	kvm_flush_remote_tlbs_cond(kvm, need_tlb_flush);
 
 }
 

@@ -866,8 +873,7 @@ static void kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn,
 	spin_unlock(&kvm->mmu_lock);
 
 	/* we've to flush the tlb before the pages can be freed */
-	if (need_tlb_flush)
-		kvm_flush_remote_tlbs(kvm);
+	kvm_flush_remote_tlbs_cond(kvm, need_tlb_flush);
 }
 
 static void kvm_mmu_notifier_invalidate_range_end(struct mmu_notifier *mn,

@@ -907,8 +913,7 @@ static int kvm_mmu_notifier_clear_flush_young(struct mmu_notifier *mn,
 	young = kvm_age_hva(kvm, address);
 	spin_unlock(&kvm->mmu_lock);
 
-	if (young)
-		kvm_flush_remote_tlbs(kvm);
+	kvm_flush_remote_tlbs_cond(kvm, young);
 
 	return young;
 }

Quoted 5 lines · Avi Kivity

KVM currently flushes the tlbs on all cpus when emulating invlpg. This is because at the time of invlpg we lose track of the page, and leaving stale tlb entries could cause the guest to access the page when it is later freed (say after being swapped out).

Quoted 5 lines

However we have a second change to flush the tlbs, when an mmu notifier is called to let us know the host pte has been invalidated. We can safely defer the flush to this point, which occurs much less frequently. Of course, we still do a local tlb flush when emulating invlpg.

I should be able to run some performance comparisons with this in the next day or two.

KVM currently flushes the tlbs on all cpus when emulating invlpg. This is because at the time of invlpg we lose track of the page, and leaving stale tlb entries could cause the guest to access the page when it is later freed (say after being swapped out).

However we have a second change to flush the tlbs, when an mmu notifier is called to let us know the host pte has been invalidated. We can safely defer the flush to this point, which occurs much less frequently. Of course, we still do a local tlb flush when emulating invlpg.

Signed-off-by Avi Kivity <[email protected]>

---

Changes from v2:
- dropped remote flushes from guest pagetable write protect paths
- fixed up memory barriers
- use existing local tlb flush in invlpg, no need to add another one

 arch/x86/kvm/mmu.c         |    3 +--
 arch/x86/kvm/paging_tmpl.h |    5 +----
 include/linux/kvm_host.h   |    2 ++
 virt/kvm/kvm_main.c        |   17 +++++++++++------
 4 files changed, 15 insertions(+), 12 deletions(-)

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

index 2a36f7f..f0ea56c 100644
@@ -1184,8 +1184,7 @@ static void mmu_sync_children(struct kvm_vcpu *vcpu,
 		for_each_sp(pages, sp, parents, i)
 			protected |= rmap_write_protect(vcpu->kvm, sp->gfn);
 
-		if (protected)
-			kvm_flush_remote_tlbs(vcpu->kvm);
+		kvm_flush_remote_tlbs_cond(vcpu->kvm, protected);
 
 		for_each_sp(pages, sp, parents, i) {
 			kvm_sync_page(vcpu, sp);

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

index 855eb71..2273b26 100644
@@ -445,7 +445,6 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
 	gpa_t pte_gpa = -1;
 	int level;
 	u64 *sptep;
-	int need_flush = 0;
 
 	spin_lock(&vcpu->kvm->mmu_lock);
 

@@ -465,7 +464,7 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
 				rmap_remove(vcpu->kvm, sptep);
 				if (is_large_pte(*sptep))
 					--vcpu->kvm->stat.lpages;
-				need_flush = 1;
+				vcpu->kvm->remote_tlbs_dirty = true;
 			}
 			set_shadow_pte(sptep, shadow_trap_nonpresent_pte);
 			break;

@@ -475,8 +474,6 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
 			break;
 	}
 
-	if (need_flush)
-		kvm_flush_remote_tlbs(vcpu->kvm);
 	spin_unlock(&vcpu->kvm->mmu_lock);
 
 	if (pte_gpa == -1)

include/linux/kvm_host.h +2 -0

index 11eb702..b779c57 100644
@@ -125,6 +125,7 @@ struct kvm_kernel_irq_routing_entry {
 struct kvm {
 	struct mutex lock; /* protects the vcpus array and APIC accesses */
 	spinlock_t mmu_lock;
+	bool remote_tlbs_dirty;
 	struct rw_semaphore slots_lock;
 	struct mm_struct *mm; /* userspace tied to this vm */
 	int nmemslots;

@@ -235,6 +236,7 @@ void kvm_resched(struct kvm_vcpu *vcpu);
 void kvm_load_guest_fpu(struct kvm_vcpu *vcpu);
 void kvm_put_guest_fpu(struct kvm_vcpu *vcpu);
 void kvm_flush_remote_tlbs(struct kvm *kvm);
+void kvm_flush_remote_tlbs_cond(struct kvm *kvm, bool cond);
 void kvm_reload_remote_mmus(struct kvm *kvm);
 
 long kvm_arch_dev_ioctl(struct file *filp,

virt/kvm/kvm_main.c +11 -6

index 68b217e..12afa50 100644
@@ -758,10 +758,18 @@ static bool make_all_cpus_request(struct kvm *kvm, unsigned int req)
 
 void kvm_flush_remote_tlbs(struct kvm *kvm)
 {
+	kvm->remote_tlbs_dirty = false;
+	smp_wmb();
 	if (make_all_cpus_request(kvm, KVM_REQ_TLB_FLUSH))
 		++kvm->stat.remote_tlb_flush;
 }
 
+void kvm_flush_remote_tlbs_cond(struct kvm *kvm, bool cond)
+{
+	if (cond || kvm->remote_tlbs_dirty)
+		kvm_flush_remote_tlbs(kvm);
+}
+
 void kvm_reload_remote_mmus(struct kvm *kvm)
 {
 	make_all_cpus_request(kvm, KVM_REQ_MMU_RELOAD);

@@ -841,8 +849,7 @@ static void kvm_mmu_notifier_invalidate_page(struct mmu_notifier *mn,
 	spin_unlock(&kvm->mmu_lock);
 
 	/* we've to flush the tlb before the pages can be freed */
-	if (need_tlb_flush)
-		kvm_flush_remote_tlbs(kvm);
+	kvm_flush_remote_tlbs_cond(kvm, need_tlb_flush);
 
 }
 

@@ -866,8 +873,7 @@ static void kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn,
 	spin_unlock(&kvm->mmu_lock);
 
 	/* we've to flush the tlb before the pages can be freed */
-	if (need_tlb_flush)
-		kvm_flush_remote_tlbs(kvm);
+	kvm_flush_remote_tlbs_cond(kvm, need_tlb_flush);
 }
 
 static void kvm_mmu_notifier_invalidate_range_end(struct mmu_notifier *mn,

@@ -907,8 +913,7 @@ static int kvm_mmu_notifier_clear_flush_young(struct mmu_notifier *mn,
 	young = kvm_age_hva(kvm, address);
 	spin_unlock(&kvm->mmu_lock);
 
-	if (young)
-		kvm_flush_remote_tlbs(kvm);
+	kvm_flush_remote_tlbs_cond(kvm, young);
 
 	return young;
 }

Marcelo, Andrea?

arch/x86/kvm/mmu.c static void mmu_sync_children(struct kvm_vcpu *vcpu, · line 1189

Quoted 12 lines · Avi Kivity

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

index 2a36f7f..f0ea56c 100644
@@ -1184,8 +1184,7 @@ static void mmu_sync_children(struct kvm_vcpu *vcpu,
 		for_each_sp(pages, sp, parents, i)
 			protected |= rmap_write_protect(vcpu->kvm, sp->gfn);
 -		if (protected)
-			kvm_flush_remote_tlbs(vcpu->kvm);
+		kvm_flush_remote_tlbs_cond(vcpu->kvm, protected);
  		for_each_sp(pages, sp, parents, i) {
 			kvm_sync_page(vcpu, sp);

Quoted 9 lines

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

 @@ -465,7 +464,7 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, 

gva_t gva)

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

 				rmap_remove(vcpu->kvm, sptep);
 				if (is_large_pte(*sptep))
 					--vcpu->kvm->stat.lpages;
-				need_flush = 1;
+				vcpu->kvm->remote_tlbs_dirty = true;
 			}
 			set_shadow_pte(sptep, shadow_trap_nonpresent_pte);

arch/x86/kvm/mmu.c static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t · line 478

Quoted 8 lines

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

 			break;

@@ -475,8 +474,6 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t 
 gva)
 			break;
 	}
 -	if (need_flush)
-		kvm_flush_remote_tlbs(vcpu->kvm);
 	spin_unlock(&vcpu->kvm->mmu_lock);

virt/kvm/kvm_main.c static bool make_all_cpus_request(struct kvm *kvm, · line 762

Quoted 10 lines

virt/kvm/kvm_main.c +2 -0

index 68b217e..12afa50 100644
@@ -758,10 +758,18 @@ static bool make_all_cpus_request(struct kvm *kvm, 
 unsigned int req)
  void kvm_flush_remote_tlbs(struct kvm *kvm)
 {
+	kvm->remote_tlbs_dirty = false;
+	smp_wmb();

virt/kvm/kvm_main.c static int kvm_mmu_notifier_clear_flush_young(struct · line 919

Quoted 9 lines

virt/kvm/kvm_main.c +1 -1

@@ -907,8 +913,7 @@ static int kvm_mmu_notifier_clear_flush_young(struct 
 mmu_notifier *mn,
 	young = kvm_age_hva(kvm, address);
 	spin_unlock(&kvm->mmu_lock);
 -	if (young)
-		kvm_flush_remote_tlbs(kvm);
+	kvm_flush_remote_tlbs_cond(kvm, young);
  	return young;
 }

Quoted 1 line · Avi Kivity

Marcelo, Andrea?

Had to read the code a bit more to understand the reason of the unsync_mmu flush in cr3 overwrite.

Quoted 13 lines

arch/x86/kvm/mmu.c static void mmu_sync_children(struct kvm_vcpu *vcpu, · line 1189

Quoted 12 lines · Avi Kivity

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

index 2a36f7f..f0ea56c 100644
@@ -1184,8 +1184,7 @@ static void mmu_sync_children(struct kvm_vcpu *vcpu,
 		for_each_sp(pages, sp, parents, i)
 			protected |= rmap_write_protect(vcpu->kvm, sp->gfn);
 -		if (protected)
-			kvm_flush_remote_tlbs(vcpu->kvm);
+		kvm_flush_remote_tlbs_cond(vcpu->kvm, protected);
  		for_each_sp(pages, sp, parents, i) {
 			kvm_sync_page(vcpu, sp);

Ok so because we didn't flush the tlb on the other vcpus when invlpg run, if cr3 overwrite needs to re-sync sptes wrprotecting them, we've

to flush the tlb in all vcpus to be sure the possibly writable tlb entry reflecting the old writable spte instantiated before invlpg run, is removed from the physical cpus. We wouldn't find it in for_each_sp because it was rmap_removed, but we'll find something in mmu_unsync_walk (right? we definitely have to find something in mmu_unsync_walk for this to work, the parent sp have to leave child->unsync set even after rmap_remove run in invlpg without flushing the other vcpus tlbs).

Quoted 14 lines

Quoted 14 lines

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

 @@ -465,7 +464,7 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, 

gva_t gva)

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

 				rmap_remove(vcpu->kvm, sptep);
 				if (is_large_pte(*sptep))
 					--vcpu->kvm->stat.lpages;
-				need_flush = 1;
+				vcpu->kvm->remote_tlbs_dirty = true;
 			}
 			set_shadow_pte(sptep, shadow_trap_nonpresent_pte);
 			break;

@@ -475,8 +474,6 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t 
 gva)
 			break;
 	}

Quoted 3 lines

arch/x86/kvm/mmu.c static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t · line 478

Quoted 3 lines

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

 -	if (need_flush)
-		kvm_flush_remote_tlbs(vcpu->kvm);
 	spin_unlock(&vcpu->kvm->mmu_lock);

AFIK to be compliant with lowlevel archs (without ASN it doesn't matter I think as vmx always flush on exit), we have to flush the local tlb here, with set_bit(KVM_REQ_TLB_FLUSH, &vcpu->requests). I don't see why it's missing. Or am I wrong?

Quoted 10 lines

virt/kvm/kvm_main.c static bool make_all_cpus_request(struct kvm *kvm, · line 762

Quoted 10 lines

virt/kvm/kvm_main.c +2 -0

index 68b217e..12afa50 100644
@@ -758,10 +758,18 @@ static bool make_all_cpus_request(struct kvm *kvm, 
 unsigned int req)
  void kvm_flush_remote_tlbs(struct kvm *kvm)
 {
+	kvm->remote_tlbs_dirty = false;
+	smp_wmb();

Still no lock prefix to the asm insn and here it runs outside the mmu_lock, but ok, I tend to agree smp_wmb should be enough to be sure the write is fully finished by the time smb_wmb returns. There's another problem though.

CPU0 CPU1
----------- -------------
remote_tlbs_dirty = false
remote_tlbs_dirty = true
smp_tlb_flush

				set_shadow_pte(sptep, shadow_trap_nonpresent_pte);

The flush for the sptep will be lost.

Quoted 9 lines

virt/kvm/kvm_main.c static int kvm_mmu_notifier_clear_flush_young(struct · line 919

Quoted 9 lines

virt/kvm/kvm_main.c +1 -1

@@ -907,8 +913,7 @@ static int kvm_mmu_notifier_clear_flush_young(struct 
 mmu_notifier *mn,
 	young = kvm_age_hva(kvm, address);
 	spin_unlock(&kvm->mmu_lock);
 -	if (young)
-		kvm_flush_remote_tlbs(kvm);
+	kvm_flush_remote_tlbs_cond(kvm, young);
  	return young;
 }

No need to flush for clear_flush_young method, pages can't be freed there.

I mangled over the patch a bit, plus fixed the above smp race, let me know what you think.

The the best workload to exercise this is running a VM with lots of VCPUs and 8G of ram with a 32bit guest kernel and then just malloc and touch a byte for each 4096 page allocated by malloc. That will run a flood of invlpg. Then push the system to swap. while :; do cp /dev/hda /dev/null; done, also works without O_DIRECT so the host cache make it fast at the second run (not so much faster with host swapping though).

I only tested it so far with 12 VM on swap with 64bit kernels with heavy I/O so it's not good test as I doubt any invlpg runs, not even munmap(addr, 4k) uses invlpg.

Signed-off-by Andrea Arcangeli <[email protected]>

---

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

index d5bdf3a..900bc31 100644

include/linux/kvm_host.h +31 -0

+void kvm_flush_local_tlb(struct kvm_vcpu *vcpu)
+{
+	/*
+	 * This will guarantee that our current sptep is flushed from
+	 * the tlb too, even if there's a kvm_flush_remote_tlbs
+	 * running from under us (so if it's not running under
+	 * mmu_lock like in the mmu notifier invocation).
+	 */
+	smp_wmb();
+	/*
+	 * If the below assignment get lost because of lack of atomic ops
+	 * and one or more concurrent writers in kvm_flush_remote_tlbs
+	 * we know that any set_shadow_pte preceding kvm_flush_local_tlb
+	 * was visible to the other CPU, before KVM_REQ_TLB_FLUSH was set
+	 * in each vcpu->requests by kvm_flush_remote_tlbs.
+	 */
+	vcpu->kvm->remote_tlbs_dirty = true;
+
+#if defined(CONFIG_MMU_NOTIFIER) && defined(KVM_ARCH_WANT_MMU_NOTIFIER)
+	/* get new asid before returning to guest mode */
+	if (!test_bit(KVM_REQ_TLB_FLUSH, &vcpu->requests))
+		set_bit(KVM_REQ_TLB_FLUSH, &vcpu->requests);
+#else
+	/*
+	 * Without CONFIG_MMU_NOTIFIER enabled this isn't enough but
+	 * it will reduce the risk window at least.
+	 */
+	kvm_flush_remote_tlbs(vcpu->kvm);
+#endif
+}
+
 void kvm_flush_remote_tlbs(struct kvm *kvm)

include/linux/kvm_host.h +20 -9

@@ -810,6 +847,23 @@ static inline struct kvm *mmu_notifier_to_kvm(struct mmu_notifier *mn)
 	return container_of(mn, struct kvm, mmu_notifier);
 }
 
+static void kvm_mmu_notifier_tlb_flush(int need_tlb_flush, struct kvm *kvm)
+{
+	/*
+	 * We've to flush the tlb before the pages can be freed.
+	 *
+	 * The important "remote_tlbs_dirty = true" that we can't miss
+	 * always runs under mmu_lock before we taken it in the above
+	 * spin_lock. Other than this we've to be sure not to set
+	 * remote_tlbs_dirty to false inside kvm_flush_remote_tlbs
+	 * unless we're going to flush the guest smp tlb for any
+	 * relevant spte that has been invalidated just before setting
+	 * "remote_tlbs_dirty = true".
+	 */
+	if (need_tlb_flush || kvm->remote_tlbs_dirty)
+		kvm_flush_remote_tlbs(kvm);
+}
+
 static void kvm_mmu_notifier_invalidate_page(struct mmu_notifier *mn,
 					     struct mm_struct *mm,
 					     unsigned long address)

@@ -840,10 +894,7 @@ static void kvm_mmu_notifier_invalidate_page(struct mmu_notifier *mn,
 	need_tlb_flush = kvm_unmap_hva(kvm, address);
 	spin_unlock(&kvm->mmu_lock);
 
-	/* we've to flush the tlb before the pages can be freed */
-	if (need_tlb_flush)
-		kvm_flush_remote_tlbs(kvm);
-
+	kvm_mmu_notifier_tlb_flush(need_tlb_flush, kvm);
 }
 
 static void kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn,

@@ -865,9 +916,7 @@ static void kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn,
 		need_tlb_flush |= kvm_unmap_hva(kvm, start);
 	spin_unlock(&kvm->mmu_lock);
 
-	/* we've to flush the tlb before the pages can be freed */
-	if (need_tlb_flush)
-		kvm_flush_remote_tlbs(kvm);
+	kvm_mmu_notifier_tlb_flush(need_tlb_flush, kvm);
 }
 
 static void kvm_mmu_notifier_invalidate_range_end(struct mmu_notifier *mn,

@@ -888,7 +937,7 @@ static void kvm_mmu_notifier_invalidate_range_end(struct mmu_notifier *mn,
 	 * The above sequence increase must be visible before the
 	 * below count decrease but both values are read by the kvm
 	 * page fault under mmu_lock spinlock so we don't need to add
-	 * a smb_wmb() here in between the two.
+	 * a smp_wmb() here in between the two.
 	 */
 	kvm->mmu_notifier_count--;
 	spin_unlock(&kvm->mmu_lock);
--

To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

mmu_sync_children needs to find any _reachable_ sptes that are unsync, read the guest pagetable, and sync the sptes. Before it can inspect the guest pagetable, it needs to write protect it, with rmap_write_protect.

In theory it wouldnt be necesarry to call kvm_flush_remote_tlbs_cond(protected) here, but only kvm_flush_remote_tlbs(), since the "kvm->remote_tlbs_dirty" information is not pertinent to mmu_sync_children.

But this is done here to "clear" remote_tlbs_dirty (after a kvm_flush_remote_tlbs remote_tlbs_dirty is clean), ie: as an optimization.

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

@@ -465,7 +464,7 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu,
gva_t gva)

(quoted diff) +1 -1

 				rmap_remove(vcpu->kvm, sptep);
 				if (is_large_pte(*sptep))
 					--vcpu->kvm->stat.lpages;
-				need_flush = 1;
+				vcpu->kvm->remote_tlbs_dirty = true;
 			}
 			set_shadow_pte(sptep, shadow_trap_nonpresent_pte);
 			break;

@@ -475,8 +474,6 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t 
 gva)
 			break;
 	}

Quoted 8 lines

Quoted 3 lines

arch/x86/kvm/paging_tmpl.h static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva) · line 487

Quoted 3 lines

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

 -	if (need_flush)
-		kvm_flush_remote_tlbs(vcpu->kvm);
 	spin_unlock(&vcpu->kvm->mmu_lock);

AFIK to be compliant with lowlevel archs (without ASN it doesn't matter I think as vmx always flush on exit), we have to flush the local tlb here, with set_bit(KVM_REQ_TLB_FLUSH, &vcpu->requests). I don't see why it's missing. Or am I wrong?

Caller does it:

void kvm_mmu_invlpg(struct kvm_vcpu *vcpu, gva_t gva)

{
        vcpu->arch.mmu.invlpg(vcpu, gva);
        kvm_mmu_flush_tlb(vcpu);
        ++vcpu->stat.invlpg;
}

What about protecting remote_tlbs_dirty with mmu_lock? Only caller of kvm_flush_remote_tlbs that lacks mmu_notifier is kvm_mmu_zap_all, which is not performance sensitive anyway.

Quoted 7 lines

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

-		if (protected)
+		/*
+		 * Avoid leaving stale tlb entries in this vcpu representing
+		 * sptes rmap_removed by invlpg without vcpu smp tlb flush.
+		 */
+		if (protected || vcpu->kvm->remote_tlbs_dirty)
 			kvm_flush_remote_tlbs(vcpu->kvm);

virt/kvm/kvm_main.c +5 -0

+void kvm_flush_remote_tlbs_cond(struct kvm *kvm, bool cond)
+{
+   if (cond || kvm->remote_tlbs_dirty)
+       kvm_flush_remote_tlbs(kvm);
+}

Aren't they the same?

Quoted 7 lines

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

index 09782a9..060b4a3 100644
@@ -483,7 +483,7 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
 	}
 

arch/x86/kvm/paging_tmpl.h static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva) · line 487

Quoted 4 lines

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

 	if (need_flush)
-		kvm_flush_remote_tlbs(vcpu->kvm);
+		kvm_flush_local_tlb(vcpu);
 	spin_unlock(&vcpu->kvm->mmu_lock);

No need, caller does it for us.

OOO ? Out Of Options?

If remote_tlbs_dirty is protected by mmu_lock, most of this complexity is unecessary?

Quoted 3 lines · Marcelo Tosatti

mmu_sync_children needs to find any _reachable_ sptes that are unsync, read the guest pagetable, and sync the sptes. Before it can inspect the guest pagetable, it needs to write protect it, with rmap_write_protect.

So far so good.

Quoted 4 lines

In theory it wouldnt be necesarry to call kvm_flush_remote_tlbs_cond(protected) here, but only kvm_flush_remote_tlbs(), since the "kvm->remote_tlbs_dirty" information is not pertinent to mmu_sync_children.

Hmm I'm not sure I fully understand how it is not pertinent.

When we have a cr3 write in a remote vcpu, mmu_sync_children runs and it resyncs all sptes reachaeble for that remote vcpu context. But to resync the sptes, it also has to get rid of any old writable tlb entry for its remote vcpu where cr3 write is running. Checking only sptes to find writable ones, updating the sptes that are mapped by the writable sptes, and marking them wrprotected, isn't enough, as old spte contents may still be cached in the tlb if remote_tlbs_dirty is true!

Think if the invlpg'd spte that got nuked by rmap_remove in the invlpg handler running in the current vcpu has a writable tlb entry active in the vcpu that later does cr3 overwrite. mmu_sync_children running in the remote vcpu will find no writable spte in the rmap chain representing that spte (because that spte that is still cached in the remote tlb, has already been zapped by the current vcpu) but it is still cached and writable in the remote vcpu TLB cache, when cr3 overwrite runs.

Quoted 3 lines

But this is done here to "clear" remote_tlbs_dirty (after a kvm_flush_remote_tlbs remote_tlbs_dirty is clean), ie: as an optimization.

The whole point of remote_tlbs_dirty is to defer any smp tlb flush at the least possible time, so how can it be an optimization to run a kvm_flush_remote_tlbs earlier than necessary? The only way this can be an optimization, is to never run kvm_flush_remote_tlbs unless absolutely necessary, and to leave remote_tlbs_dirty true instead of calling kvm_flush_remote_tlbs. Calling kvm_flush_remote_tlbs_cond instead of kvm_flush_remote_tlbs cannot ever be an optimization.

Quoted 23 lines

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

@@ -465,7 +464,7 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu,
gva_t gva)

(quoted diff) +1 -1

 				rmap_remove(vcpu->kvm, sptep);
 				if (is_large_pte(*sptep))
 					--vcpu->kvm->stat.lpages;
-				need_flush = 1;
+				vcpu->kvm->remote_tlbs_dirty = true;
 			}
 			set_shadow_pte(sptep, shadow_trap_nonpresent_pte);
 			break;

@@ -475,8 +474,6 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t 
 gva)
 			break;
 	}

Quoted 8 lines

Quoted 3 lines

arch/x86/kvm/paging_tmpl.h static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva) · line 487

Quoted 3 lines

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

 -	if (need_flush)
-		kvm_flush_remote_tlbs(vcpu->kvm);
 	spin_unlock(&vcpu->kvm->mmu_lock);

AFIK to be compliant with lowlevel archs (without ASN it doesn't matter I think as vmx always flush on exit), we have to flush the local tlb here, with set_bit(KVM_REQ_TLB_FLUSH, &vcpu->requests). I don't see why it's missing. Or am I wrong?

Quoted 8 lines

Caller does it:

void kvm_mmu_invlpg(struct kvm_vcpu *vcpu, gva_t gva)

{
        vcpu->arch.mmu.invlpg(vcpu, gva);
        kvm_mmu_flush_tlb(vcpu);
        ++vcpu->stat.invlpg;
}

Ah great! Avi also mentioned it I recall but I didn't figure out it was after FNAME(invlpg) returns. But isn't always more efficient to set_bit(KVM_REQ_TLB_FLUSH, &vcpu->requests) instead like I'm doing?

See my version of kvm_flush_local_tlb, that's a bit different from kvm_mmu_flush_tlb and I'm made the old no-mmu notifier kernel safe too which I think is worth it. If you like to keep my version of kvm_flush_local_tlb, then I've simply to remove the kvm_mmu_flush_tlb from kvm_mmu_invlpg and move it inside the arch.mmu.invlpg handler so each shadow implementation does it its way. Comments?

If you disagree, and you want to run kvm_mmu_flush_tlb in kvm_mmu_invlpg instead of set_bit(KVM_REQ_TLB_FLUSH, &vcpu->requests), then I can expand the kvm_flush_local_tlb with the smp_wmb() in the FNAME(invlpg) code. Like:

	      if (need_flush) {
	         smp_wmb();
		 remote_tlbs_dirty = true;
	      }
	      spin_unlock(mmu_lock);

Then the local tlb flush will run when we return from FNAME(invlpg) and remote_tlbs_dirty is set _after_ set_shadow_pte and _before_ releasing mmu_lock, making it still safe against mmu_notifier_invalidate_page/range.

Quoted 3 lines

What about protecting remote_tlbs_dirty with mmu_lock? Only caller of kvm_flush_remote_tlbs that lacks mmu_notifier is kvm_mmu_zap_all, which is not performance sensitive anyway.

I thought it too I've to say. Tried a bit too, then I figured out why Avi wanted to do out of order. The reason is that we want to allow kvm_flush_remote_tlbs to run outside the mmu_lock too. So this actually results in more self-containment of the remote_tlb_dirty logic and simpler code. But I documented at least what the smb_wmb is serializing and how it works.

Quoted 14 lines

Quoted 7 lines

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

-		if (protected)
+		/*
+		 * Avoid leaving stale tlb entries in this vcpu representing
+		 * sptes rmap_removed by invlpg without vcpu smp tlb flush.
+		 */
+		if (protected || vcpu->kvm->remote_tlbs_dirty)
 			kvm_flush_remote_tlbs(vcpu->kvm);

virt/kvm/kvm_main.c +5 -0

+void kvm_flush_remote_tlbs_cond(struct kvm *kvm, bool cond)
+{
+   if (cond || kvm->remote_tlbs_dirty)
+       kvm_flush_remote_tlbs(kvm);
+}

Quoted 1 line

Aren't they the same?

I didn't particularly like the kvm_flush_remote_tlbs_cond, and because I ended up editing the patch by hand in my code to fix it as I understood it, I ended up refactoring some bits like this. But if you like I can add it back, I don't really care.

Quoted 8 lines

Quoted 8 lines

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

index 09782a9..060b4a3 100644
@@ -483,7 +483,7 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
 	}
 

Quoted 6 lines

arch/x86/kvm/paging_tmpl.h static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva) · line 487

Quoted 4 lines

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

 	if (need_flush)
-		kvm_flush_remote_tlbs(vcpu->kvm);
+		kvm_flush_local_tlb(vcpu);
 	spin_unlock(&vcpu->kvm->mmu_lock);

No need, caller does it for us.

Right but caller does it different, as commented above. Clearly one of the two has to go ;).

Quoted 1 line

OOO ? Out Of Options?

Eheeh, I meant out of order.

Quoted 2 lines

If remote_tlbs_dirty is protected by mmu_lock, most of this complexity is unecessary?

Answered above.

Thanks a lot for review!! My current thought is that we should just move the unnecessary ->tlb_flush from the common invlpg handler to the other lowlevel .invlpg handler and then I hope this optimization will be measurable ;).

I'm fine with your kvm_flush_local_tlb. Just one minor nit:

virt/kvm/kvm_main.c +3 -0

+   /* get new asid before returning to guest mode */
+   if (!test_bit(KVM_REQ_TLB_FLUSH, &vcpu->requests))
+               set_bit(KVM_REQ_TLB_FLUSH, &vcpu->requests);

Whats the test_bit for?

It was nice to hide explicit knowledge about vcpu->kvm->remote_tlbs_dirty behind the interface instead of exposing it.