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

[Alex Williamson]

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?

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.

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?

Is there a use case for the GET operation in userspace?  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,