Re: [PATCH 2/2 v4] vfio-cdx: add ioctl support for bus master enable

[Gupta, Nipun]

On 7/26/2023 10:33 PM, Alex Williamson wrote:
> On Wed, 26 Jul 2023 16:32:20 +0530
> Nipun Gupta <nipun.gupta@xxxxxxx> wrote:
>
> > add VFIO_DEVICE_CDX_CTRL IOCTL to expose control operations for CDX
> > devices to VFIO users. Support Bus master enable and Bus master disable
> > on CDX bus control.
> >
> > Signed-off-by: Shubham Rohila <shubham.rohila@xxxxxxx>
> > Signed-off-by: Nipun Gupta <nipun.gupta@xxxxxxx>
> > ---
> >
> > Changes in v4:
> > - This patch is newly added which uses cdx_set_master() and
> >    cdx_clear_master() APIs.
> >
> >   drivers/vfio/cdx/main.c   | 26 ++++++++++++++++++++++++++
> >   include/uapi/linux/vfio.h | 14 ++++++++++++++
> >   2 files changed, 40 insertions(+)
> >
> > diff --git a/drivers/vfio/cdx/main.c b/drivers/vfio/cdx/main.c
> > index c376a69d2db2..c39a965716f4 100644
> > --- a/drivers/vfio/cdx/main.c
> > +++ b/drivers/vfio/cdx/main.c
> > @@ -98,6 +98,30 @@ static int vfio_cdx_ioctl_get_region_info(struct vfio_cdx_device *vdev,
> >   	return copy_to_user(arg, &info, minsz) ? -EFAULT : 0;
> >   }
> >   
> > +static int vfio_cdx_ioctl_ctrl(struct vfio_cdx_device *vdev,
> > +			       struct vfio_device_cdx_ctrl __user *arg)
> > +{
> > +	unsigned long minsz = offsetofend(struct vfio_device_cdx_ctrl, op);
> > +	struct cdx_device *cdx_dev = to_cdx_device(vdev->vdev.dev);
> > +	struct vfio_device_cdx_ctrl ops;
> > +
> > +	if (copy_from_user(&ops, arg, minsz))
> > +		return -EFAULT;
> > +
> > +	if (ops.argsz < minsz)
> > +		return -EINVAL;
> > +
> > +	switch (ops.op) {
> > +	case VFIO_CDX_CTRL_CLEAR_MASTER:
> > +		cdx_clear_master(cdx_dev);
> > +		return 0;
> > +	case VFIO_CDX_CTRL_SET_MASTER:
> > +		return cdx_set_master(cdx_dev);
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> >   static long vfio_cdx_ioctl(struct vfio_device *core_vdev,
> >   			   unsigned int cmd, unsigned long arg)
> >   {
> > @@ -112,6 +136,8 @@ static long vfio_cdx_ioctl(struct vfio_device *core_vdev,
> >   		return vfio_cdx_ioctl_get_region_info(vdev, uarg);
> >   	case VFIO_DEVICE_RESET:
> >   		return cdx_dev_reset(core_vdev->dev);
> > +	case VFIO_DEVICE_CDX_CTRL:
> > +		return vfio_cdx_ioctl_ctrl(vdev, uarg);
> >   	default:
> >   		return -ENOTTY;
> >   	}
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index 20c804bdc09c..5f6a58f9f8e2 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -1649,6 +1649,20 @@ struct vfio_iommu_spapr_tce_remove {
> >   };
> >   #define VFIO_IOMMU_SPAPR_TCE_REMOVE	_IO(VFIO_TYPE, VFIO_BASE + 20)
> >   
> > +/**
> > + * VFIO_DEVICE_CDX_CTRL - _IO(VFIO_TYPE, VFIO_BASE + 21)
> > + *
> > + * Control CDX device.
> > + * Variable op is set as per the operation required
> > + */
> > +struct vfio_device_cdx_ctrl {
> > +	__u32 argsz;
> > +	__u32 op;
> > +#define	VFIO_CDX_CTRL_SET_MASTER	0	/* Set Bus Master */
> > +#define	VFIO_CDX_CTRL_CLEAR_MASTER	1	/* Clear Bus Master */
> > +};
> > +#define VFIO_DEVICE_CDX_CTRL		_IO(VFIO_TYPE, VFIO_BASE + 21)
> > +
> >   /* ***************************************************************** */
> >   
> >   #endif /* _UAPIVFIO_H */
>
> This doesn't follow standard conventions for a vfio ioctl.  Not knowing
> how CDX works, is there really a need for this ioctl vs enabling bus
> master when the device is opened and clearing it when closed?  I think
> this is effectively the behavior of vfio-platform.  Otherwise, I assume
> this is not a high frequency operation, would the VFIO_DEVICE_FEATURE
> ioctl be an option?  Thanks,

Thank you for the quick feedback. There are few use-cases where VFIO-cdx 
users need to explicitly trigger bus master enable/disable. As you 
suggest VFIO_DEVICE_FEATURE is better suited in this case, and we will 
move the BME handling in this ioctl.

Regards,
Nipun

> Alex