On Mon, Sep 22, 2014 at 08:34:56PM +0100, Alex Bligh wrote:
> Add a machine parameter qemu-kvm-migration for live migrate compatibility
> with qemu-kvm version 1.0. Usage:
> -machine pc-1.0,qemu-kvm-migration=on
I would rename "migration" to "compatibility" everywhere.
Though it's a matter of taste, if you strongly prefer this one,
go ahead.
>
> This has three effects:
> 1. cirrus-vga.vgamem_mb defaults to 16 rather than 8
> 2. A type 2 piix4_pm record is interpreted as being type 3
> 3. A type 2 i8254 PIT common state is interpreted as being type 3
>
> Signed-off-by: Alex Bligh <alex@xxxxxxxxxxx>
Mostly looks good to me, except:
> ---
> hw/acpi/piix4.c | 47 +++++++++++++++++++++++++++++++++++++++++++++--
> hw/core/machine.c | 16 ++++++++++++++++
> hw/i386/pc.c | 1 +
> hw/i386/pc_piix.c | 25 +++++++++++++++++++++++++
> hw/timer/i8254_common.c | 11 ++++++++++-
> include/hw/boards.h | 7 +++++++
> vl.c | 7 +++++++
> 7 files changed, 111 insertions(+), 3 deletions(-)
>
> 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.
> static int acpi_load_old(QEMUFile *f, void *opaque, int version_id)
> {
> PIIX4PMState *s = opaque;
> int ret, i;
> uint16_t temp;
>
> + /* If we are expecting the inbound migration to come from
> + * qemu-kvm 1.0, it will have a version_id of 2 but really
> + * be version 3, so call back the original vmstate_load_state
> + * with a different more tolerant vmstate descriptor
> + */
> + if ((version_id == 2) &&
drop () around comparison pls
> + 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?
> +
> ret = pci_device_load(PCI_DEVICE(s), f);
> if (ret < 0) {
> return ret;
> @@ -267,8 +281,9 @@ static const VMStateDescription vmstate_memhp_state = {
> };
>
> /* 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/
> *
> */
> @@ -307,6 +322,34 @@ static const VMStateDescription vmstate_acpi = {
> }
> };
>
> +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?
> static void piix4_reset(void *opaque)
> {
> PIIX4PMState *s = opaque;
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 7a66c57..bbccba9 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -235,6 +235,20 @@ static void machine_set_firmware(Object *obj, const char *value, Error **errp)
> ms->firmware = g_strdup(value);
> }
>
> +static bool machine_get_qemu_kvm_migration(Object *obj, Error **errp)
> +{
> + MachineState *ms = MACHINE(obj);
> +
> + return ms->qemu_kvm_migration;
> +}
> +
> +static void machine_set_qemu_kvm_migration(Object *obj, bool value, Error **errp)
> +{
> + MachineState *ms = MACHINE(obj);
> +
> + ms->qemu_kvm_migration = value;
> +}
> +
> static void machine_initfn(Object *obj)
> {
> object_property_add_str(obj, "accel",
> @@ -274,6 +288,8 @@ static void machine_initfn(Object *obj)
> object_property_add_bool(obj, "usb", machine_get_usb, machine_set_usb, NULL);
> object_property_add_str(obj, "firmware",
> machine_get_firmware, machine_set_firmware, NULL);
> + object_property_add_bool(obj, "qemu-kvm-migration",
> + machine_get_qemu_kvm_migration, machine_set_qemu_kvm_migration, NULL);
> }
>
> static void machine_finalize(Object *obj)
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 2cf22b1..c74f2f9 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1516,6 +1516,7 @@ static void pc_generic_machine_class_init(ObjectClass *oc, void *data)
> mc->alias = qm->alias;
> mc->desc = qm->desc;
> mc->init = qm->init;
> + mc->earlyinit = qm->earlyinit;
> mc->reset = qm->reset;
> mc->hot_add_cpu = qm->hot_add_cpu;
> mc->kvm_type = qm->kvm_type;
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 7081c08..0f122e2 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -386,6 +386,21 @@ static void pc_init_pci_1_2(MachineState *machine)
> pc_init_pci(machine);
> }
>
> +static void pc_early_init_pci_1_0(MachineState *machine)
> +{
> + /* default value of cirrus-vga.vgamem_mb should be 16 if migrating from qemu-kvm */
> + if (qemu_opt_get_bool(qemu_get_machine_opts(), "qemu-kvm-migration",
> + DEFAULT_QEMU_KVM_MIGRATION)) {
> + GlobalProperty *gp;
> + MachineClass *mc = MACHINE_GET_CLASS(machine);
> + for (gp = mc->compat_props; gp->driver; gp++) {
> + if (!strcmp(gp->driver, "cirrus-vga") && !strcmp(gp->property, "vgamem_mb")) {
> + gp->value = stringify(16);
> + }
> + }
> + }
> +}
> +
> /* PC init function for pc-0.10 to pc-0.13 */
> static void pc_init_pci_no_kvmclock(MachineState *machine)
> {
> @@ -614,6 +629,11 @@ static QEMUMachine pc_machine_v1_1 = {
> },
> };
>
> +/* 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?
> #define PC_COMPAT_1_0 \
> PC_COMPAT_1_1,\
> {\
> @@ -632,10 +652,15 @@ static QEMUMachine pc_machine_v1_1 = {
> .driver = TYPE_USB_DEVICE,\
> .property = "full-path",\
> .value = "no",\
> + },{\
> + .driver = "cirrus-vga",\
> + .property = "vgamem_mb",\
> + .value = stringify(8),\
> }
>
> static QEMUMachine pc_machine_v1_0 = {
> PC_I440FX_1_2_MACHINE_OPTIONS,
> + .earlyinit = pc_early_init_pci_1_0,
> .name = "pc-1.0",
> .compat_props = (GlobalProperty[]) {
> PC_COMPAT_1_0,
> diff --git a/hw/timer/i8254_common.c b/hw/timer/i8254_common.c
> index 07345f6..0342e93 100644
> --- a/hw/timer/i8254_common.c
> +++ b/hw/timer/i8254_common.c
> @@ -257,6 +257,14 @@ static int pit_dispatch_post_load(void *opaque, int version_id)
> return 0;
> }
>
> +static bool has_irq_disabled(void *opaque, int version_id)
> +{
> + return (version_id >= 3) ||
> + ((version_id == 2) &&
> + qemu_opt_get_bool(qemu_get_machine_opts(), "qemu-kvm-migration",
> + DEFAULT_QEMU_KVM_MIGRATION));
> +}
> +
> static const VMStateDescription vmstate_pit_common = {
> .name = "i8254",
> .version_id = 3,
> @@ -266,7 +274,8 @@ static const VMStateDescription vmstate_pit_common = {
> .pre_save = pit_dispatch_pre_save,
> .post_load = pit_dispatch_post_load,
> .fields = (VMStateField[]) {
> - VMSTATE_UINT32_V(channels[0].irq_disabled, PITCommonState, 3),
> + VMSTATE_UINT32_TEST(channels[0].irq_disabled, PITCommonState,
> + has_irq_disabled),
> VMSTATE_STRUCT_ARRAY(channels, PITCommonState, 3, 2,
> vmstate_pit_channel, PITChannelState),
> VMSTATE_INT64(channels[0].next_transition_time,
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 605a970..ffde5dd 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -13,6 +13,8 @@ typedef struct MachineState MachineState;
>
> typedef void QEMUMachineInitFunc(MachineState *ms);
>
> +typedef void QEMUMachineEarlyInitFunc(MachineState *ms);
> +
> typedef void QEMUMachineResetFunc(void);
>
> typedef void QEMUMachineHotAddCPUFunc(const int64_t id, Error **errp);
> @@ -24,6 +26,7 @@ struct QEMUMachine {
> const char *alias;
> const char *desc;
> QEMUMachineInitFunc *init;
> + QEMUMachineEarlyInitFunc *earlyinit;
> QEMUMachineResetFunc *reset;
> QEMUMachineHotAddCPUFunc *hot_add_cpu;
> QEMUMachineGetKvmtypeFunc *kvm_type;
> @@ -59,6 +62,8 @@ int qemu_register_machine(QEMUMachine *m);
> #define MACHINE_CLASS(klass) \
> OBJECT_CLASS_CHECK(MachineClass, (klass), TYPE_MACHINE)
>
> +#define DEFAULT_QEMU_KVM_MIGRATION false
> +
> MachineClass *find_default_machine(void);
> extern MachineState *current_machine;
>
> @@ -81,6 +86,7 @@ struct MachineClass {
> const char *desc;
>
> void (*init)(MachineState *state);
> + void (*earlyinit)(MachineState *state);
> void (*reset)(void);
> void (*hot_add_cpu)(const int64_t id, Error **errp);
> int (*kvm_type)(const char *arg);
> @@ -123,6 +129,7 @@ struct MachineState {
> bool mem_merge;
> bool usb;
> char *firmware;
> + bool qemu_kvm_migration;
>
> ram_addr_t ram_size;
> ram_addr_t maxram_size;
> diff --git a/vl.c b/vl.c
> index fe451aa..622afea 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -387,6 +387,10 @@ static QemuOptsList qemu_machine_opts = {
> .name = PC_MACHINE_MAX_RAM_BELOW_4G,
> .type = QEMU_OPT_SIZE,
> .help = "maximum ram below the 4G boundary (32bit boundary)",
> + },{
> + .name = "qemu-kvm-migration",
> + .type = QEMU_OPT_BOOL,
> + .help = "enable/disable migration compatibility from qemu-kvm",
> },
> { /* End of list */ }
> },
> @@ -1557,6 +1561,7 @@ static void machine_class_init(ObjectClass *oc, void *data)
> mc->alias = qm->alias;
> mc->desc = qm->desc;
> mc->init = qm->init;
> + mc->earlyinit = qm->earlyinit;
> mc->reset = qm->reset;
> mc->hot_add_cpu = qm->hot_add_cpu;
> mc->kvm_type = qm->kvm_type;
> @@ -4003,6 +4008,8 @@ int main(int argc, char **argv, char **envp)
> OBJECT_CLASS(machine_class))));
> object_property_add_child(object_get_root(), "machine",
> OBJECT(current_machine), &error_abort);
> + if (machine_class->earlyinit)
> + machine_class->earlyinit(current_machine);
> cpu_exec_init_all();
>
> if (machine_class->hw_version) {
> --
> 1.7.9.5
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list