| Message ID | 20230206123137.31149-8-avihaih@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio/migration: Implement VFIO migration protocol v2 | expand |
On Mon, 6 Feb 2023 14:31:30 +0200 Avihai Horon <avihaih@nvidia.com> wrote: > Currently VFIO migration doesn't implement some kind of intermediate > quiescent state in which P2P DMAs are quiesced before stopping or > running the device. This can cause problems in multi-device migration > where the devices are doing P2P DMAs, since the devices are not stopped > together at the same time. > > Until such support is added, block migration of multiple devices. > > Signed-off-by: Avihai Horon <avihaih@nvidia.com> > --- > include/hw/vfio/vfio-common.h | 2 ++ > hw/vfio/common.c | 51 +++++++++++++++++++++++++++++++++++ > hw/vfio/migration.c | 6 +++++ > 3 files changed, 59 insertions(+) > > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h > index e573f5a9f1..56b1683824 100644 > --- a/include/hw/vfio/vfio-common.h > +++ b/include/hw/vfio/vfio-common.h > @@ -218,6 +218,8 @@ typedef QLIST_HEAD(VFIOGroupList, VFIOGroup) VFIOGroupList; > extern VFIOGroupList vfio_group_list; > > bool vfio_mig_active(void); > +int vfio_block_multiple_devices_migration(Error **errp); > +void vfio_unblock_multiple_devices_migration(void); > int64_t vfio_mig_bytes_transferred(void); > > #ifdef CONFIG_LINUX > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index 3a35f4afad..01db41b735 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -41,6 +41,7 @@ > #include "qapi/error.h" > #include "migration/migration.h" > #include "migration/misc.h" > +#include "migration/blocker.h" > #include "sysemu/tpm.h" > > VFIOGroupList vfio_group_list = > @@ -337,6 +338,56 @@ bool vfio_mig_active(void) > return true; > } > > +Error *multiple_devices_migration_blocker; > + > +static unsigned int vfio_migratable_device_num(void) > +{ > + VFIOGroup *group; > + VFIODevice *vbasedev; > + unsigned int device_num = 0; > + > + QLIST_FOREACH(group, &vfio_group_list, next) { > + QLIST_FOREACH(vbasedev, &group->device_list, next) { > + if (vbasedev->migration) { > + device_num++; > + } > + } > + } > + > + return device_num; > +} > + > +int vfio_block_multiple_devices_migration(Error **errp) > +{ > + int ret; > + > + if (vfio_migratable_device_num() != 2) { > + return 0; > + } > + > + error_setg(&multiple_devices_migration_blocker, > + "Migration is currently not supported with multiple " > + "VFIO devices"); > + ret = migrate_add_blocker(multiple_devices_migration_blocker, errp); > + if (ret < 0) { > + error_free(multiple_devices_migration_blocker); > + multiple_devices_migration_blocker = NULL; > + } > + > + return ret; > +} > + > +void vfio_unblock_multiple_devices_migration(void) > +{ > + if (vfio_migratable_device_num() != 2) { > + return; > + } > + > + migrate_del_blocker(multiple_devices_migration_blocker); > + error_free(multiple_devices_migration_blocker); > + multiple_devices_migration_blocker = NULL; > +} A couple awkward things here. First I wish we could do something cleaner or more intuitive than the != 2 test. I get that we're trying to do this on the addition of the 2nd device supporting migration, or the removal of the next to last device independent of all other devices, but I wonder if it wouldn't be better to remove the multiple-device blocker after migration is torn down for the device so we can test device >1 or ==1 in combination with whether multiple_devices_migration_blocker is NULL. Which comes to the second awkwardness, if we fail to add the blocker we free and clear the blocker, but when we tear down the device due to that failure we'll remove the blocker that doesn't exist, free NULL, and clear it again. Thanks to the glib slist the migration blocker is using, I think that all works, but I'd rather not be dependent on that implementation to avoid a segfault here. Incorporating a test of multiple_devices_migration_blocker as above would avoid this too. Thanks, Alex > + > static bool vfio_devices_all_dirty_tracking(VFIOContainer *container) > { > VFIOGroup *group; > diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c > index 552c2313b2..8085a4ff44 100644 > --- a/hw/vfio/migration.c > +++ b/hw/vfio/migration.c > @@ -880,6 +880,11 @@ int vfio_migration_probe(VFIODevice *vbasedev, Error **errp) > goto add_blocker; > } > > + ret = vfio_block_multiple_devices_migration(errp); > + if (ret) { > + return ret; > + } > + > trace_vfio_migration_probe(vbasedev->name, info->index); > g_free(info); > return 0; > @@ -902,6 +907,7 @@ void vfio_migration_finalize(VFIODevice *vbasedev) > if (vbasedev->migration) { > VFIOMigration *migration = vbasedev->migration; > > + vfio_unblock_multiple_devices_migration(); > remove_migration_state_change_notifier(&migration->migration_state); > qemu_del_vm_change_state_handler(migration->vm_state); > unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio", vbasedev);
On 08/02/2023 0:34, Alex Williamson wrote: > External email: Use caution opening links or attachments > > > On Mon, 6 Feb 2023 14:31:30 +0200 > Avihai Horon <avihaih@nvidia.com> wrote: > >> Currently VFIO migration doesn't implement some kind of intermediate >> quiescent state in which P2P DMAs are quiesced before stopping or >> running the device. This can cause problems in multi-device migration >> where the devices are doing P2P DMAs, since the devices are not stopped >> together at the same time. >> >> Until such support is added, block migration of multiple devices. >> >> Signed-off-by: Avihai Horon <avihaih@nvidia.com> >> --- >> include/hw/vfio/vfio-common.h | 2 ++ >> hw/vfio/common.c | 51 +++++++++++++++++++++++++++++++++++ >> hw/vfio/migration.c | 6 +++++ >> 3 files changed, 59 insertions(+) >> >> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h >> index e573f5a9f1..56b1683824 100644 >> --- a/include/hw/vfio/vfio-common.h >> +++ b/include/hw/vfio/vfio-common.h >> @@ -218,6 +218,8 @@ typedef QLIST_HEAD(VFIOGroupList, VFIOGroup) VFIOGroupList; >> extern VFIOGroupList vfio_group_list; >> >> bool vfio_mig_active(void); >> +int vfio_block_multiple_devices_migration(Error **errp); >> +void vfio_unblock_multiple_devices_migration(void); >> int64_t vfio_mig_bytes_transferred(void); >> >> #ifdef CONFIG_LINUX >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index 3a35f4afad..01db41b735 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -41,6 +41,7 @@ >> #include "qapi/error.h" >> #include "migration/migration.h" >> #include "migration/misc.h" >> +#include "migration/blocker.h" >> #include "sysemu/tpm.h" >> >> VFIOGroupList vfio_group_list = >> @@ -337,6 +338,56 @@ bool vfio_mig_active(void) >> return true; >> } >> >> +Error *multiple_devices_migration_blocker; >> + >> +static unsigned int vfio_migratable_device_num(void) >> +{ >> + VFIOGroup *group; >> + VFIODevice *vbasedev; >> + unsigned int device_num = 0; >> + >> + QLIST_FOREACH(group, &vfio_group_list, next) { >> + QLIST_FOREACH(vbasedev, &group->device_list, next) { >> + if (vbasedev->migration) { >> + device_num++; >> + } >> + } >> + } >> + >> + return device_num; >> +} >> + >> +int vfio_block_multiple_devices_migration(Error **errp) >> +{ >> + int ret; >> + >> + if (vfio_migratable_device_num() != 2) { >> + return 0; >> + } >> + >> + error_setg(&multiple_devices_migration_blocker, >> + "Migration is currently not supported with multiple " >> + "VFIO devices"); >> + ret = migrate_add_blocker(multiple_devices_migration_blocker, errp); >> + if (ret < 0) { >> + error_free(multiple_devices_migration_blocker); >> + multiple_devices_migration_blocker = NULL; >> + } >> + >> + return ret; >> +} >> + >> +void vfio_unblock_multiple_devices_migration(void) >> +{ >> + if (vfio_migratable_device_num() != 2) { >> + return; >> + } >> + >> + migrate_del_blocker(multiple_devices_migration_blocker); >> + error_free(multiple_devices_migration_blocker); >> + multiple_devices_migration_blocker = NULL; >> +} > A couple awkward things here. First I wish we could do something > cleaner or more intuitive than the != 2 test. I get that we're trying > to do this on the addition of the 2nd device supporting migration, or > the removal of the next to last device independent of all other devices, > but I wonder if it wouldn't be better to remove the multiple-device > blocker after migration is torn down for the device so we can test > device >1 or ==1 in combination with whether > multiple_devices_migration_blocker is NULL. > > Which comes to the second awkwardness, if we fail to add the blocker we > free and clear the blocker, but when we tear down the device due to that > failure we'll remove the blocker that doesn't exist, free NULL, and > clear it again. Thanks to the glib slist the migration blocker is > using, I think that all works, but I'd rather not be dependent on that > implementation to avoid a segfault here. Incorporating a test of > multiple_devices_migration_blocker as above would avoid this too. You mean something like this? diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 3a35f4afad..f3e08eff58 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c [...] +int vfio_block_multiple_devices_migration(Error **errp) +{ + int ret; + + if (vfio_migratable_device_num() <= 1 || + multiple_devices_migration_blocker) { + return 0; + } + + error_setg(&multiple_devices_migration_blocker, + "Migration is currently not supported with multiple " + "VFIO devices"); + ret = migrate_add_blocker(multiple_devices_migration_blocker, errp); + if (ret < 0) { + error_free(multiple_devices_migration_blocker); + multiple_devices_migration_blocker = NULL; + } + + return ret; +} + +void vfio_unblock_multiple_devices_migration(void) +{ + if (vfio_migratable_device_num() > 1 || + !multiple_devices_migration_blocker) { + return; + } + + migrate_del_blocker(multiple_devices_migration_blocker); + error_free(multiple_devices_migration_blocker); + multiple_devices_migration_blocker = NULL; +} + static bool vfio_devices_all_dirty_tracking(VFIOContainer *container) { VFIOGroup *group; diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c index 552c2313b2..15b446c0ec 100644 --- a/hw/vfio/migration.c +++ b/hw/vfio/migration.c @@ -880,6 +880,11 @@ int vfio_migration_probe(VFIODevice *vbasedev, Error **errp) goto add_blocker; } + ret = vfio_block_multiple_devices_migration(errp); + if (ret) { + return ret; + } + trace_vfio_migration_probe(vbasedev->name, info->index); g_free(info); return 0; @@ -906,6 +911,7 @@ void vfio_migration_finalize(VFIODevice *vbasedev) qemu_del_vm_change_state_handler(migration->vm_state); unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio", vbasedev); vfio_migration_exit(vbasedev); + vfio_unblock_multiple_devices_migration(); } if (vbasedev->migration_blocker) { Maybe also negate the if conditions and put the add/remove blocker code inside it? Is it more readable this way? E.g.: +int vfio_block_multiple_devices_migration(Error **errp) +{ + int ret = 0; + + if (vfio_migratable_device_num() > 1 && + !multiple_devices_migration_blocker) { + error_setg(&multiple_devices_migration_blocker, + "Migration is currently not supported with multiple " + "VFIO devices"); + ret = migrate_add_blocker(multiple_devices_migration_blocker, errp); + if (ret < 0) { + error_free(multiple_devices_migration_blocker); + multiple_devices_migration_blocker = NULL; + } + } + + return ret; +} + +void vfio_unblock_multiple_devices_migration(void) +{ + if (vfio_migratable_device_num() <= 1 && + multiple_devices_migration_blocker) { + migrate_del_blocker(multiple_devices_migration_blocker); + error_free(multiple_devices_migration_blocker); + multiple_devices_migration_blocker = NULL; + } +} Thanks.
On 2/8/23 14:08, Avihai Horon wrote: > > On 08/02/2023 0:34, Alex Williamson wrote: >> External email: Use caution opening links or attachments >> >> >> On Mon, 6 Feb 2023 14:31:30 +0200 >> Avihai Horon <avihaih@nvidia.com> wrote: >> >>> Currently VFIO migration doesn't implement some kind of intermediate >>> quiescent state in which P2P DMAs are quiesced before stopping or >>> running the device. This can cause problems in multi-device migration >>> where the devices are doing P2P DMAs, since the devices are not stopped >>> together at the same time. >>> >>> Until such support is added, block migration of multiple devices. >>> >>> Signed-off-by: Avihai Horon <avihaih@nvidia.com> >>> --- >>> include/hw/vfio/vfio-common.h | 2 ++ >>> hw/vfio/common.c | 51 +++++++++++++++++++++++++++++++++++ >>> hw/vfio/migration.c | 6 +++++ >>> 3 files changed, 59 insertions(+) >>> >>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h >>> index e573f5a9f1..56b1683824 100644 >>> --- a/include/hw/vfio/vfio-common.h >>> +++ b/include/hw/vfio/vfio-common.h >>> @@ -218,6 +218,8 @@ typedef QLIST_HEAD(VFIOGroupList, VFIOGroup) VFIOGroupList; >>> extern VFIOGroupList vfio_group_list; >>> >>> bool vfio_mig_active(void); >>> +int vfio_block_multiple_devices_migration(Error **errp); >>> +void vfio_unblock_multiple_devices_migration(void); >>> int64_t vfio_mig_bytes_transferred(void); >>> >>> #ifdef CONFIG_LINUX >>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >>> index 3a35f4afad..01db41b735 100644 >>> --- a/hw/vfio/common.c >>> +++ b/hw/vfio/common.c >>> @@ -41,6 +41,7 @@ >>> #include "qapi/error.h" >>> #include "migration/migration.h" >>> #include "migration/misc.h" >>> +#include "migration/blocker.h" >>> #include "sysemu/tpm.h" >>> >>> VFIOGroupList vfio_group_list = >>> @@ -337,6 +338,56 @@ bool vfio_mig_active(void) >>> return true; >>> } >>> >>> +Error *multiple_devices_migration_blocker; static ? So we have two migration blockers, one per device and one global. I guess it is easier that way than trying to update the per device Error*. C. >>> + >>> +static unsigned int vfio_migratable_device_num(void) >>> +{ >>> + VFIOGroup *group; >>> + VFIODevice *vbasedev; >>> + unsigned int device_num = 0; >>> + >>> + QLIST_FOREACH(group, &vfio_group_list, next) { >>> + QLIST_FOREACH(vbasedev, &group->device_list, next) { >>> + if (vbasedev->migration) { >>> + device_num++; >>> + } >>> + } >>> + } >>> + >>> + return device_num; >>> +} >>> + >>> +int vfio_block_multiple_devices_migration(Error **errp) >>> +{ >>> + int ret; >>> + >>> + if (vfio_migratable_device_num() != 2) { >>> + return 0; >>> + } >>> + >>> + error_setg(&multiple_devices_migration_blocker, >>> + "Migration is currently not supported with multiple " >>> + "VFIO devices"); >>> + ret = migrate_add_blocker(multiple_devices_migration_blocker, errp); >>> + if (ret < 0) { >>> + error_free(multiple_devices_migration_blocker); >>> + multiple_devices_migration_blocker = NULL; >>> + } >>> + >>> + return ret; >>> +} >>> + >>> +void vfio_unblock_multiple_devices_migration(void) >>> +{ >>> + if (vfio_migratable_device_num() != 2) { >>> + return; >>> + } >>> + >>> + migrate_del_blocker(multiple_devices_migration_blocker); >>> + error_free(multiple_devices_migration_blocker); >>> + multiple_devices_migration_blocker = NULL; >>> +} >> A couple awkward things here. First I wish we could do something >> cleaner or more intuitive than the != 2 test. I get that we're trying >> to do this on the addition of the 2nd device supporting migration, or >> the removal of the next to last device independent of all other devices, >> but I wonder if it wouldn't be better to remove the multiple-device >> blocker after migration is torn down for the device so we can test >> device >1 or ==1 in combination with whether >> multiple_devices_migration_blocker is NULL. >> >> Which comes to the second awkwardness, if we fail to add the blocker we >> free and clear the blocker, but when we tear down the device due to that >> failure we'll remove the blocker that doesn't exist, free NULL, and >> clear it again. Thanks to the glib slist the migration blocker is >> using, I think that all works, but I'd rather not be dependent on that >> implementation to avoid a segfault here. Incorporating a test of >> multiple_devices_migration_blocker as above would avoid this too. > > You mean something like this? > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index 3a35f4afad..f3e08eff58 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > > [...] > > +int vfio_block_multiple_devices_migration(Error **errp) > +{ > + int ret; > + > + if (vfio_migratable_device_num() <= 1 || > + multiple_devices_migration_blocker) { > + return 0; > + } > + > + error_setg(&multiple_devices_migration_blocker, > + "Migration is currently not supported with multiple " > + "VFIO devices"); > + ret = migrate_add_blocker(multiple_devices_migration_blocker, errp); > + if (ret < 0) { > + error_free(multiple_devices_migration_blocker); > + multiple_devices_migration_blocker = NULL; > + } > + > + return ret; > +} > + > +void vfio_unblock_multiple_devices_migration(void) > +{ > + if (vfio_migratable_device_num() > 1 || > + !multiple_devices_migration_blocker) { > + return; > + } > + > + migrate_del_blocker(multiple_devices_migration_blocker); > + error_free(multiple_devices_migration_blocker); > + multiple_devices_migration_blocker = NULL; > +} > + > static bool vfio_devices_all_dirty_tracking(VFIOContainer *container) > { > VFIOGroup *group; > diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c > index 552c2313b2..15b446c0ec 100644 > --- a/hw/vfio/migration.c > +++ b/hw/vfio/migration.c > @@ -880,6 +880,11 @@ int vfio_migration_probe(VFIODevice *vbasedev, Error **errp) > goto add_blocker; > } > > + ret = vfio_block_multiple_devices_migration(errp); > + if (ret) { > + return ret; > + } > + > trace_vfio_migration_probe(vbasedev->name, info->index); > g_free(info); > return 0; > @@ -906,6 +911,7 @@ void vfio_migration_finalize(VFIODevice *vbasedev) > qemu_del_vm_change_state_handler(migration->vm_state); > unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio", vbasedev); > vfio_migration_exit(vbasedev); > + vfio_unblock_multiple_devices_migration(); > } > > if (vbasedev->migration_blocker) { > > > Maybe also negate the if conditions and put the add/remove blocker code inside it? Is it more readable this way? > E.g.: > > +int vfio_block_multiple_devices_migration(Error **errp) > +{ > + int ret = 0; > + > + if (vfio_migratable_device_num() > 1 && > + !multiple_devices_migration_blocker) { > + error_setg(&multiple_devices_migration_blocker, > + "Migration is currently not supported with multiple " > + "VFIO devices"); > + ret = migrate_add_blocker(multiple_devices_migration_blocker, errp); > + if (ret < 0) { > + error_free(multiple_devices_migration_blocker); > + multiple_devices_migration_blocker = NULL; > + } > + } > + > + return ret; > +} > + > +void vfio_unblock_multiple_devices_migration(void) > +{ > + if (vfio_migratable_device_num() <= 1 && > + multiple_devices_migration_blocker) { > + migrate_del_blocker(multiple_devices_migration_blocker); > + error_free(multiple_devices_migration_blocker); > + multiple_devices_migration_blocker = NULL; > + } > +} > > Thanks. >
On 08/02/2023 18:44, Cédric Le Goater wrote: > External email: Use caution opening links or attachments > > > On 2/8/23 14:08, Avihai Horon wrote: >> >> On 08/02/2023 0:34, Alex Williamson wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> On Mon, 6 Feb 2023 14:31:30 +0200 >>> Avihai Horon <avihaih@nvidia.com> wrote: >>> >>>> Currently VFIO migration doesn't implement some kind of intermediate >>>> quiescent state in which P2P DMAs are quiesced before stopping or >>>> running the device. This can cause problems in multi-device migration >>>> where the devices are doing P2P DMAs, since the devices are not >>>> stopped >>>> together at the same time. >>>> >>>> Until such support is added, block migration of multiple devices. >>>> >>>> Signed-off-by: Avihai Horon <avihaih@nvidia.com> >>>> --- >>>> include/hw/vfio/vfio-common.h | 2 ++ >>>> hw/vfio/common.c | 51 >>>> +++++++++++++++++++++++++++++++++++ >>>> hw/vfio/migration.c | 6 +++++ >>>> 3 files changed, 59 insertions(+) >>>> >>>> diff --git a/include/hw/vfio/vfio-common.h >>>> b/include/hw/vfio/vfio-common.h >>>> index e573f5a9f1..56b1683824 100644 >>>> --- a/include/hw/vfio/vfio-common.h >>>> +++ b/include/hw/vfio/vfio-common.h >>>> @@ -218,6 +218,8 @@ typedef QLIST_HEAD(VFIOGroupList, VFIOGroup) >>>> VFIOGroupList; >>>> extern VFIOGroupList vfio_group_list; >>>> >>>> bool vfio_mig_active(void); >>>> +int vfio_block_multiple_devices_migration(Error **errp); >>>> +void vfio_unblock_multiple_devices_migration(void); >>>> int64_t vfio_mig_bytes_transferred(void); >>>> >>>> #ifdef CONFIG_LINUX >>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >>>> index 3a35f4afad..01db41b735 100644 >>>> --- a/hw/vfio/common.c >>>> +++ b/hw/vfio/common.c >>>> @@ -41,6 +41,7 @@ >>>> #include "qapi/error.h" >>>> #include "migration/migration.h" >>>> #include "migration/misc.h" >>>> +#include "migration/blocker.h" >>>> #include "sysemu/tpm.h" >>>> >>>> VFIOGroupList vfio_group_list = >>>> @@ -337,6 +338,56 @@ bool vfio_mig_active(void) >>>> return true; >>>> } >>>> >>>> +Error *multiple_devices_migration_blocker; > > static ? > Yes, I will add. > So we have two migration blockers, one per device and one global. I guess > it is easier that way than trying to update the per device Error*. Yes, because this blocker is not related to a specific device but rather to how many devices we have at a specific moment. Thanks.
On Wed, 8 Feb 2023 15:08:15 +0200 Avihai Horon <avihaih@nvidia.com> wrote: > On 08/02/2023 0:34, Alex Williamson wrote: > > External email: Use caution opening links or attachments > > > > > > On Mon, 6 Feb 2023 14:31:30 +0200 > > Avihai Horon <avihaih@nvidia.com> wrote: > > > >> Currently VFIO migration doesn't implement some kind of intermediate > >> quiescent state in which P2P DMAs are quiesced before stopping or > >> running the device. This can cause problems in multi-device migration > >> where the devices are doing P2P DMAs, since the devices are not stopped > >> together at the same time. > >> > >> Until such support is added, block migration of multiple devices. > >> > >> Signed-off-by: Avihai Horon <avihaih@nvidia.com> > >> --- > >> include/hw/vfio/vfio-common.h | 2 ++ > >> hw/vfio/common.c | 51 +++++++++++++++++++++++++++++++++++ > >> hw/vfio/migration.c | 6 +++++ > >> 3 files changed, 59 insertions(+) > >> > >> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h > >> index e573f5a9f1..56b1683824 100644 > >> --- a/include/hw/vfio/vfio-common.h > >> +++ b/include/hw/vfio/vfio-common.h > >> @@ -218,6 +218,8 @@ typedef QLIST_HEAD(VFIOGroupList, VFIOGroup) VFIOGroupList; > >> extern VFIOGroupList vfio_group_list; > >> > >> bool vfio_mig_active(void); > >> +int vfio_block_multiple_devices_migration(Error **errp); > >> +void vfio_unblock_multiple_devices_migration(void); > >> int64_t vfio_mig_bytes_transferred(void); > >> > >> #ifdef CONFIG_LINUX > >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c > >> index 3a35f4afad..01db41b735 100644 > >> --- a/hw/vfio/common.c > >> +++ b/hw/vfio/common.c > >> @@ -41,6 +41,7 @@ > >> #include "qapi/error.h" > >> #include "migration/migration.h" > >> #include "migration/misc.h" > >> +#include "migration/blocker.h" > >> #include "sysemu/tpm.h" > >> > >> VFIOGroupList vfio_group_list = > >> @@ -337,6 +338,56 @@ bool vfio_mig_active(void) > >> return true; > >> } > >> > >> +Error *multiple_devices_migration_blocker; > >> + > >> +static unsigned int vfio_migratable_device_num(void) > >> +{ > >> + VFIOGroup *group; > >> + VFIODevice *vbasedev; > >> + unsigned int device_num = 0; > >> + > >> + QLIST_FOREACH(group, &vfio_group_list, next) { > >> + QLIST_FOREACH(vbasedev, &group->device_list, next) { > >> + if (vbasedev->migration) { > >> + device_num++; > >> + } > >> + } > >> + } > >> + > >> + return device_num; > >> +} > >> + > >> +int vfio_block_multiple_devices_migration(Error **errp) > >> +{ > >> + int ret; > >> + > >> + if (vfio_migratable_device_num() != 2) { > >> + return 0; > >> + } > >> + > >> + error_setg(&multiple_devices_migration_blocker, > >> + "Migration is currently not supported with multiple " > >> + "VFIO devices"); > >> + ret = migrate_add_blocker(multiple_devices_migration_blocker, errp); > >> + if (ret < 0) { > >> + error_free(multiple_devices_migration_blocker); > >> + multiple_devices_migration_blocker = NULL; > >> + } > >> + > >> + return ret; > >> +} > >> + > >> +void vfio_unblock_multiple_devices_migration(void) > >> +{ > >> + if (vfio_migratable_device_num() != 2) { > >> + return; > >> + } > >> + > >> + migrate_del_blocker(multiple_devices_migration_blocker); > >> + error_free(multiple_devices_migration_blocker); > >> + multiple_devices_migration_blocker = NULL; > >> +} > > A couple awkward things here. First I wish we could do something > > cleaner or more intuitive than the != 2 test. I get that we're trying > > to do this on the addition of the 2nd device supporting migration, or > > the removal of the next to last device independent of all other devices, > > but I wonder if it wouldn't be better to remove the multiple-device > > blocker after migration is torn down for the device so we can test > > device >1 or ==1 in combination with whether > > multiple_devices_migration_blocker is NULL. > > > > Which comes to the second awkwardness, if we fail to add the blocker we > > free and clear the blocker, but when we tear down the device due to that > > failure we'll remove the blocker that doesn't exist, free NULL, and > > clear it again. Thanks to the glib slist the migration blocker is > > using, I think that all works, but I'd rather not be dependent on that > > implementation to avoid a segfault here. Incorporating a test of > > multiple_devices_migration_blocker as above would avoid this too. > > You mean something like this? > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index 3a35f4afad..f3e08eff58 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > > [...] > > +int vfio_block_multiple_devices_migration(Error **errp) > +{ > + int ret; > + > + if (vfio_migratable_device_num() <= 1 || > + multiple_devices_migration_blocker) { > + return 0; > + } Nit, I'd reverse the order of the test here and below, otherwise yes, this is what I was thinking of. > + > + error_setg(&multiple_devices_migration_blocker, > + "Migration is currently not supported with multiple " > + "VFIO devices"); > + ret = migrate_add_blocker(multiple_devices_migration_blocker, errp); > + if (ret < 0) { > + error_free(multiple_devices_migration_blocker); > + multiple_devices_migration_blocker = NULL; > + } > + > + return ret; > +} > + > +void vfio_unblock_multiple_devices_migration(void) > +{ > + if (vfio_migratable_device_num() > 1 || > + !multiple_devices_migration_blocker) { > + return; > + } > + > + migrate_del_blocker(multiple_devices_migration_blocker); > + error_free(multiple_devices_migration_blocker); > + multiple_devices_migration_blocker = NULL; > +} > + > static bool vfio_devices_all_dirty_tracking(VFIOContainer *container) > { > VFIOGroup *group; > diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c > index 552c2313b2..15b446c0ec 100644 > --- a/hw/vfio/migration.c > +++ b/hw/vfio/migration.c > @@ -880,6 +880,11 @@ int vfio_migration_probe(VFIODevice *vbasedev, > Error **errp) > goto add_blocker; > } > > + ret = vfio_block_multiple_devices_migration(errp); > + if (ret) { > + return ret; > + } > + > trace_vfio_migration_probe(vbasedev->name, info->index); > g_free(info); > return 0; > @@ -906,6 +911,7 @@ void vfio_migration_finalize(VFIODevice *vbasedev) > qemu_del_vm_change_state_handler(migration->vm_state); > unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio", vbasedev); > vfio_migration_exit(vbasedev); > + vfio_unblock_multiple_devices_migration(); > } > > if (vbasedev->migration_blocker) { > > > Maybe also negate the if conditions and put the add/remove blocker code > inside it? Is it more readable this way? I think the previous aligns more with the success oriented flow that Jason like to promote. Thanks, Alex > E.g.: > > +int vfio_block_multiple_devices_migration(Error **errp) > +{ > + int ret = 0; > + > + if (vfio_migratable_device_num() > 1 && > + !multiple_devices_migration_blocker) { > + error_setg(&multiple_devices_migration_blocker, > + "Migration is currently not supported with multiple " > + "VFIO devices"); > + ret = migrate_add_blocker(multiple_devices_migration_blocker, > errp); > + if (ret < 0) { > + error_free(multiple_devices_migration_blocker); > + multiple_devices_migration_blocker = NULL; > + } > + } > + > + return ret; > +} > + > +void vfio_unblock_multiple_devices_migration(void) > +{ > + if (vfio_migratable_device_num() <= 1 && > + multiple_devices_migration_blocker) { > + migrate_del_blocker(multiple_devices_migration_blocker); > + error_free(multiple_devices_migration_blocker); > + multiple_devices_migration_blocker = NULL; > + } > +} > > Thanks. >
On 08/02/2023 19:22, Alex Williamson wrote: > External email: Use caution opening links or attachments > > > On Wed, 8 Feb 2023 15:08:15 +0200 > Avihai Horon <avihaih@nvidia.com> wrote: > >> On 08/02/2023 0:34, Alex Williamson wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> On Mon, 6 Feb 2023 14:31:30 +0200 >>> Avihai Horon <avihaih@nvidia.com> wrote: >>> >>>> Currently VFIO migration doesn't implement some kind of intermediate >>>> quiescent state in which P2P DMAs are quiesced before stopping or >>>> running the device. This can cause problems in multi-device migration >>>> where the devices are doing P2P DMAs, since the devices are not stopped >>>> together at the same time. >>>> >>>> Until such support is added, block migration of multiple devices. >>>> >>>> Signed-off-by: Avihai Horon <avihaih@nvidia.com> >>>> --- >>>> include/hw/vfio/vfio-common.h | 2 ++ >>>> hw/vfio/common.c | 51 +++++++++++++++++++++++++++++++++++ >>>> hw/vfio/migration.c | 6 +++++ >>>> 3 files changed, 59 insertions(+) >>>> >>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h >>>> index e573f5a9f1..56b1683824 100644 >>>> --- a/include/hw/vfio/vfio-common.h >>>> +++ b/include/hw/vfio/vfio-common.h >>>> @@ -218,6 +218,8 @@ typedef QLIST_HEAD(VFIOGroupList, VFIOGroup) VFIOGroupList; >>>> extern VFIOGroupList vfio_group_list; >>>> >>>> bool vfio_mig_active(void); >>>> +int vfio_block_multiple_devices_migration(Error **errp); >>>> +void vfio_unblock_multiple_devices_migration(void); >>>> int64_t vfio_mig_bytes_transferred(void); >>>> >>>> #ifdef CONFIG_LINUX >>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >>>> index 3a35f4afad..01db41b735 100644 >>>> --- a/hw/vfio/common.c >>>> +++ b/hw/vfio/common.c >>>> @@ -41,6 +41,7 @@ >>>> #include "qapi/error.h" >>>> #include "migration/migration.h" >>>> #include "migration/misc.h" >>>> +#include "migration/blocker.h" >>>> #include "sysemu/tpm.h" >>>> >>>> VFIOGroupList vfio_group_list = >>>> @@ -337,6 +338,56 @@ bool vfio_mig_active(void) >>>> return true; >>>> } >>>> >>>> +Error *multiple_devices_migration_blocker; >>>> + >>>> +static unsigned int vfio_migratable_device_num(void) >>>> +{ >>>> + VFIOGroup *group; >>>> + VFIODevice *vbasedev; >>>> + unsigned int device_num = 0; >>>> + >>>> + QLIST_FOREACH(group, &vfio_group_list, next) { >>>> + QLIST_FOREACH(vbasedev, &group->device_list, next) { >>>> + if (vbasedev->migration) { >>>> + device_num++; >>>> + } >>>> + } >>>> + } >>>> + >>>> + return device_num; >>>> +} >>>> + >>>> +int vfio_block_multiple_devices_migration(Error **errp) >>>> +{ >>>> + int ret; >>>> + >>>> + if (vfio_migratable_device_num() != 2) { >>>> + return 0; >>>> + } >>>> + >>>> + error_setg(&multiple_devices_migration_blocker, >>>> + "Migration is currently not supported with multiple " >>>> + "VFIO devices"); >>>> + ret = migrate_add_blocker(multiple_devices_migration_blocker, errp); >>>> + if (ret < 0) { >>>> + error_free(multiple_devices_migration_blocker); >>>> + multiple_devices_migration_blocker = NULL; >>>> + } >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +void vfio_unblock_multiple_devices_migration(void) >>>> +{ >>>> + if (vfio_migratable_device_num() != 2) { >>>> + return; >>>> + } >>>> + >>>> + migrate_del_blocker(multiple_devices_migration_blocker); >>>> + error_free(multiple_devices_migration_blocker); >>>> + multiple_devices_migration_blocker = NULL; >>>> +} >>> A couple awkward things here. First I wish we could do something >>> cleaner or more intuitive than the != 2 test. I get that we're trying >>> to do this on the addition of the 2nd device supporting migration, or >>> the removal of the next to last device independent of all other devices, >>> but I wonder if it wouldn't be better to remove the multiple-device >>> blocker after migration is torn down for the device so we can test >>> device >1 or ==1 in combination with whether >>> multiple_devices_migration_blocker is NULL. >>> >>> Which comes to the second awkwardness, if we fail to add the blocker we >>> free and clear the blocker, but when we tear down the device due to that >>> failure we'll remove the blocker that doesn't exist, free NULL, and >>> clear it again. Thanks to the glib slist the migration blocker is >>> using, I think that all works, but I'd rather not be dependent on that >>> implementation to avoid a segfault here. Incorporating a test of >>> multiple_devices_migration_blocker as above would avoid this too. >> You mean something like this? >> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index 3a35f4afad..f3e08eff58 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> >> [...] >> >> +int vfio_block_multiple_devices_migration(Error **errp) >> +{ >> + int ret; >> + >> + if (vfio_migratable_device_num() <= 1 || >> + multiple_devices_migration_blocker) { >> + return 0; >> + } > Nit, I'd reverse the order of the test here and below, otherwise yes, > this is what I was thinking of. Sure, I will reverse the order. >> + >> + error_setg(&multiple_devices_migration_blocker, >> + "Migration is currently not supported with multiple " >> + "VFIO devices"); >> + ret = migrate_add_blocker(multiple_devices_migration_blocker, errp); >> + if (ret < 0) { >> + error_free(multiple_devices_migration_blocker); >> + multiple_devices_migration_blocker = NULL; >> + } >> + >> + return ret; >> +} >> + >> +void vfio_unblock_multiple_devices_migration(void) >> +{ >> + if (vfio_migratable_device_num() > 1 || >> + !multiple_devices_migration_blocker) { >> + return; >> + } >> + >> + migrate_del_blocker(multiple_devices_migration_blocker); >> + error_free(multiple_devices_migration_blocker); >> + multiple_devices_migration_blocker = NULL; >> +} >> + >> static bool vfio_devices_all_dirty_tracking(VFIOContainer *container) >> { >> VFIOGroup *group; >> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c >> index 552c2313b2..15b446c0ec 100644 >> --- a/hw/vfio/migration.c >> +++ b/hw/vfio/migration.c >> @@ -880,6 +880,11 @@ int vfio_migration_probe(VFIODevice *vbasedev, >> Error **errp) >> goto add_blocker; >> } >> >> + ret = vfio_block_multiple_devices_migration(errp); >> + if (ret) { >> + return ret; >> + } >> + >> trace_vfio_migration_probe(vbasedev->name, info->index); >> g_free(info); >> return 0; >> @@ -906,6 +911,7 @@ void vfio_migration_finalize(VFIODevice *vbasedev) >> qemu_del_vm_change_state_handler(migration->vm_state); >> unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio", vbasedev); >> vfio_migration_exit(vbasedev); >> + vfio_unblock_multiple_devices_migration(); >> } >> >> if (vbasedev->migration_blocker) { >> >> >> Maybe also negate the if conditions and put the add/remove blocker code >> inside it? Is it more readable this way? > I think the previous aligns more with the success oriented flow that > Jason like to promote. OK, I will keep the previous then. Thanks. > >> E.g.: >> >> +int vfio_block_multiple_devices_migration(Error **errp) >> +{ >> + int ret = 0; >> + >> + if (vfio_migratable_device_num() > 1 && >> + !multiple_devices_migration_blocker) { >> + error_setg(&multiple_devices_migration_blocker, >> + "Migration is currently not supported with multiple " >> + "VFIO devices"); >> + ret = migrate_add_blocker(multiple_devices_migration_blocker, >> errp); >> + if (ret < 0) { >> + error_free(multiple_devices_migration_blocker); >> + multiple_devices_migration_blocker = NULL; >> + } >> + } >> + >> + return ret; >> +} >> + >> +void vfio_unblock_multiple_devices_migration(void) >> +{ >> + if (vfio_migratable_device_num() <= 1 && >> + multiple_devices_migration_blocker) { >> + migrate_del_blocker(multiple_devices_migration_blocker); >> + error_free(multiple_devices_migration_blocker); >> + multiple_devices_migration_blocker = NULL; >> + } >> +} >> >> Thanks. >>
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h index e573f5a9f1..56b1683824 100644 --- a/include/hw/vfio/vfio-common.h +++ b/include/hw/vfio/vfio-common.h @@ -218,6 +218,8 @@ typedef QLIST_HEAD(VFIOGroupList, VFIOGroup) VFIOGroupList; extern VFIOGroupList vfio_group_list; bool vfio_mig_active(void); +int vfio_block_multiple_devices_migration(Error **errp); +void vfio_unblock_multiple_devices_migration(void); int64_t vfio_mig_bytes_transferred(void); #ifdef CONFIG_LINUX diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 3a35f4afad..01db41b735 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -41,6 +41,7 @@ #include "qapi/error.h" #include "migration/migration.h" #include "migration/misc.h" +#include "migration/blocker.h" #include "sysemu/tpm.h" VFIOGroupList vfio_group_list = @@ -337,6 +338,56 @@ bool vfio_mig_active(void) return true; } +Error *multiple_devices_migration_blocker; + +static unsigned int vfio_migratable_device_num(void) +{ + VFIOGroup *group; + VFIODevice *vbasedev; + unsigned int device_num = 0; + + QLIST_FOREACH(group, &vfio_group_list, next) { + QLIST_FOREACH(vbasedev, &group->device_list, next) { + if (vbasedev->migration) { + device_num++; + } + } + } + + return device_num; +} + +int vfio_block_multiple_devices_migration(Error **errp) +{ + int ret; + + if (vfio_migratable_device_num() != 2) { + return 0; + } + + error_setg(&multiple_devices_migration_blocker, + "Migration is currently not supported with multiple " + "VFIO devices"); + ret = migrate_add_blocker(multiple_devices_migration_blocker, errp); + if (ret < 0) { + error_free(multiple_devices_migration_blocker); + multiple_devices_migration_blocker = NULL; + } + + return ret; +} + +void vfio_unblock_multiple_devices_migration(void) +{ + if (vfio_migratable_device_num() != 2) { + return; + } + + migrate_del_blocker(multiple_devices_migration_blocker); + error_free(multiple_devices_migration_blocker); + multiple_devices_migration_blocker = NULL; +} + static bool vfio_devices_all_dirty_tracking(VFIOContainer *container) { VFIOGroup *group; diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c index 552c2313b2..8085a4ff44 100644 --- a/hw/vfio/migration.c +++ b/hw/vfio/migration.c @@ -880,6 +880,11 @@ int vfio_migration_probe(VFIODevice *vbasedev, Error **errp) goto add_blocker; } + ret = vfio_block_multiple_devices_migration(errp); + if (ret) { + return ret; + } + trace_vfio_migration_probe(vbasedev->name, info->index); g_free(info); return 0; @@ -902,6 +907,7 @@ void vfio_migration_finalize(VFIODevice *vbasedev) if (vbasedev->migration) { VFIOMigration *migration = vbasedev->migration; + vfio_unblock_multiple_devices_migration(); remove_migration_state_change_notifier(&migration->migration_state); qemu_del_vm_change_state_handler(migration->vm_state); unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio", vbasedev);
Currently VFIO migration doesn't implement some kind of intermediate quiescent state in which P2P DMAs are quiesced before stopping or running the device. This can cause problems in multi-device migration where the devices are doing P2P DMAs, since the devices are not stopped together at the same time. Until such support is added, block migration of multiple devices. Signed-off-by: Avihai Horon <avihaih@nvidia.com> --- include/hw/vfio/vfio-common.h | 2 ++ hw/vfio/common.c | 51 +++++++++++++++++++++++++++++++++++ hw/vfio/migration.c | 6 +++++ 3 files changed, 59 insertions(+)