* [Qemu-devel] [PATCH 0/2] Fix migration of old pseries
@ 2016-02-15 10:14 Greg Kurz
2016-02-15 10:15 ` [Qemu-devel] [PATCH 1/2] migration: allow configuration section to be optional Greg Kurz
2016-02-15 10:15 ` [Qemu-devel] [PATCH 2/2] spapr: fix migration of older pseries Greg Kurz
0 siblings, 2 replies; 13+ messages in thread
From: Greg Kurz @ 2016-02-15 10:14 UTC (permalink / raw)
To: Juan Quintela
Cc: Amit Shah, David Gibson, qemu-ppc, qemu-devel,
Dr. David Alan Gilbert
QEMU 2.4 broke the migration of old pseries machine with the addition
of configuration sections, which are sent unconditionally.
This series fixes the issue by allowing the configuration sections to be
made optional with a machine property.
This approach is an alternative to simply skipping configuration for older
machines (as it done for x86): it allows QEMU to accept incoming migration
of older pseries with or without the configuration section. So it fixes
migration from QEMU 2.3 without breaking QEMU 2.4.
---
Greg Kurz (2):
migration: allow configuration section to be optional
spapr: fix migration of older pseries
hw/core/machine.c | 21 +++++++++++++++++++++
hw/ppc/spapr.c | 1 +
include/hw/boards.h | 1 +
migration/savevm.c | 21 +++++++++++++++------
qemu-options.hx | 3 ++-
5 files changed, 40 insertions(+), 7 deletions(-)
^ permalink raw reply [flat|nested] 13+ messages in thread* [Qemu-devel] [PATCH 1/2] migration: allow configuration section to be optional 2016-02-15 10:14 [Qemu-devel] [PATCH 0/2] Fix migration of old pseries Greg Kurz @ 2016-02-15 10:15 ` Greg Kurz 2016-02-15 11:23 ` Laurent Vivier 2016-02-16 17:01 ` Dr. David Alan Gilbert 2016-02-15 10:15 ` [Qemu-devel] [PATCH 2/2] spapr: fix migration of older pseries Greg Kurz 1 sibling, 2 replies; 13+ messages in thread From: Greg Kurz @ 2016-02-15 10:15 UTC (permalink / raw) To: Juan Quintela Cc: Amit Shah, David Gibson, qemu-ppc, qemu-devel, Dr. David Alan Gilbert Since QEMU 2.4, the migration stream begins with a configuration section. It is known to break migration of pseries machine from older QEMU. It is possible to fix this in the pseries compat code but it will then break migration of old pseries from latest QEMU. As an alternative, this patch introduces a new machine property which allows to ignore the abscence of configuration section during incoming migration. It boils to adding: -machine config-section=off Using this property only makes sense when migrating from an older QEMU. It has no effect on outgoing migration. Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> --- hw/core/machine.c | 21 +++++++++++++++++++++ include/hw/boards.h | 1 + migration/savevm.c | 21 +++++++++++++++------ qemu-options.hx | 3 ++- 4 files changed, 39 insertions(+), 7 deletions(-) diff --git a/hw/core/machine.c b/hw/core/machine.c index 6d1a0d8eebc4..4a7322988fb5 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -312,6 +312,20 @@ static bool machine_get_suppress_vmdesc(Object *obj, Error **errp) return ms->suppress_vmdesc; } +static void machine_set_config_section(Object *obj, bool value, Error **errp) +{ + MachineState *ms = MACHINE(obj); + + ms->config_section = value; +} + +static bool machine_get_config_section(Object *obj, Error **errp) +{ + MachineState *ms = MACHINE(obj); + + return ms->config_section; +} + static int error_on_sysbus_device(SysBusDevice *sbdev, void *opaque) { error_report("Option '-device %s' cannot be handled by this machine", @@ -365,6 +379,7 @@ static void machine_initfn(Object *obj) ms->kvm_shadow_mem = -1; ms->dump_guest_core = true; ms->mem_merge = true; + ms->config_section = true; object_property_add_str(obj, "accel", machine_get_accel, machine_set_accel, NULL); @@ -467,6 +482,12 @@ static void machine_initfn(Object *obj) object_property_set_description(obj, "suppress-vmdesc", "Set on to disable self-describing migration", NULL); + object_property_add_bool(obj, "config-section", + machine_get_config_section, + machine_set_config_section, NULL); + object_property_set_description(obj, "config-section", + "Set on/off to accept migration with/without configuration section", + NULL); /* Register notifier when init is done for sysbus sanity checks */ ms->sysbus_notifier.notify = machine_init_notify; diff --git a/include/hw/boards.h b/include/hw/boards.h index 0f30959e2e3b..853bb5905ec1 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -128,6 +128,7 @@ struct MachineState { char *firmware; bool iommu; bool suppress_vmdesc; + bool config_section; ram_addr_t ram_size; ram_addr_t maxram_size; diff --git a/migration/savevm.c b/migration/savevm.c index 94f2894243ce..3795489aeaec 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -1847,6 +1847,12 @@ static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis) return 0; } +static bool must_receive_configuration(void) +{ + MachineState *machine = MACHINE(qdev_get_machine()); + return machine->config_section; +} + int qemu_loadvm_state(QEMUFile *f) { MigrationIncomingState *mis = migration_incoming_get_current(); @@ -1876,15 +1882,18 @@ int qemu_loadvm_state(QEMUFile *f) } if (!savevm_state.skip_configuration) { - if (qemu_get_byte(f) != QEMU_VM_CONFIGURATION) { + if (qemu_peek_byte(f, 0) == QEMU_VM_CONFIGURATION) { + qemu_file_skip(f, 1); + ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, + 0); + + if (ret) { + return ret; + } + } else if (must_receive_configuration()) { error_report("Configuration section missing"); return -EINVAL; } - ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0); - - if (ret) { - return ret; - } } ret = qemu_loadvm_state_main(f, mis); diff --git a/qemu-options.hx b/qemu-options.hx index 2f0465eeb1d1..10cd64dc266b 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -43,7 +43,8 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \ " aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n" " dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n" " suppress-vmdesc=on|off disables self-describing migration (default=off)\n" - " nvdimm=on|off controls NVDIMM support (default=off)\n", + " nvdimm=on|off controls NVDIMM support (default=off)\n" + " config-section=on|off migration requires configuration section (default=on)\n", QEMU_ARCH_ALL) STEXI @item -machine [type=]@var{name}[,prop=@var{value}[,...]] ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] migration: allow configuration section to be optional 2016-02-15 10:15 ` [Qemu-devel] [PATCH 1/2] migration: allow configuration section to be optional Greg Kurz @ 2016-02-15 11:23 ` Laurent Vivier 2016-02-15 12:58 ` Greg Kurz 2016-02-16 17:01 ` Dr. David Alan Gilbert 1 sibling, 1 reply; 13+ messages in thread From: Laurent Vivier @ 2016-02-15 11:23 UTC (permalink / raw) To: Greg Kurz, Juan Quintela Cc: Amit Shah, qemu-ppc, qemu-devel, Dr. David Alan Gilbert, David Gibson On 15/02/2016 11:15, Greg Kurz wrote: > Since QEMU 2.4, the migration stream begins with a configuration section. > It is known to break migration of pseries machine from older QEMU. It is > possible to fix this in the pseries compat code but it will then break > migration of old pseries from latest QEMU. > > As an alternative, this patch introduces a new machine property which > allows to ignore the abscence of configuration section during incoming > migration. It boils to adding: > > -machine config-section=off > > Using this property only makes sense when migrating from an older > QEMU. It has no effect on outgoing migration. I think this is not true: migrating a pseries-2.3 machine from a qemu-2.4 to a qemu-2.3 must not send the configuration section because the qemu-2.3 will not be able to decode it. 2016-02-15T11:22:08.645978Z qemu-system-ppc64: Unknown savevm section type 7 2016-02-15T11:22:08.646041Z qemu-system-ppc64: load of migration failed: Invalid argument [This is the result without your patch] Laurent > > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > --- > hw/core/machine.c | 21 +++++++++++++++++++++ > include/hw/boards.h | 1 + > migration/savevm.c | 21 +++++++++++++++------ > qemu-options.hx | 3 ++- > 4 files changed, 39 insertions(+), 7 deletions(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index 6d1a0d8eebc4..4a7322988fb5 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -312,6 +312,20 @@ static bool machine_get_suppress_vmdesc(Object *obj, Error **errp) > return ms->suppress_vmdesc; > } > > +static void machine_set_config_section(Object *obj, bool value, Error **errp) > +{ > + MachineState *ms = MACHINE(obj); > + > + ms->config_section = value; > +} > + > +static bool machine_get_config_section(Object *obj, Error **errp) > +{ > + MachineState *ms = MACHINE(obj); > + > + return ms->config_section; > +} > + > static int error_on_sysbus_device(SysBusDevice *sbdev, void *opaque) > { > error_report("Option '-device %s' cannot be handled by this machine", > @@ -365,6 +379,7 @@ static void machine_initfn(Object *obj) > ms->kvm_shadow_mem = -1; > ms->dump_guest_core = true; > ms->mem_merge = true; > + ms->config_section = true; > > object_property_add_str(obj, "accel", > machine_get_accel, machine_set_accel, NULL); > @@ -467,6 +482,12 @@ static void machine_initfn(Object *obj) > object_property_set_description(obj, "suppress-vmdesc", > "Set on to disable self-describing migration", > NULL); > + object_property_add_bool(obj, "config-section", > + machine_get_config_section, > + machine_set_config_section, NULL); > + object_property_set_description(obj, "config-section", > + "Set on/off to accept migration with/without configuration section", > + NULL); > > /* Register notifier when init is done for sysbus sanity checks */ > ms->sysbus_notifier.notify = machine_init_notify; > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 0f30959e2e3b..853bb5905ec1 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -128,6 +128,7 @@ struct MachineState { > char *firmware; > bool iommu; > bool suppress_vmdesc; > + bool config_section; > > ram_addr_t ram_size; > ram_addr_t maxram_size; > diff --git a/migration/savevm.c b/migration/savevm.c > index 94f2894243ce..3795489aeaec 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -1847,6 +1847,12 @@ static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis) > return 0; > } > > +static bool must_receive_configuration(void) > +{ > + MachineState *machine = MACHINE(qdev_get_machine()); > + return machine->config_section; > +} > + > int qemu_loadvm_state(QEMUFile *f) > { > MigrationIncomingState *mis = migration_incoming_get_current(); > @@ -1876,15 +1882,18 @@ int qemu_loadvm_state(QEMUFile *f) > } > > if (!savevm_state.skip_configuration) { > - if (qemu_get_byte(f) != QEMU_VM_CONFIGURATION) { > + if (qemu_peek_byte(f, 0) == QEMU_VM_CONFIGURATION) { > + qemu_file_skip(f, 1); > + ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, > + 0); > + > + if (ret) { > + return ret; > + } > + } else if (must_receive_configuration()) { > error_report("Configuration section missing"); > return -EINVAL; > } > - ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0); > - > - if (ret) { > - return ret; > - } > } > > ret = qemu_loadvm_state_main(f, mis); > diff --git a/qemu-options.hx b/qemu-options.hx > index 2f0465eeb1d1..10cd64dc266b 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -43,7 +43,8 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \ > " aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n" > " dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n" > " suppress-vmdesc=on|off disables self-describing migration (default=off)\n" > - " nvdimm=on|off controls NVDIMM support (default=off)\n", > + " nvdimm=on|off controls NVDIMM support (default=off)\n" > + " config-section=on|off migration requires configuration section (default=on)\n", > QEMU_ARCH_ALL) > STEXI > @item -machine [type=]@var{name}[,prop=@var{value}[,...]] > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] migration: allow configuration section to be optional 2016-02-15 11:23 ` Laurent Vivier @ 2016-02-15 12:58 ` Greg Kurz 2016-02-15 14:49 ` Laurent Vivier 0 siblings, 1 reply; 13+ messages in thread From: Greg Kurz @ 2016-02-15 12:58 UTC (permalink / raw) To: Laurent Vivier Cc: Juan Quintela, qemu-devel, Dr. David Alan Gilbert, qemu-ppc, Amit Shah, David Gibson On Mon, 15 Feb 2016 12:23:39 +0100 Laurent Vivier <lvivier@redhat.com> wrote: > On 15/02/2016 11:15, Greg Kurz wrote: > > Since QEMU 2.4, the migration stream begins with a configuration section. > > It is known to break migration of pseries machine from older QEMU. It is > > possible to fix this in the pseries compat code but it will then break > > migration of old pseries from latest QEMU. > > > > As an alternative, this patch introduces a new machine property which > > allows to ignore the abscence of configuration section during incoming > > migration. It boils to adding: > > > > -machine config-section=off > > > > Using this property only makes sense when migrating from an older > > QEMU. It has no effect on outgoing migration. > > I think this is not true: migrating a pseries-2.3 machine from a > qemu-2.4 to a qemu-2.3 must not send the configuration section because > the qemu-2.3 will not be able to decode it. > I did not mind to also fix backward compatibility... is it mandatory ? > 2016-02-15T11:22:08.645978Z qemu-system-ppc64: Unknown savevm section type 7 > 2016-02-15T11:22:08.646041Z qemu-system-ppc64: load of migration failed: > Invalid argument > > [This is the result without your patch] > > Laurent > > > > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > > --- > > hw/core/machine.c | 21 +++++++++++++++++++++ > > include/hw/boards.h | 1 + > > migration/savevm.c | 21 +++++++++++++++------ > > qemu-options.hx | 3 ++- > > 4 files changed, 39 insertions(+), 7 deletions(-) > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c > > index 6d1a0d8eebc4..4a7322988fb5 100644 > > --- a/hw/core/machine.c > > +++ b/hw/core/machine.c > > @@ -312,6 +312,20 @@ static bool machine_get_suppress_vmdesc(Object *obj, Error **errp) > > return ms->suppress_vmdesc; > > } > > > > +static void machine_set_config_section(Object *obj, bool value, Error **errp) > > +{ > > + MachineState *ms = MACHINE(obj); > > + > > + ms->config_section = value; > > +} > > + > > +static bool machine_get_config_section(Object *obj, Error **errp) > > +{ > > + MachineState *ms = MACHINE(obj); > > + > > + return ms->config_section; > > +} > > + > > static int error_on_sysbus_device(SysBusDevice *sbdev, void *opaque) > > { > > error_report("Option '-device %s' cannot be handled by this machine", > > @@ -365,6 +379,7 @@ static void machine_initfn(Object *obj) > > ms->kvm_shadow_mem = -1; > > ms->dump_guest_core = true; > > ms->mem_merge = true; > > + ms->config_section = true; > > > > object_property_add_str(obj, "accel", > > machine_get_accel, machine_set_accel, NULL); > > @@ -467,6 +482,12 @@ static void machine_initfn(Object *obj) > > object_property_set_description(obj, "suppress-vmdesc", > > "Set on to disable self-describing migration", > > NULL); > > + object_property_add_bool(obj, "config-section", > > + machine_get_config_section, > > + machine_set_config_section, NULL); > > + object_property_set_description(obj, "config-section", > > + "Set on/off to accept migration with/without configuration section", > > + NULL); > > > > /* Register notifier when init is done for sysbus sanity checks */ > > ms->sysbus_notifier.notify = machine_init_notify; > > diff --git a/include/hw/boards.h b/include/hw/boards.h > > index 0f30959e2e3b..853bb5905ec1 100644 > > --- a/include/hw/boards.h > > +++ b/include/hw/boards.h > > @@ -128,6 +128,7 @@ struct MachineState { > > char *firmware; > > bool iommu; > > bool suppress_vmdesc; > > + bool config_section; > > > > ram_addr_t ram_size; > > ram_addr_t maxram_size; > > diff --git a/migration/savevm.c b/migration/savevm.c > > index 94f2894243ce..3795489aeaec 100644 > > --- a/migration/savevm.c > > +++ b/migration/savevm.c > > @@ -1847,6 +1847,12 @@ static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis) > > return 0; > > } > > > > +static bool must_receive_configuration(void) > > +{ > > + MachineState *machine = MACHINE(qdev_get_machine()); > > + return machine->config_section; > > +} > > + > > int qemu_loadvm_state(QEMUFile *f) > > { > > MigrationIncomingState *mis = migration_incoming_get_current(); > > @@ -1876,15 +1882,18 @@ int qemu_loadvm_state(QEMUFile *f) > > } > > > > if (!savevm_state.skip_configuration) { > > - if (qemu_get_byte(f) != QEMU_VM_CONFIGURATION) { > > + if (qemu_peek_byte(f, 0) == QEMU_VM_CONFIGURATION) { > > + qemu_file_skip(f, 1); > > + ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, > > + 0); > > + > > + if (ret) { > > + return ret; > > + } > > + } else if (must_receive_configuration()) { > > error_report("Configuration section missing"); > > return -EINVAL; > > } > > - ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0); > > - > > - if (ret) { > > - return ret; > > - } > > } > > > > ret = qemu_loadvm_state_main(f, mis); > > diff --git a/qemu-options.hx b/qemu-options.hx > > index 2f0465eeb1d1..10cd64dc266b 100644 > > --- a/qemu-options.hx > > +++ b/qemu-options.hx > > @@ -43,7 +43,8 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \ > > " aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n" > > " dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n" > > " suppress-vmdesc=on|off disables self-describing migration (default=off)\n" > > - " nvdimm=on|off controls NVDIMM support (default=off)\n", > > + " nvdimm=on|off controls NVDIMM support (default=off)\n" > > + " config-section=on|off migration requires configuration section (default=on)\n", > > QEMU_ARCH_ALL) > > STEXI > > @item -machine [type=]@var{name}[,prop=@var{value}[,...]] > > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] migration: allow configuration section to be optional 2016-02-15 12:58 ` Greg Kurz @ 2016-02-15 14:49 ` Laurent Vivier 2016-02-16 9:09 ` Greg Kurz 0 siblings, 1 reply; 13+ messages in thread From: Laurent Vivier @ 2016-02-15 14:49 UTC (permalink / raw) To: Greg Kurz Cc: Juan Quintela, qemu-devel, Dr. David Alan Gilbert, qemu-ppc, Amit Shah, David Gibson On 15/02/2016 13:58, Greg Kurz wrote: > On Mon, 15 Feb 2016 12:23:39 +0100 > Laurent Vivier <lvivier@redhat.com> wrote: > >> On 15/02/2016 11:15, Greg Kurz wrote: >>> Since QEMU 2.4, the migration stream begins with a configuration section. >>> It is known to break migration of pseries machine from older QEMU. It is >>> possible to fix this in the pseries compat code but it will then break >>> migration of old pseries from latest QEMU. >>> >>> As an alternative, this patch introduces a new machine property which >>> allows to ignore the abscence of configuration section during incoming >>> migration. It boils to adding: >>> >>> -machine config-section=off >>> >>> Using this property only makes sense when migrating from an older >>> QEMU. It has no effect on outgoing migration. >> >> I think this is not true: migrating a pseries-2.3 machine from a >> qemu-2.4 to a qemu-2.3 must not send the configuration section because >> the qemu-2.3 will not be able to decode it. >> > > I did not mind to also fix backward compatibility... is it mandatory ? I don't know, but as a user I would like to be able to do that (and it is possible in the other cases). And I think the fix could be smaller: something like just setting "savevm_state.skip_configuration" to the value of "config-section". [I don't know if it is as simple as it looks...] Laurent > >> 2016-02-15T11:22:08.645978Z qemu-system-ppc64: Unknown savevm section type 7 >> 2016-02-15T11:22:08.646041Z qemu-system-ppc64: load of migration failed: >> Invalid argument >> >> [This is the result without your patch] >> >> Laurent >>> >>> Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> >>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> >>> --- >>> hw/core/machine.c | 21 +++++++++++++++++++++ >>> include/hw/boards.h | 1 + >>> migration/savevm.c | 21 +++++++++++++++------ >>> qemu-options.hx | 3 ++- >>> 4 files changed, 39 insertions(+), 7 deletions(-) >>> >>> diff --git a/hw/core/machine.c b/hw/core/machine.c >>> index 6d1a0d8eebc4..4a7322988fb5 100644 >>> --- a/hw/core/machine.c >>> +++ b/hw/core/machine.c >>> @@ -312,6 +312,20 @@ static bool machine_get_suppress_vmdesc(Object *obj, Error **errp) >>> return ms->suppress_vmdesc; >>> } >>> >>> +static void machine_set_config_section(Object *obj, bool value, Error **errp) >>> +{ >>> + MachineState *ms = MACHINE(obj); >>> + >>> + ms->config_section = value; >>> +} >>> + >>> +static bool machine_get_config_section(Object *obj, Error **errp) >>> +{ >>> + MachineState *ms = MACHINE(obj); >>> + >>> + return ms->config_section; >>> +} >>> + >>> static int error_on_sysbus_device(SysBusDevice *sbdev, void *opaque) >>> { >>> error_report("Option '-device %s' cannot be handled by this machine", >>> @@ -365,6 +379,7 @@ static void machine_initfn(Object *obj) >>> ms->kvm_shadow_mem = -1; >>> ms->dump_guest_core = true; >>> ms->mem_merge = true; >>> + ms->config_section = true; >>> >>> object_property_add_str(obj, "accel", >>> machine_get_accel, machine_set_accel, NULL); >>> @@ -467,6 +482,12 @@ static void machine_initfn(Object *obj) >>> object_property_set_description(obj, "suppress-vmdesc", >>> "Set on to disable self-describing migration", >>> NULL); >>> + object_property_add_bool(obj, "config-section", >>> + machine_get_config_section, >>> + machine_set_config_section, NULL); >>> + object_property_set_description(obj, "config-section", >>> + "Set on/off to accept migration with/without configuration section", >>> + NULL); >>> >>> /* Register notifier when init is done for sysbus sanity checks */ >>> ms->sysbus_notifier.notify = machine_init_notify; >>> diff --git a/include/hw/boards.h b/include/hw/boards.h >>> index 0f30959e2e3b..853bb5905ec1 100644 >>> --- a/include/hw/boards.h >>> +++ b/include/hw/boards.h >>> @@ -128,6 +128,7 @@ struct MachineState { >>> char *firmware; >>> bool iommu; >>> bool suppress_vmdesc; >>> + bool config_section; >>> >>> ram_addr_t ram_size; >>> ram_addr_t maxram_size; >>> diff --git a/migration/savevm.c b/migration/savevm.c >>> index 94f2894243ce..3795489aeaec 100644 >>> --- a/migration/savevm.c >>> +++ b/migration/savevm.c >>> @@ -1847,6 +1847,12 @@ static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis) >>> return 0; >>> } >>> >>> +static bool must_receive_configuration(void) >>> +{ >>> + MachineState *machine = MACHINE(qdev_get_machine()); >>> + return machine->config_section; >>> +} >>> + >>> int qemu_loadvm_state(QEMUFile *f) >>> { >>> MigrationIncomingState *mis = migration_incoming_get_current(); >>> @@ -1876,15 +1882,18 @@ int qemu_loadvm_state(QEMUFile *f) >>> } >>> >>> if (!savevm_state.skip_configuration) { >>> - if (qemu_get_byte(f) != QEMU_VM_CONFIGURATION) { >>> + if (qemu_peek_byte(f, 0) == QEMU_VM_CONFIGURATION) { >>> + qemu_file_skip(f, 1); >>> + ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, >>> + 0); >>> + >>> + if (ret) { >>> + return ret; >>> + } >>> + } else if (must_receive_configuration()) { >>> error_report("Configuration section missing"); >>> return -EINVAL; >>> } >>> - ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0); >>> - >>> - if (ret) { >>> - return ret; >>> - } >>> } >>> >>> ret = qemu_loadvm_state_main(f, mis); >>> diff --git a/qemu-options.hx b/qemu-options.hx >>> index 2f0465eeb1d1..10cd64dc266b 100644 >>> --- a/qemu-options.hx >>> +++ b/qemu-options.hx >>> @@ -43,7 +43,8 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \ >>> " aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n" >>> " dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n" >>> " suppress-vmdesc=on|off disables self-describing migration (default=off)\n" >>> - " nvdimm=on|off controls NVDIMM support (default=off)\n", >>> + " nvdimm=on|off controls NVDIMM support (default=off)\n" >>> + " config-section=on|off migration requires configuration section (default=on)\n", >>> QEMU_ARCH_ALL) >>> STEXI >>> @item -machine [type=]@var{name}[,prop=@var{value}[,...]] >>> >>> >> > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] migration: allow configuration section to be optional 2016-02-15 14:49 ` Laurent Vivier @ 2016-02-16 9:09 ` Greg Kurz 2016-02-16 9:31 ` Laurent Vivier 0 siblings, 1 reply; 13+ messages in thread From: Greg Kurz @ 2016-02-16 9:09 UTC (permalink / raw) To: Laurent Vivier Cc: Juan Quintela, qemu-devel, Dr. David Alan Gilbert, qemu-ppc, Amit Shah, David Gibson On Mon, 15 Feb 2016 15:49:17 +0100 Laurent Vivier <lvivier@redhat.com> wrote: > On 15/02/2016 13:58, Greg Kurz wrote: > > On Mon, 15 Feb 2016 12:23:39 +0100 > > Laurent Vivier <lvivier@redhat.com> wrote: > > > >> On 15/02/2016 11:15, Greg Kurz wrote: > >>> Since QEMU 2.4, the migration stream begins with a configuration section. > >>> It is known to break migration of pseries machine from older QEMU. It is > >>> possible to fix this in the pseries compat code but it will then break > >>> migration of old pseries from latest QEMU. > >>> > >>> As an alternative, this patch introduces a new machine property which > >>> allows to ignore the abscence of configuration section during incoming > >>> migration. It boils to adding: > >>> > >>> -machine config-section=off > >>> > >>> Using this property only makes sense when migrating from an older > >>> QEMU. It has no effect on outgoing migration. > >> > >> I think this is not true: migrating a pseries-2.3 machine from a > >> qemu-2.4 to a qemu-2.3 must not send the configuration section because > >> the qemu-2.3 will not be able to decode it. > >> > > > > I did not mind to also fix backward compatibility... is it mandatory ? > > I don't know, but as a user I would like to be able to do that (and it > is possible in the other cases). > I fully agree backward migration is cool and we should not break it if possible. The situation is bit different here: QEMU 2.4 broke migration both ways for pseries-2.3 => all QEMU versions that were released since then (2.4/2.4.1/2.5) are ultimately incompatible with QEMU 2.3 and this cannot be fixed. What we may try to achieve is that QEMU 2.6 can accept incoming migration of pseries-2.3 from QEMU 2.3 (without configuration section) and from QEMU 2.4/2.5 (with configuration section). This is what this series does, without the need for the user to actually pass config-section=off thanks to patch 2. > And I think the fix could be smaller: something like just setting > "savevm_state.skip_configuration" to the value of "config-section". > [I don't know if it is as simple as it looks...] > Not so simple is the word indeed... if we do this, a pseries-2.3 machine started with config-section=off can only be migrated back to QEMU 2.3 since QEMU 2.4/2.4.1/2.5 want the config section... is this what you want ? If yes, then an even simpler fix is: https://lists.nongnu.org/archive/html/qemu-devel/2016-02/msg01738.html We can either fix forward migration for all QEMU versions with this series, or fix backward migration to QEMU-2.3. Not both. What is the most important ? > Laurent > > > >> 2016-02-15T11:22:08.645978Z qemu-system-ppc64: Unknown savevm section type 7 > >> 2016-02-15T11:22:08.646041Z qemu-system-ppc64: load of migration failed: > >> Invalid argument > >> > >> [This is the result without your patch] > >> > >> Laurent > >>> > >>> Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > >>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > >>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > >>> --- > >>> hw/core/machine.c | 21 +++++++++++++++++++++ > >>> include/hw/boards.h | 1 + > >>> migration/savevm.c | 21 +++++++++++++++------ > >>> qemu-options.hx | 3 ++- > >>> 4 files changed, 39 insertions(+), 7 deletions(-) > >>> > >>> diff --git a/hw/core/machine.c b/hw/core/machine.c > >>> index 6d1a0d8eebc4..4a7322988fb5 100644 > >>> --- a/hw/core/machine.c > >>> +++ b/hw/core/machine.c > >>> @@ -312,6 +312,20 @@ static bool machine_get_suppress_vmdesc(Object *obj, Error **errp) > >>> return ms->suppress_vmdesc; > >>> } > >>> > >>> +static void machine_set_config_section(Object *obj, bool value, Error **errp) > >>> +{ > >>> + MachineState *ms = MACHINE(obj); > >>> + > >>> + ms->config_section = value; > >>> +} > >>> + > >>> +static bool machine_get_config_section(Object *obj, Error **errp) > >>> +{ > >>> + MachineState *ms = MACHINE(obj); > >>> + > >>> + return ms->config_section; > >>> +} > >>> + > >>> static int error_on_sysbus_device(SysBusDevice *sbdev, void *opaque) > >>> { > >>> error_report("Option '-device %s' cannot be handled by this machine", > >>> @@ -365,6 +379,7 @@ static void machine_initfn(Object *obj) > >>> ms->kvm_shadow_mem = -1; > >>> ms->dump_guest_core = true; > >>> ms->mem_merge = true; > >>> + ms->config_section = true; > >>> > >>> object_property_add_str(obj, "accel", > >>> machine_get_accel, machine_set_accel, NULL); > >>> @@ -467,6 +482,12 @@ static void machine_initfn(Object *obj) > >>> object_property_set_description(obj, "suppress-vmdesc", > >>> "Set on to disable self-describing migration", > >>> NULL); > >>> + object_property_add_bool(obj, "config-section", > >>> + machine_get_config_section, > >>> + machine_set_config_section, NULL); > >>> + object_property_set_description(obj, "config-section", > >>> + "Set on/off to accept migration with/without configuration section", > >>> + NULL); > >>> > >>> /* Register notifier when init is done for sysbus sanity checks */ > >>> ms->sysbus_notifier.notify = machine_init_notify; > >>> diff --git a/include/hw/boards.h b/include/hw/boards.h > >>> index 0f30959e2e3b..853bb5905ec1 100644 > >>> --- a/include/hw/boards.h > >>> +++ b/include/hw/boards.h > >>> @@ -128,6 +128,7 @@ struct MachineState { > >>> char *firmware; > >>> bool iommu; > >>> bool suppress_vmdesc; > >>> + bool config_section; > >>> > >>> ram_addr_t ram_size; > >>> ram_addr_t maxram_size; > >>> diff --git a/migration/savevm.c b/migration/savevm.c > >>> index 94f2894243ce..3795489aeaec 100644 > >>> --- a/migration/savevm.c > >>> +++ b/migration/savevm.c > >>> @@ -1847,6 +1847,12 @@ static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis) > >>> return 0; > >>> } > >>> > >>> +static bool must_receive_configuration(void) > >>> +{ > >>> + MachineState *machine = MACHINE(qdev_get_machine()); > >>> + return machine->config_section; > >>> +} > >>> + > >>> int qemu_loadvm_state(QEMUFile *f) > >>> { > >>> MigrationIncomingState *mis = migration_incoming_get_current(); > >>> @@ -1876,15 +1882,18 @@ int qemu_loadvm_state(QEMUFile *f) > >>> } > >>> > >>> if (!savevm_state.skip_configuration) { > >>> - if (qemu_get_byte(f) != QEMU_VM_CONFIGURATION) { > >>> + if (qemu_peek_byte(f, 0) == QEMU_VM_CONFIGURATION) { > >>> + qemu_file_skip(f, 1); > >>> + ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, > >>> + 0); > >>> + > >>> + if (ret) { > >>> + return ret; > >>> + } > >>> + } else if (must_receive_configuration()) { > >>> error_report("Configuration section missing"); > >>> return -EINVAL; > >>> } > >>> - ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0); > >>> - > >>> - if (ret) { > >>> - return ret; > >>> - } > >>> } > >>> > >>> ret = qemu_loadvm_state_main(f, mis); > >>> diff --git a/qemu-options.hx b/qemu-options.hx > >>> index 2f0465eeb1d1..10cd64dc266b 100644 > >>> --- a/qemu-options.hx > >>> +++ b/qemu-options.hx > >>> @@ -43,7 +43,8 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \ > >>> " aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n" > >>> " dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n" > >>> " suppress-vmdesc=on|off disables self-describing migration (default=off)\n" > >>> - " nvdimm=on|off controls NVDIMM support (default=off)\n", > >>> + " nvdimm=on|off controls NVDIMM support (default=off)\n" > >>> + " config-section=on|off migration requires configuration section (default=on)\n", > >>> QEMU_ARCH_ALL) > >>> STEXI > >>> @item -machine [type=]@var{name}[,prop=@var{value}[,...]] > >>> > >>> > >> > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] migration: allow configuration section to be optional 2016-02-16 9:09 ` Greg Kurz @ 2016-02-16 9:31 ` Laurent Vivier 2016-02-16 10:39 ` Greg Kurz 2016-02-16 17:09 ` Dr. David Alan Gilbert 0 siblings, 2 replies; 13+ messages in thread From: Laurent Vivier @ 2016-02-16 9:31 UTC (permalink / raw) To: Greg Kurz Cc: Juan Quintela, qemu-devel, Dr. David Alan Gilbert, qemu-ppc, Amit Shah, David Gibson On 16/02/2016 10:09, Greg Kurz wrote: > On Mon, 15 Feb 2016 15:49:17 +0100 > Laurent Vivier <lvivier@redhat.com> wrote: > >> On 15/02/2016 13:58, Greg Kurz wrote: >>> On Mon, 15 Feb 2016 12:23:39 +0100 >>> Laurent Vivier <lvivier@redhat.com> wrote: >>> >>>> On 15/02/2016 11:15, Greg Kurz wrote: >>>>> Since QEMU 2.4, the migration stream begins with a configuration section. >>>>> It is known to break migration of pseries machine from older QEMU. It is >>>>> possible to fix this in the pseries compat code but it will then break >>>>> migration of old pseries from latest QEMU. >>>>> >>>>> As an alternative, this patch introduces a new machine property which >>>>> allows to ignore the abscence of configuration section during incoming >>>>> migration. It boils to adding: >>>>> >>>>> -machine config-section=off >>>>> >>>>> Using this property only makes sense when migrating from an older >>>>> QEMU. It has no effect on outgoing migration. >>>> >>>> I think this is not true: migrating a pseries-2.3 machine from a >>>> qemu-2.4 to a qemu-2.3 must not send the configuration section because >>>> the qemu-2.3 will not be able to decode it. >>>> >>> >>> I did not mind to also fix backward compatibility... is it mandatory ? >> >> I don't know, but as a user I would like to be able to do that (and it >> is possible in the other cases). >> > > I fully agree backward migration is cool and we should not break it if > possible. > > The situation is bit different here: QEMU 2.4 broke migration both ways for > pseries-2.3 => all QEMU versions that were released since then (2.4/2.4.1/2.5) > are ultimately incompatible with QEMU 2.3 and this cannot be fixed. > > What we may try to achieve is that QEMU 2.6 can accept incoming migration > of pseries-2.3 from QEMU 2.3 (without configuration section) and from > QEMU 2.4/2.5 (with configuration section). > > This is what this series does, without the need for the user to actually pass > config-section=off thanks to patch 2. > >> And I think the fix could be smaller: something like just setting >> "savevm_state.skip_configuration" to the value of "config-section". >> [I don't know if it is as simple as it looks...] >> > > Not so simple is the word indeed... if we do this, a pseries-2.3 machine > started with config-section=off can only be migrated back to QEMU 2.3 since > QEMU 2.4/2.4.1/2.5 want the config section... is this what you want ? If yes, > then an even simpler fix is: > > https://lists.nongnu.org/archive/html/qemu-devel/2016-02/msg01738.html > > We can either fix forward migration for all QEMU versions with this series, > or fix backward migration to QEMU-2.3. Not both. > > What is the most important ? Yes, I agree with you, but if you pass the parameter from the QEMU monitor you can pass it when you want to migrate the machine (if needed). In fact, my opinion is not really important here, but we need the one from David Gilbert or Juan. >> Laurent >>> >>>> 2016-02-15T11:22:08.645978Z qemu-system-ppc64: Unknown savevm section type 7 >>>> 2016-02-15T11:22:08.646041Z qemu-system-ppc64: load of migration failed: >>>> Invalid argument >>>> >>>> [This is the result without your patch] >>>> >>>> Laurent >>>>> >>>>> Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >>>>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> >>>>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> >>>>> --- >>>>> hw/core/machine.c | 21 +++++++++++++++++++++ >>>>> include/hw/boards.h | 1 + >>>>> migration/savevm.c | 21 +++++++++++++++------ >>>>> qemu-options.hx | 3 ++- >>>>> 4 files changed, 39 insertions(+), 7 deletions(-) >>>>> >>>>> diff --git a/hw/core/machine.c b/hw/core/machine.c >>>>> index 6d1a0d8eebc4..4a7322988fb5 100644 >>>>> --- a/hw/core/machine.c >>>>> +++ b/hw/core/machine.c >>>>> @@ -312,6 +312,20 @@ static bool machine_get_suppress_vmdesc(Object *obj, Error **errp) >>>>> return ms->suppress_vmdesc; >>>>> } >>>>> >>>>> +static void machine_set_config_section(Object *obj, bool value, Error **errp) >>>>> +{ >>>>> + MachineState *ms = MACHINE(obj); >>>>> + >>>>> + ms->config_section = value; >>>>> +} >>>>> + >>>>> +static bool machine_get_config_section(Object *obj, Error **errp) >>>>> +{ >>>>> + MachineState *ms = MACHINE(obj); >>>>> + >>>>> + return ms->config_section; >>>>> +} >>>>> + >>>>> static int error_on_sysbus_device(SysBusDevice *sbdev, void *opaque) >>>>> { >>>>> error_report("Option '-device %s' cannot be handled by this machine", >>>>> @@ -365,6 +379,7 @@ static void machine_initfn(Object *obj) >>>>> ms->kvm_shadow_mem = -1; >>>>> ms->dump_guest_core = true; >>>>> ms->mem_merge = true; >>>>> + ms->config_section = true; >>>>> >>>>> object_property_add_str(obj, "accel", >>>>> machine_get_accel, machine_set_accel, NULL); >>>>> @@ -467,6 +482,12 @@ static void machine_initfn(Object *obj) >>>>> object_property_set_description(obj, "suppress-vmdesc", >>>>> "Set on to disable self-describing migration", >>>>> NULL); >>>>> + object_property_add_bool(obj, "config-section", >>>>> + machine_get_config_section, >>>>> + machine_set_config_section, NULL); >>>>> + object_property_set_description(obj, "config-section", >>>>> + "Set on/off to accept migration with/without configuration section", >>>>> + NULL); >>>>> >>>>> /* Register notifier when init is done for sysbus sanity checks */ >>>>> ms->sysbus_notifier.notify = machine_init_notify; >>>>> diff --git a/include/hw/boards.h b/include/hw/boards.h >>>>> index 0f30959e2e3b..853bb5905ec1 100644 >>>>> --- a/include/hw/boards.h >>>>> +++ b/include/hw/boards.h >>>>> @@ -128,6 +128,7 @@ struct MachineState { >>>>> char *firmware; >>>>> bool iommu; >>>>> bool suppress_vmdesc; >>>>> + bool config_section; >>>>> >>>>> ram_addr_t ram_size; >>>>> ram_addr_t maxram_size; >>>>> diff --git a/migration/savevm.c b/migration/savevm.c >>>>> index 94f2894243ce..3795489aeaec 100644 >>>>> --- a/migration/savevm.c >>>>> +++ b/migration/savevm.c >>>>> @@ -1847,6 +1847,12 @@ static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis) >>>>> return 0; >>>>> } >>>>> >>>>> +static bool must_receive_configuration(void) >>>>> +{ >>>>> + MachineState *machine = MACHINE(qdev_get_machine()); >>>>> + return machine->config_section; >>>>> +} >>>>> + >>>>> int qemu_loadvm_state(QEMUFile *f) >>>>> { >>>>> MigrationIncomingState *mis = migration_incoming_get_current(); >>>>> @@ -1876,15 +1882,18 @@ int qemu_loadvm_state(QEMUFile *f) >>>>> } >>>>> >>>>> if (!savevm_state.skip_configuration) { >>>>> - if (qemu_get_byte(f) != QEMU_VM_CONFIGURATION) { >>>>> + if (qemu_peek_byte(f, 0) == QEMU_VM_CONFIGURATION) { >>>>> + qemu_file_skip(f, 1); >>>>> + ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, >>>>> + 0); >>>>> + >>>>> + if (ret) { >>>>> + return ret; >>>>> + } >>>>> + } else if (must_receive_configuration()) { >>>>> error_report("Configuration section missing"); >>>>> return -EINVAL; >>>>> } >>>>> - ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0); >>>>> - >>>>> - if (ret) { >>>>> - return ret; >>>>> - } >>>>> } >>>>> >>>>> ret = qemu_loadvm_state_main(f, mis); >>>>> diff --git a/qemu-options.hx b/qemu-options.hx >>>>> index 2f0465eeb1d1..10cd64dc266b 100644 >>>>> --- a/qemu-options.hx >>>>> +++ b/qemu-options.hx >>>>> @@ -43,7 +43,8 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \ >>>>> " aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n" >>>>> " dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n" >>>>> " suppress-vmdesc=on|off disables self-describing migration (default=off)\n" >>>>> - " nvdimm=on|off controls NVDIMM support (default=off)\n", >>>>> + " nvdimm=on|off controls NVDIMM support (default=off)\n" >>>>> + " config-section=on|off migration requires configuration section (default=on)\n", >>>>> QEMU_ARCH_ALL) >>>>> STEXI >>>>> @item -machine [type=]@var{name}[,prop=@var{value}[,...]] >>>>> >>>>> >>>> >>> >> > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] migration: allow configuration section to be optional 2016-02-16 9:31 ` Laurent Vivier @ 2016-02-16 10:39 ` Greg Kurz 2016-02-16 17:09 ` Dr. David Alan Gilbert 1 sibling, 0 replies; 13+ messages in thread From: Greg Kurz @ 2016-02-16 10:39 UTC (permalink / raw) To: Laurent Vivier Cc: Juan Quintela, qemu-devel, Dr. David Alan Gilbert, qemu-ppc, Amit Shah, David Gibson On Tue, 16 Feb 2016 10:31:35 +0100 Laurent Vivier <lvivier@redhat.com> wrote: > On 16/02/2016 10:09, Greg Kurz wrote: > > On Mon, 15 Feb 2016 15:49:17 +0100 > > Laurent Vivier <lvivier@redhat.com> wrote: > > > >> On 15/02/2016 13:58, Greg Kurz wrote: > >>> On Mon, 15 Feb 2016 12:23:39 +0100 > >>> Laurent Vivier <lvivier@redhat.com> wrote: > >>> > >>>> On 15/02/2016 11:15, Greg Kurz wrote: > >>>>> Since QEMU 2.4, the migration stream begins with a configuration section. > >>>>> It is known to break migration of pseries machine from older QEMU. It is > >>>>> possible to fix this in the pseries compat code but it will then break > >>>>> migration of old pseries from latest QEMU. > >>>>> > >>>>> As an alternative, this patch introduces a new machine property which > >>>>> allows to ignore the abscence of configuration section during incoming > >>>>> migration. It boils to adding: > >>>>> > >>>>> -machine config-section=off > >>>>> > >>>>> Using this property only makes sense when migrating from an older > >>>>> QEMU. It has no effect on outgoing migration. > >>>> > >>>> I think this is not true: migrating a pseries-2.3 machine from a > >>>> qemu-2.4 to a qemu-2.3 must not send the configuration section because > >>>> the qemu-2.3 will not be able to decode it. > >>>> > >>> > >>> I did not mind to also fix backward compatibility... is it mandatory ? > >> > >> I don't know, but as a user I would like to be able to do that (and it > >> is possible in the other cases). > >> > > > > I fully agree backward migration is cool and we should not break it if > > possible. > > > > The situation is bit different here: QEMU 2.4 broke migration both ways for > > pseries-2.3 => all QEMU versions that were released since then (2.4/2.4.1/2.5) > > are ultimately incompatible with QEMU 2.3 and this cannot be fixed. > > > > What we may try to achieve is that QEMU 2.6 can accept incoming migration > > of pseries-2.3 from QEMU 2.3 (without configuration section) and from > > QEMU 2.4/2.5 (with configuration section). > > > > This is what this series does, without the need for the user to actually pass > > config-section=off thanks to patch 2. > > > >> And I think the fix could be smaller: something like just setting > >> "savevm_state.skip_configuration" to the value of "config-section". > >> [I don't know if it is as simple as it looks...] > >> > > > > Not so simple is the word indeed... if we do this, a pseries-2.3 machine > > started with config-section=off can only be migrated back to QEMU 2.3 since > > QEMU 2.4/2.4.1/2.5 want the config section... is this what you want ? If yes, > > then an even simpler fix is: > > > > https://lists.nongnu.org/archive/html/qemu-devel/2016-02/msg01738.html > > > > We can either fix forward migration for all QEMU versions with this series, > > or fix backward migration to QEMU-2.3. Not both. > > > > What is the most important ? > > Yes, I agree with you, but if you pass the parameter from the QEMU > monitor you can pass it when you want to migrate the machine (if needed). > Indeed... maybe it is acceptable when the user knows about the QEMU version on the target host. Is it something that could be handled by libvirt ? > In fact, my opinion is not really important here, but we need the one > from David Gilbert or Juan. > Yeah definitely. And FWIW, this patch was suggested by Dave Gilbert: https://lists.nongnu.org/archive/html/qemu-devel/2016-02/msg02639.html On the other hand, Dave Gibson agrees to break compatibility with QEMU 2.4/2.5 if we assume pseries-2.3 are mostly QEMU 2.3 based: https://lists.nongnu.org/archive/html/qemu-devel/2016-02/msg02963.html And your suggestion which should allow migration to/from all previous releases, at the cost of some extra complexity for users. I cannot decide which is less bad... > >> Laurent > >>> > >>>> 2016-02-15T11:22:08.645978Z qemu-system-ppc64: Unknown savevm section type 7 > >>>> 2016-02-15T11:22:08.646041Z qemu-system-ppc64: load of migration failed: > >>>> Invalid argument > >>>> > >>>> [This is the result without your patch] > >>>> > >>>> Laurent > >>>>> > >>>>> Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > >>>>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > >>>>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > >>>>> --- > >>>>> hw/core/machine.c | 21 +++++++++++++++++++++ > >>>>> include/hw/boards.h | 1 + > >>>>> migration/savevm.c | 21 +++++++++++++++------ > >>>>> qemu-options.hx | 3 ++- > >>>>> 4 files changed, 39 insertions(+), 7 deletions(-) > >>>>> > >>>>> diff --git a/hw/core/machine.c b/hw/core/machine.c > >>>>> index 6d1a0d8eebc4..4a7322988fb5 100644 > >>>>> --- a/hw/core/machine.c > >>>>> +++ b/hw/core/machine.c > >>>>> @@ -312,6 +312,20 @@ static bool machine_get_suppress_vmdesc(Object *obj, Error **errp) > >>>>> return ms->suppress_vmdesc; > >>>>> } > >>>>> > >>>>> +static void machine_set_config_section(Object *obj, bool value, Error **errp) > >>>>> +{ > >>>>> + MachineState *ms = MACHINE(obj); > >>>>> + > >>>>> + ms->config_section = value; > >>>>> +} > >>>>> + > >>>>> +static bool machine_get_config_section(Object *obj, Error **errp) > >>>>> +{ > >>>>> + MachineState *ms = MACHINE(obj); > >>>>> + > >>>>> + return ms->config_section; > >>>>> +} > >>>>> + > >>>>> static int error_on_sysbus_device(SysBusDevice *sbdev, void *opaque) > >>>>> { > >>>>> error_report("Option '-device %s' cannot be handled by this machine", > >>>>> @@ -365,6 +379,7 @@ static void machine_initfn(Object *obj) > >>>>> ms->kvm_shadow_mem = -1; > >>>>> ms->dump_guest_core = true; > >>>>> ms->mem_merge = true; > >>>>> + ms->config_section = true; > >>>>> > >>>>> object_property_add_str(obj, "accel", > >>>>> machine_get_accel, machine_set_accel, NULL); > >>>>> @@ -467,6 +482,12 @@ static void machine_initfn(Object *obj) > >>>>> object_property_set_description(obj, "suppress-vmdesc", > >>>>> "Set on to disable self-describing migration", > >>>>> NULL); > >>>>> + object_property_add_bool(obj, "config-section", > >>>>> + machine_get_config_section, > >>>>> + machine_set_config_section, NULL); > >>>>> + object_property_set_description(obj, "config-section", > >>>>> + "Set on/off to accept migration with/without configuration section", > >>>>> + NULL); > >>>>> > >>>>> /* Register notifier when init is done for sysbus sanity checks */ > >>>>> ms->sysbus_notifier.notify = machine_init_notify; > >>>>> diff --git a/include/hw/boards.h b/include/hw/boards.h > >>>>> index 0f30959e2e3b..853bb5905ec1 100644 > >>>>> --- a/include/hw/boards.h > >>>>> +++ b/include/hw/boards.h > >>>>> @@ -128,6 +128,7 @@ struct MachineState { > >>>>> char *firmware; > >>>>> bool iommu; > >>>>> bool suppress_vmdesc; > >>>>> + bool config_section; > >>>>> > >>>>> ram_addr_t ram_size; > >>>>> ram_addr_t maxram_size; > >>>>> diff --git a/migration/savevm.c b/migration/savevm.c > >>>>> index 94f2894243ce..3795489aeaec 100644 > >>>>> --- a/migration/savevm.c > >>>>> +++ b/migration/savevm.c > >>>>> @@ -1847,6 +1847,12 @@ static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis) > >>>>> return 0; > >>>>> } > >>>>> > >>>>> +static bool must_receive_configuration(void) > >>>>> +{ > >>>>> + MachineState *machine = MACHINE(qdev_get_machine()); > >>>>> + return machine->config_section; > >>>>> +} > >>>>> + > >>>>> int qemu_loadvm_state(QEMUFile *f) > >>>>> { > >>>>> MigrationIncomingState *mis = migration_incoming_get_current(); > >>>>> @@ -1876,15 +1882,18 @@ int qemu_loadvm_state(QEMUFile *f) > >>>>> } > >>>>> > >>>>> if (!savevm_state.skip_configuration) { > >>>>> - if (qemu_get_byte(f) != QEMU_VM_CONFIGURATION) { > >>>>> + if (qemu_peek_byte(f, 0) == QEMU_VM_CONFIGURATION) { > >>>>> + qemu_file_skip(f, 1); > >>>>> + ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, > >>>>> + 0); > >>>>> + > >>>>> + if (ret) { > >>>>> + return ret; > >>>>> + } > >>>>> + } else if (must_receive_configuration()) { > >>>>> error_report("Configuration section missing"); > >>>>> return -EINVAL; > >>>>> } > >>>>> - ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0); > >>>>> - > >>>>> - if (ret) { > >>>>> - return ret; > >>>>> - } > >>>>> } > >>>>> > >>>>> ret = qemu_loadvm_state_main(f, mis); > >>>>> diff --git a/qemu-options.hx b/qemu-options.hx > >>>>> index 2f0465eeb1d1..10cd64dc266b 100644 > >>>>> --- a/qemu-options.hx > >>>>> +++ b/qemu-options.hx > >>>>> @@ -43,7 +43,8 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \ > >>>>> " aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n" > >>>>> " dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n" > >>>>> " suppress-vmdesc=on|off disables self-describing migration (default=off)\n" > >>>>> - " nvdimm=on|off controls NVDIMM support (default=off)\n", > >>>>> + " nvdimm=on|off controls NVDIMM support (default=off)\n" > >>>>> + " config-section=on|off migration requires configuration section (default=on)\n", > >>>>> QEMU_ARCH_ALL) > >>>>> STEXI > >>>>> @item -machine [type=]@var{name}[,prop=@var{value}[,...]] > >>>>> > >>>>> > >>>> > >>> > >> > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] migration: allow configuration section to be optional 2016-02-16 9:31 ` Laurent Vivier 2016-02-16 10:39 ` Greg Kurz @ 2016-02-16 17:09 ` Dr. David Alan Gilbert 2016-02-17 9:29 ` Greg Kurz 1 sibling, 1 reply; 13+ messages in thread From: Dr. David Alan Gilbert @ 2016-02-16 17:09 UTC (permalink / raw) To: Laurent Vivier Cc: Juan Quintela, qemu-devel, qemu-ppc, Amit Shah, Greg Kurz, David Gibson * Laurent Vivier (lvivier@redhat.com) wrote: > > > On 16/02/2016 10:09, Greg Kurz wrote: > > On Mon, 15 Feb 2016 15:49:17 +0100 > > Laurent Vivier <lvivier@redhat.com> wrote: > > > >> On 15/02/2016 13:58, Greg Kurz wrote: > >>> On Mon, 15 Feb 2016 12:23:39 +0100 > >>> Laurent Vivier <lvivier@redhat.com> wrote: > >>> > >>>> On 15/02/2016 11:15, Greg Kurz wrote: > >>>>> Since QEMU 2.4, the migration stream begins with a configuration section. > >>>>> It is known to break migration of pseries machine from older QEMU. It is > >>>>> possible to fix this in the pseries compat code but it will then break > >>>>> migration of old pseries from latest QEMU. > >>>>> > >>>>> As an alternative, this patch introduces a new machine property which > >>>>> allows to ignore the abscence of configuration section during incoming > >>>>> migration. It boils to adding: > >>>>> > >>>>> -machine config-section=off > >>>>> > >>>>> Using this property only makes sense when migrating from an older > >>>>> QEMU. It has no effect on outgoing migration. > >>>> > >>>> I think this is not true: migrating a pseries-2.3 machine from a > >>>> qemu-2.4 to a qemu-2.3 must not send the configuration section because > >>>> the qemu-2.3 will not be able to decode it. > >>>> > >>> > >>> I did not mind to also fix backward compatibility... is it mandatory ? > >> > >> I don't know, but as a user I would like to be able to do that (and it > >> is possible in the other cases). > >> > > > > I fully agree backward migration is cool and we should not break it if > > possible. > > > > The situation is bit different here: QEMU 2.4 broke migration both ways for > > pseries-2.3 => all QEMU versions that were released since then (2.4/2.4.1/2.5) > > are ultimately incompatible with QEMU 2.3 and this cannot be fixed. > > > > What we may try to achieve is that QEMU 2.6 can accept incoming migration > > of pseries-2.3 from QEMU 2.3 (without configuration section) and from > > QEMU 2.4/2.5 (with configuration section). > > > > This is what this series does, without the need for the user to actually pass > > config-section=off thanks to patch 2. > > > >> And I think the fix could be smaller: something like just setting > >> "savevm_state.skip_configuration" to the value of "config-section". > >> [I don't know if it is as simple as it looks...] > >> > > > > Not so simple is the word indeed... if we do this, a pseries-2.3 machine > > started with config-section=off can only be migrated back to QEMU 2.3 since > > QEMU 2.4/2.4.1/2.5 want the config section... is this what you want ? If yes, > > then an even simpler fix is: > > > > https://lists.nongnu.org/archive/html/qemu-devel/2016-02/msg01738.html > > > > We can either fix forward migration for all QEMU versions with this series, > > or fix backward migration to QEMU-2.3. Not both. > > > > What is the most important ? > > Yes, I agree with you, but if you pass the parameter from the QEMU > monitor you can pass it when you want to migrate the machine (if needed). > > In fact, my opinion is not really important here, but we need the one > from David Gilbert or Juan. I dont think we do anything like this at the moment, however if you actually did make this machine property influence whether you sent a configuration (opposite to my previous reply!), then I think you could do: qom-set /machine config-section off before migration. However, you would have to teach all the tooling to do that, and I don't think we've got anything that would do it at the moment. Dave > > >> Laurent > >>> > >>>> 2016-02-15T11:22:08.645978Z qemu-system-ppc64: Unknown savevm section type 7 > >>>> 2016-02-15T11:22:08.646041Z qemu-system-ppc64: load of migration failed: > >>>> Invalid argument > >>>> > >>>> [This is the result without your patch] > >>>> > >>>> Laurent > >>>>> > >>>>> Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > >>>>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > >>>>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > >>>>> --- > >>>>> hw/core/machine.c | 21 +++++++++++++++++++++ > >>>>> include/hw/boards.h | 1 + > >>>>> migration/savevm.c | 21 +++++++++++++++------ > >>>>> qemu-options.hx | 3 ++- > >>>>> 4 files changed, 39 insertions(+), 7 deletions(-) > >>>>> > >>>>> diff --git a/hw/core/machine.c b/hw/core/machine.c > >>>>> index 6d1a0d8eebc4..4a7322988fb5 100644 > >>>>> --- a/hw/core/machine.c > >>>>> +++ b/hw/core/machine.c > >>>>> @@ -312,6 +312,20 @@ static bool machine_get_suppress_vmdesc(Object *obj, Error **errp) > >>>>> return ms->suppress_vmdesc; > >>>>> } > >>>>> > >>>>> +static void machine_set_config_section(Object *obj, bool value, Error **errp) > >>>>> +{ > >>>>> + MachineState *ms = MACHINE(obj); > >>>>> + > >>>>> + ms->config_section = value; > >>>>> +} > >>>>> + > >>>>> +static bool machine_get_config_section(Object *obj, Error **errp) > >>>>> +{ > >>>>> + MachineState *ms = MACHINE(obj); > >>>>> + > >>>>> + return ms->config_section; > >>>>> +} > >>>>> + > >>>>> static int error_on_sysbus_device(SysBusDevice *sbdev, void *opaque) > >>>>> { > >>>>> error_report("Option '-device %s' cannot be handled by this machine", > >>>>> @@ -365,6 +379,7 @@ static void machine_initfn(Object *obj) > >>>>> ms->kvm_shadow_mem = -1; > >>>>> ms->dump_guest_core = true; > >>>>> ms->mem_merge = true; > >>>>> + ms->config_section = true; > >>>>> > >>>>> object_property_add_str(obj, "accel", > >>>>> machine_get_accel, machine_set_accel, NULL); > >>>>> @@ -467,6 +482,12 @@ static void machine_initfn(Object *obj) > >>>>> object_property_set_description(obj, "suppress-vmdesc", > >>>>> "Set on to disable self-describing migration", > >>>>> NULL); > >>>>> + object_property_add_bool(obj, "config-section", > >>>>> + machine_get_config_section, > >>>>> + machine_set_config_section, NULL); > >>>>> + object_property_set_description(obj, "config-section", > >>>>> + "Set on/off to accept migration with/without configuration section", > >>>>> + NULL); > >>>>> > >>>>> /* Register notifier when init is done for sysbus sanity checks */ > >>>>> ms->sysbus_notifier.notify = machine_init_notify; > >>>>> diff --git a/include/hw/boards.h b/include/hw/boards.h > >>>>> index 0f30959e2e3b..853bb5905ec1 100644 > >>>>> --- a/include/hw/boards.h > >>>>> +++ b/include/hw/boards.h > >>>>> @@ -128,6 +128,7 @@ struct MachineState { > >>>>> char *firmware; > >>>>> bool iommu; > >>>>> bool suppress_vmdesc; > >>>>> + bool config_section; > >>>>> > >>>>> ram_addr_t ram_size; > >>>>> ram_addr_t maxram_size; > >>>>> diff --git a/migration/savevm.c b/migration/savevm.c > >>>>> index 94f2894243ce..3795489aeaec 100644 > >>>>> --- a/migration/savevm.c > >>>>> +++ b/migration/savevm.c > >>>>> @@ -1847,6 +1847,12 @@ static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis) > >>>>> return 0; > >>>>> } > >>>>> > >>>>> +static bool must_receive_configuration(void) > >>>>> +{ > >>>>> + MachineState *machine = MACHINE(qdev_get_machine()); > >>>>> + return machine->config_section; > >>>>> +} > >>>>> + > >>>>> int qemu_loadvm_state(QEMUFile *f) > >>>>> { > >>>>> MigrationIncomingState *mis = migration_incoming_get_current(); > >>>>> @@ -1876,15 +1882,18 @@ int qemu_loadvm_state(QEMUFile *f) > >>>>> } > >>>>> > >>>>> if (!savevm_state.skip_configuration) { > >>>>> - if (qemu_get_byte(f) != QEMU_VM_CONFIGURATION) { > >>>>> + if (qemu_peek_byte(f, 0) == QEMU_VM_CONFIGURATION) { > >>>>> + qemu_file_skip(f, 1); > >>>>> + ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, > >>>>> + 0); > >>>>> + > >>>>> + if (ret) { > >>>>> + return ret; > >>>>> + } > >>>>> + } else if (must_receive_configuration()) { > >>>>> error_report("Configuration section missing"); > >>>>> return -EINVAL; > >>>>> } > >>>>> - ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0); > >>>>> - > >>>>> - if (ret) { > >>>>> - return ret; > >>>>> - } > >>>>> } > >>>>> > >>>>> ret = qemu_loadvm_state_main(f, mis); > >>>>> diff --git a/qemu-options.hx b/qemu-options.hx > >>>>> index 2f0465eeb1d1..10cd64dc266b 100644 > >>>>> --- a/qemu-options.hx > >>>>> +++ b/qemu-options.hx > >>>>> @@ -43,7 +43,8 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \ > >>>>> " aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n" > >>>>> " dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n" > >>>>> " suppress-vmdesc=on|off disables self-describing migration (default=off)\n" > >>>>> - " nvdimm=on|off controls NVDIMM support (default=off)\n", > >>>>> + " nvdimm=on|off controls NVDIMM support (default=off)\n" > >>>>> + " config-section=on|off migration requires configuration section (default=on)\n", > >>>>> QEMU_ARCH_ALL) > >>>>> STEXI > >>>>> @item -machine [type=]@var{name}[,prop=@var{value}[,...]] > >>>>> > >>>>> > >>>> > >>> > >> > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] migration: allow configuration section to be optional 2016-02-16 17:09 ` Dr. David Alan Gilbert @ 2016-02-17 9:29 ` Greg Kurz 0 siblings, 0 replies; 13+ messages in thread From: Greg Kurz @ 2016-02-17 9:29 UTC (permalink / raw) To: Dr. David Alan Gilbert Cc: Laurent Vivier, Juan Quintela, qemu-devel, qemu-ppc, Amit Shah, David Gibson On Tue, 16 Feb 2016 17:09:52 +0000 "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * Laurent Vivier (lvivier@redhat.com) wrote: > > > > > > On 16/02/2016 10:09, Greg Kurz wrote: > > > On Mon, 15 Feb 2016 15:49:17 +0100 > > > Laurent Vivier <lvivier@redhat.com> wrote: > > > > > >> On 15/02/2016 13:58, Greg Kurz wrote: > > >>> On Mon, 15 Feb 2016 12:23:39 +0100 > > >>> Laurent Vivier <lvivier@redhat.com> wrote: > > >>> > > >>>> On 15/02/2016 11:15, Greg Kurz wrote: > > >>>>> Since QEMU 2.4, the migration stream begins with a configuration section. > > >>>>> It is known to break migration of pseries machine from older QEMU. It is > > >>>>> possible to fix this in the pseries compat code but it will then break > > >>>>> migration of old pseries from latest QEMU. > > >>>>> > > >>>>> As an alternative, this patch introduces a new machine property which > > >>>>> allows to ignore the abscence of configuration section during incoming > > >>>>> migration. It boils to adding: > > >>>>> > > >>>>> -machine config-section=off > > >>>>> > > >>>>> Using this property only makes sense when migrating from an older > > >>>>> QEMU. It has no effect on outgoing migration. > > >>>> > > >>>> I think this is not true: migrating a pseries-2.3 machine from a > > >>>> qemu-2.4 to a qemu-2.3 must not send the configuration section because > > >>>> the qemu-2.3 will not be able to decode it. > > >>>> > > >>> > > >>> I did not mind to also fix backward compatibility... is it mandatory ? > > >> > > >> I don't know, but as a user I would like to be able to do that (and it > > >> is possible in the other cases). > > >> > > > > > > I fully agree backward migration is cool and we should not break it if > > > possible. > > > > > > The situation is bit different here: QEMU 2.4 broke migration both ways for > > > pseries-2.3 => all QEMU versions that were released since then (2.4/2.4.1/2.5) > > > are ultimately incompatible with QEMU 2.3 and this cannot be fixed. > > > > > > What we may try to achieve is that QEMU 2.6 can accept incoming migration > > > of pseries-2.3 from QEMU 2.3 (without configuration section) and from > > > QEMU 2.4/2.5 (with configuration section). > > > > > > This is what this series does, without the need for the user to actually pass > > > config-section=off thanks to patch 2. > > > > > >> And I think the fix could be smaller: something like just setting > > >> "savevm_state.skip_configuration" to the value of "config-section". > > >> [I don't know if it is as simple as it looks...] > > >> > > > > > > Not so simple is the word indeed... if we do this, a pseries-2.3 machine > > > started with config-section=off can only be migrated back to QEMU 2.3 since > > > QEMU 2.4/2.4.1/2.5 want the config section... is this what you want ? If yes, > > > then an even simpler fix is: > > > > > > https://lists.nongnu.org/archive/html/qemu-devel/2016-02/msg01738.html > > > > > > We can either fix forward migration for all QEMU versions with this series, > > > or fix backward migration to QEMU-2.3. Not both. > > > > > > What is the most important ? > > > > Yes, I agree with you, but if you pass the parameter from the QEMU > > monitor you can pass it when you want to migrate the machine (if needed). > > > > In fact, my opinion is not really important here, but we need the one > > from David Gilbert or Juan. > > I dont think we do anything like this at the moment, however if you actually > did make this machine property influence whether you sent a configuration > (opposite to my previous reply!), then I think you could do: > > qom-set /machine config-section off > > before migration. > Yes, I guess that's what Laurent was suggesting in his previous mail. Does this call for another machine option to be able to not send the configuration section (the (a) case in your other mail) ? > However, you would have to teach all the tooling to do that, and I don't > think we've got anything that would do it at the moment. > Especially the tooling should know about the target QEMU to decide if a configuration section must be sent or not... > Dave > Thanks. -- Greg > > > > >> Laurent > > >>> > > >>>> 2016-02-15T11:22:08.645978Z qemu-system-ppc64: Unknown savevm section type 7 > > >>>> 2016-02-15T11:22:08.646041Z qemu-system-ppc64: load of migration failed: > > >>>> Invalid argument > > >>>> > > >>>> [This is the result without your patch] > > >>>> > > >>>> Laurent > > >>>>> > > >>>>> Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > >>>>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > > >>>>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > > >>>>> --- > > >>>>> hw/core/machine.c | 21 +++++++++++++++++++++ > > >>>>> include/hw/boards.h | 1 + > > >>>>> migration/savevm.c | 21 +++++++++++++++------ > > >>>>> qemu-options.hx | 3 ++- > > >>>>> 4 files changed, 39 insertions(+), 7 deletions(-) > > >>>>> > > >>>>> diff --git a/hw/core/machine.c b/hw/core/machine.c > > >>>>> index 6d1a0d8eebc4..4a7322988fb5 100644 > > >>>>> --- a/hw/core/machine.c > > >>>>> +++ b/hw/core/machine.c > > >>>>> @@ -312,6 +312,20 @@ static bool machine_get_suppress_vmdesc(Object *obj, Error **errp) > > >>>>> return ms->suppress_vmdesc; > > >>>>> } > > >>>>> > > >>>>> +static void machine_set_config_section(Object *obj, bool value, Error **errp) > > >>>>> +{ > > >>>>> + MachineState *ms = MACHINE(obj); > > >>>>> + > > >>>>> + ms->config_section = value; > > >>>>> +} > > >>>>> + > > >>>>> +static bool machine_get_config_section(Object *obj, Error **errp) > > >>>>> +{ > > >>>>> + MachineState *ms = MACHINE(obj); > > >>>>> + > > >>>>> + return ms->config_section; > > >>>>> +} > > >>>>> + > > >>>>> static int error_on_sysbus_device(SysBusDevice *sbdev, void *opaque) > > >>>>> { > > >>>>> error_report("Option '-device %s' cannot be handled by this machine", > > >>>>> @@ -365,6 +379,7 @@ static void machine_initfn(Object *obj) > > >>>>> ms->kvm_shadow_mem = -1; > > >>>>> ms->dump_guest_core = true; > > >>>>> ms->mem_merge = true; > > >>>>> + ms->config_section = true; > > >>>>> > > >>>>> object_property_add_str(obj, "accel", > > >>>>> machine_get_accel, machine_set_accel, NULL); > > >>>>> @@ -467,6 +482,12 @@ static void machine_initfn(Object *obj) > > >>>>> object_property_set_description(obj, "suppress-vmdesc", > > >>>>> "Set on to disable self-describing migration", > > >>>>> NULL); > > >>>>> + object_property_add_bool(obj, "config-section", > > >>>>> + machine_get_config_section, > > >>>>> + machine_set_config_section, NULL); > > >>>>> + object_property_set_description(obj, "config-section", > > >>>>> + "Set on/off to accept migration with/without configuration section", > > >>>>> + NULL); > > >>>>> > > >>>>> /* Register notifier when init is done for sysbus sanity checks */ > > >>>>> ms->sysbus_notifier.notify = machine_init_notify; > > >>>>> diff --git a/include/hw/boards.h b/include/hw/boards.h > > >>>>> index 0f30959e2e3b..853bb5905ec1 100644 > > >>>>> --- a/include/hw/boards.h > > >>>>> +++ b/include/hw/boards.h > > >>>>> @@ -128,6 +128,7 @@ struct MachineState { > > >>>>> char *firmware; > > >>>>> bool iommu; > > >>>>> bool suppress_vmdesc; > > >>>>> + bool config_section; > > >>>>> > > >>>>> ram_addr_t ram_size; > > >>>>> ram_addr_t maxram_size; > > >>>>> diff --git a/migration/savevm.c b/migration/savevm.c > > >>>>> index 94f2894243ce..3795489aeaec 100644 > > >>>>> --- a/migration/savevm.c > > >>>>> +++ b/migration/savevm.c > > >>>>> @@ -1847,6 +1847,12 @@ static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis) > > >>>>> return 0; > > >>>>> } > > >>>>> > > >>>>> +static bool must_receive_configuration(void) > > >>>>> +{ > > >>>>> + MachineState *machine = MACHINE(qdev_get_machine()); > > >>>>> + return machine->config_section; > > >>>>> +} > > >>>>> + > > >>>>> int qemu_loadvm_state(QEMUFile *f) > > >>>>> { > > >>>>> MigrationIncomingState *mis = migration_incoming_get_current(); > > >>>>> @@ -1876,15 +1882,18 @@ int qemu_loadvm_state(QEMUFile *f) > > >>>>> } > > >>>>> > > >>>>> if (!savevm_state.skip_configuration) { > > >>>>> - if (qemu_get_byte(f) != QEMU_VM_CONFIGURATION) { > > >>>>> + if (qemu_peek_byte(f, 0) == QEMU_VM_CONFIGURATION) { > > >>>>> + qemu_file_skip(f, 1); > > >>>>> + ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, > > >>>>> + 0); > > >>>>> + > > >>>>> + if (ret) { > > >>>>> + return ret; > > >>>>> + } > > >>>>> + } else if (must_receive_configuration()) { > > >>>>> error_report("Configuration section missing"); > > >>>>> return -EINVAL; > > >>>>> } > > >>>>> - ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0); > > >>>>> - > > >>>>> - if (ret) { > > >>>>> - return ret; > > >>>>> - } > > >>>>> } > > >>>>> > > >>>>> ret = qemu_loadvm_state_main(f, mis); > > >>>>> diff --git a/qemu-options.hx b/qemu-options.hx > > >>>>> index 2f0465eeb1d1..10cd64dc266b 100644 > > >>>>> --- a/qemu-options.hx > > >>>>> +++ b/qemu-options.hx > > >>>>> @@ -43,7 +43,8 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \ > > >>>>> " aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n" > > >>>>> " dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n" > > >>>>> " suppress-vmdesc=on|off disables self-describing migration (default=off)\n" > > >>>>> - " nvdimm=on|off controls NVDIMM support (default=off)\n", > > >>>>> + " nvdimm=on|off controls NVDIMM support (default=off)\n" > > >>>>> + " config-section=on|off migration requires configuration section (default=on)\n", > > >>>>> QEMU_ARCH_ALL) > > >>>>> STEXI > > >>>>> @item -machine [type=]@var{name}[,prop=@var{value}[,...]] > > >>>>> > > >>>>> > > >>>> > > >>> > > >> > > > > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] migration: allow configuration section to be optional 2016-02-15 10:15 ` [Qemu-devel] [PATCH 1/2] migration: allow configuration section to be optional Greg Kurz 2016-02-15 11:23 ` Laurent Vivier @ 2016-02-16 17:01 ` Dr. David Alan Gilbert 2016-02-16 17:43 ` Greg Kurz 1 sibling, 1 reply; 13+ messages in thread From: Dr. David Alan Gilbert @ 2016-02-16 17:01 UTC (permalink / raw) To: Greg Kurz; +Cc: Amit Shah, David Gibson, qemu-ppc, qemu-devel, Juan Quintela * Greg Kurz (gkurz@linux.vnet.ibm.com) wrote: > Since QEMU 2.4, the migration stream begins with a configuration section. > It is known to break migration of pseries machine from older QEMU. It is > possible to fix this in the pseries compat code but it will then break > migration of old pseries from latest QEMU. > > As an alternative, this patch introduces a new machine property which > allows to ignore the abscence of configuration section during incoming > migration. It boils to adding: > > -machine config-section=off > > Using this property only makes sense when migrating from an older > QEMU. It has no effect on outgoing migration. Hi Greg, So I'm mostly OK with this, but please change it to: 'require-config-section' and the bool similarly. We've effectively got 2 separate things: a) Whether a config section is sent b) Whether a config section is required upon reception I want to make sure the naming makes it obvious which is being changed, and you're only affecting (b). Dave > > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > --- > hw/core/machine.c | 21 +++++++++++++++++++++ > include/hw/boards.h | 1 + > migration/savevm.c | 21 +++++++++++++++------ > qemu-options.hx | 3 ++- > 4 files changed, 39 insertions(+), 7 deletions(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index 6d1a0d8eebc4..4a7322988fb5 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -312,6 +312,20 @@ static bool machine_get_suppress_vmdesc(Object *obj, Error **errp) > return ms->suppress_vmdesc; > } > > +static void machine_set_config_section(Object *obj, bool value, Error **errp) > +{ > + MachineState *ms = MACHINE(obj); > + > + ms->config_section = value; > +} > + > +static bool machine_get_config_section(Object *obj, Error **errp) > +{ > + MachineState *ms = MACHINE(obj); > + > + return ms->config_section; > +} > + > static int error_on_sysbus_device(SysBusDevice *sbdev, void *opaque) > { > error_report("Option '-device %s' cannot be handled by this machine", > @@ -365,6 +379,7 @@ static void machine_initfn(Object *obj) > ms->kvm_shadow_mem = -1; > ms->dump_guest_core = true; > ms->mem_merge = true; > + ms->config_section = true; > > object_property_add_str(obj, "accel", > machine_get_accel, machine_set_accel, NULL); > @@ -467,6 +482,12 @@ static void machine_initfn(Object *obj) > object_property_set_description(obj, "suppress-vmdesc", > "Set on to disable self-describing migration", > NULL); > + object_property_add_bool(obj, "config-section", > + machine_get_config_section, > + machine_set_config_section, NULL); > + object_property_set_description(obj, "config-section", > + "Set on/off to accept migration with/without configuration section", > + NULL); > > /* Register notifier when init is done for sysbus sanity checks */ > ms->sysbus_notifier.notify = machine_init_notify; > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 0f30959e2e3b..853bb5905ec1 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -128,6 +128,7 @@ struct MachineState { > char *firmware; > bool iommu; > bool suppress_vmdesc; > + bool config_section; > > ram_addr_t ram_size; > ram_addr_t maxram_size; > diff --git a/migration/savevm.c b/migration/savevm.c > index 94f2894243ce..3795489aeaec 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -1847,6 +1847,12 @@ static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis) > return 0; > } > > +static bool must_receive_configuration(void) > +{ > + MachineState *machine = MACHINE(qdev_get_machine()); > + return machine->config_section; > +} > + > int qemu_loadvm_state(QEMUFile *f) > { > MigrationIncomingState *mis = migration_incoming_get_current(); > @@ -1876,15 +1882,18 @@ int qemu_loadvm_state(QEMUFile *f) > } > > if (!savevm_state.skip_configuration) { > - if (qemu_get_byte(f) != QEMU_VM_CONFIGURATION) { > + if (qemu_peek_byte(f, 0) == QEMU_VM_CONFIGURATION) { > + qemu_file_skip(f, 1); > + ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, > + 0); > + > + if (ret) { > + return ret; > + } > + } else if (must_receive_configuration()) { > error_report("Configuration section missing"); > return -EINVAL; > } > - ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0); > - > - if (ret) { > - return ret; > - } > } > > ret = qemu_loadvm_state_main(f, mis); > diff --git a/qemu-options.hx b/qemu-options.hx > index 2f0465eeb1d1..10cd64dc266b 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -43,7 +43,8 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \ > " aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n" > " dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n" > " suppress-vmdesc=on|off disables self-describing migration (default=off)\n" > - " nvdimm=on|off controls NVDIMM support (default=off)\n", > + " nvdimm=on|off controls NVDIMM support (default=off)\n" > + " config-section=on|off migration requires configuration section (default=on)\n", > QEMU_ARCH_ALL) > STEXI > @item -machine [type=]@var{name}[,prop=@var{value}[,...]] > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] migration: allow configuration section to be optional 2016-02-16 17:01 ` Dr. David Alan Gilbert @ 2016-02-16 17:43 ` Greg Kurz 0 siblings, 0 replies; 13+ messages in thread From: Greg Kurz @ 2016-02-16 17:43 UTC (permalink / raw) To: Dr. David Alan Gilbert Cc: Amit Shah, David Gibson, qemu-ppc, qemu-devel, Juan Quintela On Tue, 16 Feb 2016 17:01:40 +0000 "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * Greg Kurz (gkurz@linux.vnet.ibm.com) wrote: > > Since QEMU 2.4, the migration stream begins with a configuration section. > > It is known to break migration of pseries machine from older QEMU. It is > > possible to fix this in the pseries compat code but it will then break > > migration of old pseries from latest QEMU. > > > > As an alternative, this patch introduces a new machine property which > > allows to ignore the abscence of configuration section during incoming > > migration. It boils to adding: > > > > -machine config-section=off > > > > Using this property only makes sense when migrating from an older > > QEMU. It has no effect on outgoing migration. > > Hi Greg, > So I'm mostly OK with this, but please change it to: > 'require-config-section' and the bool similarly. > > We've effectively got 2 separate things: > a) Whether a config section is sent > b) Whether a config section is required upon reception > > I want to make sure the naming makes it obvious which is being changed, > and you're only affecting (b). > Sure ! I'll do that. > Dave > > > > > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > > --- > > hw/core/machine.c | 21 +++++++++++++++++++++ > > include/hw/boards.h | 1 + > > migration/savevm.c | 21 +++++++++++++++------ > > qemu-options.hx | 3 ++- > > 4 files changed, 39 insertions(+), 7 deletions(-) > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c > > index 6d1a0d8eebc4..4a7322988fb5 100644 > > --- a/hw/core/machine.c > > +++ b/hw/core/machine.c > > @@ -312,6 +312,20 @@ static bool machine_get_suppress_vmdesc(Object *obj, Error **errp) > > return ms->suppress_vmdesc; > > } > > > > +static void machine_set_config_section(Object *obj, bool value, Error **errp) > > +{ > > + MachineState *ms = MACHINE(obj); > > + > > + ms->config_section = value; > > +} > > + > > +static bool machine_get_config_section(Object *obj, Error **errp) > > +{ > > + MachineState *ms = MACHINE(obj); > > + > > + return ms->config_section; > > +} > > + > > static int error_on_sysbus_device(SysBusDevice *sbdev, void *opaque) > > { > > error_report("Option '-device %s' cannot be handled by this machine", > > @@ -365,6 +379,7 @@ static void machine_initfn(Object *obj) > > ms->kvm_shadow_mem = -1; > > ms->dump_guest_core = true; > > ms->mem_merge = true; > > + ms->config_section = true; > > > > object_property_add_str(obj, "accel", > > machine_get_accel, machine_set_accel, NULL); > > @@ -467,6 +482,12 @@ static void machine_initfn(Object *obj) > > object_property_set_description(obj, "suppress-vmdesc", > > "Set on to disable self-describing migration", > > NULL); > > + object_property_add_bool(obj, "config-section", > > + machine_get_config_section, > > + machine_set_config_section, NULL); > > + object_property_set_description(obj, "config-section", > > + "Set on/off to accept migration with/without configuration section", > > + NULL); > > > > /* Register notifier when init is done for sysbus sanity checks */ > > ms->sysbus_notifier.notify = machine_init_notify; > > diff --git a/include/hw/boards.h b/include/hw/boards.h > > index 0f30959e2e3b..853bb5905ec1 100644 > > --- a/include/hw/boards.h > > +++ b/include/hw/boards.h > > @@ -128,6 +128,7 @@ struct MachineState { > > char *firmware; > > bool iommu; > > bool suppress_vmdesc; > > + bool config_section; > > > > ram_addr_t ram_size; > > ram_addr_t maxram_size; > > diff --git a/migration/savevm.c b/migration/savevm.c > > index 94f2894243ce..3795489aeaec 100644 > > --- a/migration/savevm.c > > +++ b/migration/savevm.c > > @@ -1847,6 +1847,12 @@ static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis) > > return 0; > > } > > > > +static bool must_receive_configuration(void) > > +{ > > + MachineState *machine = MACHINE(qdev_get_machine()); > > + return machine->config_section; > > +} > > + > > int qemu_loadvm_state(QEMUFile *f) > > { > > MigrationIncomingState *mis = migration_incoming_get_current(); > > @@ -1876,15 +1882,18 @@ int qemu_loadvm_state(QEMUFile *f) > > } > > > > if (!savevm_state.skip_configuration) { > > - if (qemu_get_byte(f) != QEMU_VM_CONFIGURATION) { > > + if (qemu_peek_byte(f, 0) == QEMU_VM_CONFIGURATION) { > > + qemu_file_skip(f, 1); > > + ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, > > + 0); > > + > > + if (ret) { > > + return ret; > > + } > > + } else if (must_receive_configuration()) { > > error_report("Configuration section missing"); > > return -EINVAL; > > } > > - ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0); > > - > > - if (ret) { > > - return ret; > > - } > > } > > > > ret = qemu_loadvm_state_main(f, mis); > > diff --git a/qemu-options.hx b/qemu-options.hx > > index 2f0465eeb1d1..10cd64dc266b 100644 > > --- a/qemu-options.hx > > +++ b/qemu-options.hx > > @@ -43,7 +43,8 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \ > > " aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n" > > " dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n" > > " suppress-vmdesc=on|off disables self-describing migration (default=off)\n" > > - " nvdimm=on|off controls NVDIMM support (default=off)\n", > > + " nvdimm=on|off controls NVDIMM support (default=off)\n" > > + " config-section=on|off migration requires configuration section (default=on)\n", > > QEMU_ARCH_ALL) > > STEXI > > @item -machine [type=]@var{name}[,prop=@var{value}[,...]] > > > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH 2/2] spapr: fix migration of older pseries 2016-02-15 10:14 [Qemu-devel] [PATCH 0/2] Fix migration of old pseries Greg Kurz 2016-02-15 10:15 ` [Qemu-devel] [PATCH 1/2] migration: allow configuration section to be optional Greg Kurz @ 2016-02-15 10:15 ` Greg Kurz 1 sibling, 0 replies; 13+ messages in thread From: Greg Kurz @ 2016-02-15 10:15 UTC (permalink / raw) To: Juan Quintela Cc: Amit Shah, David Gibson, qemu-ppc, qemu-devel, Dr. David Alan Gilbert Use the config_section machine property to fix migration of older pseries started with an older QEMU (version < 2.4 for both). Older machines started with QEMU 2.4 are not affected by this change since they send the configuration section unconditionally. Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> --- hw/ppc/spapr.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 5bd8fd3ef842..cd6681acb87b 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -2446,6 +2446,7 @@ static void spapr_machine_2_3_instance_options(MachineState *machine) spapr_machine_2_4_instance_options(machine); savevm_skip_section_footers(); global_state_set_optional(); + machine->config_section = false; } static void spapr_machine_2_3_class_options(MachineClass *mc) ^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-02-17 9:30 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-02-15 10:14 [Qemu-devel] [PATCH 0/2] Fix migration of old pseries Greg Kurz 2016-02-15 10:15 ` [Qemu-devel] [PATCH 1/2] migration: allow configuration section to be optional Greg Kurz 2016-02-15 11:23 ` Laurent Vivier 2016-02-15 12:58 ` Greg Kurz 2016-02-15 14:49 ` Laurent Vivier 2016-02-16 9:09 ` Greg Kurz 2016-02-16 9:31 ` Laurent Vivier 2016-02-16 10:39 ` Greg Kurz 2016-02-16 17:09 ` Dr. David Alan Gilbert 2016-02-17 9:29 ` Greg Kurz 2016-02-16 17:01 ` Dr. David Alan Gilbert 2016-02-16 17:43 ` Greg Kurz 2016-02-15 10:15 ` [Qemu-devel] [PATCH 2/2] spapr: fix migration of older pseries Greg Kurz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).