[Qemu-devel] [PATCH 0/5] arm: VMState conversion

[Qemu-devel] [PATCH 0/5] arm: VMState conversion

[Benoît Canet]

These patches convert various ARM related hardware to the VMState api.

They apply on top of akivity/master.

Benoît Canet (5):
  pl181: add vmstate
  bitbang_i2c: convert to VMState
  realview: convert realview i2c to VMState
  integratorcp: convert integratorcm to VMState
  integratorcp: convert icp_pic to VMState

 hw/bitbang_i2c.c  |   92 +++++++++++++++++++++++++++++++++++------------------
 hw/integratorcp.c |   38 ++++++++++++++++++++++
 hw/pl181.c        |   40 +++++++++++++++++++++--
 hw/realview.c     |   21 +++++++++++-
 4 files changed, 154 insertions(+), 37 deletions(-)

-- 
1.7.5.4

[Qemu-devel] [PATCH 1/5] pl181: add vmstate

[Benoît Canet]

Signed-off-by: Benoit Canet <benoit.canet@gmail.com>
---
 hw/pl181.c |   40 ++++++++++++++++++++++++++++++++++++----
 1 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/hw/pl181.c b/hw/pl181.c
index 0943c09..06c8bd2 100644
--- a/hw/pl181.c
+++ b/hw/pl181.c
@@ -37,20 +37,45 @@ typedef struct {
     uint32_t datacnt;
     uint32_t status;
     uint32_t mask[2];
-    int fifo_pos;
-    int fifo_len;
+    int32_t fifo_pos;
+    int32_t fifo_len;
     /* The linux 2.6.21 driver is buggy, and misbehaves if new data arrives
        while it is reading the FIFO.  We hack around this be defering
        subsequent transfers until after the driver polls the status word.
        http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=4446/1
      */
-    int linux_hack;
+    int32_t linux_hack;
     uint32_t fifo[PL181_FIFO_LEN];
     qemu_irq irq[2];
     /* GPIO outputs for 'card is readonly' and 'card inserted' */
     qemu_irq cardstatus[2];
 } pl181_state;
 
+static const VMStateDescription vmstate_pl181 = {
+    .name = "pl181",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(clock, pl181_state),
+        VMSTATE_UINT32(power, pl181_state),
+        VMSTATE_UINT32(cmdarg, pl181_state),
+        VMSTATE_UINT32(cmd, pl181_state),
+        VMSTATE_UINT32(datatimer, pl181_state),
+        VMSTATE_UINT32(datalength, pl181_state),
+        VMSTATE_UINT32(respcmd, pl181_state),
+        VMSTATE_UINT32_ARRAY(response, pl181_state, 4),
+        VMSTATE_UINT32(datactrl, pl181_state),
+        VMSTATE_UINT32(datacnt, pl181_state),
+        VMSTATE_UINT32(status, pl181_state),
+        VMSTATE_UINT32_ARRAY(mask, pl181_state, 2),
+        VMSTATE_INT32(fifo_pos, pl181_state),
+        VMSTATE_INT32(fifo_len, pl181_state),
+        VMSTATE_INT32(linux_hack, pl181_state),
+        VMSTATE_UINT32_ARRAY(fifo, pl181_state, PL181_FIFO_LEN),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 #define PL181_CMD_INDEX     0x3f
 #define PL181_CMD_RESPONSE  (1 << 6)
 #define PL181_CMD_LONGRESP  (1 << 7)
@@ -471,9 +496,16 @@ static int pl181_init(SysBusDevice *dev)
     return 0;
 }
 
+static SysBusDeviceInfo pl181_info = {
+    .init = pl181_init,
+    .qdev.name = "pl181",
+    .qdev.size = sizeof(pl181_state),
+    .qdev.vmsd = &vmstate_pl181,
+};
+
 static void pl181_register_devices(void)
 {
-    sysbus_register_dev("pl181", sizeof(pl181_state), pl181_init);
+    sysbus_register_withprop(&pl181_info);
 }
 
 device_init(pl181_register_devices)
-- 
1.7.5.4

[Qemu-devel] [PATCH 2/5] bitbang_i2c: convert to VMState

[Benoît Canet]

Signed-off-by: Benoit Canet <benoit.canet@gmail.com>
---
 hw/bitbang_i2c.c |   92 +++++++++++++++++++++++++++++++++++------------------
 1 files changed, 61 insertions(+), 31 deletions(-)

diff --git a/hw/bitbang_i2c.c b/hw/bitbang_i2c.c
index 431359d..b711144 100644
--- a/hw/bitbang_i2c.c
+++ b/hw/bitbang_i2c.c
@@ -19,37 +19,53 @@ do { printf("bitbang_i2c: " fmt , ## __VA_ARGS__); } while (0)
 #define DPRINTF(fmt, ...) do {} while(0)
 #endif
 
-typedef enum bitbang_i2c_state {
+enum {
     STOPPED = 0,
-    SENDING_BIT7,
-    SENDING_BIT6,
-    SENDING_BIT5,
-    SENDING_BIT4,
-    SENDING_BIT3,
-    SENDING_BIT2,
-    SENDING_BIT1,
-    SENDING_BIT0,
-    WAITING_FOR_ACK,
-    RECEIVING_BIT7,
-    RECEIVING_BIT6,
-    RECEIVING_BIT5,
-    RECEIVING_BIT4,
-    RECEIVING_BIT3,
-    RECEIVING_BIT2,
-    RECEIVING_BIT1,
-    RECEIVING_BIT0,
-    SENDING_ACK,
-    SENT_NACK
-} bitbang_i2c_state;
+    SENDING_BIT7 = 1,
+    SENDING_BIT6 = 2,
+    SENDING_BIT5 = 3,
+    SENDING_BIT4 = 4,
+    SENDING_BIT3 = 5,
+    SENDING_BIT2 = 6,
+    SENDING_BIT1 = 7,
+    SENDING_BIT0 = 8,
+    WAITING_FOR_ACK = 9,
+    RECEIVING_BIT7 = 10,
+    RECEIVING_BIT6 = 11,
+    RECEIVING_BIT5 = 12,
+    RECEIVING_BIT4 = 13,
+    RECEIVING_BIT3 = 14,
+    RECEIVING_BIT2 = 15,
+    RECEIVING_BIT1 = 16,
+    RECEIVING_BIT0 = 17,
+    SENDING_ACK = 18,
+    SENT_NACK = 19
+};
 
 struct bitbang_i2c_interface {
     i2c_bus *bus;
-    bitbang_i2c_state state;
-    int last_data;
-    int last_clock;
-    int device_out;
+    uint8_t state;
+    int32_t last_data;
+    int32_t last_clock;
+    int32_t device_out;
     uint8_t buffer;
-    int current_addr;
+    int32_t current_addr;
+};
+
+const VMStateDescription vmstate_bitbang_i2c = {
+    .name = "bitbang_i2c",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(state, bitbang_i2c_interface),
+        VMSTATE_INT32(last_data, bitbang_i2c_interface),
+        VMSTATE_INT32(last_clock, bitbang_i2c_interface),
+        VMSTATE_INT32(device_out, bitbang_i2c_interface),
+        VMSTATE_UINT8(buffer, bitbang_i2c_interface),
+        VMSTATE_INT32(current_addr, bitbang_i2c_interface),
+        VMSTATE_END_OF_LIST()
+    }
 };
 
 static void bitbang_i2c_enter_stop(bitbang_i2c_interface *i2c)
@@ -62,7 +78,7 @@ static void bitbang_i2c_enter_stop(bitbang_i2c_interface *i2c)
 }
 
 /* Set device data pin.  */
-static int bitbang_i2c_ret(bitbang_i2c_interface *i2c, int level)
+static int32_t bitbang_i2c_ret(bitbang_i2c_interface *i2c, int32_t level)
 {
     i2c->device_out = level;
     //DPRINTF("%d %d %d\n", i2c->last_clock, i2c->last_data, i2c->device_out);
@@ -70,13 +86,13 @@ static int bitbang_i2c_ret(bitbang_i2c_interface *i2c, int level)
 }
 
 /* Leave device data pin unodified.  */
-static int bitbang_i2c_nop(bitbang_i2c_interface *i2c)
+static int32_t bitbang_i2c_nop(bitbang_i2c_interface *i2c)
 {
     return bitbang_i2c_ret(i2c, i2c->device_out);
 }
 
 /* Returns data line level.  */
