[patch 0/3] kvm: split kvm_arch_flush_shadow and move mmio spte invalidation to x86

[patch 0/3] kvm: split kvm_arch_flush_shadow and move mmio spte invalidation to x86

[Marcelo Tosatti]

Paul, does that work for you.

Avi, Xiao, please review.

[patch 1/3] KVM: split kvm_arch_flush_shadow

[Marcelo Tosatti]

[-- Attachment #1: 01-flush-per-memslot --]
[-- Type: text/plain, Size: 4329 bytes --]

Introducing kvm_arch_flush_shadow_memslot, to invalidate the
translations of a single memory slot.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: kvm/include/linux/kvm_host.h
===================================================================
--- kvm.orig/include/linux/kvm_host.h
+++ kvm/include/linux/kvm_host.h
@@ -458,7 +458,11 @@ void kvm_arch_commit_memory_region(struc
 				int user_alloc);
 bool kvm_largepages_enabled(void);
 void kvm_disable_largepages(void);
-void kvm_arch_flush_shadow(struct kvm *kvm);
+/* flush all memory translations */
+void kvm_arch_flush_shadow_all(struct kvm *kvm);
+/* flush memory translations pointing to 'slot' */
+void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
+				   struct kvm_memory_slot *slot);
 
 int gfn_to_page_many_atomic(struct kvm *kvm, gfn_t gfn, struct page **pages,
 			    int nr_pages);
Index: kvm/virt/kvm/kvm_main.c
===================================================================
--- kvm.orig/virt/kvm/kvm_main.c
+++ kvm/virt/kvm/kvm_main.c
@@ -408,7 +408,7 @@ static void kvm_mmu_notifier_release(str
 	int idx;
 
 	idx = srcu_read_lock(&kvm->srcu);
-	kvm_arch_flush_shadow(kvm);
+	kvm_arch_flush_shadow_all(kvm);
 	srcu_read_unlock(&kvm->srcu, idx);
 }
 
@@ -582,7 +582,7 @@ static void kvm_destroy_vm(struct kvm *k
 #if defined(CONFIG_MMU_NOTIFIER) && defined(KVM_ARCH_WANT_MMU_NOTIFIER)
 	mmu_notifier_unregister(&kvm->mmu_notifier, kvm->mm);
 #else
-	kvm_arch_flush_shadow(kvm);
+	kvm_arch_flush_shadow_all(kvm);
 #endif
 	kvm_arch_destroy_vm(kvm);
 	kvm_free_physmem(kvm);
@@ -814,7 +814,7 @@ int __kvm_set_memory_region(struct kvm *
 		 * 	- gfn_to_hva (kvm_read_guest, gfn_to_pfn)
 		 * 	- kvm_is_visible_gfn (mmu_check_roots)
 		 */
-		kvm_arch_flush_shadow(kvm);
+		kvm_arch_flush_shadow_memslot(kvm, slot);
 		kfree(old_memslots);
 	}
 
@@ -854,7 +854,7 @@ int __kvm_set_memory_region(struct kvm *
 	 * mmio sptes.
 	 */
 	if (npages && old.base_gfn != mem->guest_phys_addr >> PAGE_SHIFT)
-		kvm_arch_flush_shadow(kvm);
+		kvm_arch_flush_shadow_all(kvm);
 
 	kvm_free_physmem_slot(&old, &new);
 	kfree(old_memslots);
Index: kvm/arch/ia64/kvm/kvm-ia64.c
===================================================================
--- kvm.orig/arch/ia64/kvm/kvm-ia64.c
+++ kvm/arch/ia64/kvm/kvm-ia64.c
@@ -1613,11 +1613,17 @@ void kvm_arch_commit_memory_region(struc
 	return;
 }
 
-void kvm_arch_flush_shadow(struct kvm *kvm)
+void kvm_arch_flush_shadow_all(struct kvm *kvm)
 {
 	kvm_flush_remote_tlbs(kvm);
 }
 
+void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
+				   struct kvm_memory_slot *slot)
+{
+	kvm_arch_flush_shadow_all();
+}
+
 long kvm_arch_dev_ioctl(struct file *filp,
 			unsigned int ioctl, unsigned long arg)
 {
Index: kvm/arch/powerpc/kvm/powerpc.c
===================================================================
--- kvm.orig/arch/powerpc/kvm/powerpc.c
+++ kvm/arch/powerpc/kvm/powerpc.c
@@ -334,8 +334,12 @@ void kvm_arch_commit_memory_region(struc
 	kvmppc_core_commit_memory_region(kvm, mem);
 }
 
+void kvm_arch_flush_shadow_all(struct kvm *kvm)
+{
+}
 
-void kvm_arch_flush_shadow(struct kvm *kvm)
+void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
+				   struct kvm_memory_slot *slot)
 {
 }
 
Index: kvm/arch/s390/kvm/kvm-s390.c
===================================================================
--- kvm.orig/arch/s390/kvm/kvm-s390.c
+++ kvm/arch/s390/kvm/kvm-s390.c
@@ -969,7 +969,12 @@ void kvm_arch_commit_memory_region(struc
 	return;
 }
 
-void kvm_arch_flush_shadow(struct kvm *kvm)
+void kvm_arch_flush_shadow_all(struct kvm *kvm)
+{
+}
+
+void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
+				   struct kvm_memory_slot *slot)
 {
 }
 
Index: kvm/arch/x86/kvm/x86.c
===================================================================
--- kvm.orig/arch/x86/kvm/x86.c
+++ kvm/arch/x86/kvm/x86.c
@@ -6457,12 +6457,18 @@ void kvm_arch_commit_memory_region(struc
 	spin_unlock(&kvm->mmu_lock);
 }
 
-void kvm_arch_flush_shadow(struct kvm *kvm)
+void kvm_arch_flush_shadow_all(struct kvm *kvm)
 {
 	kvm_mmu_zap_all(kvm);
 	kvm_reload_remote_mmus(kvm);
 }
 
+void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
+				   struct kvm_memory_slot *slot)
+{
+	kvm_arch_flush_shadow_all(kvm);
+}
+
 int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
 {
 	return (vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE &&

[patch 2/3] KVM: perform an invalid memslot step for gpa base change

[Marcelo Tosatti]

[-- Attachment #1: 02-invalidate-translations-on-memslot-gpa-change --]
[-- Type: text/plain, Size: 1006 bytes --]

PPC must flush all translations before the new memory slot
is visible.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: kvm/virt/kvm/kvm_main.c
===================================================================
--- kvm.orig/virt/kvm/kvm_main.c
+++ kvm/virt/kvm/kvm_main.c
@@ -791,7 +791,7 @@ int __kvm_set_memory_region(struct kvm *
 		/* destroy any largepage mappings for dirty tracking */
 	}
 
-	if (!npages) {
+	if (!npages || base_gfn != old.base_gfn) {
 		struct kvm_memory_slot *slot;
 
 		r = -ENOMEM;
@@ -807,8 +807,8 @@ int __kvm_set_memory_region(struct kvm *
 		old_memslots = kvm->memslots;
 		rcu_assign_pointer(kvm->memslots, slots);
 		synchronize_srcu_expedited(&kvm->srcu);
-		/* From this point no new shadow pages pointing to a deleted
-		 * memslot will be created.
+		/* From this point no new shadow pages pointing to a deleted,
+ 		 * or moved, memslot will be created.
 		 *
 		 * validation of sp->gfn happens in:
 		 * 	- gfn_to_hva (kvm_read_guest, gfn_to_pfn)

[patch 3/3] KVM: move postcommit flush to x86, as mmio sptes are x86 specific

[Marcelo Tosatti]

[-- Attachment #1: 03-move-postcommit-flush-to-x86 --]
[-- Type: text/plain, Size: 1221 bytes --]

Other arches do not need this.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: kvm/arch/x86/kvm/x86.c
===================================================================
--- kvm.orig/arch/x86/kvm/x86.c
+++ kvm/arch/x86/kvm/x86.c
@@ -6455,6 +6455,14 @@ void kvm_arch_commit_memory_region(struc
 		kvm_mmu_change_mmu_pages(kvm, nr_mmu_pages);
 	kvm_mmu_slot_remove_write_access(kvm, mem->slot);
 	spin_unlock(&kvm->mmu_lock);
+	/*
+	 * If the new memory slot is created, we need to clear all
+	 * mmio sptes.
+	 */
+	if (old.npages == 0 && npages) {
+		kvm_mmu_zap_all(kvm);
+		kvm_reload_remote_mmus(kvm);
+	}
 }
 
 void kvm_arch_flush_shadow_all(struct kvm *kvm)
Index: kvm/virt/kvm/kvm_main.c
===================================================================
--- kvm.orig/virt/kvm/kvm_main.c
+++ kvm/virt/kvm/kvm_main.c
@@ -849,13 +849,6 @@ int __kvm_set_memory_region(struct kvm *
 
 	kvm_arch_commit_memory_region(kvm, mem, old, user_alloc);
 
-	/*
-	 * If the new memory slot is created, we need to clear all
-	 * mmio sptes.
-	 */
-	if (npages && old.base_gfn != mem->guest_phys_addr >> PAGE_SHIFT)
-		kvm_arch_flush_shadow_all(kvm);
-
 	kvm_free_physmem_slot(&old, &new);
 	kfree(old_memslots);
 

Re: [patch 3/3] KVM: move postcommit flush to x86, as mmio sptes are x86 specific

[Xiao Guangrong]

On 08/25/2012 02:54 AM, Marcelo Tosatti wrote:
> Other arches do not need this.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Index: kvm/arch/x86/kvm/x86.c
> ===================================================================
> --- kvm.orig/arch/x86/kvm/x86.c
> +++ kvm/arch/x86/kvm/x86.c
> @@ -6455,6 +6455,14 @@ void kvm_arch_commit_memory_region(struc
>  		kvm_mmu_change_mmu_pages(kvm, nr_mmu_pages);
>  	kvm_mmu_slot_remove_write_access(kvm, mem->slot);
>  	spin_unlock(&kvm->mmu_lock);
> +	/*
> +	 * If the new memory slot is created, we need to clear all
> +	 * mmio sptes.
> +	 */
> +	if (old.npages == 0 && npages) {
> +		kvm_mmu_zap_all(kvm);
> +		kvm_reload_remote_mmus(kvm);
> +	}

Can not use kvm_arch_flush_shadow_all()?

Others are fine to me.

Re: [patch 3/3] KVM: move postcommit flush to x86, as mmio sptes are x86 specific

[Marcelo Tosatti]

On Mon, Aug 27, 2012 at 04:04:44PM +0800, Xiao Guangrong wrote:
> On 08/25/2012 02:54 AM, Marcelo Tosatti wrote:
> > Other arches do not need this.
> > 
> > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> > 
> > Index: kvm/arch/x86/kvm/x86.c
> > ===================================================================
> > --- kvm.orig/arch/x86/kvm/x86.c
> > +++ kvm/arch/x86/kvm/x86.c
> > @@ -6455,6 +6455,14 @@ void kvm_arch_commit_memory_region(struc
> >  		kvm_mmu_change_mmu_pages(kvm, nr_mmu_pages);
> >  	kvm_mmu_slot_remove_write_access(kvm, mem->slot);
> >  	spin_unlock(&kvm->mmu_lock);
> > +	/*
> > +	 * If the new memory slot is created, we need to clear all
> > +	 * mmio sptes.
> > +	 */
> > +	if (old.npages == 0 && npages) {
> > +		kvm_mmu_zap_all(kvm);
> > +		kvm_reload_remote_mmus(kvm);
> > +	}
> 
> Can not use kvm_arch_flush_shadow_all()?

Better reduce it to necessary cases only.

Re: [patch 3/3] KVM: move postcommit flush to x86, as mmio sptes are x86 specific

[Takuya Yoshikawa]

On Fri, 24 Aug 2012 15:54:59 -0300
Marcelo Tosatti <mtosatti@redhat.com> wrote:

> Other arches do not need this.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Index: kvm/arch/x86/kvm/x86.c
> ===================================================================
> --- kvm.orig/arch/x86/kvm/x86.c
> +++ kvm/arch/x86/kvm/x86.c
> @@ -6455,6 +6455,14 @@ void kvm_arch_commit_memory_region(struc
>  		kvm_mmu_change_mmu_pages(kvm, nr_mmu_pages);
>  	kvm_mmu_slot_remove_write_access(kvm, mem->slot);
>  	spin_unlock(&kvm->mmu_lock);
> +	/*
> +	 * If the new memory slot is created, we need to clear all
> +	 * mmio sptes.
> +	 */
> +	if (old.npages == 0 && npages) {
> +		kvm_mmu_zap_all(kvm);
> +		kvm_reload_remote_mmus(kvm);
> +	}
>  }

Any explanation why (old.base_gfn != new.base_gfn) case can be
omitted?

Takuya

>  
>  void kvm_arch_flush_shadow_all(struct kvm *kvm)
> Index: kvm/virt/kvm/kvm_main.c
> ===================================================================
> --- kvm.orig/virt/kvm/kvm_main.c
> +++ kvm/virt/kvm/kvm_main.c
> @@ -849,13 +849,6 @@ int __kvm_set_memory_region(struct kvm *
>  
>  	kvm_arch_commit_memory_region(kvm, mem, old, user_alloc);
>  
> -	/*
> -	 * If the new memory slot is created, we need to clear all
> -	 * mmio sptes.
> -	 */
> -	if (npages && old.base_gfn != mem->guest_phys_addr >> PAGE_SHIFT)
> -		kvm_arch_flush_shadow_all(kvm);
> -
>  	kvm_free_physmem_slot(&old, &new);
>  	kfree(old_memslots);

Re: [patch 3/3] KVM: move postcommit flush to x86, as mmio sptes are x86 specific

[Marcelo Tosatti]

On Mon, Aug 27, 2012 at 11:41:08PM +0900, Takuya Yoshikawa wrote:
> On Fri, 24 Aug 2012 15:54:59 -0300
> Marcelo Tosatti <mtosatti@redhat.com> wrote:
> 
> > Other arches do not need this.
> > 
> > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> > 
> > Index: kvm/arch/x86/kvm/x86.c
> > ===================================================================
> > --- kvm.orig/arch/x86/kvm/x86.c
> > +++ kvm/arch/x86/kvm/x86.c
> > @@ -6455,6 +6455,14 @@ void kvm_arch_commit_memory_region(struc
> >  		kvm_mmu_change_mmu_pages(kvm, nr_mmu_pages);
> >  	kvm_mmu_slot_remove_write_access(kvm, mem->slot);
> >  	spin_unlock(&kvm->mmu_lock);
> > +	/*
> > +	 * If the new memory slot is created, we need to clear all
> > +	 * mmio sptes.
> > +	 */
> > +	if (old.npages == 0 && npages) {
> > +		kvm_mmu_zap_all(kvm);
> > +		kvm_reload_remote_mmus(kvm);
> > +	}
> >  }
> 
> Any explanation why (old.base_gfn != new.base_gfn) case can be
> omitted?

(old.base_gfn != new.base_gfn) check covers the cases

1. old.base_gfn = 0, new.base_gfn = !0 (slot creation)

and

x != 0, y != 0, x != y.
2. old.base_gfn = x, new.base_gfn = y (gpa base change)

Patch 2 covers case 2, so its only necessary to cover case
1 here.

Makes sense?

> Takuya
> 
> >  
> >  void kvm_arch_flush_shadow_all(struct kvm *kvm)
> > Index: kvm/virt/kvm/kvm_main.c
> > ===================================================================
> > --- kvm.orig/virt/kvm/kvm_main.c
> > +++ kvm/virt/kvm/kvm_main.c
> > @@ -849,13 +849,6 @@ int __kvm_set_memory_region(struct kvm *
> >  
> >  	kvm_arch_commit_memory_region(kvm, mem, old, user_alloc);
> >  
> > -	/*
> > -	 * If the new memory slot is created, we need to clear all
> > -	 * mmio sptes.
> > -	 */
> > -	if (npages && old.base_gfn != mem->guest_phys_addr >> PAGE_SHIFT)
> > -		kvm_arch_flush_shadow_all(kvm);
> > -
> >  	kvm_free_physmem_slot(&old, &new);
> >  	kfree(old_memslots);

Re: [patch 3/3] KVM: move postcommit flush to x86, as mmio sptes are x86 specific

[Takuya Yoshikawa]

On Mon, 27 Aug 2012 16:06:01 -0300
Marcelo Tosatti <mtosatti@redhat.com> wrote:

> > Any explanation why (old.base_gfn != new.base_gfn) case can be
> > omitted?
> 
> (old.base_gfn != new.base_gfn) check covers the cases
> 
> 1. old.base_gfn = 0, new.base_gfn = !0 (slot creation)
> 
> and
> 
> x != 0, y != 0, x != y.
> 2. old.base_gfn = x, new.base_gfn = y (gpa base change)
> 
> Patch 2 covers case 2, so its only necessary to cover case
> 1 here.
> 
> Makes sense?

Yes.

But didn't you change the flush in the if block modified by patch 2
to kvm_arch_flush_shadow_memslot()?

Although current implementation flushes everything, this may trigger
problem when we change it.

	Takuya

Re: [patch 3/3] KVM: move postcommit flush to x86, as mmio sptes are x86 specific

[Marcelo Tosatti]

On Tue, Aug 28, 2012 at 09:30:24AM +0900, Takuya Yoshikawa wrote:
> On Mon, 27 Aug 2012 16:06:01 -0300
> Marcelo Tosatti <mtosatti@redhat.com> wrote:
> 
> > > Any explanation why (old.base_gfn != new.base_gfn) case can be
> > > omitted?
> > 
> > (old.base_gfn != new.base_gfn) check covers the cases
> > 
> > 1. old.base_gfn = 0, new.base_gfn = !0 (slot creation)
> > 
> > and
> > 
> > x != 0, y != 0, x != y.
> > 2. old.base_gfn = x, new.base_gfn = y (gpa base change)
> > 
> > Patch 2 covers case 2, so its only necessary to cover case
> > 1 here.
> > 
> > Makes sense?
> 
> Yes.
> 
> But didn't you change the flush in the if block modified by patch 2
> to kvm_arch_flush_shadow_memslot()?
> 
> Although current implementation flushes everything, this may trigger
> problem when we change it.
> 
> 	Takuya

Yay, thanks.

[patch 3/3 V2] KVM: move postcommit flush to x86, as mmio sptes are x86 specific

[Marcelo Tosatti]

Other arches do not need this.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

v2: fix incorrect deletion of mmio sptes on gpa move (noticed by Takuya)

Index: kvm/arch/x86/kvm/x86.c
===================================================================
--- kvm.orig/arch/x86/kvm/x86.c
+++ kvm/arch/x86/kvm/x86.c
@@ -6446,6 +6446,14 @@ void kvm_arch_commit_memory_region(struc
 		kvm_mmu_change_mmu_pages(kvm, nr_mmu_pages);
 	kvm_mmu_slot_remove_write_access(kvm, mem->slot);
 	spin_unlock(&kvm->mmu_lock);
+	/*
+	 * If memory slot is created, or moved, we need to clear all
+	 * mmio sptes.
+	 */
+	if (npages && old.base_gfn != mem->guest_phys_addr >> PAGE_SHIFT) {
+		kvm_mmu_zap_all(kvm);
+		kvm_reload_remote_mmus(kvm);
+	}
 }
 
 void kvm_arch_flush_shadow_all(struct kvm *kvm)
Index: kvm/virt/kvm/kvm_main.c
===================================================================
--- kvm.orig/virt/kvm/kvm_main.c
+++ kvm/virt/kvm/kvm_main.c
@@ -849,13 +849,6 @@ int __kvm_set_memory_region(struct kvm *
 
 	kvm_arch_commit_memory_region(kvm, mem, old, user_alloc);
 
-	/*
-	 * If the new memory slot is created, we need to clear all
-	 * mmio sptes.
-	 */
-	if (npages && old.base_gfn != mem->guest_phys_addr >> PAGE_SHIFT)
-		kvm_arch_flush_shadow_all(kvm);
-
 	kvm_free_physmem_slot(&old, &new);
 	kfree(old_memslots);
 

Re: [patch 0/3] kvm: split kvm_arch_flush_shadow and move mmio spte invalidation to x86

[Paul Mackerras]

On Fri, Aug 24, 2012 at 03:54:56PM -0300, Marcelo Tosatti wrote:
> Paul, does that work for you.

Yes, looks great, thanks.

Paul.

Re: [patch 0/3] kvm: split kvm_arch_flush_shadow and move mmio spte invalidation to x86

[Avi Kivity]

On 08/24/2012 09:54 PM, Marcelo Tosatti wrote:
> Paul, does that work for you.
> 

Thanks, applied.


-- 
error compiling committee.c: too many arguments to function