From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757539AbcJXVW7 (ORCPT ); Mon, 24 Oct 2016 17:22:59 -0400 Received: from hqemgate14.nvidia.com ([216.228.121.143]:2426 "EHLO hqemgate14.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757329AbcJXVW4 (ORCPT ); Mon, 24 Oct 2016 17:22:56 -0400 X-PGP-Universal: processed; by hqnvupgp08.nvidia.com on Mon, 24 Oct 2016 14:15:29 -0700 Subject: Re: [PATCH v9 06/12] vfio_pci: Update vfio_pci to use vfio_info_add_capability() To: Alex Williamson References: <1476739332-4911-1-git-send-email-kwankhede@nvidia.com> <1476739332-4911-7-git-send-email-kwankhede@nvidia.com> <20161020132454.079606c1@t450s.home> CC: , , , , , , , , X-Nvconfidentiality: public From: Kirti Wankhede Message-ID: <77c0e8bf-1a61-c009-ab18-a62d9fe2f8a0@nvidia.com> Date: Tue, 25 Oct 2016 02:52:39 +0530 MIME-Version: 1.0 In-Reply-To: <20161020132454.079606c1@t450s.home> X-Originating-IP: [10.24.70.85] X-ClientProxiedBy: DRUKMAIL101.nvidia.com (10.25.59.19) To bgmail102.nvidia.com (10.25.59.11) Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 10/21/2016 12:54 AM, Alex Williamson wrote: > On Tue, 18 Oct 2016 02:52:06 +0530 > Kirti Wankhede wrote: > >> Update msix_sparse_mmap_cap() to use vfio_info_add_capability() >> Update region type capability to use vfio_info_add_capability() >> Can't split this commit for MSIx and region_type cap since there is a >> common code which need to be updated for both the cases. >> >> Signed-off-by: Kirti Wankhede >> Signed-off-by: Neo Jia >> Change-Id: I52bb28c7875a6da5a79ddad1843e6088aff58a45 >> --- >> drivers/vfio/pci/vfio_pci.c | 72 +++++++++++++++++---------------------------- >> 1 file changed, 27 insertions(+), 45 deletions(-) >> >> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c >> index d624a527777f..1ec0565b48ea 100644 >> --- a/drivers/vfio/pci/vfio_pci.c >> +++ b/drivers/vfio/pci/vfio_pci.c >> @@ -556,12 +556,12 @@ static int vfio_pci_for_each_slot_or_bus(struct pci_dev *pdev, >> } >> >> static int msix_sparse_mmap_cap(struct vfio_pci_device *vdev, >> + struct vfio_region_info *info, >> struct vfio_info_cap *caps) >> { >> - struct vfio_info_cap_header *header; >> struct vfio_region_info_cap_sparse_mmap *sparse; >> size_t end, size; >> - int nr_areas = 2, i = 0; >> + int nr_areas = 2, i = 0, ret; >> >> end = pci_resource_len(vdev->pdev, vdev->msix_bar); >> >> @@ -572,13 +572,10 @@ static int msix_sparse_mmap_cap(struct vfio_pci_device *vdev, >> >> size = sizeof(*sparse) + (nr_areas * sizeof(*sparse->areas)); >> >> - header = vfio_info_cap_add(caps, size, >> - VFIO_REGION_INFO_CAP_SPARSE_MMAP, 1); >> - if (IS_ERR(header)) >> - return PTR_ERR(header); >> + sparse = kzalloc(size, GFP_KERNEL); >> + if (!sparse) >> + return -ENOMEM; >> >> - sparse = container_of(header, >> - struct vfio_region_info_cap_sparse_mmap, header); >> sparse->nr_areas = nr_areas; >> >> if (vdev->msix_offset & PAGE_MASK) { >> @@ -594,26 +591,11 @@ static int msix_sparse_mmap_cap(struct vfio_pci_device *vdev, >> i++; >> } >> >> - return 0; >> -} >> - >> -static int region_type_cap(struct vfio_pci_device *vdev, >> - struct vfio_info_cap *caps, >> - unsigned int type, unsigned int subtype) >> -{ >> - struct vfio_info_cap_header *header; >> - struct vfio_region_info_cap_type *cap; >> - >> - header = vfio_info_cap_add(caps, sizeof(*cap), >> - VFIO_REGION_INFO_CAP_TYPE, 1); >> - if (IS_ERR(header)) >> - return PTR_ERR(header); >> + ret = vfio_info_add_capability(info, caps, >> + VFIO_REGION_INFO_CAP_SPARSE_MMAP, sparse); >> + kfree(sparse); >> >> - cap = container_of(header, struct vfio_region_info_cap_type, header); >> - cap->type = type; >> - cap->subtype = subtype; >> - >> - return 0; >> + return ret; >> } >> >> int vfio_pci_register_dev_region(struct vfio_pci_device *vdev, >> @@ -704,7 +686,8 @@ static long vfio_pci_ioctl(void *device_data, >> if (vdev->bar_mmap_supported[info.index]) { >> info.flags |= VFIO_REGION_INFO_FLAG_MMAP; >> if (info.index == vdev->msix_bar) { >> - ret = msix_sparse_mmap_cap(vdev, &caps); >> + ret = msix_sparse_mmap_cap(vdev, &info, >> + &caps); >> if (ret) >> return ret; >> } >> @@ -752,6 +735,9 @@ static long vfio_pci_ioctl(void *device_data, >> >> break; >> default: >> + { >> + struct vfio_region_info_cap_type cap_type; >> + >> if (info.index >= >> VFIO_PCI_NUM_REGIONS + vdev->num_regions) >> return -EINVAL; >> @@ -762,27 +748,23 @@ static long vfio_pci_ioctl(void *device_data, >> info.size = vdev->region[i].size; >> info.flags = vdev->region[i].flags; >> >> - ret = region_type_cap(vdev, &caps, >> - vdev->region[i].type, >> - vdev->region[i].subtype); >> + cap_type.type = vdev->region[i].type; >> + cap_type.subtype = vdev->region[i].subtype; >> + >> + ret = vfio_info_add_capability(&info, &caps, >> + VFIO_REGION_INFO_CAP_TYPE, >> + &cap_type); >> if (ret) >> return ret; >> + >> + } >> } >> >> - if (caps.size) { >> - info.flags |= VFIO_REGION_INFO_FLAG_CAPS; >> - if (info.argsz < sizeof(info) + caps.size) { >> - info.argsz = sizeof(info) + caps.size; >> - info.cap_offset = 0; >> - } else { >> - vfio_info_cap_shift(&caps, sizeof(info)); >> - if (copy_to_user((void __user *)arg + >> - sizeof(info), caps.buf, >> - caps.size)) { >> - kfree(caps.buf); >> - return -EFAULT; >> - } >> - info.cap_offset = sizeof(info); > > I prefer the case above, I'm fine with breaking out helpers to build a > buffer containing the capability chain, but I would rather have the > caller manage placing that back into the return structure. That also > allows the helper to be independent of the structure we're operating > on, it could be a region_info, irq_info, device_info, etc. It only > needs to know the layout of the capability type we're trying to add, > not the info structure itself. Thanks, > The capability feature is for region_info (VFIO_REGION_INFO_FLAG_CAPS), then structure we are operating couldn't be irq_info or device_info, right? Kirti. From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37486) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bymhx-0001h3-R8 for qemu-devel@nongnu.org; Mon, 24 Oct 2016 17:23:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bymht-0001CH-PD for qemu-devel@nongnu.org; Mon, 24 Oct 2016 17:23:01 -0400 Received: from hqemgate14.nvidia.com ([216.228.121.143]:2427) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bymht-0001BU-Fu for qemu-devel@nongnu.org; Mon, 24 Oct 2016 17:22:57 -0400 References: <1476739332-4911-1-git-send-email-kwankhede@nvidia.com> <1476739332-4911-7-git-send-email-kwankhede@nvidia.com> <20161020132454.079606c1@t450s.home> From: Kirti Wankhede Message-ID: <77c0e8bf-1a61-c009-ab18-a62d9fe2f8a0@nvidia.com> Date: Tue, 25 Oct 2016 02:52:39 +0530 MIME-Version: 1.0 In-Reply-To: <20161020132454.079606c1@t450s.home> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v9 06/12] vfio_pci: Update vfio_pci to use vfio_info_add_capability() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Williamson Cc: pbonzini@redhat.com, kraxel@redhat.com, cjia@nvidia.com, qemu-devel@nongnu.org, kvm@vger.kernel.org, kevin.tian@intel.com, jike.song@intel.com, bjsdjshi@linux.vnet.ibm.com, linux-kernel@vger.kernel.org On 10/21/2016 12:54 AM, Alex Williamson wrote: > On Tue, 18 Oct 2016 02:52:06 +0530 > Kirti Wankhede wrote: > >> Update msix_sparse_mmap_cap() to use vfio_info_add_capability() >> Update region type capability to use vfio_info_add_capability() >> Can't split this commit for MSIx and region_type cap since there is a >> common code which need to be updated for both the cases. >> >> Signed-off-by: Kirti Wankhede >> Signed-off-by: Neo Jia >> Change-Id: I52bb28c7875a6da5a79ddad1843e6088aff58a45 >> --- >> drivers/vfio/pci/vfio_pci.c | 72 +++++++++++++++++---------------------------- >> 1 file changed, 27 insertions(+), 45 deletions(-) >> >> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c >> index d624a527777f..1ec0565b48ea 100644 >> --- a/drivers/vfio/pci/vfio_pci.c >> +++ b/drivers/vfio/pci/vfio_pci.c >> @@ -556,12 +556,12 @@ static int vfio_pci_for_each_slot_or_bus(struct pci_dev *pdev, >> } >> >> static int msix_sparse_mmap_cap(struct vfio_pci_device *vdev, >> + struct vfio_region_info *info, >> struct vfio_info_cap *caps) >> { >> - struct vfio_info_cap_header *header; >> struct vfio_region_info_cap_sparse_mmap *sparse; >> size_t end, size; >> - int nr_areas = 2, i = 0; >> + int nr_areas = 2, i = 0, ret; >> >> end = pci_resource_len(vdev->pdev, vdev->msix_bar); >> >> @@ -572,13 +572,10 @@ static int msix_sparse_mmap_cap(struct vfio_pci_device *vdev, >> >> size = sizeof(*sparse) + (nr_areas * sizeof(*sparse->areas)); >> >> - header = vfio_info_cap_add(caps, size, >> - VFIO_REGION_INFO_CAP_SPARSE_MMAP, 1); >> - if (IS_ERR(header)) >> - return PTR_ERR(header); >> + sparse = kzalloc(size, GFP_KERNEL); >> + if (!sparse) >> + return -ENOMEM; >> >> - sparse = container_of(header, >> - struct vfio_region_info_cap_sparse_mmap, header); >> sparse->nr_areas = nr_areas; >> >> if (vdev->msix_offset & PAGE_MASK) { >> @@ -594,26 +591,11 @@ static int msix_sparse_mmap_cap(struct vfio_pci_device *vdev, >> i++; >> } >> >> - return 0; >> -} >> - >> -static int region_type_cap(struct vfio_pci_device *vdev, >> - struct vfio_info_cap *caps, >> - unsigned int type, unsigned int subtype) >> -{ >> - struct vfio_info_cap_header *header; >> - struct vfio_region_info_cap_type *cap; >> - >> - header = vfio_info_cap_add(caps, sizeof(*cap), >> - VFIO_REGION_INFO_CAP_TYPE, 1); >> - if (IS_ERR(header)) >> - return PTR_ERR(header); >> + ret = vfio_info_add_capability(info, caps, >> + VFIO_REGION_INFO_CAP_SPARSE_MMAP, sparse); >> + kfree(sparse); >> >> - cap = container_of(header, struct vfio_region_info_cap_type, header); >> - cap->type = type; >> - cap->subtype = subtype; >> - >> - return 0; >> + return ret; >> } >> >> int vfio_pci_register_dev_region(struct vfio_pci_device *vdev, >> @@ -704,7 +686,8 @@ static long vfio_pci_ioctl(void *device_data, >> if (vdev->bar_mmap_supported[info.index]) { >> info.flags |= VFIO_REGION_INFO_FLAG_MMAP; >> if (info.index == vdev->msix_bar) { >> - ret = msix_sparse_mmap_cap(vdev, &caps); >> + ret = msix_sparse_mmap_cap(vdev, &info, >> + &caps); >> if (ret) >> return ret; >> } >> @@ -752,6 +735,9 @@ static long vfio_pci_ioctl(void *device_data, >> >> break; >> default: >> + { >> + struct vfio_region_info_cap_type cap_type; >> + >> if (info.index >= >> VFIO_PCI_NUM_REGIONS + vdev->num_regions) >> return -EINVAL; >> @@ -762,27 +748,23 @@ static long vfio_pci_ioctl(void *device_data, >> info.size = vdev->region[i].size; >> info.flags = vdev->region[i].flags; >> >> - ret = region_type_cap(vdev, &caps, >> - vdev->region[i].type, >> - vdev->region[i].subtype); >> + cap_type.type = vdev->region[i].type; >> + cap_type.subtype = vdev->region[i].subtype; >> + >> + ret = vfio_info_add_capability(&info, &caps, >> + VFIO_REGION_INFO_CAP_TYPE, >> + &cap_type); >> if (ret) >> return ret; >> + >> + } >> } >> >> - if (caps.size) { >> - info.flags |= VFIO_REGION_INFO_FLAG_CAPS; >> - if (info.argsz < sizeof(info) + caps.size) { >> - info.argsz = sizeof(info) + caps.size; >> - info.cap_offset = 0; >> - } else { >> - vfio_info_cap_shift(&caps, sizeof(info)); >> - if (copy_to_user((void __user *)arg + >> - sizeof(info), caps.buf, >> - caps.size)) { >> - kfree(caps.buf); >> - return -EFAULT; >> - } >> - info.cap_offset = sizeof(info); > > I prefer the case above, I'm fine with breaking out helpers to build a > buffer containing the capability chain, but I would rather have the > caller manage placing that back into the return structure. That also > allows the helper to be independent of the structure we're operating > on, it could be a region_info, irq_info, device_info, etc. It only > needs to know the layout of the capability type we're trying to add, > not the info structure itself. Thanks, > The capability feature is for region_info (VFIO_REGION_INFO_FLAG_CAPS), then structure we are operating couldn't be irq_info or device_info, right? Kirti.