[PATCH v5 1/3] cdx: add support for bus mastering

[PATCH v5 1/3] 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 v4->v5:
- No change in this patch, patch 2/3 and patch 3/3 are updated

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 v5 2/3] vfio: add bus master feature to device feature ioctl

[Nipun Gupta]

add bus mastering control to VFIO_DEVICE_FEATURE IOCTL. The VFIO user
can use this feature to enable or disable the Bus Mastering of a
device bound to VFIO.

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 in v5:
- This patch is newly added which proposes a new flag
  VFIO_DEVICE_FEATURE_BUS_MASTER in VFIO_DEVICE_FEATURE IOCTL. 

---
 include/uapi/linux/vfio.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 20c804bdc09c..05350a2f1eab 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -1287,6 +1287,22 @@ struct vfio_device_feature_mig_data_size {
 
 #define VFIO_DEVICE_FEATURE_MIG_DATA_SIZE 9
 
+/**
+ * Upon VFIO_DEVICE_FEATURE_SET, allow the BUS mastering for the device to be
+ * set or clear based on the operation specified in op flag.
+ *
+ * If the BUS MASTER of the device is configured to CLEAR,
+ * all the incoming DMA from the device will be blocked.
+ * If the BUS MASTER of the device is configured to SET (enable),
+ * device would be able to do DMA to host memory.
+ */
+struct vfio_device_feature_bus_master {
+	__u32 op;
+#define        VFIO_DEVICE_FEATURE_SET_MASTER        0       /* Set Bus Master */
+#define        VFIO_DEVICE_FEATURE_CLEAR_MASTER      1       /* Clear Bus Master */
+};
+#define VFIO_DEVICE_FEATURE_BUS_MASTER 9
+
 /* -------- API for Type1 VFIO IOMMU -------- */
 
 /**
-- 
2.17.1

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

[Nipun Gupta]

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 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);
+	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,
-- 
2.17.1

Re: [PATCH v5 2/3] vfio: add bus master feature to device feature ioctl

[Alex Williamson]

On Thu, 3 Aug 2023 20:02:52 +0530
Nipun Gupta <nipun.gupta@amd.com> wrote:

> add bus mastering control to VFIO_DEVICE_FEATURE IOCTL. The VFIO user
> can use this feature to enable or disable the Bus Mastering of a
> device bound to VFIO.
> 
> 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 in v5:
> - This patch is newly added which proposes a new flag
>   VFIO_DEVICE_FEATURE_BUS_MASTER in VFIO_DEVICE_FEATURE IOCTL. 
> 
> ---
>  include/uapi/linux/vfio.h | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 20c804bdc09c..05350a2f1eab 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -1287,6 +1287,22 @@ struct vfio_device_feature_mig_data_size {
>  
>  #define VFIO_DEVICE_FEATURE_MIG_DATA_SIZE 9
>  
> +/**
> + * Upon VFIO_DEVICE_FEATURE_SET, allow the BUS mastering for the device to be
> + * set or clear based on the operation specified in op flag.
> + *
> + * If the BUS MASTER of the device is configured to CLEAR,
> + * all the incoming DMA from the device will be blocked.
> + * If the BUS MASTER of the device is configured to SET (enable),
> + * device would be able to do DMA to host memory.

CDX is clearly not the only bus that has the concept of controlling a
device's ability to perform DMA, so I'm concerned about this
description.  For example someone with no prior vfio-pci experience
might be confused reading the uAPI and then not having support for this
feature in vfio-pci.

One option would be to make this CDX specific through the name, ie.
VFIO_DEVICE_FEATURE_CDX_BUS_MASTER, but maybe it's sufficient to leave
it general but explain here that this is only implemented for devices
which require bus master control and have no means in the in-band
device interface to provide that control.  Going on to state that PCI
bus master is controlled in-band through config space and that this is
initially only provided for CDX devices would be useful.  Of course the
PROBE interface can be used to determine if this is available for a
given device.

> + */
> +struct vfio_device_feature_bus_master {
> +	__u32 op;
> +#define        VFIO_DEVICE_FEATURE_SET_MASTER        0       /* Set Bus Master */
> +#define        VFIO_DEVICE_FEATURE_CLEAR_MASTER      1       /* Clear Bus Master */

Ultimately it doesn't matter, but this is a strange choice, set = 0,
clear = 1.  I think the reverse would be better for raw debugging.
Thanks,

Alex

> +};
> +#define VFIO_DEVICE_FEATURE_BUS_MASTER 9
> +
>  /* -------- API for Type1 VFIO IOMMU -------- */
>  
>  /**

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

Re: [PATCH v5 2/3] vfio: add bus master feature to device feature ioctl

[Gupta, Nipun]

On 8/4/2023 3:30 AM, Alex Williamson wrote:
> On Thu, 3 Aug 2023 20:02:52 +0530
> Nipun Gupta <nipun.gupta@amd.com> wrote:
> 
>> add bus mastering control to VFIO_DEVICE_FEATURE IOCTL. The VFIO user
>> can use this feature to enable or disable the Bus Mastering of a
>> device bound to VFIO.
>>
>> 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 in v5:
>> - This patch is newly added which proposes a new flag
>>    VFIO_DEVICE_FEATURE_BUS_MASTER in VFIO_DEVICE_FEATURE IOCTL.
>>
>> ---
>>   include/uapi/linux/vfio.h | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> index 20c804bdc09c..05350a2f1eab 100644
>> --- a/include/uapi/linux/vfio.h
>> +++ b/include/uapi/linux/vfio.h
>> @@ -1287,6 +1287,22 @@ struct vfio_device_feature_mig_data_size {
>>   
>>   #define VFIO_DEVICE_FEATURE_MIG_DATA_SIZE 9
>>   
>> +/**
>> + * Upon VFIO_DEVICE_FEATURE_SET, allow the BUS mastering for the device to be
>> + * set or clear based on the operation specified in op flag.
>> + *
>> + * If the BUS MASTER of the device is configured to CLEAR,
>> + * all the incoming DMA from the device will be blocked.
>> + * If the BUS MASTER of the device is configured to SET (enable),
>> + * device would be able to do DMA to host memory.
> 
> CDX is clearly not the only bus that has the concept of controlling a
> device's ability to perform DMA, so I'm concerned about this
> description.  For example someone with no prior vfio-pci experience
> might be confused reading the uAPI and then not having support for this
> feature in vfio-pci.
> 
> One option would be to make this CDX specific through the name, ie.
> VFIO_DEVICE_FEATURE_CDX_BUS_MASTER, but maybe it's sufficient to leave
> it general but explain here that this is only implemented for devices
> which require bus master control and have no means in the in-band
> device interface to provide that control.  Going on to state that PCI
> bus master is controlled in-band through config space and that this is
> initially only provided for CDX devices would be useful.  Of course the
> PROBE interface can be used to determine if this is available for a
> given device.

Agreed. Will update the comment to reflect the same.

> 
>> + */
>> +struct vfio_device_feature_bus_master {
>> +	__u32 op;
>> +#define        VFIO_DEVICE_FEATURE_SET_MASTER        0       /* Set Bus Master */
>> +#define        VFIO_DEVICE_FEATURE_CLEAR_MASTER      1       /* Clear Bus Master */
> 
> Ultimately it doesn't matter, but this is a strange choice, set = 0,
> clear = 1.  I think the reverse would be better for raw debugging.

Yes, makes sense to use 1 for set and 0 for clear. Will fix in next respin.

Thanks,
Nipun

> Thanks,
> 
> Alex
> 
>> +};
>> +#define VFIO_DEVICE_FEATURE_BUS_MASTER 9
>> +
>>   /* -------- API for Type1 VFIO IOMMU -------- */
>>   
>>   /**
> 

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

[Gupta, Nipun]

On 8/4/2023 3:47 AM, Alex Williamson wrote:
> On Thu, 3 Aug 2023 20:02:53 +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 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?

Would update cdx_clear_master() with non-void return.

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

Yes, will fix in the uAPI.

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

cdx_dev_reset() clears the bus mastering and user would enable bus 
mastering once ready for DMA. Probably we also need to add reset in 
device open. So user should assume that bus master is disabled when 
opening the device, and close would also clear the bus mastering (by 
calling reset).

> 
> Is there a use case for the GET operation in userspace?

There is no use-case for get operation in CDX as of now.

Thanks,
Nipun

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