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

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

[Igor Mitsyanko]

This patchset cleans up and optimizes vmstate implementation.

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.h to vmstate.h. Explicit dependency
on NEED_CPU_H is droped, I failed to understand why it was presented at all.
Patch 5 introduces mechanism to save variable-size buffers. This already
received plenty of discusion, but I still have not understand what was the
final decision about it.

Igor Mitsyanko (5):
  target-alpha/machine.c: use VMSTATE_UINT64* instead of
    VMSTATE_UINTTL*
  hw/pxa2xx_dma.c: drop VMSTATE_UINTTL usage
  hw/pxa2xx_lcd.c: drop VMSTATE_UINTTL usage
  vmstate: refactor and move VMSTATE_UINTTL* macro
  vmstate: introduce get_bufsize entry in VMStateField

 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               |   39 +++++++++++++++++++++++++++++-----
 target-alpha/machine.c |   34 +++++++++++++++---------------
 vmstate.h              |   54 ++++++++++++++++-------------------------------
 11 files changed, 115 insertions(+), 94 deletions(-)

-- 
1.7.4.1

[Qemu-devel] [PATCH 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 2/5] hw/pxa2xx_dma.c: drop VMSTATE_UINTTL usage

[Igor Mitsyanko]

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 it safely because:
1) pxa2xx has 32-bit physical address;
2) rest of the code in this file treats these variables as 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>
---
 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 3/5] hw/pxa2xx_lcd.c: drop VMSTATE_UINTTL usage

[Igor Mitsyanko]

Convert three variables in DMAChannel state from type target_phys_addr_t to uint32_t,
use VMSTATE_UINT32 instead of VMSTATE_UINTTL for these variables.
We can do it safely because:
1) pxa2xx has 32-bit physical address;
2) rest of the code in this file treats these variables as 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>
---
 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 9495226..4b712bb 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;
 };
@@ -934,11 +934,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

[Qemu-devel] [PATCH 4/5] vmstate: refactor and move VMSTATE_UINTTL* macro

[Igor Mitsyanko]

Instead of defining VMSTATE_UINTTL* based on TARGET_LONG_BITS value, we can
use qemu_put_betls/qemu_get_betls functions. These two functions depend on
TARGET_LONG_BITS as well, and this new approach for VMSTATE_UINTTL* will result
in the same thing as before (will call qemu_get_be32s/qemu_put_be32s for 32-bit
target or qemu_put_be64s/qemu_get_be64s for 64-bit target).
Move VMSTATE_UINTTL* definitions to vmstate.h where they belong.

Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
---
 hw/hw.h   |   19 -------------------
 savevm.c  |   21 +++++++++++++++++++++
 vmstate.h |   11 +++++++++++
 3 files changed, 32 insertions(+), 19 deletions(-)

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
diff --git a/savevm.c b/savevm.c
index 80be1ff..af33157 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1081,6 +1081,27 @@ const VMStateInfo vmstate_info_uint16_equal = {
     .put  = put_uint16,
 };
 
+/* target unsigned long */
+
+static int get_target_ul(QEMUFile *f, void *pv, size_t size)
+{
+    target_ulong *v = pv;
+    qemu_get_betls(f, v);
+    return 0;
+}
+
+static void put_target_ul(QEMUFile *f, void *pv, size_t size)
+{
+    target_ulong *v = pv;
+    qemu_put_betls(f, v);
+}
+
+const VMStateInfo vmstate_info_target_ul = {
+    .name = "target ulong",
+    .get  = get_target_ul,
+    .put  = put_target_ul,
+};
+
 /* timers  */
 
 static int get_timer(QEMUFile *f, void *pv, size_t size)
diff --git a/vmstate.h b/vmstate.h
index 9d3c49c..218c303 100644
--- a/vmstate.h
+++ b/vmstate.h
@@ -130,6 +130,7 @@ extern const VMStateInfo vmstate_info_uint8;
 extern const VMStateInfo vmstate_info_uint16;
 extern const VMStateInfo vmstate_info_uint32;
 extern const VMStateInfo vmstate_info_uint64;
+extern const VMStateInfo vmstate_info_target_ul;
 
 extern const VMStateInfo vmstate_info_timer;
 extern const VMStateInfo vmstate_info_buffer;
@@ -446,6 +447,8 @@ extern const VMStateInfo vmstate_info_unused_buffer;
     VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint32, uint32_t)
 #define VMSTATE_UINT64_V(_f, _s, _v)                                  \
     VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint64, uint64_t)
+#define VMSTATE_UINTTL_V(_f, _s, _v)                                  \
+    VMSTATE_SINGLE(_f, _s, _v, vmstate_info_target_ul, target_ulong)
 
 #define VMSTATE_BOOL(_f, _s)                                          \
     VMSTATE_BOOL_V(_f, _s, 0)
@@ -467,6 +470,8 @@ extern const VMStateInfo vmstate_info_unused_buffer;
     VMSTATE_UINT32_V(_f, _s, 0)
 #define VMSTATE_UINT64(_f, _s)                                        \
     VMSTATE_UINT64_V(_f, _s, 0)
+#define VMSTATE_UINTTL(_f, _s)                                        \
+    VMSTATE_UINTTL_V(_f, _s, 0)
 
 #define VMSTATE_UINT8_EQUAL(_f, _s)                                   \
     VMSTATE_SINGLE(_f, _s, 0, vmstate_info_uint8_equal, uint8_t)
@@ -558,6 +563,12 @@ extern const VMStateInfo vmstate_info_unused_buffer;
 #define VMSTATE_INT64_ARRAY(_f, _s, _n)                               \
     VMSTATE_INT64_ARRAY_V(_f, _s, _n, 0)
 
