Re: [PATCH rfcv1 4/6] vfio: initialize IOMMUFDDevice and pass to vIOMMU

[Cédric Le Goater]

On 1/23/24 10:46, Duan, Zhenzhong wrote:
> > -----Original Message-----
> > From: Cédric Le Goater <clg@redhat.com>
> > Subject: Re: [PATCH rfcv1 4/6] vfio: initialize IOMMUFDDevice and pass to
> > vIOMMU
> >
> > On 1/15/24 11:13, Zhenzhong Duan wrote:
> > > Initialize IOMMUFDDevice in vfio and pass to vIOMMU, so that vIOMMU
> > > could get hw IOMMU information.
> > >
> > > In VFIO legacy backend mode, we still pass a NULL IOMMUFDDevice to
> > vIOMMU,
> > > in case vIOMMU needs some processing for VFIO legacy backend mode.
> > >
> > > Originally-by: Yi Liu <yi.l.liu@intel.com>
> > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> > > Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> > > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> > > ---
> > >    include/hw/vfio/vfio-common.h |  2 ++
> > >    hw/vfio/iommufd.c             |  2 ++
> > >    hw/vfio/pci.c                 | 24 +++++++++++++++++++-----
> > >    3 files changed, 23 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
> > common.h
> > > index 9b7ef7d02b..fde0d0ca60 100644
> > > --- a/include/hw/vfio/vfio-common.h
> > > +++ b/include/hw/vfio/vfio-common.h
> > > @@ -31,6 +31,7 @@
> > >    #endif
> > >    #include "sysemu/sysemu.h"
> > >    #include "hw/vfio/vfio-container-base.h"
> > > +#include "sysemu/iommufd_device.h"
> > >
> > >    #define VFIO_MSG_PREFIX "vfio %s: "
> > >
> > > @@ -126,6 +127,7 @@ typedef struct VFIODevice {
> > >        bool dirty_tracking;
> > >        int devid;
> > >        IOMMUFDBackend *iommufd;
> > > +    IOMMUFDDevice idev;
> > >    } VFIODevice;
> > >
> > >    struct VFIODeviceOps {
> > > diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> > > index 9bfddc1360..cbd035f148 100644
> > > --- a/hw/vfio/iommufd.c
> > > +++ b/hw/vfio/iommufd.c
> > > @@ -309,6 +309,7 @@ static int iommufd_cdev_attach(const char *name,
> > VFIODevice *vbasedev,
> > >        VFIOContainerBase *bcontainer;
> > >        VFIOIOMMUFDContainer *container;
> > >        VFIOAddressSpace *space;
> > > +    IOMMUFDDevice *idev = &vbasedev->idev;
> > >        struct vfio_device_info dev_info = { .argsz = sizeof(dev_info) };
> > >        int ret, devfd;
> > >        uint32_t ioas_id;
> > > @@ -428,6 +429,7 @@ found_container:
> > >        QLIST_INSERT_HEAD(&bcontainer->device_list, vbasedev,
> > container_next);
> > >        QLIST_INSERT_HEAD(&vfio_device_list, vbasedev, global_next);
> > >
> > > +    iommufd_device_init(idev, sizeof(*idev), container->be, vbasedev-
> > > devid);
> > >        trace_iommufd_cdev_device_info(vbasedev->name, devfd, vbasedev-
> > > num_irqs,
> > >                                       vbasedev->num_regions, vbasedev->flags);
> > >        return 0;
> > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > > index d7fe06715c..2c3a5d267b 100644
> > > --- a/hw/vfio/pci.c
> > > +++ b/hw/vfio/pci.c
> > > @@ -3107,11 +3107,21 @@ static void vfio_realize(PCIDevice *pdev,
> > Error **errp)
> > >        vfio_bars_register(vdev);
> > >
> > > -    ret = vfio_add_capabilities(vdev, errp);
> > > +    if (vbasedev->iommufd) {
> > > +        ret = pci_device_set_iommu_device(pdev, &vbasedev->idev, errp);
> > > +    } else {
> > > +        ret = pci_device_set_iommu_device(pdev, 0, errp);
> >
> >
> > AFAICT, pci_device_set_iommu_device() with a NULL IOMMUFDDevice will
> > do
> > nothing. Why call it ?
>
> We will do something in nesting series, see 
> https://github.com/yiliu1765/qemu/commit/7f0bb59575bb5cf38618ae950f68a8661676e881

ok, that's not much. idev is used as a capability bool and later on
to pass the /dev/iommu fd.  We don't need to support the legacy mode ?

> Another choice is to call pci_device_set_iommu_device() no matter which backend
> is used and check idev->iommufd in vtd_dev_set_iommu_device(). Is this better
> for you?

yes. Should be fine. There is more to it though.

IIUC, what will determine most of the requirements, is the legacy
mode. We also need the host iommu info in that case. As said Eric,
ideally, we should introduce a common abstract "host-iommu-info" struct
and sub structs associated with the iommu backends (iommufd + legacy)
which would be allocated accordingly.

So, IOMMUFDDevice usage should be limited to the iommufd files. All PCI
files should use the common abstract type. We should define these data
structures first. They could be simple C struct for now. We will see if
QOM applies after.

Will take a look at Eric's patchset next.

Thanks,

C.