[Qemu-devel] [PATCH 0/4] Fix VMState design flaws

[Qemu-devel] [PATCH 0/4] Fix VMState design flaws

[Juan Quintela]

Hi

I know you are not going to believe it, but VMState design have flaws:

(everybody) Ooohhhhhhh!!!!

This patches fixes (some) of them:

  * When we added .pre_save() and .post_save(), we have it basically
    to change values in the variable pointed by the "void *opaque".
    Without this functions, it made sense that this variables were const,
    with them, it means that we have to do
      FooState *s = (void *)opaque
    in every pre/post_save() function.  Just remove the const.
  * Add version_id field to post_load().  Now we can assign default values
    in post_load for old versions of the state (ps2_kbd as example).
    This one is also needed for ioapic in qemu-kvm.
  * Add support for sending partial struct arrays (i.e. only some fields starting
    from the beggining) fdc cleanups for getting pc98 in need it.

That is all for now :)

Later, Juan.

Juan Quintela (4):
  vmstate: remove const for put operations
  vmstate: add version_id argument to post_load
  vmstate: remove ps2_kbd_load_old()
  vmstate: Add support for sending partial arrays

 exec.c             |    2 +-
 hw/acpi.c          |    2 +-
 hw/cirrus_vga.c    |    2 +-
 hw/dma.c           |    2 +-
 hw/fdc.c           |    2 +-
 hw/hpet.c          |    2 +-
 hw/hw.h            |   18 +++++++++++++++---
 hw/pci.c           |    2 +-
 hw/piix_pci.c      |    2 +-
 hw/ps2.c           |   14 +++-----------
 hw/ptimer.c        |    4 ++--
 hw/serial.c        |    2 +-
 hw/slavio_intctl.c |    2 +-
 hw/tcx.c           |    2 +-
 hw/vmmouse.c       |    2 +-
 savevm.c           |   48 ++++++++++++++++++++++++------------------------
 16 files changed, 56 insertions(+), 52 deletions(-)

[Qemu-devel] [PATCH 1/4] vmstate: remove const for put operations

[Juan Quintela]

In a later patch, we introduce pre_save() and post_save() functions.
The whole point of that operation is to change things in the state.
Without this patch, we have to remove the const qualifier in each
use with a cast

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/hw.h     |    4 ++--
 hw/pci.c    |    2 +-
 hw/ptimer.c |    4 ++--
 savevm.c    |   46 +++++++++++++++++++++++-----------------------
 4 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/hw/hw.h b/hw/hw.h
index cf266b3..e407815 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -276,7 +276,7 @@ typedef struct VMStateDescription VMStateDescription;
 struct VMStateInfo {
     const char *name;
     int (*get)(QEMUFile *f, void *pv, size_t size);
-    void (*put)(QEMUFile *f, const void *pv, size_t size);
+    void (*put)(QEMUFile *f, void *pv, size_t size);
 };

 enum VMStateFlags {
@@ -534,7 +534,7 @@ extern const VMStateDescription vmstate_pci_device;
 extern int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
                               void *opaque, int version_id);
 extern void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
-                               const void *opaque);
+                               void *opaque);
 extern int vmstate_register(int instance_id, const VMStateDescription *vmsd,
                             void *base);
 void vmstate_unregister(const VMStateDescription *vmsd, void *opaque);
diff --git a/hw/pci.c b/hw/pci.c
index c12b0be..7d7d619 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -156,7 +156,7 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)
 }

 /* just put buffer */
-static void put_pci_config_device(QEMUFile *f, const void *pv, size_t size)
+static void put_pci_config_device(QEMUFile *f, void *pv, size_t size)
 {
     const uint8_t *v = pv;
     qemu_put_buffer(f, v, size);
diff --git a/hw/ptimer.c b/hw/ptimer.c
index a4343b6..4ddbc59 100644
--- a/hw/ptimer.c
+++ b/hw/ptimer.c
@@ -220,9 +220,9 @@ static int get_ptimer(QEMUFile *f, void *pv, size_t size)
     return 0;
 }

-static void put_ptimer(QEMUFile *f, const void *pv, size_t size)
+static void put_ptimer(QEMUFile *f, void *pv, size_t size)
 {
-    ptimer_state *v = (void *)pv;
+    ptimer_state *v = pv;

     qemu_put_ptimer(f, v);
 }
