| Message ID | 20240712114704.8708-12-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 v4 11/12] vfio/migration: Don't block migration device dirty >tracking is unsupported > >By default VFIO migration is set to auto, which will support live >migration if the migration capability is set *and* also dirty page >tracking is supported. > >For testing purposes one can force enable without dirty page tracking >via enable-migration=on, but that option is generally left for testing >purposes. > >So starting with IOMMU dirty tracking it can use to acomodate the lack of >VF dirty page tracking allowing us to minimize the VF requirements for >migration and thus enabling migration by default for those too. > >Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >--- > hw/vfio/migration.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > >diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c >index 34d4be2ce1b1..ce3d1b6e9a25 100644 >--- a/hw/vfio/migration.c >+++ b/hw/vfio/migration.c >@@ -1036,7 +1036,8 @@ bool vfio_migration_realize(VFIODevice >*vbasedev, Error **errp) > return !vfio_block_migration(vbasedev, err, errp); > } > >- if (!vbasedev->dirty_pages_supported) { >+ if (!vbasedev->dirty_pages_supported && >+ !vbasedev->bcontainer->dirty_pages_supported) { > if (vbasedev->enable_migration == ON_OFF_AUTO_AUTO) { > error_setg(&err, > "%s: VFIO device doesn't support device dirty tracking", I'm not sure if this message needs to be updated, " VFIO device doesn't support device and IOMMU dirty tracking" Same for the below: warn_report("%s: VFIO device doesn't support device dirty tracking"
On 17/07/2024 03:38, Duan, Zhenzhong wrote: > > >> -----Original Message----- >> From: Joao Martins <joao.m.martins@oracle.com> >> Subject: [PATCH v4 11/12] vfio/migration: Don't block migration device dirty >> tracking is unsupported >> >> By default VFIO migration is set to auto, which will support live >> migration if the migration capability is set *and* also dirty page >> tracking is supported. >> >> For testing purposes one can force enable without dirty page tracking >> via enable-migration=on, but that option is generally left for testing >> purposes. >> >> So starting with IOMMU dirty tracking it can use to acomodate the lack of >> VF dirty page tracking allowing us to minimize the VF requirements for >> migration and thus enabling migration by default for those too. >> >> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >> --- >> hw/vfio/migration.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c >> index 34d4be2ce1b1..ce3d1b6e9a25 100644 >> --- a/hw/vfio/migration.c >> +++ b/hw/vfio/migration.c >> @@ -1036,7 +1036,8 @@ bool vfio_migration_realize(VFIODevice >> *vbasedev, Error **errp) >> return !vfio_block_migration(vbasedev, err, errp); >> } >> >> - if (!vbasedev->dirty_pages_supported) { >> + if (!vbasedev->dirty_pages_supported && >> + !vbasedev->bcontainer->dirty_pages_supported) { >> if (vbasedev->enable_migration == ON_OFF_AUTO_AUTO) { >> error_setg(&err, >> "%s: VFIO device doesn't support device dirty tracking", > > I'm not sure if this message needs to be updated, " VFIO device doesn't support device and IOMMU dirty tracking" > > Same for the below: > > warn_report("%s: VFIO device doesn't support device dirty tracking" Ah yes, good catch. Additionally I think I should check device hwpt rather than container::dirty_pages_supported i.e. if (!vbasedev->dirty_pages_supported && (vbasedev->hwpt && !iommufd_hwpt_dirty_tracking(vbasedev->hwpt))) This makes sure that migration is blocked with more accuracy
On 7/12/24 13:47, Joao Martins wrote: > By default VFIO migration is set to auto, which will support live > migration if the migration capability is set *and* also dirty page > tracking is supported. > > For testing purposes one can force enable without dirty page tracking > via enable-migration=on, but that option is generally left for testing > purposes. > > So starting with IOMMU dirty tracking it can use to acomodate the lack of accomodate Eric > VF dirty page tracking allowing us to minimize the VF requirements for > migration and thus enabling migration by default for those too. > > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > --- > hw/vfio/migration.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c > index 34d4be2ce1b1..ce3d1b6e9a25 100644 > --- a/hw/vfio/migration.c > +++ b/hw/vfio/migration.c > @@ -1036,7 +1036,8 @@ bool vfio_migration_realize(VFIODevice *vbasedev, Error **errp) > return !vfio_block_migration(vbasedev, err, errp); > } > > - if (!vbasedev->dirty_pages_supported) { > + if (!vbasedev->dirty_pages_supported && > + !vbasedev->bcontainer->dirty_pages_supported) { > if (vbasedev->enable_migration == ON_OFF_AUTO_AUTO) { > error_setg(&err, > "%s: VFIO device doesn't support device dirty tracking",
On 17/07/2024 10:20, Joao Martins wrote: > On 17/07/2024 03:38, Duan, Zhenzhong wrote: >> >> >>> -----Original Message----- >>> From: Joao Martins <joao.m.martins@oracle.com> >>> Subject: [PATCH v4 11/12] vfio/migration: Don't block migration device dirty >>> tracking is unsupported >>> >>> By default VFIO migration is set to auto, which will support live >>> migration if the migration capability is set *and* also dirty page >>> tracking is supported. >>> >>> For testing purposes one can force enable without dirty page tracking >>> via enable-migration=on, but that option is generally left for testing >>> purposes. >>> >>> So starting with IOMMU dirty tracking it can use to acomodate the lack of >>> VF dirty page tracking allowing us to minimize the VF requirements for >>> migration and thus enabling migration by default for those too. >>> >>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >>> --- >>> hw/vfio/migration.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c >>> index 34d4be2ce1b1..ce3d1b6e9a25 100644 >>> --- a/hw/vfio/migration.c >>> +++ b/hw/vfio/migration.c >>> @@ -1036,7 +1036,8 @@ bool vfio_migration_realize(VFIODevice >>> *vbasedev, Error **errp) >>> return !vfio_block_migration(vbasedev, err, errp); >>> } >>> >>> - if (!vbasedev->dirty_pages_supported) { >>> + if (!vbasedev->dirty_pages_supported && >>> + !vbasedev->bcontainer->dirty_pages_supported) { >>> if (vbasedev->enable_migration == ON_OFF_AUTO_AUTO) { >>> error_setg(&err, >>> "%s: VFIO device doesn't support device dirty tracking", >> >> I'm not sure if this message needs to be updated, " VFIO device doesn't support device and IOMMU dirty tracking" >> >> Same for the below: >> >> warn_report("%s: VFIO device doesn't support device dirty tracking" > > > Ah yes, good catch. Additionally I think I should check device hwpt rather than > container::dirty_pages_supported i.e. > > if (!vbasedev->dirty_pages_supported && > (vbasedev->hwpt && !iommufd_hwpt_dirty_tracking(vbasedev->hwpt))) > > This makes sure that migration is blocked with more accuracy I retract this comment as I think it can all be easily detected by not OR-ing the setting of vbasedev->bcontainer->dirty_pages_supported. I should put a warn_report_once() there. Joao
On 17/07/2024 16:35, Joao Martins wrote: > On 17/07/2024 10:20, Joao Martins wrote: >> On 17/07/2024 03:38, Duan, Zhenzhong wrote: >>>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c >>>> index 34d4be2ce1b1..ce3d1b6e9a25 100644 >>>> --- a/hw/vfio/migration.c >>>> +++ b/hw/vfio/migration.c >>>> @@ -1036,7 +1036,8 @@ bool vfio_migration_realize(VFIODevice >>>> *vbasedev, Error **errp) >>>> return !vfio_block_migration(vbasedev, err, errp); >>>> } >>>> >>>> - if (!vbasedev->dirty_pages_supported) { >>>> + if (!vbasedev->dirty_pages_supported && >>>> + !vbasedev->bcontainer->dirty_pages_supported) { >>>> if (vbasedev->enable_migration == ON_OFF_AUTO_AUTO) { >>>> error_setg(&err, >>>> "%s: VFIO device doesn't support device dirty tracking", >>> >>> I'm not sure if this message needs to be updated, " VFIO device doesn't support device and IOMMU dirty tracking" >>> >>> Same for the below: >>> >>> warn_report("%s: VFIO device doesn't support device dirty tracking" >> >> >> Ah yes, good catch. Additionally I think I should check device hwpt rather than >> container::dirty_pages_supported i.e. >> >> if (!vbasedev->dirty_pages_supported && >> (vbasedev->hwpt && !iommufd_hwpt_dirty_tracking(vbasedev->hwpt))) >> >> This makes sure that migration is blocked with more accuracy > > I retract this comment as I think it can all be easily detected by not OR-ing > the setting of vbasedev->bcontainer->dirty_pages_supported. I should put a > warn_report_once() there. Something like this below. To be clear: this is mostly a safe guard against a theoretic case that we don't know it exists. For example on x86, this is homogeneous and I suspect server ARM to be the case too. embedded ARM might be different as there's so many incantations of it. @@ -267,6 +282,13 @@ 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); + + if (container->bcontainer.dirty_pages_supported && + !iommufd_hwpt_dirty_tracking(hwpt)) { + warn_report("%s: IOMMU dirty tracking not supported\n", vbasedev->name); + } + container->bcontainer.dirty_pages_supported = + iommufd_hwpt_dirty_tracking(hwpt); return true; }
On 17/07/2024 17:02, Joao Martins wrote: > On 17/07/2024 16:35, Joao Martins wrote: >> On 17/07/2024 10:20, Joao Martins wrote: >>> On 17/07/2024 03:38, Duan, Zhenzhong wrote: >>>>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c >>>>> index 34d4be2ce1b1..ce3d1b6e9a25 100644 >>>>> --- a/hw/vfio/migration.c >>>>> +++ b/hw/vfio/migration.c >>>>> @@ -1036,7 +1036,8 @@ bool vfio_migration_realize(VFIODevice >>>>> *vbasedev, Error **errp) >>>>> return !vfio_block_migration(vbasedev, err, errp); >>>>> } >>>>> >>>>> - if (!vbasedev->dirty_pages_supported) { >>>>> + if (!vbasedev->dirty_pages_supported && >>>>> + !vbasedev->bcontainer->dirty_pages_supported) { >>>>> if (vbasedev->enable_migration == ON_OFF_AUTO_AUTO) { >>>>> error_setg(&err, >>>>> "%s: VFIO device doesn't support device dirty tracking", >>>> >>>> I'm not sure if this message needs to be updated, " VFIO device doesn't support device and IOMMU dirty tracking" >>>> >>>> Same for the below: >>>> >>>> warn_report("%s: VFIO device doesn't support device dirty tracking" >>> >>> >>> Ah yes, good catch. Additionally I think I should check device hwpt rather than >>> container::dirty_pages_supported i.e. >>> >>> if (!vbasedev->dirty_pages_supported && >>> (vbasedev->hwpt && !iommufd_hwpt_dirty_tracking(vbasedev->hwpt))) >>> >>> This makes sure that migration is blocked with more accuracy >> >> I retract this comment as I think it can all be easily detected by not OR-ing >> the setting of vbasedev->bcontainer->dirty_pages_supported. I should put a >> warn_report_once() there. > > Something like this below. > > To be clear: this is mostly a safe guard against a theoretic case that we don't > know it exists. For example on x86, this is homogeneous and I suspect server ARM > to be the case too. embedded ARM might be different as there's so many > incantations of it. > Except that it won't work with hotplug :( so the previous snip was actually a bit better. > @@ -267,6 +282,13 @@ 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); > + > + if (container->bcontainer.dirty_pages_supported && > + !iommufd_hwpt_dirty_tracking(hwpt)) { > + warn_report("%s: IOMMU dirty tracking not supported\n", vbasedev->name); > + } > + container->bcontainer.dirty_pages_supported = > + iommufd_hwpt_dirty_tracking(hwpt); > return true; > } > >
>-----Original Message----- >From: Joao Martins <joao.m.martins@oracle.com> >Subject: Re: [PATCH v4 11/12] vfio/migration: Don't block migration device >dirty tracking is unsupported > >On 17/07/2024 03:38, Duan, Zhenzhong wrote: >> >> >>> -----Original Message----- >>> From: Joao Martins <joao.m.martins@oracle.com> >>> Subject: [PATCH v4 11/12] vfio/migration: Don't block migration device >dirty >>> tracking is unsupported >>> >>> By default VFIO migration is set to auto, which will support live >>> migration if the migration capability is set *and* also dirty page >>> tracking is supported. >>> >>> For testing purposes one can force enable without dirty page tracking >>> via enable-migration=on, but that option is generally left for testing >>> purposes. >>> >>> So starting with IOMMU dirty tracking it can use to acomodate the lack of >>> VF dirty page tracking allowing us to minimize the VF requirements for >>> migration and thus enabling migration by default for those too. >>> >>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >>> --- >>> hw/vfio/migration.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c >>> index 34d4be2ce1b1..ce3d1b6e9a25 100644 >>> --- a/hw/vfio/migration.c >>> +++ b/hw/vfio/migration.c >>> @@ -1036,7 +1036,8 @@ bool vfio_migration_realize(VFIODevice >>> *vbasedev, Error **errp) >>> return !vfio_block_migration(vbasedev, err, errp); >>> } >>> >>> - if (!vbasedev->dirty_pages_supported) { >>> + if (!vbasedev->dirty_pages_supported && >>> + !vbasedev->bcontainer->dirty_pages_supported) { >>> if (vbasedev->enable_migration == ON_OFF_AUTO_AUTO) { >>> error_setg(&err, >>> "%s: VFIO device doesn't support device dirty tracking", >> >> I'm not sure if this message needs to be updated, " VFIO device doesn't >support device and IOMMU dirty tracking" >> >> Same for the below: >> >> warn_report("%s: VFIO device doesn't support device dirty tracking" > > >Ah yes, good catch. Additionally I think I should check device hwpt rather >than >container::dirty_pages_supported i.e. > >if (!vbasedev->dirty_pages_supported && > (vbasedev->hwpt && !iommufd_hwpt_dirty_tracking(vbasedev->hwpt))) > >This makes sure that migration is blocked with more accuracy Yes, this is better. Looks bcontainer->dirty_pages_supported is not as accurate as in legacy VFIO days. Thanks Zhenzhong
On 18/07/2024 08:20, Duan, Zhenzhong wrote: > > >> -----Original Message----- >> From: Joao Martins <joao.m.martins@oracle.com> >> Subject: Re: [PATCH v4 11/12] vfio/migration: Don't block migration device >> dirty tracking is unsupported >> >> On 17/07/2024 03:38, Duan, Zhenzhong wrote: >>> >>> >>>> -----Original Message----- >>>> From: Joao Martins <joao.m.martins@oracle.com> >>>> Subject: [PATCH v4 11/12] vfio/migration: Don't block migration device >> dirty >>>> tracking is unsupported >>>> >>>> By default VFIO migration is set to auto, which will support live >>>> migration if the migration capability is set *and* also dirty page >>>> tracking is supported. >>>> >>>> For testing purposes one can force enable without dirty page tracking >>>> via enable-migration=on, but that option is generally left for testing >>>> purposes. >>>> >>>> So starting with IOMMU dirty tracking it can use to acomodate the lack of >>>> VF dirty page tracking allowing us to minimize the VF requirements for >>>> migration and thus enabling migration by default for those too. >>>> >>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >>>> --- >>>> hw/vfio/migration.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c >>>> index 34d4be2ce1b1..ce3d1b6e9a25 100644 >>>> --- a/hw/vfio/migration.c >>>> +++ b/hw/vfio/migration.c >>>> @@ -1036,7 +1036,8 @@ bool vfio_migration_realize(VFIODevice >>>> *vbasedev, Error **errp) >>>> return !vfio_block_migration(vbasedev, err, errp); >>>> } >>>> >>>> - if (!vbasedev->dirty_pages_supported) { >>>> + if (!vbasedev->dirty_pages_supported && >>>> + !vbasedev->bcontainer->dirty_pages_supported) { >>>> if (vbasedev->enable_migration == ON_OFF_AUTO_AUTO) { >>>> error_setg(&err, >>>> "%s: VFIO device doesn't support device dirty tracking", >>> >>> I'm not sure if this message needs to be updated, " VFIO device doesn't >> support device and IOMMU dirty tracking" >>> >>> Same for the below: >>> >>> warn_report("%s: VFIO device doesn't support device dirty tracking" >> >> >> Ah yes, good catch. Additionally I think I should check device hwpt rather >> than >> container::dirty_pages_supported i.e. >> >> if (!vbasedev->dirty_pages_supported && >> (vbasedev->hwpt && !iommufd_hwpt_dirty_tracking(vbasedev->hwpt))) >> >> This makes sure that migration is blocked with more accuracy > > Yes, this is better. Looks bcontainer->dirty_pages_supported is not as accurate as in legacy VFIO days. > Heh, That's just because legacy is always marking true (and marking anything as dirty) regardless of what the hardware does :)
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c index 34d4be2ce1b1..ce3d1b6e9a25 100644 --- a/hw/vfio/migration.c +++ b/hw/vfio/migration.c @@ -1036,7 +1036,8 @@ bool vfio_migration_realize(VFIODevice *vbasedev, Error **errp) return !vfio_block_migration(vbasedev, err, errp); } - if (!vbasedev->dirty_pages_supported) { + if (!vbasedev->dirty_pages_supported && + !vbasedev->bcontainer->dirty_pages_supported) { if (vbasedev->enable_migration == ON_OFF_AUTO_AUTO) { error_setg(&err, "%s: VFIO device doesn't support device dirty tracking",
By default VFIO migration is set to auto, which will support live migration if the migration capability is set *and* also dirty page tracking is supported. For testing purposes one can force enable without dirty page tracking via enable-migration=on, but that option is generally left for testing purposes. So starting with IOMMU dirty tracking it can use to acomodate the lack of VF dirty page tracking allowing us to minimize the VF requirements for migration and thus enabling migration by default for those too. Signed-off-by: Joao Martins <joao.m.martins@oracle.com> --- hw/vfio/migration.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)