diff mbox

live migration between qemu-kvm 1.0 and 0.15

Message ID 4F71FBE9.3040906@siemens.com
State New
Headers show

Commit Message

Jan Kiszka March 27, 2012, 5:42 p.m. UTC 
On 2012-03-27 18:49, Anthony Liguori wrote:
> On 03/27/2012 11:46 AM, Avi Kivity wrote:
>> On 03/27/2012 06:39 PM, Anthony Liguori wrote:
>>>
>>> So, since we're approaching 1.1, we should really discuss release
>>> criteria for 1.1 with respect to live migration.  I'd prefer to avoid
>>> surprises in this release.
>>
>> Agree strongly.
>>
>>>
>>> My expectation is that migration works from:
>>>
>>> qemu-1.0 -M 1.0     =>     qemu-1.1 -M 1.1
>>
>> Why do you expect that?  Maybe you meant -M 1.0 at the end?
> 
> Sorry, I did mean -M 1.0.
> 
>>
>>> qemu-1.1 -M 1.0<=    qemu-1.1 -M 1.0
>>>
>>> I would expect that migration works from:
>>>
>>> qemu-0.15 -M 0.15   =>    qemu-1.1 -M 0.15
>>>
>>
>> Ack.
>>
>>> I'm okay if this fails gracefully:
>>>
>>> qemu-1.1 -M 0.15<=   qemu-0.15 -M 0.15
>>
>> RHEL has more stringent requirements (going back to its heavily patched
>> 0.12).  I think we should have the infrastructure that allow one to add
>> the hacks to make this work, even if we don't actually do the compat
>> work for the release (I think it's fine for qemu to support just one
>> version going back; and unreasonable to require it to go as far back as
>> RHEL).
> 
> This is reasonable to me.

Here is a draft to get things written in the old format. Totally
untested and likely borken (written in a hurry). I'll split up if it
works fine.

Jan

Comments

Anthony Liguori March 29, 2012, 3:23 p.m. UTC | #1 
On 03/27/2012 12:42 PM, Jan Kiszka wrote:
> On 2012-03-27 18:49, Anthony Liguori wrote:
>> On 03/27/2012 11:46 AM, Avi Kivity wrote:
>>> On 03/27/2012 06:39 PM, Anthony Liguori wrote:
>>>>
>>>> So, since we're approaching 1.1, we should really discuss release
>>>> criteria for 1.1 with respect to live migration.  I'd prefer to avoid
>>>> surprises in this release.
>>>
>>> Agree strongly.
>>>
>>>>
>>>> My expectation is that migration works from:
>>>>
>>>> qemu-1.0 -M 1.0     =>      qemu-1.1 -M 1.1
>>>
>>> Why do you expect that?  Maybe you meant -M 1.0 at the end?
>>
>> Sorry, I did mean -M 1.0.
>>
>>>
>>>> qemu-1.1 -M 1.0<=    qemu-1.1 -M 1.0
>>>>
>>>> I would expect that migration works from:
>>>>
>>>> qemu-0.15 -M 0.15   =>     qemu-1.1 -M 0.15
>>>>
>>>
>>> Ack.
>>>
>>>> I'm okay if this fails gracefully:
>>>>
>>>> qemu-1.1 -M 0.15<=   qemu-0.15 -M 0.15
>>>
>>> RHEL has more stringent requirements (going back to its heavily patched
>>> 0.12).  I think we should have the infrastructure that allow one to add
>>> the hacks to make this work, even if we don't actually do the compat
>>> work for the release (I think it's fine for qemu to support just one
>>> version going back; and unreasonable to require it to go as far back as
>>> RHEL).
>>
>> This is reasonable to me.
>
> Here is a draft to get things written in the old format. Totally
> untested and likely borken (written in a hurry). I'll split up if it
> works fine.

I don't really like this as a matter of principle.

Knowingly migrating when the result may be a broken guest is a bug, it's not a 
feature.

It's one thing if we're changing formats for other reasons, but if we're 
changing the format to send what's effectively broken migration state, then 
that's an evil thing to do.

