| Message ID | 1476739332-4911-7-git-send-email-kwankhede@nvidia.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 18 Oct 2016 02:52:06 +0530 Kirti Wankhede <kwankhede@nvidia.com> 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 <kwankhede@nvidia.com> > Signed-off-by: Neo Jia <cjia@nvidia.com> > 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, Alex > + if (info.cap_offset) { > + if (copy_to_user((void __user *)arg + info.cap_offset, > + caps.buf, caps.size)) { > + kfree(caps.buf); > + return -EFAULT; > } > > kfree(caps.buf);
On 10/21/2016 12:54 AM, Alex Williamson wrote: > On Tue, 18 Oct 2016 02:52:06 +0530 > Kirti Wankhede <kwankhede@nvidia.com> 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 <kwankhede@nvidia.com> >> Signed-off-by: Neo Jia <cjia@nvidia.com> >> 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.
On Tue, 25 Oct 2016 02:52:39 +0530 Kirti Wankhede <kwankhede@nvidia.com> wrote: > On 10/21/2016 12:54 AM, Alex Williamson wrote: > > On Tue, 18 Oct 2016 02:52:06 +0530 > > Kirti Wankhede <kwankhede@nvidia.com> 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 <kwankhede@nvidia.com> > >> Signed-off-by: Neo Jia <cjia@nvidia.com> > >> 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? My point was that I prefer the caller managing region_info.flags and region_info.cap_offset, the helper only managing creating the capability chain within the buffer. There is a namespace conflict for the various capabilities though, each foo_info has their own space, which can conflict, which make make a common helper impractical. Thanks, Alex
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); + if (info.cap_offset) { + if (copy_to_user((void __user *)arg + info.cap_offset, + caps.buf, caps.size)) { + kfree(caps.buf); + return -EFAULT; } kfree(caps.buf);