On 2017.12.13 12:13:34 +1100, Alexey Kardashevskiy wrote:
> 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>
>
>
Looks good for KVMGT part.
Acked-by: Zhenyu Wang <zhenyuw@xxxxxxxxxxxxxxx>
> 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.
>
yeah, we could clean that up, thanks for pointing out.
>
>
> > 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
--
Open Source Technology Center, Intel ltd.
$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
Attachment:
signature.asc
Description: PGP signature
