| Message ID | 20240719120501.81279-10-joao.m.martins@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/iommufd: IOMMUFD Dirty Tracking | expand |
>-----Original Message----- >From: Joao Martins <joao.m.martins@oracle.com> >Subject: [PATCH v5 09/13] vfio/iommufd: Probe and request hwpt dirty >tracking capability > >In preparation to using the dirty tracking UAPI, probe whether the IOMMU >supports dirty tracking. This is done via the data stored in >hiod::caps::hw_caps initialized from GET_HW_INFO. > >Qemu doesn't know if VF dirty tracking is supported when allocating >hardware pagetable in iommufd_cdev_autodomains_get(). This is because >VFIODevice migration state hasn't been initialized *yet* hence it can't pick >between VF dirty tracking vs IOMMU dirty tracking. So, if IOMMU supports >dirty tracking it always creates HWPTs with >IOMMU_HWPT_ALLOC_DIRTY_TRACKING >even if later on VFIOMigration decides to use VF dirty tracking instead. I thought there is no overhead for HWPT with IOMMU_HWPT_ALLOC_DIRTY_TRACKING vs. HWPT without IOMMU_HWPT_ALLOC_DIRTY_TRACKING if we don't enable dirty tracking. Right? > >Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >--- > include/hw/vfio/vfio-common.h | 1 + > hw/vfio/iommufd.c | 19 +++++++++++++++++++ > 2 files changed, 20 insertions(+) > >diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio- >common.h >index 4e44b26d3c45..7e530c7869dc 100644 >--- a/include/hw/vfio/vfio-common.h >+++ b/include/hw/vfio/vfio-common.h >@@ -97,6 +97,7 @@ typedef struct IOMMUFDBackend IOMMUFDBackend; > > typedef struct VFIOIOASHwpt { > uint32_t hwpt_id; >+ uint32_t hwpt_flags; > QLIST_HEAD(, VFIODevice) device_list; > QLIST_ENTRY(VFIOIOASHwpt) next; > } VFIOIOASHwpt; >diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c >index bb44d948c735..2e5c207bbca0 100644 >--- a/hw/vfio/iommufd.c >+++ b/hw/vfio/iommufd.c >@@ -110,6 +110,11 @@ static void >iommufd_cdev_unbind_and_disconnect(VFIODevice *vbasedev) > iommufd_backend_disconnect(vbasedev->iommufd); > } > >+static bool iommufd_hwpt_dirty_tracking(VFIOIOASHwpt *hwpt) >+{ >+ return hwpt && hwpt->hwpt_flags & >IOMMU_HWPT_ALLOC_DIRTY_TRACKING; >+} >+ > static int iommufd_cdev_getfd(const char *sysfs_path, Error **errp) > { > ERRP_GUARD(); >@@ -246,6 +251,17 @@ static bool >iommufd_cdev_autodomains_get(VFIODevice *vbasedev, > } > } > >+ /* >+ * This is quite early and VFIO Migration state isn't yet fully >+ * initialized, thus rely only on IOMMU hardware capabilities as to >+ * whether IOMMU dirty tracking is going to be requested. Later >+ * vfio_migration_realize() may decide to use VF dirty tracking >+ * instead. >+ */ >+ if (vbasedev->hiod->caps.hw_caps & >IOMMU_HW_CAP_DIRTY_TRACKING) { Looks there is still reference to hw_caps, then would suggest to bring back the NEW CAP. >+ flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING; >+ } >+ > if (!iommufd_backend_alloc_hwpt(iommufd, vbasedev->devid, > container->ioas_id, flags, > IOMMU_HWPT_DATA_NONE, 0, NULL, >@@ -255,6 +271,7 @@ static bool >iommufd_cdev_autodomains_get(VFIODevice *vbasedev, > > hwpt = g_malloc0(sizeof(*hwpt)); > hwpt->hwpt_id = hwpt_id; >+ hwpt->hwpt_flags = flags; > QLIST_INIT(&hwpt->device_list); > > ret = iommufd_cdev_attach_ioas_hwpt(vbasedev, hwpt->hwpt_id, errp); >@@ -267,6 +284,8 @@ static bool >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 |= >+ iommufd_hwpt_dirty_tracking(hwpt); If there is at least one hwpt without dirty tracking, shouldn't we make bcontainer.dirty_pages_supported false? Thanks Zhenzhong > return true; > } > >-- >2.17.2
On 22/07/2024 07:05, Duan, Zhenzhong wrote: > > >> -----Original Message----- >> From: Joao Martins <joao.m.martins@oracle.com> >> Subject: [PATCH v5 09/13] vfio/iommufd: Probe and request hwpt dirty >> tracking capability >> >> In preparation to using the dirty tracking UAPI, probe whether the IOMMU >> supports dirty tracking. This is done via the data stored in >> hiod::caps::hw_caps initialized from GET_HW_INFO. >> >> Qemu doesn't know if VF dirty tracking is supported when allocating >> hardware pagetable in iommufd_cdev_autodomains_get(). This is because >> VFIODevice migration state hasn't been initialized *yet* hence it can't pick >> between VF dirty tracking vs IOMMU dirty tracking. So, if IOMMU supports >> dirty tracking it always creates HWPTs with >> IOMMU_HWPT_ALLOC_DIRTY_TRACKING >> even if later on VFIOMigration decides to use VF dirty tracking instead. > > I thought there is no overhead for HWPT with IOMMU_HWPT_ALLOC_DIRTY_TRACKING vs. HWPT without IOMMU_HWPT_ALLOC_DIRTY_TRACKING if we don't enable dirty tracking. Right? > Correct. >> >> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >> --- >> include/hw/vfio/vfio-common.h | 1 + >> hw/vfio/iommufd.c | 19 +++++++++++++++++++ >> 2 files changed, 20 insertions(+) >> >> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio- >> common.h >> index 4e44b26d3c45..7e530c7869dc 100644 >> --- a/include/hw/vfio/vfio-common.h >> +++ b/include/hw/vfio/vfio-common.h >> @@ -97,6 +97,7 @@ typedef struct IOMMUFDBackend IOMMUFDBackend; >> >> typedef struct VFIOIOASHwpt { >> uint32_t hwpt_id; >> + uint32_t hwpt_flags; >> QLIST_HEAD(, VFIODevice) device_list; >> QLIST_ENTRY(VFIOIOASHwpt) next; >> } VFIOIOASHwpt; >> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c >> index bb44d948c735..2e5c207bbca0 100644 >> --- a/hw/vfio/iommufd.c >> +++ b/hw/vfio/iommufd.c >> @@ -110,6 +110,11 @@ static void >> iommufd_cdev_unbind_and_disconnect(VFIODevice *vbasedev) >> iommufd_backend_disconnect(vbasedev->iommufd); >> } >> >> +static bool iommufd_hwpt_dirty_tracking(VFIOIOASHwpt *hwpt) >> +{ >> + return hwpt && hwpt->hwpt_flags & >> IOMMU_HWPT_ALLOC_DIRTY_TRACKING; >> +} >> + >> static int iommufd_cdev_getfd(const char *sysfs_path, Error **errp) >> { >> ERRP_GUARD(); >> @@ -246,6 +251,17 @@ static bool >> iommufd_cdev_autodomains_get(VFIODevice *vbasedev, >> } >> } >> >> + /* >> + * This is quite early and VFIO Migration state isn't yet fully >> + * initialized, thus rely only on IOMMU hardware capabilities as to >> + * whether IOMMU dirty tracking is going to be requested. Later >> + * vfio_migration_realize() may decide to use VF dirty tracking >> + * instead. >> + */ >> + if (vbasedev->hiod->caps.hw_caps & >> IOMMU_HW_CAP_DIRTY_TRACKING) { > > Looks there is still reference to hw_caps, then would suggest to bring back the NEW CAP. > Ah, but below helper is checking for GET_HW_INFO stuff, and not hwpt flags gioven that we haven't allocated a hwpt yet. While I could place this check into a helper it would only have an user. I will need below helper iommufd_hwpt_dirty_tracking() in another patch, so this is a bit of a one off check only (unless we want a new helper for cosmetic purposes) >> + flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING; >> + } >> + >> if (!iommufd_backend_alloc_hwpt(iommufd, vbasedev->devid, >> container->ioas_id, flags, >> IOMMU_HWPT_DATA_NONE, 0, NULL, >> @@ -255,6 +271,7 @@ static bool >> iommufd_cdev_autodomains_get(VFIODevice *vbasedev, >> >> hwpt = g_malloc0(sizeof(*hwpt)); >> hwpt->hwpt_id = hwpt_id; >> + hwpt->hwpt_flags = flags; >> QLIST_INIT(&hwpt->device_list); >> >> ret = iommufd_cdev_attach_ioas_hwpt(vbasedev, hwpt->hwpt_id, errp); >> @@ -267,6 +284,8 @@ static bool >> 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 |= >> + iommufd_hwpt_dirty_tracking(hwpt); > > If there is at least one hwpt without dirty tracking, shouldn't we make bcontainer.dirty_pages_supported false? > > Thanks > Zhenzhong > >> return true; >> } >> >> -- >> 2.17.2 >
On 22/07/2024 09:58, Joao Martins wrote: > On 22/07/2024 07:05, Duan, Zhenzhong wrote: >> >> >>> -----Original Message----- >>> From: Joao Martins <joao.m.martins@oracle.com> >>> Subject: [PATCH v5 09/13] vfio/iommufd: Probe and request hwpt dirty >>> tracking capability >>> >>> In preparation to using the dirty tracking UAPI, probe whether the IOMMU >>> supports dirty tracking. This is done via the data stored in >>> hiod::caps::hw_caps initialized from GET_HW_INFO. >>> >>> Qemu doesn't know if VF dirty tracking is supported when allocating >>> hardware pagetable in iommufd_cdev_autodomains_get(). This is because >>> VFIODevice migration state hasn't been initialized *yet* hence it can't pick >>> between VF dirty tracking vs IOMMU dirty tracking. So, if IOMMU supports >>> dirty tracking it always creates HWPTs with >>> IOMMU_HWPT_ALLOC_DIRTY_TRACKING >>> even if later on VFIOMigration decides to use VF dirty tracking instead. >> >> I thought there is no overhead for HWPT with IOMMU_HWPT_ALLOC_DIRTY_TRACKING vs. HWPT without IOMMU_HWPT_ALLOC_DIRTY_TRACKING if we don't enable dirty tracking. Right? >> > > Correct. > >>> >>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >>> --- >>> include/hw/vfio/vfio-common.h | 1 + >>> hw/vfio/iommufd.c | 19 +++++++++++++++++++ >>> 2 files changed, 20 insertions(+) >>> >>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio- >>> common.h >>> index 4e44b26d3c45..7e530c7869dc 100644 >>> --- a/include/hw/vfio/vfio-common.h >>> +++ b/include/hw/vfio/vfio-common.h >>> @@ -97,6 +97,7 @@ typedef struct IOMMUFDBackend IOMMUFDBackend; >>> >>> typedef struct VFIOIOASHwpt { >>> uint32_t hwpt_id; >>> + uint32_t hwpt_flags; >>> QLIST_HEAD(, VFIODevice) device_list; >>> QLIST_ENTRY(VFIOIOASHwpt) next; >>> } VFIOIOASHwpt; >>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c >>> index bb44d948c735..2e5c207bbca0 100644 >>> --- a/hw/vfio/iommufd.c >>> +++ b/hw/vfio/iommufd.c >>> @@ -110,6 +110,11 @@ static void >>> iommufd_cdev_unbind_and_disconnect(VFIODevice *vbasedev) >>> iommufd_backend_disconnect(vbasedev->iommufd); >>> } >>> >>> +static bool iommufd_hwpt_dirty_tracking(VFIOIOASHwpt *hwpt) >>> +{ >>> + return hwpt && hwpt->hwpt_flags & >>> IOMMU_HWPT_ALLOC_DIRTY_TRACKING; >>> +} >>> + >>> static int iommufd_cdev_getfd(const char *sysfs_path, Error **errp) >>> { >>> ERRP_GUARD(); >>> @@ -246,6 +251,17 @@ static bool >>> iommufd_cdev_autodomains_get(VFIODevice *vbasedev, >>> } >>> } >>> >>> + /* >>> + * This is quite early and VFIO Migration state isn't yet fully >>> + * initialized, thus rely only on IOMMU hardware capabilities as to >>> + * whether IOMMU dirty tracking is going to be requested. Later >>> + * vfio_migration_realize() may decide to use VF dirty tracking >>> + * instead. >>> + */ >>> + if (vbasedev->hiod->caps.hw_caps & >>> IOMMU_HW_CAP_DIRTY_TRACKING) { >> >> Looks there is still reference to hw_caps, then would suggest to bring back the NEW CAP. >> > Ah, but below helper is checking for GET_HW_INFO stuff, and not hwpt flags > gioven that we haven't allocated a hwpt yet. > > While I could place this check into a helper it would only have an user. I will > need below helper iommufd_hwpt_dirty_tracking() in another patch, so this is a > bit of a one off check only (unless we want a new helper for cosmetic purposes) > >>> + flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING; >>> + } >>> + >>> if (!iommufd_backend_alloc_hwpt(iommufd, vbasedev->devid, >>> container->ioas_id, flags, >>> IOMMU_HWPT_DATA_NONE, 0, NULL, >>> @@ -255,6 +271,7 @@ static bool >>> iommufd_cdev_autodomains_get(VFIODevice *vbasedev, >>> >>> hwpt = g_malloc0(sizeof(*hwpt)); >>> hwpt->hwpt_id = hwpt_id; >>> + hwpt->hwpt_flags = flags; >>> QLIST_INIT(&hwpt->device_list); >>> >>> ret = iommufd_cdev_attach_ioas_hwpt(vbasedev, hwpt->hwpt_id, errp); >>> @@ -267,6 +284,8 @@ static bool >>> 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 |= >>> + iommufd_hwpt_dirty_tracking(hwpt); >> >> If there is at least one hwpt without dirty tracking, shouldn't we make bcontainer.dirty_pages_supported false? >> Missed this comment. We could set to false but the generic container abstraction is utilizing this to let the ioctls() of the individual backend to go through to the defined callback, and that's why I set to true. At that is really the only effect of this patch. By the time we reach to patch 12 (which is what really enables live migration with IOMMU automatically), the IOMMUFD dirty tracking is only called 1) when not one of the VF doesn't support device dirty tracking [only if you're using IOMMUFD backend], and finally 2) that no VF/mdev has added the migration blocker which essentially looks at the HWPT flags (as opposed to the container attribute). Joao
On 22/07/2024 15:09, Joao Martins wrote: > On 22/07/2024 09:58, Joao Martins wrote: >> On 22/07/2024 07:05, Duan, Zhenzhong wrote: >>> >>> >>>> -----Original Message----- >>>> From: Joao Martins <joao.m.martins@oracle.com> >>>> Subject: [PATCH v5 09/13] vfio/iommufd: Probe and request hwpt dirty >>>> tracking capability >>>> >>>> In preparation to using the dirty tracking UAPI, probe whether the IOMMU >>>> supports dirty tracking. This is done via the data stored in >>>> hiod::caps::hw_caps initialized from GET_HW_INFO. >>>> >>>> Qemu doesn't know if VF dirty tracking is supported when allocating >>>> hardware pagetable in iommufd_cdev_autodomains_get(). This is because >>>> VFIODevice migration state hasn't been initialized *yet* hence it can't pick >>>> between VF dirty tracking vs IOMMU dirty tracking. So, if IOMMU supports >>>> dirty tracking it always creates HWPTs with >>>> IOMMU_HWPT_ALLOC_DIRTY_TRACKING >>>> even if later on VFIOMigration decides to use VF dirty tracking instead. >>> >>> I thought there is no overhead for HWPT with IOMMU_HWPT_ALLOC_DIRTY_TRACKING vs. HWPT without IOMMU_HWPT_ALLOC_DIRTY_TRACKING if we don't enable dirty tracking. Right? >>> >> >> Correct. >> >>>> >>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >>>> --- >>>> include/hw/vfio/vfio-common.h | 1 + >>>> hw/vfio/iommufd.c | 19 +++++++++++++++++++ >>>> 2 files changed, 20 insertions(+) >>>> >>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio- >>>> common.h >>>> index 4e44b26d3c45..7e530c7869dc 100644 >>>> --- a/include/hw/vfio/vfio-common.h >>>> +++ b/include/hw/vfio/vfio-common.h >>>> @@ -97,6 +97,7 @@ typedef struct IOMMUFDBackend IOMMUFDBackend; >>>> >>>> typedef struct VFIOIOASHwpt { >>>> uint32_t hwpt_id; >>>> + uint32_t hwpt_flags; >>>> QLIST_HEAD(, VFIODevice) device_list; >>>> QLIST_ENTRY(VFIOIOASHwpt) next; >>>> } VFIOIOASHwpt; >>>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c >>>> index bb44d948c735..2e5c207bbca0 100644 >>>> --- a/hw/vfio/iommufd.c >>>> +++ b/hw/vfio/iommufd.c >>>> @@ -110,6 +110,11 @@ static void >>>> iommufd_cdev_unbind_and_disconnect(VFIODevice *vbasedev) >>>> iommufd_backend_disconnect(vbasedev->iommufd); >>>> } >>>> >>>> +static bool iommufd_hwpt_dirty_tracking(VFIOIOASHwpt *hwpt) >>>> +{ >>>> + return hwpt && hwpt->hwpt_flags & >>>> IOMMU_HWPT_ALLOC_DIRTY_TRACKING; >>>> +} >>>> + >>>> static int iommufd_cdev_getfd(const char *sysfs_path, Error **errp) >>>> { >>>> ERRP_GUARD(); >>>> @@ -246,6 +251,17 @@ static bool >>>> iommufd_cdev_autodomains_get(VFIODevice *vbasedev, >>>> } >>>> } >>>> >>>> + /* >>>> + * This is quite early and VFIO Migration state isn't yet fully >>>> + * initialized, thus rely only on IOMMU hardware capabilities as to >>>> + * whether IOMMU dirty tracking is going to be requested. Later >>>> + * vfio_migration_realize() may decide to use VF dirty tracking >>>> + * instead. >>>> + */ >>>> + if (vbasedev->hiod->caps.hw_caps & >>>> IOMMU_HW_CAP_DIRTY_TRACKING) { >>> >>> Looks there is still reference to hw_caps, then would suggest to bring back the NEW CAP. >>> >> Ah, but below helper is checking for GET_HW_INFO stuff, and not hwpt flags >> given that we haven't allocated a hwpt yet. >> >> While I could place this check into a helper it would only have an user. I will >> need below helper iommufd_hwpt_dirty_tracking() in another patch, so this is a >> bit of a one off check only (unless we want a new helper for cosmetic purposes) >> >>>> + flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING; >>>> + } >>>> + >>>> if (!iommufd_backend_alloc_hwpt(iommufd, vbasedev->devid, >>>> container->ioas_id, flags, >>>> IOMMU_HWPT_DATA_NONE, 0, NULL, >>>> @@ -255,6 +271,7 @@ static bool >>>> iommufd_cdev_autodomains_get(VFIODevice *vbasedev, >>>> >>>> hwpt = g_malloc0(sizeof(*hwpt)); >>>> hwpt->hwpt_id = hwpt_id; >>>> + hwpt->hwpt_flags = flags; >>>> QLIST_INIT(&hwpt->device_list); >>>> >>>> ret = iommufd_cdev_attach_ioas_hwpt(vbasedev, hwpt->hwpt_id, errp); >>>> @@ -267,6 +284,8 @@ static bool >>>> 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 |= >>>> + iommufd_hwpt_dirty_tracking(hwpt); >>> >>> If there is at least one hwpt without dirty tracking, shouldn't we make bcontainer.dirty_pages_supported false? >>> > > Missed this comment. We could set to false but the generic container abstraction > is utilizing this to let the ioctls() of the individual backend to go through to > the defined callback, and that's why I set to true. > Let me rephrase, I meant: "(...) utilizing this to let the individual backend container callbacks of dirty tracking to go through, and that's why I set to true." > And that is really the only effect of this patch. By the time we reach to patch > 12 (which is what really enables live migration with IOMMU automatically), the > IOMMUFD dirty tracking is only called 1) when not one of the VF doesn't support > device dirty tracking [only if you're using IOMMUFD backend], and finally 2) > that no VF/mdev has added the migration blocker which essentially looks at the > HWPT flags (as opposed to the container attribute). > > Joao >
>-----Original Message----- >From: Joao Martins <joao.m.martins@oracle.com> >Subject: Re: [PATCH v5 09/13] vfio/iommufd: Probe and request hwpt dirty >tracking capability > >On 22/07/2024 15:09, Joao Martins wrote: >> On 22/07/2024 09:58, Joao Martins wrote: >>> On 22/07/2024 07:05, Duan, Zhenzhong wrote: >>>> >>>> >>>>> -----Original Message----- >>>>> From: Joao Martins <joao.m.martins@oracle.com> >>>>> Subject: [PATCH v5 09/13] vfio/iommufd: Probe and request hwpt >dirty >>>>> tracking capability >>>>> >>>>> In preparation to using the dirty tracking UAPI, probe whether the >IOMMU >>>>> supports dirty tracking. This is done via the data stored in >>>>> hiod::caps::hw_caps initialized from GET_HW_INFO. >>>>> >>>>> Qemu doesn't know if VF dirty tracking is supported when allocating >>>>> hardware pagetable in iommufd_cdev_autodomains_get(). This is >because >>>>> VFIODevice migration state hasn't been initialized *yet* hence it can't >pick >>>>> between VF dirty tracking vs IOMMU dirty tracking. So, if IOMMU >supports >>>>> dirty tracking it always creates HWPTs with >>>>> IOMMU_HWPT_ALLOC_DIRTY_TRACKING >>>>> even if later on VFIOMigration decides to use VF dirty tracking instead. >>>> >>>> I thought there is no overhead for HWPT with >IOMMU_HWPT_ALLOC_DIRTY_TRACKING vs. HWPT without >IOMMU_HWPT_ALLOC_DIRTY_TRACKING if we don't enable dirty tracking. >Right? >>>> >>> >>> Correct. >>> >>>>> >>>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >>>>> --- >>>>> include/hw/vfio/vfio-common.h | 1 + >>>>> hw/vfio/iommufd.c | 19 +++++++++++++++++++ >>>>> 2 files changed, 20 insertions(+) >>>>> >>>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio- >>>>> common.h >>>>> index 4e44b26d3c45..7e530c7869dc 100644 >>>>> --- a/include/hw/vfio/vfio-common.h >>>>> +++ b/include/hw/vfio/vfio-common.h >>>>> @@ -97,6 +97,7 @@ typedef struct IOMMUFDBackend >IOMMUFDBackend; >>>>> >>>>> typedef struct VFIOIOASHwpt { >>>>> uint32_t hwpt_id; >>>>> + uint32_t hwpt_flags; >>>>> QLIST_HEAD(, VFIODevice) device_list; >>>>> QLIST_ENTRY(VFIOIOASHwpt) next; >>>>> } VFIOIOASHwpt; >>>>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c >>>>> index bb44d948c735..2e5c207bbca0 100644 >>>>> --- a/hw/vfio/iommufd.c >>>>> +++ b/hw/vfio/iommufd.c >>>>> @@ -110,6 +110,11 @@ static void >>>>> iommufd_cdev_unbind_and_disconnect(VFIODevice *vbasedev) >>>>> iommufd_backend_disconnect(vbasedev->iommufd); >>>>> } >>>>> >>>>> +static bool iommufd_hwpt_dirty_tracking(VFIOIOASHwpt *hwpt) >>>>> +{ >>>>> + return hwpt && hwpt->hwpt_flags & >>>>> IOMMU_HWPT_ALLOC_DIRTY_TRACKING; >>>>> +} >>>>> + >>>>> static int iommufd_cdev_getfd(const char *sysfs_path, Error **errp) >>>>> { >>>>> ERRP_GUARD(); >>>>> @@ -246,6 +251,17 @@ static bool >>>>> iommufd_cdev_autodomains_get(VFIODevice *vbasedev, >>>>> } >>>>> } >>>>> >>>>> + /* >>>>> + * This is quite early and VFIO Migration state isn't yet fully >>>>> + * initialized, thus rely only on IOMMU hardware capabilities as to >>>>> + * whether IOMMU dirty tracking is going to be requested. Later >>>>> + * vfio_migration_realize() may decide to use VF dirty tracking >>>>> + * instead. >>>>> + */ >>>>> + if (vbasedev->hiod->caps.hw_caps & >>>>> IOMMU_HW_CAP_DIRTY_TRACKING) { >>>> >>>> Looks there is still reference to hw_caps, then would suggest to bring >back the NEW CAP. >>>> >>> Ah, but below helper is checking for GET_HW_INFO stuff, and not hwpt >flags >>> given that we haven't allocated a hwpt yet. >>> >>> While I could place this check into a helper it would only have an user. I >will >>> need below helper iommufd_hwpt_dirty_tracking() in another patch, so >this is a >>> bit of a one off check only (unless we want a new helper for cosmetic >purposes) >>> >>>>> + flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING; >>>>> + } >>>>> + >>>>> if (!iommufd_backend_alloc_hwpt(iommufd, vbasedev->devid, >>>>> container->ioas_id, flags, >>>>> IOMMU_HWPT_DATA_NONE, 0, NULL, >>>>> @@ -255,6 +271,7 @@ static bool >>>>> iommufd_cdev_autodomains_get(VFIODevice *vbasedev, >>>>> >>>>> hwpt = g_malloc0(sizeof(*hwpt)); >>>>> hwpt->hwpt_id = hwpt_id; >>>>> + hwpt->hwpt_flags = flags; >>>>> QLIST_INIT(&hwpt->device_list); >>>>> >>>>> ret = iommufd_cdev_attach_ioas_hwpt(vbasedev, hwpt->hwpt_id, >errp); >>>>> @@ -267,6 +284,8 @@ static bool >>>>> 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 |= >>>>> + iommufd_hwpt_dirty_tracking(hwpt); >>>> >>>> If there is at least one hwpt without dirty tracking, shouldn't we make >bcontainer.dirty_pages_supported false? >>>> >> >> Missed this comment. We could set to false but the generic container >abstraction >> is utilizing this to let the ioctls() of the individual backend to go through to >> the defined callback, and that's why I set to true. >> >Let me rephrase, I meant: "(...) utilizing this to let the individual backend >container callbacks of dirty tracking to go through, and that's why I set to >true." Not quite get. If there is at least one hwpt not supporting dirty tracking, we can presume all dirty, no need to go through individual backend callbacks? > >> And that is really the only effect of this patch. By the time we reach to >patch >> 12 (which is what really enables live migration with IOMMU automatically), >the >> IOMMUFD dirty tracking is only called 1) when not one of the VF doesn't >support >> device dirty tracking [only if you're using IOMMUFD backend], and finally 2) >> that no VF/mdev has added the migration blocker which essentially looks >at the >> HWPT flags (as opposed to the container attribute). >> >> Joao >>
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h index 4e44b26d3c45..7e530c7869dc 100644 --- a/include/hw/vfio/vfio-common.h +++ b/include/hw/vfio/vfio-common.h @@ -97,6 +97,7 @@ typedef struct IOMMUFDBackend IOMMUFDBackend; typedef struct VFIOIOASHwpt { uint32_t hwpt_id; + uint32_t hwpt_flags; QLIST_HEAD(, VFIODevice) device_list; QLIST_ENTRY(VFIOIOASHwpt) next; } VFIOIOASHwpt; diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c index bb44d948c735..2e5c207bbca0 100644 --- a/hw/vfio/iommufd.c +++ b/hw/vfio/iommufd.c @@ -110,6 +110,11 @@ static void iommufd_cdev_unbind_and_disconnect(VFIODevice *vbasedev) iommufd_backend_disconnect(vbasedev->iommufd); } +static bool iommufd_hwpt_dirty_tracking(VFIOIOASHwpt *hwpt) +{ + return hwpt && hwpt->hwpt_flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING; +} + static int iommufd_cdev_getfd(const char *sysfs_path, Error **errp) { ERRP_GUARD(); @@ -246,6 +251,17 @@ static bool iommufd_cdev_autodomains_get(VFIODevice *vbasedev, } } + /* + * This is quite early and VFIO Migration state isn't yet fully + * initialized, thus rely only on IOMMU hardware capabilities as to + * whether IOMMU dirty tracking is going to be requested. Later + * vfio_migration_realize() may decide to use VF dirty tracking + * instead. + */ + if (vbasedev->hiod->caps.hw_caps & IOMMU_HW_CAP_DIRTY_TRACKING) { + flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING; + } + if (!iommufd_backend_alloc_hwpt(iommufd, vbasedev->devid, container->ioas_id, flags, IOMMU_HWPT_DATA_NONE, 0, NULL, @@ -255,6 +271,7 @@ static bool iommufd_cdev_autodomains_get(VFIODevice *vbasedev, hwpt = g_malloc0(sizeof(*hwpt)); hwpt->hwpt_id = hwpt_id; + hwpt->hwpt_flags = flags; QLIST_INIT(&hwpt->device_list); ret = iommufd_cdev_attach_ioas_hwpt(vbasedev, hwpt->hwpt_id, errp); @@ -267,6 +284,8 @@ static bool 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 |= + iommufd_hwpt_dirty_tracking(hwpt); return true; }
In preparation to using the dirty tracking UAPI, probe whether the IOMMU supports dirty tracking. This is done via the data stored in hiod::caps::hw_caps initialized from GET_HW_INFO. Qemu doesn't know if VF dirty tracking is supported when allocating hardware pagetable in iommufd_cdev_autodomains_get(). This is because VFIODevice migration state hasn't been initialized *yet* hence it can't pick between VF dirty tracking vs IOMMU dirty tracking. So, if IOMMU supports dirty tracking it always creates HWPTs with IOMMU_HWPT_ALLOC_DIRTY_TRACKING even if later on VFIOMigration decides to use VF dirty tracking instead. Signed-off-by: Joao Martins <joao.m.martins@oracle.com> --- include/hw/vfio/vfio-common.h | 1 + hw/vfio/iommufd.c | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+)