Messages in this thread | | | Date | Fri, 18 Aug 2023 14:02:32 +0530 | Subject | Re: [PATCH v6 3/3] vfio-cdx: add bus mastering device feature support | From | "Gupta, Nipun" <> |
| |
On 8/16/2023 11:16 PM, Alex Williamson wrote: > On Thu, 10 Aug 2023 14:14:09 +0530 > Nipun Gupta <nipun.gupta@amd.com> wrote: > >> Support Bus master enable and disable on VFIO-CDX devices using >> VFIO_DEVICE_FEATURE_BUS_MASTER flag over VFIO_DEVICE_FEATURE IOCTL. >> >> Co-developed-by: Shubham Rohila <shubham.rohila@amd.com> >> Signed-off-by: Shubham Rohila <shubham.rohila@amd.com> >> Signed-off-by: Nipun Gupta <nipun.gupta@amd.com> >> --- >> >> Changes v5->v6: >> - Called CDX device reset at cdx_open_device() >> >> Changes v4->v5: >> - Use device feature IOCTL instead of adding a new VFIO IOCTL >> for bus master feature. >> >> Changes in v4: >> - This patch is newly added which uses cdx_set_master() and >> cdx_clear_master() APIs. >> >> drivers/vfio/cdx/main.c | 46 +++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 44 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/vfio/cdx/main.c b/drivers/vfio/cdx/main.c >> index c376a69d2db2..bf0e1f56e0f9 100644 >> --- a/drivers/vfio/cdx/main.c >> +++ b/drivers/vfio/cdx/main.c >> @@ -14,7 +14,7 @@ static int vfio_cdx_open_device(struct vfio_device *core_vdev) >> container_of(core_vdev, struct vfio_cdx_device, vdev); >> struct cdx_device *cdx_dev = to_cdx_device(core_vdev->dev); >> int count = cdx_dev->res_count; >> - int i; >> + int i, ret; >> >> vdev->regions = kcalloc(count, sizeof(struct vfio_cdx_region), >> GFP_KERNEL_ACCOUNT); >> @@ -39,8 +39,11 @@ static int vfio_cdx_open_device(struct vfio_device *core_vdev) >> if (!(cdx_dev->res[i].flags & IORESOURCE_READONLY)) >> vdev->regions[i].flags |= VFIO_REGION_INFO_FLAG_WRITE; >> } >> + ret = cdx_dev_reset(core_vdev->dev); >> + if (ret) >> + kfree(vdev->regions); > > AIUI, this reset clears bus master, but per the first patch in the > series the ability to set or clear bus master depends on whether the > underlying cdx_ops supports dev_configure. Apparently all currently > do, but will that always be true? > > It seems like this could make a gratuitous call to cdx_clear_master() > to validate the return value and only conditionally support this device > feature based on that result (or fail the device open if it's meant to > be required).
CDX bus driver does not explicitly call cdx_clear_master during reset. It is triggered by device implicitly and hence device_reset would never fail due to lack of bus mastering capability.
Do you mean if cdx_dev_reset() fails we should not set the VFIO_DEVICE_FLAGS_RESET in vfio_device_info? something like:
ret = cdx_dev_reset(core_vdev->dev); if (ret == -EOPNOTSUPP) /* make sure VFIO_DEVICE_FLAGS_RESET is not returned in * flags for device get info */ else if (ret) kfree(vdev->regions);
From new device feature added for BUS mastering if cdx_clear_master() fails due to no support, the bus driver will return -EOPNOTSUPP, so same would be communicated to the user-space, so it seems fine from this end.
> > It might also be a good idea to set vdev->regions = NULL in the error > path to avoid the possibility of a double-free. Thanks,
Sure. Will update.
Thanks, Nipun
> > Alex
| |