Subsections are the compromise.  We send a subsection when we think migration 
can work and fail gracefully when it can't.  Presumably there's a reason we're 
not using subsections here.

Regards,

Anthony Liguori
Jan Kiszka March 29, 2012, 3:53 p.m. UTC | #2 
On 2012-03-29 17:23, Anthony Liguori wrote:
> On 03/27/2012 12:42 PM, Jan Kiszka wrote:
>> On 2012-03-27 18:49, Anthony Liguori wrote:
>>> On 03/27/2012 11:46 AM, Avi Kivity wrote:
>>>> On 03/27/2012 06:39 PM, Anthony Liguori wrote:
>>>>>
>>>>> So, since we're approaching 1.1, we should really discuss release
>>>>> criteria for 1.1 with respect to live migration.  I'd prefer to avoid
>>>>> surprises in this release.
>>>>
>>>> Agree strongly.
>>>>
>>>>>
>>>>> My expectation is that migration works from:
>>>>>
>>>>> qemu-1.0 -M 1.0     =>      qemu-1.1 -M 1.1
>>>>
>>>> Why do you expect that?  Maybe you meant -M 1.0 at the end?
>>>
>>> Sorry, I did mean -M 1.0.
>>>
>>>>
>>>>> qemu-1.1 -M 1.0<=    qemu-1.1 -M 1.0
>>>>>
>>>>> I would expect that migration works from:
>>>>>
>>>>> qemu-0.15 -M 0.15   =>     qemu-1.1 -M 0.15
>>>>>
>>>>
>>>> Ack.
>>>>
>>>>> I'm okay if this fails gracefully:
>>>>>
>>>>> qemu-1.1 -M 0.15<=   qemu-0.15 -M 0.15
>>>>
>>>> RHEL has more stringent requirements (going back to its heavily patched
>>>> 0.12).  I think we should have the infrastructure that allow one to add
>>>> the hacks to make this work, even if we don't actually do the compat
>>>> work for the release (I think it's fine for qemu to support just one
>>>> version going back; and unreasonable to require it to go as far back as
>>>> RHEL).
>>>
>>> This is reasonable to me.
>>
>> Here is a draft to get things written in the old format. Totally
>> untested and likely borken (written in a hurry). I'll split up if it
>> works fine.
> 
> I don't really like this as a matter of principle.
> 
> Knowingly migrating when the result may be a broken guest is a bug, it's not a 
> feature.
> 
> It's one thing if we're changing formats for other reasons, but if we're 
> changing the format to send what's effectively broken migration state, then 
> that's an evil thing to do.
> 
> Subsections are the compromise.  We send a subsection when we think migration 
> can work and fail gracefully when it can't.  Presumably there's a reason we're 
> not using subsections here.

In this case (instance ID), it's actually not about a bug fix but a
consolidation of the vmstate format. So I think it's an exception,
though I don't like the code changes it requires as well.

Jan
diff mbox

Patch

diff --git a/hw/fdc.c b/hw/fdc.c
index a0236b7..bf0e59d 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -1981,7 +1981,7 @@  static int isabus_fdc_init1(ISADevice *dev)
     isa_init_irq(&isa->busdev, &fdctrl->irq, isairq);
     fdctrl->dma_chann = dma_chann;
 
-    qdev_set_legacy_instance_id(&dev->qdev, iobase, 2);
+    qdev_set_legacy_instance_id(&dev->qdev, iobase, 2, false);
     ret = fdctrl_init_common(fdctrl);
 
     add_boot_device_path(isa->bootindexA, &dev->qdev, "/floppy@0");
@@ -2002,7 +2002,7 @@  static int sysbus_fdc_init1(SysBusDevice *dev)
     qdev_init_gpio_in(&dev->qdev, fdctrl_handle_tc, 1);
     fdctrl->dma_chann = -1;
 
