Re: [PATCH v2 15/16] KVM: introduce a KVM_DESTROY_DEVICE ioctl

["Cédric Le Goater" <clg@kaod.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 <clg@kaod.org>
> 
> 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
>>  ------------------------
>>  
>