diff --git a/savevm.c b/savevm.c
index fd767be..b36c657 100644
--- a/savevm.c
+++ b/savevm.c
@@ -649,9 +649,9 @@ static int get_int8(QEMUFile *f, void *pv, size_t size)
     return 0;
 }

-static void put_int8(QEMUFile *f, const void *pv, size_t size)
+static void put_int8(QEMUFile *f, void *pv, size_t size)
 {
-    const int8_t *v = pv;
+    int8_t *v = pv;
     qemu_put_s8s(f, v);
 }

@@ -670,9 +670,9 @@ static int get_int16(QEMUFile *f, void *pv, size_t size)
     return 0;
 }

-static void put_int16(QEMUFile *f, const void *pv, size_t size)
+static void put_int16(QEMUFile *f, void *pv, size_t size)
 {
-    const int16_t *v = pv;
+    int16_t *v = pv;
     qemu_put_sbe16s(f, v);
 }

@@ -691,9 +691,9 @@ static int get_int32(QEMUFile *f, void *pv, size_t size)
     return 0;
 }

-static void put_int32(QEMUFile *f, const void *pv, size_t size)
+static void put_int32(QEMUFile *f, void *pv, size_t size)
 {
-    const int32_t *v = pv;
+    int32_t *v = pv;
     qemu_put_sbe32s(f, v);
 }

@@ -752,9 +752,9 @@ static int get_int64(QEMUFile *f, void *pv, size_t size)
     return 0;
 }

-static void put_int64(QEMUFile *f, const void *pv, size_t size)
+static void put_int64(QEMUFile *f, void *pv, size_t size)
 {
-    const int64_t *v = pv;
+    int64_t *v = pv;
     qemu_put_sbe64s(f, v);
 }

@@ -773,9 +773,9 @@ static int get_uint8(QEMUFile *f, void *pv, size_t size)
     return 0;
 }

-static void put_uint8(QEMUFile *f, const void *pv, size_t size)
+static void put_uint8(QEMUFile *f, void *pv, size_t size)
 {
-    const uint8_t *v = pv;
+    uint8_t *v = pv;
     qemu_put_8s(f, v);
 }

@@ -794,9 +794,9 @@ static int get_uint16(QEMUFile *f, void *pv, size_t size)
     return 0;
 }

-static void put_uint16(QEMUFile *f, const void *pv, size_t size)
+static void put_uint16(QEMUFile *f, void *pv, size_t size)
 {
-    const uint16_t *v = pv;
+    uint16_t *v = pv;
     qemu_put_be16s(f, v);
 }

@@ -815,9 +815,9 @@ static int get_uint32(QEMUFile *f, void *pv, size_t size)
     return 0;
 }

-static void put_uint32(QEMUFile *f, const void *pv, size_t size)
+static void put_uint32(QEMUFile *f, void *pv, size_t size)
 {
-    const uint32_t *v = pv;
+    uint32_t *v = pv;
     qemu_put_be32s(f, v);
 }

@@ -836,9 +836,9 @@ static int get_uint64(QEMUFile *f, void *pv, size_t size)
     return 0;
 }

-static void put_uint64(QEMUFile *f, const void *pv, size_t size)
+static void put_uint64(QEMUFile *f, void *pv, size_t size)
 {
-    const uint64_t *v = pv;
+    uint64_t *v = pv;
     qemu_put_be64s(f, v);
 }

@@ -877,9 +877,9 @@ static int get_timer(QEMUFile *f, void *pv, size_t size)
     return 0;
 }

-static void put_timer(QEMUFile *f, const void *pv, size_t size)
+static void put_timer(QEMUFile *f, void *pv, size_t size)
 {
-    QEMUTimer *v = (void *)pv;
+    QEMUTimer *v = pv;
     qemu_put_timer(f, v);
 }

@@ -898,9 +898,9 @@ static int get_buffer(QEMUFile *f, void *pv, size_t size)
     return 0;
 }

-static void put_buffer(QEMUFile *f, const void *pv, size_t size)
+static void put_buffer(QEMUFile *f, void *pv, size_t size)
 {
-    uint8_t *v = (void *)pv;
+    uint8_t *v = pv;
     qemu_put_buffer(f, v, size);
 }

@@ -1090,7 +1090,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
 }

 void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
