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

[Eric Auger <eric.auger@redhat.com>]

Hi Cédric,

On 6/14/24 11:13, Cédric Le Goater wrote:
> On 6/13/24 11:20 AM, Eric Auger wrote:
>> Store the agent device (VFIO or VDPA) in the host IOMMU device.
>> This will allow easy access to some of its resources.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   include/sysemu/host_iommu_device.h | 1 +
>>   hw/vfio/container.c                | 1 +
>>   hw/vfio/iommufd.c                  | 2 ++
>>   3 files changed, 4 insertions(+)
>>
>> diff --git a/include/sysemu/host_iommu_device.h
>> b/include/sysemu/host_iommu_device.h
>> index a57873958b..3e5f058e7b 100644
>> --- a/include/sysemu/host_iommu_device.h
>> +++ b/include/sysemu/host_iommu_device.h
>> @@ -34,6 +34,7 @@ struct HostIOMMUDevice {
>>       Object parent_obj;
>>         char *name;
>> +    void *agent; /* pointer to agent device, ie. VFIO or VDPA device */
>>       HostIOMMUDeviceCaps caps;
>>   };
>>   diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>> index 26e6f7fb4f..b728b978a2 100644
>> --- a/hw/vfio/container.c
>> +++ b/hw/vfio/container.c
>> @@ -1145,6 +1145,7 @@ static bool
>> hiod_legacy_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
>>         hiod->name = g_strdup(vdev->name);
>>       hiod->caps.aw_bits = vfio_device_get_aw_bits(vdev);
>> +    hiod->agent = opaque;
>>         return true;
>>   }
>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>> index 409ed3dcc9..dbdae1adbb 100644
>> --- a/hw/vfio/iommufd.c
>> +++ b/hw/vfio/iommufd.c
>> @@ -631,6 +631,8 @@ static bool
>> hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
>>           struct iommu_hw_info_vtd vtd;
>>       } data;
>>   +    hiod->agent = opaque;
>> +
>
> This opaque pointer could be assigned in vfio_attach_device().
>
> 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.

Thanks

Eric

>
> Thanks,
>
> C.
>
>
>
>
>>       if (!iommufd_backend_get_device_info(vdev->iommufd, vdev->devid,
>>                                            &type, &data,
>> sizeof(data), errp)) {
>>           return false;
>