Re: [PATCH rfcv2 03/20] HostIOMMUDevice: Introduce realize_late callback

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

On 4/9/25 10:27, Duan, Zhenzhong wrote:
> 
> 
>> -----Original Message-----
>> From: Cédric Le Goater <clg@redhat.com>
>> Subject: Re: [PATCH rfcv2 03/20] HostIOMMUDevice: Introduce realize_late
>> callback
>>
>> On 4/7/25 13:19, Cédric Le Goater wrote:
>>> On 2/19/25 09:22, Zhenzhong Duan wrote:
>>>> Currently we have realize() callback which is called before attachment.
>>>> But there are still some elements e.g., hwpt_id is not ready before
>>>> attachment. So we need a realize_late() callback to further initialize
>>>> them.
>>>
>>> The relation between objects HostIOMMUDevice and VFIOIOMMU is starting
>>> to look too complex for me.
> 
> Agree.
> 
>>>
>>> I think it makes sense to realize HostIOMMUDevice after the device
>>> is attached. Can't we move :
>>>
>>>           hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename));
>>>           vbasedev->hiod = hiod;
>>>
>>> under ->attach_device() and also the call :
>>>
>>>       if (!vfio_device_hiod_realize(vbasedev, errp)) {
>>>
>>> later in the ->attach_device() patch ?
>>>
>>> hiod_legacy_vfio_realize() doesn't do much. We might need to rework
>>> hiod_iommufd_vfio_realize() which queries the iommufd hw caps, later
>>> used by intel-iommu.
>>
>> The only dependency I see on the IOMMUFD HostIOMMUDevice when attaching
>> the device to the container is in iommufd_cdev_autodomains_get(). The
>> flags for IOMMU_HWPT_ALLOC depends on the HW capability of the IOMMFD
>> backend and we rely on hiod_iommufd_vfio_realize() to have done the
>> query on the iommufd kernel device before.
>>
>> Since this is not a hot path, I don't think it is a problem to add
>> a redundant call to iommufd_backend_get_device_info() in
>> iommufd_cdev_autodomains_get() and avoid the IOMMUFD HostIOMMUDevice
>> dependency. With that we can move the HostIOMMUDevice creation and
>> realize sequence at the end of the device attach sequence.
> 
> Yes.
> 
>>
>> I think this makes the code cleaner when it comes to using the
>> vbasedev->hiod pointer too.
>>
>>> Anyway, it is good time to cleanup our interfaces before adding more.
> 
> OK, let me think about this further and write some patches to move .realize() after .attach_device().
> will be based on vfio-next.

I just updated the vfio-next branch with what should be in the next PR
for QEMU 10.1.

> 
>>
>> On that topic, I think
>>
>>     iommufd_cdev_attach_ioas_hwpt
>>     iommufd_cdev_detach_ioas_hwpt
>>
>> belong to IOMMUFD backend.
> 
> They are operation on VFIODevice, backends/iommufd.c are for operation on IOMMUFDBackend,
> Do we need to move iommufd_cdev_attach/detach_ioas_hwpt to backends/iommufd.c which is VFIODevice agnostic?

My mistake. I was confused with

   int iommufd = vbasedev->iommufd->fd

and thought we could simply replace 'VFIODevice *' parameter with a
'IOMMUFDBackend *be' parameter but this is not the case.

Thanks,

C.