| Message ID | 1714406135-451286-27-git-send-email-steven.sistare@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Live update: cpr-exec | expand |
Steve Sistare <steven.sistare@oracle.com> writes: > Add the only-migratable-modes option as a generalization of only-migratable. > Only devices that support all requested modes are allowed. > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com> > --- > include/migration/misc.h | 3 +++ > include/sysemu/sysemu.h | 1 - > migration/migration-hmp-cmds.c | 26 +++++++++++++++++++++++++- > migration/migration.c | 22 +++++++++++++++++----- > migration/savevm.c | 2 +- > qemu-options.hx | 16 ++++++++++++++-- > system/globals.c | 1 - > system/vl.c | 13 ++++++++++++- > target/s390x/cpu_models.c | 4 +++- > 9 files changed, 75 insertions(+), 13 deletions(-) > > diff --git a/include/migration/misc.h b/include/migration/misc.h > index 5b963ba..3ad2cd9 100644 > --- a/include/migration/misc.h > +++ b/include/migration/misc.h > @@ -119,6 +119,9 @@ bool migration_incoming_postcopy_advised(void); > /* True if background snapshot is active */ > bool migration_in_bg_snapshot(void); > > +void migration_set_required_mode(MigMode mode); > +bool migration_mode_required(MigMode mode); > + > /* migration/block-dirty-bitmap.c */ > void dirty_bitmap_mig_init(void); > > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h > index 5b4397e..0a9c4b4 100644 > --- a/include/sysemu/sysemu.h > +++ b/include/sysemu/sysemu.h > @@ -8,7 +8,6 @@ > > /* vl.c */ > > -extern int only_migratable; > extern const char *qemu_name; > extern QemuUUID qemu_uuid; > extern bool qemu_uuid_set; > diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c > index 414c7e8..ca913b7 100644 > --- a/migration/migration-hmp-cmds.c > +++ b/migration/migration-hmp-cmds.c > @@ -16,6 +16,7 @@ > #include "qemu/osdep.h" > #include "block/qapi.h" > #include "migration/snapshot.h" > +#include "migration/misc.h" > #include "monitor/hmp.h" > #include "monitor/monitor.h" > #include "qapi/error.h" > @@ -33,6 +34,28 @@ > #include "options.h" > #include "migration.h" > > +static void migration_dump_modes(Monitor *mon) > +{ > + int mode, n = 0; > + > + monitor_printf(mon, "only-migratable-modes: "); > + > + for (mode = 0; mode < MIG_MODE__MAX; mode++) { > + if (migration_mode_required(mode)) { > + if (n++) { > + monitor_printf(mon, ","); > + } > + monitor_printf(mon, "%s", MigMode_str(mode)); > + } > + } > + > + if (!n) { > + monitor_printf(mon, "none\n"); > + } else { > + monitor_printf(mon, "\n"); > + } > +} > + > static void migration_global_dump(Monitor *mon) > { > MigrationState *ms = migrate_get_current(); > @@ -41,7 +64,7 @@ static void migration_global_dump(Monitor *mon) > monitor_printf(mon, "store-global-state: %s\n", > ms->store_global_state ? "on" : "off"); > monitor_printf(mon, "only-migratable: %s\n", > - only_migratable ? "on" : "off"); > + migration_mode_required(MIG_MODE_NORMAL) ? "on" : "off"); > monitor_printf(mon, "send-configuration: %s\n", > ms->send_configuration ? "on" : "off"); > monitor_printf(mon, "send-section-footer: %s\n", > @@ -50,6 +73,7 @@ static void migration_global_dump(Monitor *mon) > ms->decompress_error_check ? "on" : "off"); > monitor_printf(mon, "clear-bitmap-shift: %u\n", > ms->clear_bitmap_shift); > + migration_dump_modes(mon); > } > > void hmp_info_migrate(Monitor *mon, const QDict *qdict) > diff --git a/migration/migration.c b/migration/migration.c > index 4984dee..5535b84 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -1719,17 +1719,29 @@ static bool is_busy(Error **reasonp, Error **errp) > return false; > } > > -static bool is_only_migratable(Error **reasonp, Error **errp, int modes) > +static int migration_modes_required; > + > +void migration_set_required_mode(MigMode mode) > +{ > + migration_modes_required |= BIT(mode); > +} > + > +bool migration_mode_required(MigMode mode) > +{ > + return !!(migration_modes_required & BIT(mode)); > +} > + > +static bool modes_are_required(Error **reasonp, Error **errp, int modes) > { > ERRP_GUARD(); > > - if (only_migratable && (modes & BIT(MIG_MODE_NORMAL))) { > + if (migration_modes_required & modes) { > error_propagate_prepend(errp, *reasonp, > - "disallowing migration blocker " > - "(--only-migratable) for: "); > + "-only-migratable{-modes} specified, but: "); extra space before 'specified' > *reasonp = NULL; > return true; > } > + > return false; > } > > @@ -1783,7 +1795,7 @@ int migrate_add_blocker_modes(Error **reasonp, Error **errp, MigMode mode, ...) > modes = get_modes(mode, ap); > va_end(ap); > > - if (is_only_migratable(reasonp, errp, modes)) { > + if (modes_are_required(reasonp, errp, modes)) { > return -EACCES; > } else if (is_busy(reasonp, errp)) { > return -EBUSY; > diff --git a/migration/savevm.c b/migration/savevm.c > index 6087c3a..e53ac84 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -3585,7 +3585,7 @@ void vmstate_register_ram_global(MemoryRegion *mr) > bool vmstate_check_only_migratable(const VMStateDescription *vmsd) > { > /* check needed if --only-migratable is specified */ > - if (!only_migratable) { > + if (!migration_mode_required(MIG_MODE_NORMAL)) { > return true; > } > > diff --git a/qemu-options.hx b/qemu-options.hx > index f0dfda5..946d731 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -4807,8 +4807,20 @@ DEF("only-migratable", 0, QEMU_OPTION_only_migratable, \ > "-only-migratable allow only migratable devices\n", QEMU_ARCH_ALL) > SRST > ``-only-migratable`` > - Only allow migratable devices. Devices will not be allowed to enter > - an unmigratable state. > + Only allow devices that can migrate using normal mode. Devices will not > + be allowed to enter an unmigratable state. What's a "normal" mode is what people will ask. I don't think we need to expose this. This option never had anything to do with "modes" and I think we can keep it this way. See below... > +ERST > + > +DEF("only-migratable-modes", HAS_ARG, QEMU_OPTION_only_migratable_modes, \ > + "-only-migratable-modes mode1[,...]\n" > + " allow only devices that are migratable using mode(s)\n", > + QEMU_ARCH_ALL) > +SRST > +``-only-migratable-modes mode1[,...]`` > + Only allow devices which are migratable using all modes in the list, > + which guarantees that migration will not fail due to a blocker. > + If both only-migratable-modes and only-migratable are specified, > + or are specified multiple times, then the required modes accumulate. > ERST > > DEF("nodefaults", 0, QEMU_OPTION_nodefaults, \ > diff --git a/system/globals.c b/system/globals.c > index e353584..fdc263e 100644 > --- a/system/globals.c > +++ b/system/globals.c > @@ -48,7 +48,6 @@ const char *qemu_name; > unsigned int nb_prom_envs; > const char *prom_envs[MAX_PROM_ENVS]; > uint8_t *boot_splash_filedata; > -int only_migratable; /* turn it off unless user states otherwise */ > int icount_align_option; > > /* The bytes in qemu_uuid are in the order specified by RFC4122, _not_ in the > diff --git a/system/vl.c b/system/vl.c > index b76881e..7e73be9 100644 > --- a/system/vl.c > +++ b/system/vl.c > @@ -3458,7 +3458,18 @@ void qemu_init(int argc, char **argv) > incoming = optarg; > break; > case QEMU_OPTION_only_migratable: > - only_migratable = 1; > + migration_set_required_mode(MIG_MODE_NORMAL); ...from the point of view of user intent, I think this should be MIG_MODE_ALL. If I have this option set I never want to see a blocker, period. That's not a change in behavior because the mode has to be explicitly selected anyway. > + break; > + case QEMU_OPTION_only_migratable_modes: > + { > + int i, mode; > + g_autofree char **words = g_strsplit(optarg, ",", -1); > + for (i = 0; words[i]; i++) { > + mode = qapi_enum_parse(&MigMode_lookup, words[i], -1, > + &error_fatal); > + migration_set_required_mode(mode); This option can be used to refine the modes being considered, it should take precedence if both are present. > + } > + } > break; > case QEMU_OPTION_nodefaults: > has_defaults = 0; > diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c > index 8ed3bb6..42ad160 100644 > --- a/target/s390x/cpu_models.c > +++ b/target/s390x/cpu_models.c > @@ -16,6 +16,7 @@ > #include "kvm/kvm_s390x.h" > #include "sysemu/kvm.h" > #include "sysemu/tcg.h" > +#include "migration/misc.h" > #include "qapi/error.h" > #include "qemu/error-report.h" > #include "qapi/visitor.h" > @@ -526,7 +527,8 @@ static void check_compatibility(const S390CPUModel *max_model, > } > > #ifndef CONFIG_USER_ONLY > - if (only_migratable && test_bit(S390_FEAT_UNPACK, model->features)) { > + if (migration_mode_required(MIG_MODE_NORMAL) && > + test_bit(S390_FEAT_UNPACK, model->features)) { > error_setg(errp, "The unpack facility is not compatible with " > "the --only-migratable option. You must remove either " > "the 'unpack' facility or the --only-migratable option");
On 5/9/2024 3:14 PM, Fabiano Rosas wrote: > Steve Sistare <steven.sistare@oracle.com> writes: > >> Add the only-migratable-modes option as a generalization of only-migratable. >> Only devices that support all requested modes are allowed. >> >> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> >> --- >> include/migration/misc.h | 3 +++ >> include/sysemu/sysemu.h | 1 - >> migration/migration-hmp-cmds.c | 26 +++++++++++++++++++++++++- >> migration/migration.c | 22 +++++++++++++++++----- >> migration/savevm.c | 2 +- >> qemu-options.hx | 16 ++++++++++++++-- >> system/globals.c | 1 - >> system/vl.c | 13 ++++++++++++- >> target/s390x/cpu_models.c | 4 +++- >> 9 files changed, 75 insertions(+), 13 deletions(-) >> >> diff --git a/include/migration/misc.h b/include/migration/misc.h >> index 5b963ba..3ad2cd9 100644 >> --- a/include/migration/misc.h >> +++ b/include/migration/misc.h >> @@ -119,6 +119,9 @@ bool migration_incoming_postcopy_advised(void); >> /* True if background snapshot is active */ >> bool migration_in_bg_snapshot(void); >> >> +void migration_set_required_mode(MigMode mode); >> +bool migration_mode_required(MigMode mode); >> + >> /* migration/block-dirty-bitmap.c */ >> void dirty_bitmap_mig_init(void); >> >> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h >> index 5b4397e..0a9c4b4 100644 >> --- a/include/sysemu/sysemu.h >> +++ b/include/sysemu/sysemu.h >> @@ -8,7 +8,6 @@ >> >> /* vl.c */ >> >> -extern int only_migratable; >> extern const char *qemu_name; >> extern QemuUUID qemu_uuid; >> extern bool qemu_uuid_set; >> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c >> index 414c7e8..ca913b7 100644 >> --- a/migration/migration-hmp-cmds.c >> +++ b/migration/migration-hmp-cmds.c >> @@ -16,6 +16,7 @@ >> #include "qemu/osdep.h" >> #include "block/qapi.h" >> #include "migration/snapshot.h" >> +#include "migration/misc.h" >> #include "monitor/hmp.h" >> #include "monitor/monitor.h" >> #include "qapi/error.h" >> @@ -33,6 +34,28 @@ >> #include "options.h" >> #include "migration.h" >> >> +static void migration_dump_modes(Monitor *mon) >> +{ >> + int mode, n = 0; >> + >> + monitor_printf(mon, "only-migratable-modes: "); >> + >> + for (mode = 0; mode < MIG_MODE__MAX; mode++) { >> + if (migration_mode_required(mode)) { >> + if (n++) { >> + monitor_printf(mon, ","); >> + } >> + monitor_printf(mon, "%s", MigMode_str(mode)); >> + } >> + } >> + >> + if (!n) { >> + monitor_printf(mon, "none\n"); >> + } else { >> + monitor_printf(mon, "\n"); >> + } >> +} >> + >> static void migration_global_dump(Monitor *mon) >> { >> MigrationState *ms = migrate_get_current(); >> @@ -41,7 +64,7 @@ static void migration_global_dump(Monitor *mon) >> monitor_printf(mon, "store-global-state: %s\n", >> ms->store_global_state ? "on" : "off"); >> monitor_printf(mon, "only-migratable: %s\n", >> - only_migratable ? "on" : "off"); >> + migration_mode_required(MIG_MODE_NORMAL) ? "on" : "off"); >> monitor_printf(mon, "send-configuration: %s\n", >> ms->send_configuration ? "on" : "off"); >> monitor_printf(mon, "send-section-footer: %s\n", >> @@ -50,6 +73,7 @@ static void migration_global_dump(Monitor *mon) >> ms->decompress_error_check ? "on" : "off"); >> monitor_printf(mon, "clear-bitmap-shift: %u\n", >> ms->clear_bitmap_shift); >> + migration_dump_modes(mon); >> } >> >> void hmp_info_migrate(Monitor *mon, const QDict *qdict) >> diff --git a/migration/migration.c b/migration/migration.c >> index 4984dee..5535b84 100644 >> --- a/migration/migration.c >> +++ b/migration/migration.c >> @@ -1719,17 +1719,29 @@ static bool is_busy(Error **reasonp, Error **errp) >> return false; >> } >> >> -static bool is_only_migratable(Error **reasonp, Error **errp, int modes) >> +static int migration_modes_required; >> + >> +void migration_set_required_mode(MigMode mode) >> +{ >> + migration_modes_required |= BIT(mode); >> +} >> + >> +bool migration_mode_required(MigMode mode) >> +{ >> + return !!(migration_modes_required & BIT(mode)); >> +} >> + >> +static bool modes_are_required(Error **reasonp, Error **errp, int modes) >> { >> ERRP_GUARD(); >> >> - if (only_migratable && (modes & BIT(MIG_MODE_NORMAL))) { >> + if (migration_modes_required & modes) { >> error_propagate_prepend(errp, *reasonp, >> - "disallowing migration blocker " >> - "(--only-migratable) for: "); >> + "-only-migratable{-modes} specified, but: "); > > extra space before 'specified' Will fix, thanks. >> *reasonp = NULL; >> return true; >> } >> + >> return false; >> } >> >> @@ -1783,7 +1795,7 @@ int migrate_add_blocker_modes(Error **reasonp, Error **errp, MigMode mode, ...) >> modes = get_modes(mode, ap); >> va_end(ap); >> >> - if (is_only_migratable(reasonp, errp, modes)) { >> + if (modes_are_required(reasonp, errp, modes)) { >> return -EACCES; >> } else if (is_busy(reasonp, errp)) { >> return -EBUSY; >> diff --git a/migration/savevm.c b/migration/savevm.c >> index 6087c3a..e53ac84 100644 >> --- a/migration/savevm.c >> +++ b/migration/savevm.c >> @@ -3585,7 +3585,7 @@ void vmstate_register_ram_global(MemoryRegion *mr) >> bool vmstate_check_only_migratable(const VMStateDescription *vmsd) >> { >> /* check needed if --only-migratable is specified */ >> - if (!only_migratable) { >> + if (!migration_mode_required(MIG_MODE_NORMAL)) { >> return true; >> } >> >> diff --git a/qemu-options.hx b/qemu-options.hx >> index f0dfda5..946d731 100644 >> --- a/qemu-options.hx >> +++ b/qemu-options.hx >> @@ -4807,8 +4807,20 @@ DEF("only-migratable", 0, QEMU_OPTION_only_migratable, \ >> "-only-migratable allow only migratable devices\n", QEMU_ARCH_ALL) >> SRST >> ``-only-migratable`` >> - Only allow migratable devices. Devices will not be allowed to enter >> - an unmigratable state. >> + Only allow devices that can migrate using normal mode. Devices will not >> + be allowed to enter an unmigratable state. > > What's a "normal" mode is what people will ask. I don't think we need to > expose this. This option never had anything to do with "modes" and I > think we can keep it this way. See below... We now have a mode parameter and enum MigMode which includes normal, and is documented in qapi. >> +ERST >> + >> +DEF("only-migratable-modes", HAS_ARG, QEMU_OPTION_only_migratable_modes, \ >> + "-only-migratable-modes mode1[,...]\n" >> + " allow only devices that are migratable using mode(s)\n", >> + QEMU_ARCH_ALL) >> +SRST >> +``-only-migratable-modes mode1[,...]`` >> + Only allow devices which are migratable using all modes in the list, >> + which guarantees that migration will not fail due to a blocker. >> + If both only-migratable-modes and only-migratable are specified, >> + or are specified multiple times, then the required modes accumulate. >> ERST >> >> DEF("nodefaults", 0, QEMU_OPTION_nodefaults, \ >> diff --git a/system/globals.c b/system/globals.c >> index e353584..fdc263e 100644 >> --- a/system/globals.c >> +++ b/system/globals.c >> @@ -48,7 +48,6 @@ const char *qemu_name; >> unsigned int nb_prom_envs; >> const char *prom_envs[MAX_PROM_ENVS]; >> uint8_t *boot_splash_filedata; >> -int only_migratable; /* turn it off unless user states otherwise */ >> int icount_align_option; >> >> /* The bytes in qemu_uuid are in the order specified by RFC4122, _not_ in the >> diff --git a/system/vl.c b/system/vl.c >> index b76881e..7e73be9 100644 >> --- a/system/vl.c >> +++ b/system/vl.c >> @@ -3458,7 +3458,18 @@ void qemu_init(int argc, char **argv) >> incoming = optarg; >> break; >> case QEMU_OPTION_only_migratable: >> - only_migratable = 1; >> + migration_set_required_mode(MIG_MODE_NORMAL); > > ...from the point of view of user intent, I think this should be > MIG_MODE_ALL. If only-migratable applies to all modes, then: If a user only intends to use mode A, then a blocker for mode B will terminate qemu. Not good. Defining only-migratable to apply to normal mode is the backwards-compatible solution. - Steve > If I have this option set I never want to see a blocker, > period. That's not a change in behavior because the mode has to be > explicitly selected anyway. > >> + break; >> + case QEMU_OPTION_only_migratable_modes: >> + { >> + int i, mode; >> + g_autofree char **words = g_strsplit(optarg, ",", -1); >> + for (i = 0; words[i]; i++) { >> + mode = qapi_enum_parse(&MigMode_lookup, words[i], -1, >> + &error_fatal); >> + migration_set_required_mode(mode); > > This option can be used to refine the modes being considered, it should > take precedence if both are present. > >> + } >> + } >> break; >> case QEMU_OPTION_nodefaults: >> has_defaults = 0; >> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c >> index 8ed3bb6..42ad160 100644 >> --- a/target/s390x/cpu_models.c >> +++ b/target/s390x/cpu_models.c >> @@ -16,6 +16,7 @@ >> #include "kvm/kvm_s390x.h" >> #include "sysemu/kvm.h" >> #include "sysemu/tcg.h" >> +#include "migration/misc.h" >> #include "qapi/error.h" >> #include "qemu/error-report.h" >> #include "qapi/visitor.h" >> @@ -526,7 +527,8 @@ static void check_compatibility(const S390CPUModel *max_model, >> } >> >> #ifndef CONFIG_USER_ONLY >> - if (only_migratable && test_bit(S390_FEAT_UNPACK, model->features)) { >> + if (migration_mode_required(MIG_MODE_NORMAL) && >> + test_bit(S390_FEAT_UNPACK, model->features)) { >> error_setg(errp, "The unpack facility is not compatible with " >> "the --only-migratable option. You must remove either " >> "the 'unpack' facility or the --only-migratable option");
Steven Sistare <steven.sistare@oracle.com> writes: > On 5/9/2024 3:14 PM, Fabiano Rosas wrote: >> Steve Sistare <steven.sistare@oracle.com> writes: >> >>> Add the only-migratable-modes option as a generalization of only-migratable. >>> Only devices that support all requested modes are allowed. >>> >>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> >>> --- >>> include/migration/misc.h | 3 +++ >>> include/sysemu/sysemu.h | 1 - >>> migration/migration-hmp-cmds.c | 26 +++++++++++++++++++++++++- >>> migration/migration.c | 22 +++++++++++++++++----- >>> migration/savevm.c | 2 +- >>> qemu-options.hx | 16 ++++++++++++++-- >>> system/globals.c | 1 - >>> system/vl.c | 13 ++++++++++++- >>> target/s390x/cpu_models.c | 4 +++- >>> 9 files changed, 75 insertions(+), 13 deletions(-) >>> >>> diff --git a/include/migration/misc.h b/include/migration/misc.h >>> index 5b963ba..3ad2cd9 100644 >>> --- a/include/migration/misc.h >>> +++ b/include/migration/misc.h >>> @@ -119,6 +119,9 @@ bool migration_incoming_postcopy_advised(void); >>> /* True if background snapshot is active */ >>> bool migration_in_bg_snapshot(void); >>> >>> +void migration_set_required_mode(MigMode mode); >>> +bool migration_mode_required(MigMode mode); >>> + >>> /* migration/block-dirty-bitmap.c */ >>> void dirty_bitmap_mig_init(void); >>> >>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h >>> index 5b4397e..0a9c4b4 100644 >>> --- a/include/sysemu/sysemu.h >>> +++ b/include/sysemu/sysemu.h >>> @@ -8,7 +8,6 @@ >>> >>> /* vl.c */ >>> >>> -extern int only_migratable; >>> extern const char *qemu_name; >>> extern QemuUUID qemu_uuid; >>> extern bool qemu_uuid_set; >>> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c >>> index 414c7e8..ca913b7 100644 >>> --- a/migration/migration-hmp-cmds.c >>> +++ b/migration/migration-hmp-cmds.c >>> @@ -16,6 +16,7 @@ >>> #include "qemu/osdep.h" >>> #include "block/qapi.h" >>> #include "migration/snapshot.h" >>> +#include "migration/misc.h" >>> #include "monitor/hmp.h" >>> #include "monitor/monitor.h" >>> #include "qapi/error.h" >>> @@ -33,6 +34,28 @@ >>> #include "options.h" >>> #include "migration.h" >>> >>> +static void migration_dump_modes(Monitor *mon) >>> +{ >>> + int mode, n = 0; >>> + >>> + monitor_printf(mon, "only-migratable-modes: "); >>> + >>> + for (mode = 0; mode < MIG_MODE__MAX; mode++) { >>> + if (migration_mode_required(mode)) { >>> + if (n++) { >>> + monitor_printf(mon, ","); >>> + } >>> + monitor_printf(mon, "%s", MigMode_str(mode)); >>> + } >>> + } >>> + >>> + if (!n) { >>> + monitor_printf(mon, "none\n"); >>> + } else { >>> + monitor_printf(mon, "\n"); >>> + } >>> +} >>> + >>> static void migration_global_dump(Monitor *mon) >>> { >>> MigrationState *ms = migrate_get_current(); >>> @@ -41,7 +64,7 @@ static void migration_global_dump(Monitor *mon) >>> monitor_printf(mon, "store-global-state: %s\n", >>> ms->store_global_state ? "on" : "off"); >>> monitor_printf(mon, "only-migratable: %s\n", >>> - only_migratable ? "on" : "off"); >>> + migration_mode_required(MIG_MODE_NORMAL) ? "on" : "off"); >>> monitor_printf(mon, "send-configuration: %s\n", >>> ms->send_configuration ? "on" : "off"); >>> monitor_printf(mon, "send-section-footer: %s\n", >>> @@ -50,6 +73,7 @@ static void migration_global_dump(Monitor *mon) >>> ms->decompress_error_check ? "on" : "off"); >>> monitor_printf(mon, "clear-bitmap-shift: %u\n", >>> ms->clear_bitmap_shift); >>> + migration_dump_modes(mon); >>> } >>> >>> void hmp_info_migrate(Monitor *mon, const QDict *qdict) >>> diff --git a/migration/migration.c b/migration/migration.c >>> index 4984dee..5535b84 100644 >>> --- a/migration/migration.c >>> +++ b/migration/migration.c >>> @@ -1719,17 +1719,29 @@ static bool is_busy(Error **reasonp, Error **errp) >>> return false; >>> } >>> >>> -static bool is_only_migratable(Error **reasonp, Error **errp, int modes) >>> +static int migration_modes_required; >>> + >>> +void migration_set_required_mode(MigMode mode) >>> +{ >>> + migration_modes_required |= BIT(mode); >>> +} >>> + >>> +bool migration_mode_required(MigMode mode) >>> +{ >>> + return !!(migration_modes_required & BIT(mode)); >>> +} >>> + >>> +static bool modes_are_required(Error **reasonp, Error **errp, int modes) >>> { >>> ERRP_GUARD(); >>> >>> - if (only_migratable && (modes & BIT(MIG_MODE_NORMAL))) { >>> + if (migration_modes_required & modes) { >>> error_propagate_prepend(errp, *reasonp, >>> - "disallowing migration blocker " >>> - "(--only-migratable) for: "); >>> + "-only-migratable{-modes} specified, but: "); >> >> extra space before 'specified' > > Will fix, thanks. > >>> *reasonp = NULL; >>> return true; >>> } >>> + >>> return false; >>> } >>> >>> @@ -1783,7 +1795,7 @@ int migrate_add_blocker_modes(Error **reasonp, Error **errp, MigMode mode, ...) >>> modes = get_modes(mode, ap); >>> va_end(ap); >>> >>> - if (is_only_migratable(reasonp, errp, modes)) { >>> + if (modes_are_required(reasonp, errp, modes)) { >>> return -EACCES; >>> } else if (is_busy(reasonp, errp)) { >>> return -EBUSY; >>> diff --git a/migration/savevm.c b/migration/savevm.c >>> index 6087c3a..e53ac84 100644 >>> --- a/migration/savevm.c >>> +++ b/migration/savevm.c >>> @@ -3585,7 +3585,7 @@ void vmstate_register_ram_global(MemoryRegion *mr) >>> bool vmstate_check_only_migratable(const VMStateDescription *vmsd) >>> { >>> /* check needed if --only-migratable is specified */ >>> - if (!only_migratable) { >>> + if (!migration_mode_required(MIG_MODE_NORMAL)) { >>> return true; >>> } >>> >>> diff --git a/qemu-options.hx b/qemu-options.hx >>> index f0dfda5..946d731 100644 >>> --- a/qemu-options.hx >>> +++ b/qemu-options.hx >>> @@ -4807,8 +4807,20 @@ DEF("only-migratable", 0, QEMU_OPTION_only_migratable, \ >>> "-only-migratable allow only migratable devices\n", QEMU_ARCH_ALL) >>> SRST >>> ``-only-migratable`` >>> - Only allow migratable devices. Devices will not be allowed to enter >>> - an unmigratable state. >>> + Only allow devices that can migrate using normal mode. Devices will not >>> + be allowed to enter an unmigratable state. >> >> What's a "normal" mode is what people will ask. I don't think we need to >> expose this. This option never had anything to do with "modes" and I >> think we can keep it this way. See below... > > We now have a mode parameter and enum MigMode which includes normal, and is > documented in qapi. > Alright, I take your point below. We could declare -only-migratable superseded by -only-migratable-modes then: ``-only-migratable`` Only allow devices that can migrate using normal mode. Devices will not be allowed to enter an unmigratable state. Same as -only-migratable-modes normal. Kept for backward-compatibility. >>> +ERST >>> + >>> +DEF("only-migratable-modes", HAS_ARG, QEMU_OPTION_only_migratable_modes, \ >>> + "-only-migratable-modes mode1[,...]\n" >>> + " allow only devices that are migratable using mode(s)\n", >>> + QEMU_ARCH_ALL) >>> +SRST >>> +``-only-migratable-modes mode1[,...]`` >>> + Only allow devices which are migratable using all modes in the list, >>> + which guarantees that migration will not fail due to a blocker. >>> + If both only-migratable-modes and only-migratable are specified, >>> + or are specified multiple times, then the required modes accumulate. >>> ERST >>> >>> DEF("nodefaults", 0, QEMU_OPTION_nodefaults, \ >>> diff --git a/system/globals.c b/system/globals.c >>> index e353584..fdc263e 100644 >>> --- a/system/globals.c >>> +++ b/system/globals.c >>> @@ -48,7 +48,6 @@ const char *qemu_name; >>> unsigned int nb_prom_envs; >>> const char *prom_envs[MAX_PROM_ENVS]; >>> uint8_t *boot_splash_filedata; >>> -int only_migratable; /* turn it off unless user states otherwise */ >>> int icount_align_option; >>> >>> /* The bytes in qemu_uuid are in the order specified by RFC4122, _not_ in the >>> diff --git a/system/vl.c b/system/vl.c >>> index b76881e..7e73be9 100644 >>> --- a/system/vl.c >>> +++ b/system/vl.c >>> @@ -3458,7 +3458,18 @@ void qemu_init(int argc, char **argv) >>> incoming = optarg; >>> break; >>> case QEMU_OPTION_only_migratable: >>> - only_migratable = 1; >>> + migration_set_required_mode(MIG_MODE_NORMAL); >> >> ...from the point of view of user intent, I think this should be >> MIG_MODE_ALL. > If only-migratable applies to all modes, then: > If a user only intends to use mode A, then a blocker for mode B will terminate > qemu. Not good. Ok, we can't know the mode before migration time, nevermind. > > Defining only-migratable to apply to normal mode is the backwards-compatible > solution. > > - Steve > >> If I have this option set I never want to see a blocker, >> period. That's not a change in behavior because the mode has to be >> explicitly selected anyway. >> >>> + break; >>> + case QEMU_OPTION_only_migratable_modes: >>> + { >>> + int i, mode; >>> + g_autofree char **words = g_strsplit(optarg, ",", -1); >>> + for (i = 0; words[i]; i++) { >>> + mode = qapi_enum_parse(&MigMode_lookup, words[i], -1, >>> + &error_fatal); >>> + migration_set_required_mode(mode); >> >> This option can be used to refine the modes being considered, it should >> take precedence if both are present. >> >>> + } >>> + } >>> break; >>> case QEMU_OPTION_nodefaults: >>> has_defaults = 0; >>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c >>> index 8ed3bb6..42ad160 100644 >>> --- a/target/s390x/cpu_models.c >>> +++ b/target/s390x/cpu_models.c >>> @@ -16,6 +16,7 @@ >>> #include "kvm/kvm_s390x.h" >>> #include "sysemu/kvm.h" >>> #include "sysemu/tcg.h" >>> +#include "migration/misc.h" >>> #include "qapi/error.h" >>> #include "qemu/error-report.h" >>> #include "qapi/visitor.h" >>> @@ -526,7 +527,8 @@ static void check_compatibility(const S390CPUModel *max_model, >>> } >>> >>> #ifndef CONFIG_USER_ONLY >>> - if (only_migratable && test_bit(S390_FEAT_UNPACK, model->features)) { >>> + if (migration_mode_required(MIG_MODE_NORMAL) && >>> + test_bit(S390_FEAT_UNPACK, model->features)) { >>> error_setg(errp, "The unpack facility is not compatible with " >>> "the --only-migratable option. You must remove either " >>> "the 'unpack' facility or the --only-migratable option");
On Mon, Apr 29, 2024 at 08:55:35AM -0700, Steve Sistare wrote: > Add the only-migratable-modes option as a generalization of only-migratable. > Only devices that support all requested modes are allowed. > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com> > --- > include/migration/misc.h | 3 +++ > include/sysemu/sysemu.h | 1 - > migration/migration-hmp-cmds.c | 26 +++++++++++++++++++++++++- > migration/migration.c | 22 +++++++++++++++++----- > migration/savevm.c | 2 +- > qemu-options.hx | 16 ++++++++++++++-- > system/globals.c | 1 - > system/vl.c | 13 ++++++++++++- > target/s390x/cpu_models.c | 4 +++- > 9 files changed, 75 insertions(+), 13 deletions(-) > diff --git a/qemu-options.hx b/qemu-options.hx > index f0dfda5..946d731 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -4807,8 +4807,20 @@ DEF("only-migratable", 0, QEMU_OPTION_only_migratable, \ > "-only-migratable allow only migratable devices\n", QEMU_ARCH_ALL) > SRST > ``-only-migratable`` > - Only allow migratable devices. Devices will not be allowed to enter > - an unmigratable state. > + Only allow devices that can migrate using normal mode. Devices will not > + be allowed to enter an unmigratable state. > +ERST > + > +DEF("only-migratable-modes", HAS_ARG, QEMU_OPTION_only_migratable_modes, \ > + "-only-migratable-modes mode1[,...]\n" > + " allow only devices that are migratable using mode(s)\n", > + QEMU_ARCH_ALL) > +SRST > +``-only-migratable-modes mode1[,...]`` > + Only allow devices which are migratable using all modes in the list, > + which guarantees that migration will not fail due to a blocker. > + If both only-migratable-modes and only-migratable are specified, > + or are specified multiple times, then the required modes accumulate. > ERST Adding new top level CLI options is not something we much like doing these days. Also its is preferrable to define args using QAPI rather than creating hand written parsers The pre-existing -only-migratable flag isn't ideal either, as a random top level flag on its own. I tend to think we should probably make both these arguments become properties in the Machine class, and thus settable with the existing -machine argument, since they're describing a requirement of the machine and devices added to it. The existing -only-migratable can be deprecated and simply made to set the corresponding machien property > diff --git a/system/globals.c b/system/globals.c > index e353584..fdc263e 100644 > --- a/system/globals.c > +++ b/system/globals.c > @@ -48,7 +48,6 @@ const char *qemu_name; > unsigned int nb_prom_envs; > const char *prom_envs[MAX_PROM_ENVS]; > uint8_t *boot_splash_filedata; > -int only_migratable; /* turn it off unless user states otherwise */ > int icount_align_option; > > /* The bytes in qemu_uuid are in the order specified by RFC4122, _not_ in the > diff --git a/system/vl.c b/system/vl.c > index b76881e..7e73be9 100644 > --- a/system/vl.c > +++ b/system/vl.c > @@ -3458,7 +3458,18 @@ void qemu_init(int argc, char **argv) > incoming = optarg; > break; > case QEMU_OPTION_only_migratable: > - only_migratable = 1; > + migration_set_required_mode(MIG_MODE_NORMAL); > + break; > + case QEMU_OPTION_only_migratable_modes: > + { > + int i, mode; > + g_autofree char **words = g_strsplit(optarg, ",", -1); > + for (i = 0; words[i]; i++) { > + mode = qapi_enum_parse(&MigMode_lookup, words[i], -1, > + &error_fatal); > + migration_set_required_mode(mode); > + } > + } > break; > case QEMU_OPTION_nodefaults: > has_defaults = 0; > diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c > index 8ed3bb6..42ad160 100644 > --- a/target/s390x/cpu_models.c > +++ b/target/s390x/cpu_models.c > @@ -16,6 +16,7 @@ > #include "kvm/kvm_s390x.h" > #include "sysemu/kvm.h" > #include "sysemu/tcg.h" > +#include "migration/misc.h" > #include "qapi/error.h" > #include "qemu/error-report.h" > #include "qapi/visitor.h" > @@ -526,7 +527,8 @@ static void check_compatibility(const S390CPUModel *max_model, > } > > #ifndef CONFIG_USER_ONLY > - if (only_migratable && test_bit(S390_FEAT_UNPACK, model->features)) { > + if (migration_mode_required(MIG_MODE_NORMAL) && > + test_bit(S390_FEAT_UNPACK, model->features)) { > error_setg(errp, "The unpack facility is not compatible with " > "the --only-migratable option. You must remove either " > "the 'unpack' facility or the --only-migratable option"); > -- > 1.8.3.1 > With regards, Daniel
diff --git a/include/migration/misc.h b/include/migration/misc.h index 5b963ba..3ad2cd9 100644 --- a/include/migration/misc.h +++ b/include/migration/misc.h @@ -119,6 +119,9 @@ bool migration_incoming_postcopy_advised(void); /* True if background snapshot is active */ bool migration_in_bg_snapshot(void); +void migration_set_required_mode(MigMode mode); +bool migration_mode_required(MigMode mode); + /* migration/block-dirty-bitmap.c */ void dirty_bitmap_mig_init(void); diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h index 5b4397e..0a9c4b4 100644 --- a/include/sysemu/sysemu.h +++ b/include/sysemu/sysemu.h @@ -8,7 +8,6 @@ /* vl.c */ -extern int only_migratable; extern const char *qemu_name; extern QemuUUID qemu_uuid; extern bool qemu_uuid_set; diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c index 414c7e8..ca913b7 100644 --- a/migration/migration-hmp-cmds.c +++ b/migration/migration-hmp-cmds.c @@ -16,6 +16,7 @@ #include "qemu/osdep.h" #include "block/qapi.h" #include "migration/snapshot.h" +#include "migration/misc.h" #include "monitor/hmp.h" #include "monitor/monitor.h" #include "qapi/error.h" @@ -33,6 +34,28 @@ #include "options.h" #include "migration.h" +static void migration_dump_modes(Monitor *mon) +{ + int mode, n = 0; + + monitor_printf(mon, "only-migratable-modes: "); + + for (mode = 0; mode < MIG_MODE__MAX; mode++) { + if (migration_mode_required(mode)) { + if (n++) { + monitor_printf(mon, ","); + } + monitor_printf(mon, "%s", MigMode_str(mode)); + } + } + + if (!n) { + monitor_printf(mon, "none\n"); + } else { + monitor_printf(mon, "\n"); + } +} + static void migration_global_dump(Monitor *mon) { MigrationState *ms = migrate_get_current(); @@ -41,7 +64,7 @@ static void migration_global_dump(Monitor *mon) monitor_printf(mon, "store-global-state: %s\n", ms->store_global_state ? "on" : "off"); monitor_printf(mon, "only-migratable: %s\n", - only_migratable ? "on" : "off"); + migration_mode_required(MIG_MODE_NORMAL) ? "on" : "off"); monitor_printf(mon, "send-configuration: %s\n", ms->send_configuration ? "on" : "off"); monitor_printf(mon, "send-section-footer: %s\n", @@ -50,6 +73,7 @@ static void migration_global_dump(Monitor *mon) ms->decompress_error_check ? "on" : "off"); monitor_printf(mon, "clear-bitmap-shift: %u\n", ms->clear_bitmap_shift); + migration_dump_modes(mon); } void hmp_info_migrate(Monitor *mon, const QDict *qdict) diff --git a/migration/migration.c b/migration/migration.c index 4984dee..5535b84 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -1719,17 +1719,29 @@ static bool is_busy(Error **reasonp, Error **errp) return false; } -static bool is_only_migratable(Error **reasonp, Error **errp, int modes) +static int migration_modes_required; + +void migration_set_required_mode(MigMode mode) +{ + migration_modes_required |= BIT(mode); +} + +bool migration_mode_required(MigMode mode) +{ + return !!(migration_modes_required & BIT(mode)); +} + +static bool modes_are_required(Error **reasonp, Error **errp, int modes) { ERRP_GUARD(); - if (only_migratable && (modes & BIT(MIG_MODE_NORMAL))) { + if (migration_modes_required & modes) { error_propagate_prepend(errp, *reasonp, - "disallowing migration blocker " - "(--only-migratable) for: "); + "-only-migratable{-modes} specified, but: "); *reasonp = NULL; return true; } + return false; } @@ -1783,7 +1795,7 @@ int migrate_add_blocker_modes(Error **reasonp, Error **errp, MigMode mode, ...) modes = get_modes(mode, ap); va_end(ap); - if (is_only_migratable(reasonp, errp, modes)) { + if (modes_are_required(reasonp, errp, modes)) { return -EACCES; } else if (is_busy(reasonp, errp)) { return -EBUSY; diff --git a/migration/savevm.c b/migration/savevm.c index 6087c3a..e53ac84 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -3585,7 +3585,7 @@ void vmstate_register_ram_global(MemoryRegion *mr) bool vmstate_check_only_migratable(const VMStateDescription *vmsd) { /* check needed if --only-migratable is specified */ - if (!only_migratable) { + if (!migration_mode_required(MIG_MODE_NORMAL)) { return true; } diff --git a/qemu-options.hx b/qemu-options.hx index f0dfda5..946d731 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -4807,8 +4807,20 @@ DEF("only-migratable", 0, QEMU_OPTION_only_migratable, \ "-only-migratable allow only migratable devices\n", QEMU_ARCH_ALL) SRST ``-only-migratable`` - Only allow migratable devices. Devices will not be allowed to enter - an unmigratable state. + Only allow devices that can migrate using normal mode. Devices will not + be allowed to enter an unmigratable state. +ERST + +DEF("only-migratable-modes", HAS_ARG, QEMU_OPTION_only_migratable_modes, \ + "-only-migratable-modes mode1[,...]\n" + " allow only devices that are migratable using mode(s)\n", + QEMU_ARCH_ALL) +SRST +``-only-migratable-modes mode1[,...]`` + Only allow devices which are migratable using all modes in the list, + which guarantees that migration will not fail due to a blocker. + If both only-migratable-modes and only-migratable are specified, + or are specified multiple times, then the required modes accumulate. ERST DEF("nodefaults", 0, QEMU_OPTION_nodefaults, \ diff --git a/system/globals.c b/system/globals.c index e353584..fdc263e 100644 --- a/system/globals.c +++ b/system/globals.c @@ -48,7 +48,6 @@ const char *qemu_name; unsigned int nb_prom_envs; const char *prom_envs[MAX_PROM_ENVS]; uint8_t *boot_splash_filedata; -int only_migratable; /* turn it off unless user states otherwise */ int icount_align_option; /* The bytes in qemu_uuid are in the order specified by RFC4122, _not_ in the diff --git a/system/vl.c b/system/vl.c index b76881e..7e73be9 100644 --- a/system/vl.c +++ b/system/vl.c @@ -3458,7 +3458,18 @@ void qemu_init(int argc, char **argv) incoming = optarg; break; case QEMU_OPTION_only_migratable: - only_migratable = 1; + migration_set_required_mode(MIG_MODE_NORMAL); + break; + case QEMU_OPTION_only_migratable_modes: + { + int i, mode; + g_autofree char **words = g_strsplit(optarg, ",", -1); + for (i = 0; words[i]; i++) { + mode = qapi_enum_parse(&MigMode_lookup, words[i], -1, + &error_fatal); + migration_set_required_mode(mode); + } + } break; case QEMU_OPTION_nodefaults: has_defaults = 0; diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c index 8ed3bb6..42ad160 100644 --- a/target/s390x/cpu_models.c +++ b/target/s390x/cpu_models.c @@ -16,6 +16,7 @@ #include "kvm/kvm_s390x.h" #include "sysemu/kvm.h" #include "sysemu/tcg.h" +#include "migration/misc.h" #include "qapi/error.h" #include "qemu/error-report.h" #include "qapi/visitor.h" @@ -526,7 +527,8 @@ static void check_compatibility(const S390CPUModel *max_model, } #ifndef CONFIG_USER_ONLY - if (only_migratable && test_bit(S390_FEAT_UNPACK, model->features)) { + if (migration_mode_required(MIG_MODE_NORMAL) && + test_bit(S390_FEAT_UNPACK, model->features)) { error_setg(errp, "The unpack facility is not compatible with " "the --only-migratable option. You must remove either " "the 'unpack' facility or the --only-migratable option");
Add the only-migratable-modes option as a generalization of only-migratable. Only devices that support all requested modes are allowed. Signed-off-by: Steve Sistare <steven.sistare@oracle.com> --- include/migration/misc.h | 3 +++ include/sysemu/sysemu.h | 1 - migration/migration-hmp-cmds.c | 26 +++++++++++++++++++++++++- migration/migration.c | 22 +++++++++++++++++----- migration/savevm.c | 2 +- qemu-options.hx | 16 ++++++++++++++-- system/globals.c | 1 - system/vl.c | 13 ++++++++++++- target/s390x/cpu_models.c | 4 +++- 9 files changed, 75 insertions(+), 13 deletions(-)