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

[Cédric Le Goater]

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.au...@redhat.com>
Reviewed-by: Zhenzhong Duan <zhenzhong.d...@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.

Thanks,

C.




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