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

[Laurent Vivier]

On 16/02/2016 10:09, Greg Kurz wrote:
> On Mon, 15 Feb 2016 15:49:17 +0100
> Laurent Vivier <address@hidden> wrote:
> 
>> On 15/02/2016 13:58, Greg Kurz wrote:
>>> On Mon, 15 Feb 2016 12:23:39 +0100
>>> Laurent Vivier <address@hidden> 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 <address@hidden>
>>>>> Reviewed-by: David Gibson <address@hidden>
>>>>> Signed-off-by: Greg Kurz <address@hidden>
>>>>> ---
>>>>>  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 address@hidden,address@hidden,...]]
>>>>>
>>>>>     
>>>>  
>>>   
>>
>