[Qemu-devel] [PATCH 0/6] vmstate: Drop post_save / allow instance ID aliases

[Qemu-devel] [PATCH 0/6] vmstate: Drop post_save / allow instance ID aliases

[Jan Kiszka]

This series comes with two major contributions:
 - after moving away the last user of post_save (tmp105), this vmstate
   callback is dropped
 - introduction of an instance ID alias to vmstate, allowing to drop a few
   more explicit vmstate_register calls

Jan Kiszka (6):
  tmp105: Drop unused faults field
  vmstate: Drop unused post_save handler
  vmstate: Add support for alias ID
  serial: Register vmstate via qdev
  fdc: Register vmstate via qdev
  mc146818rtc: Register vmstate via qdev

 hw/fdc.c         |   35 ++++++++++++++++++++++++++++++-----
 hw/hw.h          |    4 +++-
 hw/mc146818rtc.c |    3 ++-
 hw/qdev.c        |   13 +++++++++++--
 hw/qdev.h        |    2 ++
 hw/serial.c      |   13 ++++++++++++-
 hw/tmp105.c      |   10 ----------
 savevm.c         |   19 +++++++++++++------
 8 files changed, 73 insertions(+), 26 deletions(-)

[Qemu-devel] [PATCH 1/6] tmp105: Drop unused faults field

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

It was only written, but never read.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
CC: Andrzej Zaborowski <balrogg@gmail.com>
---
 hw/tmp105.c |   10 ----------
 1 files changed, 0 insertions(+), 10 deletions(-)

diff --git a/hw/tmp105.c b/hw/tmp105.c
index 8343aff..cac5f2b 100644
--- a/hw/tmp105.c
+++ b/hw/tmp105.c
@@ -31,7 +31,6 @@ typedef struct {
     uint8_t config;
     int16_t temperature;
     int16_t limit[2];
-    int faults;
     uint8_t alarm;
 } TMP105State;
 
@@ -124,7 +123,6 @@ static void tmp105_write(TMP105State *s)
         if (s->buf[0] & ~s->config & (1 << 0))			/* SD */
             printf("%s: TMP105 shutdown\n", __FUNCTION__);
         s->config = s->buf[0];
-        s->faults = tmp105_faultq[(s->config >> 3) & 3];	/* F */
         tmp105_alarm_update(s);
         break;
 
@@ -173,12 +171,6 @@ static void tmp105_event(i2c_slave *i2c, enum i2c_event event)
     s->len = 0;
 }
 
-static void tmp105_post_save(void *opaque)
-{
-    TMP105State *s = opaque;
-    s->faults = tmp105_faultq[(s->config >> 3) & 3];		/* F */
-}
-
 static int tmp105_post_load(void *opaque, int version_id)
 {
     TMP105State *s = opaque;
@@ -192,7 +184,6 @@ static const VMStateDescription vmstate_tmp105 = {
     .version_id = 0,
     .minimum_version_id = 0,
     .minimum_version_id_old = 0,
-    .post_save = tmp105_post_save,
     .post_load = tmp105_post_load,
     .fields      = (VMStateField []) {
         VMSTATE_UINT8(len, TMP105State),
@@ -214,7 +205,6 @@ static void tmp105_reset(i2c_slave *i2c)
     s->temperature = 0;
     s->pointer = 0;
     s->config = 0;
-    s->faults = tmp105_faultq[(s->config >> 3) & 3];
     s->alarm = 0;
 
     tmp105_interrupt_update(s);
-- 
1.6.0.2

[Qemu-devel] [PATCH 2/6] vmstate: Drop unused post_save handler

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

No device makes use of it anymore.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/hw.h  |    1 -
 savevm.c |    3 ---
 2 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/hw/hw.h b/hw/hw.h
index 328b704..2d39724 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -321,7 +321,6 @@ struct VMStateDescription {
     int (*pre_load)(void *opaque);
     int (*post_load)(void *opaque, int version_id);
     void (*pre_save)(void *opaque);
-    void (*post_save)(void *opaque);
     VMStateField *fields;
 };
 
diff --git a/savevm.c b/savevm.c
index 31a419c..74e553c 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1231,9 +1231,6 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
         }
         field++;
     }
-    if (vmsd->post_save) {
-        vmsd->post_save(opaque);
-    }
 }
 
 static int vmstate_load(QEMUFile *f, SaveStateEntry *se, int version_id)
-- 
1.6.0.2

