Messages in this thread | | | Date | Wed, 16 Aug 2023 11:46:10 -0600 | From | Alex Williamson <> | Subject | Re: [PATCH v6 3/3] vfio-cdx: add bus mastering device feature support |
| |
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).
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,
Alex
> > - return 0; > + return ret; > } > > static void vfio_cdx_close_device(struct vfio_device *core_vdev) > @@ -52,6 +55,44 @@ static void vfio_cdx_close_device(struct vfio_device *core_vdev) > cdx_dev_reset(core_vdev->dev); > } > > +static int vfio_cdx_bm_ctrl(struct vfio_device *core_vdev, u32 flags, > + void __user *arg, size_t argsz) > +{ > + size_t minsz = > + offsetofend(struct vfio_device_feature_bus_master, op); > + struct cdx_device *cdx_dev = to_cdx_device(core_vdev->dev); > + struct vfio_device_feature_bus_master ops; > + int ret; > + > + ret = vfio_check_feature(flags, argsz, VFIO_DEVICE_FEATURE_SET, > + sizeof(ops)); > + if (ret != 1) > + return ret; > + > + if (copy_from_user(&ops, arg, minsz)) > + return -EFAULT; > + > + switch (ops.op) { > + case VFIO_DEVICE_FEATURE_CLEAR_MASTER: > + return cdx_clear_master(cdx_dev); > + case VFIO_DEVICE_FEATURE_SET_MASTER: > + return cdx_set_master(cdx_dev); > + default: > + return -EINVAL; > + } > +} > + > +static int vfio_cdx_ioctl_feature(struct vfio_device *device, u32 flags, > + void __user *arg, size_t argsz) > +{ > + switch (flags & VFIO_DEVICE_FEATURE_MASK) { > + case VFIO_DEVICE_FEATURE_BUS_MASTER: > + return vfio_cdx_bm_ctrl(device, flags, arg, argsz); > + default: > + return -ENOTTY; > + } > +} > + > static int vfio_cdx_ioctl_get_info(struct vfio_cdx_device *vdev, > struct vfio_device_info __user *arg) > { > @@ -169,6 +210,7 @@ static const struct vfio_device_ops vfio_cdx_ops = { > .open_device = vfio_cdx_open_device, > .close_device = vfio_cdx_close_device, > .ioctl = vfio_cdx_ioctl, > + .device_feature = vfio_cdx_ioctl_feature, > .mmap = vfio_cdx_mmap, > .bind_iommufd = vfio_iommufd_physical_bind, > .unbind_iommufd = vfio_iommufd_physical_unbind,
| |