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

[Alex Williamson <alex.williamson@redhat.com>]

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