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

["Cédric Le Goater" <clg@redhat.com>]

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.