Re: [Qemu-devel] [PATCH 1/2] migration: allow configuration section to be optional

[Greg Kurz <gkurz@linux.vnet.ibm.com>]

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}[,...]]
> >>>
> >>>     
> >>  
> >   
>