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

[Gupta, Nipun]

On 8/4/2023 3:47 AM, Alex Williamson wrote:
> On Thu, 3 Aug 2023 20:02:53 +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 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 | 40 ++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 40 insertions(+)
> >
> > diff --git a/drivers/vfio/cdx/main.c b/drivers/vfio/cdx/main.c
> > index c376a69d2db2..6420cf6eb2f9 100644
> > --- a/drivers/vfio/cdx/main.c
> > +++ b/drivers/vfio/cdx/main.c
> > @@ -52,6 +52,45 @@ 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:
> > +		cdx_clear_master(cdx_dev);
> > +		return 0;
> > +	case VFIO_DEVICE_FEATURE_SET_MASTER:
> > +		return cdx_set_master(cdx_dev);
>
> It's curious that the implementation of set and clear in CDX call
> through to functions with non-void returns, but we simply ignore the
> return in cdx_clear_master().  Does something prevent clear from
> failing?

Would update cdx_clear_master() with non-void return.

> I also note internally that true is used for enabling and false for
> disabling, which is effectively opposite of the proposed uAPI in the
> previous patch.

Yes, will fix in the uAPI.

> If the idea here is that the user should assume bus master is disabled
> when opening the device, what happens if the user closes the device
> with bus master enabled?  What would cleanup that state for the next
> user?

cdx_dev_reset() clears the bus mastering and user would enable bus 
mastering once ready for DMA. Probably we also need to add reset in 
device open. So user should assume that bus master is disabled when 
opening the device, and close would also clear the bus mastering (by 
calling reset).

> Is there a use case for the GET operation in userspace?

There is no use-case for get operation in CDX as of now.

Thanks,
Nipun

> Thanks,
>
> Alex
>
> > +	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 +208,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,