-    qdev_set_legacy_instance_id(&dev->qdev, 0 /* io */, 2); /* FIXME */
+    qdev_set_legacy_instance_id(&dev->qdev, 0 /* io */, 2, false); /* FIXME */
     ret = fdctrl_init_common(fdctrl);
 
     return ret;
@@ -2019,7 +2019,7 @@  static int sun4m_fdc_init1(SysBusDevice *dev)
     qdev_init_gpio_in(&dev->qdev, fdctrl_handle_tc, 1);
 
     fdctrl->sun4m = 1;
-    qdev_set_legacy_instance_id(&dev->qdev, 0 /* io */, 2); /* FIXME */
+    qdev_set_legacy_instance_id(&dev->qdev, 0 /* io */, 2, false); /* FIXME */
     return fdctrl_init_common(fdctrl);
 }
 
diff --git a/hw/i8254_common.c b/hw/i8254_common.c
index a03d7cd..b8d0d55 100644
--- a/hw/i8254_common.c
+++ b/hw/i8254_common.c
@@ -179,7 +179,7 @@  static int pit_init_common(ISADevice *dev)
 
     isa_register_ioport(dev, &pit->ioports, pit->iobase);
 
-    qdev_set_legacy_instance_id(&dev->qdev, pit->iobase, 2);
+    qdev_set_legacy_instance_id(&dev->qdev, pit->iobase, 2, false);
 
     return 0;
 }
diff --git a/hw/i8259_common.c b/hw/i8259_common.c
index ab3d98b..e37fab6 100644
--- a/hw/i8259_common.c
+++ b/hw/i8259_common.c
@@ -78,7 +78,8 @@  static int pic_init_common(ISADevice *dev)
         isa_register_ioport(NULL, &s->elcr_io, s->elcr_addr);
     }
 
-    qdev_set_legacy_instance_id(&s->dev.qdev, s->iobase, 1);
+    qdev_set_legacy_instance_id(&s->dev.qdev, s->iobase, 1,
+                                s->compat_instance_id);
 
     return 0;
 }
@@ -130,6 +131,8 @@  static Property pic_properties_common[] = {
     DEFINE_PROP_HEX32("elcr_addr", PICCommonState, elcr_addr,  -1),
     DEFINE_PROP_HEX8("elcr_mask", PICCommonState, elcr_mask,  -1),
     DEFINE_PROP_BIT("master", PICCommonState, master,  0, false),
+    DEFINE_PROP_BIT("compat_instance_id", PICCommonState, compat_instance_id,
+                    0, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/i8259_internal.h b/hw/i8259_internal.h
index 4137b61..a47668d 100644
--- a/hw/i8259_internal.h
+++ b/hw/i8259_internal.h
@@ -70,6 +70,7 @@  struct PICCommonState {
     uint32_t master; /* reflects /SP input pin */
     uint32_t iobase;
     uint32_t elcr_addr;
+    uint32_t compat_instance_id;
     MemoryRegion base_io;
     MemoryRegion elcr_io;
 };
diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index 2b59c36..3b0e07c 100644
--- a/hw/mc146818rtc.c
+++ b/hw/mc146818rtc.c
@@ -697,7 +697,7 @@  static int rtc_initfn(ISADevice *dev)
     memory_region_init_io(&s->io, &cmos_ops, s, "rtc", 2);
     isa_register_ioport(dev, &s->io, base);
 
-    qdev_set_legacy_instance_id(&dev->qdev, base, 2);
+    qdev_set_legacy_instance_id(&dev->qdev, base, 2, false);
     qemu_register_reset(rtc_reset, s);
 
     object_property_add(OBJECT(s), "date", "struct tm",
diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 3f99f9a..4c8d591 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -386,6 +386,10 @@  static QEMUMachine pc_machine_v1_0 = {
             .driver   = "isa-fdc",
             .property = "check_media_rate",
             .value    = "off",
+        },{
+            .driver   = "i8259",
+            .property = "compat_instance_id",
+            .value    = "on",
         },
         { /* end of list */ }
     },
@@ -405,6 +409,10 @@  static QEMUMachine pc_machine_v0_15 = {
             .driver   = "isa-fdc",
             .property = "check_media_rate",
             .value    = "off",
+        },{
+            .driver   = "i8259",
+            .property = "compat_instance_id",
+            .value    = "on",
         },
         { /* end of list */ }
     },
@@ -449,6 +457,10 @@  static QEMUMachine pc_machine_v0_14 = {
             .driver   = "pc-sysfw",
             .property = "rom_only",
             .value    = stringify(1),
+        },{
+            .driver   = "i8259",
+            .property = "compat_instance_id",
+            .value    = "on",
         },
         { /* end of list */ }
     },
