diff mbox series

[v1,21/22] vfio/pci: Allow the selection of a given iommu backend

Message ID 20230830103754.36461-22-zhenzhong.duan@intel.com
State New
Headers show
Series vfio: Adopt iommufd | expand

Commit Message

Zhenzhong Duan Aug. 30, 2023, 10:37 a.m. UTC 
From: Eric Auger <eric.auger@redhat.com>

Now we support two types of iommu backends, let's add the capability
to select one of them. This depends on whether an iommufd object has
been linked with the vfio-pci device:

if the user wants to use the legacy backend, it shall not
link the vfio-pci device with any iommufd object:

-device vfio-pci,host=0000:02:00.0

This is called the legacy mode/backend.

If the user wants to use the iommufd backend (/dev/iommu) it
shall pass an iommufd object id in the vfio-pci device options:

 -object iommufd,id=iommufd0
 -device vfio-pci,host=0000:02:00.0,iommufd=iommufd0

Note the /dev/iommu device may have been pre-opened by a
management tool such as libvirt. This mode is no more considered
for the legacy backend. So let's remove the "TODO" comment.

Suggested-by: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/vfio/pci.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Jason Gunthorpe Sept. 6, 2023, 6:10 p.m. UTC | #1 
On Wed, Aug 30, 2023 at 06:37:53PM +0800, Zhenzhong Duan wrote:
> Note the /dev/iommu device may have been pre-opened by a
> management tool such as libvirt. This mode is no more considered
> for the legacy backend. So let's remove the "TODO" comment.

Can you show an example of that syntax too?

Also, the vfio device should be openable externally as well

Jason
Alex Williamson Sept. 6, 2023, 7:09 p.m. UTC | #2 
On Wed, 6 Sep 2023 15:10:39 -0300
Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Wed, Aug 30, 2023 at 06:37:53PM +0800, Zhenzhong Duan wrote:
> > Note the /dev/iommu device may have been pre-opened by a
> > management tool such as libvirt. This mode is no more considered
> > for the legacy backend. So let's remove the "TODO" comment.  
> 
> Can you show an example of that syntax too?

Unless you're just looking for something in the commit log, patch 16/
added the following to the qemu help output:

+#ifdef CONFIG_IOMMUFD
+    ``-object iommufd,id=id[,fd=fd]``
+        Creates an iommufd backend which allows control of DMA mapping
+        through the /dev/iommu device.
+
+        The ``id`` parameter is a unique ID which frontends (such as
+        vfio-pci of vdpa) will use to connect withe the iommufd backend.
+
+        The ``fd`` parameter is an optional pre-opened file descriptor
+        resulting from /dev/iommu opening. Usually the iommufd is shared
+        accross all subsystems, bringing the benefit of centralized
+        reference counting.
+#endif
 
> Also, the vfio device should be openable externally as well

Appears to be added in the very next patch in the series.  Thanks,

Alex
Jason Gunthorpe Sept. 7, 2023, 1:10 a.m. UTC | #3 
On Wed, Sep 06, 2023 at 01:09:26PM -0600, Alex Williamson wrote:
> On Wed, 6 Sep 2023 15:10:39 -0300
> Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> > On Wed, Aug 30, 2023 at 06:37:53PM +0800, Zhenzhong Duan wrote:
> > > Note the /dev/iommu device may have been pre-opened by a
> > > management tool such as libvirt. This mode is no more considered
> > > for the legacy backend. So let's remove the "TODO" comment.  
> > 
> > Can you show an example of that syntax too?
> 
> Unless you're just looking for something in the commit log, 

Yeah, I was thinking the commit log

> patch 16/ added the following to the qemu help output:
> 
> +#ifdef CONFIG_IOMMUFD
> +    ``-object iommufd,id=id[,fd=fd]``
> +        Creates an iommufd backend which allows control of DMA mapping
> +        through the /dev/iommu device.
> +
> +        The ``id`` parameter is a unique ID which frontends (such as
> +        vfio-pci of vdpa) will use to connect withe the iommufd backend.
> +
> +        The ``fd`` parameter is an optional pre-opened file descriptor
> +        resulting from /dev/iommu opening. Usually the iommufd is shared
> +        accross all subsystems, bringing the benefit of centralized
> +        reference counting.
> +#endif
>  
> > Also, the vfio device should be openable externally as well
> 
> Appears to be added in the very next patch in the series.  Thanks,

