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

  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@amd.com>
Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
---

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(+)
  

On Wed, Jul 26, 2023 at 04:32:20PM +0530, Nipun Gupta 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@amd.com>
> Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>

Who wrote this?  The signed-off-by ordering seems odd.


> ---
> 
> 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

But what is argsz set to?

> + */
> +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)
> +

Doesn't vfio stuff require a spec and agreement on the interface
somewhere?  Has that happened here already?

And why an ioctl?  Why would userspace care about this type of control?

thanks,

greg k-h
  

On Wed, 26 Jul 2023 16:32:20 +0530
Nipun Gupta <nipun.gupta@amd.com> 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@amd.com>
> Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
> ---
> 
> 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,

Alex
  

On 7/26/2023 10:33 PM, Alex Williamson wrote:
> On Wed, 26 Jul 2023 16:32:20 +0530
> Nipun Gupta <nipun.gupta@amd.com> 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@amd.com>
>> Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
>> ---
>>
>> 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
>
  

On 7/26/2023 4:55 PM, Greg KH wrote:
> On Wed, Jul 26, 2023 at 04:32:20PM +0530, Nipun Gupta 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@amd.com>
>> Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
> 
> Who wrote this?  The signed-off-by ordering seems odd.
> 
> 
>> ---
>>
>> 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
> 
> But what is argsz set to?

This is required to be set as size of the data being passed; which in 
this case is sizeof(struct vfio_device_cdx_ctrl).

> 
>> + */
>> +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)
>> +
> 
> Doesn't vfio stuff require a spec and agreement on the interface
> somewhere?  Has that happened here already?

As suggested by Alex, we can use VFIO_DEVICE_FEATURE which seems well 
suited here.

> 
> And why an ioctl?  Why would userspace care about this type of control?

CDX devices are by default in bus master disabled state and user-space 
drivers will set the bus mastering once appropriate IOMMU mappings have 
been set-up.

Thanks,
Nipun

> 
> thanks,
> 
> greg k-h