[Qemu-devel] [0/7] vmstate extensions

[Qemu-devel] [0/7] vmstate extensions

[David Gibson]

This is the second posting of a patch series to make straightforward
extensions to the savevm / vmstate / migration infrastructure.  This
is chiefly extra helper macros in vmstate.h to fill trivial gaps in
functionality.

v1->v2:
 * Fixed small style bug
 * Added new extension in patch 7/7

[Qemu-devel] [PATCH 1/7] savevm: Add VMSTATE_UINT64_EQUAL helpers

[David Gibson]

The savevm code already includes a number of *_EQUAL helpers which act as
sanity checks verifying that the configuration of the saved state matches
that of the machine we're loading into to work.  Variants already exist
for 8 bit 16 bit and 32 bit integers, but not 64 bit integers.  This patch
fills that hole, adding a UINT64 version.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
v2: Added missing braces
---
 savevm.c  |   21 +++++++++++++++++++++
 vmstate.h |    7 +++++++
 2 files changed, 28 insertions(+)

diff --git a/savevm.c b/savevm.c
index 31fd2e0..fcbc706 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1043,6 +1043,27 @@ const VMStateInfo vmstate_info_uint64 = {
     .put  = put_uint64,
 };
 
+/* 64 bit unsigned int. See that the received value is the same than the one
+   in the field */
+
+static int get_uint64_equal(QEMUFile *f, void *pv, size_t size)
+{
+    uint64_t *v = pv;
+    uint64_t v2;
+    qemu_get_be64s(f, &v2);
+
+    if (*v == v2) {
+        return 0;
+    }
+    return -EINVAL;
+}
+
+const VMStateInfo vmstate_info_uint64_equal = {
+    .name = "int64 equal",
+    .get  = get_uint64_equal,
+    .put  = put_uint64,
+};
+
 /* 8 bit int. See that the received value is the same than the one
    in the field */
 
diff --git a/vmstate.h b/vmstate.h
index c9c320e..6c7fbe0 100644
--- a/vmstate.h
+++ b/vmstate.h
@@ -129,6 +129,7 @@ extern const VMStateInfo vmstate_info_uint8_equal;
 extern const VMStateInfo vmstate_info_uint16_equal;
 extern const VMStateInfo vmstate_info_int32_equal;
 extern const VMStateInfo vmstate_info_uint32_equal;
+extern const VMStateInfo vmstate_info_uint64_equal;
 extern const VMStateInfo vmstate_info_int32_le;
 
 extern const VMStateInfo vmstate_info_uint8;
@@ -488,6 +489,12 @@ extern const VMStateInfo vmstate_info_unused_buffer;
 #define VMSTATE_UINT32_EQUAL(_f, _s)                                   \
     VMSTATE_SINGLE(_f, _s, 0, vmstate_info_uint32_equal, uint32_t)
 
+#define VMSTATE_UINT64_EQUAL_V(_f, _s, _v)                            \
+    VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint64_equal, uint64_t)
+
+#define VMSTATE_UINT64_EQUAL(_f, _s)                                  \
+    VMSTATE_UINT64_EQUAL_V(_f, _s, 0)
+
 #define VMSTATE_INT32_LE(_f, _s)                                   \
     VMSTATE_SINGLE(_f, _s, 0, vmstate_info_int32_le, int32_t)
 
-- 
1.7.10.4

[Qemu-devel] [PATCH 2/7] savevm: Add VMSTATE_UINTTL_EQUAL helper

[David Gibson]

This adds an _EQUAL VMSTATE helper for target_ulongs, defined in terms of
VMSTATE_UINT32_EQUAL or VMSTATE_UINT64_EQUAL as appropriate.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/hw.h   |    6 ++++++
 vmstate.h |    7 +++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/hw.h b/hw/hw.h
index 16101de..b727b0e 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -50,16 +50,22 @@ int qemu_boot_set(const char *boot_devices);
 #if TARGET_LONG_BITS == 64
 #define VMSTATE_UINTTL_V(_f, _s, _v)                                  \
     VMSTATE_UINT64_V(_f, _s, _v)
