RE: [PATCH v1 05/11] vfio: Introduce host_iommu_device_create callback

["Duan, Zhenzhong" <zhenzhong.duan@intel.com>]

>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Subject: Re: [PATCH v1 05/11] vfio: Introduce host_iommu_device_create
>callback
>
>
>
>On 2/28/24 04:58, Zhenzhong Duan wrote:
>> Introduce host_iommu_device_create callback and a wrapper for it.
>>
>> This callback is used to allocate a host iommu device instance and
>> initialize it based on type.
>>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>  include/hw/vfio/vfio-common.h         | 1 +
>>  include/hw/vfio/vfio-container-base.h | 1 +
>>  hw/vfio/common.c                      | 8 ++++++++
>>  3 files changed, 10 insertions(+)
>>
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>common.h
>> index b6676c9f79..9fefea4b89 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -208,6 +208,7 @@ struct vfio_device_info *vfio_get_device_info(int
>fd);
>>  int vfio_attach_device(char *name, VFIODevice *vbasedev,
>>                         AddressSpace *as, Error **errp);
>>  void vfio_detach_device(VFIODevice *vbasedev);
>> +void host_iommu_device_create(VFIODevice *vbasedev);
>>
>>  int vfio_kvm_device_add_fd(int fd, Error **errp);
>>  int vfio_kvm_device_del_fd(int fd, Error **errp);
>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-
>container-base.h
>> index b2813b0c11..dc003f6eb2 100644
>> --- a/include/hw/vfio/vfio-container-base.h
>> +++ b/include/hw/vfio/vfio-container-base.h
>> @@ -120,6 +120,7 @@ struct VFIOIOMMUClass {
>>      int (*attach_device)(const char *name, VFIODevice *vbasedev,
>>                           AddressSpace *as, Error **errp);
>>      void (*detach_device)(VFIODevice *vbasedev);
>> +    void (*host_iommu_device_create)(VFIODevice *vbasedev);
>Maybe return an int instead. It is common the allocation can fail and
>the deallocation cannot. While at it I would also pass an errp in case
>it fails

Currently host_iommu_device_create implementation only calls g_malloc0,
so never fails, so I returned void.

I'm fine to return an int, will be like below, take iommufd for example:

--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -651,7 +651,7 @@ static IOMMUFDDeviceOps vfio_iommufd_device_ops = {
     .detach_hwpt = vfio_iommufd_device_detach_hwpt,
 };

-static void vfio_cdev_host_iommu_device_create(VFIODevice *vbasedev)
+static int vfio_cdev_host_iommu_device_create(VFIODevice *vbasedev, Error **errp)
 {
     IOMMUFDDevice *idev = g_malloc0(sizeof(IOMMUFDDevice));
     VFIOIOMMUFDContainer *container = container_of(vbasedev->bcontainer,
@@ -661,6 +661,8 @@ static void vfio_cdev_host_iommu_device_create(VFIODevice *vbasedev)

     iommufd_device_init(idev, vbasedev->iommufd, vbasedev->devid,
                         container->ioas_id, vbasedev, &vfio_iommufd_device_ops);
+
+    return 0;
 }

Thanks
Zhenzhong

>
>Eric
>>      /* migration feature */
>>      int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
>>                                     bool start);
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 059bfdc07a..41e9031c59 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -1521,3 +1521,11 @@ void vfio_detach_device(VFIODevice
>*vbasedev)
>>      }
>>      vbasedev->bcontainer->ops->detach_device(vbasedev);
>>  }
>> +
>> +void host_iommu_device_create(VFIODevice *vbasedev)
>> +{
>> +    const VFIOIOMMUClass *ops = vbasedev->bcontainer->ops;
>> +
>> +    assert(ops->host_iommu_device_create);
>> +    ops->host_iommu_device_create(vbasedev);
>> +}