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

[Kirti Wankhede]

On 10/21/2016 12:54 AM, Alex Williamson wrote:
> On Tue, 18 Oct 2016 02:52:06 +0530
> Kirti Wankhede <address@hidden> 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 <address@hidden>
>> Signed-off-by: Neo Jia <address@hidden>
>> 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.