@@ -505,6 +517,10 @@  static QEMUMachine pc_machine_v0_13 = {
             .driver   = "pc-sysfw",
             .property = "rom_only",
             .value    = stringify(1),
+        },{
+            .driver   = "i8259",
+            .property = "compat_instance_id",
+            .value    = "on",
         },
         { /* end of list */ }
     },
@@ -565,6 +581,10 @@  static QEMUMachine pc_machine_v0_12 = {
             .driver   = "pc-sysfw",
             .property = "rom_only",
             .value    = stringify(1),
+        },{
+            .driver   = "i8259",
+            .property = "compat_instance_id",
+            .value    = "on",
         },
         { /* end of list */ }
     }
@@ -633,6 +653,10 @@  static QEMUMachine pc_machine_v0_11 = {
             .driver   = "pc-sysfw",
             .property = "rom_only",
             .value    = stringify(1),
+        },{
+            .driver   = "i8259",
+            .property = "compat_instance_id",
+            .value    = "on",
         },
         { /* end of list */ }
     }
@@ -713,6 +737,10 @@  static QEMUMachine pc_machine_v0_10 = {
             .driver   = "pc-sysfw",
             .property = "rom_only",
             .value    = stringify(1),
+        },{
+            .driver   = "i8259",
+            .property = "compat_instance_id",
+            .value    = "on",
         },
         { /* end of list */ }
     },
diff --git a/hw/qdev.c b/hw/qdev.c
index ee21d90..7f54ecf 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -155,7 +155,8 @@  int qdev_init(DeviceState *dev)
     if (qdev_get_vmsd(dev)) {
         vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
                                        dev->instance_id_alias,
-                                       dev->alias_required_for_version);
+                                       dev->alias_required_for_version,
+                                       dev->write_alias_id);
     }
     dev->state = DEV_STATE_INITIALIZED;
     if (dev->hotplugged) {
@@ -165,11 +166,13 @@  int qdev_init(DeviceState *dev)
 }
 
 void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
-                                 int required_for_version)
+                                 int required_for_version,
+                                 bool write_alias_id)
 {
     assert(dev->state == DEV_STATE_CREATED);
     dev->instance_id_alias = alias_id;
     dev->alias_required_for_version = required_for_version;
+    dev->write_alias_id = write_alias_id;
 }
 
 int qdev_unplug(DeviceState *dev)
diff --git a/hw/qdev.h b/hw/qdev.h
index 9cc3f98..3917020 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -76,6 +76,7 @@  struct DeviceState {
     QTAILQ_ENTRY(DeviceState) sibling;
     int instance_id_alias;
     int alias_required_for_version;
+    bool write_alias_id;
 };
 
 typedef void (*bus_dev_printfn)(Monitor *mon, DeviceState *dev, int indent);
@@ -147,7 +148,8 @@  DeviceState *qdev_device_add(QemuOpts *opts);
 int qdev_init(DeviceState *dev) QEMU_WARN_UNUSED_RESULT;
 void qdev_init_nofail(DeviceState *dev);
 void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
-                                 int required_for_version);
+                                 int required_for_version,
+                                 bool write_alias_id);
 int qdev_unplug(DeviceState *dev);
 void qdev_free(DeviceState *dev);
 int qdev_simple_unplug_cb(DeviceState *dev);
