Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752160AbdLMJFT (ORCPT ); Wed, 13 Dec 2017 04:05:19 -0500 Received: from hqemgate14.nvidia.com ([216.228.121.143]:8937 "EHLO hqemgate14.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751641AbdLMJFJ (ORCPT ); Wed, 13 Dec 2017 04:05:09 -0500 X-PGP-Universal: processed; by hqpgpgate102.nvidia.com on Wed, 13 Dec 2017 01:05:55 -0800 Subject: Re: [PATCH] vfio: Simplify capability helper To: Zhenyu Wang , Alexey Kardashevskiy CC: Alex Williamson , , , , References: <20171212195850.13691.40496.stgit@gimli.home> <47fab4f4-3f13-312c-aff8-5b3576b12578@ozlabs.ru> <20171213070123.jw2isiu5xhjnnqie@zhen-hp.sh.intel.com> X-Nvconfidentiality: public From: Kirti Wankhede Message-ID: <84b42bfc-1e07-ee1e-31f0-7425618aa48c@nvidia.com> Date: Wed, 13 Dec 2017 14:34:59 +0530 MIME-Version: 1.0 In-Reply-To: <20171213070123.jw2isiu5xhjnnqie@zhen-hp.sh.intel.com> X-Originating-IP: [10.24.71.121] X-ClientProxiedBy: BGMAIL102.nvidia.com (10.25.59.11) To bgmail102.nvidia.com (10.25.59.11) Content-Type: text/plain; charset="windows-1252" Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8421 Lines: 243 On 12/13/2017 12:31 PM, Zhenyu Wang wrote: > 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 >> >> >> Makes more sense now, thanks. I'll repost mine on top of this. >> >> >> Reviewed-by: Alexey Kardashevskiy >> >> > > Looks good for KVMGT part. > > Acked-by: Zhenyu Wang > Looks good to me too. Reviewed-by: Kirti Wankhede Thanks, Kirti >> 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 >