[Qemu-devel] [PATCH 3/6] vmstate: Add support for alias ID

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Some legacy users (mostly PC devices) of vmstate_register manage
instance IDs on their own, and that unfortunately in a way that is
incompatible with automatically generated ones. This so far prevents
switching those users to vmstates that are registered by qdev.

To establish a migration path, this patch introduces the concept of
alias IDs. They can be passed to an extended vmstate registration
service, and qdev provides a set service to be used during device init.
find_se will consider the alias in addition to the default ID. We can
then start generating the default ID automatically and writing it on
vmsave, thus converting that format without breaking support for upward
migration.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/hw.h   |    3 +++
 hw/qdev.c |   13 +++++++++++--
 hw/qdev.h |    2 ++
 savevm.c  |   16 +++++++++++++---
 4 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/hw/hw.h b/hw/hw.h
index 2d39724..d124716 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -758,5 +758,8 @@ extern void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
                                void *opaque);
 extern int vmstate_register(int instance_id, const VMStateDescription *vmsd,
                             void *base);
+extern int vmstate_register_with_alias_id(int instance_id,
+                                          const VMStateDescription *vmsd,
+                                          void *base, int alias_id);
 void vmstate_unregister(const VMStateDescription *vmsd, void *opaque);
 #endif
diff --git a/hw/qdev.c b/hw/qdev.c
index d3bf0fa..445d4d7 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -93,6 +93,7 @@ static DeviceState *qdev_create_from_info(BusState *bus, DeviceInfo *info)
         assert(bus->allow_hotplug);
         dev->hotplugged = 1;
     }
+    dev->instance_id_alias = -1;
     dev->state = DEV_STATE_CREATED;
     return dev;
 }
@@ -278,12 +279,20 @@ int qdev_init(DeviceState *dev)
         return rc;
     }
     qemu_register_reset(qdev_reset, dev);
-    if (dev->info->vmsd)
-        vmstate_register(-1, dev->info->vmsd, dev);
+    if (dev->info->vmsd) {
+        vmstate_register_with_alias_id(-1, dev->info->vmsd, dev,
+                                       dev->instance_id_alias);
+    }
     dev->state = DEV_STATE_INITIALIZED;
     return 0;
 }
 
