* [Qemu-devel] [very-WIP 0/4] Migration: VMSTATE_WITH_TMP
@ 2016-10-11 17:18 Dr. David Alan Gilbert (git)
2016-10-11 17:18 ` [Qemu-devel] [very-WIP 1/7] migration: Add VMSTATE_WITH_TMP Dr. David Alan Gilbert (git)
` (3 more replies)
0 siblings, 4 replies; 17+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2016-10-11 17:18 UTC (permalink / raw)
To: qemu-devel, amit.shah, quintela, duanj, pasic; +Cc: david
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Hi,
I'm interested in comments on the idea here, it's certainly
not ready to go in yet; and VERY untested.
Here I've added a macro, VMSTATE_WITH_TMP which allows
you to allocate a temporary structure, transmit that with a
VMStateDescription and then free that temporary.
A pointer to the parent structure is set up in the temporary
allowing the pre_load/pre_save/post_load off the child vmsd
to copy/calculate things to fill in the temporary.
There are 4 patches:
a) Add VMSTATE_WITH_TMP
b) A test for (a)
An easy read to see how (a) works
c) Conversion of SLIRPs sbuf structure to a VMState
This is a nice use of VMSTATE_WITH_TMP since
it needs to calculate and send a couple of offsets
based off the contents of the structure but not
the raw fields.
d) Conversion of virtio-net to VMState; this is
pretty hairy, and the previous version I posted
added a bunch of migration only variables into the
structure; here I use 3 VMSTATE_WITH_TMP's to
avoid it.
Both (c) & (d) depend on a whole bunch of related
patches I'm still cooking, but they should let people
see how VMSTATE_WITH_TMP is used.
I think it works OK; it's not exactly compact though,
you often seem to end up with more VMStateDescription
boiler plate and quite a few trivial pre_load/pre_save.
Note this isn't standalone; it depends on Jianjun's
patches that modify the .get/.put parameters and a bunch
of smaller patches of mine.
Comments welcome,
Dave
Dr. David Alan Gilbert (7):
migration: Add VMSTATE_WITH_TMP
tests/migration: Add test for VMSTATE_WITH_TMP
virtio/migration: Migrate virtio-net to VMState
slirp: VMStatify sbuf
--
2.9.3
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [very-WIP 1/7] migration: Add VMSTATE_WITH_TMP
2016-10-11 17:18 [Qemu-devel] [very-WIP 0/4] Migration: VMSTATE_WITH_TMP Dr. David Alan Gilbert (git)
@ 2016-10-11 17:18 ` Dr. David Alan Gilbert (git)
2016-10-17 3:31 ` David Gibson
2016-10-11 17:18 ` [Qemu-devel] [very-WIP 2/7] tests/migration: Add test for VMSTATE_WITH_TMP Dr. David Alan Gilbert (git)
` (2 subsequent siblings)
3 siblings, 1 reply; 17+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2016-10-11 17:18 UTC (permalink / raw)
To: qemu-devel, amit.shah, quintela, duanj, pasic; +Cc: david
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
VMSTATE_WITH_TMP is for handling structures where some calculation
or rearrangement of the data needs to be performed before the data
hits the wire.
For example, where the value on the wire is an offset from a
non-migrated base, but the data in the structure is the actual pointer.
To use it, a temporary type is created and a vmsd used on that type.
The first element of the type must be 'parent' a pointer back to the
type of the main structure. VMSTATE_WITH_TMP takes care of allocating
and freeing the temporary before running the child vmsd.
The post_load/pre_save on the child vmsd can copy things from the parent
to the temporary using the parent pointer and do any other calculations
needed; it can then use normal VMSD entries to do the actual data
storage without having to fiddle around with qemu_get_*/qemu_put_*
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
include/migration/vmstate.h | 20 ++++++++++++++++++++
migration/vmstate.c | 38 ++++++++++++++++++++++++++++++++++++++
2 files changed, 58 insertions(+)
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 9500da1..efb0e90 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -259,6 +259,7 @@ extern const VMStateInfo vmstate_info_cpudouble;
extern const VMStateInfo vmstate_info_timer;
extern const VMStateInfo vmstate_info_buffer;
extern const VMStateInfo vmstate_info_unused_buffer;
+extern const VMStateInfo vmstate_info_tmp;
extern const VMStateInfo vmstate_info_bitmap;
extern const VMStateInfo vmstate_info_qtailq;
@@ -651,6 +652,25 @@ extern const VMStateInfo vmstate_info_qtailq;
.offset = offsetof(_state, _field), \
}
+/* Allocate a temporary of type 'tmp_type', set tmp->parent to _state
+ * and execute the vmsd on the temporary. Note that we're working with
+ * the whole of _state here, not a field within it.
+ * We compile time check that:
+ * That _tmp_type contains a 'parent' member that's a pointer to the
+ * '_state' type
+ * That the pointer is right at the start of _tmp_type.
+ */
+#define VMSTATE_WITH_TMP(_state, _tmp_type, _vmsd) { \
+ .name = "tmp", \
+ .size = sizeof(_tmp_type) + \
+ QEMU_BUILD_BUG_EXPR(offsetof(_tmp_type, parent) != 0) + \
+ type_check_pointer(_state, \
+ typeof_field(_tmp_type, parent)), \
+ .vmsd = &(_vmsd), \
+ .info = &vmstate_info_tmp, \
+ .flags = VMS_LINKED, \
+}
+
#define VMSTATE_UNUSED_BUFFER(_test, _version, _size) { \
.name = "unused", \
.field_exists = (_test), \
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 2157997..f2563c5 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -925,6 +925,44 @@ const VMStateInfo vmstate_info_unused_buffer = {
.put = put_unused_buffer,
};
+/* vmstate_info_tmp, see VMSTATE_WITH_TMP, the idea is that we allocate
+ * a temporary buffer and the pre_load/pre_save methods in the child vmsd
+ * copy stuff from the parent into the child and do calculations to fill
+ * in fields that don't really exist in the parent but need to be in the
+ * stream.
+ */
+static int get_tmp(QEMUFile *f, void *pv, size_t size, VMStateField *field)
+{
+ int ret;
+ const VMStateDescription *vmsd = field->vmsd;
+ int version_id = field->version_id;
+ void *tmp = g_malloc(size);
+
+ /* Writes the parent field which is at the start of the tmp */
+ *(void **)tmp = pv;
+ ret = vmstate_load_state(f, vmsd, tmp, version_id);
+ g_free(tmp);
+ return ret;
+}
+
+static void put_tmp(QEMUFile *f, void *pv, size_t size, VMStateField *field,
+ QJSON *vmdesc)
+{
+ const VMStateDescription *vmsd = field->vmsd;
+ void *tmp = g_malloc(size);
+
+ /* Writes the parent field which is at the start of the tmp */
+ *(void **)tmp = pv;
+ vmstate_save_state(f, vmsd, tmp, vmdesc);
+ g_free(tmp);
+}
+
+const VMStateInfo vmstate_info_tmp = {
+ .name = "tmp",
+ .get = get_tmp,
+ .put = put_tmp,
+};
+
/* bitmaps (as defined by bitmap.h). Note that size here is the size
* of the bitmap in bits. The on-the-wire format of a bitmap is 64
* bit words with the bits in big endian order. The in-memory format
--
2.9.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [very-WIP 2/7] tests/migration: Add test for VMSTATE_WITH_TMP
2016-10-11 17:18 [Qemu-devel] [very-WIP 0/4] Migration: VMSTATE_WITH_TMP Dr. David Alan Gilbert (git)
2016-10-11 17:18 ` [Qemu-devel] [very-WIP 1/7] migration: Add VMSTATE_WITH_TMP Dr. David Alan Gilbert (git)
@ 2016-10-11 17:18 ` Dr. David Alan Gilbert (git)
2016-10-17 3:34 ` David Gibson
2016-10-11 17:18 ` [Qemu-devel] [very-WIP 3/4] slirp: VMStatify sbuf Dr. David Alan Gilbert (git)
2016-10-11 17:18 ` [Qemu-devel] [very-WIP 4/4] virtio/migration: Migrate virtio-net to VMState Dr. David Alan Gilbert (git)
3 siblings, 1 reply; 17+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2016-10-11 17:18 UTC (permalink / raw)
To: qemu-devel, amit.shah, quintela, duanj, pasic; +Cc: david
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Add a test for VMSTATE_WITH_TMP to tests/test-vmstate.c
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
tests/test-vmstate.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 93 insertions(+), 4 deletions(-)
diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
index d8da26f..203ab4a 100644
--- a/tests/test-vmstate.c
+++ b/tests/test-vmstate.c
@@ -83,7 +83,7 @@ static void save_vmstate(const VMStateDescription *desc, void *obj)
qemu_fclose(f);
}
-static void compare_vmstate(uint8_t *wire, size_t size)
+static void compare_vmstate(const uint8_t *wire, size_t size)
{
QEMUFile *f = open_test_file(false);
uint8_t result[size];
@@ -106,7 +106,7 @@ static void compare_vmstate(uint8_t *wire, size_t size)
}
static int load_vmstate_one(const VMStateDescription *desc, void *obj,
- int version, uint8_t *wire, size_t size)
+ int version, const uint8_t *wire, size_t size)
{
QEMUFile *f;
int ret;
@@ -130,7 +130,7 @@ static int load_vmstate_one(const VMStateDescription *desc, void *obj,
static int load_vmstate(const VMStateDescription *desc,
void *obj, void *obj_clone,
void (*obj_copy)(void *, void*),
- int version, uint8_t *wire, size_t size)
+ int version, const uint8_t *wire, size_t size)
{
/* We test with zero size */
obj_copy(obj_clone, obj);
@@ -282,7 +282,6 @@ static void test_simple_primitive(void)
FIELD_EQUAL(i64_1);
FIELD_EQUAL(i64_2);
}
-#undef FIELD_EQUAL
typedef struct TestStruct {
uint32_t a, b, c, e;
@@ -475,6 +474,95 @@ static void test_load_skip(void)
qemu_fclose(loading);
}
+typedef struct TmpTestStruct {
+ TestStruct *parent;
+ int64_t diff;
+} TmpTestStruct;
+
+static void tmp_child_pre_save(void *opaque)
+{
+ struct TmpTestStruct *tts = opaque;
+
+ tts->diff = tts->parent->b - tts->parent->a;
+}
+
+static int tmp_child_post_load(void *opaque, int version_id)
+{
+ struct TmpTestStruct *tts = opaque;
+
+ tts->parent->b = tts->parent->a + tts->diff;
+
+ return 0;
+}
+
+static const VMStateDescription vmstate_tmp_back_to_parent = {
+ .name = "test/tmp_child_parent",
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT64(f, TestStruct),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static const VMStateDescription vmstate_tmp_child = {
+ .name = "test/tmp_child",
+ .pre_save = tmp_child_pre_save,
+ .post_load = tmp_child_post_load,
+ .fields = (VMStateField[]) {
+ VMSTATE_INT64(diff, TmpTestStruct),
+ VMSTATE_STRUCT_POINTER(parent, TmpTestStruct,
+ vmstate_tmp_back_to_parent, TestStruct),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static const VMStateDescription vmstate_with_tmp = {
+ .name = "test/with_tmp",
+ .version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32(a, TestStruct),
+ VMSTATE_UINT64(d, TestStruct),
+ VMSTATE_WITH_TMP(TestStruct, TmpTestStruct, vmstate_tmp_child),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static void obj_tmp_copy(void *target, void *source)
+{
+ memcpy(target, source, sizeof(TestStruct));
+}
+
+static void test_tmp_struct(void)
+{
+ TestStruct obj, obj_clone;
+
+ uint8_t const wire_with_tmp[] = {
+ /* u32 a */ 0x00, 0x00, 0x00, 0x02,
+ /* u64 d */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01,
+ /* diff */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02,
+ /* u64 f */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x08,
+ QEMU_VM_EOF, /* just to ensure we won't get EOF reported prematurely */
+ };
+
+ memset(&obj, 0, sizeof(obj));
+ obj.a = 2;
+ obj.b = 4;
+ obj.d = 1;
+ obj.f = 8;
+ save_vmstate(&vmstate_with_tmp, &obj);
+
+ compare_vmstate(wire_with_tmp, sizeof(wire_with_tmp));
+
+ memset(&obj, 0, sizeof(obj));
+ fprintf(stderr, "test_tmp_struct load\n");
+ SUCCESS(load_vmstate(&vmstate_with_tmp, &obj, &obj_clone,
+ obj_tmp_copy, 1, wire_with_tmp,
+ sizeof(wire_with_tmp)));
+ g_assert_cmpint(obj.a, ==, 2); /* From top level vmsd */
+ g_assert_cmpint(obj.b, ==, 4); /* from the post_load */
+ g_assert_cmpint(obj.d, ==, 1); /* From top level vmsd */
+ g_assert_cmpint(obj.f, ==, 8); /* From the child->parent */
+}
+
int main(int argc, char **argv)
{
temp_fd = mkstemp(temp_file);
@@ -489,6 +577,7 @@ int main(int argc, char **argv)
g_test_add_func("/vmstate/field_exists/load/skip", test_load_skip);
g_test_add_func("/vmstate/field_exists/save/noskip", test_save_noskip);
g_test_add_func("/vmstate/field_exists/save/skip", test_save_skip);
+ g_test_add_func("/vmstate/tmp_struct", test_tmp_struct);
g_test_run();
close(temp_fd);
--
2.9.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [very-WIP 3/4] slirp: VMStatify sbuf
2016-10-11 17:18 [Qemu-devel] [very-WIP 0/4] Migration: VMSTATE_WITH_TMP Dr. David Alan Gilbert (git)
2016-10-11 17:18 ` [Qemu-devel] [very-WIP 1/7] migration: Add VMSTATE_WITH_TMP Dr. David Alan Gilbert (git)
2016-10-11 17:18 ` [Qemu-devel] [very-WIP 2/7] tests/migration: Add test for VMSTATE_WITH_TMP Dr. David Alan Gilbert (git)
@ 2016-10-11 17:18 ` Dr. David Alan Gilbert (git)
2016-10-17 3:36 ` David Gibson
2016-10-11 17:18 ` [Qemu-devel] [very-WIP 4/4] virtio/migration: Migrate virtio-net to VMState Dr. David Alan Gilbert (git)
3 siblings, 1 reply; 17+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2016-10-11 17:18 UTC (permalink / raw)
To: qemu-devel, amit.shah, quintela, duanj, pasic; +Cc: david
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Convert the sbuf structure to a VMStateDescription.
Note this uses the VMSTATE_WITH_TMP mechanism to calculate
and reload the offsets based on the pointers.
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
slirp/sbuf.h | 4 +-
slirp/slirp.c | 116 ++++++++++++++++++++++++++++++++++++++--------------------
2 files changed, 78 insertions(+), 42 deletions(-)
diff --git a/slirp/sbuf.h b/slirp/sbuf.h
index efcec39..a722ecb 100644
--- a/slirp/sbuf.h
+++ b/slirp/sbuf.h
@@ -12,8 +12,8 @@
#define sbspace(sb) ((sb)->sb_datalen - (sb)->sb_cc)
struct sbuf {
- u_int sb_cc; /* actual chars in buffer */
- u_int sb_datalen; /* Length of data */
+ uint32_t sb_cc; /* actual chars in buffer */
+ uint32_t sb_datalen; /* Length of data */
char *sb_wptr; /* write pointer. points to where the next
* bytes should be written in the sbuf */
char *sb_rptr; /* read pointer. points to where the next
diff --git a/slirp/slirp.c b/slirp/slirp.c
index 6276315..2f7802e 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -1185,19 +1185,72 @@ static const VMStateDescription vmstate_slirp_tcp = {
}
};
-static void slirp_sbuf_save(QEMUFile *f, struct sbuf *sbuf)
+/* The sbuf has a pair of pointers that are migrated as offsets;
+ * we calculate the offsets and restore the pointers using
+ * pre_save/post_load on a tmp structure.
+ */
+struct sbuf_tmp {
+ struct sbuf *parent;
+ uint32_t roff, woff;
+};
+
+static void sbuf_tmp_pre_save(void *opaque)
+{
+ struct sbuf_tmp *tmp = opaque;
+ tmp->woff = tmp->parent->sb_wptr - tmp->parent->sb_data;
+ tmp->roff = tmp->parent->sb_rptr - tmp->parent->sb_data;
+}
+
+static int sbuf_tmp_post_load(void *opaque, int version)
{
- uint32_t off;
-
- qemu_put_be32(f, sbuf->sb_cc);
- qemu_put_be32(f, sbuf->sb_datalen);
- off = (uint32_t)(sbuf->sb_wptr - sbuf->sb_data);
- qemu_put_sbe32(f, off);
- off = (uint32_t)(sbuf->sb_rptr - sbuf->sb_data);
- qemu_put_sbe32(f, off);
- qemu_put_buffer(f, (unsigned char*)sbuf->sb_data, sbuf->sb_datalen);
+ struct sbuf_tmp *tmp = opaque;
+ uint32_t requested_len = tmp->parent->sb_datalen;
+
+ /* Allocate the buffer space used by the field after the tmp */
+ sbreserve(tmp->parent, tmp->parent->sb_datalen);
+
+ if (tmp->parent->sb_datalen != requested_len) {
+ return -ENOMEM;
+ }
+ if (tmp->woff >= requested_len ||
+ tmp->roff >= requested_len) {
+ error_report("invalid sbuf offsets r/w=%u/%u len=%u",
+ tmp->roff, tmp->woff, requested_len);
+ return -EINVAL;
+ }
+
+ tmp->parent->sb_wptr = tmp->parent->sb_data + tmp->woff;
+ tmp->parent->sb_rptr = tmp->parent->sb_data + tmp->roff;
+
+ return 0;
}
+
+static const VMStateDescription vmstate_slirp_sbuf_tmp = {
+ .name = "slirp-sbuf-tmp",
+ .post_load = sbuf_tmp_post_load,
+ .pre_save = sbuf_tmp_pre_save,
+ .version_id = 0,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32(woff, struct sbuf_tmp),
+ VMSTATE_UINT32(roff, struct sbuf_tmp),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static const VMStateDescription vmstate_slirp_sbuf = {
+ .name = "slirp-sbuf",
+ .version_id = 0,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32(sb_cc, struct sbuf),
+ VMSTATE_UINT32(sb_datalen, struct sbuf),
+ VMSTATE_WITH_TMP(struct sbuf, struct sbuf_tmp, vmstate_slirp_sbuf_tmp),
+ VMSTATE_VBUFFER_UINT32(sb_data, struct sbuf, 0, NULL, 0, sb_datalen),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+
static void slirp_socket_save(QEMUFile *f, struct socket *so)
{
qemu_put_be32(f, so->so_urgc);
@@ -1225,8 +1278,9 @@ static void slirp_socket_save(QEMUFile *f, struct socket *so)
qemu_put_byte(f, so->so_emu);
qemu_put_byte(f, so->so_type);
qemu_put_be32(f, so->so_state);
- slirp_sbuf_save(f, &so->so_rcv);
- slirp_sbuf_save(f, &so->so_snd);
+ /* TODO: Build vmstate at this level */
+ vmstate_save_state(f, &vmstate_slirp_sbuf, &so->so_rcv, 0);
+ vmstate_save_state(f, &vmstate_slirp_sbuf, &so->so_snd, 0);
vmstate_save_state(f, &vmstate_slirp_tcp, so->so_tcpcb, 0);
}
@@ -1263,31 +1317,9 @@ static void slirp_state_save(QEMUFile *f, void *opaque)
slirp_bootp_save(f, slirp);
}
-static int slirp_sbuf_load(QEMUFile *f, struct sbuf *sbuf)
-{
- uint32_t off, sb_cc, sb_datalen;
-
- sb_cc = qemu_get_be32(f);
- sb_datalen = qemu_get_be32(f);
-
- sbreserve(sbuf, sb_datalen);
-
- if (sbuf->sb_datalen != sb_datalen)
- return -ENOMEM;
-
- sbuf->sb_cc = sb_cc;
-
- off = qemu_get_sbe32(f);
- sbuf->sb_wptr = sbuf->sb_data + off;
- off = qemu_get_sbe32(f);
- sbuf->sb_rptr = sbuf->sb_data + off;
- qemu_get_buffer(f, (unsigned char*)sbuf->sb_data, sbuf->sb_datalen);
-
- return 0;
-}
-
static int slirp_socket_load(QEMUFile *f, struct socket *so, int version_id)
{
+ int ret = 0;
if (tcp_attach(so) < 0)
return -ENOMEM;
@@ -1324,11 +1356,15 @@ static int slirp_socket_load(QEMUFile *f, struct socket *so, int version_id)
so->so_emu = qemu_get_byte(f);
so->so_type = qemu_get_byte(f);
so->so_state = qemu_get_be32(f);
- if (slirp_sbuf_load(f, &so->so_rcv) < 0)
- return -ENOMEM;
- if (slirp_sbuf_load(f, &so->so_snd) < 0)
- return -ENOMEM;
- return vmstate_load_state(f, &vmstate_slirp_tcp, so->so_tcpcb, 0);
+ /* TODO: VMState at this level */
+ ret = vmstate_load_state(f, &vmstate_slirp_sbuf, &so->so_rcv, 0);
+ if (!ret) {
+ ret = vmstate_load_state(f, &vmstate_slirp_sbuf, &so->so_snd, 0);
+ }
+ if (!ret) {
+ ret = vmstate_load_state(f, &vmstate_slirp_tcp, so->so_tcpcb, 0);
+ }
+ return ret;
}
static void slirp_bootp_load(QEMUFile *f, Slirp *slirp)
--
2.9.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [very-WIP 4/4] virtio/migration: Migrate virtio-net to VMState
2016-10-11 17:18 [Qemu-devel] [very-WIP 0/4] Migration: VMSTATE_WITH_TMP Dr. David Alan Gilbert (git)
` (2 preceding siblings ...)
2016-10-11 17:18 ` [Qemu-devel] [very-WIP 3/4] slirp: VMStatify sbuf Dr. David Alan Gilbert (git)
@ 2016-10-11 17:18 ` Dr. David Alan Gilbert (git)
3 siblings, 0 replies; 17+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2016-10-11 17:18 UTC (permalink / raw)
To: qemu-devel, amit.shah, quintela, duanj, pasic; +Cc: david
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Only lightly smoke-tested so far
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
hw/net/virtio-net.c | 317 +++++++++++++++++++++++++++--------------
include/hw/virtio/virtio-net.h | 4 +-
2 files changed, 214 insertions(+), 107 deletions(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 06bfe4b..2ac1e5f 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1514,119 +1514,23 @@ static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue)
virtio_net_set_queues(n);
}
-static void virtio_net_save_device(VirtIODevice *vdev, QEMUFile *f)
+static int virtio_net_post_load_device(void *opaque, int version_id)
{
- VirtIONet *n = VIRTIO_NET(vdev);
- int i;
-
- qemu_put_buffer(f, n->mac, ETH_ALEN);
- qemu_put_be32(f, n->vqs[0].tx_waiting);
- qemu_put_be32(f, n->mergeable_rx_bufs);
- qemu_put_be16(f, n->status);
- qemu_put_byte(f, n->promisc);
- qemu_put_byte(f, n->allmulti);
- qemu_put_be32(f, n->mac_table.in_use);
- qemu_put_buffer(f, n->mac_table.macs, n->mac_table.in_use * ETH_ALEN);
- qemu_put_buffer(f, (uint8_t *)n->vlans, MAX_VLAN >> 3);
- qemu_put_be32(f, n->has_vnet_hdr);
- qemu_put_byte(f, n->mac_table.multi_overflow);
- qemu_put_byte(f, n->mac_table.uni_overflow);
- qemu_put_byte(f, n->alluni);
- qemu_put_byte(f, n->nomulti);
- qemu_put_byte(f, n->nouni);
- qemu_put_byte(f, n->nobcast);
- qemu_put_byte(f, n->has_ufo);
- if (n->max_queues > 1) {
- qemu_put_be16(f, n->max_queues);
- qemu_put_be16(f, n->curr_queues);
- for (i = 1; i < n->curr_queues; i++) {
- qemu_put_be32(f, n->vqs[i].tx_waiting);
- }
- }
-
- if (virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)) {
- qemu_put_be64(f, n->curr_guest_offloads);
- }
-}
-
-static int virtio_net_load_device(VirtIODevice *vdev, QEMUFile *f,
- int version_id)
-{
- VirtIONet *n = VIRTIO_NET(vdev);
+ VirtIONet *n = opaque;
+ VirtIODevice *vdev = VIRTIO_DEVICE(n);
int i, link_down;
- qemu_get_buffer(f, n->mac, ETH_ALEN);
- n->vqs[0].tx_waiting = qemu_get_be32(f);
-
- virtio_net_set_mrg_rx_bufs(n, qemu_get_be32(f),
+ virtio_net_set_mrg_rx_bufs(n, n->mergeable_rx_bufs,
virtio_vdev_has_feature(vdev,
VIRTIO_F_VERSION_1));
- n->status = qemu_get_be16(f);
-
- n->promisc = qemu_get_byte(f);
- n->allmulti = qemu_get_byte(f);
-
- n->mac_table.in_use = qemu_get_be32(f);
/* MAC_TABLE_ENTRIES may be different from the saved image */
- if (n->mac_table.in_use <= MAC_TABLE_ENTRIES) {
- qemu_get_buffer(f, n->mac_table.macs,
- n->mac_table.in_use * ETH_ALEN);
- } else {
- int64_t i;
-
- /* Overflow detected - can happen if source has a larger MAC table.
- * We simply set overflow flag so there's no need to maintain the
- * table of addresses, discard them all.
- * Note: 64 bit math to avoid integer overflow.
- */
- for (i = 0; i < (int64_t)n->mac_table.in_use * ETH_ALEN; ++i) {
- qemu_get_byte(f);
- }
- n->mac_table.multi_overflow = n->mac_table.uni_overflow = 1;
+ if (n->mac_table.in_use > MAC_TABLE_ENTRIES) {
+ /* TODO: Should we or in multi_overflow/uni_overflow ? */
n->mac_table.in_use = 0;
}
-
- qemu_get_buffer(f, (uint8_t *)n->vlans, MAX_VLAN >> 3);
-
- if (qemu_get_be32(f) && !peer_has_vnet_hdr(n)) {
- error_report("virtio-net: saved image requires vnet_hdr=on");
- return -1;
- }
-
- n->mac_table.multi_overflow = qemu_get_byte(f);
- n->mac_table.uni_overflow = qemu_get_byte(f);
-
- n->alluni = qemu_get_byte(f);
- n->nomulti = qemu_get_byte(f);
- n->nouni = qemu_get_byte(f);
- n->nobcast = qemu_get_byte(f);
-
- if (qemu_get_byte(f) && !peer_has_ufo(n)) {
- error_report("virtio-net: saved image requires TUN_F_UFO support");
- return -1;
- }
- if (n->max_queues > 1) {
- if (n->max_queues != qemu_get_be16(f)) {
- error_report("virtio-net: different max_queues ");
- return -1;
- }
-
- n->curr_queues = qemu_get_be16(f);
- if (n->curr_queues > n->max_queues) {
- error_report("virtio-net: curr_queues %x > max_queues %x",
- n->curr_queues, n->max_queues);
- return -1;
- }
- for (i = 1; i < n->curr_queues; i++) {
- n->vqs[i].tx_waiting = qemu_get_be32(f);
- }
- }
-
- if (virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)) {
- n->curr_guest_offloads = qemu_get_be64(f);
- } else {
+ if (!virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)) {
n->curr_guest_offloads = virtio_net_supported_guest_offloads(n);
}
@@ -1660,6 +1564,210 @@ static int virtio_net_load_device(VirtIODevice *vdev, QEMUFile *f,
return 0;
}
+/* tx_waiting field of a VirtIONetQueue */
+static const VMStateDescription vmstate_virtio_net_queue_tx_waiting = {
+ .name = "virtio-net-queue-tx_waiting",
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32(tx_waiting, VirtIONetQueue),
+ VMSTATE_END_OF_LIST()
+ },
+};
+
+static bool max_queues_gt_1(void *opaque, int version_id)
+{
+ return VIRTIO_NET(opaque)->max_queues > 1;
+}
+
+static bool has_ctrl_guest_offloads(void *opaque, int version_id)
+{
+ return virtio_vdev_has_feature(VIRTIO_DEVICE(opaque),
+ VIRTIO_NET_F_CTRL_GUEST_OFFLOADS);
+}
+
+static bool mac_table_fits(void *opaque, int version_id)
+{
+ return VIRTIO_NET(opaque)->mac_table.in_use <= MAC_TABLE_ENTRIES;
+}
+
+static bool mac_table_doesnt_fit(void *opaque, int version_id)
+{
+ return !mac_table_fits(opaque, version_id);
+}
+
+/* This temporary type is shared by all the WITH_TMP methods
+ * although only some fields are used by each.
+ */
+struct VirtIONetMigTmp {
+ VirtIONet *parent;
+ VirtIONetQueue *vqs_1;
+ uint16_t curr_queues_1;
+ uint8_t has_ufo;
+ uint32_t has_vnet_hdr;
+};
+
+/* The 2nd and subsequent tx_waiting flags are loaded later than
+ * the 1st entry in the queues and only if there's more than one
+ * entry. We use the tmp mechanism to calculate a temporary
+ * pointer and count and also validate the count.
+ */
+
+static void virtio_net_tx_waiting_pre_save(void *opaque)
+{
+ struct VirtIONetMigTmp *tmp = opaque;
+
+ tmp->vqs_1 = tmp->parent->vqs + 1;
+ tmp->curr_queues_1 = tmp->parent->curr_queues - 1;
+ if (tmp->parent->curr_queues == 0) {
+ tmp->curr_queues_1 = 0;
+ }
+}
+
+static int virtio_net_tx_waiting_pre_load(void *opaque, int version_id)
+{
+ struct VirtIONetMigTmp *tmp = opaque;
+
+ /* Reuse the pointer setup from save */
+ virtio_net_tx_waiting_pre_save(opaque);
+
+ if (tmp->parent->curr_queues > tmp->parent->max_queues) {
+ error_report("virtio-net: curr_queues %x > max_queues %x",
+ tmp->parent->curr_queues, tmp->parent->max_queues);
+
+ return -EINVAL;
+ }
+
+ return 0; /* all good */
+}
+
+static const VMStateDescription vmstate_virtio_net_tx_waiting = {
+ .name = "virtio-net-tx_waiting",
+ .post_load = virtio_net_tx_waiting_pre_load,
+ .pre_save = virtio_net_tx_waiting_pre_save,
+ .fields = (VMStateField[]) {
+ VMSTATE_STRUCT_VARRAY_POINTER_UINT16(vqs_1, struct VirtIONetMigTmp,
+ curr_queues_1,
+ vmstate_virtio_net_queue_tx_waiting,
+ struct VirtIONetQueue),
+ VMSTATE_END_OF_LIST()
+ },
+};
+
+/* the 'has_ufo' flag is just tested; if the incoming stream has the
+ * flag set we need to check that we have it
+ */
+static int virtio_net_ufo_post_load(void *opaque, int version_id)
+{
+ struct VirtIONetMigTmp *tmp = opaque;
+
+ if (tmp->has_ufo && !peer_has_ufo(tmp->parent)) {
+ error_report("virtio-net: saved image requires TUN_F_UFO support");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static void virtio_net_ufo_pre_save(void *opaque)
+{
+ struct VirtIONetMigTmp *tmp = opaque;
+
+ tmp->has_ufo = tmp->parent->has_ufo;
+}
+
+static const VMStateDescription vmstate_virtio_net_has_ufo = {
+ .name = "virtio-net-ufo",
+ .post_load = virtio_net_ufo_post_load,
+ .pre_save = virtio_net_ufo_pre_save,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT8(has_ufo, struct VirtIONetMigTmp),
+ VMSTATE_END_OF_LIST()
+ },
+};
+
+/* the 'has_vnet_hdr' flag is just tested; if the incoming stream has the
+ * flag set we need to check that we have it
+ */
+static int virtio_net_vnet_post_load(void *opaque, int version_id)
+{
+ struct VirtIONetMigTmp *tmp = opaque;
+
+ if (tmp->has_vnet_hdr && !peer_has_vnet_hdr(tmp->parent)) {
+ error_report("virtio-net: saved image requires vnet_hdr=on");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static void virtio_net_vnet_pre_save(void *opaque)
+{
+ struct VirtIONetMigTmp *tmp = opaque;
+
+ tmp->has_vnet_hdr = tmp->parent->has_vnet_hdr;
+}
+
+static const VMStateDescription vmstate_virtio_net_has_vnet = {
+ .name = "virtio-net-vnet",
+ .post_load = virtio_net_vnet_post_load,
+ .pre_save = virtio_net_vnet_pre_save,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32(has_vnet_hdr, struct VirtIONetMigTmp),
+ VMSTATE_END_OF_LIST()
+ },
+};
+
+static const VMStateDescription vmstate_virtio_net_device = {
+ .name = "virtio-net-device",
+ .version_id = VIRTIO_NET_VM_VERSION,
+ .minimum_version_id = VIRTIO_NET_VM_VERSION,
+ .post_load = virtio_net_post_load_device,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT8_ARRAY(mac, VirtIONet, ETH_ALEN),
+ VMSTATE_STRUCT_POINTER(vqs, VirtIONet,
+ vmstate_virtio_net_queue_tx_waiting,
+ VirtIONetQueue),
+ VMSTATE_UINT32(mergeable_rx_bufs, VirtIONet),
+ VMSTATE_UINT16(status, VirtIONet),
+ VMSTATE_UINT8(promisc, VirtIONet),
+ VMSTATE_UINT8(allmulti, VirtIONet),
+ VMSTATE_UINT32(mac_table.in_use, VirtIONet),
+
+ /* Guarded pair: If it fits we load it, else we throw it away
+ * - can happen if source has a larger MAC table.; post-load
+ * sets flags in this case.
+ */
+ VMSTATE_VBUFFER_MULTIPLY(mac_table.macs, VirtIONet,
+ 0, mac_table_fits, 0, mac_table.in_use,
+ ETH_ALEN),
+ VMSTATE_UNUSED_VARRAY_UINT32(VirtIONet, mac_table_doesnt_fit, 0,
+ mac_table.in_use, ETH_ALEN),
+
+ /* Note: This is an array of uint32's that's always been saved as a
+ * buffer; hold onto your endiannesses; it's actually used as a bitmap
+ * but based on the uint.
+ */
+ VMSTATE_BUFFER_POINTER_UNSAFE(vlans, VirtIONet, 0, MAX_VLAN >> 3),
+ VMSTATE_WITH_TMP(VirtIONet, struct VirtIONetMigTmp,
+ vmstate_virtio_net_has_vnet),
+ VMSTATE_UINT8(mac_table.multi_overflow, VirtIONet),
+ VMSTATE_UINT8(mac_table.uni_overflow, VirtIONet),
+ VMSTATE_UINT8(alluni, VirtIONet),
+ VMSTATE_UINT8(nomulti, VirtIONet),
+ VMSTATE_UINT8(nouni, VirtIONet),
+ VMSTATE_UINT8(nobcast, VirtIONet),
+ VMSTATE_WITH_TMP(VirtIONet, struct VirtIONetMigTmp,
+ vmstate_virtio_net_has_ufo),
+ VMSTATE_SINGLE_TEST(max_queues, VirtIONet, max_queues_gt_1, 0,
+ vmstate_info_uint16_equal, uint16_t),
+ VMSTATE_UINT16_TEST(curr_queues, VirtIONet, max_queues_gt_1),
+ VMSTATE_WITH_TMP(VirtIONet, struct VirtIONetMigTmp,
+ vmstate_virtio_net_tx_waiting),
+ VMSTATE_UINT64_TEST(curr_guest_offloads, VirtIONet,
+ has_ctrl_guest_offloads),
+ VMSTATE_END_OF_LIST()
+ },
+};
+
static NetClientInfo net_virtio_info = {
.type = NET_CLIENT_DRIVER_NIC,
.size = sizeof(NICState),
@@ -1940,8 +2048,7 @@ static void virtio_net_class_init(ObjectClass *klass, void *data)
vdc->set_status = virtio_net_set_status;
vdc->guest_notifier_mask = virtio_net_guest_notifier_mask;
vdc->guest_notifier_pending = virtio_net_guest_notifier_pending;
- vdc->load = virtio_net_load_device;
- vdc->save = virtio_net_save_device;
+ vdc->vmsd = &vmstate_virtio_net_device;
}
static const TypeInfo virtio_net_info = {
diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index 0ced975..5a798f2 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -46,7 +46,7 @@ typedef struct VirtIONetQueue {
VirtQueue *tx_vq;
QEMUTimer *tx_timer;
QEMUBH *tx_bh;
- int tx_waiting;
+ uint32_t tx_waiting;
struct {
VirtQueueElement *elem;
} async_tx;
@@ -67,7 +67,7 @@ typedef struct VirtIONet {
size_t guest_hdr_len;
uint32_t host_features;
uint8_t has_ufo;
- int mergeable_rx_bufs;
+ uint32_t mergeable_rx_bufs;
uint8_t promisc;
uint8_t allmulti;
uint8_t alluni;
--
2.9.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [very-WIP 1/7] migration: Add VMSTATE_WITH_TMP
2016-10-11 17:18 ` [Qemu-devel] [very-WIP 1/7] migration: Add VMSTATE_WITH_TMP Dr. David Alan Gilbert (git)
@ 2016-10-17 3:31 ` David Gibson
2016-10-17 18:49 ` Jianjun Duan
0 siblings, 1 reply; 17+ messages in thread
From: David Gibson @ 2016-10-17 3:31 UTC (permalink / raw)
To: Dr. David Alan Gilbert (git)
Cc: qemu-devel, amit.shah, quintela, duanj, pasic
[-- Attachment #1: Type: text/plain, Size: 5647 bytes --]
On Tue, Oct 11, 2016 at 06:18:30PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> VMSTATE_WITH_TMP is for handling structures where some calculation
> or rearrangement of the data needs to be performed before the data
> hits the wire.
> For example, where the value on the wire is an offset from a
> non-migrated base, but the data in the structure is the actual pointer.
>
> To use it, a temporary type is created and a vmsd used on that type.
> The first element of the type must be 'parent' a pointer back to the
> type of the main structure. VMSTATE_WITH_TMP takes care of allocating
> and freeing the temporary before running the child vmsd.
>
> The post_load/pre_save on the child vmsd can copy things from the parent
> to the temporary using the parent pointer and do any other calculations
> needed; it can then use normal VMSD entries to do the actual data
> storage without having to fiddle around with qemu_get_*/qemu_put_*
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
The requirement for the parent pointer is a little clunky, but I don't
quickly see a better way, and it is compile-time verified. As noted
elsewhere I think this is a really useful approach which could allow a
bunch of internal state cleanups while preserving migration.
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> include/migration/vmstate.h | 20 ++++++++++++++++++++
> migration/vmstate.c | 38 ++++++++++++++++++++++++++++++++++++++
> 2 files changed, 58 insertions(+)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 9500da1..efb0e90 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -259,6 +259,7 @@ extern const VMStateInfo vmstate_info_cpudouble;
> extern const VMStateInfo vmstate_info_timer;
> extern const VMStateInfo vmstate_info_buffer;
> extern const VMStateInfo vmstate_info_unused_buffer;
> +extern const VMStateInfo vmstate_info_tmp;
> extern const VMStateInfo vmstate_info_bitmap;
> extern const VMStateInfo vmstate_info_qtailq;
>
> @@ -651,6 +652,25 @@ extern const VMStateInfo vmstate_info_qtailq;
> .offset = offsetof(_state, _field), \
> }
>
> +/* Allocate a temporary of type 'tmp_type', set tmp->parent to _state
> + * and execute the vmsd on the temporary. Note that we're working with
> + * the whole of _state here, not a field within it.
> + * We compile time check that:
> + * That _tmp_type contains a 'parent' member that's a pointer to the
> + * '_state' type
> + * That the pointer is right at the start of _tmp_type.
> + */
> +#define VMSTATE_WITH_TMP(_state, _tmp_type, _vmsd) { \
> + .name = "tmp", \
> + .size = sizeof(_tmp_type) + \
> + QEMU_BUILD_BUG_EXPR(offsetof(_tmp_type, parent) != 0) + \
> + type_check_pointer(_state, \
> + typeof_field(_tmp_type, parent)), \
> + .vmsd = &(_vmsd), \
> + .info = &vmstate_info_tmp, \
> + .flags = VMS_LINKED, \
> +}
> +
> #define VMSTATE_UNUSED_BUFFER(_test, _version, _size) { \
> .name = "unused", \
> .field_exists = (_test), \
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 2157997..f2563c5 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -925,6 +925,44 @@ const VMStateInfo vmstate_info_unused_buffer = {
> .put = put_unused_buffer,
> };
>
> +/* vmstate_info_tmp, see VMSTATE_WITH_TMP, the idea is that we allocate
> + * a temporary buffer and the pre_load/pre_save methods in the child vmsd
> + * copy stuff from the parent into the child and do calculations to fill
> + * in fields that don't really exist in the parent but need to be in the
> + * stream.
> + */
> +static int get_tmp(QEMUFile *f, void *pv, size_t size, VMStateField *field)
> +{
> + int ret;
> + const VMStateDescription *vmsd = field->vmsd;
> + int version_id = field->version_id;
> + void *tmp = g_malloc(size);
> +
> + /* Writes the parent field which is at the start of the tmp */
> + *(void **)tmp = pv;
> + ret = vmstate_load_state(f, vmsd, tmp, version_id);
> + g_free(tmp);
> + return ret;
> +}
> +
> +static void put_tmp(QEMUFile *f, void *pv, size_t size, VMStateField *field,
> + QJSON *vmdesc)
> +{
> + const VMStateDescription *vmsd = field->vmsd;
> + void *tmp = g_malloc(size);
> +
> + /* Writes the parent field which is at the start of the tmp */
> + *(void **)tmp = pv;
> + vmstate_save_state(f, vmsd, tmp, vmdesc);
> + g_free(tmp);
> +}
> +
> +const VMStateInfo vmstate_info_tmp = {
> + .name = "tmp",
> + .get = get_tmp,
> + .put = put_tmp,
> +};
> +
> /* bitmaps (as defined by bitmap.h). Note that size here is the size
> * of the bitmap in bits. The on-the-wire format of a bitmap is 64
> * bit words with the bits in big endian order. The in-memory format
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [very-WIP 2/7] tests/migration: Add test for VMSTATE_WITH_TMP
2016-10-11 17:18 ` [Qemu-devel] [very-WIP 2/7] tests/migration: Add test for VMSTATE_WITH_TMP Dr. David Alan Gilbert (git)
@ 2016-10-17 3:34 ` David Gibson
0 siblings, 0 replies; 17+ messages in thread
From: David Gibson @ 2016-10-17 3:34 UTC (permalink / raw)
To: Dr. David Alan Gilbert (git)
Cc: qemu-devel, amit.shah, quintela, duanj, pasic
[-- Attachment #1: Type: text/plain, Size: 5978 bytes --]
On Tue, Oct 11, 2016 at 06:18:31PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Add a test for VMSTATE_WITH_TMP to tests/test-vmstate.c
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Looks reasonable. The other obvious use case to me - which involves a
little less awkwardness with command line supplied values - is
changing an internal value to an equivalent-but-scaled representation.
e.g. changing from size (in bytes, but must be page aligned), to
number of pages.
> ---
> tests/test-vmstate.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 93 insertions(+), 4 deletions(-)
>
> diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
> index d8da26f..203ab4a 100644
> --- a/tests/test-vmstate.c
> +++ b/tests/test-vmstate.c
> @@ -83,7 +83,7 @@ static void save_vmstate(const VMStateDescription *desc, void *obj)
> qemu_fclose(f);
> }
>
> -static void compare_vmstate(uint8_t *wire, size_t size)
> +static void compare_vmstate(const uint8_t *wire, size_t size)
> {
> QEMUFile *f = open_test_file(false);
> uint8_t result[size];
> @@ -106,7 +106,7 @@ static void compare_vmstate(uint8_t *wire, size_t size)
> }
>
> static int load_vmstate_one(const VMStateDescription *desc, void *obj,
> - int version, uint8_t *wire, size_t size)
> + int version, const uint8_t *wire, size_t size)
> {
> QEMUFile *f;
> int ret;
> @@ -130,7 +130,7 @@ static int load_vmstate_one(const VMStateDescription *desc, void *obj,
> static int load_vmstate(const VMStateDescription *desc,
> void *obj, void *obj_clone,
> void (*obj_copy)(void *, void*),
> - int version, uint8_t *wire, size_t size)
> + int version, const uint8_t *wire, size_t size)
> {
> /* We test with zero size */
> obj_copy(obj_clone, obj);
> @@ -282,7 +282,6 @@ static void test_simple_primitive(void)
> FIELD_EQUAL(i64_1);
> FIELD_EQUAL(i64_2);
> }
> -#undef FIELD_EQUAL
>
> typedef struct TestStruct {
> uint32_t a, b, c, e;
> @@ -475,6 +474,95 @@ static void test_load_skip(void)
> qemu_fclose(loading);
> }
>
> +typedef struct TmpTestStruct {
> + TestStruct *parent;
> + int64_t diff;
> +} TmpTestStruct;
> +
> +static void tmp_child_pre_save(void *opaque)
> +{
> + struct TmpTestStruct *tts = opaque;
> +
> + tts->diff = tts->parent->b - tts->parent->a;
> +}
> +
> +static int tmp_child_post_load(void *opaque, int version_id)
> +{
> + struct TmpTestStruct *tts = opaque;
> +
> + tts->parent->b = tts->parent->a + tts->diff;
> +
> + return 0;
> +}
> +
> +static const VMStateDescription vmstate_tmp_back_to_parent = {
> + .name = "test/tmp_child_parent",
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT64(f, TestStruct),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static const VMStateDescription vmstate_tmp_child = {
> + .name = "test/tmp_child",
> + .pre_save = tmp_child_pre_save,
> + .post_load = tmp_child_post_load,
> + .fields = (VMStateField[]) {
> + VMSTATE_INT64(diff, TmpTestStruct),
> + VMSTATE_STRUCT_POINTER(parent, TmpTestStruct,
> + vmstate_tmp_back_to_parent, TestStruct),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static const VMStateDescription vmstate_with_tmp = {
> + .name = "test/with_tmp",
> + .version_id = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT32(a, TestStruct),
> + VMSTATE_UINT64(d, TestStruct),
> + VMSTATE_WITH_TMP(TestStruct, TmpTestStruct, vmstate_tmp_child),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static void obj_tmp_copy(void *target, void *source)
> +{
> + memcpy(target, source, sizeof(TestStruct));
> +}
> +
> +static void test_tmp_struct(void)
> +{
> + TestStruct obj, obj_clone;
> +
> + uint8_t const wire_with_tmp[] = {
> + /* u32 a */ 0x00, 0x00, 0x00, 0x02,
> + /* u64 d */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01,
> + /* diff */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02,
> + /* u64 f */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x08,
> + QEMU_VM_EOF, /* just to ensure we won't get EOF reported prematurely */
> + };
> +
> + memset(&obj, 0, sizeof(obj));
> + obj.a = 2;
> + obj.b = 4;
> + obj.d = 1;
> + obj.f = 8;
> + save_vmstate(&vmstate_with_tmp, &obj);
> +
> + compare_vmstate(wire_with_tmp, sizeof(wire_with_tmp));
> +
> + memset(&obj, 0, sizeof(obj));
> + fprintf(stderr, "test_tmp_struct load\n");
> + SUCCESS(load_vmstate(&vmstate_with_tmp, &obj, &obj_clone,
> + obj_tmp_copy, 1, wire_with_tmp,
> + sizeof(wire_with_tmp)));
> + g_assert_cmpint(obj.a, ==, 2); /* From top level vmsd */
> + g_assert_cmpint(obj.b, ==, 4); /* from the post_load */
> + g_assert_cmpint(obj.d, ==, 1); /* From top level vmsd */
> + g_assert_cmpint(obj.f, ==, 8); /* From the child->parent */
> +}
> +
> int main(int argc, char **argv)
> {
> temp_fd = mkstemp(temp_file);
> @@ -489,6 +577,7 @@ int main(int argc, char **argv)
> g_test_add_func("/vmstate/field_exists/load/skip", test_load_skip);
> g_test_add_func("/vmstate/field_exists/save/noskip", test_save_noskip);
> g_test_add_func("/vmstate/field_exists/save/skip", test_save_skip);
> + g_test_add_func("/vmstate/tmp_struct", test_tmp_struct);
> g_test_run();
>
> close(temp_fd);
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [very-WIP 3/4] slirp: VMStatify sbuf
2016-10-11 17:18 ` [Qemu-devel] [very-WIP 3/4] slirp: VMStatify sbuf Dr. David Alan Gilbert (git)
@ 2016-10-17 3:36 ` David Gibson
2016-10-17 17:54 ` Halil Pasic
0 siblings, 1 reply; 17+ messages in thread
From: David Gibson @ 2016-10-17 3:36 UTC (permalink / raw)
To: Dr. David Alan Gilbert (git)
Cc: qemu-devel, amit.shah, quintela, duanj, pasic
[-- Attachment #1: Type: text/plain, Size: 6713 bytes --]
On Tue, Oct 11, 2016 at 06:18:32PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Convert the sbuf structure to a VMStateDescription.
> Note this uses the VMSTATE_WITH_TMP mechanism to calculate
> and reload the offsets based on the pointers.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> slirp/sbuf.h | 4 +-
> slirp/slirp.c | 116 ++++++++++++++++++++++++++++++++++++++--------------------
> 2 files changed, 78 insertions(+), 42 deletions(-)
>
> diff --git a/slirp/sbuf.h b/slirp/sbuf.h
> index efcec39..a722ecb 100644
> --- a/slirp/sbuf.h
> +++ b/slirp/sbuf.h
> @@ -12,8 +12,8 @@
> #define sbspace(sb) ((sb)->sb_datalen - (sb)->sb_cc)
>
> struct sbuf {
> - u_int sb_cc; /* actual chars in buffer */
> - u_int sb_datalen; /* Length of data */
> + uint32_t sb_cc; /* actual chars in buffer */
> + uint32_t sb_datalen; /* Length of data */
> char *sb_wptr; /* write pointer. points to where the next
> * bytes should be written in the sbuf */
> char *sb_rptr; /* read pointer. points to where the next
> diff --git a/slirp/slirp.c b/slirp/slirp.c
> index 6276315..2f7802e 100644
> --- a/slirp/slirp.c
> +++ b/slirp/slirp.c
> @@ -1185,19 +1185,72 @@ static const VMStateDescription vmstate_slirp_tcp = {
> }
> };
>
> -static void slirp_sbuf_save(QEMUFile *f, struct sbuf *sbuf)
> +/* The sbuf has a pair of pointers that are migrated as offsets;
> + * we calculate the offsets and restore the pointers using
> + * pre_save/post_load on a tmp structure.
> + */
> +struct sbuf_tmp {
> + struct sbuf *parent;
> + uint32_t roff, woff;
> +};
> +
> +static void sbuf_tmp_pre_save(void *opaque)
> +{
> + struct sbuf_tmp *tmp = opaque;
> + tmp->woff = tmp->parent->sb_wptr - tmp->parent->sb_data;
> + tmp->roff = tmp->parent->sb_rptr - tmp->parent->sb_data;
> +}
> +
> +static int sbuf_tmp_post_load(void *opaque, int version)
> {
> - uint32_t off;
> -
> - qemu_put_be32(f, sbuf->sb_cc);
> - qemu_put_be32(f, sbuf->sb_datalen);
> - off = (uint32_t)(sbuf->sb_wptr - sbuf->sb_data);
> - qemu_put_sbe32(f, off);
> - off = (uint32_t)(sbuf->sb_rptr - sbuf->sb_data);
> - qemu_put_sbe32(f, off);
> - qemu_put_buffer(f, (unsigned char*)sbuf->sb_data, sbuf->sb_datalen);
> + struct sbuf_tmp *tmp = opaque;
> + uint32_t requested_len = tmp->parent->sb_datalen;
> +
> + /* Allocate the buffer space used by the field after the tmp */
> + sbreserve(tmp->parent, tmp->parent->sb_datalen);
> +
> + if (tmp->parent->sb_datalen != requested_len) {
> + return -ENOMEM;
> + }
> + if (tmp->woff >= requested_len ||
> + tmp->roff >= requested_len) {
> + error_report("invalid sbuf offsets r/w=%u/%u len=%u",
> + tmp->roff, tmp->woff, requested_len);
> + return -EINVAL;
> + }
> +
> + tmp->parent->sb_wptr = tmp->parent->sb_data + tmp->woff;
> + tmp->parent->sb_rptr = tmp->parent->sb_data + tmp->roff;
> +
> + return 0;
> }
>
> +
> +static const VMStateDescription vmstate_slirp_sbuf_tmp = {
> + .name = "slirp-sbuf-tmp",
> + .post_load = sbuf_tmp_post_load,
> + .pre_save = sbuf_tmp_pre_save,
> + .version_id = 0,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT32(woff, struct sbuf_tmp),
> + VMSTATE_UINT32(roff, struct sbuf_tmp),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static const VMStateDescription vmstate_slirp_sbuf = {
> + .name = "slirp-sbuf",
> + .version_id = 0,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT32(sb_cc, struct sbuf),
> + VMSTATE_UINT32(sb_datalen, struct sbuf),
> + VMSTATE_WITH_TMP(struct sbuf, struct sbuf_tmp, vmstate_slirp_sbuf_tmp),
> + VMSTATE_VBUFFER_UINT32(sb_data, struct sbuf, 0, NULL, 0, sb_datalen),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +
> static void slirp_socket_save(QEMUFile *f, struct socket *so)
> {
> qemu_put_be32(f, so->so_urgc);
> @@ -1225,8 +1278,9 @@ static void slirp_socket_save(QEMUFile *f, struct socket *so)
> qemu_put_byte(f, so->so_emu);
> qemu_put_byte(f, so->so_type);
> qemu_put_be32(f, so->so_state);
> - slirp_sbuf_save(f, &so->so_rcv);
> - slirp_sbuf_save(f, &so->so_snd);
> + /* TODO: Build vmstate at this level */
> + vmstate_save_state(f, &vmstate_slirp_sbuf, &so->so_rcv, 0);
> + vmstate_save_state(f, &vmstate_slirp_sbuf, &so->so_snd, 0);
> vmstate_save_state(f, &vmstate_slirp_tcp, so->so_tcpcb, 0);
> }
>
> @@ -1263,31 +1317,9 @@ static void slirp_state_save(QEMUFile *f, void *opaque)
> slirp_bootp_save(f, slirp);
> }
>
> -static int slirp_sbuf_load(QEMUFile *f, struct sbuf *sbuf)
> -{
> - uint32_t off, sb_cc, sb_datalen;
> -
> - sb_cc = qemu_get_be32(f);
> - sb_datalen = qemu_get_be32(f);
> -
> - sbreserve(sbuf, sb_datalen);
> -
> - if (sbuf->sb_datalen != sb_datalen)
> - return -ENOMEM;
> -
> - sbuf->sb_cc = sb_cc;
> -
> - off = qemu_get_sbe32(f);
> - sbuf->sb_wptr = sbuf->sb_data + off;
> - off = qemu_get_sbe32(f);
> - sbuf->sb_rptr = sbuf->sb_data + off;
> - qemu_get_buffer(f, (unsigned char*)sbuf->sb_data, sbuf->sb_datalen);
> -
> - return 0;
> -}
> -
> static int slirp_socket_load(QEMUFile *f, struct socket *so, int version_id)
> {
> + int ret = 0;
> if (tcp_attach(so) < 0)
> return -ENOMEM;
>
> @@ -1324,11 +1356,15 @@ static int slirp_socket_load(QEMUFile *f, struct socket *so, int version_id)
> so->so_emu = qemu_get_byte(f);
> so->so_type = qemu_get_byte(f);
> so->so_state = qemu_get_be32(f);
> - if (slirp_sbuf_load(f, &so->so_rcv) < 0)
> - return -ENOMEM;
> - if (slirp_sbuf_load(f, &so->so_snd) < 0)
> - return -ENOMEM;
> - return vmstate_load_state(f, &vmstate_slirp_tcp, so->so_tcpcb, 0);
> + /* TODO: VMState at this level */
> + ret = vmstate_load_state(f, &vmstate_slirp_sbuf, &so->so_rcv, 0);
> + if (!ret) {
> + ret = vmstate_load_state(f, &vmstate_slirp_sbuf, &so->so_snd, 0);
> + }
> + if (!ret) {
> + ret = vmstate_load_state(f, &vmstate_slirp_tcp, so->so_tcpcb, 0);
> + }
> + return ret;
> }
>
> static void slirp_bootp_load(QEMUFile *f, Slirp *slirp)
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [very-WIP 3/4] slirp: VMStatify sbuf
2016-10-17 3:36 ` David Gibson
@ 2016-10-17 17:54 ` Halil Pasic
2016-10-17 19:06 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 17+ messages in thread
From: Halil Pasic @ 2016-10-17 17:54 UTC (permalink / raw)
To: Dr. David Alan Gilbert (git)
Cc: David Gibson, qemu-devel, amit.shah, quintela, duanj
[-- Attachment #1: Type: text/plain, Size: 5524 bytes --]
On 10/17/2016 05:36 AM, David Gibson wrote:
> On Tue, Oct 11, 2016 at 06:18:32PM +0100, Dr. David Alan Gilbert (git) wrote:
>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>
>> Convert the sbuf structure to a VMStateDescription.
>> Note this uses the VMSTATE_WITH_TMP mechanism to calculate
>> and reload the offsets based on the pointers.
>>
>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Hi Dave!
I had a brief look, which means I intend to have a deeper
one too, but for now you will have to live with this.
>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>
>> ---
>> slirp/sbuf.h | 4 +-
>> slirp/slirp.c | 116 ++++++++++++++++++++++++++++++++++++++--------------------
>> 2 files changed, 78 insertions(+), 42 deletions(-)
>>
>> diff --git a/slirp/sbuf.h b/slirp/sbuf.h
>> index efcec39..a722ecb 100644
>> --- a/slirp/sbuf.h
>> +++ b/slirp/sbuf.h
>> @@ -12,8 +12,8 @@
>> #define sbspace(sb) ((sb)->sb_datalen - (sb)->sb_cc)
>>
>> struct sbuf {
>> - u_int sb_cc; /* actual chars in buffer */
>> - u_int sb_datalen; /* Length of data */
>> + uint32_t sb_cc; /* actual chars in buffer */
>> + uint32_t sb_datalen; /* Length of data */
>> char *sb_wptr; /* write pointer. points to where the next
>> * bytes should be written in the sbuf */
>> char *sb_rptr; /* read pointer. points to where the next
>> diff --git a/slirp/slirp.c b/slirp/slirp.c
>> index 6276315..2f7802e 100644
>> --- a/slirp/slirp.c
>> +++ b/slirp/slirp.c
>> @@ -1185,19 +1185,72 @@ static const VMStateDescription vmstate_slirp_tcp = {
>> }
>> };
>>
>> -static void slirp_sbuf_save(QEMUFile *f, struct sbuf *sbuf)
>> +/* The sbuf has a pair of pointers that are migrated as offsets;
>> + * we calculate the offsets and restore the pointers using
>> + * pre_save/post_load on a tmp structure.
>> + */
>> +struct sbuf_tmp {
>> + struct sbuf *parent;
>> + uint32_t roff, woff;
>> +};
>> +
>> +static void sbuf_tmp_pre_save(void *opaque)
>> +{
>> + struct sbuf_tmp *tmp = opaque;
>> + tmp->woff = tmp->parent->sb_wptr - tmp->parent->sb_data;
>> + tmp->roff = tmp->parent->sb_rptr - tmp->parent->sb_data;
>> +}
>> +
>> +static int sbuf_tmp_post_load(void *opaque, int version)
>> {
What makes me think about the properties of this approach,
is, that each time we use a parent pointer to read we have
a data dependency. This seems to me much more complicated
that the current massaging function approach were we say
"OK now everything below me is there, now let us transform".
Of course the proposed approach is more powerful.
>> - uint32_t off;
>> -
>> - qemu_put_be32(f, sbuf->sb_cc);
>> - qemu_put_be32(f, sbuf->sb_datalen);
>> - off = (uint32_t)(sbuf->sb_wptr - sbuf->sb_data);
>> - qemu_put_sbe32(f, off);
>> - off = (uint32_t)(sbuf->sb_rptr - sbuf->sb_data);
>> - qemu_put_sbe32(f, off);
>> - qemu_put_buffer(f, (unsigned char*)sbuf->sb_data, sbuf->sb_datalen);
>> + struct sbuf_tmp *tmp = opaque;
>> + uint32_t requested_len = tmp->parent->sb_datalen;
Ok, data parent->sb_datalen was previously loaded at #1
>> +
>> + /* Allocate the buffer space used by the field after the tmp */
>> + sbreserve(tmp->parent, tmp->parent->sb_datalen);
#2
>> +
>> + if (tmp->parent->sb_datalen != requested_len) {
>> + return -ENOMEM;
>> + }
>> + if (tmp->woff >= requested_len ||
>> + tmp->roff >= requested_len) {
>> + error_report("invalid sbuf offsets r/w=%u/%u len=%u",
>> + tmp->roff, tmp->woff, requested_len);
>> + return -EINVAL;
>> + }
>> +
>> + tmp->parent->sb_wptr = tmp->parent->sb_data + tmp->woff;
>> + tmp->parent->sb_rptr = tmp->parent->sb_data + tmp->roff;
Ok, parent->sb_data is assigned and the backing memory allocated
at #2
>> +
>> + return 0;
>> }
>>
>> +
>> +static const VMStateDescription vmstate_slirp_sbuf_tmp = {
>> + .name = "slirp-sbuf-tmp",
>> + .post_load = sbuf_tmp_post_load,
>> + .pre_save = sbuf_tmp_pre_save,
>> + .version_id = 0,
>> + .fields = (VMStateField[]) {
>> + VMSTATE_UINT32(woff, struct sbuf_tmp),
>> + VMSTATE_UINT32(roff, struct sbuf_tmp),
>> + VMSTATE_END_OF_LIST()
>> + }
>> +};
>> +
>> +static const VMStateDescription vmstate_slirp_sbuf = {
>> + .name = "slirp-sbuf",
>> + .version_id = 0,
>> + .fields = (VMStateField[]) {
>> + VMSTATE_UINT32(sb_cc, struct sbuf),
>> + VMSTATE_UINT32(sb_datalen, struct sbuf),
#1
>> + VMSTATE_WITH_TMP(struct sbuf, struct sbuf_tmp, vmstate_slirp_sbuf_tmp),
>> + VMSTATE_VBUFFER_UINT32(sb_data, struct sbuf, 0, NULL, 0, sb_datalen),
OK, memory was allocated at #2
It is a bit confusing though (for a novice like me) that we have a non ALLOC VBUFFER
whose pointer is NULL after post_load.
Now if I imagine the original stream were written in the following sequence:
vbuffer_length (sb_datalen), vbuffer_data (sb_data), offsets (sb_wptr, sb_rptr)
which seems completely valid to me then the context would not be sufficient
to compute sb_wptr and sb_rptr because the lifetime of vbuffer_data and
the tmp do not overlap.
I aware it's a trade-off between how long the temporary data lives and
how complicated the dependencies get. Or am I getting something wrong?
Cheers,
Halil
>> + VMSTATE_END_OF_LIST()
>> + }
>> +};
[..]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [very-WIP 1/7] migration: Add VMSTATE_WITH_TMP
2016-10-17 3:31 ` David Gibson
@ 2016-10-17 18:49 ` Jianjun Duan
2016-10-17 18:52 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 17+ messages in thread
From: Jianjun Duan @ 2016-10-17 18:49 UTC (permalink / raw)
To: David Gibson, Dr. David Alan Gilbert (git)
Cc: qemu-devel, amit.shah, quintela, pasic
On 10/16/2016 08:31 PM, David Gibson wrote:
> On Tue, Oct 11, 2016 at 06:18:30PM +0100, Dr. David Alan Gilbert (git) wrote:
>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>
>> VMSTATE_WITH_TMP is for handling structures where some calculation
>> or rearrangement of the data needs to be performed before the data
>> hits the wire.
>> For example, where the value on the wire is an offset from a
>> non-migrated base, but the data in the structure is the actual pointer.
>>
>> To use it, a temporary type is created and a vmsd used on that type.
>> The first element of the type must be 'parent' a pointer back to the
>> type of the main structure. VMSTATE_WITH_TMP takes care of allocating
>> and freeing the temporary before running the child vmsd.
>>
>> The post_load/pre_save on the child vmsd can copy things from the parent
>> to the temporary using the parent pointer and do any other calculations
>> needed; it can then use normal VMSD entries to do the actual data
>> storage without having to fiddle around with qemu_get_*/qemu_put_*
>>
If customized put/get can do transformation and dumping/loading data
to/from the parent structure, you don't have to go through
pre_save/post_load, and may get rid of parent pointer.
Thanks,
Jianjun
>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
> The requirement for the parent pointer is a little clunky, but I don't
> quickly see a better way, and it is compile-time verified. As noted
> elsewhere I think this is a really useful approach which could allow a
> bunch of internal state cleanups while preserving migration.
>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>
>> ---
>> include/migration/vmstate.h | 20 ++++++++++++++++++++
>> migration/vmstate.c | 38 ++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 58 insertions(+)
>>
>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>> index 9500da1..efb0e90 100644
>> --- a/include/migration/vmstate.h
>> +++ b/include/migration/vmstate.h
>> @@ -259,6 +259,7 @@ extern const VMStateInfo vmstate_info_cpudouble;
>> extern const VMStateInfo vmstate_info_timer;
>> extern const VMStateInfo vmstate_info_buffer;
>> extern const VMStateInfo vmstate_info_unused_buffer;
>> +extern const VMStateInfo vmstate_info_tmp;
>> extern const VMStateInfo vmstate_info_bitmap;
>> extern const VMStateInfo vmstate_info_qtailq;
>>
>> @@ -651,6 +652,25 @@ extern const VMStateInfo vmstate_info_qtailq;
>> .offset = offsetof(_state, _field), \
>> }
>>
>> +/* Allocate a temporary of type 'tmp_type', set tmp->parent to _state
>> + * and execute the vmsd on the temporary. Note that we're working with
>> + * the whole of _state here, not a field within it.
>> + * We compile time check that:
>> + * That _tmp_type contains a 'parent' member that's a pointer to the
>> + * '_state' type
>> + * That the pointer is right at the start of _tmp_type.
>> + */
>> +#define VMSTATE_WITH_TMP(_state, _tmp_type, _vmsd) { \
>> + .name = "tmp", \
>> + .size = sizeof(_tmp_type) + \
>> + QEMU_BUILD_BUG_EXPR(offsetof(_tmp_type, parent) != 0) + \
>> + type_check_pointer(_state, \
>> + typeof_field(_tmp_type, parent)), \
>> + .vmsd = &(_vmsd), \
>> + .info = &vmstate_info_tmp, \
>> + .flags = VMS_LINKED, \
>> +}
>> +
>> #define VMSTATE_UNUSED_BUFFER(_test, _version, _size) { \
>> .name = "unused", \
>> .field_exists = (_test), \
>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>> index 2157997..f2563c5 100644
>> --- a/migration/vmstate.c
>> +++ b/migration/vmstate.c
>> @@ -925,6 +925,44 @@ const VMStateInfo vmstate_info_unused_buffer = {
>> .put = put_unused_buffer,
>> };
>>
>> +/* vmstate_info_tmp, see VMSTATE_WITH_TMP, the idea is that we allocate
>> + * a temporary buffer and the pre_load/pre_save methods in the child vmsd
>> + * copy stuff from the parent into the child and do calculations to fill
>> + * in fields that don't really exist in the parent but need to be in the
>> + * stream.
>> + */
>> +static int get_tmp(QEMUFile *f, void *pv, size_t size, VMStateField *field)
>> +{
>> + int ret;
>> + const VMStateDescription *vmsd = field->vmsd;
>> + int version_id = field->version_id;
>> + void *tmp = g_malloc(size);
>> +
>> + /* Writes the parent field which is at the start of the tmp */
>> + *(void **)tmp = pv;
>> + ret = vmstate_load_state(f, vmsd, tmp, version_id);
>> + g_free(tmp);
>> + return ret;
>> +}
>> +
>> +static void put_tmp(QEMUFile *f, void *pv, size_t size, VMStateField *field,
>> + QJSON *vmdesc)
>> +{
>> + const VMStateDescription *vmsd = field->vmsd;
>> + void *tmp = g_malloc(size);
>> +
>> + /* Writes the parent field which is at the start of the tmp */
>> + *(void **)tmp = pv;
>> + vmstate_save_state(f, vmsd, tmp, vmdesc);
>> + g_free(tmp);
>> +}
>> +
>> +const VMStateInfo vmstate_info_tmp = {
>> + .name = "tmp",
>> + .get = get_tmp,
>> + .put = put_tmp,
>> +};
>> +
>> /* bitmaps (as defined by bitmap.h). Note that size here is the size
>> * of the bitmap in bits. The on-the-wire format of a bitmap is 64
>> * bit words with the bits in big endian order. The in-memory format
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [very-WIP 1/7] migration: Add VMSTATE_WITH_TMP
2016-10-17 18:49 ` Jianjun Duan
@ 2016-10-17 18:52 ` Dr. David Alan Gilbert
2016-10-17 19:02 ` Jianjun Duan
0 siblings, 1 reply; 17+ messages in thread
From: Dr. David Alan Gilbert @ 2016-10-17 18:52 UTC (permalink / raw)
To: Jianjun Duan; +Cc: David Gibson, qemu-devel, amit.shah, quintela, pasic
* Jianjun Duan (duanj@linux.vnet.ibm.com) wrote:
>
>
> On 10/16/2016 08:31 PM, David Gibson wrote:
> > On Tue, Oct 11, 2016 at 06:18:30PM +0100, Dr. David Alan Gilbert (git) wrote:
> >> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >>
> >> VMSTATE_WITH_TMP is for handling structures where some calculation
> >> or rearrangement of the data needs to be performed before the data
> >> hits the wire.
> >> For example, where the value on the wire is an offset from a
> >> non-migrated base, but the data in the structure is the actual pointer.
> >>
> >> To use it, a temporary type is created and a vmsd used on that type.
> >> The first element of the type must be 'parent' a pointer back to the
> >> type of the main structure. VMSTATE_WITH_TMP takes care of allocating
> >> and freeing the temporary before running the child vmsd.
> >>
> >> The post_load/pre_save on the child vmsd can copy things from the parent
> >> to the temporary using the parent pointer and do any other calculations
> >> needed; it can then use normal VMSD entries to do the actual data
> >> storage without having to fiddle around with qemu_get_*/qemu_put_*
> >>
> If customized put/get can do transformation and dumping/loading data
> to/from the parent structure, you don't have to go through
> pre_save/post_load, and may get rid of parent pointer.
Yes but I'd rather try and get rid of the customized put/get from
every device, because then people start using qemu_put/qemu_get in them all.
Dave
>
> Thanks,
> Jianjun
>
> >> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >
> > The requirement for the parent pointer is a little clunky, but I don't
> > quickly see a better way, and it is compile-time verified. As noted
> > elsewhere I think this is a really useful approach which could allow a
> > bunch of internal state cleanups while preserving migration.
> >
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> >
> >> ---
> >> include/migration/vmstate.h | 20 ++++++++++++++++++++
> >> migration/vmstate.c | 38 ++++++++++++++++++++++++++++++++++++++
> >> 2 files changed, 58 insertions(+)
> >>
> >> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> >> index 9500da1..efb0e90 100644
> >> --- a/include/migration/vmstate.h
> >> +++ b/include/migration/vmstate.h
> >> @@ -259,6 +259,7 @@ extern const VMStateInfo vmstate_info_cpudouble;
> >> extern const VMStateInfo vmstate_info_timer;
> >> extern const VMStateInfo vmstate_info_buffer;
> >> extern const VMStateInfo vmstate_info_unused_buffer;
> >> +extern const VMStateInfo vmstate_info_tmp;
> >> extern const VMStateInfo vmstate_info_bitmap;
> >> extern const VMStateInfo vmstate_info_qtailq;
> >>
> >> @@ -651,6 +652,25 @@ extern const VMStateInfo vmstate_info_qtailq;
> >> .offset = offsetof(_state, _field), \
> >> }
> >>
> >> +/* Allocate a temporary of type 'tmp_type', set tmp->parent to _state
> >> + * and execute the vmsd on the temporary. Note that we're working with
> >> + * the whole of _state here, not a field within it.
> >> + * We compile time check that:
> >> + * That _tmp_type contains a 'parent' member that's a pointer to the
> >> + * '_state' type
> >> + * That the pointer is right at the start of _tmp_type.
> >> + */
> >> +#define VMSTATE_WITH_TMP(_state, _tmp_type, _vmsd) { \
> >> + .name = "tmp", \
> >> + .size = sizeof(_tmp_type) + \
> >> + QEMU_BUILD_BUG_EXPR(offsetof(_tmp_type, parent) != 0) + \
> >> + type_check_pointer(_state, \
> >> + typeof_field(_tmp_type, parent)), \
> >> + .vmsd = &(_vmsd), \
> >> + .info = &vmstate_info_tmp, \
> >> + .flags = VMS_LINKED, \
> >> +}
> >> +
> >> #define VMSTATE_UNUSED_BUFFER(_test, _version, _size) { \
> >> .name = "unused", \
> >> .field_exists = (_test), \
> >> diff --git a/migration/vmstate.c b/migration/vmstate.c
> >> index 2157997..f2563c5 100644
> >> --- a/migration/vmstate.c
> >> +++ b/migration/vmstate.c
> >> @@ -925,6 +925,44 @@ const VMStateInfo vmstate_info_unused_buffer = {
> >> .put = put_unused_buffer,
> >> };
> >>
> >> +/* vmstate_info_tmp, see VMSTATE_WITH_TMP, the idea is that we allocate
> >> + * a temporary buffer and the pre_load/pre_save methods in the child vmsd
> >> + * copy stuff from the parent into the child and do calculations to fill
> >> + * in fields that don't really exist in the parent but need to be in the
> >> + * stream.
> >> + */
> >> +static int get_tmp(QEMUFile *f, void *pv, size_t size, VMStateField *field)
> >> +{
> >> + int ret;
> >> + const VMStateDescription *vmsd = field->vmsd;
> >> + int version_id = field->version_id;
> >> + void *tmp = g_malloc(size);
> >> +
> >> + /* Writes the parent field which is at the start of the tmp */
> >> + *(void **)tmp = pv;
> >> + ret = vmstate_load_state(f, vmsd, tmp, version_id);
> >> + g_free(tmp);
> >> + return ret;
> >> +}
> >> +
> >> +static void put_tmp(QEMUFile *f, void *pv, size_t size, VMStateField *field,
> >> + QJSON *vmdesc)
> >> +{
> >> + const VMStateDescription *vmsd = field->vmsd;
> >> + void *tmp = g_malloc(size);
> >> +
> >> + /* Writes the parent field which is at the start of the tmp */
> >> + *(void **)tmp = pv;
> >> + vmstate_save_state(f, vmsd, tmp, vmdesc);
> >> + g_free(tmp);
> >> +}
> >> +
> >> +const VMStateInfo vmstate_info_tmp = {
> >> + .name = "tmp",
> >> + .get = get_tmp,
> >> + .put = put_tmp,
> >> +};
> >> +
> >> /* bitmaps (as defined by bitmap.h). Note that size here is the size
> >> * of the bitmap in bits. The on-the-wire format of a bitmap is 64
> >> * bit words with the bits in big endian order. The in-memory format
> >
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [very-WIP 1/7] migration: Add VMSTATE_WITH_TMP
2016-10-17 18:52 ` Dr. David Alan Gilbert
@ 2016-10-17 19:02 ` Jianjun Duan
2016-10-17 19:16 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 17+ messages in thread
From: Jianjun Duan @ 2016-10-17 19:02 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: David Gibson, qemu-devel, amit.shah, quintela, pasic
On 10/17/2016 11:52 AM, Dr. David Alan Gilbert wrote:
> * Jianjun Duan (duanj@linux.vnet.ibm.com) wrote:
>>
>>
>> On 10/16/2016 08:31 PM, David Gibson wrote:
>>> On Tue, Oct 11, 2016 at 06:18:30PM +0100, Dr. David Alan Gilbert (git) wrote:
>>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>>>
>>>> VMSTATE_WITH_TMP is for handling structures where some calculation
>>>> or rearrangement of the data needs to be performed before the data
>>>> hits the wire.
>>>> For example, where the value on the wire is an offset from a
>>>> non-migrated base, but the data in the structure is the actual pointer.
>>>>
>>>> To use it, a temporary type is created and a vmsd used on that type.
>>>> The first element of the type must be 'parent' a pointer back to the
>>>> type of the main structure. VMSTATE_WITH_TMP takes care of allocating
>>>> and freeing the temporary before running the child vmsd.
>>>>
>>>> The post_load/pre_save on the child vmsd can copy things from the parent
>>>> to the temporary using the parent pointer and do any other calculations
>>>> needed; it can then use normal VMSD entries to do the actual data
>>>> storage without having to fiddle around with qemu_get_*/qemu_put_*
>>>>
>> If customized put/get can do transformation and dumping/loading data
>> to/from the parent structure, you don't have to go through
>> pre_save/post_load, and may get rid of parent pointer.
>
> Yes but I'd rather try and get rid of the customized put/get from
> every device, because then people start using qemu_put/qemu_get in them all.
>
Then customized handling need to happen in pre_save/post_load. I think
you need a way to pass TMP pointer around?
Thanks,
Jianjun
> Dave
>
>>
>> Thanks,
>> Jianjun
>>
>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>>
>>> The requirement for the parent pointer is a little clunky, but I don't
>>> quickly see a better way, and it is compile-time verified. As noted
>>> elsewhere I think this is a really useful approach which could allow a
>>> bunch of internal state cleanups while preserving migration.
>>>
>>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>>>
>>>> ---
>>>> include/migration/vmstate.h | 20 ++++++++++++++++++++
>>>> migration/vmstate.c | 38 ++++++++++++++++++++++++++++++++++++++
>>>> 2 files changed, 58 insertions(+)
>>>>
>>>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>>>> index 9500da1..efb0e90 100644
>>>> --- a/include/migration/vmstate.h
>>>> +++ b/include/migration/vmstate.h
>>>> @@ -259,6 +259,7 @@ extern const VMStateInfo vmstate_info_cpudouble;
>>>> extern const VMStateInfo vmstate_info_timer;
>>>> extern const VMStateInfo vmstate_info_buffer;
>>>> extern const VMStateInfo vmstate_info_unused_buffer;
>>>> +extern const VMStateInfo vmstate_info_tmp;
>>>> extern const VMStateInfo vmstate_info_bitmap;
>>>> extern const VMStateInfo vmstate_info_qtailq;
>>>>
>>>> @@ -651,6 +652,25 @@ extern const VMStateInfo vmstate_info_qtailq;
>>>> .offset = offsetof(_state, _field), \
>>>> }
>>>>
>>>> +/* Allocate a temporary of type 'tmp_type', set tmp->parent to _state
>>>> + * and execute the vmsd on the temporary. Note that we're working with
>>>> + * the whole of _state here, not a field within it.
>>>> + * We compile time check that:
>>>> + * That _tmp_type contains a 'parent' member that's a pointer to the
>>>> + * '_state' type
>>>> + * That the pointer is right at the start of _tmp_type.
>>>> + */
>>>> +#define VMSTATE_WITH_TMP(_state, _tmp_type, _vmsd) { \
>>>> + .name = "tmp", \
>>>> + .size = sizeof(_tmp_type) + \
>>>> + QEMU_BUILD_BUG_EXPR(offsetof(_tmp_type, parent) != 0) + \
>>>> + type_check_pointer(_state, \
>>>> + typeof_field(_tmp_type, parent)), \
>>>> + .vmsd = &(_vmsd), \
>>>> + .info = &vmstate_info_tmp, \
>>>> + .flags = VMS_LINKED, \
>>>> +}
>>>> +
>>>> #define VMSTATE_UNUSED_BUFFER(_test, _version, _size) { \
>>>> .name = "unused", \
>>>> .field_exists = (_test), \
>>>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>>>> index 2157997..f2563c5 100644
>>>> --- a/migration/vmstate.c
>>>> +++ b/migration/vmstate.c
>>>> @@ -925,6 +925,44 @@ const VMStateInfo vmstate_info_unused_buffer = {
>>>> .put = put_unused_buffer,
>>>> };
>>>>
>>>> +/* vmstate_info_tmp, see VMSTATE_WITH_TMP, the idea is that we allocate
>>>> + * a temporary buffer and the pre_load/pre_save methods in the child vmsd
>>>> + * copy stuff from the parent into the child and do calculations to fill
>>>> + * in fields that don't really exist in the parent but need to be in the
>>>> + * stream.
>>>> + */
>>>> +static int get_tmp(QEMUFile *f, void *pv, size_t size, VMStateField *field)
>>>> +{
>>>> + int ret;
>>>> + const VMStateDescription *vmsd = field->vmsd;
>>>> + int version_id = field->version_id;
>>>> + void *tmp = g_malloc(size);
>>>> +
>>>> + /* Writes the parent field which is at the start of the tmp */
>>>> + *(void **)tmp = pv;
>>>> + ret = vmstate_load_state(f, vmsd, tmp, version_id);
>>>> + g_free(tmp);
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static void put_tmp(QEMUFile *f, void *pv, size_t size, VMStateField *field,
>>>> + QJSON *vmdesc)
>>>> +{
>>>> + const VMStateDescription *vmsd = field->vmsd;
>>>> + void *tmp = g_malloc(size);
>>>> +
>>>> + /* Writes the parent field which is at the start of the tmp */
>>>> + *(void **)tmp = pv;
>>>> + vmstate_save_state(f, vmsd, tmp, vmdesc);
>>>> + g_free(tmp);
>>>> +}
>>>> +
>>>> +const VMStateInfo vmstate_info_tmp = {
>>>> + .name = "tmp",
>>>> + .get = get_tmp,
>>>> + .put = put_tmp,
>>>> +};
>>>> +
>>>> /* bitmaps (as defined by bitmap.h). Note that size here is the size
>>>> * of the bitmap in bits. The on-the-wire format of a bitmap is 64
>>>> * bit words with the bits in big endian order. The in-memory format
>>>
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [very-WIP 3/4] slirp: VMStatify sbuf
2016-10-17 17:54 ` Halil Pasic
@ 2016-10-17 19:06 ` Dr. David Alan Gilbert
2016-10-18 10:40 ` Halil Pasic
0 siblings, 1 reply; 17+ messages in thread
From: Dr. David Alan Gilbert @ 2016-10-17 19:06 UTC (permalink / raw)
To: Halil Pasic; +Cc: David Gibson, qemu-devel, amit.shah, quintela, duanj
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>
>
> On 10/17/2016 05:36 AM, David Gibson wrote:
> > On Tue, Oct 11, 2016 at 06:18:32PM +0100, Dr. David Alan Gilbert (git) wrote:
> >> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >>
> >> Convert the sbuf structure to a VMStateDescription.
> >> Note this uses the VMSTATE_WITH_TMP mechanism to calculate
> >> and reload the offsets based on the pointers.
> >>
> >> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
> Hi Dave!
>
> I had a brief look, which means I intend to have a deeper
> one too, but for now you will have to live with this.
Thanks.
> >
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> >
> >> ---
> >> slirp/sbuf.h | 4 +-
> >> slirp/slirp.c | 116 ++++++++++++++++++++++++++++++++++++++--------------------
> >> 2 files changed, 78 insertions(+), 42 deletions(-)
> >>
> >> diff --git a/slirp/sbuf.h b/slirp/sbuf.h
> >> index efcec39..a722ecb 100644
> >> --- a/slirp/sbuf.h
> >> +++ b/slirp/sbuf.h
> >> @@ -12,8 +12,8 @@
> >> #define sbspace(sb) ((sb)->sb_datalen - (sb)->sb_cc)
> >>
> >> struct sbuf {
> >> - u_int sb_cc; /* actual chars in buffer */
> >> - u_int sb_datalen; /* Length of data */
> >> + uint32_t sb_cc; /* actual chars in buffer */
> >> + uint32_t sb_datalen; /* Length of data */
> >> char *sb_wptr; /* write pointer. points to where the next
> >> * bytes should be written in the sbuf */
> >> char *sb_rptr; /* read pointer. points to where the next
> >> diff --git a/slirp/slirp.c b/slirp/slirp.c
> >> index 6276315..2f7802e 100644
> >> --- a/slirp/slirp.c
> >> +++ b/slirp/slirp.c
> >> @@ -1185,19 +1185,72 @@ static const VMStateDescription vmstate_slirp_tcp = {
> >> }
> >> };
> >>
> >> -static void slirp_sbuf_save(QEMUFile *f, struct sbuf *sbuf)
> >> +/* The sbuf has a pair of pointers that are migrated as offsets;
> >> + * we calculate the offsets and restore the pointers using
> >> + * pre_save/post_load on a tmp structure.
> >> + */
> >> +struct sbuf_tmp {
> >> + struct sbuf *parent;
> >> + uint32_t roff, woff;
> >> +};
> >> +
> >> +static void sbuf_tmp_pre_save(void *opaque)
> >> +{
> >> + struct sbuf_tmp *tmp = opaque;
> >> + tmp->woff = tmp->parent->sb_wptr - tmp->parent->sb_data;
> >> + tmp->roff = tmp->parent->sb_rptr - tmp->parent->sb_data;
> >> +}
> >> +
> >> +static int sbuf_tmp_post_load(void *opaque, int version)
> >> {
>
> What makes me think about the properties of this approach,
> is, that each time we use a parent pointer to read we have
> a data dependency. This seems to me much more complicated
> that the current massaging function approach were we say
> "OK now everything below me is there, now let us transform".
> Of course the proposed approach is more powerful.
Yes it is, but we have to apply a transform to the data
so that means we somehow need to get to both a temporary
piece of storage and the parent data.
> >> - uint32_t off;
> >> -
> >> - qemu_put_be32(f, sbuf->sb_cc);
> >> - qemu_put_be32(f, sbuf->sb_datalen);
> >> - off = (uint32_t)(sbuf->sb_wptr - sbuf->sb_data);
> >> - qemu_put_sbe32(f, off);
> >> - off = (uint32_t)(sbuf->sb_rptr - sbuf->sb_data);
> >> - qemu_put_sbe32(f, off);
> >> - qemu_put_buffer(f, (unsigned char*)sbuf->sb_data, sbuf->sb_datalen);
> >> + struct sbuf_tmp *tmp = opaque;
> >> + uint32_t requested_len = tmp->parent->sb_datalen;
>
> Ok, data parent->sb_datalen was previously loaded at #1
>
> >> +
> >> + /* Allocate the buffer space used by the field after the tmp */
> >> + sbreserve(tmp->parent, tmp->parent->sb_datalen);
> #2
> >> +
> >> + if (tmp->parent->sb_datalen != requested_len) {
> >> + return -ENOMEM;
> >> + }
> >> + if (tmp->woff >= requested_len ||
> >> + tmp->roff >= requested_len) {
> >> + error_report("invalid sbuf offsets r/w=%u/%u len=%u",
> >> + tmp->roff, tmp->woff, requested_len);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + tmp->parent->sb_wptr = tmp->parent->sb_data + tmp->woff;
> >> + tmp->parent->sb_rptr = tmp->parent->sb_data + tmp->roff;
>
> Ok, parent->sb_data is assigned and the backing memory allocated
> at #2
>
> >> +
> >> + return 0;
> >> }
> >>
> >> +
> >> +static const VMStateDescription vmstate_slirp_sbuf_tmp = {
> >> + .name = "slirp-sbuf-tmp",
> >> + .post_load = sbuf_tmp_post_load,
> >> + .pre_save = sbuf_tmp_pre_save,
> >> + .version_id = 0,
> >> + .fields = (VMStateField[]) {
> >> + VMSTATE_UINT32(woff, struct sbuf_tmp),
> >> + VMSTATE_UINT32(roff, struct sbuf_tmp),
> >> + VMSTATE_END_OF_LIST()
> >> + }
> >> +};
> >> +
> >> +static const VMStateDescription vmstate_slirp_sbuf = {
> >> + .name = "slirp-sbuf",
> >> + .version_id = 0,
> >> + .fields = (VMStateField[]) {
> >> + VMSTATE_UINT32(sb_cc, struct sbuf),
> >> + VMSTATE_UINT32(sb_datalen, struct sbuf),
>
> #1
>
> >> + VMSTATE_WITH_TMP(struct sbuf, struct sbuf_tmp, vmstate_slirp_sbuf_tmp),
> >> + VMSTATE_VBUFFER_UINT32(sb_data, struct sbuf, 0, NULL, 0, sb_datalen),
>
> OK, memory was allocated at #2
> It is a bit confusing though (for a novice like me) that we have a non ALLOC VBUFFER
> whose pointer is NULL after post_load.
I don't think this pointer can be NULL; the sbreserve at #2 causes it to be
allocated.
But yes, it's a shame I can't use VMS_ALLOC here, but the sbreserve is not
a trivial allocation function.
> Now if I imagine the original stream were written in the following sequence:
> vbuffer_length (sb_datalen), vbuffer_data (sb_data), offsets (sb_wptr, sb_rptr)
> which seems completely valid to me then the context would not be sufficient
> to compute sb_wptr and sb_rptr because the lifetime of vbuffer_data and
> the tmp do not overlap.
If that was the case you could still do it pretty easily.
You'd have to add the sb_datalen and sb_data fields to the temporary
and then move the VMSTATE_VBUFFER_UINT32 into the tmp so it would operate
on the copied fields.
> I aware it's a trade-off between how long the temporary data lives and
> how complicated the dependencies get. Or am I getting something wrong?
No, I think that's right. The other option I thought of was a macro
to allocate a temporary and then another to free it and then someway
to tell macros in between that they should operate on the temporary
rather than the main pointer; but then you'd have to be VERY careful
to not allow yourself to access a temporary that's been freed.
This structure means you can't make that mistake.
Dave
>
> Cheers,
> Halil
>
> >> + VMSTATE_END_OF_LIST()
> >> + }
> >> +};
> [..]
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [very-WIP 1/7] migration: Add VMSTATE_WITH_TMP
2016-10-17 19:02 ` Jianjun Duan
@ 2016-10-17 19:16 ` Dr. David Alan Gilbert
2016-10-17 19:30 ` Jianjun Duan
0 siblings, 1 reply; 17+ messages in thread
From: Dr. David Alan Gilbert @ 2016-10-17 19:16 UTC (permalink / raw)
To: Jianjun Duan; +Cc: David Gibson, qemu-devel, amit.shah, quintela, pasic
* Jianjun Duan (duanj@linux.vnet.ibm.com) wrote:
>
>
> On 10/17/2016 11:52 AM, Dr. David Alan Gilbert wrote:
> > * Jianjun Duan (duanj@linux.vnet.ibm.com) wrote:
> >>
> >>
> >> On 10/16/2016 08:31 PM, David Gibson wrote:
> >>> On Tue, Oct 11, 2016 at 06:18:30PM +0100, Dr. David Alan Gilbert (git) wrote:
> >>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >>>>
> >>>> VMSTATE_WITH_TMP is for handling structures where some calculation
> >>>> or rearrangement of the data needs to be performed before the data
> >>>> hits the wire.
> >>>> For example, where the value on the wire is an offset from a
> >>>> non-migrated base, but the data in the structure is the actual pointer.
> >>>>
> >>>> To use it, a temporary type is created and a vmsd used on that type.
> >>>> The first element of the type must be 'parent' a pointer back to the
> >>>> type of the main structure. VMSTATE_WITH_TMP takes care of allocating
> >>>> and freeing the temporary before running the child vmsd.
> >>>>
> >>>> The post_load/pre_save on the child vmsd can copy things from the parent
> >>>> to the temporary using the parent pointer and do any other calculations
> >>>> needed; it can then use normal VMSD entries to do the actual data
> >>>> storage without having to fiddle around with qemu_get_*/qemu_put_*
> >>>>
> >> If customized put/get can do transformation and dumping/loading data
> >> to/from the parent structure, you don't have to go through
> >> pre_save/post_load, and may get rid of parent pointer.
> >
> > Yes but I'd rather try and get rid of the customized put/get from
> > every device, because then people start using qemu_put/qemu_get in them all.
> >
> Then customized handling need to happen in pre_save/post_load. I think
> you need a way to pass TMP pointer around?
But then why is that better than having the parent pointer?
Dave
>
> Thanks,
> Jianjun
> > Dave
> >
> >>
> >> Thanks,
> >> Jianjun
> >>
> >>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >>>
> >>> The requirement for the parent pointer is a little clunky, but I don't
> >>> quickly see a better way, and it is compile-time verified. As noted
> >>> elsewhere I think this is a really useful approach which could allow a
> >>> bunch of internal state cleanups while preserving migration.
> >>>
> >>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> >>>
> >>>> ---
> >>>> include/migration/vmstate.h | 20 ++++++++++++++++++++
> >>>> migration/vmstate.c | 38 ++++++++++++++++++++++++++++++++++++++
> >>>> 2 files changed, 58 insertions(+)
> >>>>
> >>>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> >>>> index 9500da1..efb0e90 100644
> >>>> --- a/include/migration/vmstate.h
> >>>> +++ b/include/migration/vmstate.h
> >>>> @@ -259,6 +259,7 @@ extern const VMStateInfo vmstate_info_cpudouble;
> >>>> extern const VMStateInfo vmstate_info_timer;
> >>>> extern const VMStateInfo vmstate_info_buffer;
> >>>> extern const VMStateInfo vmstate_info_unused_buffer;
> >>>> +extern const VMStateInfo vmstate_info_tmp;
> >>>> extern const VMStateInfo vmstate_info_bitmap;
> >>>> extern const VMStateInfo vmstate_info_qtailq;
> >>>>
> >>>> @@ -651,6 +652,25 @@ extern const VMStateInfo vmstate_info_qtailq;
> >>>> .offset = offsetof(_state, _field), \
> >>>> }
> >>>>
> >>>> +/* Allocate a temporary of type 'tmp_type', set tmp->parent to _state
> >>>> + * and execute the vmsd on the temporary. Note that we're working with
> >>>> + * the whole of _state here, not a field within it.
> >>>> + * We compile time check that:
> >>>> + * That _tmp_type contains a 'parent' member that's a pointer to the
> >>>> + * '_state' type
> >>>> + * That the pointer is right at the start of _tmp_type.
> >>>> + */
> >>>> +#define VMSTATE_WITH_TMP(_state, _tmp_type, _vmsd) { \
> >>>> + .name = "tmp", \
> >>>> + .size = sizeof(_tmp_type) + \
> >>>> + QEMU_BUILD_BUG_EXPR(offsetof(_tmp_type, parent) != 0) + \
> >>>> + type_check_pointer(_state, \
> >>>> + typeof_field(_tmp_type, parent)), \
> >>>> + .vmsd = &(_vmsd), \
> >>>> + .info = &vmstate_info_tmp, \
> >>>> + .flags = VMS_LINKED, \
> >>>> +}
> >>>> +
> >>>> #define VMSTATE_UNUSED_BUFFER(_test, _version, _size) { \
> >>>> .name = "unused", \
> >>>> .field_exists = (_test), \
> >>>> diff --git a/migration/vmstate.c b/migration/vmstate.c
> >>>> index 2157997..f2563c5 100644
> >>>> --- a/migration/vmstate.c
> >>>> +++ b/migration/vmstate.c
> >>>> @@ -925,6 +925,44 @@ const VMStateInfo vmstate_info_unused_buffer = {
> >>>> .put = put_unused_buffer,
> >>>> };
> >>>>
> >>>> +/* vmstate_info_tmp, see VMSTATE_WITH_TMP, the idea is that we allocate
> >>>> + * a temporary buffer and the pre_load/pre_save methods in the child vmsd
> >>>> + * copy stuff from the parent into the child and do calculations to fill
> >>>> + * in fields that don't really exist in the parent but need to be in the
> >>>> + * stream.
> >>>> + */
> >>>> +static int get_tmp(QEMUFile *f, void *pv, size_t size, VMStateField *field)
> >>>> +{
> >>>> + int ret;
> >>>> + const VMStateDescription *vmsd = field->vmsd;
> >>>> + int version_id = field->version_id;
> >>>> + void *tmp = g_malloc(size);
> >>>> +
> >>>> + /* Writes the parent field which is at the start of the tmp */
> >>>> + *(void **)tmp = pv;
> >>>> + ret = vmstate_load_state(f, vmsd, tmp, version_id);
> >>>> + g_free(tmp);
> >>>> + return ret;
> >>>> +}
> >>>> +
> >>>> +static void put_tmp(QEMUFile *f, void *pv, size_t size, VMStateField *field,
> >>>> + QJSON *vmdesc)
> >>>> +{
> >>>> + const VMStateDescription *vmsd = field->vmsd;
> >>>> + void *tmp = g_malloc(size);
> >>>> +
> >>>> + /* Writes the parent field which is at the start of the tmp */
> >>>> + *(void **)tmp = pv;
> >>>> + vmstate_save_state(f, vmsd, tmp, vmdesc);
> >>>> + g_free(tmp);
> >>>> +}
> >>>> +
> >>>> +const VMStateInfo vmstate_info_tmp = {
> >>>> + .name = "tmp",
> >>>> + .get = get_tmp,
> >>>> + .put = put_tmp,
> >>>> +};
> >>>> +
> >>>> /* bitmaps (as defined by bitmap.h). Note that size here is the size
> >>>> * of the bitmap in bits. The on-the-wire format of a bitmap is 64
> >>>> * bit words with the bits in big endian order. The in-memory format
> >>>
> >>
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [very-WIP 1/7] migration: Add VMSTATE_WITH_TMP
2016-10-17 19:16 ` Dr. David Alan Gilbert
@ 2016-10-17 19:30 ` Jianjun Duan
2016-10-18 8:06 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 17+ messages in thread
From: Jianjun Duan @ 2016-10-17 19:30 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: David Gibson, qemu-devel, amit.shah, quintela, pasic
On 10/17/2016 12:16 PM, Dr. David Alan Gilbert wrote:
> * Jianjun Duan (duanj@linux.vnet.ibm.com) wrote:
>>
>>
>> On 10/17/2016 11:52 AM, Dr. David Alan Gilbert wrote:
>>> * Jianjun Duan (duanj@linux.vnet.ibm.com) wrote:
>>>>
>>>>
>>>> On 10/16/2016 08:31 PM, David Gibson wrote:
>>>>> On Tue, Oct 11, 2016 at 06:18:30PM +0100, Dr. David Alan Gilbert (git) wrote:
>>>>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>>>>>
>>>>>> VMSTATE_WITH_TMP is for handling structures where some calculation
>>>>>> or rearrangement of the data needs to be performed before the data
>>>>>> hits the wire.
>>>>>> For example, where the value on the wire is an offset from a
>>>>>> non-migrated base, but the data in the structure is the actual pointer.
>>>>>>
>>>>>> To use it, a temporary type is created and a vmsd used on that type.
>>>>>> The first element of the type must be 'parent' a pointer back to the
>>>>>> type of the main structure. VMSTATE_WITH_TMP takes care of allocating
>>>>>> and freeing the temporary before running the child vmsd.
>>>>>>
>>>>>> The post_load/pre_save on the child vmsd can copy things from the parent
>>>>>> to the temporary using the parent pointer and do any other calculations
>>>>>> needed; it can then use normal VMSD entries to do the actual data
>>>>>> storage without having to fiddle around with qemu_get_*/qemu_put_*
>>>>>>
>>>> If customized put/get can do transformation and dumping/loading data
>>>> to/from the parent structure, you don't have to go through
>>>> pre_save/post_load, and may get rid of parent pointer.
>>>
>>> Yes but I'd rather try and get rid of the customized put/get from
>>> every device, because then people start using qemu_put/qemu_get in them all.
>>>
>> Then customized handling need to happen in pre_save/post_load. I think
>> you need a way to pass TMP pointer around?
>
> But then why is that better than having the parent pointer?
>
IIUC, from the put_tmp, I didn't see how tmp is filled with data. I
suppose it is to be filled by pre_save. So tmp pointer needs to find a
way from inside pre_save to put_tmp. How does it happen?
Thanks,
Jianjun
> Dave
>
>>
>> Thanks,
>> Jianjun
>>> Dave
>>>
>>>>
>>>> Thanks,
>>>> Jianjun
>>>>
>>>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>>>>
>>>>> The requirement for the parent pointer is a little clunky, but I don't
>>>>> quickly see a better way, and it is compile-time verified. As noted
>>>>> elsewhere I think this is a really useful approach which could allow a
>>>>> bunch of internal state cleanups while preserving migration.
>>>>>
>>>>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>>>>>
>>>>>> ---
>>>>>> include/migration/vmstate.h | 20 ++++++++++++++++++++
>>>>>> migration/vmstate.c | 38 ++++++++++++++++++++++++++++++++++++++
>>>>>> 2 files changed, 58 insertions(+)
>>>>>>
>>>>>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>>>>>> index 9500da1..efb0e90 100644
>>>>>> --- a/include/migration/vmstate.h
>>>>>> +++ b/include/migration/vmstate.h
>>>>>> @@ -259,6 +259,7 @@ extern const VMStateInfo vmstate_info_cpudouble;
>>>>>> extern const VMStateInfo vmstate_info_timer;
>>>>>> extern const VMStateInfo vmstate_info_buffer;
>>>>>> extern const VMStateInfo vmstate_info_unused_buffer;
>>>>>> +extern const VMStateInfo vmstate_info_tmp;
>>>>>> extern const VMStateInfo vmstate_info_bitmap;
>>>>>> extern const VMStateInfo vmstate_info_qtailq;
>>>>>>
>>>>>> @@ -651,6 +652,25 @@ extern const VMStateInfo vmstate_info_qtailq;
>>>>>> .offset = offsetof(_state, _field), \
>>>>>> }
>>>>>>
>>>>>> +/* Allocate a temporary of type 'tmp_type', set tmp->parent to _state
>>>>>> + * and execute the vmsd on the temporary. Note that we're working with
>>>>>> + * the whole of _state here, not a field within it.
>>>>>> + * We compile time check that:
>>>>>> + * That _tmp_type contains a 'parent' member that's a pointer to the
>>>>>> + * '_state' type
>>>>>> + * That the pointer is right at the start of _tmp_type.
>>>>>> + */
>>>>>> +#define VMSTATE_WITH_TMP(_state, _tmp_type, _vmsd) { \
>>>>>> + .name = "tmp", \
>>>>>> + .size = sizeof(_tmp_type) + \
>>>>>> + QEMU_BUILD_BUG_EXPR(offsetof(_tmp_type, parent) != 0) + \
>>>>>> + type_check_pointer(_state, \
>>>>>> + typeof_field(_tmp_type, parent)), \
>>>>>> + .vmsd = &(_vmsd), \
>>>>>> + .info = &vmstate_info_tmp, \
>>>>>> + .flags = VMS_LINKED, \
>>>>>> +}
>>>>>> +
>>>>>> #define VMSTATE_UNUSED_BUFFER(_test, _version, _size) { \
>>>>>> .name = "unused", \
>>>>>> .field_exists = (_test), \
>>>>>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>>>>>> index 2157997..f2563c5 100644
>>>>>> --- a/migration/vmstate.c
>>>>>> +++ b/migration/vmstate.c
>>>>>> @@ -925,6 +925,44 @@ const VMStateInfo vmstate_info_unused_buffer = {
>>>>>> .put = put_unused_buffer,
>>>>>> };
>>>>>>
>>>>>> +/* vmstate_info_tmp, see VMSTATE_WITH_TMP, the idea is that we allocate
>>>>>> + * a temporary buffer and the pre_load/pre_save methods in the child vmsd
>>>>>> + * copy stuff from the parent into the child and do calculations to fill
>>>>>> + * in fields that don't really exist in the parent but need to be in the
>>>>>> + * stream.
>>>>>> + */
>>>>>> +static int get_tmp(QEMUFile *f, void *pv, size_t size, VMStateField *field)
>>>>>> +{
>>>>>> + int ret;
>>>>>> + const VMStateDescription *vmsd = field->vmsd;
>>>>>> + int version_id = field->version_id;
>>>>>> + void *tmp = g_malloc(size);
>>>>>> +
>>>>>> + /* Writes the parent field which is at the start of the tmp */
>>>>>> + *(void **)tmp = pv;
>>>>>> + ret = vmstate_load_state(f, vmsd, tmp, version_id);
>>>>>> + g_free(tmp);
>>>>>> + return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static void put_tmp(QEMUFile *f, void *pv, size_t size, VMStateField *field,
>>>>>> + QJSON *vmdesc)
>>>>>> +{
>>>>>> + const VMStateDescription *vmsd = field->vmsd;
>>>>>> + void *tmp = g_malloc(size);
>>>>>> +
>>>>>> + /* Writes the parent field which is at the start of the tmp */
>>>>>> + *(void **)tmp = pv;
>>>>>> + vmstate_save_state(f, vmsd, tmp, vmdesc);
>>>>>> + g_free(tmp);
>>>>>> +}
>>>>>> +
>>>>>> +const VMStateInfo vmstate_info_tmp = {
>>>>>> + .name = "tmp",
>>>>>> + .get = get_tmp,
>>>>>> + .put = put_tmp,
>>>>>> +};
>>>>>> +
>>>>>> /* bitmaps (as defined by bitmap.h). Note that size here is the size
>>>>>> * of the bitmap in bits. The on-the-wire format of a bitmap is 64
>>>>>> * bit words with the bits in big endian order. The in-memory format
>>>>>
>>>>
>>> --
>>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>>>
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [very-WIP 1/7] migration: Add VMSTATE_WITH_TMP
2016-10-17 19:30 ` Jianjun Duan
@ 2016-10-18 8:06 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 17+ messages in thread
From: Dr. David Alan Gilbert @ 2016-10-18 8:06 UTC (permalink / raw)
To: Jianjun Duan; +Cc: David Gibson, qemu-devel, amit.shah, quintela, pasic
* Jianjun Duan (duanj@linux.vnet.ibm.com) wrote:
>
>
> On 10/17/2016 12:16 PM, Dr. David Alan Gilbert wrote:
> > * Jianjun Duan (duanj@linux.vnet.ibm.com) wrote:
> >>
> >>
> >> On 10/17/2016 11:52 AM, Dr. David Alan Gilbert wrote:
> >>> * Jianjun Duan (duanj@linux.vnet.ibm.com) wrote:
> >>>>
> >>>>
> >>>> On 10/16/2016 08:31 PM, David Gibson wrote:
> >>>>> On Tue, Oct 11, 2016 at 06:18:30PM +0100, Dr. David Alan Gilbert (git) wrote:
> >>>>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >>>>>>
> >>>>>> VMSTATE_WITH_TMP is for handling structures where some calculation
> >>>>>> or rearrangement of the data needs to be performed before the data
> >>>>>> hits the wire.
> >>>>>> For example, where the value on the wire is an offset from a
> >>>>>> non-migrated base, but the data in the structure is the actual pointer.
> >>>>>>
> >>>>>> To use it, a temporary type is created and a vmsd used on that type.
> >>>>>> The first element of the type must be 'parent' a pointer back to the
> >>>>>> type of the main structure. VMSTATE_WITH_TMP takes care of allocating
> >>>>>> and freeing the temporary before running the child vmsd.
> >>>>>>
> >>>>>> The post_load/pre_save on the child vmsd can copy things from the parent
> >>>>>> to the temporary using the parent pointer and do any other calculations
> >>>>>> needed; it can then use normal VMSD entries to do the actual data
> >>>>>> storage without having to fiddle around with qemu_get_*/qemu_put_*
> >>>>>>
> >>>> If customized put/get can do transformation and dumping/loading data
> >>>> to/from the parent structure, you don't have to go through
> >>>> pre_save/post_load, and may get rid of parent pointer.
> >>>
> >>> Yes but I'd rather try and get rid of the customized put/get from
> >>> every device, because then people start using qemu_put/qemu_get in them all.
> >>>
> >> Then customized handling need to happen in pre_save/post_load. I think
> >> you need a way to pass TMP pointer around?
> >
> > But then why is that better than having the parent pointer?
> >
> IIUC, from the put_tmp, I didn't see how tmp is filled with data. I
> suppose it is to be filled by pre_save. So tmp pointer needs to find a
> way from inside pre_save to put_tmp. How does it happen?
The tmp.parent pointer is filled by the '*(void **)tmp = pv;' in the put_tmp and
get_tmp.
Only after that is the child vmsd run and it's passed the tmp pointer; it's the
child's pre_save/pre_load that has the chance to do whatever it likes to fill the
tmp.
Dave
> Thanks,
> Jianjun
>
>
> > Dave
> >
> >>
> >> Thanks,
> >> Jianjun
> >>> Dave
> >>>
> >>>>
> >>>> Thanks,
> >>>> Jianjun
> >>>>
> >>>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >>>>>
> >>>>> The requirement for the parent pointer is a little clunky, but I don't
> >>>>> quickly see a better way, and it is compile-time verified. As noted
> >>>>> elsewhere I think this is a really useful approach which could allow a
> >>>>> bunch of internal state cleanups while preserving migration.
> >>>>>
> >>>>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> >>>>>
> >>>>>> ---
> >>>>>> include/migration/vmstate.h | 20 ++++++++++++++++++++
> >>>>>> migration/vmstate.c | 38 ++++++++++++++++++++++++++++++++++++++
> >>>>>> 2 files changed, 58 insertions(+)
> >>>>>>
> >>>>>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> >>>>>> index 9500da1..efb0e90 100644
> >>>>>> --- a/include/migration/vmstate.h
> >>>>>> +++ b/include/migration/vmstate.h
> >>>>>> @@ -259,6 +259,7 @@ extern const VMStateInfo vmstate_info_cpudouble;
> >>>>>> extern const VMStateInfo vmstate_info_timer;
> >>>>>> extern const VMStateInfo vmstate_info_buffer;
> >>>>>> extern const VMStateInfo vmstate_info_unused_buffer;
> >>>>>> +extern const VMStateInfo vmstate_info_tmp;
> >>>>>> extern const VMStateInfo vmstate_info_bitmap;
> >>>>>> extern const VMStateInfo vmstate_info_qtailq;
> >>>>>>
> >>>>>> @@ -651,6 +652,25 @@ extern const VMStateInfo vmstate_info_qtailq;
> >>>>>> .offset = offsetof(_state, _field), \
> >>>>>> }
> >>>>>>
> >>>>>> +/* Allocate a temporary of type 'tmp_type', set tmp->parent to _state
> >>>>>> + * and execute the vmsd on the temporary. Note that we're working with
> >>>>>> + * the whole of _state here, not a field within it.
> >>>>>> + * We compile time check that:
> >>>>>> + * That _tmp_type contains a 'parent' member that's a pointer to the
> >>>>>> + * '_state' type
> >>>>>> + * That the pointer is right at the start of _tmp_type.
> >>>>>> + */
> >>>>>> +#define VMSTATE_WITH_TMP(_state, _tmp_type, _vmsd) { \
> >>>>>> + .name = "tmp", \
> >>>>>> + .size = sizeof(_tmp_type) + \
> >>>>>> + QEMU_BUILD_BUG_EXPR(offsetof(_tmp_type, parent) != 0) + \
> >>>>>> + type_check_pointer(_state, \
> >>>>>> + typeof_field(_tmp_type, parent)), \
> >>>>>> + .vmsd = &(_vmsd), \
> >>>>>> + .info = &vmstate_info_tmp, \
> >>>>>> + .flags = VMS_LINKED, \
> >>>>>> +}
> >>>>>> +
> >>>>>> #define VMSTATE_UNUSED_BUFFER(_test, _version, _size) { \
> >>>>>> .name = "unused", \
> >>>>>> .field_exists = (_test), \
> >>>>>> diff --git a/migration/vmstate.c b/migration/vmstate.c
> >>>>>> index 2157997..f2563c5 100644
> >>>>>> --- a/migration/vmstate.c
> >>>>>> +++ b/migration/vmstate.c
> >>>>>> @@ -925,6 +925,44 @@ const VMStateInfo vmstate_info_unused_buffer = {
> >>>>>> .put = put_unused_buffer,
> >>>>>> };
> >>>>>>
> >>>>>> +/* vmstate_info_tmp, see VMSTATE_WITH_TMP, the idea is that we allocate
> >>>>>> + * a temporary buffer and the pre_load/pre_save methods in the child vmsd
> >>>>>> + * copy stuff from the parent into the child and do calculations to fill
> >>>>>> + * in fields that don't really exist in the parent but need to be in the
> >>>>>> + * stream.
> >>>>>> + */
> >>>>>> +static int get_tmp(QEMUFile *f, void *pv, size_t size, VMStateField *field)
> >>>>>> +{
> >>>>>> + int ret;
> >>>>>> + const VMStateDescription *vmsd = field->vmsd;
> >>>>>> + int version_id = field->version_id;
> >>>>>> + void *tmp = g_malloc(size);
> >>>>>> +
> >>>>>> + /* Writes the parent field which is at the start of the tmp */
> >>>>>> + *(void **)tmp = pv;
> >>>>>> + ret = vmstate_load_state(f, vmsd, tmp, version_id);
> >>>>>> + g_free(tmp);
> >>>>>> + return ret;
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void put_tmp(QEMUFile *f, void *pv, size_t size, VMStateField *field,
> >>>>>> + QJSON *vmdesc)
> >>>>>> +{
> >>>>>> + const VMStateDescription *vmsd = field->vmsd;
> >>>>>> + void *tmp = g_malloc(size);
> >>>>>> +
> >>>>>> + /* Writes the parent field which is at the start of the tmp */
> >>>>>> + *(void **)tmp = pv;
> >>>>>> + vmstate_save_state(f, vmsd, tmp, vmdesc);
> >>>>>> + g_free(tmp);
> >>>>>> +}
> >>>>>> +
> >>>>>> +const VMStateInfo vmstate_info_tmp = {
> >>>>>> + .name = "tmp",
> >>>>>> + .get = get_tmp,
> >>>>>> + .put = put_tmp,
> >>>>>> +};
> >>>>>> +
> >>>>>> /* bitmaps (as defined by bitmap.h). Note that size here is the size
> >>>>>> * of the bitmap in bits. The on-the-wire format of a bitmap is 64
> >>>>>> * bit words with the bits in big endian order. The in-memory format
> >>>>>
> >>>>
> >>> --
> >>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >>>
> >>
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [very-WIP 3/4] slirp: VMStatify sbuf
2016-10-17 19:06 ` Dr. David Alan Gilbert
@ 2016-10-18 10:40 ` Halil Pasic
0 siblings, 0 replies; 17+ messages in thread
From: Halil Pasic @ 2016-10-18 10:40 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: David Gibson, qemu-devel, amit.shah, quintela, duanj
On 10/17/2016 09:06 PM, Dr. David Alan Gilbert wrote:
>> OK, memory was allocated at #2
>> > It is a bit confusing though (for a novice like me) that we have a non ALLOC VBUFFER
>> > whose pointer is NULL after post_load.
> I don't think this pointer can be NULL; the sbreserve at #2 causes it to be
> allocated.
> But yes, it's a shame I can't use VMS_ALLOC here, but the sbreserve is not
> a trivial allocation function.
Sorry my fault, wanted to say pre_load and not post_load. The assumption
that backing memory is allocated either during device realization or in the
pre_load of the parent or in vmstate_base_addr seemed a reasonable one
(prior to the extra possibilities introduced by the patches of Jianjun).
>
>> > Now if I imagine the original stream were written in the following sequence:
>> > vbuffer_length (sb_datalen), vbuffer_data (sb_data), offsets (sb_wptr, sb_rptr)
>> > which seems completely valid to me then the context would not be sufficient
>> > to compute sb_wptr and sb_rptr because the lifetime of vbuffer_data and
>> > the tmp do not overlap.
> If that was the case you could still do it pretty easily.
> You'd have to add the sb_datalen and sb_data fields to the temporary
> and then move the VMSTATE_VBUFFER_UINT32 into the tmp so it would operate
> on the copied fields.
>
That basically means you expand the area affected by the tmp handling
so that you have all the info you need to do the computation/transformation
before the temporary is freed, right? Then the worst it can get is that you need to
transform the first field and for that you need the last field.
>> > I aware it's a trade-off between how long the temporary data lives and
>> > how complicated the dependencies get. Or am I getting something wrong?
> No, I think that's right. The other option I thought of was a macro
> to allocate a temporary and then another to free it and then someway
> to tell macros in between that they should operate on the temporary
> rather than the main pointer; but then you'd have to be VERY careful
> to not allow yourself to access a temporary that's been freed.
> This structure means you can't make that mistake.
>
I have a couple of crazy half ideas of myself, I just do not feel
very comfortable sharing them, because right now I do not have the
capacity to explore them properly. Besides I don't know the code-base
well enough to say if this reasoning has some practical relevance
or is it 'rather academic'. Nevertheless I did not want to keep
the opinion to myself that this thing with the dependencies can
get rather convoluted under circumstances.
Cheers,
Halil
> Dave
>
>> >
>> > Cheers,
>> > Halil
>> >
>>>> > >> + VMSTATE_END_OF_LIST()
>>>> > >> + }
>>>> > >> +};
>> > [..]
>> >
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2016-10-18 10:40 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-11 17:18 [Qemu-devel] [very-WIP 0/4] Migration: VMSTATE_WITH_TMP Dr. David Alan Gilbert (git)
2016-10-11 17:18 ` [Qemu-devel] [very-WIP 1/7] migration: Add VMSTATE_WITH_TMP Dr. David Alan Gilbert (git)
2016-10-17 3:31 ` David Gibson
2016-10-17 18:49 ` Jianjun Duan
2016-10-17 18:52 ` Dr. David Alan Gilbert
2016-10-17 19:02 ` Jianjun Duan
2016-10-17 19:16 ` Dr. David Alan Gilbert
2016-10-17 19:30 ` Jianjun Duan
2016-10-18 8:06 ` Dr. David Alan Gilbert
2016-10-11 17:18 ` [Qemu-devel] [very-WIP 2/7] tests/migration: Add test for VMSTATE_WITH_TMP Dr. David Alan Gilbert (git)
2016-10-17 3:34 ` David Gibson
2016-10-11 17:18 ` [Qemu-devel] [very-WIP 3/4] slirp: VMStatify sbuf Dr. David Alan Gilbert (git)
2016-10-17 3:36 ` David Gibson
2016-10-17 17:54 ` Halil Pasic
2016-10-17 19:06 ` Dr. David Alan Gilbert
2016-10-18 10:40 ` Halil Pasic
2016-10-11 17:18 ` [Qemu-devel] [very-WIP 4/4] virtio/migration: Migrate virtio-net to VMState Dr. David Alan Gilbert (git)
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.