Message ID | 1513099915-107126-1-git-send-email-pbonzini@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
> > 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) are you mixing spaces and tabs? > { > 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) dito > +{ > + 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) can we now also get rid of this ugly define? > /* > * 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 > > - unrelated change but the right thing to do :) > if (mutex_lock_killable(&vcpu->mutex)) > return -EINTR; > switch (ioctl) { > Looks good to me!
On 12/12/2017 18:47, David Hildenbrand wrote: >> @@ -2547,13 +2547,13 @@ static long kvm_vcpu_ioctl(struct file *filp, >> #if defined(CONFIG_S390) || defined(CONFIG_PPC) || defined(CONFIG_MIPS) > can we now also get rid of this ugly define? Not yet, I'd have to add the function to x86 and ARM, or a dummy inline. Paolo >> /* >> * 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 >> >> - > unrelated change but the right thing to do :) >
On 12.12.2017 18:48, Paolo Bonzini wrote: > On 12/12/2017 18:47, David Hildenbrand wrote: >>> @@ -2547,13 +2547,13 @@ static long kvm_vcpu_ioctl(struct file *filp, >>> #if defined(CONFIG_S390) || defined(CONFIG_PPC) || defined(CONFIG_MIPS) >> can we now also get rid of this ugly define? > > Not yet, I'd have to add the function to x86 and ARM, or a dummy inline. > > Paolo dummy inline ++ :)
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@kernel.org> > Cc: Paul Mackerras <paulus@ozlabs.org> > Cc: Christian Borntraeger <borntraeger@de.ibm.com> > Suggested-by: Cornelia Huck <cohuck@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > 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@linaro.org> (although it'd be great if we can introduce the dummy inline to get rid of the #ifdef'ery as well). Thanks, -Christoffer
On 12/12/2017 06:31 PM, 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@kernel.org> > Cc: Paul Mackerras <paulus@ozlabs.org> > Cc: Christian Borntraeger <borntraeger@de.ibm.com> looks sane. > Suggested-by: Cornelia Huck <cohuck@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > 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(-) >
On Tue, 12 Dec 2017 18:31:55 +0100 Paolo Bonzini <pbonzini@redhat.com> 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@kernel.org> > Cc: Paul Mackerras <paulus@ozlabs.org> > Cc: Christian Borntraeger <borntraeger@de.ibm.com> > Suggested-by: Cornelia Huck <cohuck@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > 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(-) Another vote for a dummy function here, but otherwise Reviewed-by: Cornelia Huck <cohuck@redhat.com>
On 12/12/2017 19:25, Christoffer Dall wrote: > Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org> > > (although it'd be great if we can introduce the dummy inline to get rid > of the #ifdef'ery as well). Well, it's ARM and x86 that are the culprit so it's up to us. :) I'll add an #ifdef HAVE_KVM_VCPU_ASYNC_IOCTL to include/linux/kvm_host.h. Thanks, Paolo
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) {
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@kernel.org> Cc: Paul Mackerras <paulus@ozlabs.org> Cc: Christian Borntraeger <borntraeger@de.ibm.com> Suggested-by: Cornelia Huck <cohuck@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- 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(-)