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

["Gupta, Nipun" <nipun.gupta@amd.com>]

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