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 <[email protected]>
Reviewed-by: Pieter Jansen van Vuuren <[email protected]>
---
Changes v5->v6:
- change cdx_clear_master() to int return type
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 | 31 +++++++++++++
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 | 18 ++++++++
5 files changed, 124 insertions(+)
diff --git a/drivers/cdx/cdx.c b/drivers/cdx/cdx.c
index d2cad4c670a0..363951b50034 100644
--- a/drivers/cdx/cdx.c
+++ b/drivers/cdx/cdx.c
@@ -182,6 +182,37 @@ 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);
+
+int cdx_clear_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 = false;
+ 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_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..8320ec3b9e37 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,20 @@ 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
+ *
+ * Return: 0 for success, -errno on failure
+ */
+int cdx_clear_master(struct cdx_device *cdx_dev);
+
#endif /* _CDX_BUS_H_ */
--
2.17.1
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 <[email protected]>
Signed-off-by: Shubham Rohila <[email protected]>
Signed-off-by: Nipun Gupta <[email protected]>
---
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);
- 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,
--
2.17.1
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 <[email protected]>
Signed-off-by: Shubham Rohila <[email protected]>
Signed-off-by: Nipun Gupta <[email protected]>
---
Changes v5->v6:
- Updated description of VFIO_DEVICE_FEATURE_BUS_MASTER IOCTL
- Used 0 for clear and 1 for set bus master operation
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 | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 20c804bdc09c..6858cd80d597 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -1287,6 +1287,27 @@ struct vfio_device_feature_mig_data_size {
#define VFIO_DEVICE_FEATURE_MIG_DATA_SIZE 9
+/**
+ * Upon VFIO_DEVICE_FEATURE_SET, set or clear the BUS mastering for the device
+ * based on the operation specified in op flag.
+ *
+ * The functionality is incorporated for devices that needs bus master control,
+ * but the in-band device interface lacks the support. Consequently, it is not
+ * applicable to PCI devices, as bus master control for PCI devices is managed
+ * in-band through the configuration space. At present, this feature is supported
+ * only for CDX devices.
+ * When the device's BUS MASTER setting is configured as CLEAR, it will result in
+ * blocking all incoming DMA requests from the device. On the other hand, configuring
+ * the device's BUS MASTER setting as SET (enable) will grant the device the
+ * capability to perform DMA to the host memory.
+ */
+struct vfio_device_feature_bus_master {
+ __u32 op;
+#define VFIO_DEVICE_FEATURE_CLEAR_MASTER 0 /* Clear Bus Master */
+#define VFIO_DEVICE_FEATURE_SET_MASTER 1 /* Set Bus Master */
+};
+#define VFIO_DEVICE_FEATURE_BUS_MASTER 10
+
/* -------- API for Type1 VFIO IOMMU -------- */
/**
--
2.17.1
On Mon, 21 Aug 2023 16:27:40 +0530
"Gupta, Nipun" <[email protected]> wrote:
> On 8/18/2023 8:07 PM, Alex Williamson wrote:
> > On Fri, 18 Aug 2023 14:02:32 +0530
> > "Gupta, Nipun" <[email protected]> wrote:
> >
> >> On 8/16/2023 11:16 PM, Alex Williamson wrote:
> >>> On Thu, 10 Aug 2023 14:14:09 +0530
> >>> Nipun Gupta <[email protected]> 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 <[email protected]>
> >>>> Signed-off-by: Shubham Rohila <[email protected]>
> >>>> Signed-off-by: Nipun Gupta <[email protected]>
> >>>> ---
> >>>>
> >>>> 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).
> >>
> >> CDX bus driver does not explicitly call cdx_clear_master during reset.
> >> It is triggered by device implicitly and hence device_reset would never
> >> fail due to lack of bus mastering capability.
> >>
> >> Do you mean if cdx_dev_reset() fails we should not set the
> >> VFIO_DEVICE_FLAGS_RESET in vfio_device_info? something like:
> >>
> >> ret = cdx_dev_reset(core_vdev->dev);
> >> if (ret == -EOPNOTSUPP)
> >> /* make sure VFIO_DEVICE_FLAGS_RESET is not returned in
> >> * flags for device get info */
> >> else if (ret)
> >> kfree(vdev->regions);
> >>
> >> From new device feature added for BUS mastering if cdx_clear_master()
> >> fails due to no support, the bus driver will return -EOPNOTSUPP, so same
> >> would be communicated to the user-space, so it seems fine from this end.
> >
> > It's inconsistent to the user to allow the bus master device feature
> > probe to indicate the feature is available if it's going to fail on
> > every call. My suggestion was specifically relative to that, a
> > gratuitous call to clear bus master, determine if the call works, then
> > the feature probe could succeed or fail based on that result.
> >
> > However, now that I look at cdx_dev_reset() I notice the inconsistency
> > with dev_configure. The reset path unconditionally calls
> > dev_configure, but the bus master paths tests dev_configure. Is
> > dev_configure a required op or not? Are reset and bus master control
> > required features of CDX? If the core CDX code requires these then the
> > vfio support gets easier, we don't need to make all these conditional.
>
> Hi Alex,
>
> dev_configure is a required op for CDX bus controller. The check in
> cdx_set_master()/cdx_clear_master() is just precautionary and can be
> removed.
>
> On the other part where you mention making device feature optional, I
> was not able to locate any API/flags to export capabilities to the VFIO
> user regarding the features supported by the device. Though it is not
> required as all CDX devices would support the BUS mastering.
The flags for the feature itself is how the user determines whether the
feature is available. For example here we're expecting the user to
call with flags (VFIO_DEVICE_FEATURE_PROBE | VFIO_DEVICE_FEATURE_SET)
to determine the VFIO_DEVICE_FEATURE_BUS_MASTER is available. This is
handled automatically by the boilerplate usage of vfio_check_feature().
In this series we introduce the possibility that there might be no
dev_configure callback for a device, which would create a scenario were
the vfio device feature probing indicates support for a feature that
maybe isn't actually present. Then, even if dev_configure is
supported and required, it's just multiplexing the specific op via a
switch statement, so we need to make a leap of faith whether every
future dev_configure implementation will support these ops.
I wonder if dev_configure is really necessary or it wouldn't be better
to to have .reset and .bus_master callbacks on struct cdx_ops. Then
the cdx subsystem could properly enforce required callbacks. Thanks,
Alex