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 <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Christian Borntraeger <[email protected]>
Suggested-by: Cornelia Huck <[email protected]>
Signed-off-by: Paolo Bonzini <[email protected]>
---
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
>
> 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!
--
Thanks,
David / dhildenb
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 ++
:)
--
Thanks,
David / dhildenb
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 <[email protected]>
> Cc: Paul Mackerras <[email protected]>
> Cc: Christian Borntraeger <[email protected]>
> Suggested-by: Cornelia Huck <[email protected]>
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> 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 <[email protected]>
(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 <[email protected]>
> Cc: Paul Mackerras <[email protected]>
> Cc: Christian Borntraeger <[email protected]>
looks sane.
> Suggested-by: Cornelia Huck <[email protected]>
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]>
> Cc: Paul Mackerras <[email protected]>
> Cc: Christian Borntraeger <[email protected]>
> Suggested-by: Cornelia Huck <[email protected]>
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> 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 <[email protected]>
On 12/12/2017 19:25, Christoffer Dall wrote:
> Reviewed-by: Christoffer Dall <[email protected]>
>
> (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