diff mbox series

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

Message ID 20230803143253.7817-2-nipun.gupta@amd.com
State New
Headers
Series [v5,1/3] cdx: add support for bus mastering |

Commit Message

Gupta, Nipun Aug. 3, 2023, 2:32 p.m. UTC 
  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(+)

Comments

Alex Williamson Aug. 3, 2023, 10 p.m. UTC | #1 
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 -------- */
>  
>  /**
Gupta, Nipun Aug. 8, 2023, 10:39 a.m. UTC | #2 
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 -------- */
>>   
>>   /**
>
diff mbox series

Patch

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.
+ */
+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 */
+};
+#define VFIO_DEVICE_FEATURE_BUS_MASTER 9
+
 /* -------- API for Type1 VFIO IOMMU -------- */
 
 /**