| Message ID | 20240719120501.81279-9-joao.m.martins@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/iommufd: IOMMUFD Dirty Tracking | expand |
On 7/19/24 14:04, Joao Martins wrote: > Move the HostIOMMUDevice::realize() to be invoked during the attach of the device > before we allocate IOMMUFD hardware pagetable objects (HWPT). This allows the use > of the hw_caps obtained by IOMMU_GET_HW_INFO that essentially tell if the IOMMU > behind the device supports dirty tracking. > > Note: The HostIOMMUDevice data from legacy backend is static and doesn't > need any information from the (type1-iommu) backend to be initialized. > In contrast however, the IOMMUFD HostIOMMUDevice data requires the > iommufd FD to be connected and having a devid to be able to successfully > GET_HW_INFO. This means vfio_device_hiod_realize() is called in > different places within the backend .attach_device() implementation. > > Suggested-by: Cédric Le Goater <clg@redhat.cm> > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> Reviewed-by: Cédric Le Goater <clg@redhat.com> Thanks, C. > --- > include/hw/vfio/vfio-common.h | 1 + > hw/vfio/common.c | 16 ++++++---------- > hw/vfio/container.c | 4 ++++ > hw/vfio/helpers.c | 11 +++++++++++ > hw/vfio/iommufd.c | 4 ++++ > 5 files changed, 26 insertions(+), 10 deletions(-) > > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h > index 1a96678f8c38..4e44b26d3c45 100644 > --- a/include/hw/vfio/vfio-common.h > +++ b/include/hw/vfio/vfio-common.h > @@ -242,6 +242,7 @@ void vfio_region_finalize(VFIORegion *region); > void vfio_reset_handler(void *opaque); > struct vfio_device_info *vfio_get_device_info(int fd); > bool vfio_device_is_mdev(VFIODevice *vbasedev); > +bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp); > bool vfio_attach_device(char *name, VFIODevice *vbasedev, > AddressSpace *as, Error **errp); > void vfio_detach_device(VFIODevice *vbasedev); > 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..adb302216e23 100644 > --- a/hw/vfio/container.c > +++ b/hw/vfio/container.c > @@ -917,6 +917,10 @@ static bool vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev, > > trace_vfio_attach_device(vbasedev->name, groupid); > > + if (!vfio_device_hiod_realize(vbasedev, errp)) { > + return false; > + } > + > group = vfio_get_group(groupid, as, errp); > if (!group) { > return false; > diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c > index 7e23e9080c9d..ea15c79db0a3 100644 > --- a/hw/vfio/helpers.c > +++ b/hw/vfio/helpers.c > @@ -689,3 +689,14 @@ bool vfio_device_is_mdev(VFIODevice *vbasedev) > subsys = realpath(tmp, NULL); > return subsys && (strcmp(subsys, "/sys/bus/mdev") == 0); > } > + > +bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp) > +{ > + HostIOMMUDevice *hiod = vbasedev->hiod; > + > + if (!hiod) { > + return true; > + } > + > + return HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp); > +} > diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c > index 7a10b1e90a6f..bb44d948c735 100644 > --- a/hw/vfio/iommufd.c > +++ b/hw/vfio/iommufd.c > @@ -403,6 +403,10 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, > > space = vfio_get_address_space(as); > > + if (!vfio_device_hiod_realize(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);
>-----Original Message----- >From: Joao Martins <joao.m.martins@oracle.com> >Subject: [PATCH v5 08/13] vfio/{iommufd,container}: Invoke >HostIOMMUDevice::realize() during attach_device() > >Move the HostIOMMUDevice::realize() to be invoked during the attach of >the device >before we allocate IOMMUFD hardware pagetable objects (HWPT). This >allows the use >of the hw_caps obtained by IOMMU_GET_HW_INFO that essentially tell if >the IOMMU >behind the device supports dirty tracking. > >Note: The HostIOMMUDevice data from legacy backend is static and doesn't >need any information from the (type1-iommu) backend to be initialized. >In contrast however, the IOMMUFD HostIOMMUDevice data requires the >iommufd FD to be connected and having a devid to be able to successfully >GET_HW_INFO. This means vfio_device_hiod_realize() is called in >different places within the backend .attach_device() implementation. > >Suggested-by: Cédric Le Goater <clg@redhat.cm> >Signed-off-by: Joao Martins <joao.m.martins@oracle.com> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com> Thanks Zhenzhong >--- > include/hw/vfio/vfio-common.h | 1 + > hw/vfio/common.c | 16 ++++++---------- > hw/vfio/container.c | 4 ++++ > hw/vfio/helpers.c | 11 +++++++++++ > hw/vfio/iommufd.c | 4 ++++ > 5 files changed, 26 insertions(+), 10 deletions(-) > >diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio- >common.h >index 1a96678f8c38..4e44b26d3c45 100644 >--- a/include/hw/vfio/vfio-common.h >+++ b/include/hw/vfio/vfio-common.h >@@ -242,6 +242,7 @@ void vfio_region_finalize(VFIORegion *region); > void vfio_reset_handler(void *opaque); > struct vfio_device_info *vfio_get_device_info(int fd); > bool vfio_device_is_mdev(VFIODevice *vbasedev); >+bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp); > bool vfio_attach_device(char *name, VFIODevice *vbasedev, > AddressSpace *as, Error **errp); > void vfio_detach_device(VFIODevice *vbasedev); >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_IOMMUF >D)); >@@ -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..adb302216e23 100644 >--- a/hw/vfio/container.c >+++ b/hw/vfio/container.c >@@ -917,6 +917,10 @@ static bool vfio_legacy_attach_device(const char >*name, VFIODevice *vbasedev, > > trace_vfio_attach_device(vbasedev->name, groupid); > >+ if (!vfio_device_hiod_realize(vbasedev, errp)) { >+ return false; >+ } >+ > group = vfio_get_group(groupid, as, errp); > if (!group) { > return false; >diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c >index 7e23e9080c9d..ea15c79db0a3 100644 >--- a/hw/vfio/helpers.c >+++ b/hw/vfio/helpers.c >@@ -689,3 +689,14 @@ bool vfio_device_is_mdev(VFIODevice *vbasedev) > subsys = realpath(tmp, NULL); > return subsys && (strcmp(subsys, "/sys/bus/mdev") == 0); > } >+ >+bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp) >+{ >+ HostIOMMUDevice *hiod = vbasedev->hiod; >+ >+ if (!hiod) { >+ return true; >+ } >+ >+ return HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, >errp); >+} >diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c >index 7a10b1e90a6f..bb44d948c735 100644 >--- a/hw/vfio/iommufd.c >+++ b/hw/vfio/iommufd.c >@@ -403,6 +403,10 @@ static bool iommufd_cdev_attach(const char >*name, VFIODevice *vbasedev, > > space = vfio_get_address_space(as); > >+ if (!vfio_device_hiod_realize(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); >-- >2.17.2
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h index 1a96678f8c38..4e44b26d3c45 100644 --- a/include/hw/vfio/vfio-common.h +++ b/include/hw/vfio/vfio-common.h @@ -242,6 +242,7 @@ void vfio_region_finalize(VFIORegion *region); void vfio_reset_handler(void *opaque); struct vfio_device_info *vfio_get_device_info(int fd); bool vfio_device_is_mdev(VFIODevice *vbasedev); +bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp); bool vfio_attach_device(char *name, VFIODevice *vbasedev, AddressSpace *as, Error **errp); void vfio_detach_device(VFIODevice *vbasedev); 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..adb302216e23 100644 --- a/hw/vfio/container.c +++ b/hw/vfio/container.c @@ -917,6 +917,10 @@ static bool vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev, trace_vfio_attach_device(vbasedev->name, groupid); + if (!vfio_device_hiod_realize(vbasedev, errp)) { + return false; + } + group = vfio_get_group(groupid, as, errp); if (!group) { return false; diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c index 7e23e9080c9d..ea15c79db0a3 100644 --- a/hw/vfio/helpers.c +++ b/hw/vfio/helpers.c @@ -689,3 +689,14 @@ bool vfio_device_is_mdev(VFIODevice *vbasedev) subsys = realpath(tmp, NULL); return subsys && (strcmp(subsys, "/sys/bus/mdev") == 0); } + +bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp) +{ + HostIOMMUDevice *hiod = vbasedev->hiod; + + if (!hiod) { + return true; + } + + return HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp); +} diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c index 7a10b1e90a6f..bb44d948c735 100644 --- a/hw/vfio/iommufd.c +++ b/hw/vfio/iommufd.c @@ -403,6 +403,10 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, space = vfio_get_address_space(as); + if (!vfio_device_hiod_realize(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);
Move the HostIOMMUDevice::realize() to be invoked during the attach of the device before we allocate IOMMUFD hardware pagetable objects (HWPT). This allows the use of the hw_caps obtained by IOMMU_GET_HW_INFO that essentially tell if the IOMMU behind the device supports dirty tracking. Note: The HostIOMMUDevice data from legacy backend is static and doesn't need any information from the (type1-iommu) backend to be initialized. In contrast however, the IOMMUFD HostIOMMUDevice data requires the iommufd FD to be connected and having a devid to be able to successfully GET_HW_INFO. This means vfio_device_hiod_realize() is called in different places within the backend .attach_device() implementation. Suggested-by: Cédric Le Goater <clg@redhat.cm> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> --- include/hw/vfio/vfio-common.h | 1 + hw/vfio/common.c | 16 ++++++---------- hw/vfio/container.c | 4 ++++ hw/vfio/helpers.c | 11 +++++++++++ hw/vfio/iommufd.c | 4 ++++ 5 files changed, 26 insertions(+), 10 deletions(-)