Indeed, I got confused because this removed the TODO - that could
reasonably be pushed to the next patch and include a bit more detail
in the commit message

Jason
Zhenzhong Duan Sept. 7, 2023, 2:27 a.m. UTC | #4 
>-----Original Message-----
>From: Jason Gunthorpe <jgg@nvidia.com>
>Sent: Thursday, September 7, 2023 9:11 AM
>To: Alex Williamson <alex.williamson@redhat.com>
>Subject: Re: [PATCH v1 21/22] vfio/pci: Allow the selection of a given iommu
>backend
>
>On Wed, Sep 06, 2023 at 01:09:26PM -0600, Alex Williamson wrote:
>> On Wed, 6 Sep 2023 15:10:39 -0300
>> Jason Gunthorpe <jgg@nvidia.com> wrote:
>>
>> > On Wed, Aug 30, 2023 at 06:37:53PM +0800, Zhenzhong Duan wrote:
>> > > Note the /dev/iommu device may have been pre-opened by a
>> > > management tool such as libvirt. This mode is no more considered
>> > > for the legacy backend. So let's remove the "TODO" comment.
>> >
>> > Can you show an example of that syntax too?
>>
>> Unless you're just looking for something in the commit log,
>
>Yeah, I was thinking the commit log
>
>> patch 16/ added the following to the qemu help output:
>>
>> +#ifdef CONFIG_IOMMUFD
>> +    ``-object iommufd,id=id[,fd=fd]``
>> +        Creates an iommufd backend which allows control of DMA mapping
>> +        through the /dev/iommu device.
>> +
>> +        The ``id`` parameter is a unique ID which frontends (such as
>> +        vfio-pci of vdpa) will use to connect withe the iommufd backend.
>> +
>> +        The ``fd`` parameter is an optional pre-opened file descriptor
>> +        resulting from /dev/iommu opening. Usually the iommufd is shared
>> +        accross all subsystems, bringing the benefit of centralized
>> +        reference counting.
>> +#endif

Thanks for point out this issue.
I can think of two choices:
1. squash this patch to PATCH16
2. keep this patch separate and to pull fd passing related change from PATCH16 into this one
Please kindly suggest which way is preferred in community.

Btw: I only enable fd passing for vfio pci device, let me know if it's preferred
to include all other vfio devices in this series, then I'll add them.

>>
>> > Also, the vfio device should be openable externally as well
>>
>> Appears to be added in the very next patch in the series.  Thanks,
>
>Indeed, I got confused because this removed the TODO - that could
>reasonably be pushed to the next patch and include a bit more detail
>in the commit message

Good idea, will fix.

Thanks
Zhenzhong
diff mbox series

Patch

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 3a8fee3c99..99265253f8 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -43,6 +43,7 @@ 
 #include "migration/blocker.h"
 #include "migration/qemu-file.h"
 #include "linux/iommufd.h"
+#include "sysemu/iommufd.h"
 
 #define TYPE_VFIO_PCI_NOHOTPLUG "vfio-pci-nohotplug"
 
@@ -3611,11 +3612,10 @@  static Property vfio_pci_dev_properties[] = {
                                    qdev_prop_nv_gpudirect_clique, uint8_t),
     DEFINE_PROP_OFF_AUTO_PCIBAR("x-msix-relocation", VFIOPCIDevice, msix_relo,
                                 OFF_AUTOPCIBAR_OFF),
-    /*
-     * TODO - support passed fds... is this necessary?
-     * DEFINE_PROP_STRING("vfiofd", VFIOPCIDevice, vfiofd_name),
-     * DEFINE_PROP_STRING("vfiogroupfd, VFIOPCIDevice, vfiogroupfd_name),
-     */
+#ifdef CONFIG_IOMMUFD
+    DEFINE_PROP_LINK("iommufd", VFIOPCIDevice, vbasedev.iommufd,
+                     TYPE_IOMMUFD_BACKEND, IOMMUFDBackend *),
+#endif
     DEFINE_PROP_END_OF_LIST(),
 };