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@xxxxxxx> 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@xxxxxxx>
> > Signed-off-by: Shubham Rohila <shubham.rohila@xxxxxxx>
> > Signed-off-by: Nipun Gupta <nipun.gupta@xxxxxxx>
> >
> > 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 -------- */
> >   
> >   /**