[PATCH 0/2] KVM: Introduce kvm_arch_memslots_updated() and use it for x86's mmio optimization

[PATCH 0/2] KVM: Introduce kvm_arch_memslots_updated() and use it for x86's mmio optimization

[Takuya Yoshikawa]

Patch 1: KVM-arch maintainers, please review this one.
  {x86, power, s390, arm}-kvm maintainers CCed.
  Could not find mips-kvm maintainer in MAINTAINERS.

Patch 2: I did not move the body of kvm_mmu_invalidate_mmio_sptes() into
  x86.c because it looked like mmu details.

Takuya Yoshikawa (2):
  KVM: Introduce kvm_arch_memslots_updated()
  KVM: x86: Avoid zapping mmio sptes twice for generation wraparound

 arch/arm/kvm/arm.c         |    4 ++++
 arch/ia64/kvm/kvm-ia64.c   |    4 ++++
 arch/mips/kvm/kvm_mips.c   |    4 ++++
 arch/powerpc/kvm/powerpc.c |    4 ++++
 arch/s390/kvm/kvm-s390.c   |    4 ++++
 arch/x86/kvm/mmu.c         |    5 +----
 arch/x86/kvm/x86.c         |   14 +++++++++-----
 include/linux/kvm_host.h   |    1 +
 virt/kvm/kvm_main.c        |    5 ++++-
 9 files changed, 35 insertions(+), 10 deletions(-)

-- 
1.7.9.5

[PATCH 1/2] KVM: Introduce kvm_arch_memslots_updated()

[Takuya Yoshikawa]

This is called right after the memslots is updated, i.e. when the result
of update_memslots() gets installed in install_new_memslots().  Since
the memslots needs to be updated twice when we delete or move a memslot,
kvm_arch_commit_memory_region() does not correspond to this exactly.

In the following patch, x86 will use this new API to check if the mmio
generation has reached its maximum value, in which case mmio sptes need
to be flushed out.

Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
---
 Removed the trailing space after "return old_memslots;" at this chance.

 arch/arm/kvm/arm.c         |    4 ++++
 arch/ia64/kvm/kvm-ia64.c   |    4 ++++
 arch/mips/kvm/kvm_mips.c   |    4 ++++
 arch/powerpc/kvm/powerpc.c |    4 ++++
 arch/s390/kvm/kvm-s390.c   |    4 ++++
 arch/x86/kvm/x86.c         |    4 ++++
 include/linux/kvm_host.h   |    1 +
 virt/kvm/kvm_main.c        |    5 ++++-
 8 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 327a1fb..51c3659 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -219,6 +219,10 @@ long kvm_arch_dev_ioctl(struct file *filp,
 	return -EINVAL;
 }
 
+void kvm_arch_memslots_updated(struct kvm *kvm)
+{
+}
+
 int kvm_arch_prepare_memory_region(struct kvm *kvm,
 				   struct kvm_memory_slot *memslot,
 				   struct kvm_userspace_memory_region *mem,
diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index 5b2dc0d..bdfd878 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -1560,6 +1560,10 @@ int kvm_arch_create_memslot(struct kvm_memory_slot *slot, unsigned long npages)
 	return 0;
 }
 
+void kvm_arch_memslots_updated(struct kvm *kvm)
+{
+}
+
 int kvm_arch_prepare_memory_region(struct kvm *kvm,
 		struct kvm_memory_slot *memslot,
 		struct kvm_userspace_memory_region *mem,
diff --git a/arch/mips/kvm/kvm_mips.c b/arch/mips/kvm/kvm_mips.c
index e0dad02..daa32ea 100644
--- a/arch/mips/kvm/kvm_mips.c
+++ b/arch/mips/kvm/kvm_mips.c
@@ -208,6 +208,10 @@ int kvm_arch_create_memslot(struct kvm_memory_slot *slot, unsigned long npages)
 	return 0;
 }
 
