RE: [PATCH v3 1/7] HostIOMMUDevice: Store the VFIO/VDPA agent

[Duan, Zhenzhong]

Hi Cédric,

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Sent: Friday, June 14, 2024 6:05 PM
>To: eric.auger@redhat.com; eric.auger.pro@gmail.com; qemu-
>devel@nongnu.org; qemu-arm@nongnu.org; mst@redhat.com; jean-
>philippe@linaro.org; peter.maydell@linaro.org; yanghliu@redhat.com; Duan,
>Zhenzhong <zhenzhong.duan@intel.com>
>Cc: alex.williamson@redhat.com; jasowang@redhat.com;
>pbonzini@redhat.com; berrange@redhat.com
>Subject: Re: [PATCH v3 1/7] HostIOMMUDevice: Store the VFIO/VDPA agent
>
>
>>> Talking of which, why are we passing a 'VFIODevice *' parameter to
>>> HostIOMMUDeviceClass::realize ? I don't see a good reason
>>>
>>> I think a 'VFIOContainerBase *' would be more appropriate since
>>> 'HostIOMMUDevice' represents a device on the host which is common
>>> to all VFIO devices.
>>>
>>> In that case, HostIOMMUDevice::agent wouldn't need to be opaque
>>> anymore. It could  simply be a 'VFIOContainerBase *' and
>>> hiod_legacy_vfio_get_iova_ranges() in patch 3 would grab the
>>> 'iova_ranges' from the 'VFIOContainerBase *' directly.
>>>
>>> This means some rework :
>>>
>>> * vfio_device_get_aw_bits() would use a  'VFIOContainerBase *' instead.
>>> * HostIOMMUDevice::name would be removed. This is just for error
>>> messages.
>>> * hiod_iommufd_vfio_realize() would use VFIOIOMMUFDContainer::be.
>>>
>>> That said, I think we need the QOMification changes first.
>>
>> OK I need to review your series first. At the moment I have just
>> addressed Zhenzhong's comment in v4, just sent.
>
>Yep. Just take a look at mine. If both of you agree with above
>proposal, I can care of it and resend all 3. It's a small change.

I would suggest using opaque pointer and VFIODevice for two reasons,
1. in nesting series vIOMMU needs to attach/detaching hwpt which is VFIODevice 
operations.
See 
https://github.com/yiliu1765/qemu/commit/3ca559d35adc9840555e361a56708af4c6338b3d

2. If we plan to support VDPA Device in future, the opaque pointer can also 
point
to an VDPADevice structure.

Thanks
Zhenzhong