Re: [PATCH v6 3/3] vfio-cdx: add bus mastering device feature support
From: Gupta, Nipun
Date: Thu Aug 24 2023 - 06:58:20 EST
On 8/22/2023 2:37 AM, Alex Williamson wrote:
On Mon, 21 Aug 2023 16:27:40 +0530
"Gupta, Nipun" <nipun.gupta@xxxxxxx> wrote:
On 8/18/2023 8:07 PM, Alex Williamson wrote:
On Fri, 18 Aug 2023 14:02:32 +0530
"Gupta, Nipun" <nipun.gupta@xxxxxxx> wrote:
On 8/16/2023 11:16 PM, Alex Williamson wrote:
On Thu, 10 Aug 2023 14:14:09 +0530
Nipun Gupta <nipun.gupta@xxxxxxx> 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@xxxxxxx>
Signed-off-by: Shubham Rohila <shubham.rohila@xxxxxxx>
Signed-off-by: Nipun Gupta <nipun.gupta@xxxxxxx>
---
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.
Hi Alex,
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.
Although there are no CDX devices which do not support BME feature, but
you are correct that we should provide correct information regarding the
device feature support when (VFIO_DEVICE_FEATURE_PROBE |
VFIO_DEVICE_FEATURE_SET) flags are used. We will update this in the next
version.
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.
dev_configure is per CDX controller, but there can be possibility some
devices on same controller support a callback feature but others does
not. So current implementation also seems to be fine.
Thanks,
Nipun
Thanks,
Alex