+void qdev_set_instance_id_alias(DeviceState *dev, int alias_id)
+{
+    assert(dev->state == DEV_STATE_CREATED);
+    dev->instance_id_alias = alias_id;
+}
+
 int qdev_unplug(DeviceState *dev)
 {
     if (!dev->parent_bus->allow_hotplug) {
diff --git a/hw/qdev.h b/hw/qdev.h
index d8fbc73..b7dbee4 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -44,6 +44,7 @@ struct DeviceState {
     QLIST_HEAD(, BusState) child_bus;
     int num_child_bus;
     QLIST_ENTRY(DeviceState) sibling;
+    int instance_id_alias;
 };
 
 typedef void (*bus_dev_printfn)(Monitor *mon, DeviceState *dev, int indent);
@@ -112,6 +113,7 @@ int qdev_device_help(QemuOpts *opts);
 DeviceState *qdev_device_add(QemuOpts *opts);
 int qdev_init(DeviceState *dev) QEMU_WARN_UNUSED_RESULT;
 void qdev_init_nofail(DeviceState *dev);
+void qdev_set_instance_id_alias(DeviceState *dev, int alias_id);
 int qdev_unplug(DeviceState *dev);
 void qdev_free(DeviceState *dev);
 int qdev_simple_unplug_cb(DeviceState *dev);
diff --git a/savevm.c b/savevm.c
index 74e553c..f155582 100644
--- a/savevm.c
+++ b/savevm.c
@@ -991,6 +991,7 @@ typedef struct SaveStateEntry {
     QTAILQ_ENTRY(SaveStateEntry) entry;
     char idstr[256];
     int instance_id;
+    int alias_id;
     int version_id;
     int section_id;
     SaveSetParamsHandler *set_params;
@@ -1079,8 +1080,9 @@ void unregister_savevm(const char *idstr, void *opaque)
     }
 }
 
-int vmstate_register(int instance_id, const VMStateDescription *vmsd,
-                     void *opaque)
+int vmstate_register_with_alias_id(int instance_id,
+                                   const VMStateDescription *vmsd,
+                                   void *opaque, int alias_id)
 {
     SaveStateEntry *se;
 
@@ -1093,6 +1095,7 @@ int vmstate_register(int instance_id, const VMStateDescription *vmsd,
     se->load_state = NULL;
     se->opaque = opaque;
     se->vmsd = vmsd;
+    se->alias_id = alias_id;
 
     if (instance_id == -1) {
         se->instance_id = calculate_new_instance_id(vmsd->name);
@@ -1104,6 +1107,12 @@ int vmstate_register(int instance_id, const VMStateDescription *vmsd,
     return 0;
 }
 
+int vmstate_register(int instance_id, const VMStateDescription *vmsd,
+                     void *opaque)
+{
+    return vmstate_register_with_alias_id(instance_id, vmsd, opaque, -1);
+}
+
 void vmstate_unregister(const VMStateDescription *vmsd, void *opaque)
 {
     SaveStateEntry *se, *new_se;
@@ -1433,7 +1442,8 @@ static SaveStateEntry *find_se(const char *idstr, int instance_id)
 
     QTAILQ_FOREACH(se, &savevm_handlers, entry) {
         if (!strcmp(se->idstr, idstr) &&
-            instance_id == se->instance_id)
+            (instance_id == se->instance_id ||
+             instance_id == se->alias_id))
             return se;
     }
     return NULL;
-- 
1.6.0.2

Re: [Qemu-devel] [PATCH 3/6] vmstate: Add support for alias ID

[Blue Swirl]

On 5/13/10, Jan Kiszka <jan.kiszka@web.de> wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
>  Some legacy users (mostly PC devices) of vmstate_register manage
>  instance IDs on their own, and that unfortunately in a way that is
>  incompatible with automatically generated ones. This so far prevents
>  switching those users to vmstates that are registered by qdev.
>
>  To establish a migration path, this patch introduces the concept of
>  alias IDs. They can be passed to an extended vmstate registration
>  service, and qdev provides a set service to be used during device init.
>  find_se will consider the alias in addition to the default ID. We can
>  then start generating the default ID automatically and writing it on
>  vmsave, thus converting that format without breaking support for upward
>  migration.

If this is only for compatibility, I think the name should show it,
like vmstate_set_compat_instance_id(), or
vmstate_set_legacy_instance_id(). That way, if there happens to be an
incompatible version bump, the function name suggests that it can be
removed.

The function should also take a last_legacy_version_id parameter.
Consider for example that a vmstate format with the legacy ID is
currently used with version_id of 2. We also start using this
compatibility system. A new, compatible version 3 arrives but we only
want to support legacy ID for version 2, as indicated by
last_legacy_version_id=2. Then with a new version, let's say 5, which
is no longer compatible with 2 or 3, the legacy ID stuff can finally
be thrown away. qdev.c should check if last_legacy_id >=
minimum_version_id and complain otherwise.

Re: [Qemu-devel] [PATCH 3/6] vmstate: Add support for alias ID

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1926 bytes --]

Blue Swirl wrote:
> On 5/13/10, Jan Kiszka <jan.kiszka@web.de> wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>>  Some legacy users (mostly PC devices) of vmstate_register manage
>>  instance IDs on their own, and that unfortunately in a way that is
>>  incompatible with automatically generated ones. This so far prevents
>>  switching those users to vmstates that are registered by qdev.
>>
>>  To establish a migration path, this patch introduces the concept of
>>  alias IDs. They can be passed to an extended vmstate registration
>>  service, and qdev provides a set service to be used during device init.
>>  find_se will consider the alias in addition to the default ID. We can
>>  then start generating the default ID automatically and writing it on
>>  vmsave, thus converting that format without breaking support for upward
>>  migration.
> 
> If this is only for compatibility, I think the name should show it,
> like vmstate_set_compat_instance_id(), or
> vmstate_set_legacy_instance_id(). That way, if there happens to be an
> incompatible version bump, the function name suggests that it can be
> removed.

Hmm, makes some sense, not for the vmstate interface (no new code
outside the core should touch it anymore), but for clarifying the qdev part.

> 
> The function should also take a last_legacy_version_id parameter.
> Consider for example that a vmstate format with the legacy ID is
> currently used with version_id of 2. We also start using this
> compatibility system. A new, compatible version 3 arrives but we only
> want to support legacy ID for version 2, as indicated by
> last_legacy_version_id=2. Then with a new version, let's say 5, which
> is no longer compatible with 2 or 3, the legacy ID stuff can finally
> be thrown away. qdev.c should check if last_legacy_id >=
> minimum_version_id and complain otherwise.

OK, will look into this.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] [PATCH 4/6] serial: Register vmstate via qdev

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

At least for isa-serial, we can already let qdev do the vmstate
registration for us. It just takes wrapping vmstate for the
encapsulating ISASerialState and defining the proper instance ID
aliases.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/serial.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/hw/serial.c b/hw/serial.c
index 90213c4..d4e4b7c 100644
--- a/hw/serial.c
+++ b/hw/serial.c
@@ -771,7 +771,7 @@ static int serial_isa_initfn(ISADevice *dev)
     s->baudbase = 115200;
     isa_init_irq(dev, &s->irq, isa->isairq);
     serial_init_core(s);
-    vmstate_register(isa->iobase, &vmstate_serial, s);
+    qdev_set_instance_id_alias(&dev->qdev, isa->iobase);
 
     register_ioport_write(isa->iobase, 8, 1, serial_ioport_write, s);
     register_ioport_read(isa->iobase, 8, 1, serial_ioport_read, s);
@@ -790,6 +790,16 @@ SerialState *serial_isa_init(int index, CharDriverState *chr)
     return &DO_UPCAST(ISASerialState, dev, dev)->state;
 }
 
+static const VMStateDescription vmstate_isa_serial = {
+    .name = "serial",
+    .version_id = 3,
+    .minimum_version_id = 2,
+    .fields      = (VMStateField []) {
+        VMSTATE_STRUCT(state, ISASerialState, 0, vmstate_serial, SerialState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 SerialState *serial_init(int base, qemu_irq irq, int baudbase,
                          CharDriverState *chr)
 {
@@ -956,6 +966,7 @@ SerialState *serial_mm_init (target_phys_addr_t base, int it_shift,
 static ISADeviceInfo serial_isa_info = {
     .qdev.name  = "isa-serial",
     .qdev.size  = sizeof(ISASerialState),
+    .qdev.vmsd  = &vmstate_isa_serial,
     .init       = serial_isa_initfn,
     .qdev.props = (Property[]) {
         DEFINE_PROP_UINT32("index", ISASerialState, index,   -1),
-- 
1.6.0.2

[Qemu-devel] [PATCH 5/6] fdc: Register vmstate via qdev

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Establish vmstate containers for ISA and sysbus variant, define the
iobase as instance ID alias, and let qdev do the vmstate registration
work.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/fdc.c |   35 ++++++++++++++++++++++++++++++-----
 1 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/hw/fdc.c b/hw/fdc.c
index c4ca9e9..6842808 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -1918,7 +1918,7 @@ FDCtrl *sun4m_fdctrl_init(qemu_irq irq, target_phys_addr_t io_base,
     return fdctrl;
 }
 
-static int fdctrl_init_common(FDCtrl *fdctrl, target_phys_addr_t io_base)
+static int fdctrl_init_common(FDCtrl *fdctrl)
 {
     int i, j;
     static int command_tables_inited = 0;
@@ -1949,7 +1949,6 @@ static int fdctrl_init_common(FDCtrl *fdctrl, target_phys_addr_t io_base)
         DMA_register_channel(fdctrl->dma_chann, &fdctrl_transfer_handler, fdctrl);
     fdctrl_connect_drives(fdctrl);
 
-    vmstate_register(io_base, &vmstate_fdc, fdctrl);
     return 0;
 }
 
@@ -1973,7 +1972,8 @@ static int isabus_fdc_init1(ISADevice *dev)
     isa_init_irq(&isa->busdev, &fdctrl->irq, isairq);
     fdctrl->dma_chann = dma_chann;
 
-    ret = fdctrl_init_common(fdctrl, iobase);
+    qdev_set_instance_id_alias(&dev->qdev, iobase);
+    ret = fdctrl_init_common(fdctrl);
 
     return ret;
 }
@@ -1991,7 +1991,8 @@ static int sysbus_fdc_init1(SysBusDevice *dev)
     qdev_init_gpio_in(&dev->qdev, fdctrl_handle_tc, 1);
     fdctrl->dma_chann = -1;
 
-    ret = fdctrl_init_common(fdctrl, io);
+    qdev_set_instance_id_alias(&dev->qdev, io);
+    ret = fdctrl_init_common(fdctrl);
 
     return ret;
 }
@@ -2008,14 +2009,26 @@ static int sun4m_fdc_init1(SysBusDevice *dev)
     qdev_init_gpio_in(&dev->qdev, fdctrl_handle_tc, 1);
 
     fdctrl->sun4m = 1;
-    return fdctrl_init_common(fdctrl, io);
+    qdev_set_instance_id_alias(&dev->qdev, io);
+    return fdctrl_init_common(fdctrl);
 }
 
+static const VMStateDescription vmstate_isa_fdc ={
+    .name = "fdc",
+    .version_id = 2,
+    .minimum_version_id = 2,
+    .fields = (VMStateField []) {
+        VMSTATE_STRUCT(state, FDCtrlISABus, 0, vmstate_fdc, FDCtrl),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static ISADeviceInfo isa_fdc_info = {
     .init = isabus_fdc_init1,
     .qdev.name  = "isa-fdc",
     .qdev.size  = sizeof(FDCtrlISABus),
     .qdev.no_user = 1,
+    .qdev.vmsd  = &vmstate_isa_fdc,
     .qdev.reset = fdctrl_external_reset_isa,
     .qdev.props = (Property[]) {
         DEFINE_PROP_DRIVE("driveA", FDCtrlISABus, state.drives[0].dinfo),
@@ -2024,10 +2037,21 @@ static ISADeviceInfo isa_fdc_info = {
     },
 };
 
+static const VMStateDescription vmstate_sysbus_fdc ={
+    .name = "fdc",
+    .version_id = 2,
+    .minimum_version_id = 2,
+    .fields = (VMStateField []) {
+        VMSTATE_STRUCT(state, FDCtrlSysBus, 0, vmstate_fdc, FDCtrl),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static SysBusDeviceInfo sysbus_fdc_info = {
     .init = sysbus_fdc_init1,
     .qdev.name  = "sysbus-fdc",
     .qdev.size  = sizeof(FDCtrlSysBus),
+    .qdev.vmsd  = &vmstate_sysbus_fdc,
     .qdev.reset = fdctrl_external_reset_sysbus,
     .qdev.props = (Property[]) {
         DEFINE_PROP_DRIVE("driveA", FDCtrlSysBus, state.drives[0].dinfo),
@@ -2040,6 +2064,7 @@ static SysBusDeviceInfo sun4m_fdc_info = {
     .init = sun4m_fdc_init1,
     .qdev.name  = "SUNW,fdtwo",
     .qdev.size  = sizeof(FDCtrlSysBus),
+    .qdev.vmsd  = &vmstate_sysbus_fdc,
     .qdev.reset = fdctrl_external_reset_sysbus,
     .qdev.props = (Property[]) {
         DEFINE_PROP_DRIVE("drive", FDCtrlSysBus, state.drives[0].dinfo),
-- 
1.6.0.2

[Qemu-devel] [PATCH 6/6] mc146818rtc: Register vmstate via qdev

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

After defining the required alias ID, we can push vmstate registration
of mc146818rtc to qdev.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/mc146818rtc.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index 89a423e..16b033a 100644
--- a/hw/mc146818rtc.c
+++ b/hw/mc146818rtc.c
@@ -609,7 +609,7 @@ static int rtc_initfn(ISADevice *dev)
     register_ioport_write(base, 2, 1, cmos_ioport_write, s);
     register_ioport_read(base, 2, 1, cmos_ioport_read, s);
 
-    vmstate_register(base, &vmstate_rtc, s);
+    qdev_set_instance_id_alias(&dev->qdev, base);
     qemu_register_reset(rtc_reset, s);
     return 0;
 }
@@ -628,6 +628,7 @@ static ISADeviceInfo mc146818rtc_info = {
     .qdev.name     = "mc146818rtc",
     .qdev.size     = sizeof(RTCState),
     .qdev.no_user  = 1,
+    .qdev.vmsd     = &vmstate_rtc,
     .init          = rtc_initfn,
     .qdev.props    = (Property[]) {
         DEFINE_PROP_INT32("base_year", RTCState, base_year, 1980),
-- 
1.6.0.2

[Qemu-devel] Re: [PATCH 0/6] vmstate: Drop post_save / allow instance ID aliases

[Juan Quintela]

Jan Kiszka <jan.kiszka@web.de> wrote:
> This series comes with two major contributions:
>  - after moving away the last user of post_save (tmp105), this vmstate
>    callback is dropped
>  - introduction of an instance ID alias to vmstate, allowing to drop a few
>    more explicit vmstate_register calls

Reviewed-by: Juan Quintela <quintela@redhat.com>