Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752501AbdLLSZT (ORCPT ); Tue, 12 Dec 2017 13:25:19 -0500 Received: from mail-wm0-f66.google.com ([74.125.82.66]:35166 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751688AbdLLSZP (ORCPT ); Tue, 12 Dec 2017 13:25:15 -0500 X-Google-Smtp-Source: ACJfBosMthk39VJX0bmwkX5hf+J4etCJ62mICFih3d3+a2F0zeWlSEO6dVDk+dEXICun7Ha7yl3GPA== Date: Tue, 12 Dec 2017 19:25:09 +0100 From: Christoffer Dall To: Paolo Bonzini Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, cohuck@redhat.com, James Hogan , Paul Mackerras , Christian Borntraeger Subject: Re: [PATCH] KVM: introduce kvm_arch_vcpu_async_ioctl Message-ID: <20171212182509.GO910@cbox> References: <1513099915-107126-1-git-send-email-pbonzini@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1513099915-107126-1-git-send-email-pbonzini@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5938 Lines: 177 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 > Cc: Paul Mackerras > Cc: Christian Borntraeger > Suggested-by: Cornelia Huck > Signed-off-by: Paolo Bonzini > --- > 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 (although it'd be great if we can introduce the dummy inline to get rid of the #ifdef'ery as well). Thanks, -Christoffer