Re: [PATCH v3 07/37] vfio/container: Introduce a empty VFIOIOMMUOps

[Cédric Le Goater]

On 10/30/23 03:43, Duan, Zhenzhong wrote:
> > -----Original Message-----
> > From: Cédric Le Goater <clg@redhat.com>
> > Sent: Friday, October 27, 2023 10:21 PM
> > Subject: Re: [PATCH v3 07/37] vfio/container: Introduce a empty
> > VFIOIOMMUOps
> >
> > On 10/26/23 12:30, Zhenzhong Duan wrote:
> > > This empty VFIOIOMMUOps named vfio_legacy_ops will hold all general
> > > IOMMU ops of legacy container.
> > >
> > > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> > > ---
> > >    include/hw/vfio/vfio-common.h | 2 +-
> > >    hw/vfio/container.c           | 5 +++++
> > >    2 files changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> > > index d8f293cb57..8ded5cd8e4 100644
> > > --- a/include/hw/vfio/vfio-common.h
> > > +++ b/include/hw/vfio/vfio-common.h
> > > @@ -255,7 +255,7 @@ typedef QLIST_HEAD(VFIOGroupList, VFIOGroup)
> > VFIOGroupList;
> > >    typedef QLIST_HEAD(VFIODeviceList, VFIODevice) VFIODeviceList;
> > >    extern VFIOGroupList vfio_group_list;
> > >    extern VFIODeviceList vfio_device_list;
> > > -
> > > +extern const VFIOIOMMUOps vfio_legacy_ops;
> >
> >
> > why does it need to be external ?
>
> It is referenced by vfio_connect_container() and vfio_attach_device().

Yes. I realized that later on.

The backend is chosen when the device id attached :
  
    int vfio_attach_device(char *name, VFIODevice *vbasedev,
                           AddressSpace *as, Error **errp)
    {
        const VFIOIOMMUOps *ops;
  
        if (vbasedev->iommufd) {
            ops = &vfio_iommufd_ops;
        } else  {
            ops = &vfio_legacy_ops;
        }
        return ops->attach_device(name, vbasedev, as, errp);
    }

To be noted that we don't need the backend ops but the attach_device()
handler only.

And then, the backend ops is assigned to the base container deeper
in the call stack with vfio_container_init(), which is a bit like a
chicken and egg problem to me.

    vfio_legacy_ops.attach_device = vfio_legacy_attach_device()
      vfio_get_group()
        vfio_connect_container()
           vfio_container_init(&vfio_legacy_ops)
  
    vfio_iommufd_ops.attach_device = iommufd_attach_device()
      vfio_container_init(&vfio_iommufd_ops)
  
vfio_legacy_attach_device() and iommufd_attach_device() are similar but
have different requirements. I don't see a good alternative. Unless we
introduce a QOM object wrapping the IOMMUFDBackend and the legacy one
to hold the VFIOIOMMUOps struct. Looks overkill.

Thanks,

C.