+#define VMSTATE_UINTTL_EQUAL_V(_f, _s, _v)                            \
+    VMSTATE_UINT64_EQUAL_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_EQUAL_V(_f, _s, _v)                            \
+    VMSTATE_UINT32_EQUAL_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_EQUAL(_f, _s)                                  \
+    VMSTATE_UINTTL_EQUAL_V(_f, _s, 0)
 #define VMSTATE_UINTTL_ARRAY(_f, _s, _n)                              \
     VMSTATE_UINTTL_ARRAY_V(_f, _s, _n, 0)
 
diff --git a/vmstate.h b/vmstate.h
index 6c7fbe0..5d1c4f5 100644
--- a/vmstate.h
+++ b/vmstate.h
@@ -486,8 +486,11 @@ extern const VMStateInfo vmstate_info_unused_buffer;
 #define VMSTATE_INT32_EQUAL(_f, _s)                                   \
     VMSTATE_SINGLE(_f, _s, 0, vmstate_info_int32_equal, int32_t)
 
-#define VMSTATE_UINT32_EQUAL(_f, _s)                                   \
-    VMSTATE_SINGLE(_f, _s, 0, vmstate_info_uint32_equal, uint32_t)
+#define VMSTATE_UINT32_EQUAL_V(_f, _s, _v)                            \
+    VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint32_equal, uint32_t)
+
+#define VMSTATE_UINT32_EQUAL(_f, _s)                                  \
+    VMSTATE_UINT32_EQUAL_V(_f, _s, 0)
 
 #define VMSTATE_UINT64_EQUAL_V(_f, _s, _v)                            \
     VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint64_equal, uint64_t)
-- 
1.7.10.4

[Qemu-devel] [PATCH 3/7] savevm: Add VMSTATE_FLOAT64 helpers

[David Gibson]

The current savevm code includes VMSTATE helpers for a number of commonly
used data types, but not for the float64 type used by the internal floating
point emulation code.  This patch fixes the deficiency.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 savevm.c  |   23 +++++++++++++++++++++++
 vmstate.h |   15 +++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/savevm.c b/savevm.c
index fcbc706..02e9da0 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1104,6 +1104,29 @@ const VMStateInfo vmstate_info_uint16_equal = {
     .put  = put_uint16,
 };
 
+/* floating point */
+
+static int get_float64(QEMUFile *f, void *pv, size_t size)
+{
+    float64 *v = pv;
+
+    *v = make_float64(qemu_get_be64(f));
+    return 0;
+}
+
+static void put_float64(QEMUFile *f, void *pv, size_t size)
+{
+    uint64_t *v = pv;
+
+    qemu_put_be64(f, float64_val(*v));
+}
+
+const VMStateInfo vmstate_info_float64 = {
+    .name = "float64",
+    .get  = get_float64,
+    .put  = put_float64,
+};
+
 /* timers  */
 
 static int get_timer(QEMUFile *f, void *pv, size_t size)
diff --git a/vmstate.h b/vmstate.h
index 5d1c4f5..a04561e 100644
--- a/vmstate.h
+++ b/vmstate.h
@@ -137,6 +137,8 @@ extern const VMStateInfo vmstate_info_uint16;
 extern const VMStateInfo vmstate_info_uint32;
 extern const VMStateInfo vmstate_info_uint64;
 
+extern const VMStateInfo vmstate_info_float64;
+
 extern const VMStateInfo vmstate_info_timer;
 extern const VMStateInfo vmstate_info_buffer;
 extern const VMStateInfo vmstate_info_unused_buffer;
@@ -510,6 +512,13 @@ extern const VMStateInfo vmstate_info_unused_buffer;
 #define VMSTATE_UINT32_TEST(_f, _s, _t)                                  \
     VMSTATE_SINGLE_TEST(_f, _s, _t, 0, vmstate_info_uint32, uint32_t)
 
+
+#define VMSTATE_FLOAT64_V(_f, _s, _v)                                 \
+    VMSTATE_SINGLE(_f, _s, _v, vmstate_info_float64, float64)
+
+#define VMSTATE_FLOAT64(_f, _s)                                       \
+    VMSTATE_FLOAT64_V(_f, _s, 0)
+
 #define VMSTATE_TIMER_TEST(_f, _s, _test)                             \
     VMSTATE_POINTER_TEST(_f, _s, _test, vmstate_info_timer, QEMUTimer *)
 
