Re: [PATCH v3 14/16] KVM: Move vcpu_load to arch-specific kvm_arch_vcpu_ioctl

[Cornelia Huck <cohuck@redhat.com>]

On Mon,  4 Dec 2017 21:35:36 +0100
Christoffer Dall <cdall@kernel.org> wrote:

> From: Christoffer Dall <christoffer.dall@linaro.org>
> 
> Move the calls to vcpu_load() and vcpu_put() in to the architecture
> specific implementations of kvm_arch_vcpu_ioctl() which dispatches
> further architecture-specific ioctls on to other functions.
> 
> Some architectures support asynchronous vcpu ioctls which cannot call
> vcpu_load() or take the vcpu->mutex, because that would prevent
> concurrent execution with a running VCPU, which is the intended purpose
> of these ioctls, for example because they inject interrupts.
> 
> We repeat the separate checks for these specifics in the architecture
> code for MIPS, S390 and PPC, and avoid taking the vcpu->mutex and
> calling vcpu_load for these ioctls.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  arch/mips/kvm/mips.c       | 49 +++++++++++++++++++++++----------------
>  arch/powerpc/kvm/powerpc.c | 13 ++++++-----
>  arch/s390/kvm/kvm-s390.c   | 19 ++++++++-------
>  arch/x86/kvm/x86.c         | 22 +++++++++++++-----
>  virt/kvm/arm/arm.c         | 58 ++++++++++++++++++++++++++++++++--------------
>  virt/kvm/kvm_main.c        |  2 --
>  6 files changed, 103 insertions(+), 60 deletions(-)
> 
> diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c
> index 3a898712d6cd..4a039341dc29 100644
> --- a/arch/mips/kvm/mips.c
> +++ b/arch/mips/kvm/mips.c
> @@ -913,56 +913,65 @@ long kvm_arch_vcpu_ioctl(struct file *filp, unsigned int ioctl,
>  	void __user *argp = (void __user *)arg;
>  	long r;
>  
> +	if (ioctl == KVM_INTERRUPT) {

I would add a comment here that this ioctl is async to vcpu execution,
so it is understandable why you skip the vcpu_load().

[As an aside, it is nice that this is now more obvious when looking at
the architectures' handlers.]

> +		struct kvm_mips_interrupt irq;
> +
> +		if (copy_from_user(&irq, argp, sizeof(irq)))
> +			return -EFAULT;
> +		kvm_debug("[%d] %s: irq: %d\n", vcpu->vcpu_id, __func__,
> +			  irq.irq);
> +
> +		return kvm_vcpu_ioctl_interrupt(vcpu, &irq);
> +	}
> +
> +	vcpu_load(vcpu);
> +
>  	switch (ioctl) {

(...)

> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index c06bc9552438..6b5dd3a25fe8 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -1617,16 +1617,16 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>  	void __user *argp = (void __user *)arg;
>  	long r;
>  
> -	switch (ioctl) {
> -	case KVM_INTERRUPT: {
> +	if (ioctl == KVM_INTERRUPT) {

Same here.

>  		struct kvm_interrupt irq;
> -		r = -EFAULT;
>  		if (copy_from_user(&irq, argp, sizeof(irq)))
> -			goto out;
> -		r = kvm_vcpu_ioctl_interrupt(vcpu, &irq);
> -		goto out;
> +			return -EFAULT;
> +		return kvm_vcpu_ioctl_interrupt(vcpu, &irq);
>  	}
>  
> +	vcpu_load(vcpu);
> +
> +	switch (ioctl) {
>  	case KVM_ENABLE_CAP:
>  	{
>  		struct kvm_enable_cap cap;
> @@ -1666,6 +1666,7 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>  	}
>  
>  out:
> +	vcpu_put(vcpu);
>  	return r;
>  }
>  
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 43278f334ce3..cd067b63d77f 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -3743,24 +3743,25 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>  	case KVM_S390_IRQ: {
>  		struct kvm_s390_irq s390irq;
>  
> -		r = -EFAULT;
>  		if (copy_from_user(&s390irq, argp, sizeof(s390irq)))
> -			break;
> -		r = kvm_s390_inject_vcpu(vcpu, &s390irq);
> -		break;
> +			return -EFAULT;
> +		return kvm_s390_inject_vcpu(vcpu, &s390irq);
>  	}
>  	case KVM_S390_INTERRUPT: {
>  		struct kvm_s390_interrupt s390int;
>  		struct kvm_s390_irq s390irq;
>  
> -		r = -EFAULT;
>  		if (copy_from_user(&s390int, argp, sizeof(s390int)))
> -			break;
> +			return -EFAULT;
>  		if (s390int_to_s390irq(&s390int, &s390irq))
>  			return -EINVAL;
> -		r = kvm_s390_inject_vcpu(vcpu, &s390irq);
> -		break;
> +		return kvm_s390_inject_vcpu(vcpu, &s390irq);
>  	}
> +	}

I find the special casing with the immediate return a bit ugly. Maybe
introduce a helper async_vcpu_ioctl() or so that sets -ENOIOCTLCMD in
its default case and return here if ret != -ENOIOCTLCMD? Christian,
what do you think?

> +
> +	vcpu_load(vcpu);
> +
> +	switch (ioctl) {
>  	case KVM_S390_STORE_STATUS:
>  		idx = srcu_read_lock(&vcpu->kvm->srcu);
>  		r = kvm_s390_vcpu_store_status(vcpu, arg);
> @@ -3883,6 +3884,8 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>  	default:
>  		r = -ENOTTY;
>  	}
> +
> +	vcpu_put(vcpu);
>  	return r;
>  }
>  

(...)

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 06751bbecd58..ad5f83159a15 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2693,9 +2693,7 @@ static long kvm_vcpu_ioctl(struct file *filp,
>  		break;
>  	}
>  	default:
> -		vcpu_load(vcpu);
>  		r = kvm_arch_vcpu_ioctl(filp, ioctl, arg);
> -		vcpu_put(vcpu);
>  	}
>  out:
>  	mutex_unlock(&vcpu->mutex);

It would be nice if we could get rid of the special casing at the
beginning of this function, but as it would involve not taking the
mutex for special cases (and not releasing it for those special cases),
I don't see an elegant way to do that.