Re: [PATCH v8 6/6] Add common functions for SET_IRQS and GET_REGION_INFO ioctls

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 10/12/2016 4:48 AM, Alex Williamson wrote:
> On Tue, 11 Oct 2016 01:58:37 +0530
> Kirti Wankhede <kwankhede@xxxxxxxxxx> wrote:
> 
>> Add common functions for SET_IRQS and to add capability buffer for
>> GET_REGION_INFO ioctls
> 
> Clearly should be two (or more) separate patches since SET_IRQS and
> REGION_INFO are unrelated changes.  Each of the two capabilities handled
> could possibly be separate patches as well.
> 

Ok. I'll have the two separated.

>  
...

>> @@ -754,35 +742,22 @@ static long vfio_pci_ioctl(void *device_data,
>>  	} else if (cmd == VFIO_DEVICE_SET_IRQS) {
>>  		struct vfio_irq_set hdr;
>>  		u8 *data = NULL;
>> -		int ret = 0;
>> +		int max, ret = 0, data_size = 0;
>>  
>>  		minsz = offsetofend(struct vfio_irq_set, count);
>>  
>>  		if (copy_from_user(&hdr, (void __user *)arg, minsz))
>>  			return -EFAULT;
>>  
>> -		if (hdr.argsz < minsz || hdr.index >= VFIO_PCI_NUM_IRQS ||
>> -		    hdr.flags & ~(VFIO_IRQ_SET_DATA_TYPE_MASK |
>> -				  VFIO_IRQ_SET_ACTION_TYPE_MASK))
>> -			return -EINVAL;
>> -
>> -		if (!(hdr.flags & VFIO_IRQ_SET_DATA_NONE)) {
>> -			size_t size;
>> -			int max = vfio_pci_get_irq_count(vdev, hdr.index);
>> +		max = vfio_pci_get_irq_count(vdev, hdr.index);
>>  
>> -			if (hdr.flags & VFIO_IRQ_SET_DATA_BOOL)
>> -				size = sizeof(uint8_t);
>> -			else if (hdr.flags & VFIO_IRQ_SET_DATA_EVENTFD)
>> -				size = sizeof(int32_t);
>> -			else
>> -				return -EINVAL;
>> -
>> -			if (hdr.argsz - minsz < hdr.count * size ||
>> -			    hdr.start >= max || hdr.start + hdr.count > max)
>> -				return -EINVAL;
> 
> 
> vfio_platform has very similar code that would also need to be updated.
>

Ok. Thanks for pointing that out. I'll update that too.


>> +		ret = vfio_set_irqs_validate_and_prepare(&hdr, max, &data_size);
>> +		if (ret)
>> +			return ret;
>>  
>> +		if (data_size) {
>>  			data = memdup_user((void __user *)(arg + minsz),
>> -					   hdr.count * size);
>> +					    data_size);
>>  			if (IS_ERR(data))
>>  				return PTR_ERR(data);
>>  		}
>> @@ -790,7 +765,7 @@ static long vfio_pci_ioctl(void *device_data,
>>  		mutex_lock(&vdev->igate);
>>  
>>  		ret = vfio_pci_set_irqs_ioctl(vdev, hdr.flags, hdr.index,
>> -					      hdr.start, hdr.count, data);
>> +				hdr.start, hdr.count, data);
> 
> White space bogosity.
> 
>>  
>>  		mutex_unlock(&vdev->igate);
>>  		kfree(data);
>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
>> index e3e342861e04..0185d5fb2c85 100644
>> --- a/drivers/vfio/vfio.c
>> +++ b/drivers/vfio/vfio.c
>> @@ -1782,6 +1782,122 @@ void vfio_info_cap_shift(struct vfio_info_cap *caps, size_t offset)
>>  }
>>  EXPORT_SYMBOL_GPL(vfio_info_cap_shift);
>>  
>> +static int sparse_mmap_cap(struct vfio_info_cap *caps, void *cap_type)
>> +{
>> +	struct vfio_info_cap_header *header;
>> +	struct vfio_region_info_cap_sparse_mmap *sparse_cap, *sparse = cap_type;
>> +	size_t size;
>> +
>> +	size = sizeof(*sparse) + sparse->nr_areas *  sizeof(*sparse->areas);
>> +	header = vfio_info_cap_add(caps, size,
>> +				   VFIO_REGION_INFO_CAP_SPARSE_MMAP, 1);
>> +	if (IS_ERR(header))
>> +		return PTR_ERR(header);
>> +
>> +	sparse_cap = container_of(header,
>> +			struct vfio_region_info_cap_sparse_mmap, header);
>> +	sparse_cap->nr_areas = sparse->nr_areas;
>> +	memcpy(sparse_cap->areas, sparse->areas,
>> +	       sparse->nr_areas * sizeof(*sparse->areas));
>> +	return 0;
>> +}
>> +
>> +static int region_type_cap(struct vfio_info_cap *caps, void *cap_type)
>> +{
>> +	struct vfio_info_cap_header *header;
>> +	struct vfio_region_info_cap_type *type_cap, *cap = cap_type;
>> +
>> +	header = vfio_info_cap_add(caps, sizeof(*cap),
>> +				   VFIO_REGION_INFO_CAP_TYPE, 1);
>> +	if (IS_ERR(header))
>> +		return PTR_ERR(header);
>> +
>> +	type_cap = container_of(header, struct vfio_region_info_cap_type,
>> +				header);
>> +	type_cap->type = cap->type;
>> +	type_cap->subtype = cap->subtype;
>> +	return 0;
>> +}
> 
> Why can't we just do a memcpy of all the data past the header?  Do we
> need separate functions for these?
> 

In case of sparse_cap, data past header is variable, depends on
nr_areas. For region_type_cap, data is fixed. For both capabilities,
structures are different and id are different. I think we need seperate
functions.

> vfio_info_cap_add() should now be static and unexported, right?
> 

Yes.

>> +
>> +int vfio_info_add_capability(struct vfio_region_info *info,
>> +			     struct vfio_info_cap *caps,
>> +			     int cap_type_id,
>> +			     void *cap_type)
>> +{
>> +	int ret;
>> +
>> +	if (!(info->flags & VFIO_REGION_INFO_FLAG_CAPS) || !cap_type)
> 
> Why make the caller set flags, seems rather arbitrary since this
> function controls the cap_offset and whether we actually end up copying
> the data.
> 

Kept this flag to be set at caller side so that if caller sets this flag
it should also fill cap_type.
Yes, it could be moved in here, so in that case sanity check will be
only on !cap_type and based on this cap_type flag would be set.

Thanks,
Kirti
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux