On 16/07/2024 11:20, Cédric Le Goater wrote: > On 7/12/24 13:46, Joao Martins wrote: >> Fetch IOMMU hw raw caps behind the device and thus move the >> HostIOMMUDevice::realize() to be done during the attach of the device. It >> allows it to cache the information obtained from IOMMU_GET_HW_INFO from >> iommufd early on. However, while legacy HostIOMMUDevice caps >> always return true and doesn't have dependency on other things, the IOMMUFD >> backend requires the iommufd FD to be connected and having a devid to be >> able to query capabilities. Hence when exactly is HostIOMMUDevice >> initialized inside backend ::attach_device() implementation is backend >> specific. >> >> This is in preparation to fetch parse hw capabilities and understand if >> dirty tracking is supported by device backing IOMMU without necessarily >> duplicating the amount of calls we do to IOMMU_GET_HW_INFO. >> >> Suggested-by: Cédric Le Goater <c...@redhat.com> >> Signed-off-by: Joao Martins <joao.m.mart...@oracle.com> >> --- >> include/sysemu/host_iommu_device.h | 1 + >> hw/vfio/common.c | 16 ++++++---------- >> hw/vfio/container.c | 6 ++++++ >> hw/vfio/iommufd.c | 7 +++++++ >> 4 files changed, 20 insertions(+), 10 deletions(-) >> >> diff --git a/include/sysemu/host_iommu_device.h >> b/include/sysemu/host_iommu_device.h >> index 20e77cf54568..b1e5f4b8ac3e 100644 >> --- a/include/sysemu/host_iommu_device.h >> +++ b/include/sysemu/host_iommu_device.h >> @@ -24,6 +24,7 @@ >> */ >> typedef struct HostIOMMUDeviceCaps { >> uint32_t type; >> + uint64_t hw_caps; >> } HostIOMMUDeviceCaps; >> #define TYPE_HOST_IOMMU_DEVICE "host-iommu-device" >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index b0beed44116e..cc14f0e3fe24 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -1544,7 +1544,7 @@ bool vfio_attach_device(char *name, VFIODevice >> *vbasedev, >> { >> const VFIOIOMMUClass *ops = >> VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY)); >> - HostIOMMUDevice *hiod; >> + HostIOMMUDevice *hiod = NULL; >> if (vbasedev->iommufd) { >> ops = >> VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD)); >> @@ -1552,21 +1552,17 @@ bool vfio_attach_device(char *name, VFIODevice >> *vbasedev, >> assert(ops); >> - if (!ops->attach_device(name, vbasedev, as, errp)) { >> - return false; >> - } >> - if (vbasedev->mdev) { >> - return true; >> + if (!vbasedev->mdev) { >> + hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename)); >> + vbasedev->hiod = hiod; >> } >> - hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename)); >> - if (!HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp)) { >> + if (!ops->attach_device(name, vbasedev, as, errp)) { >> object_unref(hiod); >> - ops->detach_device(vbasedev); >> + vbasedev->hiod = NULL; >> return false; >> } >> - vbasedev->hiod = hiod; >> return true; >> } >> diff --git a/hw/vfio/container.c b/hw/vfio/container.c >> index c27f448ba26e..29da261bbf3e 100644 >> --- a/hw/vfio/container.c >> +++ b/hw/vfio/container.c >> @@ -907,6 +907,7 @@ static bool vfio_legacy_attach_device(const char *name, >> VFIODevice *vbasedev, >> AddressSpace *as, Error **errp) >> { >> int groupid = vfio_device_groupid(vbasedev, errp); >> + HostIOMMUDevice *hiod = vbasedev->hiod; >> VFIODevice *vbasedev_iter; >> VFIOGroup *group; >> VFIOContainerBase *bcontainer; >> @@ -917,6 +918,11 @@ static bool vfio_legacy_attach_device(const char *name, >> VFIODevice *vbasedev, >> trace_vfio_attach_device(vbasedev->name, groupid); >> + if (hiod && >> + !HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp)) { >> + return false; >> + } >> + > > > Could you please introduce an helper : > > bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp); >
Yeah, let me do that > > Thanks, > > C. > > > >> group = vfio_get_group(groupid, as, errp); >> if (!group) { >> return false; >> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c >> index 873c919e319c..d34dc88231ec 100644 >> --- a/hw/vfio/iommufd.c >> +++ b/hw/vfio/iommufd.c >> @@ -384,6 +384,7 @@ static bool iommufd_cdev_attach(const char *name, >> VFIODevice *vbasedev, >> Error *err = NULL; >> const VFIOIOMMUClass *iommufd_vioc = >> VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD)); >> + HostIOMMUDevice *hiod = vbasedev->hiod; >> if (vbasedev->fd < 0) { >> devfd = iommufd_cdev_getfd(vbasedev->sysfsdev, errp); >> @@ -401,6 +402,11 @@ static bool iommufd_cdev_attach(const char *name, >> VFIODevice *vbasedev, >> space = vfio_get_address_space(as); >> + if (hiod && >> + !HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp)) { >> + return false; >> + } >> + >> /* try to attach to an existing container in this space */ >> QLIST_FOREACH(bcontainer, &space->containers, next) { >> container = container_of(bcontainer, VFIOIOMMUFDContainer, >> bcontainer); >> @@ -722,6 +728,7 @@ static bool hiod_iommufd_vfio_realize(HostIOMMUDevice >> *hiod, void *opaque, >> hiod->name = g_strdup(vdev->name); >> caps->type = type; >> + caps->hw_caps = hw_caps; >> return true; >> } >