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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* 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