Re: [PATCH] KVM: introduce kvm_arch_vcpu_async_ioctl

[Christoffer Dall]

On Tue, Dec 12, 2017 at 06:31:55PM +0100, Paolo Bonzini wrote:
> After the vcpu_load/vcpu_put pushdown, the handling of asynchronous VCPU
> ioctl is already much clearer in that it is obvious that they bypass
> vcpu_load and vcpu_put.
> 
> However, it is still not perfect in that the different state of the VCPU
> mutex is still hidden in the caller.  Separate those ioctls into a new
> function kvm_arch_vcpu_async_ioctl that returns -ENOIOCTLCMD for more
> "traditional" synchronous ioctls.
> 
> Cc: James Hogan <jhogan@xxxxxxxxxx>
> Cc: Paul Mackerras <paulus@xxxxxxxxxx>
> Cc: Christian Borntraeger <borntraeger@xxxxxxxxxx>
> Suggested-by: Cornelia Huck <cohuck@xxxxxxxxxx>
> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> ---
>  arch/mips/kvm/mips.c       | 15 ++++++++++++---
>  arch/powerpc/kvm/powerpc.c | 14 +++++++++++---
>  arch/s390/kvm/kvm-s390.c   | 16 ++++++++++++----
>  include/linux/kvm_host.h   |  2 ++
>  virt/kvm/kvm_main.c        |  8 ++++----
>  5 files changed, 41 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c
> index 9200b3def440..2549fdd27ee1 100644
> --- a/arch/mips/kvm/mips.c
> +++ b/arch/mips/kvm/mips.c
> @@ -903,12 +903,11 @@ static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
>  	return r;
>  }
>  
> -long kvm_arch_vcpu_ioctl(struct file *filp, unsigned int ioctl,
> -			 unsigned long arg)
> +long kvm_arch_vcpu_async_ioctl(struct file *filp, unsigned int ioctl,
> +			       unsigned long arg)
>  {
>  	struct kvm_vcpu *vcpu = filp->private_data;
>  	void __user *argp = (void __user *)arg;
> -	long r;
>  
>  	if (ioctl == KVM_INTERRUPT) {
>  		struct kvm_mips_interrupt irq;
> @@ -921,6 +920,16 @@ long kvm_arch_vcpu_ioctl(struct file *filp, unsigned int ioctl,
>  		return kvm_vcpu_ioctl_interrupt(vcpu, &irq);
>  	}
>  
> +	return -ENOIOCTLCMD;
> +}
> +
> +long kvm_arch_vcpu_ioctl(struct file *filp, unsigned int ioctl,
> +			 unsigned long arg)
> +{
> +	struct kvm_vcpu *vcpu = filp->private_data;
> +	void __user *argp = (void __user *)arg;
> +	long r;
> +
>  	vcpu_load(vcpu);
>  
>  	switch (ioctl) {
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index ba8134a989c1..2e700753e35c 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -1607,12 +1607,11 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
>  	return -EINVAL;
>  }
>  
> -long kvm_arch_vcpu_ioctl(struct file *filp,
> -                         unsigned int ioctl, unsigned long arg)
> +long kvm_arch_vcpu_async_ioctl(struct file *filp,
> +      			       unsigned int ioctl, unsigned long arg)
>  {
>  	struct kvm_vcpu *vcpu = filp->private_data;
>  	void __user *argp = (void __user *)arg;
> -	long r;
>  
>  	if (ioctl == KVM_INTERRUPT) {
>  		struct kvm_interrupt irq;
> @@ -1620,6 +1619,15 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>  			return -EFAULT;
>  		return kvm_vcpu_ioctl_interrupt(vcpu, &irq);
>  	}
> +	return -ENOIOCTLCMD;
> +}
> +
> +long kvm_arch_vcpu_ioctl(struct file *filp,
> +                         unsigned int ioctl, unsigned long arg)
> +{
> +	struct kvm_vcpu *vcpu = filp->private_data;
> +	void __user *argp = (void __user *)arg;
> +	long r;
>  
>  	vcpu_load(vcpu);
>  
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 9700d71cb691..40f0ae5a883f 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -3725,13 +3725,11 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,
>  	return r;
>  }
>  
> -long kvm_arch_vcpu_ioctl(struct file *filp,
> -			 unsigned int ioctl, unsigned long arg)
> +long kvm_arch_vcpu_async_ioctl(struct file *filp,
> +			       unsigned int ioctl, unsigned long arg)
>  {
>  	struct kvm_vcpu *vcpu = filp->private_data;
>  	void __user *argp = (void __user *)arg;
> -	int idx;
> -	long r;
>  
>  	switch (ioctl) {
>  	case KVM_S390_IRQ: {
> @@ -3752,6 +3750,16 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>  		return kvm_s390_inject_vcpu(vcpu, &s390irq);
>  	}
>  	}
> +	return -ENOIOCTLCMD;
> +}
> +
> +long kvm_arch_vcpu_ioctl(struct file *filp,
> +			 unsigned int ioctl, unsigned long arg)
> +{
> +	struct kvm_vcpu *vcpu = filp->private_data;
> +	void __user *argp = (void __user *)arg;
> +	int idx;
> +	long r;
>  
>  	vcpu_load(vcpu);
>  
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 09de0ff3d677..a900d20a5320 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -736,6 +736,8 @@ long kvm_arch_dev_ioctl(struct file *filp,
>  			unsigned int ioctl, unsigned long arg);
>  long kvm_arch_vcpu_ioctl(struct file *filp,
>  			 unsigned int ioctl, unsigned long arg);
> +long kvm_arch_vcpu_async_ioctl(struct file *filp,
> +			       unsigned int ioctl, unsigned long arg);
>  int kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf);
>  
>  int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext);
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 19c184fa1839..262d6c910fe7 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2547,13 +2547,13 @@ static long kvm_vcpu_ioctl(struct file *filp,
>  #if defined(CONFIG_S390) || defined(CONFIG_PPC) || defined(CONFIG_MIPS)
>  	/*
>  	 * Special cases: vcpu ioctls that are asynchronous to vcpu execution,
> -	 * so vcpu_load() would break it.
> +	 * so mutex_lock() would break it.
>  	 */
> -	if (ioctl == KVM_S390_INTERRUPT || ioctl == KVM_S390_IRQ || ioctl == KVM_INTERRUPT)
> -		return kvm_arch_vcpu_ioctl(filp, ioctl, arg);
> +	r = kvm_arch_vcpu_async_ioctl(filp, ioctl, arg);
> +	if (r != -ENOIOCTLCMD)
> +		return r;
>  #endif
>  
> -
>  	if (mutex_lock_killable(&vcpu->mutex))
>  		return -EINTR;
>  	switch (ioctl) {
> -- 
> 1.8.3.1
> 

Reviewed-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>

(although it'd be great if we can introduce the dummy inline to get rid
of the #ifdef'ery as well).

Thanks,
-Christoffer