Hi Joao, On 7/12/24 13:46, Joao Martins wrote: > Fetch IOMMU hw raw caps behind the device and thus move the what does mean "Fetch IOMMU hw raw caps behind the device'" > 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 what do you mean by " It allows it to cache the information obtained from IOMMU_GET_HW_INFO from iommufd early on" > iommufd early on. However, while legacy HostIOMMUDevice caps what does mean "legacy HostIOMMUDevice caps always return true"? > 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 fetch parse? > dirty tracking is supported by device backing IOMMU without necessarily > duplicating the amount of calls we do to IOMMU_GET_HW_INFO. But we move code from generic place to BE specific place?
Sorry I feel really hard to understand the commit msg in general Eric > > Suggested-by: Cédric Le Goater <[email protected]> > Signed-off-by: Joao Martins <[email protected]> > --- > 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; please also update the doc comment > } 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; > + } > + > 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; > }
