Re: [PATCH v3 05/15] KVM: Drop kvm_arch_create_memslot()

[Christian Borntraeger]

On 25.10.19 01:07, Sean Christopherson wrote:
> Remove kvm_arch_create_memslot() now that all arch implementations are
> effectively nops.  Removing kvm_arch_create_memslot() eliminates the
> possibility for arch specific code to allocate memory prior to setting
> a memslot, which sets the stage for simplifying kvm_free_memslot().
> 
> Cc: Janosch Frank <frankja@xxxxxxxxxxxxx>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>

Acked-by: Christian Borntraeger <borntraeger@xxxxxxxxxx>


> ---
>  arch/mips/kvm/mips.c       |  6 ------
>  arch/powerpc/kvm/powerpc.c |  6 ------
>  arch/s390/kvm/kvm-s390.c   |  6 ------
>  arch/x86/kvm/x86.c         |  6 ------
>  include/linux/kvm_host.h   |  2 --
>  virt/kvm/arm/mmu.c         |  6 ------
>  virt/kvm/kvm_main.c        | 21 +++++++--------------
>  7 files changed, 7 insertions(+), 46 deletions(-)
> 
> diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c
> index 1109924560d8..713e5465edb0 100644
> --- a/arch/mips/kvm/mips.c
> +++ b/arch/mips/kvm/mips.c
> @@ -188,12 +188,6 @@ long kvm_arch_dev_ioctl(struct file *filp, unsigned int ioctl,
>  	return -ENOIOCTLCMD;
>  }
>  
> -int kvm_arch_create_memslot(struct kvm *kvm, struct kvm_memory_slot *slot,
> -			    unsigned long npages)
> -{
> -	return 0;
> -}
> -
>  void kvm_arch_flush_shadow_all(struct kvm *kvm)
>  {
>  	/* Flush whole GPA */
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 95b4a766b6cb..bb427543f8a3 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -687,12 +687,6 @@ void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *free,
>  	kvmppc_core_free_memslot(kvm, free, dont);
>  }
>  
> -int kvm_arch_create_memslot(struct kvm *kvm, struct kvm_memory_slot *slot,
> -			    unsigned long npages)
> -{
> -	return 0;
> -}
> -
>  int kvm_arch_prepare_memory_region(struct kvm *kvm,
>  				   struct kvm_memory_slot *memslot,
>  				   const struct kvm_userspace_memory_region *mem,
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index d047e846e1b9..21f61dae7db1 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -4494,12 +4494,6 @@ vm_fault_t kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
>  	return VM_FAULT_SIGBUS;
>  }
>  
> -int kvm_arch_create_memslot(struct kvm *kvm, struct kvm_memory_slot *slot,
> -			    unsigned long npages)
> -{
> -	return 0;
> -}
> -
>  /* 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 fc63b1f07ba9..aed1a4c5c999 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9626,12 +9626,6 @@ void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *free,
>  	kvm_page_track_free_memslot(free, dont);
>  }
>  
> -int kvm_arch_create_memslot(struct kvm *kvm, struct kvm_memory_slot *slot,
> -			    unsigned long npages)
> -{
> -	return 0;
> -}
> -
>  static int kvm_create_memslot(struct kvm *kvm, struct kvm_memory_slot *slot,
>  			      unsigned long npages)
>  {
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index a817e446c9aa..273fa081ae91 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -679,8 +679,6 @@ int __kvm_set_memory_region(struct kvm *kvm,
>  			    const struct kvm_userspace_memory_region *mem);
>  void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *free,
>  			   struct kvm_memory_slot *dont);
> -int kvm_arch_create_memslot(struct kvm *kvm, struct kvm_memory_slot *slot,
> -			    unsigned long npages);
>  void kvm_arch_memslots_updated(struct kvm *kvm, u64 gen);
>  int kvm_arch_prepare_memory_region(struct kvm *kvm,
>  				struct kvm_memory_slot *memslot,
> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
> index 38b4c910b6c3..f264de85f648 100644
> --- a/virt/kvm/arm/mmu.c
> +++ b/virt/kvm/arm/mmu.c
> @@ -2358,12 +2358,6 @@ void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *free,
>  {
>  }
>  
> -int kvm_arch_create_memslot(struct kvm *kvm, struct kvm_memory_slot *slot,
> -			    unsigned long npages)
> -{
> -	return 0;
> -}
> -
>  void kvm_arch_memslots_updated(struct kvm *kvm, u64 gen)
>  {
>  }
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 2d354fa11934..9d6af9044304 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -977,12 +977,13 @@ int __kvm_set_memory_region(struct kvm *kvm,
>  	new.base_gfn = base_gfn;
>  	new.npages = npages;
>  	new.flags = mem->flags;
> +	new.userspace_addr = mem->userspace_addr;
>  
>  	if (npages) {
>  		if (!old.npages)
>  			change = KVM_MR_CREATE;
>  		else { /* Modify an existing slot. */
> -			if ((mem->userspace_addr != old.userspace_addr) ||
> +			if ((new.userspace_addr != old.userspace_addr) ||
>  			    (npages != old.npages) ||
>  			    ((new.flags ^ old.flags) & KVM_MEM_READONLY))
>  				goto out;
> @@ -1017,22 +1018,14 @@ int __kvm_set_memory_region(struct kvm *kvm,
>  		}
>  	}
>  
> -	/* Free page dirty bitmap if unneeded */
> +	r = -ENOMEM;
> +
> +	/* Allocate/free page dirty bitmap as needed */
>  	if (!(new.flags & KVM_MEM_LOG_DIRTY_PAGES))
>  		new.dirty_bitmap = NULL;
> -
> -	r = -ENOMEM;
> -	if (change == KVM_MR_CREATE) {
> -		new.userspace_addr = mem->userspace_addr;
> -
> -		if (kvm_arch_create_memslot(kvm, &new, npages))
> -			goto out;
> -	}
> -
> -	/* Allocate page dirty bitmap if needed */
> -	if ((new.flags & KVM_MEM_LOG_DIRTY_PAGES) && !new.dirty_bitmap) {
> +	else if (!new.dirty_bitmap) {
>  		if (kvm_create_dirty_bitmap(&new) < 0)
> -			goto out_free;
> +			goto out;
>  	}
>  
>  	slots = kvzalloc(sizeof(struct kvm_memslots), GFP_KERNEL_ACCOUNT);
>