From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?C=c3=a9dric_Le_Goater?= Subject: Re: [PATCH v2 15/16] KVM: introduce a KVM_DESTROY_DEVICE ioctl Date: Wed, 13 Mar 2019 09:02:09 +0100 Message-ID: References: <20190222112840.25000-1-clg@kaod.org> <20190222112840.25000-16-clg@kaod.org> <20190225041507.GS7668@umbus.fritz.box> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Cc: kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, Paul Mackerras , Paolo Bonzini , linuxppc-dev@lists.ozlabs.org To: David Gibson Return-path: In-Reply-To: <20190225041507.GS7668@umbus.fritz.box> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: linuxppc-dev-bounces+glppe-linuxppc-embedded-2=m.gmane.org@lists.ozlabs.org Sender: "Linuxppc-dev" List-Id: kvm.vger.kernel.org On 2/25/19 5:15 AM, David Gibson wrote: > On Fri, Feb 22, 2019 at 12:28:39PM +0100, Cédric Le Goater wrote: >> The 'destroy' method is currently used to destroy all devices when the >> VM is destroyed after the vCPUs have been freed. >> >> This new KVM ioctl exposes the same KVM device method. It acts as a >> software reset of the VM to 'destroy' selected devices when necessary >> and perform the required cleanups on the vCPUs. Called with the >> kvm->lock. >> >> The 'destroy' method could be improved by returning an error code. >> >> Signed-off-by: Cédric Le Goater > > Again, has this been discussed with Paolo and/or other KVM core > people? Not yet. Adding Paolo for feedback. >> --- >> include/uapi/linux/kvm.h | 7 ++++++ >> virt/kvm/kvm_main.c | 38 +++++++++++++++++++++++++++++++ >> Documentation/virtual/kvm/api.txt | 19 ++++++++++++++++ >> 3 files changed, 64 insertions(+) >> >> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h >> index 52bf74a1616e..d78fafa54274 100644 >> --- a/include/uapi/linux/kvm.h >> +++ b/include/uapi/linux/kvm.h >> @@ -1183,6 +1183,11 @@ struct kvm_create_device { >> __u32 flags; /* in: KVM_CREATE_DEVICE_xxx */ >> }; >> >> +struct kvm_destroy_device { >> + __u32 fd; /* in: device handle */ >> + __u32 flags; /* in: unused */ >> +}; >> + >> struct kvm_device_attr { >> __u32 flags; /* no flags currently defined */ >> __u32 group; /* device-defined */ >> @@ -1331,6 +1336,8 @@ struct kvm_s390_ucas_mapping { >> #define KVM_GET_DEVICE_ATTR _IOW(KVMIO, 0xe2, struct kvm_device_attr) >> #define KVM_HAS_DEVICE_ATTR _IOW(KVMIO, 0xe3, struct kvm_device_attr) >> >> +#define KVM_DESTROY_DEVICE _IOWR(KVMIO, 0xf0, struct kvm_destroy_device) >> + >> /* >> * ioctls for vcpu fds >> */ >> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c >> index 84717d8cb5e4..983d5c37f710 100644 >> --- a/virt/kvm/kvm_main.c >> +++ b/virt/kvm/kvm_main.c >> @@ -3026,6 +3026,30 @@ static int kvm_ioctl_create_device(struct kvm *kvm, >> return 0; >> } >> >> +static int kvm_ioctl_destroy_device(struct kvm *kvm, >> + struct kvm_destroy_device *dd) >> +{ >> + struct fd f; >> + struct kvm_device *dev; >> + >> + f = fdget(dd->fd); >> + if (!f.file) >> + return -EBADF; >> + >> + dev = kvm_device_from_filp(f.file); >> + fdput(f); >> + >> + if (!dev) >> + return -ENODEV; > > Don't you need to verify that the device belongs to this KVM instance? ah yes. I should at least check : dev->kvm == kvm >> + mutex_lock(&kvm->lock); >> + list_del(&dev->vm_node); >> + dev->ops->destroy(dev); >> + mutex_unlock(&kvm->lock); And there, I should problably drop a ref count on the VM. I am not sure of that. Thanks, C. >> + return 0; >> +} >> + >> static long kvm_vm_ioctl_check_extension_generic(struct kvm *kvm, long arg) >> { >> switch (arg) { >> @@ -3270,6 +3294,20 @@ static long kvm_vm_ioctl(struct file *filp, >> r = 0; >> break; >> } >> + case KVM_DESTROY_DEVICE: { >> + struct kvm_destroy_device dd; >> + >> + r = -EFAULT; >> + if (copy_from_user(&dd, argp, sizeof(dd))) >> + goto out; >> + >> + r = kvm_ioctl_destroy_device(kvm, &dd); >> + if (r) >> + goto out; >> + >> + r = 0; >> + break; >> + } >> case KVM_CHECK_EXTENSION: >> r = kvm_vm_ioctl_check_extension_generic(kvm, arg); >> break; >> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt >> index 1db1435769b4..c2ba18279298 100644 >> --- a/Documentation/virtual/kvm/api.txt >> +++ b/Documentation/virtual/kvm/api.txt >> @@ -3857,6 +3857,25 @@ number of valid entries in the 'entries' array, which is then filled. >> 'index' and 'flags' fields in 'struct kvm_cpuid_entry2' are currently reserved, >> userspace should not expect to get any particular value there. >> >> +4.119 KVM_DESTROY_DEVICE >> + >> +Capability: KVM_CAP_DEVICE_CTRL >> +Type: vm ioctl >> +Parameters: struct kvm_destroy_device (in) >> +Returns: 0 on success, -1 on error >> +Errors: >> + ENODEV: The device type is unknown or unsupported >> + >> + Other error conditions may be defined by individual device types or >> + have their standard meanings. >> + >> +Destroys an emulated device in the kernel. >> + >> +struct kvm_destroy_device { >> + __u32 fd; /* in: device handle */ >> + __u32 flags; /* unused */ >> +}; >> + >> 5. The kvm_run structure >> ------------------------ >> > From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?C=c3=a9dric_Le_Goater?= Date: Wed, 13 Mar 2019 08:02:09 +0000 Subject: Re: [PATCH v2 15/16] KVM: introduce a KVM_DESTROY_DEVICE ioctl Message-Id: List-Id: References: <20190222112840.25000-1-clg@kaod.org> <20190222112840.25000-16-clg@kaod.org> <20190225041507.GS7668@umbus.fritz.box> In-Reply-To: <20190225041507.GS7668@umbus.fritz.box> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable To: David Gibson Cc: kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, Paul Mackerras , Paolo Bonzini , linuxppc-dev@lists.ozlabs.org On 2/25/19 5:15 AM, David Gibson wrote: > On Fri, Feb 22, 2019 at 12:28:39PM +0100, C=E9dric Le Goater wrote: >> The 'destroy' method is currently used to destroy all devices when the >> VM is destroyed after the vCPUs have been freed. >> >> This new KVM ioctl exposes the same KVM device method. It acts as a >> software reset of the VM to 'destroy' selected devices when necessary >> and perform the required cleanups on the vCPUs. Called with the >> kvm->lock. >> >> The 'destroy' method could be improved by returning an error code. >> >> Signed-off-by: C=E9dric Le Goater >=20 > Again, has this been discussed with Paolo and/or other KVM core > people? Not yet. Adding Paolo for feedback.=20 >> --- >> include/uapi/linux/kvm.h | 7 ++++++ >> virt/kvm/kvm_main.c | 38 +++++++++++++++++++++++++++++++ >> Documentation/virtual/kvm/api.txt | 19 ++++++++++++++++ >> 3 files changed, 64 insertions(+) >> >> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h >> index 52bf74a1616e..d78fafa54274 100644 >> --- a/include/uapi/linux/kvm.h >> +++ b/include/uapi/linux/kvm.h >> @@ -1183,6 +1183,11 @@ struct kvm_create_device { >> __u32 flags; /* in: KVM_CREATE_DEVICE_xxx */ >> }; >> =20 >> +struct kvm_destroy_device { >> + __u32 fd; /* in: device handle */ >> + __u32 flags; /* in: unused */ >> +}; >> + >> struct kvm_device_attr { >> __u32 flags; /* no flags currently defined */ >> __u32 group; /* device-defined */ >> @@ -1331,6 +1336,8 @@ struct kvm_s390_ucas_mapping { >> #define KVM_GET_DEVICE_ATTR _IOW(KVMIO, 0xe2, struct kvm_device_attr) >> #define KVM_HAS_DEVICE_ATTR _IOW(KVMIO, 0xe3, struct kvm_device_attr) >> =20 >> +#define KVM_DESTROY_DEVICE _IOWR(KVMIO, 0xf0, struct kvm_destroy_dev= ice) >> + >> /* >> * ioctls for vcpu fds >> */ >> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c >> index 84717d8cb5e4..983d5c37f710 100644 >> --- a/virt/kvm/kvm_main.c >> +++ b/virt/kvm/kvm_main.c >> @@ -3026,6 +3026,30 @@ static int kvm_ioctl_create_device(struct kvm *kv= m, >> return 0; >> } >> =20 >> +static int kvm_ioctl_destroy_device(struct kvm *kvm, >> + struct kvm_destroy_device *dd) >> +{ >> + struct fd f; >> + struct kvm_device *dev; >> + >> + f =3D fdget(dd->fd); >> + if (!f.file) >> + return -EBADF; >> + >> + dev =3D kvm_device_from_filp(f.file); >> + fdput(f); >> + >> + if (!dev) >> + return -ENODEV; >=20 > Don't you need to verify that the device belongs to this KVM instance? ah yes. I should at least check : dev->kvm =3D kvm >> + mutex_lock(&kvm->lock); >> + list_del(&dev->vm_node); >> + dev->ops->destroy(dev); >> + mutex_unlock(&kvm->lock); And there, I should problably drop a ref count on the VM. I am not sure=20 of that. Thanks, C. >> + return 0; >> +} >> + >> static long kvm_vm_ioctl_check_extension_generic(struct kvm *kvm, long = arg) >> { >> switch (arg) { >> @@ -3270,6 +3294,20 @@ static long kvm_vm_ioctl(struct file *filp, >> r =3D 0; >> break; >> } >> + case KVM_DESTROY_DEVICE: { >> + struct kvm_destroy_device dd; >> + >> + r =3D -EFAULT; >> + if (copy_from_user(&dd, argp, sizeof(dd))) >> + goto out; >> + >> + r =3D kvm_ioctl_destroy_device(kvm, &dd); >> + if (r) >> + goto out; >> + >> + r =3D 0; >> + break; >> + } >> case KVM_CHECK_EXTENSION: >> r =3D kvm_vm_ioctl_check_extension_generic(kvm, arg); >> break; >> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/k= vm/api.txt >> index 1db1435769b4..c2ba18279298 100644 >> --- a/Documentation/virtual/kvm/api.txt >> +++ b/Documentation/virtual/kvm/api.txt >> @@ -3857,6 +3857,25 @@ number of valid entries in the 'entries' array, w= hich is then filled. >> 'index' and 'flags' fields in 'struct kvm_cpuid_entry2' are currently r= eserved, >> userspace should not expect to get any particular value there. >> =20 >> +4.119 KVM_DESTROY_DEVICE >> + >> +Capability: KVM_CAP_DEVICE_CTRL >> +Type: vm ioctl >> +Parameters: struct kvm_destroy_device (in) >> +Returns: 0 on success, -1 on error >> +Errors: >> + ENODEV: The device type is unknown or unsupported >> + >> + Other error conditions may be defined by individual device types or >> + have their standard meanings. >> + >> +Destroys an emulated device in the kernel. >> + >> +struct kvm_destroy_device { >> + __u32 fd; /* in: device handle */ >> + __u32 flags; /* unused */ >> +}; >> + >> 5. The kvm_run structure >> ------------------------ >> =20 >=20