Re: [PATCH v6 3/3] vfio-cdx: add bus mastering device feature support

[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@xxxxxxx> 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@xxxxxxx>
> > Signed-off-by: Shubham Rohila <shubham.rohila@xxxxxxx>
> > Signed-off-by: Nipun Gupta <nipun.gupta@xxxxxxx>
> > ---
> >
> > 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