| Message ID | 20240722211326.70162-1-joao.m.martins@oracle.com |
|---|---|
| Headers | show |
| Series | hw/iommufd: IOMMUFD Dirty Tracking | expand |
Hi Joao, On 7/22/24 23:13, 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 Nit: maybe this comment shall be also added in iommufd.c before the call to vfio_device_hiod_realize() to avoid someone else to move that call earlier at some point > 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> > --- > 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 784e266e6aab..da12cbd56408 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -1537,7 +1537,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)); > @@ -1545,21 +1545,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 10cb4b4320ac..9ccdb639ac84 100644 > --- a/hw/vfio/container.c > +++ b/hw/vfio/container.c > @@ -914,6 +914,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; don't you want to go to err_alloc_ioas instead? > + } > + > 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 5e2fc1ce089d..2324bf892c56 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); Eric
On 7/23/24 09:38, Eric Auger wrote: > Hi Joao, > > On 7/22/24 23:13, 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 > Nit: maybe this comment shall be also added in iommufd.c before the call > to vfio_device_hiod_realize() to avoid someone else to move that call > earlier at some point >> 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> >> --- >> 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 784e266e6aab..da12cbd56408 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -1537,7 +1537,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)); >> @@ -1545,21 +1545,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 10cb4b4320ac..9ccdb639ac84 100644 >> --- a/hw/vfio/container.c >> +++ b/hw/vfio/container.c >> @@ -914,6 +914,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; > don't you want to go to err_alloc_ioas instead? hmm, the err_alloc_ioas label is in a different function iommufd_cdev_attach(). may be you meant the comment for routine iommufd_cdev_attach() and label err_connect_bind ? Thanks, C. >> + } >> + >> 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 5e2fc1ce089d..2324bf892c56 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); > Eric >
On 23/07/2024 08:38, Eric Auger wrote: > Hi Joao, > > On 7/22/24 23:13, 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 > Nit: maybe this comment shall be also added in iommufd.c before the call > to vfio_device_hiod_realize() to avoid someone else to move that call > earlier at some point >> 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> >> --- >> 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 784e266e6aab..da12cbd56408 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -1537,7 +1537,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)); >> @@ -1545,21 +1545,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 10cb4b4320ac..9ccdb639ac84 100644 >> --- a/hw/vfio/container.c >> +++ b/hw/vfio/container.c >> @@ -914,6 +914,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; > don't you want to go to err_alloc_ioas instead? Oh, yes, I thought I was doing that, but I am not :( Thanks for catching that Your comment is spot on but in the wrong place. vfio_legacy_attach_device() can just return false as it's at the top of the function but here (...) >> + } >> + >> 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 5e2fc1ce089d..2324bf892c56 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; >> + } >> + (...) we definitely need a goto err_alloc_ioas here. Snip below: @@ -482,7 +483,7 @@ 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; + goto err_alloc_ioas; } /* try to attach to an existing container in this space */
On 7/23/24 09:44, Cédric Le Goater wrote: > On 7/23/24 09:38, Eric Auger wrote: >> Hi Joao, >> >> On 7/22/24 23:13, 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 >> Nit: maybe this comment shall be also added in iommufd.c before the call >> to vfio_device_hiod_realize() to avoid someone else to move that call >> earlier at some point >>> 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> >>> --- >>> 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 784e266e6aab..da12cbd56408 100644 >>> --- a/hw/vfio/common.c >>> +++ b/hw/vfio/common.c >>> @@ -1537,7 +1537,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)); >>> @@ -1545,21 +1545,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 10cb4b4320ac..9ccdb639ac84 100644 >>> --- a/hw/vfio/container.c >>> +++ b/hw/vfio/container.c >>> @@ -914,6 +914,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; >> don't you want to go to err_alloc_ioas instead? > > hmm, the err_alloc_ioas label is in a different function > iommufd_cdev_attach(). > > may be you meant the comment for routine iommufd_cdev_attach() and > label err_connect_bind ? > > > Thanks, > > C. > > >>> + } >>> + >>> 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 5e2fc1ce089d..2324bf892c56 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; Hum sorry my previous comment was targetting that place. I think unrolling is needed up to put_address_space so effectively this does not match err_alloc_ioas but I guess we would need another label Eric >>> + } >>> + >>> /* try to attach to an existing container in this space */ >>> QLIST_FOREACH(bcontainer, &space->containers, next) { >>> container = container_of(bcontainer, VFIOIOMMUFDContainer, >>> bcontainer); >> Eric >> >
On 7/23/24 09:53, Joao Martins wrote: > On 23/07/2024 08:38, Eric Auger wrote: >> Hi Joao, >> >> On 7/22/24 23:13, 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 >> Nit: maybe this comment shall be also added in iommufd.c before the call >> to vfio_device_hiod_realize() to avoid someone else to move that call >> earlier at some point >>> 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> >>> --- >>> 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 784e266e6aab..da12cbd56408 100644 >>> --- a/hw/vfio/common.c >>> +++ b/hw/vfio/common.c >>> @@ -1537,7 +1537,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)); >>> @@ -1545,21 +1545,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 10cb4b4320ac..9ccdb639ac84 100644 >>> --- a/hw/vfio/container.c >>> +++ b/hw/vfio/container.c >>> @@ -914,6 +914,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; >> don't you want to go to err_alloc_ioas instead? > > Oh, yes, I thought I was doing that, but I am not :( Thanks for catching that > > Your comment is spot on but in the wrong place. > > vfio_legacy_attach_device() can just return false as it's at the top of the > function but here (...) > >>> + } >>> + >>> 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 5e2fc1ce089d..2324bf892c56 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; >>> + } >>> + > > (...) we definitely need a goto err_alloc_ioas here. > > Snip below: > > @@ -482,7 +483,7 @@ 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; > + goto err_alloc_ioas; > } > > /* try to attach to an existing container in this space */ > ok. Applied the changes. Thanks, C.
On 23/07/2024 08:55, Eric Auger wrote: > > > On 7/23/24 09:44, Cédric Le Goater wrote: >> On 7/23/24 09:38, Eric Auger wrote: >>> Hi Joao, >>> >>> On 7/22/24 23:13, 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 >>> Nit: maybe this comment shall be also added in iommufd.c before the call >>> to vfio_device_hiod_realize() to avoid someone else to move that call >>> earlier at some point >>>> 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> >>>> --- >>>> 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 784e266e6aab..da12cbd56408 100644 >>>> --- a/hw/vfio/common.c >>>> +++ b/hw/vfio/common.c >>>> @@ -1537,7 +1537,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)); >>>> @@ -1545,21 +1545,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 10cb4b4320ac..9ccdb639ac84 100644 >>>> --- a/hw/vfio/container.c >>>> +++ b/hw/vfio/container.c >>>> @@ -914,6 +914,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; >>> don't you want to go to err_alloc_ioas instead? >> >> hmm, the err_alloc_ioas label is in a different function >> iommufd_cdev_attach(). >> >> may be you meant the comment for routine iommufd_cdev_attach() and >> label err_connect_bind ? >> >> >> Thanks, >> >> C. >> >> >>>> + } >>>> + >>>> 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 5e2fc1ce089d..2324bf892c56 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; > Hum sorry my previous comment was targetting that place. I think > unrolling is needed up to put_address_space > > so effectively this does not match err_alloc_ioas but I guess we would > need another label > You're right. We haven't yet attached rthe device and that's what err_alloc_ioas would do. Adding another label not sure would make things cleaner given the ordering requirement. So maybe this instead? @@ -482,7 +483,8 @@ 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; + vfio_put_address_space(space); + goto err_connect_bind; } /* try to attach to an existing container in this space */
On 7/23/24 10:05, Joao Martins wrote: > On 23/07/2024 08:55, Eric Auger wrote: >> >> >> On 7/23/24 09:44, Cédric Le Goater wrote: >>> On 7/23/24 09:38, Eric Auger wrote: >>>> Hi Joao, >>>> >>>> On 7/22/24 23:13, 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 >>>> Nit: maybe this comment shall be also added in iommufd.c before the call >>>> to vfio_device_hiod_realize() to avoid someone else to move that call >>>> earlier at some point >>>>> 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> >>>>> --- >>>>> 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 784e266e6aab..da12cbd56408 100644 >>>>> --- a/hw/vfio/common.c >>>>> +++ b/hw/vfio/common.c >>>>> @@ -1537,7 +1537,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)); >>>>> @@ -1545,21 +1545,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 10cb4b4320ac..9ccdb639ac84 100644 >>>>> --- a/hw/vfio/container.c >>>>> +++ b/hw/vfio/container.c >>>>> @@ -914,6 +914,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; >>>> don't you want to go to err_alloc_ioas instead? >>> >>> hmm, the err_alloc_ioas label is in a different function >>> iommufd_cdev_attach(). >>> >>> may be you meant the comment for routine iommufd_cdev_attach() and >>> label err_connect_bind ? >>> >>> >>> Thanks, >>> >>> C. >>> >>> >>>>> + } >>>>> + >>>>> 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 5e2fc1ce089d..2324bf892c56 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; >> Hum sorry my previous comment was targetting that place. I think >> unrolling is needed up to put_address_space >> >> so effectively this does not match err_alloc_ioas but I guess we would >> need another label >> > > You're right. We haven't yet attached rthe device and that's what err_alloc_ioas > would do. Adding another label not sure would make things cleaner given the > ordering requirement. So maybe this instead? > > @@ -482,7 +483,8 @@ 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; > + vfio_put_address_space(space); > + goto err_connect_bind; > } > > /* try to attach to an existing container in this space */ > LGTM. Thanks, C.
On 7/23/24 10:05, Joao Martins wrote: > On 23/07/2024 08:55, Eric Auger wrote: >> >> On 7/23/24 09:44, Cédric Le Goater wrote: >>> On 7/23/24 09:38, Eric Auger wrote: >>>> Hi Joao, >>>> >>>> On 7/22/24 23:13, 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 >>>> Nit: maybe this comment shall be also added in iommufd.c before the call >>>> to vfio_device_hiod_realize() to avoid someone else to move that call >>>> earlier at some point >>>>> 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> >>>>> --- >>>>> 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 784e266e6aab..da12cbd56408 100644 >>>>> --- a/hw/vfio/common.c >>>>> +++ b/hw/vfio/common.c >>>>> @@ -1537,7 +1537,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)); >>>>> @@ -1545,21 +1545,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 10cb4b4320ac..9ccdb639ac84 100644 >>>>> --- a/hw/vfio/container.c >>>>> +++ b/hw/vfio/container.c >>>>> @@ -914,6 +914,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; >>>> don't you want to go to err_alloc_ioas instead? >>> hmm, the err_alloc_ioas label is in a different function >>> iommufd_cdev_attach(). >>> >>> may be you meant the comment for routine iommufd_cdev_attach() and >>> label err_connect_bind ? >>> >>> >>> Thanks, >>> >>> C. >>> >>> >>>>> + } >>>>> + >>>>> 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 5e2fc1ce089d..2324bf892c56 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; >> Hum sorry my previous comment was targetting that place. I think >> unrolling is needed up to put_address_space >> >> so effectively this does not match err_alloc_ioas but I guess we would >> need another label >> > You're right. We haven't yet attached rthe device and that's what err_alloc_ioas > would do. Adding another label not sure would make things cleaner given the > ordering requirement. So maybe this instead? > > @@ -482,7 +483,8 @@ 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; > + vfio_put_address_space(space); > + goto err_connect_bind; > } > > /* try to attach to an existing container in this space */ > With that addition Reviewed-by: Eric Auger <eric.auger@redhat.com> Eric
>-----Original Message----- >From: Joao Martins <joao.m.martins@oracle.com> >Subject: Re: [PATCH v6 4/9] vfio/{iommufd,container}: Invoke >HostIOMMUDevice::realize() during attach_device() > >On 23/07/2024 08:55, Eric Auger wrote: >> >> >> On 7/23/24 09:44, Cédric Le Goater wrote: >>> On 7/23/24 09:38, Eric Auger wrote: >>>> Hi Joao, >>>> >>>> On 7/22/24 23:13, 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 >>>> Nit: maybe this comment shall be also added in iommufd.c before the >call >>>> to vfio_device_hiod_realize() to avoid someone else to move that call >>>> earlier at some point >>>>> 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> >>>>> --- >>>>> 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 784e266e6aab..da12cbd56408 100644 >>>>> --- a/hw/vfio/common.c >>>>> +++ b/hw/vfio/common.c >>>>> @@ -1537,7 +1537,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)); >>>>> @@ -1545,21 +1545,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 10cb4b4320ac..9ccdb639ac84 100644 >>>>> --- a/hw/vfio/container.c >>>>> +++ b/hw/vfio/container.c >>>>> @@ -914,6 +914,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; >>>> don't you want to go to err_alloc_ioas instead? >>> >>> hmm, the err_alloc_ioas label is in a different function >>> iommufd_cdev_attach(). >>> >>> may be you meant the comment for routine iommufd_cdev_attach() and >>> label err_connect_bind ? >>> >>> >>> Thanks, >>> >>> C. >>> >>> >>>>> + } >>>>> + >>>>> 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 5e2fc1ce089d..2324bf892c56 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; >> Hum sorry my previous comment was targetting that place. I think >> unrolling is needed up to put_address_space >> >> so effectively this does not match err_alloc_ioas but I guess we would >> need another label >> > >You're right. We haven't yet attached rthe device and that's what >err_alloc_ioas >would do. Adding another label not sure would make things cleaner given >the >ordering requirement. So maybe this instead? > >@@ -482,7 +483,8 @@ 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; >+ vfio_put_address_space(space); >+ goto err_connect_bind; > } > > /* try to attach to an existing container in this space */ I was confused though Cedric and Eric both ACK this change. Don't we miss the iommufd_cdev_unbind_and_disconnect() call?
On 7/23/24 10:20, Duan, Zhenzhong wrote: > >> -----Original Message----- >> From: Joao Martins <joao.m.martins@oracle.com> >> Subject: Re: [PATCH v6 4/9] vfio/{iommufd,container}: Invoke >> HostIOMMUDevice::realize() during attach_device() >> >> On 23/07/2024 08:55, Eric Auger wrote: >>> >>> On 7/23/24 09:44, Cédric Le Goater wrote: >>>> On 7/23/24 09:38, Eric Auger wrote: >>>>> Hi Joao, >>>>> >>>>> On 7/22/24 23:13, 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 >>>>> Nit: maybe this comment shall be also added in iommufd.c before the >> call >>>>> to vfio_device_hiod_realize() to avoid someone else to move that call >>>>> earlier at some point >>>>>> 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> >>>>>> --- >>>>>> 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 784e266e6aab..da12cbd56408 100644 >>>>>> --- a/hw/vfio/common.c >>>>>> +++ b/hw/vfio/common.c >>>>>> @@ -1537,7 +1537,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)); >>>>>> @@ -1545,21 +1545,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 10cb4b4320ac..9ccdb639ac84 100644 >>>>>> --- a/hw/vfio/container.c >>>>>> +++ b/hw/vfio/container.c >>>>>> @@ -914,6 +914,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; >>>>> don't you want to go to err_alloc_ioas instead? >>>> hmm, the err_alloc_ioas label is in a different function >>>> iommufd_cdev_attach(). >>>> >>>> may be you meant the comment for routine iommufd_cdev_attach() and >>>> label err_connect_bind ? >>>> >>>> >>>> Thanks, >>>> >>>> C. >>>> >>>> >>>>>> + } >>>>>> + >>>>>> 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 5e2fc1ce089d..2324bf892c56 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; >>> Hum sorry my previous comment was targetting that place. I think >>> unrolling is needed up to put_address_space >>> >>> so effectively this does not match err_alloc_ioas but I guess we would >>> need another label >>> >> You're right. We haven't yet attached rthe device and that's what >> err_alloc_ioas >> would do. Adding another label not sure would make things cleaner given >> the >> ordering requirement. So maybe this instead? >> >> @@ -482,7 +483,8 @@ 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; >> + vfio_put_address_space(space); >> + goto err_connect_bind; >> } >> >> /* try to attach to an existing container in this space */ > I was confused though Cedric and Eric both ACK this change. Don't we miss the iommufd_cdev_unbind_and_disconnect() call? Hum yes you're right. connect and bind was done. I thought this was done later. so err_alloc_ioas label looks good Eric >
On 23/07/2024 09:24, Eric Auger wrote: > > > On 7/23/24 10:20, Duan, Zhenzhong wrote: >> >>> -----Original Message----- >>> From: Joao Martins <joao.m.martins@oracle.com> >>> Subject: Re: [PATCH v6 4/9] vfio/{iommufd,container}: Invoke >>> HostIOMMUDevice::realize() during attach_device() >>> >>> On 23/07/2024 08:55, Eric Auger wrote: >>>> >>>> On 7/23/24 09:44, Cédric Le Goater wrote: >>>>> On 7/23/24 09:38, Eric Auger wrote: >>>>>> Hi Joao, >>>>>> >>>>>> On 7/22/24 23:13, 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 >>>>>> Nit: maybe this comment shall be also added in iommufd.c before the >>> call >>>>>> to vfio_device_hiod_realize() to avoid someone else to move that call >>>>>> earlier at some point >>>>>>> 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> >>>>>>> --- >>>>>>> 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 784e266e6aab..da12cbd56408 100644 >>>>>>> --- a/hw/vfio/common.c >>>>>>> +++ b/hw/vfio/common.c >>>>>>> @@ -1537,7 +1537,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)); >>>>>>> @@ -1545,21 +1545,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 10cb4b4320ac..9ccdb639ac84 100644 >>>>>>> --- a/hw/vfio/container.c >>>>>>> +++ b/hw/vfio/container.c >>>>>>> @@ -914,6 +914,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; >>>>>> don't you want to go to err_alloc_ioas instead? >>>>> hmm, the err_alloc_ioas label is in a different function >>>>> iommufd_cdev_attach(). >>>>> >>>>> may be you meant the comment for routine iommufd_cdev_attach() and >>>>> label err_connect_bind ? >>>>> >>>>> >>>>> Thanks, >>>>> >>>>> C. >>>>> >>>>> >>>>>>> + } >>>>>>> + >>>>>>> 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 5e2fc1ce089d..2324bf892c56 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; >>>> Hum sorry my previous comment was targetting that place. I think >>>> unrolling is needed up to put_address_space >>>> >>>> so effectively this does not match err_alloc_ioas but I guess we would >>>> need another label >>>> >>> You're right. We haven't yet attached rthe device and that's what >>> err_alloc_ioas >>> would do. Adding another label not sure would make things cleaner given >>> the >>> ordering requirement. So maybe this instead? >>> >>> @@ -482,7 +483,8 @@ 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; >>> + vfio_put_address_space(space); >>> + goto err_connect_bind; >>> } >>> >>> /* try to attach to an existing container in this space */ >> I was confused though Cedric and Eric both ACK this change. Don't we miss the iommufd_cdev_unbind_and_disconnect() call? > Hum yes you're right. connect and bind was done. I thought this was done > later. so err_alloc_ioas label looks good > It seems you were right the first time. I definitely haven't got coffee yet.
On 7/22/24 23:13, Joao Martins wrote: > This small series adds support for IOMMU dirty tracking support via the > IOMMUFD backend. The hardware capability is available on most recent x86 > hardware (and these SMMUv3 in upcoming v6.11). The series is divided > organized as follows: > > * Patches 1 - 7: IOMMUFD backend support for dirty tracking; > > Introduce auto domains -- Patch 3 goes into more detail, but the gist is that > we will find and attach a device to a compatible IOMMU domain, or allocate a new > hardware pagetable *or* rely on kernel IOAS attach (for mdevs). Afterwards the > workflow is relatively simple: > > 1) Probe device and allow dirty tracking in the HWPT > 2) Toggling dirty tracking on/off > 3) Read-and-clear of Dirty IOVAs > > The heuristics selected for (1) were to always request the HWPT for > dirty tracking if supported, or rely on device dirty page tracking. This > is a little simplistic and we aren't necessarily utilizing IOMMU dirty > tracking even if we ask during hwpt allocation. > > The unmap case is deferred until further vIOMMU support with migration > is added[3] which will then introduce the usage of > IOMMU_HWPT_GET_DIRTY_BITMAP_NO_CLEAR in GET_DIRTY_BITMAP ioctl in the > dma unmap bitmap flow. > > * Patches 8 - 9: Don't block live migration where there's no VF dirty > tracker, considering that we have IOMMU dirty tracking. > > Comments and feedback appreciated (on patches 1, 5, 8, 9) > > Cheers, > Joao > > P.S. Suggest v6.11-rc as hypervisor kernel as there's > some bugs fixed there with regards to IOMMU hugepage dirty tracking. > > Changes since v5[6]: > * Remove patches 1-4 as these were commited to vfio-next > * Add the Rb by Cedric and Zhenzhong (previously patches 7, 8, 10, 11) > * Introduce VFIODevice::iommu_dirty_tracking and use it on patch 5, 8 > to store whether we can use IOMMU dirty tracking. > > Changes since v4[5]: > * Add various Reviewed-by in patches 2,3,4,6,8,11 > * Change error messages to mention IOMMU (Zhenzhong) > * Better improve the checking of dirty page tracking in > vfio_migration_realize() to detect per-device IOMMU instead of using > container dirty_page_supported(). > * Improve various commit messages (Eric) > * Extract the caps::hw_caps into its own patch as it was miosleading to > be hidden in another patch (new patch 7) > * Restructure patch 1 helper to be vfio_device_is_mdev() and use > vfio::mdev directly in rest of patches (Cedric) > * Improve error messages of set,query dirty tracking (Cedric) > * Add missing casts to uintptr and uint64_t* (Cedric) > * Add missing commens to struct doc from aw_bits removal (and hw_caps > addition) (Eric) > * Fix the detach flow in auto domains (Eric) > * Set hwpt to NULL on detach (Eric) > * Spurious line (Eric) > > Changes since v3[5]: > * Skip HostIOMMUDevice::realize for mdev, and introduce a helper to check if the VFIO > device is mdev. (Zhenzhong) > * Skip setting IOMMU device for mdev (Zhenzhong) > * Add Zhenzhong review tag in patch 3 > * Utilize vbasedev::bcontainer::dirty_pages_supported instead of introducing > a new HostIOMMUDevice capability and thus remove the cap patch from the series (Zhenzhong) > * Move the HostIOMMUDevice::realize() to be part of VFIODevice initialization in attach_device() > while skipping it all together for mdev. (Cedric) > * Due to the previous item, had to remove aw_bits because it depends on device attach being > finished, instead defer it to when get_cap() gets called. > * Skip auto domains for mdev instead of purposedly erroring out (Zhenzhong) > * Pass errp in all cases, and instead just free the error in case of -EINVAL > in most of all patches, and also pass Error* in iommufd_backend_alloc_hwpt() amd > set/query dirty. This is made better thanks in part to skipping auto domains for mdev (Cedric) > > Changes since RFCv2[4]: > * Always allocate hwpt with IOMMU_HWPT_ALLOC_DIRTY_TRACKING even if > we end up not actually toggling dirty tracking. (Avihai) > * Fix error handling widely in auto domains logic and all patches (Avihai) > * Reuse iommufd_backend_get_device_info() for capabilities (Zhenzhong) > * New patches 1 and 2 taking into consideration previous comments. > * Store hwpt::flags to know if we have dirty tracking (Avihai) > * New patch 8, that allows to query dirty tracking support after > provisioning. This is a cleaner way to check IOMMU dirty tracking support > when vfio::migration is iniitalized, as opposed to RFCv2 via device caps. > device caps way is still used because at vfio attach we aren't yet with > a fully initialized migration state. > * Adopt error propagation in query,set dirty tracking > * Misc improvements overall broadly and Avihai > * Drop hugepages as it's a bit unrelated; I can pursue that patch > * separately. The main motivation is to provide a way to test > without hugepages similar to what vfio_type1_iommu.disable_hugepages=1 > does. > > Changes since RFCv1[2]: > * Remove intel/amd dirty tracking emulation enabling > * Remove the dirtyrate improvement for VF/IOMMU dirty tracking > [Will pursue these two in separate series] > * Introduce auto domains support > * Enforce dirty tracking following the IOMMUFD UAPI for this > * Add support for toggling hugepages in IOMMUFD > * Auto enable support when VF supports migration to use IOMMU > when it doesn't have VF dirty tracking > * Add a parameter to toggle VF dirty tracking > > [0] https://lore.kernel.org/qemu-devel/20240201072818.327930-1-zhenzhong.duan@intel.com/ > [1] https://lore.kernel.org/qemu-devel/20240201072818.327930-10-zhenzhong.duan@intel.com/ > [2] https://lore.kernel.org/qemu-devel/20220428211351.3897-1-joao.m.martins@oracle.com/ > [3] https://lore.kernel.org/qemu-devel/20230622214845.3980-1-joao.m.martins@oracle.com/ > [4] https://lore.kernel.org/qemu-devel/20240212135643.5858-1-joao.m.martins@oracle.com/ > [5] https://lore.kernel.org/qemu-devel/20240708143420.16953-1-joao.m.martins@oracle.com/ > [6] https://lore.kernel.org/qemu-devel/20240719120501.81279-1-joao.m.martins@oracle.com/ > > Joao Martins (9): > vfio/iommufd: Introduce auto domain creation > vfio/{iommufd,container}: Remove caps::aw_bits > vfio/iommufd: Add hw_caps field to HostIOMMUDeviceCaps > vfio/{iommufd,container}: Invoke HostIOMMUDevice::realize() during > attach_device() > vfio/iommufd: Probe and request hwpt dirty tracking capability > vfio/iommufd: Implement VFIOIOMMUClass::set_dirty_tracking support > vfio/iommufd: Implement VFIOIOMMUClass::query_dirty_bitmap support > vfio/migration: Don't block migration device dirty tracking is > unsupported > vfio/common: Allow disabling device dirty page tracking > > include/hw/vfio/vfio-common.h | 13 +++ > include/sysemu/host_iommu_device.h | 5 +- > include/sysemu/iommufd.h | 11 ++ > backends/iommufd.c | 85 ++++++++++++++- > hw/vfio/common.c | 19 ++-- > hw/vfio/container.c | 9 +- > hw/vfio/helpers.c | 11 ++ > hw/vfio/iommufd.c | 170 ++++++++++++++++++++++++++++- > hw/vfio/migration.c | 12 +- > hw/vfio/pci.c | 3 + > backends/trace-events | 3 + > 11 files changed, 318 insertions(+), 23 deletions(-) > Applied to vfio-next with the changes that were discussed this morning. Please check. Thanks, C.
On 23/07/2024 09:35, Cédric Le Goater wrote: > On 7/22/24 23:13, Joao Martins wrote: >> This small series adds support for IOMMU dirty tracking support via the >> IOMMUFD backend. The hardware capability is available on most recent x86 >> hardware (and these SMMUv3 in upcoming v6.11). The series is divided >> organized as follows: >> >> * Patches 1 - 7: IOMMUFD backend support for dirty tracking; >> >> Introduce auto domains -- Patch 3 goes into more detail, but the gist is that >> we will find and attach a device to a compatible IOMMU domain, or allocate a new >> hardware pagetable *or* rely on kernel IOAS attach (for mdevs). Afterwards the >> workflow is relatively simple: >> >> 1) Probe device and allow dirty tracking in the HWPT >> 2) Toggling dirty tracking on/off >> 3) Read-and-clear of Dirty IOVAs >> >> The heuristics selected for (1) were to always request the HWPT for >> dirty tracking if supported, or rely on device dirty page tracking. This >> is a little simplistic and we aren't necessarily utilizing IOMMU dirty >> tracking even if we ask during hwpt allocation. >> >> The unmap case is deferred until further vIOMMU support with migration >> is added[3] which will then introduce the usage of >> IOMMU_HWPT_GET_DIRTY_BITMAP_NO_CLEAR in GET_DIRTY_BITMAP ioctl in the >> dma unmap bitmap flow. >> >> * Patches 8 - 9: Don't block live migration where there's no VF dirty >> tracker, considering that we have IOMMU dirty tracking. >> >> Comments and feedback appreciated (on patches 1, 5, 8, 9) >> >> Cheers, >> Joao >> >> P.S. Suggest v6.11-rc as hypervisor kernel as there's >> some bugs fixed there with regards to IOMMU hugepage dirty tracking. >> >> Changes since v5[6]: >> * Remove patches 1-4 as these were commited to vfio-next >> * Add the Rb by Cedric and Zhenzhong (previously patches 7, 8, 10, 11) >> * Introduce VFIODevice::iommu_dirty_tracking and use it on patch 5, 8 >> to store whether we can use IOMMU dirty tracking. >> >> Changes since v4[5]: >> * Add various Reviewed-by in patches 2,3,4,6,8,11 >> * Change error messages to mention IOMMU (Zhenzhong) >> * Better improve the checking of dirty page tracking in >> vfio_migration_realize() to detect per-device IOMMU instead of using >> container dirty_page_supported(). >> * Improve various commit messages (Eric) >> * Extract the caps::hw_caps into its own patch as it was miosleading to >> be hidden in another patch (new patch 7) >> * Restructure patch 1 helper to be vfio_device_is_mdev() and use >> vfio::mdev directly in rest of patches (Cedric) >> * Improve error messages of set,query dirty tracking (Cedric) >> * Add missing casts to uintptr and uint64_t* (Cedric) >> * Add missing commens to struct doc from aw_bits removal (and hw_caps >> addition) (Eric) >> * Fix the detach flow in auto domains (Eric) >> * Set hwpt to NULL on detach (Eric) >> * Spurious line (Eric) >> >> Changes since v3[5]: >> * Skip HostIOMMUDevice::realize for mdev, and introduce a helper to check if >> the VFIO >> device is mdev. (Zhenzhong) >> * Skip setting IOMMU device for mdev (Zhenzhong) >> * Add Zhenzhong review tag in patch 3 >> * Utilize vbasedev::bcontainer::dirty_pages_supported instead of introducing >> a new HostIOMMUDevice capability and thus remove the cap patch from the >> series (Zhenzhong) >> * Move the HostIOMMUDevice::realize() to be part of VFIODevice initialization >> in attach_device() >> while skipping it all together for mdev. (Cedric) >> * Due to the previous item, had to remove aw_bits because it depends on device >> attach being >> finished, instead defer it to when get_cap() gets called. >> * Skip auto domains for mdev instead of purposedly erroring out (Zhenzhong) >> * Pass errp in all cases, and instead just free the error in case of -EINVAL >> in most of all patches, and also pass Error* in >> iommufd_backend_alloc_hwpt() amd >> set/query dirty. This is made better thanks in part to skipping auto >> domains for mdev (Cedric) >> >> Changes since RFCv2[4]: >> * Always allocate hwpt with IOMMU_HWPT_ALLOC_DIRTY_TRACKING even if >> we end up not actually toggling dirty tracking. (Avihai) >> * Fix error handling widely in auto domains logic and all patches (Avihai) >> * Reuse iommufd_backend_get_device_info() for capabilities (Zhenzhong) >> * New patches 1 and 2 taking into consideration previous comments. >> * Store hwpt::flags to know if we have dirty tracking (Avihai) >> * New patch 8, that allows to query dirty tracking support after >> provisioning. This is a cleaner way to check IOMMU dirty tracking support >> when vfio::migration is iniitalized, as opposed to RFCv2 via device caps. >> device caps way is still used because at vfio attach we aren't yet with >> a fully initialized migration state. >> * Adopt error propagation in query,set dirty tracking >> * Misc improvements overall broadly and Avihai >> * Drop hugepages as it's a bit unrelated; I can pursue that patch >> * separately. The main motivation is to provide a way to test >> without hugepages similar to what vfio_type1_iommu.disable_hugepages=1 >> does. >> >> Changes since RFCv1[2]: >> * Remove intel/amd dirty tracking emulation enabling >> * Remove the dirtyrate improvement for VF/IOMMU dirty tracking >> [Will pursue these two in separate series] >> * Introduce auto domains support >> * Enforce dirty tracking following the IOMMUFD UAPI for this >> * Add support for toggling hugepages in IOMMUFD >> * Auto enable support when VF supports migration to use IOMMU >> when it doesn't have VF dirty tracking >> * Add a parameter to toggle VF dirty tracking >> >> [0] >> https://lore.kernel.org/qemu-devel/20240201072818.327930-1-zhenzhong.duan@intel.com/ >> [1] >> https://lore.kernel.org/qemu-devel/20240201072818.327930-10-zhenzhong.duan@intel.com/ >> [2] >> https://lore.kernel.org/qemu-devel/20220428211351.3897-1-joao.m.martins@oracle.com/ >> [3] >> https://lore.kernel.org/qemu-devel/20230622214845.3980-1-joao.m.martins@oracle.com/ >> [4] >> https://lore.kernel.org/qemu-devel/20240212135643.5858-1-joao.m.martins@oracle.com/ >> [5] >> https://lore.kernel.org/qemu-devel/20240708143420.16953-1-joao.m.martins@oracle.com/ >> [6] >> https://lore.kernel.org/qemu-devel/20240719120501.81279-1-joao.m.martins@oracle.com/ >> >> Joao Martins (9): >> vfio/iommufd: Introduce auto domain creation >> vfio/{iommufd,container}: Remove caps::aw_bits >> vfio/iommufd: Add hw_caps field to HostIOMMUDeviceCaps >> vfio/{iommufd,container}: Invoke HostIOMMUDevice::realize() during >> attach_device() >> vfio/iommufd: Probe and request hwpt dirty tracking capability >> vfio/iommufd: Implement VFIOIOMMUClass::set_dirty_tracking support >> vfio/iommufd: Implement VFIOIOMMUClass::query_dirty_bitmap support >> vfio/migration: Don't block migration device dirty tracking is >> unsupported >> vfio/common: Allow disabling device dirty page tracking >> >> include/hw/vfio/vfio-common.h | 13 +++ >> include/sysemu/host_iommu_device.h | 5 +- >> include/sysemu/iommufd.h | 11 ++ >> backends/iommufd.c | 85 ++++++++++++++- >> hw/vfio/common.c | 19 ++-- >> hw/vfio/container.c | 9 +- >> hw/vfio/helpers.c | 11 ++ >> hw/vfio/iommufd.c | 170 ++++++++++++++++++++++++++++- >> hw/vfio/migration.c | 12 +- >> hw/vfio/pci.c | 3 + >> backends/trace-events | 3 + >> 11 files changed, 318 insertions(+), 23 deletions(-) > > Applied to vfio-next with the changes that were discussed this morning. > Please check. > I think the only thing missing is in the fourth patch to add the comment Eric suggested (see below). Other than that, looks good to me. diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c index 50ffa4b77090..abb6f1a4b4a8 100644 --- a/hw/vfio/iommufd.c +++ b/hw/vfio/iommufd.c @@ -488,6 +488,13 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, space = vfio_get_address_space(as); + /* + * 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 call + * iommufd_backend_get_device_info(). + */ if (!vfio_device_hiod_realize(vbasedev, errp)) { goto err_alloc_ioas; }
On 7/23/24 10:56, Joao Martins wrote: > On 23/07/2024 09:35, Cédric Le Goater wrote: >> On 7/22/24 23:13, Joao Martins wrote: >>> This small series adds support for IOMMU dirty tracking support via the >>> IOMMUFD backend. The hardware capability is available on most recent x86 >>> hardware (and these SMMUv3 in upcoming v6.11). The series is divided >>> organized as follows: >>> >>> * Patches 1 - 7: IOMMUFD backend support for dirty tracking; >>> >>> Introduce auto domains -- Patch 3 goes into more detail, but the gist is that >>> we will find and attach a device to a compatible IOMMU domain, or allocate a new >>> hardware pagetable *or* rely on kernel IOAS attach (for mdevs). Afterwards the >>> workflow is relatively simple: >>> >>> 1) Probe device and allow dirty tracking in the HWPT >>> 2) Toggling dirty tracking on/off >>> 3) Read-and-clear of Dirty IOVAs >>> >>> The heuristics selected for (1) were to always request the HWPT for >>> dirty tracking if supported, or rely on device dirty page tracking. This >>> is a little simplistic and we aren't necessarily utilizing IOMMU dirty >>> tracking even if we ask during hwpt allocation. >>> >>> The unmap case is deferred until further vIOMMU support with migration >>> is added[3] which will then introduce the usage of >>> IOMMU_HWPT_GET_DIRTY_BITMAP_NO_CLEAR in GET_DIRTY_BITMAP ioctl in the >>> dma unmap bitmap flow. >>> >>> * Patches 8 - 9: Don't block live migration where there's no VF dirty >>> tracker, considering that we have IOMMU dirty tracking. >>> >>> Comments and feedback appreciated (on patches 1, 5, 8, 9) >>> >>> Cheers, >>> Joao >>> >>> P.S. Suggest v6.11-rc as hypervisor kernel as there's >>> some bugs fixed there with regards to IOMMU hugepage dirty tracking. >>> >>> Changes since v5[6]: >>> * Remove patches 1-4 as these were commited to vfio-next >>> * Add the Rb by Cedric and Zhenzhong (previously patches 7, 8, 10, 11) >>> * Introduce VFIODevice::iommu_dirty_tracking and use it on patch 5, 8 >>> to store whether we can use IOMMU dirty tracking. >>> >>> Changes since v4[5]: >>> * Add various Reviewed-by in patches 2,3,4,6,8,11 >>> * Change error messages to mention IOMMU (Zhenzhong) >>> * Better improve the checking of dirty page tracking in >>> vfio_migration_realize() to detect per-device IOMMU instead of using >>> container dirty_page_supported(). >>> * Improve various commit messages (Eric) >>> * Extract the caps::hw_caps into its own patch as it was miosleading to >>> be hidden in another patch (new patch 7) >>> * Restructure patch 1 helper to be vfio_device_is_mdev() and use >>> vfio::mdev directly in rest of patches (Cedric) >>> * Improve error messages of set,query dirty tracking (Cedric) >>> * Add missing casts to uintptr and uint64_t* (Cedric) >>> * Add missing commens to struct doc from aw_bits removal (and hw_caps >>> addition) (Eric) >>> * Fix the detach flow in auto domains (Eric) >>> * Set hwpt to NULL on detach (Eric) >>> * Spurious line (Eric) >>> >>> Changes since v3[5]: >>> * Skip HostIOMMUDevice::realize for mdev, and introduce a helper to check if >>> the VFIO >>> device is mdev. (Zhenzhong) >>> * Skip setting IOMMU device for mdev (Zhenzhong) >>> * Add Zhenzhong review tag in patch 3 >>> * Utilize vbasedev::bcontainer::dirty_pages_supported instead of introducing >>> a new HostIOMMUDevice capability and thus remove the cap patch from the >>> series (Zhenzhong) >>> * Move the HostIOMMUDevice::realize() to be part of VFIODevice initialization >>> in attach_device() >>> while skipping it all together for mdev. (Cedric) >>> * Due to the previous item, had to remove aw_bits because it depends on device >>> attach being >>> finished, instead defer it to when get_cap() gets called. >>> * Skip auto domains for mdev instead of purposedly erroring out (Zhenzhong) >>> * Pass errp in all cases, and instead just free the error in case of -EINVAL >>> in most of all patches, and also pass Error* in >>> iommufd_backend_alloc_hwpt() amd >>> set/query dirty. This is made better thanks in part to skipping auto >>> domains for mdev (Cedric) >>> >>> Changes since RFCv2[4]: >>> * Always allocate hwpt with IOMMU_HWPT_ALLOC_DIRTY_TRACKING even if >>> we end up not actually toggling dirty tracking. (Avihai) >>> * Fix error handling widely in auto domains logic and all patches (Avihai) >>> * Reuse iommufd_backend_get_device_info() for capabilities (Zhenzhong) >>> * New patches 1 and 2 taking into consideration previous comments. >>> * Store hwpt::flags to know if we have dirty tracking (Avihai) >>> * New patch 8, that allows to query dirty tracking support after >>> provisioning. This is a cleaner way to check IOMMU dirty tracking support >>> when vfio::migration is iniitalized, as opposed to RFCv2 via device caps. >>> device caps way is still used because at vfio attach we aren't yet with >>> a fully initialized migration state. >>> * Adopt error propagation in query,set dirty tracking >>> * Misc improvements overall broadly and Avihai >>> * Drop hugepages as it's a bit unrelated; I can pursue that patch >>> * separately. The main motivation is to provide a way to test >>> without hugepages similar to what vfio_type1_iommu.disable_hugepages=1 >>> does. >>> >>> Changes since RFCv1[2]: >>> * Remove intel/amd dirty tracking emulation enabling >>> * Remove the dirtyrate improvement for VF/IOMMU dirty tracking >>> [Will pursue these two in separate series] >>> * Introduce auto domains support >>> * Enforce dirty tracking following the IOMMUFD UAPI for this >>> * Add support for toggling hugepages in IOMMUFD >>> * Auto enable support when VF supports migration to use IOMMU >>> when it doesn't have VF dirty tracking >>> * Add a parameter to toggle VF dirty tracking >>> >>> [0] >>> https://lore.kernel.org/qemu-devel/20240201072818.327930-1-zhenzhong.duan@intel.com/ >>> [1] >>> https://lore.kernel.org/qemu-devel/20240201072818.327930-10-zhenzhong.duan@intel.com/ >>> [2] >>> https://lore.kernel.org/qemu-devel/20220428211351.3897-1-joao.m.martins@oracle.com/ >>> [3] >>> https://lore.kernel.org/qemu-devel/20230622214845.3980-1-joao.m.martins@oracle.com/ >>> [4] >>> https://lore.kernel.org/qemu-devel/20240212135643.5858-1-joao.m.martins@oracle.com/ >>> [5] >>> https://lore.kernel.org/qemu-devel/20240708143420.16953-1-joao.m.martins@oracle.com/ >>> [6] >>> https://lore.kernel.org/qemu-devel/20240719120501.81279-1-joao.m.martins@oracle.com/ >>> >>> Joao Martins (9): >>> vfio/iommufd: Introduce auto domain creation >>> vfio/{iommufd,container}: Remove caps::aw_bits >>> vfio/iommufd: Add hw_caps field to HostIOMMUDeviceCaps >>> vfio/{iommufd,container}: Invoke HostIOMMUDevice::realize() during >>> attach_device() >>> vfio/iommufd: Probe and request hwpt dirty tracking capability >>> vfio/iommufd: Implement VFIOIOMMUClass::set_dirty_tracking support >>> vfio/iommufd: Implement VFIOIOMMUClass::query_dirty_bitmap support >>> vfio/migration: Don't block migration device dirty tracking is >>> unsupported >>> vfio/common: Allow disabling device dirty page tracking >>> >>> include/hw/vfio/vfio-common.h | 13 +++ >>> include/sysemu/host_iommu_device.h | 5 +- >>> include/sysemu/iommufd.h | 11 ++ >>> backends/iommufd.c | 85 ++++++++++++++- >>> hw/vfio/common.c | 19 ++-- >>> hw/vfio/container.c | 9 +- >>> hw/vfio/helpers.c | 11 ++ >>> hw/vfio/iommufd.c | 170 ++++++++++++++++++++++++++++- >>> hw/vfio/migration.c | 12 +- >>> hw/vfio/pci.c | 3 + >>> backends/trace-events | 3 + >>> 11 files changed, 318 insertions(+), 23 deletions(-) >> >> Applied to vfio-next with the changes that were discussed this morning. >> Please check. >> > > I think the only thing missing is in the fourth patch to add the comment Eric > suggested (see below). Other than that, looks good to me. > > diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c > index 50ffa4b77090..abb6f1a4b4a8 100644 > --- a/hw/vfio/iommufd.c > +++ b/hw/vfio/iommufd.c > @@ -488,6 +488,13 @@ static bool iommufd_cdev_attach(const char *name, > VFIODevice *vbasedev, > > space = vfio_get_address_space(as); > > + /* > + * 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 call > + * iommufd_backend_get_device_info(). > + */ > if (!vfio_device_hiod_realize(vbasedev, errp)) { > goto err_alloc_ioas; > } > Yep. This is fixed now. I will send a vfio PR in a couple of hours. Thanks, C.
On 23/07/2024 15:23, Yi Liu wrote: > On 2024/7/23 05:13, Joao Martins wrote: >> This small series adds support for IOMMU dirty tracking support via the >> IOMMUFD backend. The hardware capability is available on most recent x86 >> hardware (and these SMMUv3 in upcoming v6.11). The series is divided >> organized as follows: >> >> * Patches 1 - 7: IOMMUFD backend support for dirty tracking; >> >> Introduce auto domains -- Patch 3 goes into more detail, but the gist is that >> we will find and attach a device to a compatible IOMMU domain, or allocate a new >> hardware pagetable *or* rely on kernel IOAS attach (for mdevs). Afterwards the >> workflow is relatively simple: >> >> 1) Probe device and allow dirty tracking in the HWPT >> 2) Toggling dirty tracking on/off >> 3) Read-and-clear of Dirty IOVAs >> >> The heuristics selected for (1) were to always request the HWPT for >> dirty tracking if supported, or rely on device dirty page tracking. This >> is a little simplistic and we aren't necessarily utilizing IOMMU dirty >> tracking even if we ask during hwpt allocation. >> >> The unmap case is deferred until further vIOMMU support with migration >> is added[3] which will then introduce the usage of >> IOMMU_HWPT_GET_DIRTY_BITMAP_NO_CLEAR in GET_DIRTY_BITMAP ioctl in the >> dma unmap bitmap flow. >> >> * Patches 8 - 9: Don't block live migration where there's no VF dirty >> tracker, considering that we have IOMMU dirty tracking. >> >> Comments and feedback appreciated (on patches 1, 5, 8, 9) > > Hi Joao, > > Do you have a github branch for this version? :) > No, but you can probably use Cedric's vfio-next branch: https://github.com/legoater/qemu/tree/vfio-next ... Given that he has just submitted his PR with this series. Joao
On 2024/7/23 05:13, Joao Martins wrote: > This small series adds support for IOMMU dirty tracking support via the > IOMMUFD backend. The hardware capability is available on most recent x86 > hardware (and these SMMUv3 in upcoming v6.11). The series is divided > organized as follows: > > * Patches 1 - 7: IOMMUFD backend support for dirty tracking; > > Introduce auto domains -- Patch 3 goes into more detail, but the gist is that > we will find and attach a device to a compatible IOMMU domain, or allocate a new > hardware pagetable *or* rely on kernel IOAS attach (for mdevs). Afterwards the > workflow is relatively simple: > > 1) Probe device and allow dirty tracking in the HWPT > 2) Toggling dirty tracking on/off > 3) Read-and-clear of Dirty IOVAs > > The heuristics selected for (1) were to always request the HWPT for > dirty tracking if supported, or rely on device dirty page tracking. This > is a little simplistic and we aren't necessarily utilizing IOMMU dirty > tracking even if we ask during hwpt allocation. > > The unmap case is deferred until further vIOMMU support with migration > is added[3] which will then introduce the usage of > IOMMU_HWPT_GET_DIRTY_BITMAP_NO_CLEAR in GET_DIRTY_BITMAP ioctl in the > dma unmap bitmap flow. > > * Patches 8 - 9: Don't block live migration where there's no VF dirty > tracker, considering that we have IOMMU dirty tracking. > > Comments and feedback appreciated (on patches 1, 5, 8, 9) Hi Joao, Do you have a github branch for this version? :) > Cheers, > Joao > > P.S. Suggest v6.11-rc as hypervisor kernel as there's > some bugs fixed there with regards to IOMMU hugepage dirty tracking. > > Changes since v5[6]: > * Remove patches 1-4 as these were commited to vfio-next > * Add the Rb by Cedric and Zhenzhong (previously patches 7, 8, 10, 11) > * Introduce VFIODevice::iommu_dirty_tracking and use it on patch 5, 8 > to store whether we can use IOMMU dirty tracking. > > Changes since v4[5]: > * Add various Reviewed-by in patches 2,3,4,6,8,11 > * Change error messages to mention IOMMU (Zhenzhong) > * Better improve the checking of dirty page tracking in > vfio_migration_realize() to detect per-device IOMMU instead of using > container dirty_page_supported(). > * Improve various commit messages (Eric) > * Extract the caps::hw_caps into its own patch as it was miosleading to > be hidden in another patch (new patch 7) > * Restructure patch 1 helper to be vfio_device_is_mdev() and use > vfio::mdev directly in rest of patches (Cedric) > * Improve error messages of set,query dirty tracking (Cedric) > * Add missing casts to uintptr and uint64_t* (Cedric) > * Add missing commens to struct doc from aw_bits removal (and hw_caps > addition) (Eric) > * Fix the detach flow in auto domains (Eric) > * Set hwpt to NULL on detach (Eric) > * Spurious line (Eric) > > Changes since v3[5]: > * Skip HostIOMMUDevice::realize for mdev, and introduce a helper to check if the VFIO > device is mdev. (Zhenzhong) > * Skip setting IOMMU device for mdev (Zhenzhong) > * Add Zhenzhong review tag in patch 3 > * Utilize vbasedev::bcontainer::dirty_pages_supported instead of introducing > a new HostIOMMUDevice capability and thus remove the cap patch from the series (Zhenzhong) > * Move the HostIOMMUDevice::realize() to be part of VFIODevice initialization in attach_device() > while skipping it all together for mdev. (Cedric) > * Due to the previous item, had to remove aw_bits because it depends on device attach being > finished, instead defer it to when get_cap() gets called. > * Skip auto domains for mdev instead of purposedly erroring out (Zhenzhong) > * Pass errp in all cases, and instead just free the error in case of -EINVAL > in most of all patches, and also pass Error* in iommufd_backend_alloc_hwpt() amd > set/query dirty. This is made better thanks in part to skipping auto domains for mdev (Cedric) > > Changes since RFCv2[4]: > * Always allocate hwpt with IOMMU_HWPT_ALLOC_DIRTY_TRACKING even if > we end up not actually toggling dirty tracking. (Avihai) > * Fix error handling widely in auto domains logic and all patches (Avihai) > * Reuse iommufd_backend_get_device_info() for capabilities (Zhenzhong) > * New patches 1 and 2 taking into consideration previous comments. > * Store hwpt::flags to know if we have dirty tracking (Avihai) > * New patch 8, that allows to query dirty tracking support after > provisioning. This is a cleaner way to check IOMMU dirty tracking support > when vfio::migration is iniitalized, as opposed to RFCv2 via device caps. > device caps way is still used because at vfio attach we aren't yet with > a fully initialized migration state. > * Adopt error propagation in query,set dirty tracking > * Misc improvements overall broadly and Avihai > * Drop hugepages as it's a bit unrelated; I can pursue that patch > * separately. The main motivation is to provide a way to test > without hugepages similar to what vfio_type1_iommu.disable_hugepages=1 > does. > > Changes since RFCv1[2]: > * Remove intel/amd dirty tracking emulation enabling > * Remove the dirtyrate improvement for VF/IOMMU dirty tracking > [Will pursue these two in separate series] > * Introduce auto domains support > * Enforce dirty tracking following the IOMMUFD UAPI for this > * Add support for toggling hugepages in IOMMUFD > * Auto enable support when VF supports migration to use IOMMU > when it doesn't have VF dirty tracking > * Add a parameter to toggle VF dirty tracking > > [0] https://lore.kernel.org/qemu-devel/20240201072818.327930-1-zhenzhong.duan@intel.com/ > [1] https://lore.kernel.org/qemu-devel/20240201072818.327930-10-zhenzhong.duan@intel.com/ > [2] https://lore.kernel.org/qemu-devel/20220428211351.3897-1-joao.m.martins@oracle.com/ > [3] https://lore.kernel.org/qemu-devel/20230622214845.3980-1-joao.m.martins@oracle.com/ > [4] https://lore.kernel.org/qemu-devel/20240212135643.5858-1-joao.m.martins@oracle.com/ > [5] https://lore.kernel.org/qemu-devel/20240708143420.16953-1-joao.m.martins@oracle.com/ > [6] https://lore.kernel.org/qemu-devel/20240719120501.81279-1-joao.m.martins@oracle.com/ > > Joao Martins (9): > vfio/iommufd: Introduce auto domain creation > vfio/{iommufd,container}: Remove caps::aw_bits > vfio/iommufd: Add hw_caps field to HostIOMMUDeviceCaps > vfio/{iommufd,container}: Invoke HostIOMMUDevice::realize() during > attach_device() > vfio/iommufd: Probe and request hwpt dirty tracking capability > vfio/iommufd: Implement VFIOIOMMUClass::set_dirty_tracking support > vfio/iommufd: Implement VFIOIOMMUClass::query_dirty_bitmap support > vfio/migration: Don't block migration device dirty tracking is > unsupported > vfio/common: Allow disabling device dirty page tracking > > include/hw/vfio/vfio-common.h | 13 +++ > include/sysemu/host_iommu_device.h | 5 +- > include/sysemu/iommufd.h | 11 ++ > backends/iommufd.c | 85 ++++++++++++++- > hw/vfio/common.c | 19 ++-- > hw/vfio/container.c | 9 +- > hw/vfio/helpers.c | 11 ++ > hw/vfio/iommufd.c | 170 ++++++++++++++++++++++++++++- > hw/vfio/migration.c | 12 +- > hw/vfio/pci.c | 3 + > backends/trace-events | 3 + > 11 files changed, 318 insertions(+), 23 deletions(-) >
Hello On 7/23/24 16:23, Yi Liu wrote: > On 2024/7/23 05:13, Joao Martins wrote: >> This small series adds support for IOMMU dirty tracking support via the >> IOMMUFD backend. The hardware capability is available on most recent x86 >> hardware (and these SMMUv3 in upcoming v6.11). The series is divided >> organized as follows: >> >> * Patches 1 - 7: IOMMUFD backend support for dirty tracking; >> >> Introduce auto domains -- Patch 3 goes into more detail, but the gist is that >> we will find and attach a device to a compatible IOMMU domain, or allocate a new >> hardware pagetable *or* rely on kernel IOAS attach (for mdevs). Afterwards the >> workflow is relatively simple: >> >> 1) Probe device and allow dirty tracking in the HWPT >> 2) Toggling dirty tracking on/off >> 3) Read-and-clear of Dirty IOVAs >> >> The heuristics selected for (1) were to always request the HWPT for >> dirty tracking if supported, or rely on device dirty page tracking. This >> is a little simplistic and we aren't necessarily utilizing IOMMU dirty >> tracking even if we ask during hwpt allocation. >> >> The unmap case is deferred until further vIOMMU support with migration >> is added[3] which will then introduce the usage of >> IOMMU_HWPT_GET_DIRTY_BITMAP_NO_CLEAR in GET_DIRTY_BITMAP ioctl in the >> dma unmap bitmap flow. >> >> * Patches 8 - 9: Don't block live migration where there's no VF dirty >> tracker, considering that we have IOMMU dirty tracking. >> >> Comments and feedback appreciated (on patches 1, 5, 8, 9) > > Hi Joao, > > Do you have a github branch for this version? :) There has been some updates since. Please use : https://github.com/legoater/qemu/commits/vfio-next or you could wait for the PR to be merged : https://lore.kernel.org/qemu-devel/20240723140019.387786-1-clg@redhat.com/ Thanks, C. > >> Cheers, >> Joao >> >> P.S. Suggest v6.11-rc as hypervisor kernel as there's >> some bugs fixed there with regards to IOMMU hugepage dirty tracking. >> >> Changes since v5[6]: >> * Remove patches 1-4 as these were commited to vfio-next >> * Add the Rb by Cedric and Zhenzhong (previously patches 7, 8, 10, 11) >> * Introduce VFIODevice::iommu_dirty_tracking and use it on patch 5, 8 >> to store whether we can use IOMMU dirty tracking. >> >> Changes since v4[5]: >> * Add various Reviewed-by in patches 2,3,4,6,8,11 >> * Change error messages to mention IOMMU (Zhenzhong) >> * Better improve the checking of dirty page tracking in >> vfio_migration_realize() to detect per-device IOMMU instead of using >> container dirty_page_supported(). >> * Improve various commit messages (Eric) >> * Extract the caps::hw_caps into its own patch as it was miosleading to >> be hidden in another patch (new patch 7) >> * Restructure patch 1 helper to be vfio_device_is_mdev() and use >> vfio::mdev directly in rest of patches (Cedric) >> * Improve error messages of set,query dirty tracking (Cedric) >> * Add missing casts to uintptr and uint64_t* (Cedric) >> * Add missing commens to struct doc from aw_bits removal (and hw_caps >> addition) (Eric) >> * Fix the detach flow in auto domains (Eric) >> * Set hwpt to NULL on detach (Eric) >> * Spurious line (Eric) >> >> Changes since v3[5]: >> * Skip HostIOMMUDevice::realize for mdev, and introduce a helper to check if the VFIO >> device is mdev. (Zhenzhong) >> * Skip setting IOMMU device for mdev (Zhenzhong) >> * Add Zhenzhong review tag in patch 3 >> * Utilize vbasedev::bcontainer::dirty_pages_supported instead of introducing >> a new HostIOMMUDevice capability and thus remove the cap patch from the series (Zhenzhong) >> * Move the HostIOMMUDevice::realize() to be part of VFIODevice initialization in attach_device() >> while skipping it all together for mdev. (Cedric) >> * Due to the previous item, had to remove aw_bits because it depends on device attach being >> finished, instead defer it to when get_cap() gets called. >> * Skip auto domains for mdev instead of purposedly erroring out (Zhenzhong) >> * Pass errp in all cases, and instead just free the error in case of -EINVAL >> in most of all patches, and also pass Error* in iommufd_backend_alloc_hwpt() amd >> set/query dirty. This is made better thanks in part to skipping auto domains for mdev (Cedric) >> >> Changes since RFCv2[4]: >> * Always allocate hwpt with IOMMU_HWPT_ALLOC_DIRTY_TRACKING even if >> we end up not actually toggling dirty tracking. (Avihai) >> * Fix error handling widely in auto domains logic and all patches (Avihai) >> * Reuse iommufd_backend_get_device_info() for capabilities (Zhenzhong) >> * New patches 1 and 2 taking into consideration previous comments. >> * Store hwpt::flags to know if we have dirty tracking (Avihai) >> * New patch 8, that allows to query dirty tracking support after >> provisioning. This is a cleaner way to check IOMMU dirty tracking support >> when vfio::migration is iniitalized, as opposed to RFCv2 via device caps. >> device caps way is still used because at vfio attach we aren't yet with >> a fully initialized migration state. >> * Adopt error propagation in query,set dirty tracking >> * Misc improvements overall broadly and Avihai >> * Drop hugepages as it's a bit unrelated; I can pursue that patch >> * separately. The main motivation is to provide a way to test >> without hugepages similar to what vfio_type1_iommu.disable_hugepages=1 >> does. >> >> Changes since RFCv1[2]: >> * Remove intel/amd dirty tracking emulation enabling >> * Remove the dirtyrate improvement for VF/IOMMU dirty tracking >> [Will pursue these two in separate series] >> * Introduce auto domains support >> * Enforce dirty tracking following the IOMMUFD UAPI for this >> * Add support for toggling hugepages in IOMMUFD >> * Auto enable support when VF supports migration to use IOMMU >> when it doesn't have VF dirty tracking >> * Add a parameter to toggle VF dirty tracking >> >> [0] https://lore.kernel.org/qemu-devel/20240201072818.327930-1-zhenzhong.duan@intel.com/ >> [1] https://lore.kernel.org/qemu-devel/20240201072818.327930-10-zhenzhong.duan@intel.com/ >> [2] https://lore.kernel.org/qemu-devel/20220428211351.3897-1-joao.m.martins@oracle.com/ >> [3] https://lore.kernel.org/qemu-devel/20230622214845.3980-1-joao.m.martins@oracle.com/ >> [4] https://lore.kernel.org/qemu-devel/20240212135643.5858-1-joao.m.martins@oracle.com/ >> [5] https://lore.kernel.org/qemu-devel/20240708143420.16953-1-joao.m.martins@oracle.com/ >> [6] https://lore.kernel.org/qemu-devel/20240719120501.81279-1-joao.m.martins@oracle.com/ >> >> Joao Martins (9): >> vfio/iommufd: Introduce auto domain creation >> vfio/{iommufd,container}: Remove caps::aw_bits >> vfio/iommufd: Add hw_caps field to HostIOMMUDeviceCaps >> vfio/{iommufd,container}: Invoke HostIOMMUDevice::realize() during >> attach_device() >> vfio/iommufd: Probe and request hwpt dirty tracking capability >> vfio/iommufd: Implement VFIOIOMMUClass::set_dirty_tracking support >> vfio/iommufd: Implement VFIOIOMMUClass::query_dirty_bitmap support >> vfio/migration: Don't block migration device dirty tracking is >> unsupported >> vfio/common: Allow disabling device dirty page tracking >> >> include/hw/vfio/vfio-common.h | 13 +++ >> include/sysemu/host_iommu_device.h | 5 +- >> include/sysemu/iommufd.h | 11 ++ >> backends/iommufd.c | 85 ++++++++++++++- >> hw/vfio/common.c | 19 ++-- >> hw/vfio/container.c | 9 +- >> hw/vfio/helpers.c | 11 ++ >> hw/vfio/iommufd.c | 170 ++++++++++++++++++++++++++++- >> hw/vfio/migration.c | 12 +- >> hw/vfio/pci.c | 3 + >> backends/trace-events | 3 + >> 11 files changed, 318 insertions(+), 23 deletions(-) >> >
This small series adds support for IOMMU dirty tracking support via the IOMMUFD backend. The hardware capability is available on most recent x86 hardware (and these SMMUv3 in upcoming v6.11). The series is divided organized as follows: * Patches 1 - 7: IOMMUFD backend support for dirty tracking; Introduce auto domains -- Patch 3 goes into more detail, but the gist is that we will find and attach a device to a compatible IOMMU domain, or allocate a new hardware pagetable *or* rely on kernel IOAS attach (for mdevs). Afterwards the workflow is relatively simple: 1) Probe device and allow dirty tracking in the HWPT 2) Toggling dirty tracking on/off 3) Read-and-clear of Dirty IOVAs The heuristics selected for (1) were to always request the HWPT for dirty tracking if supported, or rely on device dirty page tracking. This is a little simplistic and we aren't necessarily utilizing IOMMU dirty tracking even if we ask during hwpt allocation. The unmap case is deferred until further vIOMMU support with migration is added[3] which will then introduce the usage of IOMMU_HWPT_GET_DIRTY_BITMAP_NO_CLEAR in GET_DIRTY_BITMAP ioctl in the dma unmap bitmap flow. * Patches 8 - 9: Don't block live migration where there's no VF dirty tracker, considering that we have IOMMU dirty tracking. Comments and feedback appreciated (on patches 1, 5, 8, 9) Cheers, Joao P.S. Suggest v6.11-rc as hypervisor kernel as there's some bugs fixed there with regards to IOMMU hugepage dirty tracking. Changes since v5[6]: * Remove patches 1-4 as these were commited to vfio-next * Add the Rb by Cedric and Zhenzhong (previously patches 7, 8, 10, 11) * Introduce VFIODevice::iommu_dirty_tracking and use it on patch 5, 8 to store whether we can use IOMMU dirty tracking. Changes since v4[5]: * Add various Reviewed-by in patches 2,3,4,6,8,11 * Change error messages to mention IOMMU (Zhenzhong) * Better improve the checking of dirty page tracking in vfio_migration_realize() to detect per-device IOMMU instead of using container dirty_page_supported(). * Improve various commit messages (Eric) * Extract the caps::hw_caps into its own patch as it was miosleading to be hidden in another patch (new patch 7) * Restructure patch 1 helper to be vfio_device_is_mdev() and use vfio::mdev directly in rest of patches (Cedric) * Improve error messages of set,query dirty tracking (Cedric) * Add missing casts to uintptr and uint64_t* (Cedric) * Add missing commens to struct doc from aw_bits removal (and hw_caps addition) (Eric) * Fix the detach flow in auto domains (Eric) * Set hwpt to NULL on detach (Eric) * Spurious line (Eric) Changes since v3[5]: * Skip HostIOMMUDevice::realize for mdev, and introduce a helper to check if the VFIO device is mdev. (Zhenzhong) * Skip setting IOMMU device for mdev (Zhenzhong) * Add Zhenzhong review tag in patch 3 * Utilize vbasedev::bcontainer::dirty_pages_supported instead of introducing a new HostIOMMUDevice capability and thus remove the cap patch from the series (Zhenzhong) * Move the HostIOMMUDevice::realize() to be part of VFIODevice initialization in attach_device() while skipping it all together for mdev. (Cedric) * Due to the previous item, had to remove aw_bits because it depends on device attach being finished, instead defer it to when get_cap() gets called. * Skip auto domains for mdev instead of purposedly erroring out (Zhenzhong) * Pass errp in all cases, and instead just free the error in case of -EINVAL in most of all patches, and also pass Error* in iommufd_backend_alloc_hwpt() amd set/query dirty. This is made better thanks in part to skipping auto domains for mdev (Cedric) Changes since RFCv2[4]: * Always allocate hwpt with IOMMU_HWPT_ALLOC_DIRTY_TRACKING even if we end up not actually toggling dirty tracking. (Avihai) * Fix error handling widely in auto domains logic and all patches (Avihai) * Reuse iommufd_backend_get_device_info() for capabilities (Zhenzhong) * New patches 1 and 2 taking into consideration previous comments. * Store hwpt::flags to know if we have dirty tracking (Avihai) * New patch 8, that allows to query dirty tracking support after provisioning. This is a cleaner way to check IOMMU dirty tracking support when vfio::migration is iniitalized, as opposed to RFCv2 via device caps. device caps way is still used because at vfio attach we aren't yet with a fully initialized migration state. * Adopt error propagation in query,set dirty tracking * Misc improvements overall broadly and Avihai * Drop hugepages as it's a bit unrelated; I can pursue that patch * separately. The main motivation is to provide a way to test without hugepages similar to what vfio_type1_iommu.disable_hugepages=1 does. Changes since RFCv1[2]: * Remove intel/amd dirty tracking emulation enabling * Remove the dirtyrate improvement for VF/IOMMU dirty tracking [Will pursue these two in separate series] * Introduce auto domains support * Enforce dirty tracking following the IOMMUFD UAPI for this * Add support for toggling hugepages in IOMMUFD * Auto enable support when VF supports migration to use IOMMU when it doesn't have VF dirty tracking * Add a parameter to toggle VF dirty tracking [0] https://lore.kernel.org/qemu-devel/20240201072818.327930-1-zhenzhong.duan@intel.com/ [1] https://lore.kernel.org/qemu-devel/20240201072818.327930-10-zhenzhong.duan@intel.com/ [2] https://lore.kernel.org/qemu-devel/20220428211351.3897-1-joao.m.martins@oracle.com/ [3] https://lore.kernel.org/qemu-devel/20230622214845.3980-1-joao.m.martins@oracle.com/ [4] https://lore.kernel.org/qemu-devel/20240212135643.5858-1-joao.m.martins@oracle.com/ [5] https://lore.kernel.org/qemu-devel/20240708143420.16953-1-joao.m.martins@oracle.com/ [6] https://lore.kernel.org/qemu-devel/20240719120501.81279-1-joao.m.martins@oracle.com/ Joao Martins (9): vfio/iommufd: Introduce auto domain creation vfio/{iommufd,container}: Remove caps::aw_bits vfio/iommufd: Add hw_caps field to HostIOMMUDeviceCaps vfio/{iommufd,container}: Invoke HostIOMMUDevice::realize() during attach_device() vfio/iommufd: Probe and request hwpt dirty tracking capability vfio/iommufd: Implement VFIOIOMMUClass::set_dirty_tracking support vfio/iommufd: Implement VFIOIOMMUClass::query_dirty_bitmap support vfio/migration: Don't block migration device dirty tracking is unsupported vfio/common: Allow disabling device dirty page tracking include/hw/vfio/vfio-common.h | 13 +++ include/sysemu/host_iommu_device.h | 5 +- include/sysemu/iommufd.h | 11 ++ backends/iommufd.c | 85 ++++++++++++++- hw/vfio/common.c | 19 ++-- hw/vfio/container.c | 9 +- hw/vfio/helpers.c | 11 ++ hw/vfio/iommufd.c | 170 ++++++++++++++++++++++++++++- hw/vfio/migration.c | 12 +- hw/vfio/pci.c | 3 + backends/trace-events | 3 + 11 files changed, 318 insertions(+), 23 deletions(-)