[PATCH 1/2 v4] cdx: add support for bus mastering

[PATCH 1/2 v4] cdx: add support for bus mastering

[Nipun Gupta]

Introduce cdx_set_master() and cdx_clear_master() APIs to support
enable and disable of bus mastering. Drivers need to use these APIs to
enable/disable DMAs from the CDX devices.

Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-vuuren@amd.com>
---
Changes v3->v4:
- Added user of the Bus master enable and disable APIs in patch 2/2.
  There is no change in this patch.

Changes v2->v3:
- Changed return value from EOPNOTSUPP to -EOPNOTSUPP in
  cdx_set_master()

Changes v1->v2:
- Replace bme with bus_master_enable
- Added check for dev_configure API callback
- remove un-necessary error prints
- changed conditional to if-else
- updated commit message to use 72 columns

 drivers/cdx/cdx.c                       | 29 +++++++++++++
 drivers/cdx/controller/cdx_controller.c |  4 ++
 drivers/cdx/controller/mcdi_functions.c | 58 +++++++++++++++++++++++++
 drivers/cdx/controller/mcdi_functions.h | 13 ++++++
 include/linux/cdx/cdx_bus.h             | 16 +++++++
 5 files changed, 120 insertions(+)

diff --git a/drivers/cdx/cdx.c b/drivers/cdx/cdx.c
index d2cad4c670a0..10c6281b59c3 100644
--- a/drivers/cdx/cdx.c
+++ b/drivers/cdx/cdx.c
@@ -182,6 +182,35 @@ cdx_match_id(const struct cdx_device_id *ids, struct cdx_device *dev)
 	return NULL;
 }
 