@@ -576,6 +585,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_FLOAT64_ARRAY_V(_f, _s, _n, _v)                       \
+    VMSTATE_ARRAY(_f, _s, _n, _v, vmstate_info_float64, float64)
+
+#define VMSTATE_FLOAT64_ARRAY(_f, _s, _n)                             \
+    VMSTATE_FLOAT64_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.10.4

[Qemu-devel] [PATCH 4/7] savevm: Add VMSTATE_ helpers for target_phys_addr_t

[David Gibson]

The savevm code contains VMSTATE_ helpers for a number of commonly used
types, but not for target_phys_addr_t.  This patch fixes that deficiency
implementing VMSTATE_TPA helpers in terms of VMSTATE_UINT32 or
VMSTATE_UINT64 helpers as appropriate.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 targphys.h |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/targphys.h b/targphys.h
index 08cade9..a1c20b4 100644
--- a/targphys.h
+++ b/targphys.h
@@ -17,4 +17,15 @@ typedef uint64_t target_phys_addr_t;
 #define TARGET_PRIxPHYS PRIx64
 #define TARGET_PRIXPHYS PRIX64
 
+#define VMSTATE_TPA_V(_f, _s, _v) \
+    VMSTATE_UINT64_V(_f, _s, _v)
+
+#define VMSTATE_TPA_EQUAL_V(_f, _s, _v) \
+    VMSTATE_UINT64_EQUAL_V(_f, _s, _v)
+
+#define VMSTATE_TPA(_f, _s) \
+    VMSTATE_TPA_V(_f, _s, 0)
+#define VMSTATE_TPA_EQUAL(_f, _s) \
+    VMSTATE_TPA_EQUAL_V(_f, _s, 0)
+
 #endif
-- 
1.7.10.4

[Qemu-devel] [PATCH 5/7] savevm: Add VMSTATE_STRUCT_VARRAY_POINTER_UINT32

[David Gibson]

Currently the savevm code contains a VMSTATE_STRUCT_VARRAY_POINTER_INT32
helper (a variably sized array with the number of elements in an int32_t),
but not VMSTATE_STRUCT_VARRAY_POINTER_UINT32 (... with the number of
elements in a uint32_t).  This patch (trivially) fixes the deficiency.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 vmstate.h |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/vmstate.h b/vmstate.h
index a04561e..4b393a0 100644
--- a/vmstate.h
+++ b/vmstate.h
@@ -322,6 +322,16 @@ extern const VMStateInfo vmstate_info_unused_buffer;
     .offset     = vmstate_offset_pointer(_state, _field, _type),     \
 }
 
+#define VMSTATE_STRUCT_VARRAY_POINTER_UINT32(_field, _state, _field_num, _vmsd, _type) { \
+    .name       = (stringify(_field)),                               \
+    .version_id = 0,                                                 \
+    .num_offset = vmstate_offset_value(_state, _field_num, uint32_t),\
+    .size       = sizeof(_type),                                     \
+    .vmsd       = &(_vmsd),                                          \
+    .flags      = VMS_POINTER | VMS_VARRAY_INT32 | VMS_STRUCT,       \
+    .offset     = vmstate_offset_pointer(_state, _field, _type),     \
+}
+
 #define VMSTATE_STRUCT_VARRAY_POINTER_UINT16(_field, _state, _field_num, _vmsd, _type) { \
     .name       = (stringify(_field)),                               \
     .version_id = 0,                                                 \
-- 
1.7.10.4

[Qemu-devel] [PATCH 6/7] savevm: Fix bugs in the VMSTATE_VBUFFER_MULTIPLY definition

[David Gibson]

The VMSTATE_BUFFER_MULTIPLY macro is misnamed - it actually specifies
a variably sized buffer with VMS_VBUFFER, so should be named
VMSTATE_VBUFFER_MULTIPLY.  This patch fixes this (the macro had no current
users under either name).

In addition, unlike the other VMSTATE_VBUFFER variants, this macro did not
specify VMS_POINTER.  This patch fixes this bug as well.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 vmstate.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/vmstate.h b/vmstate.h
index 4b393a0..6bfdb6a 100644
--- a/vmstate.h
+++ b/vmstate.h
@@ -372,14 +372,14 @@ 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_MULTIPLY(_field, _state, _version, _test, _start, _field_size, _multiply) { \
     .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,                        \
+    .flags        = VMS_VBUFFER|VMS_POINTER|VMS_MULTIPLY,            \
     .offset       = offsetof(_state, _field),                        \
     .start        = (_start),                                        \
 }
-- 
1.7.10.4

[Qemu-devel] [PATCH 7/7] savevm: Implement VMS_DIVIDE flag

[David Gibson]

The vmstate infrastructure includes a VMS_MULTIPY flag, and associated
VMSTATE_VBUFFER_MULTIPLY helper macro.  These can be used to save a
variably sized buffer where the size in bytes of the buffer isn't directly
accessible as a structure field, but an element count from which the size
can be derived is.

This patch adds an analogous VMS_DIVIDE option, which handles a variably
sized buffer whose size is a submultiple of a field, rather than a
multiple.  For example a buffer containing per-page structures whose size
is derived from a field storing the total address space described by the
structures could use this construct.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 savevm.c  |    8 ++++++++
 vmstate.h |   13 +++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/savevm.c b/savevm.c
index 02e9da0..d830837 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1460,6 +1460,10 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
                 if (field->flags & VMS_MULTIPLY) {
                     size *= field->size;
                 }
+                if (field->flags & VMS_DIVIDE) {
+                    assert((size % field->size) == 0);
+                    size /= field->size;
+                }
             }
             if (field->flags & VMS_ARRAY) {
                 n_elems = field->num;
@@ -1524,6 +1528,10 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
                 if (field->flags & VMS_MULTIPLY) {
                     size *= field->size;
                 }
+                if (field->flags & VMS_DIVIDE) {
+                    assert((size % field->size) == 0);
+                    size /= field->size;
+                }
             }
             if (field->flags & VMS_ARRAY) {
                 n_elems = field->num;
diff --git a/vmstate.h b/vmstate.h
index 6bfdb6a..634dbde 100644
--- a/vmstate.h
+++ b/vmstate.h
@@ -82,6 +82,7 @@ enum VMStateFlags {
     VMS_MULTIPLY         = 0x200,  /* multiply "size" field by field_size */
     VMS_VARRAY_UINT8     = 0x400,  /* Array with size in uint8_t field*/
     VMS_VARRAY_UINT32    = 0x800,  /* Array with size in uint32_t field*/
+    VMS_DIVIDE           = 0x1000, /* divide "size" field by field_size */
 };
 
 typedef struct {
@@ -384,6 +385,18 @@ extern const VMStateInfo vmstate_info_unused_buffer;
     .start        = (_start),                                        \
 }
 
+#define VMSTATE_VBUFFER_DIVIDE(_field, _state, _version, _test, _start, _field_size, _divide) { \
+    .name         = (stringify(_field)),                             \
+    .version_id   = (_version),                                      \
+    .field_exists = (_test),                                         \
+    .size_offset  = vmstate_offset_value(_state, _field_size, uint32_t),\
+    .size         = (_divide),                                       \
+    .info         = &vmstate_info_buffer,                            \
+    .flags        = VMS_VBUFFER|VMS_POINTER|VMS_DIVIDE,              \
+    .offset       = offsetof(_state, _field),                        \
+    .start        = (_start),                                        \
+}
+
 #define VMSTATE_VBUFFER(_field, _state, _version, _test, _start, _field_size) { \
     .name         = (stringify(_field)),                             \
     .version_id   = (_version),                                      \
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 4/7] savevm: Add VMSTATE_ helpers for target_phys_addr_t

[Peter Maydell]

On 9 October 2012 05:53, David Gibson <david@gibson.dropbear.id.au> wrote:
> The savevm code contains VMSTATE_ helpers for a number of commonly used
> types, but not for target_phys_addr_t.  This patch fixes that deficiency
> implementing VMSTATE_TPA helpers in terms of VMSTATE_UINT32 or
> VMSTATE_UINT64 helpers as appropriate.

(This comment is out of date...)

