[v3] vfio: Adopt iommufd

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

Posted by Zhenzhong Duan 1 year, 1 month ago

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;
 extern const MemoryListener vfio_memory_listener;
 extern int vfio_kvm_device_fd;
 
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 242010036a..4bc43ddfa4 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -472,6 +472,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
                                   Error **errp)
 {
     VFIOContainer *container;
+    VFIOContainerBase *bcontainer;
     int ret, fd;
     VFIOAddressSpace *space;
 
@@ -552,6 +553,8 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
     container->iova_ranges = NULL;
     QLIST_INIT(&container->giommu_list);
     QLIST_INIT(&container->vrdl_list);
+    bcontainer = &container->bcontainer;
+    bcontainer->ops = &vfio_legacy_ops;
 
     ret = vfio_init_container(container, group->fd, errp);
     if (ret) {
@@ -933,3 +936,5 @@ void vfio_detach_device(VFIODevice *vbasedev)
     vfio_put_base_device(vbasedev);
     vfio_put_group(group);
 }
+
+const VFIOIOMMUOps vfio_legacy_ops;
-- 
2.34.1

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

Posted by Cédric Le Goater 1 year, 1 month ago

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 ?

Thanks,

C.


>   extern const MemoryListener vfio_memory_listener;
>   extern int vfio_kvm_device_fd;
>   
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index 242010036a..4bc43ddfa4 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -472,6 +472,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>                                     Error **errp)
>   {
>       VFIOContainer *container;
> +    VFIOContainerBase *bcontainer;
>       int ret, fd;
>       VFIOAddressSpace *space;
>   
> @@ -552,6 +553,8 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>       container->iova_ranges = NULL;
>       QLIST_INIT(&container->giommu_list);
>       QLIST_INIT(&container->vrdl_list);
> +    bcontainer = &container->bcontainer;
> +    bcontainer->ops = &vfio_legacy_ops;
>   
>       ret = vfio_init_container(container, group->fd, errp);
>       if (ret) {
> @@ -933,3 +936,5 @@ void vfio_detach_device(VFIODevice *vbasedev)
>       vfio_put_base_device(vbasedev);
>       vfio_put_group(group);
>   }
> +
> +const VFIOIOMMUOps vfio_legacy_ops;

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

Posted by Duan, Zhenzhong 1 year ago


>-----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().

Thanks
Zhenzhong

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

Posted by Cédric Le Goater 1 year ago

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.