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

[Alex Williamson <alex.williamson@redhat.com>]

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,