+#define VMSTATE_UINTTL_ARRAY_V(_f, _s, _n, _v)                        \
+    VMSTATE_ARRAY(_f, _s, _n, _v, vmstate_info_target_ul, target_ulong)
+
+#define VMSTATE_UINTTL_ARRAY(_f, _s, _n)                              \
+        VMSTATE_UINTTL_ARRAY_V(_f, _s, _n, 0)
+
 #define VMSTATE_BUFFER_V(_f, _s, _v)                                  \
     VMSTATE_STATIC_BUFFER(_f, _s, _v, NULL, 0, sizeof(typeof_field(_s, _f)))
 
-- 
1.7.4.1

[Qemu-devel] [PATCH 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 9c63bdd..bf9aed5 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 af33157..c1c4c9e 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1434,9 +1434,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) {
@@ -1498,9 +1501,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 218c303..3370d36 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 {
@@ -355,34 +354,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),                        \
@@ -581,14 +557,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 2/5] hw/pxa2xx_dma.c: drop VMSTATE_UINTTL usage

[Peter Maydell]

On 22 February 2012 10:15, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> 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 it safely because:
> 1) pxa2xx has 32-bit physical address;
> 2) rest of the code in this file treats these variables as 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>
> ---
>  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),
> --

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

These are clearly intending to model 32 bit registers so we shouldn't
be tying them to target_phys_addr_t (which would probably break things
if/when we ever move to target_phys_addr_t being 64 bits for all things).

-- PMM

Re: [Qemu-devel] [PATCH 3/5] hw/pxa2xx_lcd.c: drop VMSTATE_UINTTL usage

[Peter Maydell]

On 22 February 2012 10:15, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> Convert three variables in DMAChannel state from type target_phys_addr_t to uint32_t,
> use VMSTATE_UINT32 instead of VMSTATE_UINTTL for these variables.
> We can do it safely because:
> 1) pxa2xx has 32-bit physical address;
> 2) rest of the code in this file treats these variables as 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>
> ---
>  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 9495226..4b712bb 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;
>  };
> @@ -934,11 +934,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
>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

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

[Peter Maydell]

Looks plausible but cc'ing the Alpha maintainer...

-- PMM

On 22 February 2012 10:15, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> 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
>



-- 
12345678901234567890123456789012345678901234567890123456789012345678901234567890
         1         2         3         4         5         6         7         8

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

[Peter Maydell]

On 22 February 2012 10:15, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> This patchset cleans up and optimizes vmstate implementation.
>
> 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.h to vmstate.h. Explicit dependency
> on NEED_CPU_H is droped, I failed to understand why it was presented at all.

So if we apply patches 1-3 (which all look plausible) then the only
remaining user of VMSTATE_UINTTL is target-i386/machine.c as far as
I can see.

This leaves me wondering if we shouldn't just put it actually in
target-i386/machine.c as a convenience macro for that specific CPU
to avoid having to have more #ifdef TARGET_X86_64s. (I note that
the machine.c code is already pretty inconsistent, eg lstar and
cstar are defined as target_ulong and saved with VMSTATE_UINT64.)

Basically VMSTATE_UINTTL seems like a bit of a dangerous thing to
leave lying around as there aren't really very many use cases
for it...

-- PMM

Re: [Qemu-devel] [PATCH 3/5] hw/pxa2xx_lcd.c: drop VMSTATE_UINTTL usage

[andrzej zaborowski]

On 22 February 2012 11:15, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> Convert three variables in DMAChannel state from type target_phys_addr_t to uint32_t,
> use VMSTATE_UINT32 instead of VMSTATE_UINTTL for these variables.
> We can do it safely because:
> 1) pxa2xx has 32-bit physical address;
> 2) rest of the code in this file treats these variables as uint32_t;

Why's uint32_t more correct though?  The purpose of using a named type
across qemu is to mark fields as memory addresses (similar to size_t
being used for sizes, etc.), uint32_t conveys less information -- only
the size.

It's a safe hack, but I don't see the rationale.

If it's because VMSTATE_UINT32 requires that specific type than a less
ugly hack would be to make a pxa specific memory address type.

Cheers

Re: [Qemu-devel] [PATCH 3/5] hw/pxa2xx_lcd.c: drop VMSTATE_UINTTL usage

[Peter Maydell]

On 22 February 2012 11:36, andrzej zaborowski <balrog@zabor.org> wrote:
> On 22 February 2012 11:15, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
>> Convert three variables in DMAChannel state from type target_phys_addr_t to uint32_t,
>> use VMSTATE_UINT32 instead of VMSTATE_UINTTL for these variables.
>> We can do it safely because:
>> 1) pxa2xx has 32-bit physical address;
>> 2) rest of the code in this file treats these variables as uint32_t;
>
> Why's uint32_t more correct though?  The purpose of using a named type
> across qemu is to mark fields as memory addresses (similar to size_t
> being used for sizes, etc.), uint32_t conveys less information -- only
> the size.
>
> It's a safe hack, but I don't see the rationale.

Because we might change target_phys_addr_t to 64 bits globally
some day (it's certainly been mooted) and that shouldn't suddenly
change the register width and certainly shouldn't change the
migration state.

Basically VMSTATE_UINTTL in hw/ is always a bug, because its
behaviour depends on the size of target_ulong, which is a
property of the CPU, which is a completely separate device.

> If it's because VMSTATE_UINT32 requires that specific type than a less
> ugly hack would be to make a pxa specific memory address type.

Yuck.

-- PMM

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

[Mitsyanko Igor]

On 02/22/2012 03:26 PM, Peter Maydell wrote:
> On 22 February 2012 10:15, Igor Mitsyanko<i.mitsyanko@samsung.com>  wrote:
>> This patchset cleans up and optimizes vmstate implementation.
>>
>> 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.h to vmstate.h. Explicit dependency
>> on NEED_CPU_H is droped, I failed to understand why it was presented at all.
>
> So if we apply patches 1-3 (which all look plausible) then the only
> remaining user of VMSTATE_UINTTL is target-i386/machine.c as far as
> I can see.
>
> This leaves me wondering if we shouldn't just put it actually in
> target-i386/machine.c as a convenience macro for that specific CPU
> to avoid having to have more #ifdef TARGET_X86_64s. (I note that
> the machine.c code is already pretty inconsistent, eg lstar and
> cstar are defined as target_ulong and saved with VMSTATE_UINT64.)
>
> Basically VMSTATE_UINTTL seems like a bit of a dangerous thing to
> leave lying around as there aren't really very many use cases
> for it...

I personally don't really like all these "hack" VMSTATE macros spread 
all around QEMU code. I would prefer to have all convenient VMSTATE_*s 
in one place and eventually replace all file-specific hack macro with 
standard ones. Not sure that it's worth an effort though.
If we're going to move UINTTL* to target-i386/machine.c, then they 
probably should be implemented in the same way as they are implemented 
in hw.h now. But without NEED_CPU_H.

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

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

[Peter Maydell]

On 22 February 2012 10:15, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> Patch 4 moves VMSTATE_UINTTL from hw.h to vmstate.h. Explicit dependency
> on NEED_CPU_H is droped, I failed to understand why it was presented at all.

You need #ifdef NEED_CPU_H because in generic source files (where NEED_CPU_H
is not defined) there is no TARGET_LONG_BITS or target_ulong because you
don't know how wide the target CPU virtual addresses are. Only in the
source files which are compiled for a specific CPU virtual address size
is NEED_CPU_H defined. There's no point in defining these macros in
contexts where they can't possibly be used.

(This dependence on the CPU vaddr width is why I like the idea of just
sticking them in target-i386...)

-- PMM

Re: [Qemu-devel] [PATCH 3/5] hw/pxa2xx_lcd.c: drop VMSTATE_UINTTL usage

[andrzej zaborowski]

On 22 February 2012 13:00, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 22 February 2012 11:36, andrzej zaborowski <balrog@zabor.org> wrote:
>> On 22 February 2012 11:15, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
>>> Convert three variables in DMAChannel state from type target_phys_addr_t to uint32_t,
>>> use VMSTATE_UINT32 instead of VMSTATE_UINTTL for these variables.
>>> We can do it safely because:
>>> 1) pxa2xx has 32-bit physical address;
>>> 2) rest of the code in this file treats these variables as uint32_t;
>>
>> Why's uint32_t more correct though?  The purpose of using a named type
>> across qemu is to mark fields as memory addresses (similar to size_t
>> being used for sizes, etc.), uint32_t conveys less information -- only
>> the size.
>>
>> It's a safe hack, but I don't see the rationale.
>
> Because we might change target_phys_addr_t to 64 bits globally
> some day (it's certainly been mooted) and that shouldn't suddenly
> change the register width and certainly shouldn't change the
> migration state.
>
> Basically VMSTATE_UINTTL in hw/ is always a bug, because its
> behaviour depends on the size of target_ulong, which is a
> property of the CPU, which is a completely separate device.

I'm not really discussing that, my question is unrelated to
migration/savevm because the patch touches parts that shouldn't be
concerned with migration.  If a particular function (like migration)
needs the type converted to something then that's why C has type
conversions.  A type conversion that compiles to no code is still a
type conversion.

>
>> If it's because VMSTATE_UINT32 requires that specific type than a less
>> ugly hack would be to make a pxa specific memory address type.
>
> Yuck.

Or a general 32-bit memory address type, but as I said uint32_t
conveys less information.  Do you disagree?

Cheers

Re: [Qemu-devel] [PATCH 3/5] hw/pxa2xx_lcd.c: drop VMSTATE_UINTTL usage

[Mitsyanko Igor]

On 02/22/2012 03:36 PM, andrzej zaborowski wrote:
> On 22 February 2012 11:15, Igor Mitsyanko<i.mitsyanko@samsung.com>  wrote:
>> Convert three variables in DMAChannel state from type target_phys_addr_t to uint32_t,
>> use VMSTATE_UINT32 instead of VMSTATE_UINTTL for these variables.
>> We can do it safely because:
>> 1) pxa2xx has 32-bit physical address;
>> 2) rest of the code in this file treats these variables as uint32_t;
>
> Why's uint32_t more correct though?  The purpose of using a named type
> across qemu is to mark fields as memory addresses (similar to size_t
> being used for sizes, etc.), uint32_t conveys less information -- only
> the size.

It's obviously more informative, but I thought it's main purpose is to 
be used with code that could be executed for a different targets (with 
different address bus width).


> It's a safe hack, but I don't see the rationale.

I don't consider this a hack, we are trying to emulate real hardware, 
and pxa lcd and dma controllers are intended to work with 32-bit bus. We 
should not have a possibility to use them with 64-bit targets.

> If it's because VMSTATE_UINT32 requires that specific type than a less
> ugly hack would be to make a pxa specific memory address type.
>

Introducing new type doesn't look pretty to me, maybe just rename 
variables to source_addr, dest_addr e.t.c?


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

Re: [Qemu-devel] [PATCH 3/5] hw/pxa2xx_lcd.c: drop VMSTATE_UINTTL usage

[Peter Maydell]

On 22 February 2012 12:13, andrzej zaborowski <balrogg@gmail.com> wrote:
> On 22 February 2012 13:00, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 22 February 2012 11:36, andrzej zaborowski <balrog@zabor.org> wrote:
>>> On 22 February 2012 11:15, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
>>>> Convert three variables in DMAChannel state from type target_phys_addr_t to uint32_t,
>>>> use VMSTATE_UINT32 instead of VMSTATE_UINTTL for these variables.
>>>> We can do it safely because:
>>>> 1) pxa2xx has 32-bit physical address;
>>>> 2) rest of the code in this file treats these variables as uint32_t;
>>>
>>> Why's uint32_t more correct though?  The purpose of using a named type
>>> across qemu is to mark fields as memory addresses (similar to size_t
>>> being used for sizes, etc.), uint32_t conveys less information -- only
>>> the size.
>>>
>>> It's a safe hack, but I don't see the rationale.
>>
>> Because we might change target_phys_addr_t to 64 bits globally
>> some day (it's certainly been mooted) and that shouldn't suddenly
>> change the register width and certainly shouldn't change the
>> migration state.
>>
>> Basically VMSTATE_UINTTL in hw/ is always a bug, because its
>> behaviour depends on the size of target_ulong, which is a
>> property of the CPU, which is a completely separate device.
>
> I'm not really discussing that, my question is unrelated to
> migration/savevm because the patch touches parts that shouldn't be
> concerned with migration.  If a particular function (like migration)
> needs the type converted to something then that's why C has type
> conversions.  A type conversion that compiles to no code is still a
> type conversion.

Well, target_phys_addr_t is the wrong type here because it's
really "at least as large as the widest address type in the
system" (cf proposals to make it 64 bits), so using it for
a register that must be exactly 32 bits wide is wrong. So we
need to change it to something, and customarily what we use
for "I am modelling a physical register which is 32 bits wide"
is uint32_t. Introducing extra device-specific typedefs to
try to label the semantics of device registers seems a bit
unnecessary to me.

So we need to change the type, and we also need to change the
VMSTATE macro for reasons explained earlier, and we need to make
sure the macro and the struct agree about the type they are
dealing with, so it's simplest to do it all in one patch.

If you're complaining that the VMSTATE macros don't provide
a way for you to say "do this cast" then I agree that that is
a slightly irritating omission but that's the infrastructure
we have...

-- PMM

Re: [Qemu-devel] [PATCH 3/5] hw/pxa2xx_lcd.c: drop VMSTATE_UINTTL usage

[andrzej zaborowski]

On 22 February 2012 13:26, Mitsyanko Igor <i.mitsyanko@samsung.com> wrote:
> On 02/22/2012 03:36 PM, andrzej zaborowski wrote:
>> Why's uint32_t more correct though?  The purpose of using a named type
>> across qemu is to mark fields as memory addresses (similar to size_t
>> being used for sizes, etc.), uint32_t conveys less information -- only
>> the size.
>
> It's obviously more informative, but I thought it's main purpose is to be
> used with code that could be executed for a different targets (with
> different address bus width).

In some case for sure, but I believe not in most cases.

>
>
>
>> It's a safe hack, but I don't see the rationale.
>
>
> I don't consider this a hack, we are trying to emulate real hardware, and
> pxa lcd and dma controllers are intended to work with 32-bit bus. We should
> not have a possibility to use them with 64-bit targets.
>
>
>> If it's because VMSTATE_UINT32 requires that specific type than a less
>> ugly hack would be to make a pxa specific memory address type.
>>
>
> Introducing new type doesn't look pretty to me,

Why?

> maybe just rename variables
> to source_addr, dest_addr e.t.c?

Wouldn't it be analogous to changing pointer typed variables to void *
and adding the actual type in their names?  The result is that at
language level they'll all be the same type even though they are not.

(or changing le32 and be32 to uint32 in Linux)

Cheers

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

[Andreas Färber]

Am 22.02.2012 12:26, schrieb Peter Maydell:
> So if we apply patches 1-3 (which all look plausible) then the only
> remaining user of VMSTATE_UINTTL is target-i386/machine.c as far as
> I can see.
> 
> This leaves me wondering if we shouldn't just put it actually in
> target-i386/machine.c as a convenience macro for that specific CPU
> to avoid having to have more #ifdef TARGET_X86_64s.

Nack. I don't see the connection between target_ulong and TARGET_X86_64.
Just because that becomes the only user does not mean target_ulong is an
x86-specific concept.

> (I note that
> the machine.c code is already pretty inconsistent, eg lstar and
> cstar are defined as target_ulong and saved with VMSTATE_UINT64.)

Same for TCGv. We also have quite a few mixes of int and (U)INT32.

Andreas

> 
> Basically VMSTATE_UINTTL seems like a bit of a dangerous thing to
> leave lying around as there aren't really very many use cases
> for it...

-- 
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 0/5] VMState cleanups

[Peter Maydell]

On 22 February 2012 12:49, Andreas Färber <afaerber@suse.de> wrote:
> Am 22.02.2012 12:26, schrieb Peter Maydell:
>> So if we apply patches 1-3 (which all look plausible) then the only
>> remaining user of VMSTATE_UINTTL is target-i386/machine.c as far as
>> I can see.
>>
>> This leaves me wondering if we shouldn't just put it actually in
>> target-i386/machine.c as a convenience macro for that specific CPU
>> to avoid having to have more #ifdef TARGET_X86_64s.
>
> Nack. I don't see the connection between target_ulong and TARGET_X86_64.
> Just because that becomes the only user does not mean target_ulong is an
> x86-specific concept.

Yeah, I guess. I would like the macro to be protected so you can only
get at it if you're target-*/ though...

-- PMM

Re: [Qemu-devel] [PATCH 3/5] hw/pxa2xx_lcd.c: drop VMSTATE_UINTTL usage

[andrzej zaborowski]

On 22 February 2012 13:48, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 22 February 2012 12:13, andrzej zaborowski <balrogg@gmail.com> wrote:
>> On 22 February 2012 13:00, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 22 February 2012 11:36, andrzej zaborowski <balrog@zabor.org> wrote:
>>>> On 22 February 2012 11:15, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
>>>>> Convert three variables in DMAChannel state from type target_phys_addr_t to uint32_t,
>>>>> use VMSTATE_UINT32 instead of VMSTATE_UINTTL for these variables.
>>>>> We can do it safely because:
>>>>> 1) pxa2xx has 32-bit physical address;
>>>>> 2) rest of the code in this file treats these variables as uint32_t;
>>>>
>>>> Why's uint32_t more correct though?  The purpose of using a named type
>>>> across qemu is to mark fields as memory addresses (similar to size_t
>>>> being used for sizes, etc.), uint32_t conveys less information -- only
>>>> the size.
>>>>
>>>> It's a safe hack, but I don't see the rationale.
>>>
>>> Because we might change target_phys_addr_t to 64 bits globally
>>> some day (it's certainly been mooted) and that shouldn't suddenly
>>> change the register width and certainly shouldn't change the
>>> migration state.
>>>
>>> Basically VMSTATE_UINTTL in hw/ is always a bug, because its
>>> behaviour depends on the size of target_ulong, which is a
>>> property of the CPU, which is a completely separate device.
>>
>> I'm not really discussing that, my question is unrelated to
>> migration/savevm because the patch touches parts that shouldn't be
>> concerned with migration.  If a particular function (like migration)
>> needs the type converted to something then that's why C has type
>> conversions.  A type conversion that compiles to no code is still a
>> type conversion.
>
> Well, target_phys_addr_t is the wrong type here because it's
> really "at least as large as the widest address type in the
> system" (cf proposals to make it 64 bits), so using it for
> a register that must be exactly 32 bits wide is wrong. So we
> need to change it to something, and customarily what we use
> for "I am modelling a physical register which is 32 bits wide"
> is uint32_t. Introducing extra device-specific typedefs to
> try to label the semantics of device registers seems a bit
> unnecessary to me.

If we treat the struct as a representation of the register values
rather than state of the emulated device then I guess you're right.
The reason it rings an alarm is that the change is not an improvement
(other than for migration, but again the change is in code that is not
related to savevm)

Cheers

Re: [Qemu-devel] [PATCH 3/5] hw/pxa2xx_lcd.c: drop VMSTATE_UINTTL usage

[Mitsyanko Igor]

On 02/22/2012 04:48 PM, andrzej zaborowski wrote:
> On 22 February 2012 13:26, Mitsyanko Igor<i.mitsyanko@samsung.com>  wrote:
>> On 02/22/2012 03:36 PM, andrzej zaborowski wrote:
>>> Why's uint32_t more correct though?  The purpose of using a named type
>>> across qemu is to mark fields as memory addresses (similar to size_t
>>> being used for sizes, etc.), uint32_t conveys less information -- only
>>> the size.
>>
>> It's obviously more informative, but I thought it's main purpose is to be
>> used with code that could be executed for a different targets (with
>> different address bus width).
>
> In some case for sure, but I believe not in most cases.
>
>>
>>
>>
>>> It's a safe hack, but I don't see the rationale.
>>
>>
>> I don't consider this a hack, we are trying to emulate real hardware, and
>> pxa lcd and dma controllers are intended to work with 32-bit bus. We should
>> not have a possibility to use them with 64-bit targets.
>>
>>
>>> If it's because VMSTATE_UINT32 requires that specific type than a less
>>> ugly hack would be to make a pxa specific memory address type.
>>>
>>
>> Introducing new type doesn't look pretty to me,
>
> Why?

Peter already answered, this fields should be exactly 32-bit wide 
(hardware is implemented this way) and we already have a type that is 
exactly 32-bit wide. Implementing each device without any assumptions 
about other parts of emulated system seems like a right approach to me.
Doing something like typedef uint32_t pxalcd_phys_addr_t is fun, but 
then we could end up introducing typedefs like this for every device in hw/.
Also, currently we can't save custom types in VMSTATE.

>> maybe just rename variables
>> to source_addr, dest_addr e.t.c?
>
> Wouldn't it be analogous to changing pointer typed variables to void *
> and adding the actual type in their names?  The result is that at
> language level they'll all be the same type even though they are not.
>
> (or changing le32 and be32 to uint32 in Linux)
>

Yes, but you got to admit they would be more informative for human :)


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

Re: [Qemu-devel] [PATCH 3/5] hw/pxa2xx_lcd.c: drop VMSTATE_UINTTL usage

[Mitsyanko Igor]

On 02/22/2012 04:56 PM, andrzej zaborowski wrote:
> On 22 February 2012 13:48, Peter Maydell<peter.maydell@linaro.org>  wrote:
>> On 22 February 2012 12:13, andrzej zaborowski<balrogg@gmail.com>  wrote:
>>> On 22 February 2012 13:00, Peter Maydell<peter.maydell@linaro.org>  wrote:
>>>> On 22 February 2012 11:36, andrzej zaborowski<balrog@zabor.org>  wrote:
>>>>> On 22 February 2012 11:15, Igor Mitsyanko<i.mitsyanko@samsung.com>  wrote:
>>>>>> Convert three variables in DMAChannel state from type target_phys_addr_t to uint32_t,
>>>>>> use VMSTATE_UINT32 instead of VMSTATE_UINTTL for these variables.
>>>>>> We can do it safely because:
>>>>>> 1) pxa2xx has 32-bit physical address;
>>>>>> 2) rest of the code in this file treats these variables as uint32_t;
>>>>> Why's uint32_t more correct though?  The purpose of using a named type
>>>>> across qemu is to mark fields as memory addresses (similar to size_t
>>>>> being used for sizes, etc.), uint32_t conveys less information -- only
>>>>> the size.
>>>>>
>>>>> It's a safe hack, but I don't see the rationale.
>>>> Because we might change target_phys_addr_t to 64 bits globally
>>>> some day (it's certainly been mooted) and that shouldn't suddenly
>>>> change the register width and certainly shouldn't change the
>>>> migration state.
>>>>
>>>> Basically VMSTATE_UINTTL in hw/ is always a bug, because its
>>>> behaviour depends on the size of target_ulong, which is a
>>>> property of the CPU, which is a completely separate device.
>>> I'm not really discussing that, my question is unrelated to
>>> migration/savevm because the patch touches parts that shouldn't be
>>> concerned with migration.  If a particular function (like migration)
>>> needs the type converted to something then that's why C has type
>>> conversions.  A type conversion that compiles to no code is still a
>>> type conversion.
>> Well, target_phys_addr_t is the wrong type here because it's
>> really "at least as large as the widest address type in the
>> system" (cf proposals to make it 64 bits), so using it for
>> a register that must be exactly 32 bits wide is wrong. So we
>> need to change it to something, and customarily what we use
>> for "I am modelling a physical register which is 32 bits wide"
>> is uint32_t. Introducing extra device-specific typedefs to
>> try to label the semantics of device registers seems a bit
>> unnecessary to me.
> If we treat the struct as a representation of the register values
> rather than state of the emulated device then I guess you're right.
> The reason it rings an alarm is that the change is not an improvement
> (other than for migration, but again the change is in code that is not
> related to savevm)
>
> Cheers
>
It's an improvement in a way that it fixes a (style) bug in code, 
VMSTATE_UINTTL* macro are not intended for target_phys_addr_t.

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

Re: [Qemu-devel] [PATCH 2/5] hw/pxa2xx_dma.c: drop VMSTATE_UINTTL usage

[Juan Quintela]

Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> 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 it safely because:
> 1) pxa2xx has 32-bit physical address;
> 2) rest of the code in this file treats these variables as 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).

This is an incompatible change, we need to bump the version.

Looking at pxa2xx_dma_descriptor_fetch() it looks like your change is
enough:
    uint32_t desc[4];

    ....

    s->chan[ch].descr = desc[DDADR];
    s->chan[ch].src = desc[DSADR];
    s->chan[ch].dest = desc[DTADR];
    s->chan[ch].cmd = desc[DCMD];

i.e. we always asign from a 32bit register.

In general change is not valid.

As I don't know if this device can appear as 64bit hardware, I don't
know if the change is valid and let it to the ARM gurus.

Later, Juan.

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

[Juan Quintela]

Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> Do not use VMSTATE_UINTTL* macro for variables that are actually defined as uint64_t
> in CPUAlphaState.

Old code is wrong (TM).  I think that your changes are ok.  

Re: [Qemu-devel] [PATCH 2/5] hw/pxa2xx_dma.c: drop VMSTATE_UINTTL usage

[Peter Maydell]

On 22 February 2012 13:47, Juan Quintela <quintela@redhat.com> wrote:
> Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
>> 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 it safely because:
>> 1) pxa2xx has 32-bit physical address;
>> 2) rest of the code in this file treats these variables as 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).
>
> This is an incompatible change, we need to bump the version.

Why? For the cases where this device is used, target_phys_addr_t is
always 32 bits so we aren't changing anything, surely?

-- PMM

Re: [Qemu-devel] [PATCH 3/5] hw/pxa2xx_lcd.c: drop VMSTATE_UINTTL usage

[Juan Quintela]

andrzej zaborowski <balrogg@gmail.com> wrote:
> On 22 February 2012 13:00, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 22 February 2012 11:36, andrzej zaborowski <balrog@zabor.org> wrote:
>>> On 22 February 2012 11:15, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
>>>> Convert three variables in DMAChannel state from type
>>>> target_phys_addr_t to uint32_t,
>>>> use VMSTATE_UINT32 instead of VMSTATE_UINTTL for these variables.
>>>> We can do it safely because:
>>>> 1) pxa2xx has 32-bit physical address;
>>>> 2) rest of the code in this file treats these variables as uint32_t;
>>>
>>> Why's uint32_t more correct though?  The purpose of using a named type
>>> across qemu is to mark fields as memory addresses (similar to size_t
>>> being used for sizes, etc.), uint32_t conveys less information -- only
>>> the size.
>>>
>>> It's a safe hack, but I don't see the rationale.
>>
>> Because we might change target_phys_addr_t to 64 bits globally
>> some day (it's certainly been mooted) and that shouldn't suddenly
>> change the register width and certainly shouldn't change the
>> migration state.
>>
>> Basically VMSTATE_UINTTL in hw/ is always a bug, because its
>> behaviour depends on the size of target_ulong, which is a
>> property of the CPU, which is a completely separate device.
>
> I'm not really discussing that, my question is unrelated to
> migration/savevm because the patch touches parts that shouldn't be
> concerned with migration.  If a particular function (like migration)
> needs the type converted to something then that's why C has type
> conversions.  A type conversion that compiles to no code is still a
> type conversion.

For migration, UINTTL is _always_ wrong, we need to handle it that way
for backward compatibility.

#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

this was done for CPU's, not devices.  If we use this, we can't access a
device state without knowing the "long-iness" (don't you like the word)
of the cpu that it is running.

IMHO, something that always sent 64bit, and on reception if target_long
is 32bit and value don't fit -> just break migration makes much more
sense.

But that can only be done for new code :-(

On the other hand, I understand andrzej, we are missig a 

target_phys_addr32_t

or whatever we want to call it, that is for devices that _only_ support
32bit addressing.  That is something that is valuable to document,
independently of how migration is done.

Later, Juan.

Re: [Qemu-devel] [PATCH 4/5] vmstate: refactor and move VMSTATE_UINTTL* macro

[Juan Quintela]

Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> Instead of defining VMSTATE_UINTTL* based on TARGET_LONG_BITS value, we can
> use qemu_put_betls/qemu_get_betls functions. These two functions depend on
> TARGET_LONG_BITS as well, and this new approach for VMSTATE_UINTTL* will result
> in the same thing as before (will call qemu_get_be32s/qemu_put_be32s for 32-bit
> target or qemu_put_be64s/qemu_get_be64s for 64-bit target).
> Move VMSTATE_UINTTL* definitions to vmstate.h where they belong.

The idea was to removed the other functions.  Notice that the cases are
equivalent.  i.e. this just bring us a new type that we have to
represent, maintain.  The other makes use to use a define, take your poison.

Later, Juan.

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

[Juan Quintela]

Peter Maydell <peter.maydell@linaro.org> wrote:
> On 22 February 2012 10:15, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
>> This patchset cleans up and optimizes vmstate implementation.
>>
>> 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.h to vmstate.h. Explicit dependency
>> on NEED_CPU_H is droped, I failed to understand why it was presented at all.
>
> So if we apply patches 1-3 (which all look plausible) then the only
> remaining user of VMSTATE_UINTTL is target-i386/machine.c as far as
> I can see.
>
> This leaves me wondering if we shouldn't just put it actually in
> target-i386/machine.c as a convenience macro for that specific CPU
> to avoid having to have more #ifdef TARGET_X86_64s. (I note that
> the machine.c code is already pretty inconsistent, eg lstar and
> cstar are defined as target_ulong and saved with VMSTATE_UINT64.)

With my cpu-vmstate patches, all 32/64 bit cpus use it.

ppc, sparc and mips use it.

Move it to a place that is only used for cpus makes sense, though.

> Basically VMSTATE_UINTTL seems like a bit of a dangerous thing to
> leave lying around as there aren't really very many use cases
> for it...
>
> -- PMM

Re: [Qemu-devel] [PATCH 2/5] hw/pxa2xx_dma.c: drop VMSTATE_UINTTL usage

[Juan Quintela]

Peter Maydell <peter.maydell@linaro.org> wrote:
> On 22 February 2012 13:47, Juan Quintela <quintela@redhat.com> wrote:
>> Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
>>> 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 it safely because:
>>> 1) pxa2xx has 32-bit physical address;
>>> 2) rest of the code in this file treats these variables as 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).
>>
>> This is an incompatible change, we need to bump the version.
>
> Why? For the cases where this device is used, target_phys_addr_t is
> always 32 bits so we aren't changing anything, surely?

