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

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

[Alex Bligh]

This patch series adds inbound migrate capability from qemu-kvm version
1.0. The main ideas are those set out in Cole Robinson's patch here:
http://pkgs.fedoraproject.org/cgit/qemu.git/tree/0001-Fix-migration-from-qemu-kvm.patch?h=f20
however, rather than patching statically (and breaking inbound
migration on existing machine types), I have added a new machine
parameter (qemu-kvm-migration) which when turned on affects the pc-1.0
machine type. Usage:
        -machine pc-1.0,qemu-kvm-migration=on

Three aproaches are taken:

* cirrus-vga.vgamem_mb defaults to 16 rather than 8. In	order to
  keep -global cirrus-vga.vgamem_mb working even with
  qemu-kvm-migration=on, this is monkey-patched	 into the default
  value			 of the MachineState structure's  compat_props list.

* In hw/timer/i8254_common.c, the VMSTATE_UINT32_TEST macro
  is used to test the version for the irq_disable flags,
  allowing version 3 or more, or version 2 for an inbound
  migrate from qemu-kvm (only).

* In hw/acpi/piix4.c, qemu-kvm incorrectly uses version 2 for
  a version 3 structure, causing acpi_load_old to be used.
  acpi_load_old detects this situation based on the machine type
  and restarts the attempt to load the vmstate using a
  customised VMStateDescription. The above cleaner approach is
  unavailable here.

The above monkey-patching must be done between the selection of
the MachineClass and the processing of the machine parameters
(on the		 one hand) and the processing of   the compat_props list
and the		 globals   on the command line. To do this	    I have added
an earlyinit function to MachineState and QEMUMachine.

I developed this on qemu 2.0 but have forward ported it (trivially)
to master. My testing has been on a VM live-migrated-to-file from
Ubuntu Precise qemu-kvm 1.0.

I have given this a moderate degree of testing but it could do
with more.

Note that certain hardware devices (including QXL) will not
migrate properly due to a fundamental difference in their internal
state between versions.

Also note that (as expected) migration from qemu-2.x to qemu-1.0
will not work, even if the machine types are the same.

Changes since v1:
* Do not use a machine type, use a machine parameter.

Alex Bligh (1):
  Add machine parameter qemu-kvm-migration for live migrate
    compatibility with qemu-kvm

 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(-)

-- 
1.7.9.5

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

[Alex Bligh]

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

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@alex.org.uk>
---
 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;
+
 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) &&
+        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);
+    }
+
     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).
  *
  */
@@ -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()
+    }
+};
+
 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
+ */
+
 #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

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

[Michael S. Tsirkin]

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@alex.org.uk>


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

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

[Alex Bligh]

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

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

[Markus Armbruster]

Alex Bligh <alex@alex.org.uk> writes:

