Re: [Qemu-devel] [PATCH v4] Add machine parameter qemu-kvm-migration for live migrate compatibility with qemu-kvm

[Alex Bligh <alex@alex.org.uk>]

Michael,

I'm about to post a nice simple v5 of this, but there are a couple of
your comments I am NOT addressing:

>> 
>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>> index b72b34e..3c9da23 100644
>> --- a/hw/acpi/piix4.c
>> +++ b/hw/acpi/piix4.c
>> @@ -200,12 +200,26 @@ static const VMStateDescription vmstate_pci_status = {
>>     }
>> };
>> 
>> +static const VMStateDescription vmstate_acpi_compat;
>> +
> 
> don't forward declare things, put them right here.

This is not addressable in v5; in v5 this is a forward declaration of
vmstate_acpi (not _compat). vmstate_acpi references acpi_load_old,
and acpi_load_old references vmstate_acpi, so we need a forward
reference for one of them in any case.

>> +        qemu_opt_get_bool(qemu_get_machine_opts(), "qemu-kvm-migration",
>> +                          DEFAULT_QEMU_KVM_MIGRATION)) {
>> +        return vmstate_load_state(f, &vmstate_acpi_compat,
>> +                                  opaque, version_id);
>> +    }
> 
> else if version_id == 2 return -EINVAL?

This is incorrect I think. A version_id of 2 with qemu-kvm_migration
off is permissible, and indicates an inbound migration from qemu-git
1.2, i.e. the old manual loading should be run.

>> /* qemu-kvm 1.2 uses version 3 but advertised as 2
>> - * To support incoming qemu-kvm 1.2 migration, change version_id
>> - * and minimum_version_id to 2 below (which breaks migration from
>> + * To support incoming qemu-kvm 1.2 migration, we support
>> + * via a command line option a change to minimum_version_id
>> + * of 2 in a _compat structure (which breaks migration from
>>  * qemu 1.2).
> 
> Actually it's version 3 that breaks migration right?
> Pls say this explicitly: s/which/version 3 breaks migration from qemu 1.2/

It's changing the minimum version ID to 2 (from 3) which would break
migration from qemu (git) 1.2, because that uses a version ID of 2,
and we want the old loader to run in that case. I've made this clearer.

-- 
Alex Bligh