You are right, I had jsut forgot that this device is only used in ARM.

I stand corrected.

Later, Juan.

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

[Andreas Färber]

Am 22.02.2012 15:02, schrieb Juan Quintela:
> Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 22 February 2012 10:15, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
>>> This patchset cleans up and optimizes vmstate implementation.
>>>
>>> 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.h to vmstate.h. Explicit dependency
>>> on NEED_CPU_H is droped, I failed to understand why it was presented at all.
>>
>> So if we apply patches 1-3 (which all look plausible) then the only
>> remaining user of VMSTATE_UINTTL is target-i386/machine.c as far as
>> I can see.
>>
>> This leaves me wondering if we shouldn't just put it actually in
>> target-i386/machine.c as a convenience macro for that specific CPU
>> to avoid having to have more #ifdef TARGET_X86_64s. (I note that
>> the machine.c code is already pretty inconsistent, eg lstar and
>> cstar are defined as target_ulong and saved with VMSTATE_UINT64.)
> 
> With my cpu-vmstate patches, all 32/64 bit cpus use it.

NB: Your cpu-vmstate patches were not applied so far and they appear to
conflict with the plans we've made for redesigning cp15 on ARM: We want
to convert today's static fields to some list and were hoping to have a
mapping function for backwards compatibility. That works easiest in
imperative code.

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 0/5] VMState cleanups

[Peter Maydell]

On 22 February 2012 15:37, Andreas Färber <afaerber@suse.de> wrote:
> NB: Your cpu-vmstate patches were not applied so far and they appear to
> conflict with the plans we've made for redesigning cp15 on ARM: We want
> to convert today's static fields to some list and were hoping to have a
> mapping function for backwards compatibility. That works easiest in
> imperative code.

I thought the idea for cp15 for vmstate was (like ppc) to basically
have a uint32_t cp15_regs[512] which we save/load the whole of, and
then the mapping function just assigns semantics to some subset
of that array? vmstate can do a plain array without problems.
(There's an argument that this is just shoving the attempts to
maintain backcompat out of vmstate's machinery and into some
hand-written code though.)

[I have 'write up results of the last week or two's conversations'
on my todo list as the next cp15 thing to do...]

-- PMM

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

[Andreas Färber]

Am 22.02.2012 16:42, schrieb Peter Maydell:
> On 22 February 2012 15:37, Andreas Färber <afaerber@suse.de> wrote:
>> NB: Your cpu-vmstate patches were not applied so far and they appear to
>> conflict with the plans we've made for redesigning cp15 on ARM: We want
>> to convert today's static fields to some list and were hoping to have a
>> mapping function for backwards compatibility. That works easiest in
>> imperative code.
> 
> I thought the idea for cp15 for vmstate was (like ppc) to basically
> have a uint32_t cp15_regs[512] which we save/load the whole of, and
> then the mapping function just assigns semantics to some subset
> of that array? vmstate can do a plain array without problems.

I thought we had concluded that the (3+3+4+4)² or so registers were too
large for that so that Alex suggested to leave the old load/save in
place (but getting/setting through a mapping function) and dynamically
appending only the new cp15 registers we don't have fields for yet when
some arrive. Or so I've understood.

I was planning some cp15 coding once I've moved some more stuff out of
CPU_COMMON and resolved the pxa270 CPU classes mess.

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 0/5] VMState cleanups

[Peter Maydell]

On 22 February 2012 16:04, Andreas Färber <afaerber@suse.de> wrote:
> Am 22.02.2012 16:42, schrieb Peter Maydell:
>> On 22 February 2012 15:37, Andreas Färber <afaerber@suse.de> wrote:
>>> NB: Your cpu-vmstate patches were not applied so far and they appear to
>>> conflict with the plans we've made for redesigning cp15 on ARM: We want
>>> to convert today's static fields to some list and were hoping to have a
>>> mapping function for backwards compatibility. That works easiest in
>>> imperative code.
>>
>> I thought the idea for cp15 for vmstate was (like ppc) to basically
>> have a uint32_t cp15_regs[512] which we save/load the whole of, and
>> then the mapping function just assigns semantics to some subset
>> of that array? vmstate can do a plain array without problems.
>
> I thought we had concluded that the (3+3+4+4)² or so registers were too
> large for that so that Alex suggested to leave the old load/save in
> place (but getting/setting through a mapping function) and dynamically
> appending only the new cp15 registers we don't have fields for yet when
> some arrive. Or so I've understood.

So what I thought Alex was suggesting was to nuke the existing
save/load, and instead we have this generic array. All the
current env->cp15.c1_scr &co turn from being uint32_t to uint32_t*,
and there's an init function per CPU which maps those to point at
slots in the cp15_regs[] array. Indexes into cp15_regs[] are
just arbitrary (though they can't change for a particular CPU
variant or you'd break migration).

The point of this is basically to decouple the save/load format
for different ARM cpu variants from each other, so we can add
registers for (say) A15 without breaking migration compatibility
for (say) A9.

> I was planning some cp15 coding once I've moved some more stuff out of
> CPU_COMMON and resolved the pxa270 CPU classes mess.

I really am going to get on and finish up the half-a-series I have,
honest...

-- PMM

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

[Alexander Graf]

On 22.02.2012, at 17:09, Peter Maydell wrote:

> On 22 February 2012 16:04, Andreas Färber <afaerber@suse.de> wrote:
>> Am 22.02.2012 16:42, schrieb Peter Maydell:
>>> On 22 February 2012 15:37, Andreas Färber <afaerber@suse.de> wrote:
>>>> NB: Your cpu-vmstate patches were not applied so far and they appear to
>>>> conflict with the plans we've made for redesigning cp15 on ARM: We want
>>>> to convert today's static fields to some list and were hoping to have a
>>>> mapping function for backwards compatibility. That works easiest in
>>>> imperative code.
>>> 
>>> I thought the idea for cp15 for vmstate was (like ppc) to basically
>>> have a uint32_t cp15_regs[512] which we save/load the whole of, and
>>> then the mapping function just assigns semantics to some subset
>>> of that array? vmstate can do a plain array without problems.
>> 
>> I thought we had concluded that the (3+3+4+4)² or so registers were too
>> large for that so that Alex suggested to leave the old load/save in
>> place (but getting/setting through a mapping function) and dynamically
>> appending only the new cp15 registers we don't have fields for yet when
>> some arrive. Or so I've understood.
> 
> So what I thought Alex was suggesting was to nuke the existing
> save/load, and instead we have this generic array. All the
> current env->cp15.c1_scr &co turn from being uint32_t to uint32_t*,
> and there's an init function per CPU which maps those to point at
> slots in the cp15_regs[] array. Indexes into cp15_regs[] are
> just arbitrary (though they can't change for a particular CPU
> variant or you'd break migration).

Yup. A nice side effect of this is that you have a known-small size of cp15_regs[]. But I suggested a lot of things during that discussion, so I quite frankly don't remember if that was the conclusion or just one idea ;)


Alex

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

[Juan Quintela]

Alexander Graf <agraf@suse.de> wrote:
> On 22.02.2012, at 17:09, Peter Maydell wrote:
>
>> On 22 February 2012 16:04, Andreas Färber <afaerber@suse.de> wrote:
>>> Am 22.02.2012 16:42, schrieb Peter Maydell:
>>>> On 22 February 2012 15:37, Andreas Färber <afaerber@suse.de> wrote:
>>>>> NB: Your cpu-vmstate patches were not applied so far and they appear to
>>>>> conflict with the plans we've made for redesigning cp15 on ARM: We want
>>>>> to convert today's static fields to some list and were hoping to have a
>>>>> mapping function for backwards compatibility. That works easiest in
>>>>> imperative code.
>>>> 
>>>> I thought the idea for cp15 for vmstate was (like ppc) to basically
>>>> have a uint32_t cp15_regs[512] which we save/load the whole of, and
>>>> then the mapping function just assigns semantics to some subset
>>>> of that array? vmstate can do a plain array without problems.
>>> 
>>> I thought we had concluded that the (3+3+4+4)² or so registers were too
>>> large for that so that Alex suggested to leave the old load/save in
>>> place (but getting/setting through a mapping function) and dynamically
>>> appending only the new cp15 registers we don't have fields for yet when
>>> some arrive. Or so I've understood.
>> 
>> So what I thought Alex was suggesting was to nuke the existing
>> save/load, and instead we have this generic array. All the
>> current env->cp15.c1_scr &co turn from being uint32_t to uint32_t*,
>> and there's an init function per CPU which maps those to point at
>> slots in the cp15_regs[] array. Indexes into cp15_regs[] are
>> just arbitrary (though they can't change for a particular CPU
>> variant or you'd break migration).
>
> Yup. A nice side effect of this is that you have a known-small size of
> cp15_regs[]. But I suggested a lot of things during that discussion,
> so I quite frankly don't remember if that was the conclusion or just
> one idea ;)

I have to search that discussion, but I would like to send things as:

{ register_name, register value} array, otherwise inter-version
migration is just impossible (or very painful, that is similar).

Notice that this would also be very useful for x86 and MSR's.  Just now
we send every MSR ad-hock with a new position.

Later, Juan.