Re: [PATCH v9 06/12] vfio_pci: Update vfio_pci to use vfio_info_add_capability()

From: Kirti Wankhede <kwankhede@nvidia.com>
To: Alex Williamson <alex.williamson@redhat.com>
Cc: <pbonzini@redhat.com>, <kraxel@redhat.com>, <cjia@nvidia.com>,
	<qemu-devel@nongnu.org>, <kvm@vger.kernel.org>,
	<kevin.tian@intel.com>, <jike.song@intel.com>,
	<bjsdjshi@linux.vnet.ibm.com>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v9 06/12] vfio_pci: Update vfio_pci to use vfio_info_add_capability()
Date: Tue, 25 Oct 2016 02:52:39 +0530	[thread overview]
Message-ID: <77c0e8bf-1a61-c009-ab18-a62d9fe2f8a0@nvidia.com> (raw)
In-Reply-To: <20161020132454.079606c1@t450s.home>

On 10/21/2016 12:54 AM, Alex Williamson wrote:
> On Tue, 18 Oct 2016 02:52:06 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> 
>> Update msix_sparse_mmap_cap() to use vfio_info_add_capability()
>> Update region type capability to use vfio_info_add_capability()
>> Can't split this commit for MSIx and region_type cap since there is a
>> common code which need to be updated for both the cases.
>>
>> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
>> Signed-off-by: Neo Jia <cjia@nvidia.com>
>> Change-Id: I52bb28c7875a6da5a79ddad1843e6088aff58a45
>> ---
>>  drivers/vfio/pci/vfio_pci.c | 72 +++++++++++++++++----------------------------
>>  1 file changed, 27 insertions(+), 45 deletions(-)
>>
>> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
>> index d624a527777f..1ec0565b48ea 100644
>> --- a/drivers/vfio/pci/vfio_pci.c
>> +++ b/drivers/vfio/pci/vfio_pci.c
>> @@ -556,12 +556,12 @@ static int vfio_pci_for_each_slot_or_bus(struct pci_dev *pdev,
>>  }
>>  
>>  static int msix_sparse_mmap_cap(struct vfio_pci_device *vdev,
>> +				struct vfio_region_info *info,
>>  				struct vfio_info_cap *caps)
>>  {
>> -	struct vfio_info_cap_header *header;
>>  	struct vfio_region_info_cap_sparse_mmap *sparse;
>>  	size_t end, size;
>> -	int nr_areas = 2, i = 0;
>> +	int nr_areas = 2, i = 0, ret;
>>  
>>  	end = pci_resource_len(vdev->pdev, vdev->msix_bar);
>>  
>> @@ -572,13 +572,10 @@ static int msix_sparse_mmap_cap(struct vfio_pci_device *vdev,
>>  
>>  	size = sizeof(*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 = kzalloc(size, GFP_KERNEL);
>> +	if (!sparse)
>> +		return -ENOMEM;
>>  
>> -	sparse = container_of(header,
>> -			      struct vfio_region_info_cap_sparse_mmap, header);
>>  	sparse->nr_areas = nr_areas;
>>  
>>  	if (vdev->msix_offset & PAGE_MASK) {
>> @@ -594,26 +591,11 @@ static int msix_sparse_mmap_cap(struct vfio_pci_device *vdev,
>>  		i++;
>>  	}
>>  
>> -	return 0;
>> -}
>> -
>> -static int region_type_cap(struct vfio_pci_device *vdev,
>> -			   struct vfio_info_cap *caps,
>> -			   unsigned int type, unsigned int subtype)
>> -{
>> -	struct vfio_info_cap_header *header;
>> -	struct vfio_region_info_cap_type *cap;
>> -
>> -	header = vfio_info_cap_add(caps, sizeof(*cap),
>> -				   VFIO_REGION_INFO_CAP_TYPE, 1);
>> -	if (IS_ERR(header))
>> -		return PTR_ERR(header);
>> +	ret = vfio_info_add_capability(info, caps,
>> +				      VFIO_REGION_INFO_CAP_SPARSE_MMAP, sparse);
>> +	kfree(sparse);
>>  
>> -	cap = container_of(header, struct vfio_region_info_cap_type, header);
>> -	cap->type = type;
>> -	cap->subtype = subtype;
>> -
>> -	return 0;
>> +	return ret;
>>  }
>>  
>>  int vfio_pci_register_dev_region(struct vfio_pci_device *vdev,
>> @@ -704,7 +686,8 @@ static long vfio_pci_ioctl(void *device_data,
>>  			if (vdev->bar_mmap_supported[info.index]) {
>>  				info.flags |= VFIO_REGION_INFO_FLAG_MMAP;
>>  				if (info.index == vdev->msix_bar) {
>> -					ret = msix_sparse_mmap_cap(vdev, &caps);
>> +					ret = msix_sparse_mmap_cap(vdev, &info,
>> +								   &caps);
>>  					if (ret)
>>  						return ret;
>>  				}
>> @@ -752,6 +735,9 @@ static long vfio_pci_ioctl(void *device_data,
>>  
>>  			break;
>>  		default:
>> +		{
>> +			struct vfio_region_info_cap_type cap_type;
>> +
>>  			if (info.index >=
>>  			    VFIO_PCI_NUM_REGIONS + vdev->num_regions)
>>  				return -EINVAL;
>> @@ -762,27 +748,23 @@ static long vfio_pci_ioctl(void *device_data,
>>  			info.size = vdev->region[i].size;
>>  			info.flags = vdev->region[i].flags;
>>  
>> -			ret = region_type_cap(vdev, &caps,
>> -					      vdev->region[i].type,
>> -					      vdev->region[i].subtype);
>> +			cap_type.type = vdev->region[i].type;
>> +			cap_type.subtype = vdev->region[i].subtype;
>> +
>> +			ret = vfio_info_add_capability(&info, &caps,
>> +						      VFIO_REGION_INFO_CAP_TYPE,
>> +						      &cap_type);
>>  			if (ret)
>>  				return ret;
>> +
>> +		}
>>  		}
>>  
>> -		if (caps.size) {
>> -			info.flags |= VFIO_REGION_INFO_FLAG_CAPS;
>> -			if (info.argsz < sizeof(info) + caps.size) {
>> -				info.argsz = sizeof(info) + caps.size;
>> -				info.cap_offset = 0;
>> -			} else {
>> -				vfio_info_cap_shift(&caps, sizeof(info));
>> -				if (copy_to_user((void __user *)arg +
>> -						  sizeof(info), caps.buf,
>> -						  caps.size)) {
>> -					kfree(caps.buf);
>> -					return -EFAULT;
>> -				}
>> -				info.cap_offset = sizeof(info);
> 
> I prefer the case above, I'm fine with breaking out helpers to build a
> buffer containing the capability chain, but I would rather have the
> caller manage placing that back into the return structure.  That also
> allows the helper to be independent of the structure we're operating
> on, it could be a region_info, irq_info, device_info, etc.  It only
> needs to know the layout of the capability type we're trying to add,
> not the info structure itself.  Thanks,
> 

The capability feature is for region_info (VFIO_REGION_INFO_FLAG_CAPS),
then structure we are operating couldn't be irq_info or device_info, right?

Kirti.