-                        const void *opaque)
+                        void *opaque)
 {
     VMStateField *field = vmsd->fields;

@@ -1098,7 +1098,7 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
         vmsd->pre_save(opaque);
     }
     while(field->name) {
-        const void *base_addr = opaque + field->offset;
+        void *base_addr = opaque + field->offset;
         int i, n_elems = 1;

         if (field->flags & VMS_ARRAY) {
@@ -1110,7 +1110,7 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
             base_addr = *(void **)base_addr;
         }
         for (i = 0; i < n_elems; i++) {
-            const void *addr = base_addr + field->size * i;
+            void *addr = base_addr + field->size * i;

             if (field->flags & VMS_STRUCT) {
                 vmstate_save_state(f, field->vmsd, addr);
-- 
1.6.2.5

[Qemu-devel] [PATCH 2/4] vmstate: add version_id argument to post_load

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 exec.c             |    2 +-
 hw/acpi.c          |    2 +-
 hw/cirrus_vga.c    |    2 +-
 hw/dma.c           |    2 +-
 hw/fdc.c           |    2 +-
 hw/hpet.c          |    2 +-
 hw/hw.h            |    2 +-
 hw/piix_pci.c      |    2 +-
 hw/serial.c        |    2 +-
 hw/slavio_intctl.c |    2 +-
 hw/tcx.c           |    2 +-
 hw/vmmouse.c       |    2 +-
 savevm.c           |    2 +-
 13 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/exec.c b/exec.c
index c82e767..85076e6 100644
--- a/exec.c
+++ b/exec.c
@@ -528,7 +528,7 @@ static int cpu_common_pre_load(void *opaque)
     return 0;
 }

-static int cpu_common_post_load(void *opaque)
+static int cpu_common_post_load(void *opaque, int version_id)
 {
     CPUState *env = opaque;

diff --git a/hw/acpi.c b/hw/acpi.c
index b14b9f4..e67da6c 100644
--- a/hw/acpi.c
+++ b/hw/acpi.c
@@ -441,7 +441,7 @@ static void pm_write_config(PCIDevice *d,
         pm_io_space_update((PIIX4PMState *)d);
 }

-static int vmstate_acpi_post_load(void *opaque)
+static int vmstate_acpi_post_load(void *opaque, int version_id)
 {
     PIIX4PMState *s = opaque;

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index 7e95f10..004ae7d 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -2955,7 +2955,7 @@ static CPUWriteMemoryFunc * const cirrus_mmio_write[3] = {

 /* load/save state */

-static int cirrus_post_load(void *opaque)
+static int cirrus_post_load(void *opaque, int version_id)
 {
     CirrusVGAState *s = opaque;

diff --git a/hw/dma.c b/hw/dma.c
index f418e42..44c642e 100644
--- a/hw/dma.c
+++ b/hw/dma.c
@@ -517,7 +517,7 @@ static const VMStateDescription vmstate_dma_regs = {
     }
 };

-static int dma_post_load(void *opaque)
+static int dma_post_load(void *opaque, int version_id)
 {
     DMA_run();

diff --git a/hw/fdc.c b/hw/fdc.c
index 389d9e6..c03ab47 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -650,7 +650,7 @@ static void fdc_pre_save(const void *opaque)
     s->dor_vmstate = s->dor | GET_CUR_DRV(s);
 }

-static int fdc_post_load(void *opaque)
+static int fdc_post_load(void *opaque, int version_id)
 {
     fdctrl_t *s = opaque;

diff --git a/hw/hpet.c b/hw/hpet.c
index c1ead34..6535b8e 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -164,7 +164,7 @@ static void hpet_pre_save(const void *opaque)
     s->hpet_counter = hpet_get_ticks();
 }

-static int hpet_post_load(void *opaque)
+static int hpet_post_load(void *opaque, int version_id)
 {
     HPETState *s = opaque;

diff --git a/hw/hw.h b/hw/hw.h
index e407815..6f60493 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -307,7 +307,7 @@ struct VMStateDescription {
     int minimum_version_id_old;
     LoadStateHandler *load_state_old;
     int (*pre_load)(void *opaque);
-    int (*post_load)(void *opaque);
+    int (*post_load)(void *opaque, int version_id);
     void (*pre_save)(const void *opaque);
     void (*post_save)(const void *opaque);
     VMStateField *fields;
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index edd6df0..5c2bb92 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -172,7 +172,7 @@ static int i440fx_load_old(QEMUFile* f, void *opaque, int version_id)
     return 0;
 }

-static int i440fx_post_load(void *opaque)
+static int i440fx_post_load(void *opaque, int version_id)
 {
     PCII440FXState *d = opaque;

diff --git a/hw/serial.c b/hw/serial.c
index 6f7b30e..ae14021 100644
--- a/hw/serial.c
+++ b/hw/serial.c
@@ -650,7 +650,7 @@ static int serial_pre_load(void *opaque)
     return 0;
 }

-static int serial_post_load(void *opaque)
+static int serial_post_load(void *opaque, int version_id)
 {
     SerialState *s = opaque;

diff --git a/hw/slavio_intctl.c b/hw/slavio_intctl.c
index 6a95f9e..ab29ee2 100644
--- a/hw/slavio_intctl.c
+++ b/hw/slavio_intctl.c
@@ -374,7 +374,7 @@ static void slavio_set_irq_all(void *opaque, int irq, int level)
     }
 }

-static int vmstate_intctl_post_load(void *opaque)
+static int vmstate_intctl_post_load(void *opaque, int version_id)
 {
     SLAVIO_INTCTLState *s = opaque;

diff --git a/hw/tcx.c b/hw/tcx.c
index 012d01b..3816c53 100644
--- a/hw/tcx.c
+++ b/hw/tcx.c
@@ -378,7 +378,7 @@ static void tcx24_invalidate_display(void *opaque)
     qemu_console_resize(s->ds, s->width, s->height);
 }

-static int vmstate_tcx_post_load(void *opaque)
+static int vmstate_tcx_post_load(void *opaque, int version_id)
 {
     TCXState *s = opaque;

diff --git a/hw/vmmouse.c b/hw/vmmouse.c
index c207bb2..bb6e605 100644
--- a/hw/vmmouse.c
+++ b/hw/vmmouse.c
@@ -235,7 +235,7 @@ static uint32_t vmmouse_ioport_read(void *opaque, uint32_t addr)
     return data[0];
 }

-static int vmmouse_post_load(void *opaque)
+static int vmmouse_post_load(void *opaque, int version_id)
 {
     VMMouseState *s = opaque;

diff --git a/savevm.c b/savevm.c
index b36c657..fefde7c 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1084,7 +1084,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
         field++;
     }
     if (vmsd->post_load) {
-        return vmsd->post_load(opaque);
+        return vmsd->post_load(opaque, version_id);
     }
     return 0;
 }
-- 
1.6.2.5

[Qemu-devel] [PATCH 3/4] vmstate: remove ps2_kbd_load_old()

[Juan Quintela]

Now that we have version_id on post_load() we don't need the old load
function

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/ps2.c |   14 +++-----------
 1 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/hw/ps2.c b/hw/ps2.c
index fff4d13..6fe4b59 100644
--- a/hw/ps2.c
+++ b/hw/ps2.c
@@ -541,19 +541,11 @@ static const VMStateDescription vmstate_ps2_common = {
     }
 };

-static int ps2_kbd_load_old(QEMUFile* f, void* opaque, int version_id)
+static int ps2_kbd_post_load(void* opaque, int version_id)
 {
     PS2KbdState *s = (PS2KbdState*)opaque;

-    if (version_id != 2 && version_id != 3)
-        return -EINVAL;
-
-    vmstate_load_state(f, &vmstate_ps2_common, &s->common, version_id);
-    s->scan_enabled=qemu_get_be32(f);
-    s->translate=qemu_get_be32(f);
-    if (version_id == 3)
-        s->scancode_set=qemu_get_be32(f);
-    else
+    if (version_id == 2)
         s->scancode_set=2;
     return 0;
 }
@@ -563,7 +555,7 @@ static const VMStateDescription vmstate_ps2_keyboard = {
     .version_id = 3,
     .minimum_version_id = 3,
     .minimum_version_id_old = 2,
-    .load_state_old = ps2_kbd_load_old,
+    .post_load = ps2_kbd_post_load,
     .fields      = (VMStateField []) {
         VMSTATE_STRUCT(common, PS2KbdState, 0, vmstate_ps2_common, PS2State),
         VMSTATE_INT32(scan_enabled, PS2KbdState),
-- 
1.6.2.5

[Qemu-devel] Re: [PATCH 3/4] vmstate: remove ps2_kbd_load_old()

[Paolo Bonzini]

On 09/14/2009 10:15 PM, Juan Quintela wrote:
>     .minimum_version_id = 3,

This must become 2.

Paolo

[Qemu-devel] Re: [PATCH 3/4] vmstate: remove ps2_kbd_load_old()

[Juan Quintela]

Paolo Bonzini <bonzini@gnu.org> wrote:
> On 09/14/2009 10:15 PM, Juan Quintela wrote:
>>     .minimum_version_id = 3,
>
> This must become 2.

you are right.

Later, Juan.

[Qemu-devel] [PATCH 4/4] vmstate: Add support for sending partial arrays

[Juan Quintela]

This one is needed for changees happening on fdc.  It allows you to send
arrays of structs whose size we want to send it is another field with type
uint8_t.  (If you have been able to read the whole sentence without
stoping for breathing, you can use it.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/hw.h |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/hw/hw.h b/hw/hw.h
index 6f60493..823c132 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -398,6 +398,18 @@ extern const VMStateInfo vmstate_info_buffer;
         + type_check_array(_type,typeof_field(_state, _field),_num)  \
 }

+#define VMSTATE_STRUCT_ARRAY_SIZE_UINT8(_field, _state, _field__num, _version, _vmsd, _type) { \
+    .name       = (stringify(_field)),                               \
+    .num_offset = offsetof(_state, _field_num)                       \
+        + type_check(uint8_t,typeof_field(_state, _field_num)),       \
+    .version_id = (_version),                                        \
+    .vmsd       = &(_vmsd),                                          \
+    .size       = sizeof(_type),                                     \
+    .flags      = VMS_STRUCT|VMS_ARRAY,                              \
+    .offset     = offsetof(_state, _field)                           \
+        + type_check_array(_type,typeof_field(_state, _field),_num)  \
+}
+
 #define VMSTATE_STATIC_BUFFER(_field, _state, _version) {            \
     .name       = (stringify(_field)),                               \
     .version_id = (_version),                                        \
-- 
1.6.2.5

[Qemu-devel] Re: [PATCH 0/4] Fix VMState design flaws

[Paolo Bonzini]

>    * Add version_id field to post_load().  Now we can assign default values
>      in post_load for old versions of the state (ps2_kbd as example).

While this is a good idea, why don't we first reset the devices upon 
load, which would also set the default values?...

Paolo

[Qemu-devel] Re: [PATCH 0/4] Fix VMState design flaws

[Juan Quintela]

Paolo Bonzini <bonzini@gnu.org> wrote:
>>    * Add version_id field to post_load().  Now we can assign default values
>>      in post_load for old versions of the state (ps2_kbd as example).
>
> While this is a good idea, why don't we first reset the devices upon
> load, which would also set the default values?...

It is safe to do that unconditionally?
If so, I am all for it.  We can:
- add reset field to vmstate
- add default values to some fields, if it makse sense (haven't yet
  looked at reset code).

Later, Juan.

[Qemu-devel] Re: [PATCH 0/4] Fix VMState design flaws

[Paolo Bonzini]

On 09/14/2009 10:50 PM, Juan Quintela wrote:
> Paolo Bonzini<bonzini@gnu.org>  wrote:
>>>     * Add version_id field to post_load().  Now we can assign default values
>>>       in post_load for old versions of the state (ps2_kbd as example).
>>
>> While this is a good idea, why don't we first reset the devices upon
>> load, which would also set the default values?...
>
> It is safe to do that unconditionally?
> If so, I am all for it.  We can:
> - add reset field to vmstate
> - add default values to some fields, if it makse sense (haven't yet
>    looked at reset code).

The PS2 reset is this:

static void ps2_common_reset(PS2State *s)
{
     PS2Queue *q;
     s->write_cmd = -1;
     q = &s->queue;
     q->rptr = 0;
     q->wptr = 0;
     q->count = 0;
     s->update_irq(s->update_arg, 0); // calls kbd_update_kbd_irq
}

static void ps2_kbd_reset(void *opaque)
{
     PS2KbdState *s = (PS2KbdState *) opaque;

     ps2_common_reset(&s->common);
     s->scan_enabled = 0;
     s->translate = 0;
     s->scancode_set = 0;
}

All of these are later reset by load.  The update_irq may seem unsafe, 
but maybe deasserting an IRQ is better than leaving it in whatever state 
it was left by the previous VM...

Paolo