Traditionally we have deliberately not had any vmstate helpers
for target_phys_addr_t because the meaning of that type is "a
type wide enough to hold the widest address in use in the system".
vmstate fields, on the other hand, are for the state of a specific
device model, and a particular device's registers are always a
fixed width. Even if a pci card model is plugged into a system
with 64 bit PCI addresses, if the chip on the card itself only
has 32 bit wide registers controlling DMA those registers need
to be uint32_t. So if you think you need to put target_phys_addr_t
into a vmstate the chances are you don't. I think this logic
should still hold even now we have made target_phys_addr_t
64 bits for every platform.

What was the problem this patch is trying to fix?

-- PMM

Re: [Qemu-devel] [PATCH 4/7] savevm: Add VMSTATE_ helpers for target_phys_addr_t

[David Gibson]

On Tue, Oct 09, 2012 at 09:03:21AM +0100, Peter Maydell wrote:
> On 9 October 2012 05:53, David Gibson <david@gibson.dropbear.id.au> wrote:
> > The savevm code contains VMSTATE_ helpers for a number of commonly used
> > types, but not for target_phys_addr_t.  This patch fixes that deficiency
> > implementing VMSTATE_TPA helpers in terms of VMSTATE_UINT32 or
> > VMSTATE_UINT64 helpers as appropriate.
> 
> (This comment is out of date...)

Ah, yes.

> Traditionally we have deliberately not had any vmstate helpers
> for target_phys_addr_t because the meaning of that type is "a
> type wide enough to hold the widest address in use in the system".
> vmstate fields, on the other hand, are for the state of a specific
> device model, and a particular device's registers are always a
> fixed width. Even if a pci card model is plugged into a system
> with 64 bit PCI addresses, if the chip on the card itself only
> has 32 bit wide registers controlling DMA those registers need
> to be uint32_t. So if you think you need to put target_phys_addr_t
> into a vmstate the chances are you don't. I think this logic
> should still hold even now we have made target_phys_addr_t
> 64 bits for every platform.
> 
> What was the problem this patch is trying to fix?

Well, the place I've used this (in patches yet to be posted) is saving
the state of the pseries machine itself.  Specifically, I use
VMSTATE_TPA_EQUAL to sanity check that the restored machine has the
same ram size as the saved machine.  In this case the variable is qemu
internal information and represents a ram size, so I think
target_phys_addr_t is the correct type here.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [PATCH 4/7] savevm: Add VMSTATE_ helpers for target_phys_addr_t

[Peter Maydell]

On 9 October 2012 13:53, David Gibson <dwg@au1.ibm.com> wrote:
> Well, the place I've used this (in patches yet to be posted) is saving
> the state of the pseries machine itself.  Specifically, I use
> VMSTATE_TPA_EQUAL to sanity check that the restored machine has the
> same ram size as the saved machine.

This doesn't sound like the sort of check that should be in
a specific machine model -- you should be able to rely on
common qemu code to handle this. (If you can't then we need
a check in the common code...)

-- PMM

Re: [Qemu-devel] [PATCH 4/7] savevm: Add VMSTATE_ helpers for target_phys_addr_t

[David Gibson]

On Tue, Oct 09, 2012 at 05:24:29PM +0100, Peter Maydell wrote:
> On 9 October 2012 13:53, David Gibson <dwg@au1.ibm.com> wrote:
> > Well, the place I've used this (in patches yet to be posted) is saving
> > the state of the pseries machine itself.  Specifically, I use
> > VMSTATE_TPA_EQUAL to sanity check that the restored machine has the
> > same ram size as the saved machine.
> 
> This doesn't sound like the sort of check that should be in
> a specific machine model -- you should be able to rely on
> common qemu code to handle this. (If you can't then we need
> a check in the common code...)

Hm, true.  Very well, I'll drop it, and the VMSTATE_TPA patch as
well.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [PATCH 4/7] savevm: Add VMSTATE_ helpers for target_phys_addr_t

[David Gibson]

