On Mon, Dec 11, 2017 at 02:12:41PM +0100, Cornelia Huck wrote: > On Mon, 4 Dec 2017 21:35:36 +0100 > Christoffer Dall <cdall@xxxxxxxxxx> wrote: > > > From: Christoffer Dall <christoffer.dall@xxxxxxxxxx> > > > > 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@xxxxxxxxxx> > > --- > > 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(). Yes, that would be appropriate. > > [As an aside, it is nice that this is now more obvious when looking at > the architectures' handlers.] > Agreed. > > + 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. I would also have liked that, and that's essentially what I had in the first version, but Paolo thought the result was too high an increase in complexity in the architecture-specfic functions throughout. I don't have any better suggestions either. Thanks, -Christoffer -- To unsubscribe from this list: send the line "unsubscribe linux-s390" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html