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 <c...@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.d...@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.