On Wed, Oct 10, 2012 at 11:17:31AM +1100, David Gibson wrote:
> On Tue, Oct 09, 2012 at 05:24:29PM +0100, Peter Maydell wrote:
> > On 9 October 2012 13:53, David Gibson <dwg@au1.ibm.com> wrote:
> > > Well, the place I've used this (in patches yet to be posted) is saving
> > > the state of the pseries machine itself.  Specifically, I use
> > > VMSTATE_TPA_EQUAL to sanity check that the restored machine has the
> > > same ram size as the saved machine.
> > 
> > This doesn't sound like the sort of check that should be in
> > a specific machine model -- you should be able to rely on
> > common qemu code to handle this. (If you can't then we need
> > a check in the common code...)
> 
> Hm, true.  Very well, I'll drop it, and the VMSTATE_TPA patch as
> well.

Actually, turns out I had another use of these helpers.  That was to
store the real page address from the ppcmeb_tlb_t structure.  That
structure is used to represent TLB entries on a number of different
embedded chips, which don't all have the same physical bus width.  So
target_phys_addr_t does seem like the correct type there.

Obviously I could change the type to a fixed uint64_t, but I'm not
sure if that's a better idea than bringing in the VMSTATE_TPA
helpers.  Advice?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [PATCH 4/7] savevm: Add VMSTATE_ helpers for target_phys_addr_t

[Peter Maydell]

On 11 October 2012 02:57, David Gibson <dwg@au1.ibm.com> wrote:
> Actually, turns out I had another use of these helpers.  That was to
> store the real page address from the ppcmeb_tlb_t structure.  That
> structure is used to represent TLB entries on a number of different
> embedded chips, which don't all have the same physical bus width.  So
> target_phys_addr_t does seem like the correct type there.

> Obviously I could change the type to a fixed uint64_t, but I'm not
> sure if that's a better idea than bringing in the VMSTATE_TPA
> helpers.  Advice?

PPC has had 64 bit target_phys_addr_t even before the recent change.
Either these various CPUs all actually have physical hardware
TLB configs which hold the full 64 bits (use uint64_t) or they
have physical configs which vary between them (in which case
you're presumably mismodelling some of them) and you need to
use different types or alternatively always use uint64_t and
do masking on writes to the fields or something.

If you're happy to continue to bake the assumption into your
code that target_phys_addr_t is 64 bits, you can just use
VMSTATE_UINT64 which (I think) should work OK when t_p_a_t
really is 64 bits, and will give a helpful compile failure
if the assumption is ever untrue...

Basically I think that to the extent that we contemplate
t_p_a_t as distinct from "64 bit uint" it's a bad idea to put
it into vmstate because it means potential migration
compatibility breaks if it changes.

-- PMM

Re: [Qemu-devel] [PATCH 4/7] savevm: Add VMSTATE_ helpers for target_phys_addr_t

[David Gibson]

On Thu, Oct 11, 2012 at 04:07:24PM +0100, Peter Maydell wrote:
> On 11 October 2012 02:57, David Gibson <dwg@au1.ibm.com> wrote:
> > Actually, turns out I had another use of these helpers.  That was to
> > store the real page address from the ppcmeb_tlb_t structure.  That
> > structure is used to represent TLB entries on a number of different
> > embedded chips, which don't all have the same physical bus width.  So
> > target_phys_addr_t does seem like the correct type there.
> 
> > Obviously I could change the type to a fixed uint64_t, but I'm not
> > sure if that's a better idea than bringing in the VMSTATE_TPA
> > helpers.  Advice?
> 
> PPC has had 64 bit target_phys_addr_t even before the recent change.
> Either these various CPUs all actually have physical hardware
> TLB configs which hold the full 64 bits (use uint64_t) or they
> have physical configs which vary between them (in which case
> you're presumably mismodelling some of them) and you need to
> use different types or alternatively always use uint64_t and
> do masking on writes to the fields or something.

Masking will be done in the handling of the instructions that access
the TLB.  They do have different physical configs, but they also have
different access methods, the data type as it is is sufficient for
both of them.

> If you're happy to continue to bake the assumption into your
> code that target_phys_addr_t is 64 bits, you can just use
> VMSTATE_UINT64 which (I think) should work OK when t_p_a_t
> really is 64 bits, and will give a helpful compile failure
> if the assumption is ever untrue...
> 
> Basically I think that to the extent that we contemplate
> t_p_a_t as distinct from "64 bit uint" it's a bad idea to put
> it into vmstate because it means potential migration
> compatibility breaks if it changes.

Hm, yeah, that I'll grant you.  Ok, I'll remove it.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson