[Qemu-devel] [PATCH V2 0/5] VMState cleanups

[Qemu-devel] [PATCH V2 0/5] VMState cleanups

[Igor Mitsyanko]

This patchset cleans up and optimizes vmstate implementation.

v1-v2
PATCH 4 now moves VMSTATE_UINTTL* macros to cpu-defs.h instead of to vmstate.h.
put_target_ul/get_target_ul introduction is dropped.
PATCH2,PATCH3 commit messages are rewritten a little bit. But I'm still
not sure whether Andrzej Zaborowski agreed with these changes or not.


Patch 1 is a trivial bug fixing.
Patches 2 and 3 replaces target_phys_addr_t in pxa implementation
to uint32_t.
Patch 4 moves VMSTATE_UINTTL from hw/hw.h to cpu-defs.h.
Patch 5 introduces mechanism to save variable-size buffers.

Igor Mitsyanko (5):
  target-alpha/machine.c: use VMSTATE_UINT64* instead of
    VMSTATE_UINTTL*
  hw/pxa2xx_dma.c: drop target_phys_addr_t usage in device state
  hw/pxa2xx_lcd.c: drop target_phys_addr_t usage in device state
  vmstate: move VMSTATE_UINTTL* macros definitions to cpu-defs.h
  vmstate: introduce get_bufsize entry in VMStateField

 cpu-defs.h             |   13 +++++++++++++
 hw/exynos4210_uart.c   |   10 +++++++++-
 hw/g364fb.c            |    7 ++++++-
 hw/hw.h                |   19 -------------------
 hw/m48t59.c            |    7 ++++++-
 hw/mac_nvram.c         |    8 +++++++-
 hw/onenand.c           |    7 ++++++-
 hw/pxa2xx_dma.c        |   12 ++++++------
 hw/pxa2xx_lcd.c        |   12 ++++++------
 savevm.c               |   18 ++++++++++++------
 target-alpha/machine.c |   34 +++++++++++++++++-----------------
 vmstate.h              |   43 ++++++++-----------------------------------
 12 files changed, 96 insertions(+), 94 deletions(-)

-- 
1.7.4.1

[Qemu-devel] [PATCH V2 1/5] target-alpha/machine.c: use VMSTATE_UINT64* instead of VMSTATE_UINTTL*

[Igor Mitsyanko]

Do not use VMSTATE_UINTTL* macro for variables that are actually defined as uint64_t
in CPUAlphaState.

Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
---
 target-alpha/machine.c |   34 +++++++++++++++++-----------------
 1 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/target-alpha/machine.c b/target-alpha/machine.c
index 76d70d9..f1eef3d 100644
--- a/target-alpha/machine.c
+++ b/target-alpha/machine.c
@@ -21,8 +21,8 @@ static const VMStateInfo vmstate_fpcr = {
 };
 
 static VMStateField vmstate_cpu_fields[] = {
-    VMSTATE_UINTTL_ARRAY(ir, CPUState, 31),
-    VMSTATE_UINTTL_ARRAY(fir, CPUState, 31),
+    VMSTATE_UINT64_ARRAY(ir, CPUState, 31),
+    VMSTATE_UINT64_ARRAY(fir, CPUState, 31),
     /* Save the architecture value of the fpcr, not the internally
        expanded version.  Since this architecture value does not
        exist in memory to be stored, this requires a but of hoop
@@ -37,10 +37,10 @@ static VMStateField vmstate_cpu_fields[] = {
         .flags = VMS_SINGLE,
         .offset = 0
     },
-    VMSTATE_UINTTL(pc, CPUState),
-    VMSTATE_UINTTL(unique, CPUState),
-    VMSTATE_UINTTL(lock_addr, CPUState),
-    VMSTATE_UINTTL(lock_value, CPUState),
+    VMSTATE_UINT64(pc, CPUState),
+    VMSTATE_UINT64(unique, CPUState),
+    VMSTATE_UINT64(lock_addr, CPUState),
+    VMSTATE_UINT64(lock_value, CPUState),
     /* Note that lock_st_addr is not saved; it is a temporary
        used during the execution of the st[lq]_c insns.  */
 
@@ -51,19 +51,19 @@ static VMStateField vmstate_cpu_fields[] = {
 
     VMSTATE_UINT32(pcc_ofs, CPUState),
 
-    VMSTATE_UINTTL(trap_arg0, CPUState),
-    VMSTATE_UINTTL(trap_arg1, CPUState),
-    VMSTATE_UINTTL(trap_arg2, CPUState),
+    VMSTATE_UINT64(trap_arg0, CPUState),
+    VMSTATE_UINT64(trap_arg1, CPUState),
+    VMSTATE_UINT64(trap_arg2, CPUState),
 
-    VMSTATE_UINTTL(exc_addr, CPUState),
-    VMSTATE_UINTTL(palbr, CPUState),
-    VMSTATE_UINTTL(ptbr, CPUState),
-    VMSTATE_UINTTL(vptptr, CPUState),
-    VMSTATE_UINTTL(sysval, CPUState),
-    VMSTATE_UINTTL(usp, CPUState),
+    VMSTATE_UINT64(exc_addr, CPUState),
+    VMSTATE_UINT64(palbr, CPUState),
+    VMSTATE_UINT64(ptbr, CPUState),
+    VMSTATE_UINT64(vptptr, CPUState),
+    VMSTATE_UINT64(sysval, CPUState),
+    VMSTATE_UINT64(usp, CPUState),
 
-    VMSTATE_UINTTL_ARRAY(shadow, CPUState, 8),
-    VMSTATE_UINTTL_ARRAY(scratch, CPUState, 24),
+    VMSTATE_UINT64_ARRAY(shadow, CPUState, 8),
+    VMSTATE_UINT64_ARRAY(scratch, CPUState, 24),
 
     VMSTATE_END_OF_LIST()
 };
-- 
1.7.4.1

[Qemu-devel] [PATCH V2 2/5] hw/pxa2xx_dma.c: drop target_phys_addr_t usage in device state

[Igor Mitsyanko]

Pxa2xx DMA controller is a 32-bit device and it has no knowledge of system's
physical address size, so it should not use target_phys_addr_t in it's state.
Convert variables descr, src and dest from type target_phys_addr_t to uint32_t,
use VMSTATE_UINT32 instead of VMSTATE_UINTTL for these variables.

We can do this safely because:
1) pxa2xx actually has 32-bit physical address size;
2) rest of the code in file never assumes descr, src and dest variables to have
size different from uint32_t;
3) we shouldn't have used VMSTATE_UINTTL in the first place because this macro
is for target_ulong type (which can be different from target_phys_addr_t).

Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/pxa2xx_dma.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/pxa2xx_dma.c b/hw/pxa2xx_dma.c
index 8ced0dd..0310154 100644
--- a/hw/pxa2xx_dma.c
+++ b/hw/pxa2xx_dma.c
@@ -18,9 +18,9 @@
 #define PXA2XX_DMA_NUM_REQUESTS 75
 
 typedef struct {
-    target_phys_addr_t descr;
-    target_phys_addr_t src;
-    target_phys_addr_t dest;
+    uint32_t descr;
+    uint32_t src;
+    uint32_t dest;
     uint32_t cmd;
     uint32_t state;
     int request;
@@ -512,9 +512,9 @@ static VMStateDescription vmstate_pxa2xx_dma_chan = {
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
     .fields = (VMStateField[]) {
-        VMSTATE_UINTTL(descr, PXA2xxDMAChannel),
-        VMSTATE_UINTTL(src, PXA2xxDMAChannel),
-        VMSTATE_UINTTL(dest, PXA2xxDMAChannel),
+        VMSTATE_UINT32(descr, PXA2xxDMAChannel),
+        VMSTATE_UINT32(src, PXA2xxDMAChannel),
+        VMSTATE_UINT32(dest, PXA2xxDMAChannel),
         VMSTATE_UINT32(cmd, PXA2xxDMAChannel),
         VMSTATE_UINT32(state, PXA2xxDMAChannel),
         VMSTATE_INT32(request, PXA2xxDMAChannel),
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH V2 2/5] hw/pxa2xx_dma.c: drop target_phys_addr_t usage in device state

[Andreas Färber]

Am 05.03.2012 09:30, schrieb Igor Mitsyanko:
> Pxa2xx DMA controller is a 32-bit device and it has no knowledge of system's
> physical address size, so it should not use target_phys_addr_t in it's state.
> Convert variables descr, src and dest from type target_phys_addr_t to uint32_t,
> use VMSTATE_UINT32 instead of VMSTATE_UINTTL for these variables.
> 
> We can do this safely because:
> 1) pxa2xx actually has 32-bit physical address size;

> 2) rest of the code in file never assumes descr, src and dest variables to have
> size different from uint32_t;
> 3) we shouldn't have used VMSTATE_UINTTL in the first place because this macro
> is for target_ulong type (which can be different from target_phys_addr_t).

Agreed.

> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Andreas Färber <afaerber@suse.de>

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH V2 2/5] hw/pxa2xx_dma.c: drop target_phys_addr_t usage in device state

[Michael Roth]

On Mon, Mar 05, 2012 at 12:30:42PM +0400, Igor Mitsyanko wrote:
> Pxa2xx DMA controller is a 32-bit device and it has no knowledge of system's
> physical address size, so it should not use target_phys_addr_t in it's state.
> Convert variables descr, src and dest from type target_phys_addr_t to uint32_t,
> use VMSTATE_UINT32 instead of VMSTATE_UINTTL for these variables.
> 
> We can do this safely because:
> 1) pxa2xx actually has 32-bit physical address size;
> 2) rest of the code in file never assumes descr, src and dest variables to have
> size different from uint32_t;
> 3) we shouldn't have used VMSTATE_UINTTL in the first place because this macro
> is for target_ulong type (which can be different from target_phys_addr_t).

4) target-arm is the only user of pxa2xx*, and has never defined
target_ulong/TARGET_LONG_BITS to be anything other than 32-bits.

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> 
> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/pxa2xx_dma.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/pxa2xx_dma.c b/hw/pxa2xx_dma.c
> index 8ced0dd..0310154 100644
> --- a/hw/pxa2xx_dma.c
> +++ b/hw/pxa2xx_dma.c
> @@ -18,9 +18,9 @@
>  #define PXA2XX_DMA_NUM_REQUESTS 75
> 
>  typedef struct {
> -    target_phys_addr_t descr;
> -    target_phys_addr_t src;
> -    target_phys_addr_t dest;
> +    uint32_t descr;
> +    uint32_t src;
> +    uint32_t dest;
>      uint32_t cmd;
>      uint32_t state;
>      int request;
> @@ -512,9 +512,9 @@ static VMStateDescription vmstate_pxa2xx_dma_chan = {
>      .minimum_version_id = 1,
>      .minimum_version_id_old = 1,
>      .fields = (VMStateField[]) {
> -        VMSTATE_UINTTL(descr, PXA2xxDMAChannel),
> -        VMSTATE_UINTTL(src, PXA2xxDMAChannel),
> -        VMSTATE_UINTTL(dest, PXA2xxDMAChannel),
> +        VMSTATE_UINT32(descr, PXA2xxDMAChannel),
> +        VMSTATE_UINT32(src, PXA2xxDMAChannel),
> +        VMSTATE_UINT32(dest, PXA2xxDMAChannel),
>          VMSTATE_UINT32(cmd, PXA2xxDMAChannel),
>          VMSTATE_UINT32(state, PXA2xxDMAChannel),
>          VMSTATE_INT32(request, PXA2xxDMAChannel),
> -- 
> 1.7.4.1
> 
> 

[Qemu-devel] [PATCH V2 3/5] hw/pxa2xx_lcd.c: drop target_phys_addr_t usage in device state

[Igor Mitsyanko]

Pxa2xx LCD controller is intended to work with 32-bit bus and it has no knowledge
of system's physical address size, so it should not use target_phys_addr_t in it's
state. Convert three variables in DMAChannel state from target_phys_addr_t to uint32_t,
use VMSTATE_UINT32 instead of VMSTATE_UINTTL for these variables.
We can do this safely because:
1) pxa2xx has 32-bit physical address;
2) rest of the code in file never assumes converted variables to have any size
different from uint32_t;
3) we shouldn't have used VMSTATE_UINTTL in the first place because this macro
is for target_ulong type (which can be different from target_phys_addr_t).

Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/pxa2xx_lcd.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/pxa2xx_lcd.c b/hw/pxa2xx_lcd.c
index fcbdfb3..ee8bf57 100644
--- a/hw/pxa2xx_lcd.c
+++ b/hw/pxa2xx_lcd.c
@@ -19,15 +19,15 @@
 #include "framebuffer.h"
 
 struct DMAChannel {
-    target_phys_addr_t branch;
+    uint32_t branch;
     uint8_t up;
     uint8_t palette[1024];
     uint8_t pbuffer[1024];
     void (*redraw)(PXA2xxLCDState *s, target_phys_addr_t addr,
                    int *miny, int *maxy);
 
-    target_phys_addr_t descriptor;
-    target_phys_addr_t source;
+    uint32_t descriptor;
+    uint32_t source;
     uint32_t id;
     uint32_t command;
 };
@@ -929,11 +929,11 @@ static const VMStateDescription vmstate_dma_channel = {
     .minimum_version_id = 0,
     .minimum_version_id_old = 0,
     .fields      = (VMStateField[]) {
-        VMSTATE_UINTTL(branch, struct DMAChannel),
+        VMSTATE_UINT32(branch, struct DMAChannel),
         VMSTATE_UINT8(up, struct DMAChannel),
         VMSTATE_BUFFER(pbuffer, struct DMAChannel),
-        VMSTATE_UINTTL(descriptor, struct DMAChannel),
-        VMSTATE_UINTTL(source, struct DMAChannel),
+        VMSTATE_UINT32(descriptor, struct DMAChannel),
+        VMSTATE_UINT32(source, struct DMAChannel),
         VMSTATE_UINT32(id, struct DMAChannel),
         VMSTATE_UINT32(command, struct DMAChannel),
         VMSTATE_END_OF_LIST()
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH V2 3/5] hw/pxa2xx_lcd.c: drop target_phys_addr_t usage in device state

[Andreas Färber]

Am 05.03.2012 09:30, schrieb Igor Mitsyanko:
> Pxa2xx LCD controller is intended to work with 32-bit bus and it has no knowledge
> of system's physical address size, so it should not use target_phys_addr_t in it's
> state. Convert three variables in DMAChannel state from target_phys_addr_t to uint32_t,
> use VMSTATE_UINT32 instead of VMSTATE_UINTTL for these variables.
> We can do this safely because:
> 1) pxa2xx has 32-bit physical address;
> 2) rest of the code in file never assumes converted variables to have any size
> different from uint32_t;
> 3) we shouldn't have used VMSTATE_UINTTL in the first place because this macro
> is for target_ulong type (which can be different from target_phys_addr_t).
> 
> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Andreas Färber <afaerber@suse.de>

Some Coccinelle script or so to verify that also non-UINTTL fields match
types would probably be nice long-term.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PATCH V2 4/5] vmstate: move VMSTATE_UINTTL* macros definitions to cpu-defs.h

[Igor Mitsyanko]

VMSTATE_UINTTL* macros should be used only in files compiled with knowledge of
virtual address size of specific CPU. Currently this is achieved by protecting
VMSTATE_UINTTL* definitions with "#ifndef NEED_CPU_H" in hw/hw.h, but it would
be more appropriate to move these macros to cpu-defs.h header.

Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
---
 cpu-defs.h |   13 +++++++++++++
 hw/hw.h    |   19 -------------------
 2 files changed, 13 insertions(+), 19 deletions(-)

diff --git a/cpu-defs.h b/cpu-defs.h
index 4527cbf..10516e9 100644
--- a/cpu-defs.h
+++ b/cpu-defs.h
@@ -30,6 +30,8 @@
 #include "osdep.h"
 #include "qemu-queue.h"
 #include "targphys.h"
+#include "ioport.h"
+#include "vmstate.h"
 
 #ifndef TARGET_LONG_BITS
 #error TARGET_LONG_BITS must be defined before including this header
@@ -50,12 +52,20 @@ typedef uint32_t target_ulong __attribute__((aligned(TARGET_LONG_ALIGNMENT)));
 #define TARGET_FMT_lx "%08x"
 #define TARGET_FMT_ld "%d"
 #define TARGET_FMT_lu "%u"
+#define VMSTATE_UINTTL_V(_f, _s, _v)                                  \
+    VMSTATE_UINT32_V(_f, _s, _v)
+#define VMSTATE_UINTTL_ARRAY_V(_f, _s, _n, _v)                        \
+    VMSTATE_UINT32_ARRAY_V(_f, _s, _n, _v)
 #elif TARGET_LONG_SIZE == 8
 typedef int64_t target_long __attribute__((aligned(TARGET_LONG_ALIGNMENT)));
 typedef uint64_t target_ulong __attribute__((aligned(TARGET_LONG_ALIGNMENT)));
 #define TARGET_FMT_lx "%016" PRIx64
 #define TARGET_FMT_ld "%" PRId64
 #define TARGET_FMT_lu "%" PRIu64
+#define VMSTATE_UINTTL_V(_f, _s, _v)                                  \
+    VMSTATE_UINT64_V(_f, _s, _v)
+#define VMSTATE_UINTTL_ARRAY_V(_f, _s, _n, _v)                        \
+    VMSTATE_UINT64_ARRAY_V(_f, _s, _n, _v)
 #else
 #error TARGET_LONG_SIZE undefined
 #endif
@@ -68,6 +78,9 @@ typedef uint64_t target_ulong __attribute__((aligned(TARGET_LONG_ALIGNMENT)));
 #define TB_JMP_CACHE_BITS 12
 #define TB_JMP_CACHE_SIZE (1 << TB_JMP_CACHE_BITS)
 
+#define VMSTATE_UINTTL(_f, _s)           VMSTATE_UINTTL_V(_f, _s, 0)
+#define VMSTATE_UINTTL_ARRAY(_f, _s, _n) VMSTATE_UINTTL_ARRAY_V(_f, _s, _n, 0)
+
 /* Only the bottom TB_JMP_PAGE_BITS of the jump cache hash bits vary for
    addresses on the same page.  The top bits are the same.  This allows
    TLB invalidation to quickly clear a subset of the hash table.  */
diff --git a/hw/hw.h b/hw/hw.h
index e5cb9bf..fb66156 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -46,23 +46,4 @@ typedef int QEMUBootSetHandler(void *opaque, const char *boot_devices);
 void qemu_register_boot_set(QEMUBootSetHandler *func, void *opaque);
 int qemu_boot_set(const char *boot_devices);
 
-#ifdef NEED_CPU_H
-#if TARGET_LONG_BITS == 64
-#define VMSTATE_UINTTL_V(_f, _s, _v)                                  \
-    VMSTATE_UINT64_V(_f, _s, _v)
-#define VMSTATE_UINTTL_ARRAY_V(_f, _s, _n, _v)                        \
-    VMSTATE_UINT64_ARRAY_V(_f, _s, _n, _v)
-#else
-#define VMSTATE_UINTTL_V(_f, _s, _v)                                  \
-    VMSTATE_UINT32_V(_f, _s, _v)
-#define VMSTATE_UINTTL_ARRAY_V(_f, _s, _n, _v)                        \
-    VMSTATE_UINT32_ARRAY_V(_f, _s, _n, _v)
-#endif
-#define VMSTATE_UINTTL(_f, _s)                                        \
-    VMSTATE_UINTTL_V(_f, _s, 0)
-#define VMSTATE_UINTTL_ARRAY(_f, _s, _n)                              \
-    VMSTATE_UINTTL_ARRAY_V(_f, _s, _n, 0)
-
-#endif
-
 #endif
-- 
1.7.4.1

[Qemu-devel] [PATCH V2 5/5] vmstate: introduce get_bufsize entry in VMStateField

[Igor Mitsyanko]

New get_bufsize field in VMStateField is supposed to help us easily add save/restore
support of dynamically allocated buffers in device's states.
There are some cases when information about size of dynamically allocated buffer is
already presented in specific device's state structure, but in such a form that
can not be used with existing VMStateField interface. Currently, we either can get size from
another variable in device's state as it is with VMSTATE_VBUFFER_* macros, or we can
also multiply value kept in a variable by a constant with VMSTATE_BUFFER_MULTIPLY
macro. If we need to perform any other action, we're forced to add additional
variable with size information to device state structure with the only intention
to use it in VMStateDescription. This approach is not very pretty. Adding extra
flags to VMStateFlags enum for every other possible operation with size field
seems redundant, and still it would't cover cases when we need to perform a set of
operations to get size value.
With get_bufsize callback we can calculate size of dynamic array in whichever
way we need. We don't need .size_offset field anymore, so we can remove it from
VMState Field structure to compensate for extra memory consuption because of
get_bufsize addition. Macros VMSTATE_VBUFFER* are modified to use new callback
instead of .size_offset. Macro VMSTATE_BUFFER_MULTIPLY and VMFlag VMS_MULTIPLY
are removed completely as they are now redundant.

Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
---
 hw/exynos4210_uart.c |   10 +++++++++-
 hw/g364fb.c          |    7 ++++++-
 hw/m48t59.c          |    7 ++++++-
 hw/mac_nvram.c       |    8 +++++++-
 hw/onenand.c         |    7 ++++++-
 savevm.c             |   18 ++++++++++++------
 vmstate.h            |   43 ++++++++-----------------------------------
 7 files changed, 54 insertions(+), 46 deletions(-)

diff --git a/hw/exynos4210_uart.c b/hw/exynos4210_uart.c
index 73a9c18..cbd12e8 100644
--- a/hw/exynos4210_uart.c
+++ b/hw/exynos4210_uart.c
@@ -554,6 +554,13 @@ static void exynos4210_uart_reset(DeviceState *dev)
     PRINT_DEBUG("UART%d: Rx FIFO size: %d\n", s->channel, s->rx.size);
 }
 
+static size_t exynos4210_uart_get_datasize(void *opaque, int id)
+{
+    Exynos4210UartFIFO *s = (Exynos4210UartFIFO *)opaque;
+
+    return s->size;
+}
+
 static const VMStateDescription vmstate_exynos4210_uart_fifo = {
     .name = "exynos4210.uart.fifo",
     .version_id = 1,
@@ -562,7 +569,8 @@ static const VMStateDescription vmstate_exynos4210_uart_fifo = {
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(sp, Exynos4210UartFIFO),
         VMSTATE_UINT32(rp, Exynos4210UartFIFO),
-        VMSTATE_VBUFFER_UINT32(data, Exynos4210UartFIFO, 1, NULL, 0, size),
+        VMSTATE_VBUFFER(data, Exynos4210UartFIFO, 1, NULL, 0,
+                exynos4210_uart_get_datasize),
         VMSTATE_END_OF_LIST()
     }
 };
diff --git a/hw/g364fb.c b/hw/g364fb.c
index 3a0b68f..ccd4b76 100644
--- a/hw/g364fb.c
+++ b/hw/g364fb.c
@@ -491,6 +491,11 @@ static int g364fb_post_load(void *opaque, int version_id)
     return 0;
 }
 
+static size_t g364fb_get_vramsize(void *opaque, int version_id)
+{
+    return ((G364State *)opaque)->vram_size;
+}
+
 static const VMStateDescription vmstate_g364fb = {
     .name = "g364fb",
     .version_id = 1,
@@ -498,7 +503,7 @@ static const VMStateDescription vmstate_g364fb = {
     .minimum_version_id_old = 1,
     .post_load = g364fb_post_load,
     .fields = (VMStateField[]) {
-        VMSTATE_VBUFFER_UINT32(vram, G364State, 1, NULL, 0, vram_size),
+        VMSTATE_VBUFFER(vram, G364State, 1, NULL, 0, g364fb_get_vramsize),
         VMSTATE_BUFFER_UNSAFE(color_palette, G364State, 0, 256 * 3),
         VMSTATE_BUFFER_UNSAFE(cursor_palette, G364State, 0, 9),
         VMSTATE_UINT16_ARRAY(cursor, G364State, 512),
diff --git a/hw/m48t59.c b/hw/m48t59.c
index 60bbb00..34355c3 100644
--- a/hw/m48t59.c
+++ b/hw/m48t59.c
@@ -582,6 +582,11 @@ static const MemoryRegionOps nvram_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
+static size_t m48t59_get_bufsize(void *opaque, int version_id)
+{
+    return ((M48t59State *)opaque)->size;
+}
+
 static const VMStateDescription vmstate_m48t59 = {
     .name = "m48t59",
     .version_id = 1,
@@ -590,7 +595,7 @@ static const VMStateDescription vmstate_m48t59 = {
     .fields      = (VMStateField[]) {
         VMSTATE_UINT8(lock, M48t59State),
         VMSTATE_UINT16(addr, M48t59State),
-        VMSTATE_VBUFFER_UINT32(buffer, M48t59State, 0, NULL, 0, size),
+        VMSTATE_VBUFFER(buffer, M48t59State, 0, NULL, 0, m48t59_get_bufsize),
         VMSTATE_END_OF_LIST()
     }
 };
diff --git a/hw/mac_nvram.c b/hw/mac_nvram.c
index ed0a2b7..220e745 100644
--- a/hw/mac_nvram.c
+++ b/hw/mac_nvram.c
@@ -100,13 +100,19 @@ static const MemoryRegionOps macio_nvram_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
+static size_t macio_nvram_get_datasize(void *opaque, int version_id)
+{
+    return ((MacIONVRAMState *)opaque)->size;
+}
+
 static const VMStateDescription vmstate_macio_nvram = {
     .name = "macio_nvram",
     .version_id = 1,
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
     .fields      = (VMStateField[]) {
-        VMSTATE_VBUFFER_UINT32(data, MacIONVRAMState, 0, NULL, 0, size),
+        VMSTATE_VBUFFER(data, MacIONVRAMState, 0, NULL, 0,
+                macio_nvram_get_datasize),
         VMSTATE_END_OF_LIST()
     }
 };
diff --git a/hw/onenand.c b/hw/onenand.c
index db6af68..0da8a0a 100644
--- a/hw/onenand.c
+++ b/hw/onenand.c
@@ -160,6 +160,11 @@ static int onenand_post_load(void *opaque, int version_id)
     return 0;
 }
 
+static size_t onenand_get_blockwpsize(void *opaque, int version_id)
+{
+    return ((OneNANDState *)opaque)->blocks;
+}
+
 static const VMStateDescription vmstate_onenand = {
     .name = "onenand",
     .version_id = 1,
@@ -181,7 +186,7 @@ static const VMStateDescription vmstate_onenand = {
         VMSTATE_UINT16(intstatus, OneNANDState),
         VMSTATE_UINT16(wpstatus, OneNANDState),
         VMSTATE_INT32(secs_cur, OneNANDState),
-        VMSTATE_PARTIAL_VBUFFER(blockwp, OneNANDState, blocks),
+        VMSTATE_PARTIAL_VBUFFER(blockwp, OneNANDState, onenand_get_blockwpsize),
         VMSTATE_UINT8(ecc.cp, OneNANDState),
         VMSTATE_UINT16_ARRAY(ecc.lp, OneNANDState, 2),
         VMSTATE_UINT16(ecc.count, OneNANDState),
diff --git a/savevm.c b/savevm.c
index 80be1ff..333e67a 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1413,9 +1413,12 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
             int size = field->size;
 
             if (field->flags & VMS_VBUFFER) {
-                size = *(int32_t *)(opaque+field->size_offset);
-                if (field->flags & VMS_MULTIPLY) {
-                    size *= field->size;
+                if (field->get_bufsize) {
+                    size = field->get_bufsize(opaque, version_id);
+                    size > field->start ? (size -= field->start) : (size = 0);
+                } else {
+                    error_report("%s vmstate load error: couldn't get "
+                            "buffer %s size", vmsd->name, field->name);
                 }
             }
             if (field->flags & VMS_ARRAY) {
@@ -1477,9 +1480,12 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
             int size = field->size;
 
             if (field->flags & VMS_VBUFFER) {
-                size = *(int32_t *)(opaque+field->size_offset);
-                if (field->flags & VMS_MULTIPLY) {
-                    size *= field->size;
+                if (field->get_bufsize) {
+                    size = field->get_bufsize(opaque, vmsd->version_id);
+                    size > field->start ? (size -= field->start) : (size = 0);
+                } else {
+                    error_report("%s vmstate save error: couldn't get "
+                            "buffer %s size", vmsd->name, field->name);
                 }
             }
             if (field->flags & VMS_ARRAY) {
diff --git a/vmstate.h b/vmstate.h
index 9d3c49c..a210e08 100644
--- a/vmstate.h
+++ b/vmstate.h
@@ -73,8 +73,7 @@ enum VMStateFlags {
     VMS_BUFFER           = 0x020,  /* static sized buffer */
     VMS_ARRAY_OF_POINTER = 0x040,
     VMS_VARRAY_UINT16    = 0x080,  /* Array with size in uint16_t field */
-    VMS_VBUFFER          = 0x100,  /* Buffer with size in int32_t field */
-    VMS_MULTIPLY         = 0x200,  /* multiply "size" field by field_size */
+    VMS_VBUFFER          = 0x100,  /* Buffer with dynamically calculated size */
     VMS_VARRAY_UINT8     = 0x400,  /* Array with size in uint8_t field*/
     VMS_VARRAY_UINT32    = 0x800,  /* Array with size in uint32_t field*/
 };
@@ -86,12 +85,12 @@ typedef struct {
     size_t start;
     int num;
     size_t num_offset;
-    size_t size_offset;
     const VMStateInfo *info;
     enum VMStateFlags flags;
     const VMStateDescription *vmsd;
     int version_id;
     bool (*field_exists)(void *opaque, int version_id);
+    size_t (*get_bufsize)(void *opaque, int version_id);
 } VMStateField;
 
 typedef struct VMStateSubsection {
@@ -354,34 +353,11 @@ extern const VMStateInfo vmstate_info_unused_buffer;
     .offset       = vmstate_offset_buffer(_state, _field) + _start,  \
 }
 
-#define VMSTATE_BUFFER_MULTIPLY(_field, _state, _version, _test, _start, _field_size, _multiply) { \
+#define VMSTATE_VBUFFER(_field, _state, _version, _test, _start, _get_bufsize) { \
     .name         = (stringify(_field)),                             \
     .version_id   = (_version),                                      \
     .field_exists = (_test),                                         \
-    .size_offset  = vmstate_offset_value(_state, _field_size, uint32_t),\
-    .size         = (_multiply),                                      \
-    .info         = &vmstate_info_buffer,                            \
-    .flags        = VMS_VBUFFER|VMS_MULTIPLY,                        \
-    .offset       = offsetof(_state, _field),                        \
-    .start        = (_start),                                        \
-}
-
-#define VMSTATE_VBUFFER(_field, _state, _version, _test, _start, _field_size) { \
-    .name         = (stringify(_field)),                             \
-    .version_id   = (_version),                                      \
-    .field_exists = (_test),                                         \
-    .size_offset  = vmstate_offset_value(_state, _field_size, int32_t),\
-    .info         = &vmstate_info_buffer,                            \
-    .flags        = VMS_VBUFFER|VMS_POINTER,                         \
-    .offset       = offsetof(_state, _field),                        \
-    .start        = (_start),                                        \
-}
-
-#define VMSTATE_VBUFFER_UINT32(_field, _state, _version, _test, _start, _field_size) { \
-    .name         = (stringify(_field)),                             \
-    .version_id   = (_version),                                      \
-    .field_exists = (_test),                                         \
-    .size_offset  = vmstate_offset_value(_state, _field_size, uint32_t),\
+    .get_bufsize  = (_get_bufsize),                                  \
     .info         = &vmstate_info_buffer,                            \
     .flags        = VMS_VBUFFER|VMS_POINTER,                         \
     .offset       = offsetof(_state, _field),                        \
@@ -570,14 +546,11 @@ extern const VMStateInfo vmstate_info_unused_buffer;
 #define VMSTATE_BUFFER_START_MIDDLE(_f, _s, _start) \
     VMSTATE_STATIC_BUFFER(_f, _s, 0, NULL, _start, sizeof(typeof_field(_s, _f)))
 
-#define VMSTATE_PARTIAL_VBUFFER(_f, _s, _size)                        \
-    VMSTATE_VBUFFER(_f, _s, 0, NULL, 0, _size)
-
-#define VMSTATE_PARTIAL_VBUFFER_UINT32(_f, _s, _size)                        \
-    VMSTATE_VBUFFER_UINT32(_f, _s, 0, NULL, 0, _size)
+#define VMSTATE_PARTIAL_VBUFFER(_f, _s, _get_bufsize)                 \
+    VMSTATE_VBUFFER(_f, _s, 0, NULL, 0, _get_bufsize)
 
-#define VMSTATE_SUB_VBUFFER(_f, _s, _start, _size)                    \
-    VMSTATE_VBUFFER(_f, _s, 0, NULL, _start, _size)
+#define VMSTATE_SUB_VBUFFER(_f, _s, _start, _get_bufsize)             \
+    VMSTATE_VBUFFER(_f, _s, 0, NULL, _start, _get_bufsize)
 
 #define VMSTATE_BUFFER_TEST(_f, _s, _test)                            \
     VMSTATE_STATIC_BUFFER(_f, _s, 0, _test, 0, sizeof(typeof_field(_s, _f)))
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH V2 5/5] vmstate: introduce get_bufsize entry in VMStateField

[Andreas Färber]

Am 05.03.2012 09:30, schrieb Igor Mitsyanko:
> New get_bufsize field in VMStateField is supposed to help us easily add save/restore
> support of dynamically allocated buffers in device's states.
> There are some cases when information about size of dynamically allocated buffer is
> already presented in specific device's state structure, but in such a form that
> can not be used with existing VMStateField interface. Currently, we either can get size from
> another variable in device's state as it is with VMSTATE_VBUFFER_* macros, or we can
> also multiply value kept in a variable by a constant with VMSTATE_BUFFER_MULTIPLY
> macro. If we need to perform any other action, we're forced to add additional
> variable with size information to device state structure with the only intention
> to use it in VMStateDescription. This approach is not very pretty. Adding extra
> flags to VMStateFlags enum for every other possible operation with size field
> seems redundant, and still it would't cover cases when we need to perform a set of
> operations to get size value.
> With get_bufsize callback we can calculate size of dynamic array in whichever
> way we need. We don't need .size_offset field anymore, so we can remove it from
> VMState Field structure to compensate for extra memory consuption because of
> get_bufsize addition. Macros VMSTATE_VBUFFER* are modified to use new callback
> instead of .size_offset. Macro VMSTATE_BUFFER_MULTIPLY and VMFlag VMS_MULTIPLY
> are removed completely as they are now redundant.
> 
> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>

Apart from this commit message being an overwhelmingly huge block of
text ;) (maybe insert an empty line or two?) this had touched on the
overall discussion of whether to pursue a declarative or imperative
approach for VMState.

So, what about adding this as a new, optional mechanism and leaving
VBUFFER / BUFFER_MULTIPLY users untouched?

Andreas

[...]
> diff --git a/vmstate.h b/vmstate.h
> index 9d3c49c..a210e08 100644
> --- a/vmstate.h
> +++ b/vmstate.h
> @@ -73,8 +73,7 @@ enum VMStateFlags {
>      VMS_BUFFER           = 0x020,  /* static sized buffer */
>      VMS_ARRAY_OF_POINTER = 0x040,
>      VMS_VARRAY_UINT16    = 0x080,  /* Array with size in uint16_t field */
> -    VMS_VBUFFER          = 0x100,  /* Buffer with size in int32_t field */
> -    VMS_MULTIPLY         = 0x200,  /* multiply "size" field by field_size */
> +    VMS_VBUFFER          = 0x100,  /* Buffer with dynamically calculated size */
>      VMS_VARRAY_UINT8     = 0x400,  /* Array with size in uint8_t field*/
>      VMS_VARRAY_UINT32    = 0x800,  /* Array with size in uint32_t field*/
>  };
> @@ -86,12 +85,12 @@ typedef struct {
>      size_t start;
>      int num;
>      size_t num_offset;
> -    size_t size_offset;
>      const VMStateInfo *info;
>      enum VMStateFlags flags;
>      const VMStateDescription *vmsd;
>      int version_id;
>      bool (*field_exists)(void *opaque, int version_id);
> +    size_t (*get_bufsize)(void *opaque, int version_id);
>  } VMStateField;
>  
>  typedef struct VMStateSubsection {
> @@ -354,34 +353,11 @@ extern const VMStateInfo vmstate_info_unused_buffer;
>      .offset       = vmstate_offset_buffer(_state, _field) + _start,  \
>  }
>  
> -#define VMSTATE_BUFFER_MULTIPLY(_field, _state, _version, _test, _start, _field_size, _multiply) { \
> +#define VMSTATE_VBUFFER(_field, _state, _version, _test, _start, _get_bufsize) { \
>      .name         = (stringify(_field)),                             \
>      .version_id   = (_version),                                      \
>      .field_exists = (_test),                                         \
> -    .size_offset  = vmstate_offset_value(_state, _field_size, uint32_t),\
> -    .size         = (_multiply),                                      \
> -    .info         = &vmstate_info_buffer,                            \
> -    .flags        = VMS_VBUFFER|VMS_MULTIPLY,                        \
> -    .offset       = offsetof(_state, _field),                        \
> -    .start        = (_start),                                        \
> -}
> -
> -#define VMSTATE_VBUFFER(_field, _state, _version, _test, _start, _field_size) { \
> -    .name         = (stringify(_field)),                             \
> -    .version_id   = (_version),                                      \
> -    .field_exists = (_test),                                         \
> -    .size_offset  = vmstate_offset_value(_state, _field_size, int32_t),\
> -    .info         = &vmstate_info_buffer,                            \
> -    .flags        = VMS_VBUFFER|VMS_POINTER,                         \
> -    .offset       = offsetof(_state, _field),                        \
> -    .start        = (_start),                                        \
> -}
> -
> -#define VMSTATE_VBUFFER_UINT32(_field, _state, _version, _test, _start, _field_size) { \
> -    .name         = (stringify(_field)),                             \
> -    .version_id   = (_version),                                      \
> -    .field_exists = (_test),                                         \
> -    .size_offset  = vmstate_offset_value(_state, _field_size, uint32_t),\
> +    .get_bufsize  = (_get_bufsize),                                  \
>      .info         = &vmstate_info_buffer,                            \
>      .flags        = VMS_VBUFFER|VMS_POINTER,                         \
>      .offset       = offsetof(_state, _field),                        \
> @@ -570,14 +546,11 @@ extern const VMStateInfo vmstate_info_unused_buffer;
>  #define VMSTATE_BUFFER_START_MIDDLE(_f, _s, _start) \
>      VMSTATE_STATIC_BUFFER(_f, _s, 0, NULL, _start, sizeof(typeof_field(_s, _f)))
>  
> -#define VMSTATE_PARTIAL_VBUFFER(_f, _s, _size)                        \
> -    VMSTATE_VBUFFER(_f, _s, 0, NULL, 0, _size)
> -
> -#define VMSTATE_PARTIAL_VBUFFER_UINT32(_f, _s, _size)                        \
> -    VMSTATE_VBUFFER_UINT32(_f, _s, 0, NULL, 0, _size)
> +#define VMSTATE_PARTIAL_VBUFFER(_f, _s, _get_bufsize)                 \
> +    VMSTATE_VBUFFER(_f, _s, 0, NULL, 0, _get_bufsize)
>  
> -#define VMSTATE_SUB_VBUFFER(_f, _s, _start, _size)                    \
> -    VMSTATE_VBUFFER(_f, _s, 0, NULL, _start, _size)
> +#define VMSTATE_SUB_VBUFFER(_f, _s, _start, _get_bufsize)             \
> +    VMSTATE_VBUFFER(_f, _s, 0, NULL, _start, _get_bufsize)
>  
>  #define VMSTATE_BUFFER_TEST(_f, _s, _test)                            \
>      VMSTATE_STATIC_BUFFER(_f, _s, 0, _test, 0, sizeof(typeof_field(_s, _f)))

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH V2 5/5] vmstate: introduce get_bufsize entry in VMStateField

[Igor Mitsyanko]

On 03/14/2012 04:55 PM, Andreas Färber wrote:
> Am 05.03.2012 09:30, schrieb Igor Mitsyanko:
>> New get_bufsize field in VMStateField is supposed to help us easily add save/restore
>> support of dynamically allocated buffers in device's states.
>> There are some cases when information about size of dynamically allocated buffer is
>> already presented in specific device's state structure, but in such a form that
>> can not be used with existing VMStateField interface. Currently, we either can get size from
>> another variable in device's state as it is with VMSTATE_VBUFFER_* macros, or we can
>> also multiply value kept in a variable by a constant with VMSTATE_BUFFER_MULTIPLY
>> macro. If we need to perform any other action, we're forced to add additional
>> variable with size information to device state structure with the only intention
>> to use it in VMStateDescription. This approach is not very pretty. Adding extra
>> flags to VMStateFlags enum for every other possible operation with size field
>> seems redundant, and still it would't cover cases when we need to perform a set of
>> operations to get size value.
>> With get_bufsize callback we can calculate size of dynamic array in whichever
>> way we need. We don't need .size_offset field anymore, so we can remove it from
>> VMState Field structure to compensate for extra memory consuption because of
>> get_bufsize addition. Macros VMSTATE_VBUFFER* are modified to use new callback
>> instead of .size_offset. Macro VMSTATE_BUFFER_MULTIPLY and VMFlag VMS_MULTIPLY
>> are removed completely as they are now redundant.
>>
>> Signed-off-by: Igor Mitsyanko<i.mitsyanko@samsung.com>
>
> Apart from this commit message being an overwhelmingly huge block of
> text ;) (maybe insert an empty line or two?)

OK :)

  this had touched on the
> overall discussion of whether to pursue a declarative or imperative
> approach for VMState.
>
> So, what about adding this as a new, optional mechanism and leaving
> VBUFFER / BUFFER_MULTIPLY users untouched?

I don't mind.

Now that we decided (in another thread) against live saving of wp_groups 
in SDState, the question with this patch arises again: do we need to 
introduce get_bufsize or we should just use additional member in SDState 
to hold wp_groups buffer size for use with current 
VMSTATE_VBUFFER_UINT32 macro?

-- 
Mitsyanko Igor
ASWG, Moscow R&D center, Samsung Electronics
email: i.mitsyanko@samsung.com

Re: [Qemu-devel] [PATCH V2 0/5] VMState cleanups

[Peter Maydell]

On 5 March 2012 08:30, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> This patchset cleans up and optimizes vmstate implementation.
>
> v1-v2
> PATCH 4 now moves VMSTATE_UINTTL* macros to cpu-defs.h instead of to vmstate.h.
> put_target_ul/get_target_ul introduction is dropped.
> PATCH2,PATCH3 commit messages are rewritten a little bit. But I'm still
> not sure whether Andrzej Zaborowski agreed with these changes or not.
>
>
> Patch 1 is a trivial bug fixing.
> Patches 2 and 3 replaces target_phys_addr_t in pxa implementation
> to uint32_t.
> Patch 4 moves VMSTATE_UINTTL from hw/hw.h to cpu-defs.h.
> Patch 5 introduces mechanism to save variable-size buffers.

Unless anybody wants to object, I'm going to take patches 2 and 3
into arm-devs.next and put them in my next pullreq (probably end
of the week). Andrzej?

-- PMM