+int cdx_set_master(struct cdx_device *cdx_dev)
+{
+	struct cdx_controller *cdx = cdx_dev->cdx;
+	struct cdx_device_config dev_config;
+	int ret = -EOPNOTSUPP;
+
+	dev_config.type = CDX_DEV_BUS_MASTER_CONF;
+	dev_config.bus_master_enable = true;
+	if (cdx->ops->dev_configure)
+		ret = cdx->ops->dev_configure(cdx, cdx_dev->bus_num,
+					      cdx_dev->dev_num, &dev_config);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(cdx_set_master);
+
+void cdx_clear_master(struct cdx_device *cdx_dev)
+{
+	struct cdx_controller *cdx = cdx_dev->cdx;
+	struct cdx_device_config dev_config;
+
+	dev_config.type = CDX_DEV_BUS_MASTER_CONF;
+	dev_config.bus_master_enable = false;
+	if (cdx->ops->dev_configure)
+		cdx->ops->dev_configure(cdx, cdx_dev->bus_num,
+					cdx_dev->dev_num, &dev_config);
+}
+EXPORT_SYMBOL_GPL(cdx_clear_master);
+
 /**
  * cdx_bus_match - device to driver matching callback
  * @dev: the cdx device to match against
diff --git a/drivers/cdx/controller/cdx_controller.c b/drivers/cdx/controller/cdx_controller.c
index dc52f95f8978..39aa569d8e07 100644
--- a/drivers/cdx/controller/cdx_controller.c
+++ b/drivers/cdx/controller/cdx_controller.c
@@ -55,6 +55,10 @@ static int cdx_configure_device(struct cdx_controller *cdx,
 	case CDX_DEV_RESET_CONF:
 		ret = cdx_mcdi_reset_device(cdx->priv, bus_num, dev_num);
 		break;
+	case CDX_DEV_BUS_MASTER_CONF:
+		ret = cdx_mcdi_bus_master_enable(cdx->priv, bus_num, dev_num,
+						 dev_config->bus_master_enable);
+		break;
 	default:
 		ret = -EINVAL;
 	}
diff --git a/drivers/cdx/controller/mcdi_functions.c b/drivers/cdx/controller/mcdi_functions.c
index 0158f26533dd..6acd8fea4586 100644
--- a/drivers/cdx/controller/mcdi_functions.c
+++ b/drivers/cdx/controller/mcdi_functions.c
@@ -137,3 +137,61 @@ int cdx_mcdi_reset_device(struct cdx_mcdi *cdx, u8 bus_num, u8 dev_num)
 
 	return ret;
 }
+
+static int cdx_mcdi_ctrl_flag_get(struct cdx_mcdi *cdx, u8 bus_num,
+				  u8 dev_num, u32 *flags)
+{
+	MCDI_DECLARE_BUF(inbuf, MC_CMD_CDX_DEVICE_CONTROL_GET_IN_LEN);
+	MCDI_DECLARE_BUF(outbuf, MC_CMD_CDX_DEVICE_CONTROL_GET_OUT_LEN);
+	size_t outlen;
+	int ret;
+
+	MCDI_SET_DWORD(inbuf, CDX_DEVICE_CONTROL_GET_IN_BUS, bus_num);
+	MCDI_SET_DWORD(inbuf, CDX_DEVICE_CONTROL_GET_IN_DEVICE, dev_num);
+	ret = cdx_mcdi_rpc(cdx, MC_CMD_CDX_DEVICE_CONTROL_GET, inbuf,
+			   sizeof(inbuf), outbuf, sizeof(outbuf), &outlen);
+	if (ret)
+		return ret;
+
+	if (outlen != MC_CMD_CDX_DEVICE_CONTROL_GET_OUT_LEN)
+		return -EIO;
+
+	*flags = MCDI_DWORD(outbuf, CDX_DEVICE_CONTROL_GET_OUT_FLAGS);
+
+	return 0;
+}
+
+static int cdx_mcdi_ctrl_flag_set(struct cdx_mcdi *cdx, u8 bus_num,
+				  u8 dev_num, bool enable, int lbn)
+{
+	MCDI_DECLARE_BUF(inbuf, MC_CMD_CDX_DEVICE_CONTROL_SET_IN_LEN);
+	u32 flags;
+	int ret;
+
+	/*
+	 * Get flags and then set/reset BUS_MASTER_BIT according to
+	 * the input params.
+	 */
+	ret = cdx_mcdi_ctrl_flag_get(cdx, bus_num, dev_num, &flags);
+	if (ret)
+		return ret;
+
+	flags = flags & (u32)(~(BIT(lbn)));
+	if (enable)
+		flags |= (1 << lbn);
+
+	MCDI_SET_DWORD(inbuf, CDX_DEVICE_CONTROL_SET_IN_BUS, bus_num);
+	MCDI_SET_DWORD(inbuf, CDX_DEVICE_CONTROL_SET_IN_DEVICE, dev_num);
+	MCDI_SET_DWORD(inbuf, CDX_DEVICE_CONTROL_SET_IN_FLAGS, flags);
+	ret = cdx_mcdi_rpc(cdx, MC_CMD_CDX_DEVICE_CONTROL_SET, inbuf,
+			   sizeof(inbuf), NULL, 0, NULL);
+
+	return ret;
+}
+
+int cdx_mcdi_bus_master_enable(struct cdx_mcdi *cdx, u8 bus_num,
+			       u8 dev_num, bool enable)
+{
+	return cdx_mcdi_ctrl_flag_set(cdx, bus_num, dev_num, enable,
+			MC_CMD_CDX_DEVICE_CONTROL_SET_IN_BUS_MASTER_ENABLE_LBN);
+}
diff --git a/drivers/cdx/controller/mcdi_functions.h b/drivers/cdx/controller/mcdi_functions.h
index 7440ace5539a..a448d6581eb4 100644
--- a/drivers/cdx/controller/mcdi_functions.h
+++ b/drivers/cdx/controller/mcdi_functions.h
@@ -58,4 +58,17 @@ int cdx_mcdi_get_dev_config(struct cdx_mcdi *cdx,
 int cdx_mcdi_reset_device(struct cdx_mcdi *cdx,
 			  u8 bus_num, u8 dev_num);
 
+/**
+ * cdx_mcdi_bus_master_enable - Set/Reset bus mastering for cdx device
+ *				represented by bus_num:dev_num
+ * @cdx: pointer to MCDI interface.
+ * @bus_num: Bus number.
+ * @dev_num: Device number.
+ * @enable: Enable bus mastering if set, disable otherwise.
+ *
+ * Return: 0 on success, <0 on failure
+ */
+int cdx_mcdi_bus_master_enable(struct cdx_mcdi *cdx, u8 bus_num,
+			       u8 dev_num, bool enable);
+
 #endif /* CDX_MCDI_FUNCTIONS_H */
diff --git a/include/linux/cdx/cdx_bus.h b/include/linux/cdx/cdx_bus.h
index bead71b7bc73..1816c279879e 100644
--- a/include/linux/cdx/cdx_bus.h
+++ b/include/linux/cdx/cdx_bus.h
@@ -21,11 +21,13 @@
 struct cdx_controller;
 
 enum {
+	CDX_DEV_BUS_MASTER_CONF,
 	CDX_DEV_RESET_CONF,
 };
 
 struct cdx_device_config {
 	u8 type;
+	bool bus_master_enable;
 };
 
 typedef int (*cdx_scan_cb)(struct cdx_controller *cdx);
@@ -170,4 +172,18 @@ extern struct bus_type cdx_bus_type;
  */
 int cdx_dev_reset(struct device *dev);
 
+/**
+ * cdx_set_master - enables bus-mastering for CDX device
+ * @cdx_dev: the CDX device to enable
+ *
+ * Return: 0 for success, -errno on failure
+ */
+int cdx_set_master(struct cdx_device *cdx_dev);
+
+/**
+ * cdx_clear_master - disables bus-mastering for CDX device
+ * @cdx_dev: the CDX device to disable
+ */
+void cdx_clear_master(struct cdx_device *cdx_dev);
+
 #endif /* _CDX_BUS_H_ */
-- 
2.17.1

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

[Nipun Gupta]

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 */
-- 
2.17.1

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

[Greg KH]

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

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

[Alex Williamson]

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

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

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

[Gupta, Nipun]

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