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

[Eric Auger]

Hi Zhenzhong,

On 3/19/24 06:44, Duan, Zhenzhong wrote:
>
>> -----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;
>  }
I think the rationale behind introducing an error handle is to propagate
it down to the callbacks. This is were I anticipated errors to occur.
But if you don't foresee any maybe we can just wait and postpone this
addition.

Thanks

Eric
>
> 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);
>>> +}