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,

>> +static const VMStateDescription vmstate_acpi_compat = {
>> +    .name = "piix4_pm",
>> +    .version_id = 3,
>> +    .minimum_version_id = 2,
>> +    .minimum_version_id_old = 1,
>> +    .load_state_old = NULL, /* to avoid recursion */
>> +    .post_load = vmstate_acpi_post_load,
>> +    .fields      = (VMStateField[]) {
>> +        VMSTATE_PCI_DEVICE(parent_obj, PIIX4PMState),
>> +        VMSTATE_UINT16(ar.pm1.evt.sts, PIIX4PMState),
>> +        VMSTATE_UINT16(ar.pm1.evt.en, PIIX4PMState),
>> +        VMSTATE_UINT16(ar.pm1.cnt.cnt, PIIX4PMState),
>> +        VMSTATE_STRUCT(apm, PIIX4PMState, 0, vmstate_apm, APMState),
>> +        VMSTATE_TIMER(ar.tmr.timer, PIIX4PMState),
>> +        VMSTATE_INT64(ar.tmr.overflow_time, PIIX4PMState),
>> +        VMSTATE_STRUCT(ar.gpe, PIIX4PMState, 2, vmstate_gpe, ACPIGPE),
>> +        VMSTATE_STRUCT_TEST(
>> +            acpi_pci_hotplug.acpi_pcihp_pci_status[ACPI_PCIHP_BSEL_DEFAULT],
>> +            PIIX4PMState,
>> +            vmstate_test_no_use_acpi_pci_hotplug,
>> +            2, vmstate_pci_status,
>> +            struct AcpiPciHpPciStatus),
>> +        VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState,
>> +                            vmstate_test_use_acpi_pci_hotplug),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
> 
> Please don't duplicate code like this.
> What is the difference here? Is it just .minimum_version_id?
> Why not just update it in vmstate_acpi?

That and the change to load_state_old.

I thought I had tried that, but that it got memcpy'd somewhere deep
in QOM I think (as part of the inheritance). If the structure
never gets used again (for e.g. export?) I suppose I could
patch it live in acpi_load_old - is that what you meant?

I'd also have to remove the 'const' tag, which I seem to remember
was non-trivial.

Perhaps I could make a copy and change the fields.

>> +/* NB cirrus-vga default value is 8MB anyway, save if we
>> + * monkey patch it to change the default when the qemu-kvm-migration
>> + * machine parameter is selected
>> + */
>> +
> 
> This is too hacky for my taste.
> How about creating a new machine e.g. pc-qemu-kvm-1.0 and in
> pc_early_init_pci_1_0, changing compat_props for pc-1.0 to point to the
> compat_props of pc-qemu-kvm-1.0?

Hang on a second! v2 of this patch DID use a new virtual machine,
called exactly that. I thought you were objecting to that and
wanting a machine parameter instead! It's far easier with a new
machine type, and I'd far prefer a new machine type.

If you were just objecting to the fact that pc-1.0 was made to
be an alias of either one or the other at compile time, simply
drop the second patch of the v2 patchset.

If we have a new machine type, I don't /think/ I need the early_init
thing at all (I may be wrong about that).

-- 
Alex Bligh