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

[Qemu-devel] [PATCH v2 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

Changes in v2:
 - incorporated Blue Swirl's suggestion to request and check the highest
   vmstate version that requires the alias (updated patches 3..6)

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          |    5 ++++-
 hw/mc146818rtc.c |    3 ++-
 hw/qdev.c        |   16 ++++++++++++++--
 hw/qdev.h        |    4 ++++
 hw/serial.c      |   13 ++++++++++++-
 hw/tmp105.c      |   10 ----------
 savevm.c         |   23 +++++++++++++++++------
 8 files changed, 83 insertions(+), 26 deletions(-)

[Qemu-devel] [PATCH v2 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] Re: [PATCH v2 1/6] tmp105: Drop unused faults field

[andrzej zaborowski]

Hi Jan,

On 15 May 2010 13:32, Jan Kiszka <jan.kiszka@web.de> wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> It was only written, but never read.

I pushed an alternate version to just move the update from post_save
to post_load.  If you think of code as documentation, the struct
becomes less complete in representing the state of the device if you
remove the register.  State doesn't have to be readable through the
busses, some state may be readable through qemu monitor and the like.

Cheers

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

[Jan Kiszka]

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

andrzej zaborowski wrote:
> Hi Jan,
> 
> On 15 May 2010 13:32, Jan Kiszka <jan.kiszka@web.de> wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> It was only written, but never read.
> 
> I pushed an alternate version to just move the update from post_save
> to post_load.  If you think of code as documentation, the struct
> becomes less complete in representing the state of the device if you
> remove the register.  State doesn't have to be readable through the
> busses, some state may be readable through qemu monitor and the like.

That particular state is unreadable via any existing interface (except a
debugger). For pretty-printing TMP105State::config, there will once be
better mechanisms based on vmstate.

I don't mind if that dead code remains, I just want to underline that
it's useless (code as documentation was important before versioning
control entered the scene).

Jan


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

[Qemu-devel] [PATCH v2 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 v2 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.

The user is required specify the highest vmstate version for which the
alias is required. Once this version falls behind the minimum required
for a specific vmstate, an assertion triggers to motivate cleaning up
the obsolete alias.

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

diff --git a/hw/hw.h b/hw/hw.h
index 2d39724..fc2d184 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -758,5 +758,9 @@ 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,
+                                          int required_for_version);
 void vmstate_unregister(const VMStateDescription *vmsd, void *opaque);
 #endif
diff --git a/hw/qdev.c b/hw/qdev.c
index d3bf0fa..af17486 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,23 @@ 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->alias_required_for_version);
+    }
     dev->state = DEV_STATE_INITIALIZED;
     return 0;
 }
 
+void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
+                                 int required_for_version)
+{
+    assert(dev->state == DEV_STATE_CREATED);
+    dev->instance_id_alias = alias_id;
+    dev->alias_required_for_version = required_for_version;
+}
+
 int qdev_unplug(DeviceState *dev)
 {
     if (!dev->parent_bus->allow_hotplug) {
diff --git a/hw/qdev.h b/hw/qdev.h
index d8fbc73..a44060e 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -44,6 +44,8 @@ struct DeviceState {
     QLIST_HEAD(, BusState) child_bus;
     int num_child_bus;
     QLIST_ENTRY(DeviceState) sibling;
+    int instance_id_alias;
+    int alias_required_for_version;
 };
 
 typedef void (*bus_dev_printfn)(Monitor *mon, DeviceState *dev, int indent);
@@ -112,6 +114,8 @@ 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_legacy_instance_id(DeviceState *dev, int alias_id,
+                                 int required_for_version);
 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..dc20390 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,11 +1080,16 @@ 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,
+                                   int required_for_version)
 {
     SaveStateEntry *se;
 
+    /* If this triggers, alias support can be dropped for the vmsd. */
+    assert(alias_id == -1 || required_for_version >= vmsd->minimum_version_id);
+
     se = qemu_mallocz(sizeof(SaveStateEntry));
     pstrcpy(se->idstr, sizeof(se->idstr), vmsd->name);
     se->version_id = vmsd->version_id;
@@ -1093,6 +1099,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 +1111,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, 0);
+}
+
 void vmstate_unregister(const VMStateDescription *vmsd, void *opaque)
 {
     SaveStateEntry *se, *new_se;
@@ -1433,7 +1446,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

[Qemu-devel] [PATCH v2 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..9102edb 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_legacy_instance_id(&dev->qdev, isa->iobase, 3);
 
     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 v2 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..6306496 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_legacy_instance_id(&dev->qdev, iobase, 2);
+    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_legacy_instance_id(&dev->qdev, io, 2);
+    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_legacy_instance_id(&dev->qdev, io, 2);
+    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 v2 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..905e670 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_legacy_instance_id(&dev->qdev, base, 2);
     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 v2 0/6] vmstate: Drop post_save / allow instance ID aliases

[Blue Swirl]

Thanks, applied all.

On 5/15/10, 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
>
>  Changes in v2:
>   - incorporated Blue Swirl's suggestion to request and check the highest
>    vmstate version that requires the alias (updated patches 3..6)
>
>  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          |    5 ++++-
>   hw/mc146818rtc.c |    3 ++-
>   hw/qdev.c        |   16 ++++++++++++++--
>   hw/qdev.h        |    4 ++++
>   hw/serial.c      |   13 ++++++++++++-
>   hw/tmp105.c      |   10 ----------
>   savevm.c         |   23 +++++++++++++++++------
>   8 files changed, 83 insertions(+), 26 deletions(-)
>
>