+void kvm_arch_memslots_updated(struct kvm *kvm)
+{
+}
+
 int kvm_arch_prepare_memory_region(struct kvm *kvm,
                                 struct kvm_memory_slot *memslot,
                                 struct kvm_userspace_memory_region *mem,
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 6316ee3..ae63ae4 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -420,6 +420,10 @@ int kvm_arch_create_memslot(struct kvm_memory_slot *slot, unsigned long npages)
 	return kvmppc_core_create_memslot(slot, npages);
 }
 
+void kvm_arch_memslots_updated(struct kvm *kvm)
+{
+}
+
 int kvm_arch_prepare_memory_region(struct kvm *kvm,
 				   struct kvm_memory_slot *memslot,
 				   struct kvm_userspace_memory_region *mem,
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index ba694d2..a3d797b 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -1056,6 +1056,10 @@ int kvm_arch_create_memslot(struct kvm_memory_slot *slot, unsigned long npages)
 	return 0;
 }
 
+void kvm_arch_memslots_updated(struct kvm *kvm)
+{
+}
+
 /* Section: memory related */
 int kvm_arch_prepare_memory_region(struct kvm *kvm,
 				   struct kvm_memory_slot *memslot,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 7d71c0f..71912c9 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7020,6 +7020,10 @@ out_free:
 	return -ENOMEM;
 }
 
+void kvm_arch_memslots_updated(struct kvm *kvm)
+{
+}
+
 int kvm_arch_prepare_memory_region(struct kvm *kvm,
 				struct kvm_memory_slot *memslot,
 				struct kvm_userspace_memory_region *mem,
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index e3aae6d..1c1e9de 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -498,6 +498,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
 void kvm_arch_free_memslot(struct kvm_memory_slot *free,
 			   struct kvm_memory_slot *dont);
 int kvm_arch_create_memslot(struct kvm_memory_slot *slot, unsigned long npages);
+void kvm_arch_memslots_updated(struct kvm *kvm);
 int kvm_arch_prepare_memory_region(struct kvm *kvm,
 				struct kvm_memory_slot *memslot,
 				struct kvm_userspace_memory_region *mem,
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 1580dd4..4ed9d89 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -731,7 +731,10 @@ static struct kvm_memslots *install_new_memslots(struct kvm *kvm,
 	update_memslots(slots, new, kvm->memslots->generation);
 	rcu_assign_pointer(kvm->memslots, slots);
 	synchronize_srcu_expedited(&kvm->srcu);
-	return old_memslots; 
+
+	kvm_arch_memslots_updated(kvm);
+
+	return old_memslots;
 }
 
 /*
-- 
1.7.9.5

[PATCH v2 2/2] KVM: x86: Avoid zapping mmio sptes twice for generation wraparound

[Takuya Yoshikawa]

Now that kvm_arch_memslots_updated() catches every increment of the
memslots->generation, checking if the mmio generation has reached its
maximum value is enough.

Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
---
 arch/x86/kvm/mmu.c |    5 +----
 arch/x86/kvm/x86.c |   10 +++++-----
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 0d094da..bf7af1e 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -4383,11 +4383,8 @@ void kvm_mmu_invalidate_mmio_sptes(struct kvm *kvm)
 	/*
 	 * The very rare case: if the generation-number is round,
 	 * zap all shadow pages.
-	 *
-	 * The max value is MMIO_MAX_GEN - 1 since it is not called
-	 * when mark memslot invalid.
 	 */
-	if (unlikely(kvm_current_mmio_generation(kvm) >= (MMIO_MAX_GEN - 1))) {
+	if (unlikely(kvm_current_mmio_generation(kvm) >= MMIO_MAX_GEN)) {
 		printk_ratelimited(KERN_INFO "kvm: zapping shadow pages for mmio generation wraparound\n");
 		kvm_mmu_invalidate_zap_all_pages(kvm);
 	}
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 71912c9..9be6621 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7022,6 +7022,11 @@ out_free:
 
 void kvm_arch_memslots_updated(struct kvm *kvm)
 {
+	/*
+	 * memslots->generation has been incremented.
+	 * mmio generation may have reached its maximum value.
+	 */
+	kvm_mmu_invalidate_mmio_sptes(kvm);
 }
 
 int kvm_arch_prepare_memory_region(struct kvm *kvm,
@@ -7084,11 +7089,6 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
 	 */
 	if ((change != KVM_MR_DELETE) && (mem->flags & KVM_MEM_LOG_DIRTY_PAGES))
 		kvm_mmu_slot_remove_write_access(kvm, mem->slot);
-	/*
-	 * If memory slot is created, or moved, we need to clear all
-	 * mmio sptes.
-	 */
-	kvm_mmu_invalidate_mmio_sptes(kvm);
 }
 
 void kvm_arch_flush_shadow_all(struct kvm *kvm)
-- 
1.7.9.5

Re: [PATCH 1/2] KVM: Introduce kvm_arch_memslots_updated()

[Gleb Natapov]

On Thu, Jul 04, 2013 at 01:40:29PM +0900, Takuya Yoshikawa wrote:
> This is called right after the memslots is updated, i.e. when the result
> of update_memslots() gets installed in install_new_memslots().  Since
> the memslots needs to be updated twice when we delete or move a memslot,
> kvm_arch_commit_memory_region() does not correspond to this exactly.
> 
> In the following patch, x86 will use this new API to check if the mmio
> generation has reached its maximum value, in which case mmio sptes need
> to be flushed out.
> 
> Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
> ---
>  Removed the trailing space after "return old_memslots;" at this chance.
> 
>  arch/arm/kvm/arm.c         |    4 ++++
>  arch/ia64/kvm/kvm-ia64.c   |    4 ++++
>  arch/mips/kvm/kvm_mips.c   |    4 ++++
>  arch/powerpc/kvm/powerpc.c |    4 ++++
>  arch/s390/kvm/kvm-s390.c   |    4 ++++
>  arch/x86/kvm/x86.c         |    4 ++++
>  include/linux/kvm_host.h   |    1 +
>  virt/kvm/kvm_main.c        |    5 ++++-
>  8 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 327a1fb..51c3659 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -219,6 +219,10 @@ long kvm_arch_dev_ioctl(struct file *filp,
>  	return -EINVAL;
>  }
>  
> +void kvm_arch_memslots_updated(struct kvm *kvm)
> +{
> +}
> +
>  int kvm_arch_prepare_memory_region(struct kvm *kvm,
>  				   struct kvm_memory_slot *memslot,
>  				   struct kvm_userspace_memory_region *mem,
> diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
> index 5b2dc0d..bdfd878 100644
> --- a/arch/ia64/kvm/kvm-ia64.c
> +++ b/arch/ia64/kvm/kvm-ia64.c
> @@ -1560,6 +1560,10 @@ int kvm_arch_create_memslot(struct kvm_memory_slot *slot, unsigned long npages)
>  	return 0;
>  }
>  
> +void kvm_arch_memslots_updated(struct kvm *kvm)
> +{
> +}
> +
>  int kvm_arch_prepare_memory_region(struct kvm *kvm,
>  		struct kvm_memory_slot *memslot,
>  		struct kvm_userspace_memory_region *mem,
> diff --git a/arch/mips/kvm/kvm_mips.c b/arch/mips/kvm/kvm_mips.c
> index e0dad02..daa32ea 100644
> --- a/arch/mips/kvm/kvm_mips.c
> +++ b/arch/mips/kvm/kvm_mips.c
> @@ -208,6 +208,10 @@ int kvm_arch_create_memslot(struct kvm_memory_slot *slot, unsigned long npages)
>  	return 0;
>  }
>  
> +void kvm_arch_memslots_updated(struct kvm *kvm)
> +{
> +}
> +
>  int kvm_arch_prepare_memory_region(struct kvm *kvm,
>                                  struct kvm_memory_slot *memslot,
>                                  struct kvm_userspace_memory_region *mem,
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 6316ee3..ae63ae4 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -420,6 +420,10 @@ int kvm_arch_create_memslot(struct kvm_memory_slot *slot, unsigned long npages)
>  	return kvmppc_core_create_memslot(slot, npages);
>  }
>  
> +void kvm_arch_memslots_updated(struct kvm *kvm)
> +{
> +}
> +
>  int kvm_arch_prepare_memory_region(struct kvm *kvm,
>  				   struct kvm_memory_slot *memslot,
>  				   struct kvm_userspace_memory_region *mem,
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index ba694d2..a3d797b 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -1056,6 +1056,10 @@ int kvm_arch_create_memslot(struct kvm_memory_slot *slot, unsigned long npages)
>  	return 0;
>  }
>  
> +void kvm_arch_memslots_updated(struct kvm *kvm)
> +{
> +}
> +
>  /* Section: memory related */
>  int kvm_arch_prepare_memory_region(struct kvm *kvm,
>  				   struct kvm_memory_slot *memslot,
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 7d71c0f..71912c9 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -7020,6 +7020,10 @@ out_free:
>  	return -ENOMEM;
>  }
>  
> +void kvm_arch_memslots_updated(struct kvm *kvm)
> +{
> +}
> +
>  int kvm_arch_prepare_memory_region(struct kvm *kvm,
>  				struct kvm_memory_slot *memslot,
>  				struct kvm_userspace_memory_region *mem,
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index e3aae6d..1c1e9de 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -498,6 +498,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
>  void kvm_arch_free_memslot(struct kvm_memory_slot *free,
>  			   struct kvm_memory_slot *dont);
>  int kvm_arch_create_memslot(struct kvm_memory_slot *slot, unsigned long npages);
> +void kvm_arch_memslots_updated(struct kvm *kvm);
We can define empty function here like this:
#ifdef __KVM_HAVE_MEMSLOT_UPDATE
 void kvm_arch_memslots_updated(struct kvm *kvm);
#else
 static void kvm_arch_memslots_updated(struct kvm *kvm)
 {
 }
#endif

and make x86.c define __KVM_HAVE_MEMSLOT_UPDATE.

But I am fine with your approach too. Do other arch maintainers have any
preferences here?


>  int kvm_arch_prepare_memory_region(struct kvm *kvm,
>  				struct kvm_memory_slot *memslot,
>  				struct kvm_userspace_memory_region *mem,
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 1580dd4..4ed9d89 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -731,7 +731,10 @@ static struct kvm_memslots *install_new_memslots(struct kvm *kvm,
>  	update_memslots(slots, new, kvm->memslots->generation);
>  	rcu_assign_pointer(kvm->memslots, slots);
>  	synchronize_srcu_expedited(&kvm->srcu);
> -	return old_memslots; 
> +
> +	kvm_arch_memslots_updated(kvm);
> +
> +	return old_memslots;
>  }
>  
>  /*
> -- 
> 1.7.9.5

--
			Gleb.

Re: [PATCH 1/2] KVM: Introduce kvm_arch_memslots_updated()

[Alexander Graf]

On 04.07.2013, at 11:53, Gleb Natapov wrote:

> On Thu, Jul 04, 2013 at 01:40:29PM +0900, Takuya Yoshikawa wrote:
>> This is called right after the memslots is updated, i.e. when the result
>> of update_memslots() gets installed in install_new_memslots().  Since
>> the memslots needs to be updated twice when we delete or move a memslot,
>> kvm_arch_commit_memory_region() does not correspond to this exactly.
>> 
>> In the following patch, x86 will use this new API to check if the mmio
>> generation has reached its maximum value, in which case mmio sptes need
>> to be flushed out.
>> 
>> Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
>> ---
>> Removed the trailing space after "return old_memslots;" at this chance.
>> 
>> arch/arm/kvm/arm.c         |    4 ++++
>> arch/ia64/kvm/kvm-ia64.c   |    4 ++++
>> arch/mips/kvm/kvm_mips.c   |    4 ++++
>> arch/powerpc/kvm/powerpc.c |    4 ++++
>> arch/s390/kvm/kvm-s390.c   |    4 ++++
>> arch/x86/kvm/x86.c         |    4 ++++
>> include/linux/kvm_host.h   |    1 +
>> virt/kvm/kvm_main.c        |    5 ++++-
>> 8 files changed, 29 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index 327a1fb..51c3659 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -219,6 +219,10 @@ long kvm_arch_dev_ioctl(struct file *filp,
>> 	return -EINVAL;
>> }
>> 
>> +void kvm_arch_memslots_updated(struct kvm *kvm)
>> +{
>> +}
>> +
>> int kvm_arch_prepare_memory_region(struct kvm *kvm,
>> 				   struct kvm_memory_slot *memslot,
>> 				   struct kvm_userspace_memory_region *mem,
>> diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
>> index 5b2dc0d..bdfd878 100644
>> --- a/arch/ia64/kvm/kvm-ia64.c
>> +++ b/arch/ia64/kvm/kvm-ia64.c
>> @@ -1560,6 +1560,10 @@ int kvm_arch_create_memslot(struct kvm_memory_slot *slot, unsigned long npages)
>> 	return 0;
>> }
>> 
>> +void kvm_arch_memslots_updated(struct kvm *kvm)
>> +{
>> +}
>> +
>> int kvm_arch_prepare_memory_region(struct kvm *kvm,
>> 		struct kvm_memory_slot *memslot,
>> 		struct kvm_userspace_memory_region *mem,
>> diff --git a/arch/mips/kvm/kvm_mips.c b/arch/mips/kvm/kvm_mips.c
>> index e0dad02..daa32ea 100644
>> --- a/arch/mips/kvm/kvm_mips.c
>> +++ b/arch/mips/kvm/kvm_mips.c
>> @@ -208,6 +208,10 @@ int kvm_arch_create_memslot(struct kvm_memory_slot *slot, unsigned long npages)
>> 	return 0;
>> }
>> 
>> +void kvm_arch_memslots_updated(struct kvm *kvm)
>> +{
>> +}
>> +
>> int kvm_arch_prepare_memory_region(struct kvm *kvm,
>>                                 struct kvm_memory_slot *memslot,
>>                                 struct kvm_userspace_memory_region *mem,
>> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
>> index 6316ee3..ae63ae4 100644
>> --- a/arch/powerpc/kvm/powerpc.c
>> +++ b/arch/powerpc/kvm/powerpc.c
>> @@ -420,6 +420,10 @@ int kvm_arch_create_memslot(struct kvm_memory_slot *slot, unsigned long npages)
>> 	return kvmppc_core_create_memslot(slot, npages);
>> }
>> 
>> +void kvm_arch_memslots_updated(struct kvm *kvm)
>> +{
>> +}
>> +
>> int kvm_arch_prepare_memory_region(struct kvm *kvm,
>> 				   struct kvm_memory_slot *memslot,
>> 				   struct kvm_userspace_memory_region *mem,
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index ba694d2..a3d797b 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -1056,6 +1056,10 @@ int kvm_arch_create_memslot(struct kvm_memory_slot *slot, unsigned long npages)
>> 	return 0;
>> }
>> 
>> +void kvm_arch_memslots_updated(struct kvm *kvm)
>> +{
>> +}
>> +
>> /* Section: memory related */
>> int kvm_arch_prepare_memory_region(struct kvm *kvm,
>> 				   struct kvm_memory_slot *memslot,
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 7d71c0f..71912c9 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -7020,6 +7020,10 @@ out_free:
>> 	return -ENOMEM;
>> }
>> 
>> +void kvm_arch_memslots_updated(struct kvm *kvm)
>> +{
>> +}
>> +
>> int kvm_arch_prepare_memory_region(struct kvm *kvm,
>> 				struct kvm_memory_slot *memslot,
>> 				struct kvm_userspace_memory_region *mem,
>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> index e3aae6d..1c1e9de 100644
>> --- a/include/linux/kvm_host.h
>> +++ b/include/linux/kvm_host.h
>> @@ -498,6 +498,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
>> void kvm_arch_free_memslot(struct kvm_memory_slot *free,
>> 			   struct kvm_memory_slot *dont);
>> int kvm_arch_create_memslot(struct kvm_memory_slot *slot, unsigned long npages);
>> +void kvm_arch_memslots_updated(struct kvm *kvm);
> We can define empty function here like this:
> #ifdef __KVM_HAVE_MEMSLOT_UPDATE
> void kvm_arch_memslots_updated(struct kvm *kvm);
> #else
> static void kvm_arch_memslots_updated(struct kvm *kvm)
> {
> }
> #endif
> 
> and make x86.c define __KVM_HAVE_MEMSLOT_UPDATE.
> 
> But I am fine with your approach too. Do other arch maintainers have any
> preferences here?

I think this is common infrastructure, so the current approach is a better fit that anything conditional on __KVM_HAVE.

If we really want to make use of this on the PPC side, we'll have to move it into the respective host MMU source file, but I'd be more than happy to leave that as a future work item. So for now:

Acked-by: Alexander Graf <agraf@suse.de>


Alex

Re: [PATCH v2 2/2] KVM: x86: Avoid zapping mmio sptes twice for generation wraparound

[Xiao Guangrong]

On 07/04/2013 12:41 PM, Takuya Yoshikawa wrote:
> Now that kvm_arch_memslots_updated() catches every increment of the
> memslots->generation, checking if the mmio generation has reached its
> maximum value is enough.
> 

Reviewed-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>

Re: [PATCH 1/2] KVM: Introduce kvm_arch_memslots_updated()

[Xiao Guangrong]

On 07/04/2013 12:40 PM, Takuya Yoshikawa wrote:
> This is called right after the memslots is updated, i.e. when the result
> of update_memslots() gets installed in install_new_memslots().  Since
> the memslots needs to be updated twice when we delete or move a memslot,
> kvm_arch_commit_memory_region() does not correspond to this exactly.
> 
> In the following patch, x86 will use this new API to check if the mmio
> generation has reached its maximum value, in which case mmio sptes need
> to be flushed out.

Reviewed-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>

Re: [PATCH 1/2] KVM: Introduce kvm_arch_memslots_updated()

[Cornelia Huck]

On Thu, 4 Jul 2013 12:53:15 +0300
Gleb Natapov <gleb@redhat.com> wrote:

> On Thu, Jul 04, 2013 at 01:40:29PM +0900, Takuya Yoshikawa wrote:
> > This is called right after the memslots is updated, i.e. when the result
> > of update_memslots() gets installed in install_new_memslots().  Since
> > the memslots needs to be updated twice when we delete or move a memslot,
> > kvm_arch_commit_memory_region() does not correspond to this exactly.
> > 
> > In the following patch, x86 will use this new API to check if the mmio
> > generation has reached its maximum value, in which case mmio sptes need
> > to be flushed out.
> > 
> > Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
> > ---
> >  Removed the trailing space after "return old_memslots;" at this chance.
> > 
> >  arch/arm/kvm/arm.c         |    4 ++++
> >  arch/ia64/kvm/kvm-ia64.c   |    4 ++++
> >  arch/mips/kvm/kvm_mips.c   |    4 ++++
> >  arch/powerpc/kvm/powerpc.c |    4 ++++
> >  arch/s390/kvm/kvm-s390.c   |    4 ++++
> >  arch/x86/kvm/x86.c         |    4 ++++
> >  include/linux/kvm_host.h   |    1 +
> >  virt/kvm/kvm_main.c        |    5 ++++-
> >  8 files changed, 29 insertions(+), 1 deletion(-)

> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index e3aae6d..1c1e9de 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -498,6 +498,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
> >  void kvm_arch_free_memslot(struct kvm_memory_slot *free,
> >  			   struct kvm_memory_slot *dont);
> >  int kvm_arch_create_memslot(struct kvm_memory_slot *slot, unsigned long npages);
> > +void kvm_arch_memslots_updated(struct kvm *kvm);
> We can define empty function here like this:
> #ifdef __KVM_HAVE_MEMSLOT_UPDATE
>  void kvm_arch_memslots_updated(struct kvm *kvm);
> #else
>  static void kvm_arch_memslots_updated(struct kvm *kvm)
>  {
>  }
> #endif
> 
> and make x86.c define __KVM_HAVE_MEMSLOT_UPDATE.
> 
> But I am fine with your approach too. Do other arch maintainers have any
> preferences here?

I don't really have a strong preference either way, so

Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>

for the current approach.