[...]
>>> +/* 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).

I also prefer a new machine type.

Ideally, the management application understands that there are two
incompatibile versions QEMU (upstream and old qemu-kvm), and how to map
their machine types to current QEMU's.

If that's not practical, then downstream can still alias the machine
types around to make things just work in the most important downstream
scenarios.  The most important upstream scenario is QEMU <-> QEMU, of
course.

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

[Paolo Bonzini]

Il 29/09/2014 09:02, Markus Armbruster ha scritto:
>> 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.

I was objecting to making pc-1.0 special.  There's nothing special in
pc-1.0, other machine types also had differences between qemu-kvm and
qemu.  And I do not think that upstream has any reason to make pc-1.0
special.

So, if Ubuntu is okay with breaking pc-1.0 migration from 14.04-old to
14.04-new, the right thing to do is simply that Ubuntu makes its pc-1.0
machine type the qemu-kvm one.  No new machine types, no aliases, no
anything.

For upstream, the option is acceptable because that one applies just as
well to other types than 1.0.  Other distros included 0.15 or 1.2, and
can use the option as well.

>> 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).

You can add a new property to the machine, and do the early_init work in
the property setter, I think.

> I also prefer a new machine type.
> 
> Ideally, the management application understands that there are two
> incompatibile versions QEMU (upstream and old qemu-kvm), and how to map
> their machine types to current QEMU's.

That would mean patching the Ubuntu libvirt, right?  At this point it's
simpler to just patch Ubuntu QEMU to do what you write here:

> If that's not practical, then downstream can still alias the machine
> types around to make things just work in the most important downstream
> scenarios.  The most important upstream scenario is QEMU <-> QEMU, of
> course.

I'm not sure why this patch is of any interest upstream...

Paolo

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

[Alex Bligh]

On 5 Oct 2014, at 08:00, Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 29/09/2014 09:02, Markus Armbruster ha scritto:
>>> 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.
> 
> I was objecting to making pc-1.0 special.  There's nothing special in
> pc-1.0, other machine types also had differences between qemu-kvm and
> qemu.  And I do not think that upstream has any reason to make pc-1.0
> special.

OK, so in v5, pc-1.0 is unchanged, and not made special. A new machine
type is added which allows import from something (unfortunately) called
pc-1.0 in something in qemu's past, as well as some distributions.

> So, if Ubuntu is okay with breaking pc-1.0 migration from 14.04-old to
> 14.04-new, the right thing to do is simply that Ubuntu makes its pc-1.0
> machine type the qemu-kvm one.  No new machine types, no aliases, no
> anything.

That would not allow Ubuntu (or Suse - similarly affected I think)
to import pc-1.0 VMs from things actually running pc-1.0, and would
mean that newly created pc-1.0 VMs would be 'wrong', perpetuating
the problem.

-- 
Alex Bligh

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

[Paolo Bonzini]

> > > 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.
> > 
> > I was objecting to making pc-1.0 special.  There's nothing special in
> > pc-1.0, other machine types also had differences between qemu-kvm and
> > qemu.  And I do not think that upstream has any reason to make pc-1.0
> > special.
> 
> OK, so in v5, pc-1.0 is unchanged, and not made special. A new machine
> type is added which allows import from something (unfortunately) called
> pc-1.0 in something in qemu's past, as well as some distributions.

The very fact that a clone of pc-1.0 is added, but not pc-0.15 or
pc-1.2, makes pc-1.0 special.

My proposal has always been that _all_ PC machines should have a property.
This could be done, for example, by starting with Eduardo's patches that
make a class hierarchy of PC machine types.

pc-1.0 is special in Ubuntu world, because it was in an LTS release.  This
is why this patch should be added to Ubuntu, not to upstream.  pc-1.0 is
not necessarily special in other distros.  And some of them (Fedora for
example) are _always_ treating their machine types as the qemu-kvm variants,
and have been doing so for
a couple years.

I very strongly object to including a patch upstream that is tailored
after a particular downstream, and just because that particular downstream
has failed in doing the integration testing that it was supposed to do.

> > So, if Ubuntu is okay with breaking pc-1.0 migration from 14.04-old to
> > 14.04-new, the right thing to do is simply that Ubuntu makes its pc-1.0
> > machine type the qemu-kvm one.  No new machine types, no aliases, no
> > anything.
> 
> That would not allow Ubuntu (or Suse - similarly affected I think)
> to import pc-1.0 VMs from things actually running pc-1.0, and would
> mean that newly created pc-1.0 VMs would be 'wrong', perpetuating
> the problem.

The problem _is_ perpetual.  "pc-1.0" makes no sense without a context
(the distro).  This is why it is not a problem for Fedora to always use
the qemu-kvm variants.

"pc-1.0" will always be the qemu-kvm variant in Ubuntu context (apart
from the 6 months passed since the release of 14.04), because most
"pc-1.0" machines will have been created with the qemu-kvm package in
Ubuntu 12.04.

It can be confusing---to avoid confusion, RHEL drops the upstream machine
types apart from the "pc" generic type and adopts a completely different
nomenclature.

In fact as time passes the benefit of 12.04->14.04 migration becomes
smaller and smaller and, by now, it should have gone almost completely.
It's much simpler to ignore the problem at this point.  For the next
Ubuntu LTS, Canonical should include migration compatibility in their
test plans.  And start well in advance, for it may only take one
person to fix the bugs, but it takes months of testing to find them.

Paolo

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

[Michael S. Tsirkin]

On Sun, Oct 05, 2014 at 08:26:45AM -0400, Paolo Bonzini wrote:
> > > > 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.
> > > 
> > > I was objecting to making pc-1.0 special.  There's nothing special in
> > > pc-1.0, other machine types also had differences between qemu-kvm and
> > > qemu.  And I do not think that upstream has any reason to make pc-1.0
> > > special.
> > 
> > OK, so in v5, pc-1.0 is unchanged, and not made special. A new machine
> > type is added which allows import from something (unfortunately) called
> > pc-1.0 in something in qemu's past, as well as some distributions.
> 
> The very fact that a clone of pc-1.0 is added, but not pc-0.15 or
> pc-1.2, makes pc-1.0 special.
> 
> My proposal has always been that _all_ PC machines should have a property.
> This could be done, for example, by starting with Eduardo's patches that
> make a class hierarchy of PC machine types.
> 
> pc-1.0 is special in Ubuntu world, because it was in an LTS release.  This
> is why this patch should be added to Ubuntu, not to upstream.  pc-1.0 is
> not necessarily special in other distros.  And some of them (Fedora for
> example) are _always_ treating their machine types as the qemu-kvm variants,
> and have been doing so for
> a couple years.
> 
> I very strongly object to including a patch upstream that is tailored
> after a particular downstream, and just because that particular downstream
> has failed in doing the integration testing that it was supposed to do.
> 
> > > So, if Ubuntu is okay with breaking pc-1.0 migration from 14.04-old to
> > > 14.04-new, the right thing to do is simply that Ubuntu makes its pc-1.0
> > > machine type the qemu-kvm one.  No new machine types, no aliases, no
> > > anything.
> > 
> > That would not allow Ubuntu (or Suse - similarly affected I think)
> > to import pc-1.0 VMs from things actually running pc-1.0, and would
> > mean that newly created pc-1.0 VMs would be 'wrong', perpetuating
> > the problem.
> 
> The problem _is_ perpetual.  "pc-1.0" makes no sense without a context
> (the distro).  This is why it is not a problem for Fedora to always use
> the qemu-kvm variants.
> 
> "pc-1.0" will always be the qemu-kvm variant in Ubuntu context (apart
> from the 6 months passed since the release of 14.04), because most
> "pc-1.0" machines will have been created with the qemu-kvm package in
> Ubuntu 12.04.
> 
> It can be confusing---to avoid confusion, RHEL drops the upstream machine
> types apart from the "pc" generic type and adopts a completely different
> nomenclature.
> 
> In fact as time passes the benefit of 12.04->14.04 migration becomes
> smaller and smaller and, by now, it should have gone almost completely.
> It's much simpler to ignore the problem at this point.  For the next
> Ubuntu LTS, Canonical should include migration compatibility in their
> test plans.  And start well in advance, for it may only take one
> person to fix the bugs, but it takes months of testing to find them.
> 
> Paolo

In fact, if the pc_piix bits are dropped from the patch,
you get a generic patchset that does exactly what you ask,
correct?

Downstream can then enable qemu-kvm compatibility by adding:

-global cirrus-vga.vgamem_mb=16 -global pit-common.qemu-kvm-migration=on
-global PIIX4_PM.qemu-kvm-migration=on

-- 
MST

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

[Paolo Bonzini]

Il 05/10/2014 14:48, Michael S. Tsirkin ha scritto:
> In fact, if the pc_piix bits are dropped from the patch,
> you get a generic patchset that does exactly what you ask,
> correct?
> 
> Downstream can then enable qemu-kvm compatibility by adding:
> 
> -global cirrus-vga.vgamem_mb=16 -global pit-common.qemu-kvm-migration=on
> -global PIIX4_PM.qemu-kvm-migration=on

Yes.

Paolo

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

[Michael S. Tsirkin]

On Sun, Sep 28, 2014 at 09:33:08PM +0100, Alex Bligh wrote:
> 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.

I think same applies to v3 that I reviewed right?
Absolutely, I'm fine with just a new machine type.
This means that management tools will need to learn to
add -qemu-kvm suffix to the machine name if user
requested compatibility with qemu-kvm.
I think there were some implementation issues with patch 1/2
though.

> 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).

Good.

> -- 
> Alex Bligh
> 
> 
> 

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

[Alex Bligh]

On 29 Sep 2014, at 11:08, Michael S. Tsirkin <mst@redhat.com> wrote:

> On Sun, Sep 28, 2014 at 09:33:08PM +0100, Alex Bligh wrote:
>> 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.
> 
> I think same applies to v3 that I reviewed right?
> Absolutely, I'm fine with just a new machine type.
> This means that management tools will need to learn to
> add -qemu-kvm suffix to the machine name if user
> requested compatibility with qemu-kvm.
> I think there were some implementation issues with patch 1/2
> though.
> 
>> 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).
> 
> Good.

OK, I will respin this when I get a chance with the new machine
type back and hopefully address some of the other issues you
brought up.

-- 
Alex Bligh

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

[Serge E. Hallyn]

Quoting Alex Bligh (alex@alex.org.uk):
> 
> On 29 Sep 2014, at 11:08, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> > On Sun, Sep 28, 2014 at 09:33:08PM +0100, Alex Bligh wrote:
> >> 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.
> > 
> > I think same applies to v3 that I reviewed right?
> > Absolutely, I'm fine with just a new machine type.
> > This means that management tools will need to learn to
> > add -qemu-kvm suffix to the machine name if user
> > requested compatibility with qemu-kvm.
> > I think there were some implementation issues with patch 1/2
> > though.
> > 
> >> 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).
> > 
> > Good.
> 
> OK, I will respin this when I get a chance with the new machine
> type back and hopefully address some of the other issues you
> brought up.

Thanks, Alex.  I've got working packages for 12.04, 14,04 and 14.10
at ppa:serge-hallyn/qemu-migration2 and have opened three bugs
(https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1374622,
https://bugs.launchpad.net/ubuntu/+bug/1374617, and
https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1374612) to
get them into the archive.  However if the machine type patches
may end up upstream, then obviously I'd prefer to wait for the
final upstream set.  As we're approaching final freeze for 14.10,
I suspect this will mean getting the patches into 15.04 on the
first day of the cycle (no FFEs needed there) and then a quick
SRU into 12.04 and 14.04.

-serge

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

[Alex Bligh]

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

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

[Markus Armbruster]

Alex Bligh <alex@alex.org.uk> writes:

> This patch series adds inbound migrate capability from qemu-kvm version
> 1.0. The main ideas are those set out in Cole Robinson's patch here:
> http://pkgs.fedoraproject.org/cgit/qemu.git/tree/0001-Fix-migration-from-qemu-kvm.patch?h=f20
> however, rather than patching statically (and breaking inbound
> migration on existing machine types), I have added a new machine
> parameter (qemu-kvm-migration) which when turned on affects the pc-1.0
> machine type. Usage:
>         -machine pc-1.0,qemu-kvm-migration=on

Forgive me if this has been discussed already: why not simply a separate
machine type "pc-1.0-qemu-kvm"?

> Three aproaches are taken:
>
> * cirrus-vga.vgamem_mb defaults to 16 rather than 8. In	order to
>   keep -global cirrus-vga.vgamem_mb working even with
>   qemu-kvm-migration=on, this is monkey-patched	 into the default
>   value			 of the MachineState structure's  compat_props list.

This part fires only for pc-1.0, because it's in
pc_early_init_pci_1_0().

> * In hw/timer/i8254_common.c, the VMSTATE_UINT32_TEST macro
>   is used to test the version for the irq_disable flags,
>   allowing version 3 or more, or version 2 for an inbound
>   migrate from qemu-kvm (only).
>
> * In hw/acpi/piix4.c, qemu-kvm incorrectly uses version 2 for
>   a version 3 structure, causing acpi_load_old to be used.
>   acpi_load_old detects this situation based on the machine type
>   and restarts the attempt to load the vmstate using a
>   customised VMStateDescription. The above cleaner approach is
>   unavailable here.

These parts apply to all machine types, don't they?

> The above monkey-patching must be done between the selection of
> the MachineClass and the processing of the machine parameters
> (on the		 one hand) and the processing of   the compat_props list
> and the		 globals   on the command line. To do this	    I have added
> an earlyinit function to MachineState and QEMUMachine.
>
> I developed this on qemu 2.0 but have forward ported it (trivially)
> to master. My testing has been on a VM live-migrated-to-file from
> Ubuntu Precise qemu-kvm 1.0.
>
> I have given this a moderate degree of testing but it could do
> with more.
>
> Note that certain hardware devices (including QXL) will not
> migrate properly due to a fundamental difference in their internal
> state between versions.
>
> Also note that (as expected) migration from qemu-2.x to qemu-1.0
> will not work, even if the machine types are the same.
>
> Changes since v1:
> * Do not use a machine type, use a machine parameter.

Okay, it has been discussed already.  I'd appreciate a brief recap all
the same.

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

[Alex Bligh]

Markus,

On 24 Sep 2014, at 09:05, Markus Armbruster <armbru@redhat.com> wrote:

> Alex Bligh <alex@alex.org.uk> writes:
> 
>> This patch series adds inbound migrate capability from qemu-kvm version
>> 1.0. The main ideas are those set out in Cole Robinson's patch here:
>> http://pkgs.fedoraproject.org/cgit/qemu.git/tree/0001-Fix-migration-from-qemu-kvm.patch?h=f20
>> however, rather than patching statically (and breaking inbound
>> migration on existing machine types), I have added a new machine
>> parameter (qemu-kvm-migration) which when turned on affects the pc-1.0
>> machine type. Usage:
>>        -machine pc-1.0,qemu-kvm-migration=on
> 
> Forgive me if this has been discussed already: why not simply a separate
> machine type "pc-1.0-qemu-kvm"?

That's what v2 of the patch set does (and I prefer v2). However, mst
wanted it done this way.

> 
>> Three aproaches are taken:
>> 
>> * cirrus-vga.vgamem_mb defaults to 16 rather than 8. In	order to
>>  keep -global cirrus-vga.vgamem_mb working even with
>>  qemu-kvm-migration=on, this is monkey-patched	 into the default
>>  value			 of the MachineState structure's  compat_props list.
> 
> This part fires only for pc-1.0, because it's in
> pc_early_init_pci_1_0().

Yes, intentional. I should have noted that. That's because
qemu-kvm-migration=on the VRAM change was designed to
work only with pc-1.0. I haven't looked at trying to make
other (older) qemu-kvm machine migrations work, but (with the stuff
below), I would guess it would work with the appropriate
command line options for VRAM size.

Obviously I could put early init elsewhere.

>> * In hw/timer/i8254_common.c, the VMSTATE_UINT32_TEST macro
>>  is used to test the version for the irq_disable flags,
>>  allowing version 3 or more, or version 2 for an inbound
>>  migrate from qemu-kvm (only).
>> 
>> * In hw/acpi/piix4.c, qemu-kvm incorrectly uses version 2 for
>>  a version 3 structure, causing acpi_load_old to be used.
>>  acpi_load_old detects this situation based on the machine type
>>  and restarts the attempt to load the vmstate using a
>>  customised VMStateDescription. The above cleaner approach is
>>  unavailable here.
> 
> These parts apply to all machine types, don't they?

They apply only when qemu-kvm-migration=on is selected, but to
any machine type; however machine types newer than pc-1.0 will
be exporting v3 I think anyway.

>> Changes since v1:
>> * Do not use a machine type, use a machine parameter.
> 
> Okay, it has been discussed already.  I'd appreciate a brief recap all
> the same.

See above. I preferred the machine type. But I got to learn more about
QOM on the way :-)

-- 
Alex Bligh

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

[Michael Tokarev]

22.09.2014 23:34, Alex Bligh wrote:
> This patch series adds inbound migrate capability from qemu-kvm version
> 1.0. [...]

Isn't it quite a bit too late already?  That's an old version by
now, and supporting migration from it is interesting for long-term
support distributions - like redhat for example, with several
years of release cycle.  Is it really necessary at this time to
add this ability to upstream qemu?

It'd be very nice to have this capability right when qemu-kvm
tree has been merged (or even before that), but it's been some
years ago already.

Thanks,

/mjt

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

[Dr. David Alan Gilbert]

* Michael Tokarev (mjt@tls.msk.ru) wrote:
> 22.09.2014 23:34, Alex Bligh wrote:
> > This patch series adds inbound migrate capability from qemu-kvm version
> > 1.0. [...]
> 
> Isn't it quite a bit too late already?  That's an old version by
> now, and supporting migration from it is interesting for long-term
> support distributions - like redhat for example, with several
> years of release cycle.  Is it really necessary at this time to
> add this ability to upstream qemu?
> 
> It'd be very nice to have this capability right when qemu-kvm
> tree has been merged (or even before that), but it's been some
> years ago already.

It's amazing what different combinations of QEMU people have
out there; and it seems reasonable to let Alex fix this problem
if it helps him; there's no reason to deny others the same fix.

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK