| Message ID | 20240212135643.5858-4-joao.m.martins@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio/iommufd: IOMMUFD Dirty Tracking | expand |
Hi Joao, On 12/02/2024 15:56, Joao Martins wrote: > External email: Use caution opening links or attachments > > > Probe hardware dirty tracking support by querying device hw capabilities > via IOMMUFD_GET_HW_INFO. > > In preparation to using the dirty tracking UAPI, request dirty tracking in > the HWPT flags when the device doesn't support dirty page tracking or has > it disabled; or when support when the VF backing IOMMU supports dirty > tracking. The latter is in the possibility of a device being attached > that doesn't have a dirty tracker. > > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > --- > hw/vfio/common.c | 18 ++++++++++++++++++ > hw/vfio/iommufd.c | 25 ++++++++++++++++++++++++- > include/hw/vfio/vfio-common.h | 2 ++ > 3 files changed, 44 insertions(+), 1 deletion(-) > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index f7f85160be88..d8fc7077f839 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -216,6 +216,24 @@ bool vfio_devices_all_device_dirty_tracking(const VFIOContainerBase *bcontainer) > return true; > } > > +bool vfio_device_migration_supported(VFIODevice *vbasedev) > +{ > + if (!vbasedev->migration) { > + return false; > + } > + > + return vbasedev->migration->mig_flags & VFIO_MIGRATION_STOP_COPY; I think this is redundant, as (vbasedev->migration != NULL) implies (vbasedev->migration->mig_flags & VFIO_MIGRATION_STOP_COPY) == true. > +} > + > +bool vfio_device_dirty_pages_supported(VFIODevice *vbasedev) > +{ > + if (vbasedev->pre_copy_dirty_page_tracking == ON_OFF_AUTO_OFF) { > + return false; > + } > + > + return !vbasedev->dirty_pages_supported; > +} > + > /* > * Check if all VFIO devices are running and migration is active, which is > * essentially equivalent to the migration being in pre-copy phase. > diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c > index ca7ec45e725c..edacb6d72748 100644 > --- a/hw/vfio/iommufd.c > +++ b/hw/vfio/iommufd.c > @@ -219,11 +219,26 @@ static int iommufd_cdev_detach_ioas_hwpt(VFIODevice *vbasedev, Error **errp) > return ret; > } > > +static bool iommufd_dirty_pages_supported(IOMMUFDDevice *iommufd_dev, > + Error **errp) > +{ > + uint64_t caps; > + int r; > + > + r = iommufd_device_get_hw_capabilities(iommufd_dev, &caps, errp); > + if (r) { > + return false; > + } > + > + return caps & IOMMU_HW_CAP_DIRTY_TRACKING; The false return value of this function is overloaded, it can indicate both error and lack of DPT support. Should we fail iommufd_cdev_autodomains_get() if iommufd_dirty_pages_supported() fails? Otherwise, errp argument of iommufd_dirty_pages_supported() is redundant and we can handle iommufd_device_get_hw_capabilities() error locally. > +} > + > static int iommufd_cdev_autodomains_get(VFIODevice *vbasedev, > VFIOIOMMUFDContainer *container, > Error **errp) > { > int iommufd = vbasedev->iommufd_dev.iommufd->fd; > + uint32_t flags = 0; > VFIOIOASHwpt *hwpt; > Error *err = NULL; > int ret = -EINVAL; > @@ -245,9 +260,15 @@ static int iommufd_cdev_autodomains_get(VFIODevice *vbasedev, > } > } > > + if ((vfio_device_migration_supported(vbasedev) && > + !vfio_device_dirty_pages_supported(vbasedev)) || > + iommufd_dirty_pages_supported(&vbasedev->iommufd_dev, &err)) { I think it's too early to check vfio_device_migration_supported() and vfio_device_dirty_pages_supported() here, as vfio_migration_init() hasn't been called yet so vbasedev->migration and vbasedev->dirty_pages_supported are not initialized. Why do we need to check this? Can't we simply request IOMMUFD DPT if it's supported? Thanks. > + flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING; > + } > + > ret = iommufd_backend_alloc_hwpt(iommufd, > vbasedev->iommufd_dev.devid, > - container->ioas_id, 0, 0, 0, > + container->ioas_id, flags, 0, 0, > NULL, &hwpt_id); > if (ret) { > error_append_hint(&err, > @@ -271,6 +292,8 @@ static int iommufd_cdev_autodomains_get(VFIODevice *vbasedev, > vbasedev->hwpt = hwpt; > QLIST_INSERT_HEAD(&hwpt->device_list, vbasedev, hwpt_next); > QLIST_INSERT_HEAD(&container->hwpt_list, hwpt, next); > + container->bcontainer.dirty_pages_supported = > + (flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING); > return 0; > } > > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h > index 7f7d823221e2..a3e691c126c6 100644 > --- a/include/hw/vfio/vfio-common.h > +++ b/include/hw/vfio/vfio-common.h > @@ -271,6 +271,8 @@ bool > vfio_devices_all_running_and_mig_active(const VFIOContainerBase *bcontainer); > bool > vfio_devices_all_device_dirty_tracking(const VFIOContainerBase *bcontainer); > +bool vfio_device_migration_supported(VFIODevice *vbasedev); > +bool vfio_device_dirty_pages_supported(VFIODevice *vbasedev); > int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer, > VFIOBitmap *vbmap, hwaddr iova, > hwaddr size); > -- > 2.39.3 >
On 19/02/2024 09:03, Avihai Horon wrote: > Hi Joao, > > On 12/02/2024 15:56, Joao Martins wrote: >> External email: Use caution opening links or attachments >> >> >> Probe hardware dirty tracking support by querying device hw capabilities >> via IOMMUFD_GET_HW_INFO. >> >> In preparation to using the dirty tracking UAPI, request dirty tracking in >> the HWPT flags when the device doesn't support dirty page tracking or has >> it disabled; or when support when the VF backing IOMMU supports dirty >> tracking. The latter is in the possibility of a device being attached >> that doesn't have a dirty tracker. >> >> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >> --- >> hw/vfio/common.c | 18 ++++++++++++++++++ >> hw/vfio/iommufd.c | 25 ++++++++++++++++++++++++- >> include/hw/vfio/vfio-common.h | 2 ++ >> 3 files changed, 44 insertions(+), 1 deletion(-) >> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index f7f85160be88..d8fc7077f839 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -216,6 +216,24 @@ bool vfio_devices_all_device_dirty_tracking(const >> VFIOContainerBase *bcontainer) >> return true; >> } >> >> +bool vfio_device_migration_supported(VFIODevice *vbasedev) >> +{ >> + if (!vbasedev->migration) { >> + return false; >> + } >> + >> + return vbasedev->migration->mig_flags & VFIO_MIGRATION_STOP_COPY; > > I think this is redundant, as (vbasedev->migration != NULL) implies > (vbasedev->migration->mig_flags & VFIO_MIGRATION_STOP_COPY) == true. > The check was there to prevent a null-deref in case the device didn't support migration. >> +} >> + >> +bool vfio_device_dirty_pages_supported(VFIODevice *vbasedev) >> +{ >> + if (vbasedev->pre_copy_dirty_page_tracking == ON_OFF_AUTO_OFF) { >> + return false; >> + } >> + >> + return !vbasedev->dirty_pages_supported; >> +} >> + >> /* >> * Check if all VFIO devices are running and migration is active, which is >> * essentially equivalent to the migration being in pre-copy phase. >> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c >> index ca7ec45e725c..edacb6d72748 100644 >> --- a/hw/vfio/iommufd.c >> +++ b/hw/vfio/iommufd.c >> @@ -219,11 +219,26 @@ static int iommufd_cdev_detach_ioas_hwpt(VFIODevice >> *vbasedev, Error **errp) >> return ret; >> } >> >> +static bool iommufd_dirty_pages_supported(IOMMUFDDevice *iommufd_dev, >> + Error **errp) >> +{ >> + uint64_t caps; >> + int r; >> + >> + r = iommufd_device_get_hw_capabilities(iommufd_dev, &caps, errp); >> + if (r) { >> + return false; >> + } >> + >> + return caps & IOMMU_HW_CAP_DIRTY_TRACKING; > > The false return value of this function is overloaded, it can indicate both > error and lack of DPT support. > Should we fail iommufd_cdev_autodomains_get() if iommufd_dirty_pages_supported() > fails? Definitely not. > Otherwise, errp argument of iommufd_dirty_pages_supported() is redundant and we > can handle iommufd_device_get_hw_capabilities() error locally. > I'll handle locally. >> +} >> + >> static int iommufd_cdev_autodomains_get(VFIODevice *vbasedev, >> VFIOIOMMUFDContainer *container, >> Error **errp) >> { >> int iommufd = vbasedev->iommufd_dev.iommufd->fd; >> + uint32_t flags = 0; >> VFIOIOASHwpt *hwpt; >> Error *err = NULL; >> int ret = -EINVAL; >> @@ -245,9 +260,15 @@ static int iommufd_cdev_autodomains_get(VFIODevice >> *vbasedev, >> } >> } >> >> + if ((vfio_device_migration_supported(vbasedev) && >> + !vfio_device_dirty_pages_supported(vbasedev)) || >> + iommufd_dirty_pages_supported(&vbasedev->iommufd_dev, &err)) { > > I think it's too early to check vfio_device_migration_supported() and > vfio_device_dirty_pages_supported() here, as vfio_migration_init() hasn't been > called yet so vbasedev->migration and vbasedev->dirty_pages_supported are not > initialized. I should replace with its own vfio device probing but the next point invalidates this > Why do we need to check this? Can't we simply request IOMMUFD DPT if it's > supported? > There's no point in force requesting dpt in the domain if the device doesn't do migration that was my thinking here; but otoh as past hotplug bug fixes have shown it needs to proof against a new device getting add up that supports migration while and the unsupported one be removed. So I guess we might not have another option but to always ask for it if supported. > Thanks. > >> + flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING; >> + } >> + >> ret = iommufd_backend_alloc_hwpt(iommufd, >> vbasedev->iommufd_dev.devid, >> - container->ioas_id, 0, 0, 0, >> + container->ioas_id, flags, 0, 0, >> NULL, &hwpt_id); >> if (ret) { >> error_append_hint(&err, >> @@ -271,6 +292,8 @@ static int iommufd_cdev_autodomains_get(VFIODevice *vbasedev, >> vbasedev->hwpt = hwpt; >> QLIST_INSERT_HEAD(&hwpt->device_list, vbasedev, hwpt_next); >> QLIST_INSERT_HEAD(&container->hwpt_list, hwpt, next); >> + container->bcontainer.dirty_pages_supported = >> + (flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING); >> return 0; >> } >> >> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h >> index 7f7d823221e2..a3e691c126c6 100644 >> --- a/include/hw/vfio/vfio-common.h >> +++ b/include/hw/vfio/vfio-common.h >> @@ -271,6 +271,8 @@ bool >> vfio_devices_all_running_and_mig_active(const VFIOContainerBase *bcontainer); >> bool >> vfio_devices_all_device_dirty_tracking(const VFIOContainerBase *bcontainer); >> +bool vfio_device_migration_supported(VFIODevice *vbasedev); >> +bool vfio_device_dirty_pages_supported(VFIODevice *vbasedev); >> int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer, >> VFIOBitmap *vbmap, hwaddr iova, >> hwaddr size); >> -- >> 2.39.3 >>
On 20/02/2024 12:51, Joao Martins wrote: > External email: Use caution opening links or attachments > > > On 19/02/2024 09:03, Avihai Horon wrote: >> Hi Joao, >> >> On 12/02/2024 15:56, Joao Martins wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> Probe hardware dirty tracking support by querying device hw capabilities >>> via IOMMUFD_GET_HW_INFO. >>> >>> In preparation to using the dirty tracking UAPI, request dirty tracking in >>> the HWPT flags when the device doesn't support dirty page tracking or has >>> it disabled; or when support when the VF backing IOMMU supports dirty >>> tracking. The latter is in the possibility of a device being attached >>> that doesn't have a dirty tracker. >>> >>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >>> --- >>> hw/vfio/common.c | 18 ++++++++++++++++++ >>> hw/vfio/iommufd.c | 25 ++++++++++++++++++++++++- >>> include/hw/vfio/vfio-common.h | 2 ++ >>> 3 files changed, 44 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >>> index f7f85160be88..d8fc7077f839 100644 >>> --- a/hw/vfio/common.c >>> +++ b/hw/vfio/common.c >>> @@ -216,6 +216,24 @@ bool vfio_devices_all_device_dirty_tracking(const >>> VFIOContainerBase *bcontainer) >>> return true; >>> } >>> >>> +bool vfio_device_migration_supported(VFIODevice *vbasedev) >>> +{ >>> + if (!vbasedev->migration) { >>> + return false; >>> + } >>> + >>> + return vbasedev->migration->mig_flags & VFIO_MIGRATION_STOP_COPY; >> I think this is redundant, as (vbasedev->migration != NULL) implies >> (vbasedev->migration->mig_flags & VFIO_MIGRATION_STOP_COPY) == true. >> > The check was there to prevent a null-deref in case the device didn't support > migration. I meant that "vbasedev->migration->mig_flags & VFIO_MIGRATION_STOP_COPY" check is redundant because if vbasedev->migration != NULL then VFIO_MIGRATION_STOP_COPY is supported (it's already checked in vfio_migration_init()). But never mind, given what you wrote below. > >>> +} >>> + >>> +bool vfio_device_dirty_pages_supported(VFIODevice *vbasedev) >>> +{ >>> + if (vbasedev->pre_copy_dirty_page_tracking == ON_OFF_AUTO_OFF) { >>> + return false; >>> + } >>> + >>> + return !vbasedev->dirty_pages_supported; >>> +} >>> + >>> /* >>> * Check if all VFIO devices are running and migration is active, which is >>> * essentially equivalent to the migration being in pre-copy phase. >>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c >>> index ca7ec45e725c..edacb6d72748 100644 >>> --- a/hw/vfio/iommufd.c >>> +++ b/hw/vfio/iommufd.c >>> @@ -219,11 +219,26 @@ static int iommufd_cdev_detach_ioas_hwpt(VFIODevice >>> *vbasedev, Error **errp) >>> return ret; >>> } >>> >>> +static bool iommufd_dirty_pages_supported(IOMMUFDDevice *iommufd_dev, >>> + Error **errp) >>> +{ >>> + uint64_t caps; >>> + int r; >>> + >>> + r = iommufd_device_get_hw_capabilities(iommufd_dev, &caps, errp); >>> + if (r) { >>> + return false; >>> + } >>> + >>> + return caps & IOMMU_HW_CAP_DIRTY_TRACKING; >> The false return value of this function is overloaded, it can indicate both >> error and lack of DPT support. >> Should we fail iommufd_cdev_autodomains_get() if iommufd_dirty_pages_supported() >> fails? > Definitely not. > >> Otherwise, errp argument of iommufd_dirty_pages_supported() is redundant and we >> can handle iommufd_device_get_hw_capabilities() error locally. >> > I'll handle locally. > >>> +} >>> + >>> static int iommufd_cdev_autodomains_get(VFIODevice *vbasedev, >>> VFIOIOMMUFDContainer *container, >>> Error **errp) >>> { >>> int iommufd = vbasedev->iommufd_dev.iommufd->fd; >>> + uint32_t flags = 0; >>> VFIOIOASHwpt *hwpt; >>> Error *err = NULL; >>> int ret = -EINVAL; >>> @@ -245,9 +260,15 @@ static int iommufd_cdev_autodomains_get(VFIODevice >>> *vbasedev, >>> } >>> } >>> >>> + if ((vfio_device_migration_supported(vbasedev) && >>> + !vfio_device_dirty_pages_supported(vbasedev)) || >>> + iommufd_dirty_pages_supported(&vbasedev->iommufd_dev, &err)) { >> I think it's too early to check vfio_device_migration_supported() and >> vfio_device_dirty_pages_supported() here, as vfio_migration_init() hasn't been >> called yet so vbasedev->migration and vbasedev->dirty_pages_supported are not >> initialized. > I should replace with its own vfio device probing but the next point invalidates > this > >> Why do we need to check this? Can't we simply request IOMMUFD DPT if it's >> supported? >> > There's no point in force requesting dpt in the domain if the device doesn't do > migration that was my thinking here; but otoh as past hotplug bug fixes have > shown it needs to proof against a new device getting add up that supports > migration while and the unsupported one be removed. So I guess we might not have > another option but to always ask for it if supported. > >> Thanks. >> >>> + flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING; >>> + } >>> + >>> ret = iommufd_backend_alloc_hwpt(iommufd, >>> vbasedev->iommufd_dev.devid, >>> - container->ioas_id, 0, 0, 0, >>> + container->ioas_id, flags, 0, 0, >>> NULL, &hwpt_id); >>> if (ret) { >>> error_append_hint(&err, >>> @@ -271,6 +292,8 @@ static int iommufd_cdev_autodomains_get(VFIODevice *vbasedev, >>> vbasedev->hwpt = hwpt; >>> QLIST_INSERT_HEAD(&hwpt->device_list, vbasedev, hwpt_next); >>> QLIST_INSERT_HEAD(&container->hwpt_list, hwpt, next); >>> + container->bcontainer.dirty_pages_supported = >>> + (flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING); >>> return 0; >>> } >>> >>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h >>> index 7f7d823221e2..a3e691c126c6 100644 >>> --- a/include/hw/vfio/vfio-common.h >>> +++ b/include/hw/vfio/vfio-common.h >>> @@ -271,6 +271,8 @@ bool >>> vfio_devices_all_running_and_mig_active(const VFIOContainerBase *bcontainer); >>> bool >>> vfio_devices_all_device_dirty_tracking(const VFIOContainerBase *bcontainer); >>> +bool vfio_device_migration_supported(VFIODevice *vbasedev); >>> +bool vfio_device_dirty_pages_supported(VFIODevice *vbasedev); >>> int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer, >>> VFIOBitmap *vbmap, hwaddr iova, >>> hwaddr size); >>> -- >>> 2.39.3 >>>
diff --git a/hw/vfio/common.c b/hw/vfio/common.c index f7f85160be88..d8fc7077f839 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -216,6 +216,24 @@ bool vfio_devices_all_device_dirty_tracking(const VFIOContainerBase *bcontainer) return true; } +bool vfio_device_migration_supported(VFIODevice *vbasedev) +{ + if (!vbasedev->migration) { + return false; + } + + return vbasedev->migration->mig_flags & VFIO_MIGRATION_STOP_COPY; +} + +bool vfio_device_dirty_pages_supported(VFIODevice *vbasedev) +{ + if (vbasedev->pre_copy_dirty_page_tracking == ON_OFF_AUTO_OFF) { + return false; + } + + return !vbasedev->dirty_pages_supported; +} + /* * Check if all VFIO devices are running and migration is active, which is * essentially equivalent to the migration being in pre-copy phase. diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c index ca7ec45e725c..edacb6d72748 100644 --- a/hw/vfio/iommufd.c +++ b/hw/vfio/iommufd.c @@ -219,11 +219,26 @@ static int iommufd_cdev_detach_ioas_hwpt(VFIODevice *vbasedev, Error **errp) return ret; } +static bool iommufd_dirty_pages_supported(IOMMUFDDevice *iommufd_dev, + Error **errp) +{ + uint64_t caps; + int r; + + r = iommufd_device_get_hw_capabilities(iommufd_dev, &caps, errp); + if (r) { + return false; + } + + return caps & IOMMU_HW_CAP_DIRTY_TRACKING; +} + static int iommufd_cdev_autodomains_get(VFIODevice *vbasedev, VFIOIOMMUFDContainer *container, Error **errp) { int iommufd = vbasedev->iommufd_dev.iommufd->fd; + uint32_t flags = 0; VFIOIOASHwpt *hwpt; Error *err = NULL; int ret = -EINVAL; @@ -245,9 +260,15 @@ static int iommufd_cdev_autodomains_get(VFIODevice *vbasedev, } } + if ((vfio_device_migration_supported(vbasedev) && + !vfio_device_dirty_pages_supported(vbasedev)) || + iommufd_dirty_pages_supported(&vbasedev->iommufd_dev, &err)) { + flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING; + } + ret = iommufd_backend_alloc_hwpt(iommufd, vbasedev->iommufd_dev.devid, - container->ioas_id, 0, 0, 0, + container->ioas_id, flags, 0, 0, NULL, &hwpt_id); if (ret) { error_append_hint(&err, @@ -271,6 +292,8 @@ static int iommufd_cdev_autodomains_get(VFIODevice *vbasedev, vbasedev->hwpt = hwpt; QLIST_INSERT_HEAD(&hwpt->device_list, vbasedev, hwpt_next); QLIST_INSERT_HEAD(&container->hwpt_list, hwpt, next); + container->bcontainer.dirty_pages_supported = + (flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING); return 0; } diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h index 7f7d823221e2..a3e691c126c6 100644 --- a/include/hw/vfio/vfio-common.h +++ b/include/hw/vfio/vfio-common.h @@ -271,6 +271,8 @@ bool vfio_devices_all_running_and_mig_active(const VFIOContainerBase *bcontainer); bool vfio_devices_all_device_dirty_tracking(const VFIOContainerBase *bcontainer); +bool vfio_device_migration_supported(VFIODevice *vbasedev); +bool vfio_device_dirty_pages_supported(VFIODevice *vbasedev); int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer, VFIOBitmap *vbmap, hwaddr iova, hwaddr size);
Probe hardware dirty tracking support by querying device hw capabilities via IOMMUFD_GET_HW_INFO. In preparation to using the dirty tracking UAPI, request dirty tracking in the HWPT flags when the device doesn't support dirty page tracking or has it disabled; or when support when the VF backing IOMMU supports dirty tracking. The latter is in the possibility of a device being attached that doesn't have a dirty tracker. Signed-off-by: Joao Martins <joao.m.martins@oracle.com> --- hw/vfio/common.c | 18 ++++++++++++++++++ hw/vfio/iommufd.c | 25 ++++++++++++++++++++++++- include/hw/vfio/vfio-common.h | 2 ++ 3 files changed, 44 insertions(+), 1 deletion(-)