diff --git a/hw/serial.c b/hw/serial.c
index c0ee55d..20adf98 100644
--- a/hw/serial.c
+++ b/hw/serial.c
@@ -789,7 +789,7 @@  static int serial_isa_initfn(ISADevice *dev)
     s->baudbase = 115200;
     isa_init_irq(dev, &s->irq, isa->isairq);
     serial_init_core(s);
-    qdev_set_legacy_instance_id(&dev->qdev, isa->iobase, 3);
+    qdev_set_legacy_instance_id(&dev->qdev, isa->iobase, 3, false);
 
     memory_region_init_io(&s->io, &serial_io_ops, s, "serial", 8);
     isa_register_ioport(dev, &s->io, isa->iobase);
diff --git a/savevm.c b/savevm.c
index 12fb209..2f53864 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1179,6 +1179,7 @@  typedef struct SaveStateEntry {
     CompatEntry *compat;
     int no_migrate;
     int is_ram;
+    bool write_alias_id;
 } SaveStateEntry;
 
 
@@ -1316,7 +1317,8 @@  void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque)
 int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
                                    const VMStateDescription *vmsd,
                                    void *opaque, int alias_id,
-                                   int required_for_version)
+                                   int required_for_version,
+                                   bool write_alias_id)
 {
     SaveStateEntry *se;
 
@@ -1332,6 +1334,7 @@  int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
     se->opaque = opaque;
     se->vmsd = vmsd;
     se->alias_id = alias_id;
+    se->write_alias_id = write_alias_id;
     se->no_migrate = vmsd->unmigratable;
 
     if (dev && dev->parent_bus && dev->parent_bus->info->get_dev_path) {
@@ -1365,7 +1368,7 @@  int vmstate_register(DeviceState *dev, int instance_id,
                      const VMStateDescription *vmsd, void *opaque)
 {
     return vmstate_register_with_alias_id(dev, instance_id, vmsd,
-                                          opaque, -1, 0);
+                                          opaque, -1, 0, false);
 }
 
 void vmstate_unregister(DeviceState *dev, const VMStateDescription *vmsd,
@@ -1591,7 +1594,7 @@  int qemu_savevm_state_begin(QEMUFile *f, int blk_enable, int shared)
         qemu_put_byte(f, len);
         qemu_put_buffer(f, (uint8_t *)se->idstr, len);
 
-        qemu_put_be32(f, se->instance_id);
+        qemu_put_be32(f, se->write_alias_id ? se->alias_id : se->instance_id);
         qemu_put_be32(f, se->version_id);
 
         ret = se->save_live_state(f, QEMU_VM_SECTION_START, se->opaque);
@@ -1683,7 +1686,7 @@  int qemu_savevm_state_complete(QEMUFile *f)
         qemu_put_byte(f, len);
         qemu_put_buffer(f, (uint8_t *)se->idstr, len);
 
-        qemu_put_be32(f, se->instance_id);
+        qemu_put_be32(f, se->write_alias_id ? se->alias_id : se->instance_id);
         qemu_put_be32(f, se->version_id);
 
         vmstate_save(f, se);
@@ -1762,7 +1765,7 @@  static int qemu_save_device_state(QEMUFile *f)
         qemu_put_byte(f, len);
         qemu_put_buffer(f, (uint8_t *)se->idstr, len);
 
-        qemu_put_be32(f, se->instance_id);
+        qemu_put_be32(f, se->write_alias_id ? se->alias_id : se->instance_id);
         qemu_put_be32(f, se->version_id);
 
         vmstate_save(f, se);
diff --git a/vmstate.h b/vmstate.h
index 82d97ae..34b78d8 100644
--- a/vmstate.h
+++ b/vmstate.h
@@ -605,7 +605,8 @@  int vmstate_register(DeviceState *dev, int instance_id,
 int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
                                    const VMStateDescription *vmsd,
                                    void *base, int alias_id,
-                                   int required_for_version);
+                                   int required_for_version,
+                                   bool write_alias_id);
 void vmstate_unregister(DeviceState *dev, const VMStateDescription *vmsd,
                         void *opaque);