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.mart...@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 >>