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

[Gleb Natapov <gleb@redhat.com>]

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.