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.williamson@xxxxxxxxxx>
Makes more sense now, thanks. I'll repost mine on top of this.
Reviewed-by: Alexey Kardashevskiy <aik@xxxxxxxxx>
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
