Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751933AbdLMJXy (ORCPT ); Wed, 13 Dec 2017 04:23:54 -0500 Received: from mx1.redhat.com ([209.132.183.28]:33354 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751049AbdLMJXt (ORCPT ); Wed, 13 Dec 2017 04:23:49 -0500 Subject: Re: [PATCH] vfio: Simplify capability helper To: Alex Williamson References: <20171212195850.13691.40496.stgit@gimli.home> Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, intel-gvt-dev@lists.freedesktop.org, aik@ozlabs.ru, kwankhede@nvidia.com, zhenyuw@linux.intel.com, zhi.a.wang@intel.com From: Auger Eric Message-ID: Date: Wed, 13 Dec 2017 10:23:43 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20171212195850.13691.40496.stgit@gimli.home> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Wed, 13 Dec 2017 09:23:49 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7212 Lines: 206 Hi Alex, On 12/12/17 20: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 > --- > 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; > 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))); nit: there is a size local variable which would be usable here. > 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, > so yet another one: Reviewed-by: Eric Auger Thanks Eric