Messages in this thread | | | Date | Tue, 8 Aug 2023 16:09:30 +0530 | Subject | Re: [PATCH v5 2/3] vfio: add bus master feature to device feature ioctl | From | "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 -------- */ >> >> /** >
| |