-int bitbang_i2c_set(bitbang_i2c_interface *i2c, int line, int level)
+int32_t bitbang_i2c_set(bitbang_i2c_interface *i2c, int line, int32_t level)
 {
     int data;
 
@@ -185,11 +201,24 @@ bitbang_i2c_interface *bitbang_i2c_init(i2c_bus *bus)
 typedef struct {
     SysBusDevice busdev;
     bitbang_i2c_interface *bitbang;
-    int last_level;
+    int32_t last_level;
     qemu_irq out;
 } GPIOI2CState;
 
-static void bitbang_i2c_gpio_set(void *opaque, int irq, int level)
+const VMStateDescription vmstate_gpio_i2c = {
+    .name = "gpio_i2c",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_STRUCT_POINTER(bitbang, GPIOI2CState, vmstate_bitbang_i2c,
+                       bitbang_i2c_interface *),
+        VMSTATE_INT32(last_level, GPIOI2CState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void bitbang_i2c_gpio_set(void *opaque, int irq, int32_t level)
 {
     GPIOI2CState *s = opaque;
 
@@ -221,6 +250,7 @@ static SysBusDeviceInfo gpio_i2c_info = {
     .qdev.name  = "gpio_i2c",
     .qdev.desc  = "Virtual GPIO to I2C bridge",
     .qdev.size  = sizeof(GPIOI2CState),
+    .qdev.vmsd  = &vmstate_gpio_i2c,
 };
 
 static void bitbang_i2c_register(void)
-- 
1.7.5.4

[Qemu-devel] [PATCH 3/5] realview: convert realview i2c to VMState

[Benoît Canet]

Signed-off-by: Benoit Canet <benoit.canet@gmail.com>
---
 hw/realview.c |   21 +++++++++++++++++++--
 1 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/hw/realview.c b/hw/realview.c
index 11ffb8a..2120600 100644
--- a/hw/realview.c
+++ b/hw/realview.c
@@ -24,10 +24,26 @@
 typedef struct {
     SysBusDevice busdev;
     bitbang_i2c_interface *bitbang;
-    int out;
-    int in;
+    int32_t out;
+    int32_t in;
 } RealViewI2CState;
 
+extern VMStateDescription vmstate_bitbang_i2c;
+
+const VMStateDescription vmstate_realview_i2c = {
+    .name = "realview_i2c",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_STRUCT_POINTER(bitbang, RealViewI2CState, vmstate_bitbang_i2c,
+                       bitbang_i2c_interface *),
+        VMSTATE_INT32(out, RealViewI2CState),
+        VMSTATE_INT32(in, RealViewI2CState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static uint32_t realview_i2c_read(void *opaque, target_phys_addr_t offset)
 {
     RealViewI2CState *s = (RealViewI2CState *)opaque;
@@ -90,6 +106,7 @@ static SysBusDeviceInfo realview_i2c_info = {
     .init = realview_i2c_init,
     .qdev.name  = "realview_i2c",
     .qdev.size  = sizeof(RealViewI2CState),
+    .qdev.vmsd  = &vmstate_realview_i2c,
 };
 
 static void realview_register_devices(void)
-- 
1.7.5.4

[Qemu-devel] [PATCH 4/5] integratorcp: convert integratorcm to VMState

[Benoît Canet]

Signed-off-by: Benoit Canet <benoit.canet@gmail.com>
---
 hw/integratorcp.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/hw/integratorcp.c b/hw/integratorcp.c
index 9a289b4..e8d8d67 100644
--- a/hw/integratorcp.c
+++ b/hw/integratorcp.c
@@ -34,6 +34,29 @@ typedef struct {
     uint32_t fiq_enabled;
 } integratorcm_state;
 
+static const VMStateDescription vmstate_integratorcm = {
+    .name = "integratorcm",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(memsz, integratorcm_state),
+        VMSTATE_BOOL(flash_mapped, integratorcm_state),
+        VMSTATE_UINT32(cm_osc, integratorcm_state),
+        VMSTATE_UINT32(cm_ctrl, integratorcm_state),
+        VMSTATE_UINT32(cm_lock, integratorcm_state),
+        VMSTATE_UINT32(cm_auxosc, integratorcm_state),
+        VMSTATE_UINT32(cm_sdram, integratorcm_state),
+        VMSTATE_UINT32(cm_init, integratorcm_state),
+        VMSTATE_UINT32(cm_flags, integratorcm_state),
+        VMSTATE_UINT32(cm_nvflags, integratorcm_state),
+        VMSTATE_UINT32(int_level, integratorcm_state),
+        VMSTATE_UINT32(irq_enabled, integratorcm_state),
+        VMSTATE_UINT32(fiq_enabled, integratorcm_state),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static uint8_t integrator_spd[128] = {
    128, 8, 4, 11, 9, 1, 64, 0,  2, 0xa0, 0xa0, 0, 0, 8, 0, 1,
    0xe, 4, 0x1c, 1, 2, 0x20, 0xc0, 0, 0, 0, 0, 0x30, 0x28, 0x30, 0x28, 0x40
@@ -547,6 +570,7 @@ static SysBusDeviceInfo core_info = {
     .init = integratorcm_init,
     .qdev.name  = "integrator_core",
     .qdev.size  = sizeof(integratorcm_state),
+    .qdev.vmsd = &vmstate_integratorcm,
     .qdev.props = (Property[]) {
         DEFINE_PROP_UINT32("memsz", integratorcm_state, memsz, 0),
         DEFINE_PROP_END_OF_LIST(),
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH 4/5] integratorcp: convert integratorcm to VMState

[Peter Maydell]

On 25 October 2011 12:09, Benoît Canet <benoit.canet@gmail.com> wrote:
> +static const VMStateDescription vmstate_integratorcm = {
> +    .name = "integratorcm",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(memsz, integratorcm_state),
> +        VMSTATE_BOOL(flash_mapped, integratorcm_state),

This raises a question. flash_mapped here is a flag which just
tracks whether the associated MemoryRegion is currently mapped
or unmapped. Do we need to do anything special to ensure that
the MemoryRegion itself is reset to the correct mapped/unmapped
state on restore?

I recall discussing this kind of thing with Avi on IRC but I
can't remember what the conclusion was.

-- PMM

Re: [Qemu-devel] [PATCH 4/5] integratorcp: convert integratorcm to VMState

[Peter Maydell]

2011/10/26 Peter Maydell <peter.maydell@linaro.org>:
> On 25 October 2011 12:09, Benoît Canet <benoit.canet@gmail.com> wrote:
>> +static const VMStateDescription vmstate_integratorcm = {
>> +    .name = "integratorcm",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .minimum_version_id_old = 1,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT32(memsz, integratorcm_state),
>> +        VMSTATE_BOOL(flash_mapped, integratorcm_state),
>
> This raises a question. flash_mapped here is a flag which just
> tracks whether the associated MemoryRegion is currently mapped
> or unmapped. Do we need to do anything special to ensure that
> the MemoryRegion itself is reset to the correct mapped/unmapped
> state on restore?
>
> I recall discussing this kind of thing with Avi on IRC but I
> can't remember what the conclusion was.

Avi, ping? I'm still interested in the answer to this question.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 4/5] integratorcp: convert integratorcm to VMState

[Avi Kivity]

On 11/08/2011 04:07 AM, Peter Maydell wrote:
> 2011/10/26 Peter Maydell <peter.maydell@linaro.org>:
> > On 25 October 2011 12:09, Benoît Canet <benoit.canet@gmail.com> wrote:
> >> +static const VMStateDescription vmstate_integratorcm = {
> >> +    .name = "integratorcm",
> >> +    .version_id = 1,
> >> +    .minimum_version_id = 1,
> >> +    .minimum_version_id_old = 1,
> >> +    .fields = (VMStateField[]) {
> >> +        VMSTATE_UINT32(memsz, integratorcm_state),
> >> +        VMSTATE_BOOL(flash_mapped, integratorcm_state),
> >
> > This raises a question. flash_mapped here is a flag which just
> > tracks whether the associated MemoryRegion is currently mapped
> > or unmapped. Do we need to do anything special to ensure that
> > the MemoryRegion itself is reset to the correct mapped/unmapped
> > state on restore?
> >
> > I recall discussing this kind of thing with Avi on IRC but I
> > can't remember what the conclusion was.
>
> Avi, ping? I'm still interested in the answer to this question.

Sorry, missed this. Yes, you need to ensure the memory region mapping
reflects flash_mapped.

btw, is flash_mapped a real device state (corresponds to a real memory
bit on the device) or just an artefact?  It's usually a bad idea to cast
implementation artefacts in vmstate concrete.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 4/5] integratorcp: convert integratorcm to VMState

[Benoît Canet]

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

flash_mapped reflect the bit 2 of a control register.

Peter, does this patch look better ?

commit 2fa7b11ee2b2532d00056d6bbc928c5162925e1d
Author: Benoît Canet <benoit.canet@gmail.com>
Date:   Mon Oct 24 14:39:26 2011 +0200

    integratorcp: convert integratorcm to VMState

    Signed-off-by: Benoit Canet <benoit.canet@gmail.com>

diff --git a/hw/integratorcp.c b/hw/integratorcp.c
index 9a289b4..4e51579 100644
--- a/hw/integratorcp.c
+++ b/hw/integratorcp.c
@@ -34,6 +34,42 @@ typedef struct {
     uint32_t fiq_enabled;
 } integratorcm_state;

+static void integratorcm_do_remap(integratorcm_state *s, int flash);
+
+static int integratorcm_post_load(void *opaque, int version_id)
+{
+    integratorcm_state *s = opaque;
+
+    if (s->cm_ctrl & 4) {
+        integratorcm_do_remap(s, 0);
+    }
+    /* ??? tlb_flush (cpu_single_env, 1); */
+    return 0;
+}
+
+static const VMStateDescription vmstate_integratorcm = {
+    .name = "integratorcm",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .post_load = integratorcm_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(memsz, integratorcm_state),
+        VMSTATE_UINT32(cm_osc, integratorcm_state),
+        VMSTATE_UINT32(cm_ctrl, integratorcm_state),
+        VMSTATE_UINT32(cm_lock, integratorcm_state),
+        VMSTATE_UINT32(cm_auxosc, integratorcm_state),
+        VMSTATE_UINT32(cm_sdram, integratorcm_state),
+        VMSTATE_UINT32(cm_init, integratorcm_state),
+        VMSTATE_UINT32(cm_flags, integratorcm_state),
+        VMSTATE_UINT32(cm_nvflags, integratorcm_state),
+        VMSTATE_UINT32(int_level, integratorcm_state),
+        VMSTATE_UINT32(irq_enabled, integratorcm_state),
+        VMSTATE_UINT32(fiq_enabled, integratorcm_state),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static uint8_t integrator_spd[128] = {
    128, 8, 4, 11, 9, 1, 64, 0,  2, 0xa0, 0xa0, 0, 0, 8, 0, 1,
    0xe, 4, 0x1c, 1, 2, 0x20, 0xc0, 0, 0, 0, 0, 0x30, 0x28, 0x30, 0x28, 0x40
@@ -547,6 +583,7 @@ static SysBusDeviceInfo core_info = {
     .init = integratorcm_init,
     .qdev.name  = "integrator_core",
     .qdev.size  = sizeof(integratorcm_state),
+    .qdev.vmsd = &vmstate_integratorcm,
     .qdev.props = (Property[]) {
         DEFINE_PROP_UINT32("memsz", integratorcm_state, memsz, 0),
         DEFINE_PROP_END_OF_LIST(),


On Tue, Nov 8, 2011 at 7:33 AM, Avi Kivity <avi@redhat.com> wrote:

> On 11/08/2011 04:07 AM, Peter Maydell wrote:
> > 2011/10/26 Peter Maydell <peter.maydell@linaro.org>:
> > > On 25 October 2011 12:09, Benoît Canet <benoit.canet@gmail.com> wrote:
> > >> +static const VMStateDescription vmstate_integratorcm = {
> > >> +    .name = "integratorcm",
> > >> +    .version_id = 1,
> > >> +    .minimum_version_id = 1,
> > >> +    .minimum_version_id_old = 1,
> > >> +    .fields = (VMStateField[]) {
> > >> +        VMSTATE_UINT32(memsz, integratorcm_state),
> > >> +        VMSTATE_BOOL(flash_mapped, integratorcm_state),
> > >
> > > This raises a question. flash_mapped here is a flag which just
> > > tracks whether the associated MemoryRegion is currently mapped
> > > or unmapped. Do we need to do anything special to ensure that
> > > the MemoryRegion itself is reset to the correct mapped/unmapped
> > > state on restore?
> > >
> > > I recall discussing this kind of thing with Avi on IRC but I
> > > can't remember what the conclusion was.
> >
> > Avi, ping? I'm still interested in the answer to this question.
>
> Sorry, missed this. Yes, you need to ensure the memory region mapping
> reflects flash_mapped.
>
> btw, is flash_mapped a real device state (corresponds to a real memory
> bit on the device) or just an artefact?  It's usually a bad idea to cast
> implementation artefacts in vmstate concrete.
>
> --
> I have a truly marvellous patch that fixes the bug which this
> signature is too narrow to contain.
>
>

[-- Attachment #2: Type: text/html, Size: 5662 bytes --]

Re: [Qemu-devel] [PATCH 4/5] integratorcp: convert integratorcm to VMState

[Peter Maydell]

2011/11/8 Benoît Canet <benoit.canet@gmail.com>:
>
> flash_mapped reflect the bit 2 of a control register.
> Peter, does this patch look better ?

See my reply to Avi; I'd rather we fixed the MemoryRegion
API rather than working around it in devices with postload hooks.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 4/5] integratorcp: convert integratorcm to VMState

[Peter Maydell]

On 8 November 2011 06:33, Avi Kivity <avi@redhat.com> wrote:
> On 11/08/2011 04:07 AM, Peter Maydell wrote:
>> 2011/10/26 Peter Maydell <peter.maydell@linaro.org>:
>> > On 25 October 2011 12:09, Benoît Canet <benoit.canet@gmail.com> wrote:
>> >> +static const VMStateDescription vmstate_integratorcm = {
>> >> +    .name = "integratorcm",
>> >> +    .version_id = 1,
>> >> +    .minimum_version_id = 1,
>> >> +    .minimum_version_id_old = 1,
>> >> +    .fields = (VMStateField[]) {
>> >> +        VMSTATE_UINT32(memsz, integratorcm_state),
>> >> +        VMSTATE_BOOL(flash_mapped, integratorcm_state),
>> >
>> > This raises a question. flash_mapped here is a flag which just
>> > tracks whether the associated MemoryRegion is currently mapped
>> > or unmapped. Do we need to do anything special to ensure that
>> > the MemoryRegion itself is reset to the correct mapped/unmapped
>> > state on restore?
>> >
>> > I recall discussing this kind of thing with Avi on IRC but I
>> > can't remember what the conclusion was.
>>
>> Avi, ping? I'm still interested in the answer to this question.
>
> Sorry, missed this. Yes, you need to ensure the memory region mapping
> reflects flash_mapped.

This needs to be in the MemoryRegion core implementation, please.
I don't want to have to redo it for every device that has a
remappable region. In particular, at the moment memory.c's
add/delete region functions will assert if the region was already
added/deleted, which means the device has to keep track in a
not-vm-saved state flag whether the memory was mapped so it can
resync things on load (by comparing the non-saved flag and the
saved-state).

Ideally memory.c should just ensure that the memory hierarchy
is saved and restored without devices having to do anything.

> btw, is flash_mapped a real device state (corresponds to a real memory
> bit on the device) or just an artefact?  It's usually a bad idea to cast
> implementation artefacts in vmstate concrete.

It's an implementation artefact that you introduced when you
did the memoryregion conversion :-)

I have a half-a-patch to fix that lurking somewhere but it
stalled because at the moment the non-saved flag is required
in order to work around memory.c being unhelpful.

-- PMM

Re: [Qemu-devel] [PATCH 4/5] integratorcp: convert integratorcm to VMState

[Avi Kivity]

On 11/08/2011 02:15 PM, Peter Maydell wrote:
> >>
> >> Avi, ping? I'm still interested in the answer to this question.
> >
> > Sorry, missed this. Yes, you need to ensure the memory region mapping
> > reflects flash_mapped.
>
> This needs to be in the MemoryRegion core implementation, please.

Right; see the memory/mutators branch.  I plan to push this as soon as
everything is converted.

> I don't want to have to redo it for every device that has a
> remappable region. In particular, at the moment memory.c's
> add/delete region functions will assert if the region was already
> added/deleted, which means the device has to keep track in a
> not-vm-saved state flag whether the memory was mapped so it can
> resync things on load (by comparing the non-saved flag and the
> saved-state).
>
> Ideally memory.c should just ensure that the memory hierarchy
> is saved and restored without devices having to do anything.

That's a bit far-fetched - I don't want the memory core involved in
save/restore.  But if/when we integrate the memory core into QOM, then
yes, that layer can take care of it.  If we have an observable attribute
(that fires off a callback when changed), we can link memory API
properties into that attribute.

> > btw, is flash_mapped a real device state (corresponds to a real memory
> > bit on the device) or just an artefact?  It's usually a bad idea to cast
> > implementation artefacts in vmstate concrete.
>
> It's an implementation artefact that you introduced when you
> did the memoryregion conversion :-)
>
> I have a half-a-patch to fix that lurking somewhere but it
> stalled because at the moment the non-saved flag is required
> in order to work around memory.c being unhelpful.

Ideally we'd have a way to separate implementation state from real state
(after working hard to eliminate implementation state).

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 4/5] integratorcp: convert integratorcm to VMState

[Peter Maydell]

On 8 November 2011 12:21, Avi Kivity <avi@redhat.com> wrote:
> On 11/08/2011 02:15 PM, Peter Maydell wrote:
>> This needs to be in the MemoryRegion core implementation, please.
>
> Right; see the memory/mutators branch.  I plan to push this as soon as
> everything is converted.

OK, then this save/restore patch should wait until that has landed.

>> Ideally memory.c should just ensure that the memory hierarchy
>> is saved and restored without devices having to do anything.
>
> That's a bit far-fetched - I don't want the memory core involved in
> save/restore.  But if/when we integrate the memory core into QOM, then
> yes, that layer can take care of it.  If we have an observable attribute
> (that fires off a callback when changed), we can link memory API
> properties into that attribute.

The memory hierarchy is a visible part of the state which can
change as the guest does things -- it has to get saved and
restored somehow. I think it would be better done in the core
where it will always be done right, rather than relying on
each device to handle it correctly (especially since it's not
always obvious if it's been missed out from the device.)

-- PMM

Re: [Qemu-devel] [PATCH 4/5] integratorcp: convert integratorcm to VMState

[Avi Kivity]

On 11/08/2011 02:30 PM, Peter Maydell wrote:
> On 8 November 2011 12:21, Avi Kivity <avi@redhat.com> wrote:
> > On 11/08/2011 02:15 PM, Peter Maydell wrote:
> >> This needs to be in the MemoryRegion core implementation, please.
> >
> > Right; see the memory/mutators branch.  I plan to push this as soon as
> > everything is converted.
>
> OK, then this save/restore patch should wait until that has landed.

Please don't add interdependencies, especially as I can't promise a firm
date as to when everything is converted (however I'll take this
opportunity to remind you that patches, especially for omap, are more
welcome than I can possibly articulate).

> >> Ideally memory.c should just ensure that the memory hierarchy
> >> is saved and restored without devices having to do anything.
> >
> > That's a bit far-fetched - I don't want the memory core involved in
> > save/restore.  But if/when we integrate the memory core into QOM, then
> > yes, that layer can take care of it.  If we have an observable attribute
> > (that fires off a callback when changed), we can link memory API
> > properties into that attribute.
>
> The memory hierarchy is a visible part of the state which can
> change as the guest does things -- it has to get saved and
> restored somehow. I think it would be better done in the core
> where it will always be done right, rather than relying on
> each device to handle it correctly (especially since it's not
> always obvious if it's been missed out from the device.)

We agree, the only difference is in what "core" refers to.  I don't want
memory.c do become intermingled with everything else.  It should be in a
separate layer.  Devices would then talk to this layer, and not to the
gluing by themselves as they have to now.

Or maybe memory.c will be layered on top of QOM, and then it can take
care of everything.  I really don't know QOM well enough to say. 
Copying Anthony for more insight.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 4/5] integratorcp: convert integratorcm to VMState

[Peter Maydell]

On 8 November 2011 12:38, Avi Kivity <avi@redhat.com> wrote:
> On 11/08/2011 02:30 PM, Peter Maydell wrote:
>> On 8 November 2011 12:21, Avi Kivity <avi@redhat.com> wrote:
>> > On 11/08/2011 02:15 PM, Peter Maydell wrote:
>> >> This needs to be in the MemoryRegion core implementation, please.
>> >
>> > Right; see the memory/mutators branch.  I plan to push this as soon as
>> > everything is converted.
>>
>> OK, then this save/restore patch should wait until that has landed.
>
> Please don't add interdependencies, especially as I can't promise a firm
> date as to when everything is converted (however I'll take this
> opportunity to remind you that patches, especially for omap, are more
> welcome than I can possibly articulate).

I'm really not going to add workarounds for API deficiencies.
Fortunately these devices (and omap_gpmc) don't have save/load
already so nothing's going to be broken by that.

-- PMM

Re: [Qemu-devel] [PATCH 4/5] integratorcp: convert integratorcm to VMState

[Anthony Liguori]

On 11/08/2011 06:38 AM, Avi Kivity wrote:
> On 11/08/2011 02:30 PM, Peter Maydell wrote:
>> On 8 November 2011 12:21, Avi Kivity<avi@redhat.com>  wrote:
>>> On 11/08/2011 02:15 PM, Peter Maydell wrote:
>>>> This needs to be in the MemoryRegion core implementation, please.
>>>
>>> Right; see the memory/mutators branch.  I plan to push this as soon as
>>> everything is converted.
>>
>> OK, then this save/restore patch should wait until that has landed.
>
> Please don't add interdependencies, especially as I can't promise a firm
> date as to when everything is converted (however I'll take this
> opportunity to remind you that patches, especially for omap, are more
> welcome than I can possibly articulate).
>
>>>> Ideally memory.c should just ensure that the memory hierarchy
>>>> is saved and restored without devices having to do anything.
>>>
>>> That's a bit far-fetched - I don't want the memory core involved in
>>> save/restore.  But if/when we integrate the memory core into QOM, then
>>> yes, that layer can take care of it.  If we have an observable attribute
>>> (that fires off a callback when changed), we can link memory API
>>> properties into that attribute.
>>
>> The memory hierarchy is a visible part of the state which can
>> change as the guest does things -- it has to get saved and
>> restored somehow. I think it would be better done in the core
>> where it will always be done right, rather than relying on
>> each device to handle it correctly (especially since it's not
>> always obvious if it's been missed out from the device.)
>
> We agree, the only difference is in what "core" refers to.  I don't want
> memory.c do become intermingled with everything else.  It should be in a
> separate layer.  Devices would then talk to this layer, and not to the
> gluing by themselves as they have to now.
>
> Or maybe memory.c will be layered on top of QOM, and then it can take
> care of everything.  I really don't know QOM well enough to say.
> Copying Anthony for more insight.

QOM fixes all problems, but I don't think this has anything to do with QOM :-)

We fundamentally should do save/restore using a rigorous, automatically 
generated mechanism where every field is save/restored[1].  That means we should 
have a VMSTATE_MEMORY_REGION().

VMSTATE_MEMORY_REGION should save off the state of the memory region, and 
restore it appropriately.  VMSTATE_MEMORY_REGION's implementation does not need 
to live in memory.c.  It can certainly live in savevm.c or somewhere else more 
appropriate.

Where the device is mapped within the address space is no longer a property of 
the device, so restoring the mapping really should happen at whatever owns the 
address space (in this case sysbus).

In this case, integratorcp is the one mapping things into its own address space 
so flash_mapped ought to be saved by integratorcp.

[1] Deciding that a field doesn't need to be saved should be an exception.

Regards,

Anthony Liguori

>

Re: [Qemu-devel] [PATCH 4/5] integratorcp: convert integratorcm to VMState

[Avi Kivity]

On 11/08/2011 03:50 PM, Anthony Liguori wrote:
>> We agree, the only difference is in what "core" refers to.  I don't want
>> memory.c do become intermingled with everything else.  It should be in a
>> separate layer.  Devices would then talk to this layer, and not to the
>> gluing by themselves as they have to now.
>>
>> Or maybe memory.c will be layered on top of QOM, and then it can take
>> care of everything.  I really don't know QOM well enough to say.
>> Copying Anthony for more insight.
>
>
> QOM fixes all problems, but I don't think this has anything to do with
> QOM :-)
>
> We fundamentally should do save/restore using a rigorous,
> automatically generated mechanism where every field is save/restored[1].  

"fundamentally", "rigrous", "automatically", "generated", "mechanism",
"every", "field", and even "a" are all words that I associate with QOM. 
Am I misunderstanding anything?

> That means we should have a VMSTATE_MEMORY_REGION().
>
> VMSTATE_MEMORY_REGION should save off the state of the memory region,
> and restore it appropriately.  VMSTATE_MEMORY_REGION's implementation
> does not need to live in memory.c.  It can certainly live in savevm.c
> or somewhere else more appropriate.

What state is that?  Some devices have fixed size, offset, parent, and
enable/disable state (is there a word for that?), so there is no state
that needs to be transferred.  For other devices this is all dynamic.

The way I see it, we create a link between some device state (a
register) and a memory API field (like the offset).  This way, when one
changes, so does the other.  In complicated devices we'll have to write
a callback.

>
> Where the device is mapped within the address space is no longer a
> property of the device, so restoring the mapping really should happen
> at whatever owns the address space (in this case sysbus).
>
> In this case, integratorcp is the one mapping things into its own
> address space so flash_mapped ought to be saved by integratorcp.

flash_mapped always reflects a bit in a real register.  We shouldn't
duplicate state.

> [1] Deciding that a field doesn't need to be saved should be an
> exception.

It should indicate that the field doesn't need to exist.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 4/5] integratorcp: convert integratorcm to VMState

[Anthony Liguori]

On 11/08/2011 08:38 AM, Avi Kivity wrote:
> On 11/08/2011 03:50 PM, Anthony Liguori wrote:
>>> We agree, the only difference is in what "core" refers to.  I don't want
>>> memory.c do become intermingled with everything else.  It should be in a
>>> separate layer.  Devices would then talk to this layer, and not to the
>>> gluing by themselves as they have to now.
>>>
>>> Or maybe memory.c will be layered on top of QOM, and then it can take
>>> care of everything.  I really don't know QOM well enough to say.
>>> Copying Anthony for more insight.
>>
>>
>> QOM fixes all problems, but I don't think this has anything to do with
>> QOM :-)
>>
>> We fundamentally should do save/restore using a rigorous,
>> automatically generated mechanism where every field is save/restored[1].
>
> "fundamentally", "rigrous", "automatically", "generated", "mechanism",
> "every", "field", and even "a" are all words that I associate with QOM.
> Am I misunderstanding anything?

There's no code generation in QOM :-)

This just comes down to how we do save/restore.  We white list things we care 
about.  We should move to a model where we save/restore everything (preferably 
via code generation), and then black list/transform state before it goes over 
the wire.

Mike Roth's migration Visitor series is a first step in this direction.  The 
reason I bring this up in this context though is that using that mind set makes 
the answer about what to do here obvious.  If it's a member of a device's state, 
it should be save/restored.

MemoryRegion is a member of the device's state, so it should be save/restored 
with the device.

>> That means we should have a VMSTATE_MEMORY_REGION().
>>
>> VMSTATE_MEMORY_REGION should save off the state of the memory region,
>> and restore it appropriately.  VMSTATE_MEMORY_REGION's implementation
>> does not need to live in memory.c.  It can certainly live in savevm.c
>> or somewhere else more appropriate.
>
> What state is that?  Some devices have fixed size, offset, parent, and
> enable/disable state (is there a word for that?), so there is no state
> that needs to be transferred.  For other devices this is all dynamic.

Any mutable state should be save/restored.  Immutable state doesn't need to be 
saved as it's created as part of the device model.

If the question is, how do we restore the immutable state, that should be 
happening as part of device creation, no?

> The way I see it, we create a link between some device state (a
> register) and a memory API field (like the offset).  This way, when one
> changes, so does the other.  In complicated devices we'll have to write
> a callback.

In devices where we dynamically change the offset (it's mutable), we should save 
the offset and restore it.  Since offset is sometimes mutable and sometimes 
immutable, we should always save/restore it.  In the cases where it's really 
immutable, since the value isn't changing, there's no harm in doing save/restore.

Yes, we could save just the device register, and use a callback to regenerate 
the offset.  But that adds complexity and leads to more save/restore bugs.

We shouldn't be reluctant to save/restore derived state.  Whether we send it 
over the wire is a different story.  We should start by saving as much state as 
we need to, and then sit down and start removing state and adding callbacks as 
we need to.

That way, we start with a strong statement of correctness as opposed to starting 
from a position of weak correctness.

>> Where the device is mapped within the address space is no longer a
>> property of the device, so restoring the mapping really should happen
>> at whatever owns the address space (in this case sysbus).
>>
>> In this case, integratorcp is the one mapping things into its own
>> address space so flash_mapped ought to be saved by integratorcp.
>
> flash_mapped always reflects a bit in a real register.  We shouldn't
> duplicate state.

Why?  The only thing that removing it does is create additional complexity for 
save/restore.  You may argue that sending minimal state improves migration 
compatibility but I think the current state of save/restore is an existence 
proof that this line of reasoning is incorrect.

Regards,

Anthony Liguori

>
>> [1] Deciding that a field doesn't need to be saved should be an
>> exception.
>
> It should indicate that the field doesn't need to exist.
>

Re: [Qemu-devel] [PATCH 4/5] integratorcp: convert integratorcm to VMState

[Avi Kivity]

On 11/08/2011 05:04 PM, Anthony Liguori wrote:
>
> There's no code generation in QOM :-)
>
> This just comes down to how we do save/restore.  We white list things
> we care about.  We should move to a model where we save/restore
> everything (preferably via code generation), and then black
> list/transform state before it goes over the wire.
>
> Mike Roth's migration Visitor series is a first step in this
> direction.  The reason I bring this up in this context though is that
> using that mind set makes the answer about what to do here obvious. 
> If it's a member of a device's state, it should be save/restored.

Ok.

>
> MemoryRegion is a member of the device's state, so it should be
> save/restored with the device.

Not all MemoryRegion fields are state.  In some instantiations, none of
them are.

>
>>> That means we should have a VMSTATE_MEMORY_REGION().
>>>
>>> VMSTATE_MEMORY_REGION should save off the state of the memory region,
>>> and restore it appropriately.  VMSTATE_MEMORY_REGION's implementation
>>> does not need to live in memory.c.  It can certainly live in savevm.c
>>> or somewhere else more appropriate.
>>
>> What state is that?  Some devices have fixed size, offset, parent, and
>> enable/disable state (is there a word for that?), so there is no state
>> that needs to be transferred.  For other devices this is all dynamic.
>
> Any mutable state should be save/restored.  Immutable state doesn't
> need to be saved as it's created as part of the device model.

The memory API doesn't know which fields are mutable and which are not.

>
> If the question is, how do we restore the immutable state, that should
> be happening as part of device creation, no?
>
>> The way I see it, we create a link between some device state (a
>> register) and a memory API field (like the offset).  This way, when one
>> changes, so does the other.  In complicated devices we'll have to write
>> a callback.
>
> In devices where we dynamically change the offset (it's mutable), we
> should save the offset and restore it.  Since offset is sometimes
> mutable and sometimes immutable, we should always save/restore it.  In
> the cases where it's really immutable, since the value isn't changing,
> there's no harm in doing save/restore.

There is, you're taking an implementation detail and making it into an
ABI.  Change the implementation and migration breaks.

You can have a real region modeled as a set of nested regions, or as one
big region (with a more complicated switch () statement in the
callback).  This shouldn't be reflected in the save/restore ABI.

>
> Yes, we could save just the device register, and use a callback to
> regenerate the offset.  But that adds complexity and leads to more
> save/restore bugs.
>
> We shouldn't be reluctant to save/restore derived state.  Whether we
> send it over the wire is a different story.  We should start by saving
> as much state as we need to, and then sit down and start removing
> state and adding callbacks as we need to.

"saving state without sending it over the wire" is another way of saying
"not saving state".

> That way, we start with a strong statement of correctness as opposed
> to starting from a position of weak correctness.

We also start from a position of fragility wrt. implementation details.

>> flash_mapped always reflects a bit in a real register.  We shouldn't
>> duplicate state.
>
>
> Why?  The only thing that removing it does is create additional
> complexity for save/restore.  You may argue that sending minimal state
> improves migration compatibility but I think the current state of
> save/restore is an existence proof that this line of reasoning is
> incorrect.

It doesn't create additional complexity for save restore, and I don't
think that the current state of save/restore proves anything except that
it needs a lot more work.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 4/5] integratorcp: convert integratorcm to VMState

[Anthony Liguori]

On 11/08/2011 09:15 AM, Avi Kivity wrote:
> On 11/08/2011 05:04 PM, Anthony Liguori wrote:
>>> What state is that?  Some devices have fixed size, offset, parent, and
>>> enable/disable state (is there a word for that?), so there is no state
>>> that needs to be transferred.  For other devices this is all dynamic.
>>
>> Any mutable state should be save/restored.  Immutable state doesn't
>> need to be saved as it's created as part of the device model.
>
> The memory API doesn't know which fields are mutable and which are not.

Right, but sending immutable fields is just wasteful, it's not functionally 
incorrect.

>>
>> If the question is, how do we restore the immutable state, that should
>> be happening as part of device creation, no?
>>
>>> The way I see it, we create a link between some device state (a
>>> register) and a memory API field (like the offset).  This way, when one
>>> changes, so does the other.  In complicated devices we'll have to write
>>> a callback.
>>
>> In devices where we dynamically change the offset (it's mutable), we
>> should save the offset and restore it.  Since offset is sometimes
>> mutable and sometimes immutable, we should always save/restore it.  In
>> the cases where it's really immutable, since the value isn't changing,
>> there's no harm in doing save/restore.
>
> There is, you're taking an implementation detail and making it into an
> ABI.  Change the implementation and migration breaks.

Yes, that's a feature, not a bug.  If we send too little state today in version 
X, then discover this while working on version X + 1, we have no recourse.  We 
have to black list version X.

Discovering this is hard because we have to find a symptom of broken migration. 
  This is often subtle like, "if you migrate while a floppy request is in 
flight, the request is lost resulting in a timeout in the guest kernel".

If we send too much state (internal implementation that is derived from 
something else) in version X, then discover this while working on version X + 1, 
we can filter the incoming state in X + 1 to just ignore the extra state and 
derive the correct internal state from the other stable registers.

Discovering cases like this is easy because migration fails directly--not 
indirectly through a functional regression.  That means this is something we can 
very easily catch in regression testing.

I actually think this is the way to do it too.  Save/restore everything by 
default and then as we develop and discover migration breaks, add filtering in 
the new versions to ignore and not send internal state.  I don't think there's a 
tremendous amount of value is proactively filtering internal state.  A lot of 
internal state never changes over a long period of time.

>> Yes, we could save just the device register, and use a callback to
>> regenerate the offset.  But that adds complexity and leads to more
>> save/restore bugs.
>>
>> We shouldn't be reluctant to save/restore derived state.  Whether we
>> send it over the wire is a different story.  We should start by saving
>> as much state as we need to, and then sit down and start removing
>> state and adding callbacks as we need to.
>
> "saving state without sending it over the wire" is another way of saying
> "not saving state".

Or filtering it on the receiving end.  That's the fundamental difference.

>> Why?  The only thing that removing it does is create additional
>> complexity for save/restore.  You may argue that sending minimal state
>> improves migration compatibility but I think the current state of
>> save/restore is an existence proof that this line of reasoning is
>> incorrect.
>
> It doesn't create additional complexity for save restore, and I don't
> think that the current state of save/restore proves anything except that
> it needs a lot more work.

It's very hard to do the style of save/restore that we do correctly.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 4/5] integratorcp: convert integratorcm to VMState

[Avi Kivity]

On 11/08/2011 05:32 PM, Anthony Liguori wrote:
>
>>>
>>> If the question is, how do we restore the immutable state, that should
>>> be happening as part of device creation, no?
>>>
>>>> The way I see it, we create a link between some device state (a
>>>> register) and a memory API field (like the offset).  This way, when
>>>> one
>>>> changes, so does the other.  In complicated devices we'll have to
>>>> write
>>>> a callback.
>>>
>>> In devices where we dynamically change the offset (it's mutable), we
>>> should save the offset and restore it.  Since offset is sometimes
>>> mutable and sometimes immutable, we should always save/restore it.  In
>>> the cases where it's really immutable, since the value isn't changing,
>>> there's no harm in doing save/restore.
>>
>> There is, you're taking an implementation detail and making it into an
>> ABI.  Change the implementation and migration breaks.
>
> Yes, that's a feature, not a bug.  If we send too little state today
> in version X, then discover this while working on version X + 1, we
> have no recourse.  We have to black list version X.
>
> Discovering this is hard because we have to find a symptom of broken
> migration.  This is often subtle like, "if you migrate while a floppy
> request is in flight, the request is lost resulting in a timeout in
> the guest kernel".
>
> If we send too much state (internal implementation that is derived
> from something else) in version X, then discover this while working on
> version X + 1, we can filter the incoming state in X + 1 to just
> ignore the extra state and derive the correct internal state from the
> other stable registers.
>
> Discovering cases like this is easy because migration fails
> directly--not indirectly through a functional regression.  That means
> this is something we can very easily catch in regression testing.
>
> I actually think this is the way to do it too.  Save/restore
> everything by default and then as we develop and discover migration
> breaks, add filtering in the new versions to ignore and not send
> internal state.  I don't think there's a tremendous amount of value is
> proactively filtering internal state.  A lot of internal state never
> changes over a long period of time.

I might agree if a significant fraction of the memory API's state needed
to be saved.  But that's not the case -- indeed I expect it to be zero.

Take this patch for example, the only field that is mutable is the
enabled/disabled state, which mirrors some bit in a register.  PIIX's
PAM, PCI's BARs are the same.  I doubt there is *any* case where the
memory API is the sole source of this information.

The way we do this now is to call device_update_mappings() whenever a
register that contains mapping information changes, whether it is in a
device_write() callback or in device_post_load().  All that you'd save
with automatic memory API state migration is the latter call.

>
>>> Yes, we could save just the device register, and use a callback to
>>> regenerate the offset.  But that adds complexity and leads to more
>>> save/restore bugs.
>>>
>>> We shouldn't be reluctant to save/restore derived state.  Whether we
>>> send it over the wire is a different story.  We should start by saving
>>> as much state as we need to, and then sit down and start removing
>>> state and adding callbacks as we need to.
>>
>> "saving state without sending it over the wire" is another way of saying
>> "not saving state".
>
> Or filtering it on the receiving end.  That's the fundamental difference.

I might agree if I thought there is anything worthwhile in the memory
API's state.

>
>>> Why?  The only thing that removing it does is create additional
>>> complexity for save/restore.  You may argue that sending minimal state
>>> improves migration compatibility but I think the current state of
>>> save/restore is an existence proof that this line of reasoning is
>>> incorrect.
>>
>> It doesn't create additional complexity for save restore, and I don't
>> think that the current state of save/restore proves anything except that
>> it needs a lot more work.
>
> It's very hard to do the style of save/restore that we do correctly.

If we had a Register class, that would take care of device registers
automatically.  As to in flight transactions or hidden state, I don't
think there are any shortcuts.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 4/5] integratorcp: convert integratorcm to VMState

[Anthony Liguori]

On 11/08/2011 11:19 AM, Avi Kivity wrote:
> On 11/08/2011 05:32 PM, Anthony Liguori wrote:
>>
>>>>
>>>> If the question is, how do we restore the immutable state, that should
>>>> be happening as part of device creation, no?
>>>>
>>>>> The way I see it, we create a link between some device state (a
>>>>> register) and a memory API field (like the offset).  This way, when
>>>>> one
>>>>> changes, so does the other.  In complicated devices we'll have to
>>>>> write
>>>>> a callback.
>>>>
>>>> In devices where we dynamically change the offset (it's mutable), we
>>>> should save the offset and restore it.  Since offset is sometimes
>>>> mutable and sometimes immutable, we should always save/restore it.  In
>>>> the cases where it's really immutable, since the value isn't changing,
>>>> there's no harm in doing save/restore.
>>>
>>> There is, you're taking an implementation detail and making it into an
>>> ABI.  Change the implementation and migration breaks.
>>
>> Yes, that's a feature, not a bug.  If we send too little state today
>> in version X, then discover this while working on version X + 1, we
>> have no recourse.  We have to black list version X.
>>
>> Discovering this is hard because we have to find a symptom of broken
>> migration.  This is often subtle like, "if you migrate while a floppy
>> request is in flight, the request is lost resulting in a timeout in
>> the guest kernel".
>>
>> If we send too much state (internal implementation that is derived
>> from something else) in version X, then discover this while working on
>> version X + 1, we can filter the incoming state in X + 1 to just
>> ignore the extra state and derive the correct internal state from the
>> other stable registers.
>>
>> Discovering cases like this is easy because migration fails
>> directly--not indirectly through a functional regression.  That means
>> this is something we can very easily catch in regression testing.
>>
>> I actually think this is the way to do it too.  Save/restore
>> everything by default and then as we develop and discover migration
>> breaks, add filtering in the new versions to ignore and not send
>> internal state.  I don't think there's a tremendous amount of value is
>> proactively filtering internal state.  A lot of internal state never
>> changes over a long period of time.
>
> I might agree if a significant fraction of the memory API's state needed
> to be saved.  But that's not the case -- indeed I expect it to be zero.
>
> Take this patch for example, the only field that is mutable is the
> enabled/disabled state, which mirrors some bit in a register.  PIIX's
> PAM, PCI's BARs are the same.  I doubt there is *any* case where the
> memory API is the sole source of this information.
>
> The way we do this now is to call device_update_mappings() whenever a
> register that contains mapping information changes, whether it is in a
> device_write() callback or in device_post_load().  All that you'd save
> with automatic memory API state migration is the latter call.

So we have:

typedef struct {
     SysBusDevice busdev;
     uint32_t memsz;
     MemoryRegion flash;
     bool flash_mapped;
     uint32_t cm_osc;
     uint32_t cm_ctrl;
     uint32_t cm_lock;
     uint32_t cm_auxosc;
     uint32_t cm_sdram;
     uint32_t cm_init;
     uint32_t cm_flags;
     uint32_t cm_nvflags;
     uint32_t int_level;
     uint32_t irq_enabled;
     uint32_t fiq_enabled;
} integratorcm_state;

What I'm saying is let's do:

VMSTATE_SYSBUS(integratorcm_state, busdev),
VMSTATE_UINT32(integratorcm, memsz),
VMSTATE_MEMORY_REGION(integratorcm, flash),
VMSTATE_BOOL(integratorcm, flash_mapped),
VMSTATE_UINT32(integratorcm, cm_osc),
VMSTATE_UINT32(integratorcm, cm_ctrl),
VMSTATE_UINT32(integratorcm, cm_lock),
VMSTATE_UINT32(integratorcm, cm_auxosc),
VMSTATE_UINT32(integratorcm, cm_sdram),
VMSTATE_UINT32(integratorcm, cm_init),
VMSTATE_UINT32(integratorcm, cm_flags),
VMSTATE_UINT32(integratorcm, cm_nvflags),
VMSTATE_UINT32(integratorcm, int_level),
VMSTATE_UINT32(integratorcm, irq_enabled),
VMSTATE_UINT32(integratorcm, fiq_enabled),

As there's a 1-1 mapping here.

You can agree, that this is functionally correct.  But flash_mapped is derived 
state and the contents of flash are almost entirely immutable so we don't 
strictly need to send it.

Okay, then let's add something to vmstate to suppress fields.  It could be as 
simple as:

  struct VMStateDescription {
+    const char *derived_fields[];
      const char *name;

This gives us a few things.  First, it means we're describing how to marshal 
everything which I really believe is the direction we need to go.  Second, it 
makes writing VMState descriptions easier to review.  Every field should be in 
the VMState description.  Any field that is in the derived_fields array should 
have its value set in the post_load function.  You could also have an 
immutable_fields array to indicate which fields are immutable.

Since VMSTATE_MEMORY_REGION is probably just going to point to a substructure, 
you could mark all of the fields of the memory region as immutable except for 
enabled and mark that derived.  This would also let us to do things like 
systematically make sure that when we're listing derived fields, we validate 
that we have a post_load function.

>>>> Yes, we could save just the device register, and use a callback to
>>>> regenerate the offset.  But that adds complexity and leads to more
>>>> save/restore bugs.
>>>>
>>>> We shouldn't be reluctant to save/restore derived state.  Whether we
>>>> send it over the wire is a different story.  We should start by saving
>>>> as much state as we need to, and then sit down and start removing
>>>> state and adding callbacks as we need to.
>>>
>>> "saving state without sending it over the wire" is another way of saying
>>> "not saving state".
>>
>> Or filtering it on the receiving end.  That's the fundamental difference.
>
> I might agree if I thought there is anything worthwhile in the memory
> API's state.
>
>>
>>>> Why?  The only thing that removing it does is create additional
>>>> complexity for save/restore.  You may argue that sending minimal state
>>>> improves migration compatibility but I think the current state of
>>>> save/restore is an existence proof that this line of reasoning is
>>>> incorrect.
>>>
>>> It doesn't create additional complexity for save restore, and I don't
>>> think that the current state of save/restore proves anything except that
>>> it needs a lot more work.
>>
>> It's very hard to do the style of save/restore that we do correctly.
>
> If we had a Register class, that would take care of device registers
> automatically.  As to in flight transactions or hidden state, I don't
> think there are any shortcuts.

BTW, I've thought about this in the past but never came up with anything that 
really made sense.  Have you thought about what what a Register class would do?

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 4/5] integratorcp: convert integratorcm to VMState

[Avi Kivity]

On 11/09/2011 04:40 PM, Anthony Liguori wrote:
>
> typedef struct {
>     SysBusDevice busdev;
>     uint32_t memsz;
>     MemoryRegion flash;
>     bool flash_mapped;

Both flash.has_parent and flash_mapped are slaved to a bit of one of the
registers below.

>     uint32_t cm_osc;
>     uint32_t cm_ctrl;
>     uint32_t cm_lock;
>     uint32_t cm_auxosc;
>     uint32_t cm_sdram;
>     uint32_t cm_init;
>     uint32_t cm_flags;
>     uint32_t cm_nvflags;
>     uint32_t int_level;
>     uint32_t irq_enabled;
>     uint32_t fiq_enabled;
> } integratorcm_state;
>
> What I'm saying is let's do:
>
> VMSTATE_SYSBUS(integratorcm_state, busdev),
> VMSTATE_UINT32(integratorcm, memsz),
> VMSTATE_MEMORY_REGION(integratorcm, flash),

Therefore this line is 100% redundant.

> VMSTATE_BOOL(integratorcm, flash_mapped),
> VMSTATE_UINT32(integratorcm, cm_osc),
> VMSTATE_UINT32(integratorcm, cm_ctrl),
> VMSTATE_UINT32(integratorcm, cm_lock),
> VMSTATE_UINT32(integratorcm, cm_auxosc),
> VMSTATE_UINT32(integratorcm, cm_sdram),
> VMSTATE_UINT32(integratorcm, cm_init),
> VMSTATE_UINT32(integratorcm, cm_flags),
> VMSTATE_UINT32(integratorcm, cm_nvflags),
> VMSTATE_UINT32(integratorcm, int_level),
> VMSTATE_UINT32(integratorcm, irq_enabled),
> VMSTATE_UINT32(integratorcm, fiq_enabled),
>
> As there's a 1-1 mapping here.
>
> You can agree, that this is functionally correct.  But flash_mapped is
> derived state and the contents of flash are almost entirely immutable
> so we don't strictly need to send it.
>
> Okay, then let's add something to vmstate to suppress fields.  It
> could be as simple as:
>
>  struct VMStateDescription {
> +    const char *derived_fields[];
>      const char *name;
>
> This gives us a few things.  First, it means we're describing how to
> marshal everything which I really believe is the direction we need to
> go.  Second, it makes writing VMState descriptions easier to review. 
> Every field should be in the VMState description.  Any field that is
> in the derived_fields array should have its value set in the post_load
> function.  You could also have an immutable_fields array to indicate
> which fields are immutable.

100% of the memory API's fields are either immutable or derived.

>
> Since VMSTATE_MEMORY_REGION is probably just going to point to a
> substructure, you could mark all of the fields of the memory region as
> immutable except for enabled and mark that derived.  This would also
> let us to do things like systematically make sure that when we're
> listing derived fields, we validate that we have a post_load function.
>

If a post_load is missing, it's just a bug, not missing state, so it's
not a catastrophic bug.  The issues are save-side.

>> If we had a Register class, that would take care of device registers
>> automatically.  As to in flight transactions or hidden state, I don't
>> think there are any shortcuts.
>
> BTW, I've thought about this in the past but never came up with
> anything that really made sense.  Have you thought about what what a
> Register class would do?
>

name (for the monitor)
size
ptr to storage (in device state)
writeable bits mask
clear-on-read mask
read function (if computed on demand; otherwise satisfied from storage)
write function (if have side effects)

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 4/5] integratorcp: convert integratorcm to VMState

[Peter Maydell]

On 9 November 2011 15:05, Avi Kivity <avi@redhat.com> wrote:
> On 11/09/2011 04:40 PM, Anthony Liguori wrote:
>>
>> typedef struct {
>>     SysBusDevice busdev;
>>     uint32_t memsz;
>>     MemoryRegion flash;
>>     bool flash_mapped;
>
> Both flash.has_parent and flash_mapped are slaved to a bit of one of the
> registers below.

Yes. The only reason for having a separate flash_mapped would be if
it was *not* saved/loaded and we needed it to track whether the
memory region API thought the region was mapped or not (ie whether
calling memory_region_add_subregion_overlap() from the post-load-hook
would assert, which is what it does in master currently.)

This is why I'm pushing back on Benoit's patch here -- I don't
want us to carry around this extra flash_mapped flag unless we
really need it.

>From a really quick glance at Avi's memory/mutators branch I think
the long term plan would be that rather than mapping/unmapping the
region we just call memory_region_set_enabled() which would cope
happily with being asked to enable an already-enabled region.
Is that right?

-- PMM

Re: [Qemu-devel] [PATCH 4/5] integratorcp: convert integratorcm to VMState

[Avi Kivity]

On 11/09/2011 05:20 PM, Peter Maydell wrote:
> From a really quick glance at Avi's memory/mutators branch I think
> the long term plan would be that rather than mapping/unmapping the
> region we just call memory_region_set_enabled() which would cope
> happily with being asked to enable an already-enabled region.
> Is that right?

It is.  Call it from both the _write() callback that sets the register,
and from post_load(), and you're done.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 4/5] integratorcp: convert integratorcm to VMState

[Anthony Liguori]

On 11/09/2011 09:05 AM, Avi Kivity wrote:
> On 11/09/2011 04:40 PM, Anthony Liguori wrote:
>>
>> typedef struct {
>>      SysBusDevice busdev;
>>      uint32_t memsz;
>>      MemoryRegion flash;
>>      bool flash_mapped;
>
> Both flash.has_parent and flash_mapped are slaved to a bit of one of the
> registers below.
>
>>      uint32_t cm_osc;
>>      uint32_t cm_ctrl;
>>      uint32_t cm_lock;
>>      uint32_t cm_auxosc;
>>      uint32_t cm_sdram;
>>      uint32_t cm_init;
>>      uint32_t cm_flags;
>>      uint32_t cm_nvflags;
>>      uint32_t int_level;
>>      uint32_t irq_enabled;
>>      uint32_t fiq_enabled;
>> } integratorcm_state;
>>
>> What I'm saying is let's do:
>>
>> VMSTATE_SYSBUS(integratorcm_state, busdev),
>> VMSTATE_UINT32(integratorcm, memsz),
>> VMSTATE_MEMORY_REGION(integratorcm, flash),
>
> Therefore this line is 100% redundant.

Yes, but the problem is that it's not obvious *why*.  That's what I'm trying to 
get at here.  If you have a VMSTATE_MEMORY_REGION() that has all of it's fields 
marked immutable and one field marked derived, now it becomes obvious *why* we 
don't save these fields.

Just not having it in the vmstate description makes it very non-obvious.  Is it 
a bug?  Is there some field in memory region that I'm responsible for setting in 
a post load hook?

>> This gives us a few things.  First, it means we're describing how to
>> marshal everything which I really believe is the direction we need to
>> go.  Second, it makes writing VMState descriptions easier to review.
>> Every field should be in the VMState description.  Any field that is
>> in the derived_fields array should have its value set in the post_load
>> function.  You could also have an immutable_fields array to indicate
>> which fields are immutable.
>
> 100% of the memory API's fields are either immutable or derived.

Ok, let's at least make the code make it obvious that that is the case.

>> BTW, I've thought about this in the past but never came up with
>> anything that really made sense.  Have you thought about what what a
>> Register class would do?
>>
>
> name (for the monitor)
> size
> ptr to storage (in device state)
> writeable bits mask
> clear-on-read mask

Really?  Is that all that common outside of PCI config?

> read function (if computed on demand; otherwise satisfied from storage)
> write function (if have side effects)

I tried something like this in Python at one point and the code ended up very 
big to write a device model.  It's hard to beat the conciseness of the dispatch 
functions with a switch() statement.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 4/5] integratorcp: convert integratorcm to VMState

[Avi Kivity]

On 11/09/2011 05:49 PM, Anthony Liguori wrote:
>
>>> VMSTATE_MEMORY_REGION(integratorcm, flash),
>>
>> Therefore this line is 100% redundant.
>
>
> Yes, but the problem is that it's not obvious *why*.  That's what I'm
> trying to get at here.  If you have a VMSTATE_MEMORY_REGION() that has
> all of it's fields marked immutable and one field marked derived, now
> it becomes obvious *why* we don't save these fields.

Every MemoryRegion field in qemu today is either immutable or slaved to
another register.  We could have a system to annotate every field, but
it's pointless.

If we had a device that set the region offset to some value it computes
at runtime that is not derived from state (say, offset = count of writes
to some register) then there would be some point in it.  But we don't,
so there isn't.

> Just not having it in the vmstate description makes it very
> non-obvious.  Is it a bug?  Is there some field in memory region that
> I'm responsible for setting in a post load hook?

Missing post-load hook bugs are not destructive.  Of course we should
try to avoid them, but a markup system that we know ends up doing
nothing is excessive.

>
>>> This gives us a few things.  First, it means we're describing how to
>>> marshal everything which I really believe is the direction we need to
>>> go.  Second, it makes writing VMState descriptions easier to review.
>>> Every field should be in the VMState description.  Any field that is
>>> in the derived_fields array should have its value set in the post_load
>>> function.  You could also have an immutable_fields array to indicate
>>> which fields are immutable.
>>
>> 100% of the memory API's fields are either immutable or derived.
>
> Ok, let's at least make the code make it obvious that that is the case.

The memory/mutators branch simplifies it by eliminating pseudo state
like flash_mapped.

>
>>> BTW, I've thought about this in the past but never came up with
>>> anything that really made sense.  Have you thought about what what a
>>> Register class would do?
>>>
>>
>> name (for the monitor)
>> size
>> ptr to storage (in device state)
>> writeable bits mask
>> clear-on-read mask
>
> Really?  Is that all that common outside of PCI config?

Yes, ISR fields often have it (like virtio).

>
>> read function (if computed on demand; otherwise satisfied from storage)
>> write function (if have side effects)
>
> I tried something like this in Python at one point and the code ended
> up very big to write a device model.  It's hard to beat the
> conciseness of the dispatch functions with a switch() statement.

This style of code really wants lambdas.  Without them, we have 4-5
lines of boilerplate for each callback.  Even then, it's worthwhile IMO
(and many callbacks can be avoided, both read and write, or merged into
a device_update_mapping or device_update_irq read-all-state style
functions).

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 4/5] integratorcp: convert integratorcm to VMState

[Peter Maydell]

On 9 November 2011 15:56, Avi Kivity <avi@redhat.com> wrote:
> On 11/09/2011 05:49 PM, Anthony Liguori wrote:
>> Avi wrote:
>>> clear-on-read mask
>>
>> Really?  Is that all that common outside of PCI config?
>
> Yes, ISR fields often have it (like virtio).

Write-one-to-clear is pretty popular too.

-- PMM

Re: [Qemu-devel] [PATCH 4/5] integratorcp: convert integratorcm to VMState

[Anthony Liguori]

On 11/09/2011 09:56 AM, Avi Kivity wrote:
> On 11/09/2011 05:49 PM, Anthony Liguori wrote:
>>
>>>> VMSTATE_MEMORY_REGION(integratorcm, flash),
>>>
>>> Therefore this line is 100% redundant.
>>
>>
>> Yes, but the problem is that it's not obvious *why*.  That's what I'm
>> trying to get at here.  If you have a VMSTATE_MEMORY_REGION() that has
>> all of it's fields marked immutable and one field marked derived, now
>> it becomes obvious *why* we don't save these fields.
>
> Every MemoryRegion field in qemu today is either immutable or slaved to
> another register.  We could have a system to annotate every field, but
> it's pointless.

If I'm writing a device and doing save/restore and I happen to use a 
MemoryRegion, how do I determine that every field is either immutable or slaved?

> If we had a device that set the region offset to some value it computes
> at runtime that is not derived from state (say, offset = count of writes
> to some register) then there would be some point in it.  But we don't,
> so there isn't.
>
>> Just not having it in the vmstate description makes it very
>> non-obvious.  Is it a bug?  Is there some field in memory region that
>> I'm responsible for setting in a post load hook?
>
> Missing post-load hook bugs are not destructive.  Of course we should
> try to avoid them, but a markup system that we know ends up doing
> nothing is excessive.
>
>>
>>>> This gives us a few things.  First, it means we're describing how to
>>>> marshal everything which I really believe is the direction we need to
>>>> go.  Second, it makes writing VMState descriptions easier to review.
>>>> Every field should be in the VMState description.  Any field that is
>>>> in the derived_fields array should have its value set in the post_load
>>>> function.  You could also have an immutable_fields array to indicate
>>>> which fields are immutable.
>>>
>>> 100% of the memory API's fields are either immutable or derived.
>>
>> Ok, let's at least make the code make it obvious that that is the case.
>
> The memory/mutators branch simplifies it by eliminating pseudo state
> like flash_mapped.

They just moved the derived state into the MemoryRegion, no?

>>>> BTW, I've thought about this in the past but never came up with
>>>> anything that really made sense.  Have you thought about what what a
>>>> Register class would do?
>>>>
>>>
>>> name (for the monitor)
>>> size
>>> ptr to storage (in device state)
>>> writeable bits mask
>>> clear-on-read mask
>>
>> Really?  Is that all that common outside of PCI config?
>
> Yes, ISR fields often have it (like virtio).

Yes, but virtio-pci was a very special case to avoid taking an extra exit.

Do you know of any other than virtio-pci?  All the ones I can think of (RTC, 
Serial, etc.) are cleared with a write.

>>> read function (if computed on demand; otherwise satisfied from storage)
>>> write function (if have side effects)
>>
>> I tried something like this in Python at one point and the code ended
>> up very big to write a device model.  It's hard to beat the
>> conciseness of the dispatch functions with a switch() statement.
>
> This style of code really wants lambdas.  Without them, we have 4-5
> lines of boilerplate for each callback.  Even then, it's worthwhile IMO
> (and many callbacks can be avoided, both read and write, or merged into
> a device_update_mapping or device_update_irq read-all-state style
> functions).

Yeah, I looked at this but wasn't happy with the results.  In practice, many 
devices end up implementing non-trivial logic when register values change.

What I was really interested in was coming up with a way to get really high 
quality tracing of device register accesses.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 4/5] integratorcp: convert integratorcm to VMState

[Avi Kivity]

On 11/09/2011 07:43 PM, Anthony Liguori wrote:
>> Every MemoryRegion field in qemu today is either immutable or slaved to
>> another register.  We could have a system to annotate every field, but
>> it's pointless.
>
>
> If I'm writing a device and doing save/restore and I happen to use a
> MemoryRegion, how do I determine that every field is either immutable
> or slaved?

In general 'return true' should work.  I have a hard time imagining a
device where this doesn't hold.

If all memory API functions are called with parameters that are
functions of the state and only the state, you're good.

>> The memory/mutators branch simplifies it by eliminating pseudo state
>> like flash_mapped.
>
>
> They just moved the derived state into the MemoryRegion, no?
>

They do not.  We had this state in three places.  memory/mutators folds
->flash_mapped and the MemoryRegion equivalent; they both still mirror
the real device register.

If we had an Observable interface for Registers, then we could make any
write to the Register automatically update the MemoryRegion; as it is,
we have to call device_update_mapping() after every write.

>> Yes, ISR fields often have it (like virtio).
>
> Yes, but virtio-pci was a very special case to avoid taking an extra
> exit.
>
> Do you know of any other than virtio-pci?  All the ones I can think of
> (RTC, Serial, etc.) are cleared with a write.

Can't think of any offhand, but see
http://verificationguild.com/modules.php?name=Forums&file=viewtopic&p=5724. 
Anyway, if something turns out not to be useful, we don't have to keep
it in the core.

>> This style of code really wants lambdas.  Without them, we have 4-5
>> lines of boilerplate for each callback.  Even then, it's worthwhile IMO
>> (and many callbacks can be avoided, both read and write, or merged into
>> a device_update_mapping or device_update_irq read-all-state style
>> functions).
>
>
> Yeah, I looked at this but wasn't happy with the results.  In
> practice, many devices end up implementing non-trivial logic when
> register values change.
>
> What I was really interested in was coming up with a way to get really
> high quality tracing of device register accesses.

A Register can still dispatch to a common dispatch function.

Another thing I'm thinking of is wrapping addr/size/value in a
Transaction object, to keep the signatures trim.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

[Qemu-devel] [PATCH 5/5] integratorcp: convert icp_pic to VMState

[Benoît Canet]

Signed-off-by: Benoit Canet <benoit.canet@gmail.com>
---
 hw/integratorcp.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/hw/integratorcp.c b/hw/integratorcp.c
index e8d8d67..10475be 100644
--- a/hw/integratorcp.c
+++ b/hw/integratorcp.c
@@ -314,6 +314,19 @@ typedef struct icp_pic_state
   qemu_irq parent_fiq;
 } icp_pic_state;
 
+static const VMStateDescription vmstate_icp_pic = {
+    .name = "pic",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(level, icp_pic_state),
+        VMSTATE_UINT32(irq_enabled, icp_pic_state),
+        VMSTATE_UINT32(fiq_enabled, icp_pic_state),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void icp_pic_update(icp_pic_state *s)
 {
     uint32_t flags;
@@ -423,6 +436,7 @@ static int icp_pic_init(SysBusDevice *dev)
                                        icp_pic_writefn, s,
                                        DEVICE_NATIVE_ENDIAN);
     sysbus_init_mmio(dev, 0x00800000, iomemtype);
+    vmstate_register(&dev->qdev, -1, &vmstate_icp_pic, s);
     return 0;
 }
 
-- 
1.7.5.4