On 13/12/17 06:59, Alex Williamson wrote:
> The vfio_info_add_capability() helper requires the caller to pass a
> capability ID, which it then uses to fill in header fields, assuming
> hard coded versions.  This makes for an awkward and rigid interface.
> The only thing we want this helper to do is allocate sufficient
> space in the caps buffer and chain this capability into the list.
> Reduce it to that simple task.
> 
> Signed-off-by: Alex Williamson <alex.william...@redhat.com>


Makes more sense now, thanks. I'll repost mine on top of this.


Reviewed-by: Alexey Kardashevskiy <a...@ozlabs.ru>


Below one observation, unrelated to this patch.

> ---
>  drivers/gpu/drm/i915/gvt/kvmgt.c |   15 +++++++----
>  drivers/vfio/pci/vfio_pci.c      |   14 ++++++----
>  drivers/vfio/vfio.c              |   52 
> +++-----------------------------------
>  include/linux/vfio.h             |    3 +-
>  4 files changed, 24 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c 
> b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index 96060920a6fe..0a7d084da1a2 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -1012,6 +1012,8 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, 
> unsigned int cmd,
>                       if (!sparse)
>                               return -ENOMEM;
>  
> +                     sparse->header.id = VFIO_REGION_INFO_CAP_SPARSE_MMAP;
> +                     sparse->header.version = 1;
>                       sparse->nr_areas = nr_areas;
>                       cap_type_id = VFIO_REGION_INFO_CAP_SPARSE_MMAP;


@cap_type_id is initialized in just one of many cases of switch
(info.index) and after the entire switch, there is switch (cap_type_id). I
wonder why compiler missed "potentially uninitialized variable here,
although there is no bug - @cap_type_id is in sync with @spapse. It would
make it cleaner imho just to have vfio_info_add_capability() next to the
header initialization.



>                       sparse->areas[0].offset =
> @@ -1033,7 +1035,9 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, 
> unsigned int cmd,
>                       break;
>               default:
>                       {
> -                             struct vfio_region_info_cap_type cap_type;
> +                             struct vfio_region_info_cap_type cap_type = {
> +                                     .header.id = VFIO_REGION_INFO_CAP_TYPE,
> +                                     .header.version = 1 };
>  
>                               if (info.index >= VFIO_PCI_NUM_REGIONS +
>                                               vgpu->vdev.num_regions)
> @@ -1050,8 +1054,8 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, 
> unsigned int cmd,
>                               cap_type.subtype = vgpu->vdev.region[i].subtype;
>  
>                               ret = vfio_info_add_capability(&caps,
> -                                             VFIO_REGION_INFO_CAP_TYPE,
> -                                             &cap_type);
> +                                                     &cap_type.header,
> +                                                     sizeof(cap_type));
>                               if (ret)
>                                       return ret;
>                       }
> @@ -1061,8 +1065,9 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, 
> unsigned int cmd,
>                       switch (cap_type_id) {
>                       case VFIO_REGION_INFO_CAP_SPARSE_MMAP:
>                               ret = vfio_info_add_capability(&caps,
> -                                     VFIO_REGION_INFO_CAP_SPARSE_MMAP,
> -                                     sparse);
> +                                     &sparse->header, sizeof(*sparse) +
> +                                     (sparse->nr_areas *
> +                                             sizeof(*sparse->areas)));
>                               kfree(sparse);
>                               if (ret)
>                                       return ret;
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index f041b1a6cf66..a73e40983880 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -582,6 +582,8 @@ static int msix_sparse_mmap_cap(struct vfio_pci_device 
> *vdev,
>       if (!sparse)
>               return -ENOMEM;
>  
> +     sparse->header.id = VFIO_REGION_INFO_CAP_SPARSE_MMAP;
> +     sparse->header.version = 1;
>       sparse->nr_areas = nr_areas;
>  
>       if (vdev->msix_offset & PAGE_MASK) {
> @@ -597,8 +599,7 @@ static int msix_sparse_mmap_cap(struct vfio_pci_device 
> *vdev,
>               i++;
>       }
>  
> -     ret = vfio_info_add_capability(caps, VFIO_REGION_INFO_CAP_SPARSE_MMAP,
> -                                    sparse);
> +     ret = vfio_info_add_capability(caps, &sparse->header, size);
>       kfree(sparse);
>  
>       return ret;
> @@ -741,7 +742,9 @@ static long vfio_pci_ioctl(void *device_data,
>                       break;
>               default:
>               {
> -                     struct vfio_region_info_cap_type cap_type;
> +                     struct vfio_region_info_cap_type cap_type = {
> +                                     .header.id = VFIO_REGION_INFO_CAP_TYPE,
> +                                     .header.version = 1 };
>  
>                       if (info.index >=
>                           VFIO_PCI_NUM_REGIONS + vdev->num_regions)
> @@ -756,9 +759,8 @@ static long vfio_pci_ioctl(void *device_data,
>                       cap_type.type = vdev->region[i].type;
>                       cap_type.subtype = vdev->region[i].subtype;
>  
> -                     ret = vfio_info_add_capability(&caps,
> -                                                   VFIO_REGION_INFO_CAP_TYPE,
> -                                                   &cap_type);
> +                     ret = vfio_info_add_capability(&caps, &cap_type.header,
> +                                                    sizeof(cap_type));
>                       if (ret)
>                               return ret;
>  
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 2bc3705a99bd..721f97f8dac1 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -1857,63 +1857,19 @@ void vfio_info_cap_shift(struct vfio_info_cap *caps, 
> size_t offset)
>  }
>  EXPORT_SYMBOL(vfio_info_cap_shift);
>  
> -static int sparse_mmap_cap(struct vfio_info_cap *caps, void *cap_type)
> +int vfio_info_add_capability(struct vfio_info_cap *caps,
> +                          struct vfio_info_cap_header *cap, size_t size)
>  {
>       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);
> +     header = vfio_info_cap_add(caps, size, cap->id, cap->version);
>       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;
> +     memcpy(header + 1, cap + 1, size - sizeof(*header));
>  
> -     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;
>  }
> -
> -int vfio_info_add_capability(struct vfio_info_cap *caps, int cap_type_id,
> -                          void *cap_type)
> -{
> -     int ret = -EINVAL;
> -
> -     if (!cap_type)
> -             return 0;
> -
> -     switch (cap_type_id) {
> -     case VFIO_REGION_INFO_CAP_SPARSE_MMAP:
> -             ret = sparse_mmap_cap(caps, cap_type);
> -             break;
> -
> -     case VFIO_REGION_INFO_CAP_TYPE:
> -             ret = region_type_cap(caps, cap_type);
> -             break;
> -     }
> -
> -     return ret;
> -}
>  EXPORT_SYMBOL(vfio_info_add_capability);
>  
>  int vfio_set_irqs_validate_and_prepare(struct vfio_irq_set *hdr, int 
> num_irqs,
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index a47b985341d1..66741ab087c1 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -145,7 +145,8 @@ extern struct vfio_info_cap_header *vfio_info_cap_add(
>  extern void vfio_info_cap_shift(struct vfio_info_cap *caps, size_t offset);
>  
>  extern int vfio_info_add_capability(struct vfio_info_cap *caps,
> -                                 int cap_type_id, void *cap_type);
> +                                 struct vfio_info_cap_header *cap,
> +                                 size_t size);
>  
>  extern int vfio_set_irqs_validate_and_prepare(struct vfio_irq_set *hdr,
>                                             int num_irqs, int max_irq_type,
> 


-- 
Alexey

Reply via email to