[Qemu-devel] [RFC 0/4] virtio: stabilize migration format

[Qemu-devel] [RFC 0/4] virtio: stabilize migration format

[Anthony Liguori]

This series replaces:

    qemu_put_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));

With code that properly saves out each element of the structure using
a well defined endian format.  Migration is broken today from big endian to
little endian hosts.

There's no way to fix this problem without bumping the migration version
number and that's exactly what we do here.  By bumping the migration version
number, we do break new->old migration but that's unavoidable right now.

In order to support old->new, we assume that all incoming data is in little
endian.  The final patch adds a check to the load routines to fail old->new
on big endian hosts where this may not have been true.

I've not tested this thoroughly enough to apply yet but wanted to share.

---
 hw/virtio-blk.c        |   22 +++++---
 hw/virtio-serial-bus.c |   19 +++----
 hw/virtio.c            |  123 +++++++++++++++++++++++++++++++++++++++++++++++++
 hw/virtio.h            |    4 +
 qemu-file.h            |    7 ++
 savevm.c               |   45 +++++++++++++++++
 6 files changed, 201 insertions(+), 19 deletions(-)

[Qemu-devel] [PATCH 1/4] savevm: introduce little endian variants of savevm routines

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 qemu-file.h |  7 +++++++
 savevm.c    | 45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

diff --git a/qemu-file.h b/qemu-file.h
index d64bdbb..ac5286c 100644
--- a/qemu-file.h
+++ b/qemu-file.h
@@ -94,6 +94,9 @@ static inline void qemu_put_ubyte(QEMUFile *f, unsigned int v)
 void qemu_put_be16(QEMUFile *f, unsigned int v);
 void qemu_put_be32(QEMUFile *f, unsigned int v);
 void qemu_put_be64(QEMUFile *f, uint64_t v);
+void qemu_put_le16(QEMUFile *f, unsigned int v);
+void qemu_put_le32(QEMUFile *f, unsigned int v);
+void qemu_put_le64(QEMUFile *f, uint64_t v);
 int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size);
 int qemu_get_byte(QEMUFile *f);
 
@@ -108,6 +111,10 @@ unsigned int qemu_get_be16(QEMUFile *f);
 unsigned int qemu_get_be32(QEMUFile *f);
 uint64_t qemu_get_be64(QEMUFile *f);
 
+unsigned int qemu_get_le16(QEMUFile *f);
+unsigned int qemu_get_le32(QEMUFile *f);
+uint64_t qemu_get_le64(QEMUFile *f);
+
 int qemu_file_rate_limit(QEMUFile *f);
 int64_t qemu_file_set_rate_limit(QEMUFile *f, int64_t new_rate);
 int64_t qemu_file_get_rate_limit(QEMUFile *f);
diff --git a/savevm.c b/savevm.c
index 5d04d59..f6ae0ba 100644
--- a/savevm.c
+++ b/savevm.c
@@ -749,6 +749,26 @@ void qemu_put_be64(QEMUFile *f, uint64_t v)
     qemu_put_be32(f, v);
 }
 
+void qemu_put_le16(QEMUFile *f, unsigned int v)
+{
+    qemu_put_byte(f, v);
+    qemu_put_byte(f, v >> 8);
+}
+
+void qemu_put_le32(QEMUFile *f, unsigned int v)
+{
+    qemu_put_byte(f, v);
+    qemu_put_byte(f, v >> 8);
+    qemu_put_byte(f, v >> 16);
+    qemu_put_byte(f, v >> 24);
+}
+
+void qemu_put_le64(QEMUFile *f, uint64_t v)
+{
+    qemu_put_be32(f, v);
+    qemu_put_be32(f, v >> 32);
+}
+
 unsigned int qemu_get_be16(QEMUFile *f)
 {
     unsigned int v;
@@ -775,6 +795,31 @@ uint64_t qemu_get_be64(QEMUFile *f)
     return v;
 }
 
+unsigned int qemu_get_le16(QEMUFile *f)
+{
+    unsigned int v;
+    v = qemu_get_byte(f);
+    v |= qemu_get_byte(f) << 8;
+    return v;
+}
+
+unsigned int qemu_get_le32(QEMUFile *f)
+{
+    unsigned int v;
+    v = qemu_get_byte(f);
+    v |= qemu_get_byte(f) << 8;
+    v |= qemu_get_byte(f) << 16;
+    v |= qemu_get_byte(f) << 24;
+    return v;
+}
+
+uint64_t qemu_get_le64(QEMUFile *f)
+{
+    uint64_t v;
+    v = qemu_get_be32(f);
+    v |= (uint64_t)qemu_get_be32(f) << 32;
+    return v;
+}
 
 /* timer */
 
-- 
1.8.0

[Qemu-devel] [PATCH 2/4] virtio: add wrapper for saving/restoring virtqueue elements

[Anthony Liguori]

Putting raw structures on the wire is bad news.  Add a wrapper and use it.

Note that in virtio-serial-bus, we were mapping both the in and out vectors as
writable.  This is a bug that is fixed by this change.  I checked the revision
history, it has been there since the code was first added and does not appear
to be intentional.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/virtio-blk.c        |  9 ++-------
 hw/virtio-serial-bus.c | 10 ++--------
 hw/virtio.c            | 13 +++++++++++++
 hw/virtio.h            |  4 ++++
 4 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index e25cc96..7ab174f 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -555,7 +555,7 @@ static void virtio_blk_save(QEMUFile *f, void *opaque)
     
     while (req) {
         qemu_put_sbyte(f, 1);
-        qemu_put_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));
+        virtio_put_virt_queue_element(f, &req->elem);
         req = req->next;
     }
     qemu_put_sbyte(f, 0);
@@ -576,14 +576,9 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
 
     while (qemu_get_sbyte(f)) {
         VirtIOBlockReq *req = virtio_blk_alloc_request(s);
-        qemu_get_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));
+        virtio_get_virt_queue_element(f, &req->elem);
         req->next = s->rq;
         s->rq = req;
-
-        virtqueue_map_sg(req->elem.in_sg, req->elem.in_addr,
-            req->elem.in_num, 1);
-        virtqueue_map_sg(req->elem.out_sg, req->elem.out_addr,
-            req->elem.out_num, 0);
     }
 
     return 0;
diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 155da58..aa1ded0 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -629,8 +629,7 @@ static void virtio_serial_save(QEMUFile *f, void *opaque)
             qemu_put_be32s(f, &port->iov_idx);
             qemu_put_be64s(f, &port->iov_offset);
 
-            qemu_put_buffer(f, (unsigned char *)&port->elem,
-                            sizeof(port->elem));
+            virtio_put_virt_queue_element(f, &port->elem);
         }
     }
 }
@@ -731,12 +730,7 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
                 qemu_get_be32s(f, &port->iov_idx);
                 qemu_get_be64s(f, &port->iov_offset);
 
-                qemu_get_buffer(f, (unsigned char *)&port->elem,
-                                sizeof(port->elem));
-                virtqueue_map_sg(port->elem.in_sg, port->elem.in_addr,
-                                 port->elem.in_num, 1);
-                virtqueue_map_sg(port->elem.out_sg, port->elem.out_addr,
-                                 port->elem.out_num, 1);
+                virtio_get_virt_queue_element(f, &port->elem);
 
                 /*
                  *  Port was throttled on source machine.  Let's
diff --git a/hw/virtio.c b/hw/virtio.c
index f40a8c5..8eb8f69 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -875,6 +875,19 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
     return 0;
 }
 
+void virtio_put_virt_queue_element(QEMUFile *f, const VirtQueueElement *elem)
+{
+    qemu_put_buffer(f, (unsigned char*)elem, sizeof(*elem));
+}
+
+void virtio_get_virt_queue_element(QEMUFile *f, VirtQueueElement *elem)
+{
+    qemu_get_buffer(f, (unsigned char *)elem, sizeof(*elem));
+
+    virtqueue_map_sg(elem->in_sg, elem->in_addr, elem->in_num, 1);
+    virtqueue_map_sg(elem->out_sg, elem->out_addr, elem->out_num, 0);
+}
+
 void virtio_cleanup(VirtIODevice *vdev)
 {
     qemu_del_vm_change_state_handler(vdev->vmstate);
diff --git a/hw/virtio.h b/hw/virtio.h
index 7c17f7b..4af8239 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -159,6 +159,10 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f);
 
 int virtio_load(VirtIODevice *vdev, QEMUFile *f);
 
+void virtio_put_virt_queue_element(QEMUFile *f, const VirtQueueElement *elem);
+
+void virtio_get_virt_queue_element(QEMUFile *f, VirtQueueElement *elem);
+
 void virtio_cleanup(VirtIODevice *vdev);
 
 void virtio_notify_config(VirtIODevice *vdev);
-- 
1.8.0

[Qemu-devel] [PATCH 3/4] virtio: modify savevm to have a stable wire format

[Anthony Liguori]

We were memcpy()'ing a structure to the wire :-/  Since savevm really
only works on x86 today, lets just declare that this element is sent
over the wire as a little endian value in order to fix the bitness.

Unfortunately, we also send raw pointers and size_t which are going
to be different values on a 32-bit vs. 64-bit QEMU so we need to also
deal with that case.

A lot of values that should have been previously ignored are now sent
as 0 and ignored on the receive side too.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/virtio.c | 114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 112 insertions(+), 2 deletions(-)

diff --git a/hw/virtio.c b/hw/virtio.c
index 8eb8f69..0108d62 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -875,14 +875,124 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
     return 0;
 }
 
+/* We used to memcpy the structure to the wire so that's the reason for all of
+ * this ugliness */
+
+#if HOST_LONG_BITS == 32
+static uint32 virtio_get_long(QEMUFile *f)
+{
+    return qemu_get_le32(f);
+}
+
+static void virtio_put_long(QEMUFile *f, uint32_t value)
+{
+    qemu_put_le32(f, value);
+}
+#elif HOST_LONG_BITS == 64
+static uint64 virtio_get_long(QEMUFile *f)
+{
+    return qemu_get_le64(f);
+}
+
+static void virtio_put_long(QEMUFile *f, uint64_t value)
+{
+    qemu_put_le64(f, value);
+}
+#else
+#error Invalid HOST_LONG_BITS
+#endif
+
 void virtio_put_virt_queue_element(QEMUFile *f, const VirtQueueElement *elem)
 {
-    qemu_put_buffer(f, (unsigned char*)elem, sizeof(*elem));
+    int i;
+
+    qemu_put_le32(f, elem->index);
+    qemu_put_le32(f, elem->out_num);
+    qemu_put_le32(f, elem->in_num);
+
+    qemu_put_le32(f, 0); /* padding for structure */
+
+    for (i = 0; i < VIRTQUEUE_MAX_SIZE; i++) {
+        if (i < elem->in_num) {
+            qemu_put_le64(f, elem->in_addr[i]);
+        } else {
+            qemu_put_le64(f, 0);
+        }
+    }
+
+    for (i = 0; i < VIRTQUEUE_MAX_SIZE; i++) {
+        if (i < elem->out_num) {
+            qemu_put_le64(f, elem->out_addr[i]);
+        } else {
+            qemu_put_le64(f, 0);
+        }
+    }
+
+    for (i = 0; i < VIRTQUEUE_MAX_SIZE; i++) {
+        virtio_put_long(f, 0);
+        if (i < elem->in_num) {
+            virtio_put_long(f, elem->in_sg[i].iov_len);
+        } else {
+            virtio_put_long(f, 0);
+        }
+    }
+
+    for (i = 0; i < VIRTQUEUE_MAX_SIZE; i++) {
+        virtio_put_long(f, 0);
+        if (i < elem->out_num) {
+            virtio_put_long(f, elem->out_sg[i].iov_len);
+        } else {
+            virtio_put_long(f, 0);
+        }
+    }
 }
 
 void virtio_get_virt_queue_element(QEMUFile *f, VirtQueueElement *elem)
 {
-    qemu_get_buffer(f, (unsigned char *)elem, sizeof(*elem));
+    int i;
+
+    elem->index = qemu_get_le32(f);
+    elem->out_num = qemu_get_le32(f);
+    elem->in_num = qemu_get_le32(f);
+
+    assert(elem->out_num <= VIRTQUEUE_MAX_SIZE &&
+           elem->in_num <= VIRTQUEUE_MAX_SIZE);
+
+    qemu_get_le32(f); /* padding for structure */
+
+    for (i = 0; i < VIRTQUEUE_MAX_SIZE; i++) {
+        if (i < elem->in_num) {
+            elem->in_addr[i] = qemu_get_le64(f);
+        } else {
+            qemu_get_le64(f);
+        }
+    }
+
+    for (i = 0; i < VIRTQUEUE_MAX_SIZE; i++) {
+        if (i < elem->out_num) {
+            elem->out_addr[i] = qemu_get_le64(f);
+        } else {
+            qemu_get_le64(f);
+        }
+    }
+
+    for (i = 0; i < VIRTQUEUE_MAX_SIZE; i++) {
+        virtio_get_long(f);
+        if (i < elem->in_num) {
+            elem->in_sg[i].iov_len = virtio_get_long(f);
+        } else {
+            virtio_get_long(f);
+        }
+    }
+
+    for (i = 0; i < VIRTQUEUE_MAX_SIZE; i++) {
+        virtio_get_long(f);
+        if (i < elem->out_num) {
+            elem->out_sg[i].iov_len = virtio_get_long(f);
+        } else {
+            virtio_get_long(f);
+        }
+    }
 
     virtqueue_map_sg(elem->in_sg, elem->in_addr, elem->in_num, 1);
     virtqueue_map_sg(elem->out_sg, elem->out_addr, elem->out_num, 0);
-- 
1.8.0

[Qemu-devel] [PATCH 4/4] virtio: bump migration version number

[Anthony Liguori]

And disable migration on big endian hosts from older versions where
endianness of the device state was ambiguous on the wire.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/virtio-blk.c        | 13 +++++++++++--
 hw/virtio-serial-bus.c |  9 +++++++--
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 7ab174f..837a709 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -566,8 +566,17 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
     VirtIOBlock *s = opaque;
     int ret;
 
-    if (version_id != 2)
+#ifdef HOST_WORDS_BIGENDIAN
+    /* Because of the use of native endianness, we can't reliably handle
+     * migration below this version on big endian hosts. */
+    if (version < 3) {
         return -EINVAL;
+    }
+#endif
+
+    if (version_id < 2) {
+        return -EINVAL;
+    }
 
     ret = virtio_load(&s->vdev, f);
     if (ret) {
@@ -633,7 +642,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, VirtIOBlkConf *blk)
 
     qemu_add_vm_change_state_handler(virtio_blk_dma_restart_cb, s);
     s->qdev = dev;
-    register_savevm(dev, "virtio-blk", virtio_blk_id++, 2,
+    register_savevm(dev, "virtio-blk", virtio_blk_id++, 3,
                     virtio_blk_save, virtio_blk_load, s);
     bdrv_set_dev_ops(s->bs, &virtio_block_ops, s);
     bdrv_set_buffer_alignment(s->bs, s->conf->logical_block_size);
diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index aa1ded0..6aa3b85 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -665,9 +665,14 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
     unsigned int i;
     int ret;
 
-    if (version_id > 3) {
+#ifdef HOST_WORDS_BIGENDIAN
+    /* Because of the use of native endianness, we can't reliably handle
+     * migration below this version on big endian hosts. */
+
+    if (version_id < 4) {
         return -EINVAL;
     }
+#endif
 
     /* The virtio device */
     ret = virtio_load(&s->vdev, f);
@@ -987,7 +992,7 @@ VirtIODevice *virtio_serial_init(DeviceState *dev, virtio_serial_conf *conf)
      * Register for the savevm section with the virtio-console name
      * to preserve backward compat
      */
-    register_savevm(dev, "virtio-console", -1, 3, virtio_serial_save,
+    register_savevm(dev, "virtio-console", -1, 4, virtio_serial_save,
                     virtio_serial_load, vser);
 
     vser->post_load.timer = qemu_new_timer_ns(vm_clock,
-- 
1.8.0

Re: [Qemu-devel] [PATCH 1/4] savevm: introduce little endian variants of savevm routines

[Peter Maydell]

On 10 December 2012 14:29, Anthony Liguori <aliguori@us.ibm.com> wrote:
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  qemu-file.h |  7 +++++++
>  savevm.c    | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 52 insertions(+)
>
> diff --git a/qemu-file.h b/qemu-file.h
> index d64bdbb..ac5286c 100644
> --- a/qemu-file.h
> +++ b/qemu-file.h
> @@ -94,6 +94,9 @@ static inline void qemu_put_ubyte(QEMUFile *f, unsigned int v)
>  void qemu_put_be16(QEMUFile *f, unsigned int v);
>  void qemu_put_be32(QEMUFile *f, unsigned int v);
>  void qemu_put_be64(QEMUFile *f, uint64_t v);
> +void qemu_put_le16(QEMUFile *f, unsigned int v);
> +void qemu_put_le32(QEMUFile *f, unsigned int v);
> +void qemu_put_le64(QEMUFile *f, uint64_t v);

Do we want to add a comment here somewhere that says the le versions
are for backcompat with a specific thing and the be ones are the
ones to use in new code (well, new code not using vmstate)?

-- PMM

Re: [Qemu-devel] [PATCH 1/4] savevm: introduce little endian variants of savevm routines

[Anthony Liguori]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 10 December 2012 14:29, Anthony Liguori <aliguori@us.ibm.com> wrote:
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>> ---
>>  qemu-file.h |  7 +++++++
>>  savevm.c    | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 52 insertions(+)
>>
>> diff --git a/qemu-file.h b/qemu-file.h
>> index d64bdbb..ac5286c 100644
>> --- a/qemu-file.h
>> +++ b/qemu-file.h
>> @@ -94,6 +94,9 @@ static inline void qemu_put_ubyte(QEMUFile *f, unsigned int v)
>>  void qemu_put_be16(QEMUFile *f, unsigned int v);
>>  void qemu_put_be32(QEMUFile *f, unsigned int v);
>>  void qemu_put_be64(QEMUFile *f, uint64_t v);
>> +void qemu_put_le16(QEMUFile *f, unsigned int v);
>> +void qemu_put_le32(QEMUFile *f, unsigned int v);
>> +void qemu_put_le64(QEMUFile *f, uint64_t v);
>
> Do we want to add a comment here somewhere that says the le versions
> are for backcompat with a specific thing and the be ones are the
> ones to use in new code (well, new code not using vmstate)?

Yeah, that's a good idea.  I've unfortunately found a couple more cases
of this (writing native endian to the wire).

Regards,

Anthony Liguori

>
> -- PMM

Re: [Qemu-devel] [PATCH 3/4] virtio: modify savevm to have a stable wire format

[Rusty Russell]

Anthony Liguori <aliguori@us.ibm.com> writes:
> We were memcpy()'ing a structure to the wire :-/  Since savevm really
> only works on x86 today, lets just declare that this element is sent
> over the wire as a little endian value in order to fix the bitness.
>
> Unfortunately, we also send raw pointers and size_t which are going
> to be different values on a 32-bit vs. 64-bit QEMU so we need to also
> deal with that case.
>
> A lot of values that should have been previously ignored are now sent
> as 0 and ignored on the receive side too.

Don't we want to transition to vmstate anyway?  Can we just do that, and
relegate the existing slightly broken code, to legacy?

Cheers,
Rusty.

Re: [Qemu-devel] [PATCH 3/4] virtio: modify savevm to have a stable wire format

[Anthony Liguori]

Rusty Russell <rusty@rustcorp.com.au> writes:

> Anthony Liguori <aliguori@us.ibm.com> writes:
>> We were memcpy()'ing a structure to the wire :-/  Since savevm really
>> only works on x86 today, lets just declare that this element is sent
>> over the wire as a little endian value in order to fix the bitness.
>>
>> Unfortunately, we also send raw pointers and size_t which are going
>> to be different values on a 32-bit vs. 64-bit QEMU so we need to also
>> deal with that case.
>>
>> A lot of values that should have been previously ignored are now sent
>> as 0 and ignored on the receive side too.
>
> Don't we want to transition to vmstate anyway?  Can we just do that, and
> relegate the existing slightly broken code, to legacy?

What worries me is if someone changes VirtQueueElement, then all the
sudden migration breaks.  By transitioning to what I've sent, we at
least have a better documented protocol that isn't prone to subtle
breakage anymore.  Plus, we resolve the endian issue before it becomes a
bigger problem when David actually gets live migration working reliably
on PPC...

I'm certainly in favor of cleaning up the savevm format and probably
leaving the existing load/save functions as-is for legacy purposes.
I'll leave that as an exercise for someone else though :-)

Regards,

Anthony Liguori

>
> Cheers,
> Rusty.

Re: [Qemu-devel] [RFC 0/4] virtio: stabilize migration format

[Stefan Hajnoczi]

On Mon, Dec 10, 2012 at 08:29:46AM -0600, Anthony Liguori wrote:
> This series replaces:
> 
>     qemu_put_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));
> 
> With code that properly saves out each element of the structure using
> a well defined endian format.  Migration is broken today from big endian to
> little endian hosts.
> 
> There's no way to fix this problem without bumping the migration version
> number and that's exactly what we do here.  By bumping the migration version
> number, we do break new->old migration but that's unavoidable right now.
> 
> In order to support old->new, we assume that all incoming data is in little
> endian.  The final patch adds a check to the load routines to fail old->new
> on big endian hosts where this may not have been true.

Is there a way to detect the endianness of the source host - by peaking
at a known multibyte value in the incoming stream?

That way we could even support cross-endian migration.

Not sure if this much magic makes sense since cross-endian migration is
probably used rarely.

Stefan

Re: [Qemu-devel] [PATCH 3/4] virtio: modify savevm to have a stable wire format

[Rusty Russell]

Anthony Liguori <anthony@codemonkey.ws> writes:
> Rusty Russell <rusty@rustcorp.com.au> writes:
>
>> Anthony Liguori <aliguori@us.ibm.com> writes:
>>> We were memcpy()'ing a structure to the wire :-/  Since savevm really
>>> only works on x86 today, lets just declare that this element is sent
>>> over the wire as a little endian value in order to fix the bitness.
>>>
>>> Unfortunately, we also send raw pointers and size_t which are going
>>> to be different values on a 32-bit vs. 64-bit QEMU so we need to also
>>> deal with that case.
>>>
>>> A lot of values that should have been previously ignored are now sent
>>> as 0 and ignored on the receive side too.
>>
>> Don't we want to transition to vmstate anyway?  Can we just do that, and
>> relegate the existing slightly broken code, to legacy?
>
> What worries me is if someone changes VirtQueueElement, then all the
> sudden migration breaks.  By transitioning to what I've sent, we at
> least have a better documented protocol that isn't prone to subtle
> breakage anymore.  Plus, we resolve the endian issue before it becomes a
> bigger problem when David actually gets live migration working reliably
> on PPC...

My transition was to copy that structure to VirtQueueSavedElement, and
it's only used for loading old versions.

With the new code we only need the head from that structure.

> I'm certainly in favor of cleaning up the savevm format and probably
> leaving the existing load/save functions as-is for legacy purposes.
> I'll leave that as an exercise for someone else though :-)

What is the rule about new versions?  Can we introduce a new save
version at any time, or only at major qemu version changes?

Thanks,
Rusty.

Re: [Qemu-devel] [PATCH 2/4] virtio: add wrapper for saving/restoring virtqueue elements

[Paolo Bonzini]

Il 10/12/2012 15:29, Anthony Liguori ha scritto:
> Putting raw structures on the wire is bad news.  Add a wrapper and use it.
> 
> Note that in virtio-serial-bus, we were mapping both the in and out vectors as
> writable.  This is a bug that is fixed by this change.  I checked the revision
> history, it has been there since the code was first added and does not appear
> to be intentional.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/virtio-blk.c        |  9 ++-------
>  hw/virtio-serial-bus.c | 10 ++--------
>  hw/virtio.c            | 13 +++++++++++++
>  hw/virtio.h            |  4 ++++
>  4 files changed, 21 insertions(+), 15 deletions(-)

virtio-scsi is missing; see virtio_scsi_load_request and
virtio_scsi_save_request.

Paolo

> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index e25cc96..7ab174f 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -555,7 +555,7 @@ static void virtio_blk_save(QEMUFile *f, void *opaque)
>      
>      while (req) {
>          qemu_put_sbyte(f, 1);
> -        qemu_put_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));
> +        virtio_put_virt_queue_element(f, &req->elem);
>          req = req->next;
>      }
>      qemu_put_sbyte(f, 0);
> @@ -576,14 +576,9 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
>  
>      while (qemu_get_sbyte(f)) {
>          VirtIOBlockReq *req = virtio_blk_alloc_request(s);
> -        qemu_get_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));
> +        virtio_get_virt_queue_element(f, &req->elem);
>          req->next = s->rq;
>          s->rq = req;
> -
> -        virtqueue_map_sg(req->elem.in_sg, req->elem.in_addr,
> -            req->elem.in_num, 1);
> -        virtqueue_map_sg(req->elem.out_sg, req->elem.out_addr,
> -            req->elem.out_num, 0);
>      }
>  
>      return 0;
> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> index 155da58..aa1ded0 100644
> --- a/hw/virtio-serial-bus.c
> +++ b/hw/virtio-serial-bus.c
> @@ -629,8 +629,7 @@ static void virtio_serial_save(QEMUFile *f, void *opaque)
>              qemu_put_be32s(f, &port->iov_idx);
>              qemu_put_be64s(f, &port->iov_offset);
>  
> -            qemu_put_buffer(f, (unsigned char *)&port->elem,
> -                            sizeof(port->elem));
> +            virtio_put_virt_queue_element(f, &port->elem);
>          }
>      }
>  }
> @@ -731,12 +730,7 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
>                  qemu_get_be32s(f, &port->iov_idx);
>                  qemu_get_be64s(f, &port->iov_offset);
>  
> -                qemu_get_buffer(f, (unsigned char *)&port->elem,
> -                                sizeof(port->elem));
> -                virtqueue_map_sg(port->elem.in_sg, port->elem.in_addr,
> -                                 port->elem.in_num, 1);
> -                virtqueue_map_sg(port->elem.out_sg, port->elem.out_addr,
> -                                 port->elem.out_num, 1);
> +                virtio_get_virt_queue_element(f, &port->elem);
>  
>                  /*
>                   *  Port was throttled on source machine.  Let's
> diff --git a/hw/virtio.c b/hw/virtio.c
> index f40a8c5..8eb8f69 100644
> --- a/hw/virtio.c
> +++ b/hw/virtio.c
> @@ -875,6 +875,19 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
>      return 0;
>  }
>  
> +void virtio_put_virt_queue_element(QEMUFile *f, const VirtQueueElement *elem)
> +{
> +    qemu_put_buffer(f, (unsigned char*)elem, sizeof(*elem));
> +}
> +
> +void virtio_get_virt_queue_element(QEMUFile *f, VirtQueueElement *elem)
> +{
> +    qemu_get_buffer(f, (unsigned char *)elem, sizeof(*elem));
> +
> +    virtqueue_map_sg(elem->in_sg, elem->in_addr, elem->in_num, 1);
> +    virtqueue_map_sg(elem->out_sg, elem->out_addr, elem->out_num, 0);
> +}
> +
>  void virtio_cleanup(VirtIODevice *vdev)
>  {
>      qemu_del_vm_change_state_handler(vdev->vmstate);
> diff --git a/hw/virtio.h b/hw/virtio.h
> index 7c17f7b..4af8239 100644
> --- a/hw/virtio.h
> +++ b/hw/virtio.h
> @@ -159,6 +159,10 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f);
>  
>  int virtio_load(VirtIODevice *vdev, QEMUFile *f);
>  
> +void virtio_put_virt_queue_element(QEMUFile *f, const VirtQueueElement *elem);
> +
> +void virtio_get_virt_queue_element(QEMUFile *f, VirtQueueElement *elem);
> +
>  void virtio_cleanup(VirtIODevice *vdev);
>  
>  void virtio_notify_config(VirtIODevice *vdev);
> 

Re: [Qemu-devel] [PATCH 3/4] virtio: modify savevm to have a stable wire format

[Paolo Bonzini]

Il 14/12/2012 01:57, Rusty Russell ha scritto:
> With the new code we only need the head from that structure.

We also need to do again all validation of the elements if we fetch it
back from the data.  Sometimes the parsed data is saved elsewhere (e.g.
in a SCSIRequest struct that is serialized by the SCSI subsystem) and
that data may be inconsistent with whatever you read from guest memory.
 It's a can of worms.

>> I'm certainly in favor of cleaning up the savevm format and probably
>> leaving the existing load/save functions as-is for legacy purposes.
>> I'll leave that as an exercise for someone else though :-)
> 
> What is the rule about new versions?  Can we introduce a new save
> version at any time, or only at major qemu version changes?

Any time, but we provide a backwards-compatible loader for older versions.

Paolo

Re: [Qemu-devel] [PATCH 2/4] virtio: add wrapper for saving/restoring virtqueue elements

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 10/12/2012 15:29, Anthony Liguori ha scritto:
>> Putting raw structures on the wire is bad news.  Add a wrapper and use it.
>> 
>> Note that in virtio-serial-bus, we were mapping both the in and out vectors as
>> writable.  This is a bug that is fixed by this change.  I checked the revision
>> history, it has been there since the code was first added and does not appear
>> to be intentional.
>> 
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>> ---
>>  hw/virtio-blk.c        |  9 ++-------
>>  hw/virtio-serial-bus.c | 10 ++--------
>>  hw/virtio.c            | 13 +++++++++++++
>>  hw/virtio.h            |  4 ++++
>>  4 files changed, 21 insertions(+), 15 deletions(-)
>
> virtio-scsi is missing; see virtio_scsi_load_request and
> virtio_scsi_save_request.

Indeed, thanks.

Regards,

Anthony Liguori

>
> Paolo
>
>> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
>> index e25cc96..7ab174f 100644
>> --- a/hw/virtio-blk.c
>> +++ b/hw/virtio-blk.c
>> @@ -555,7 +555,7 @@ static void virtio_blk_save(QEMUFile *f, void *opaque)
>>      
>>      while (req) {
>>          qemu_put_sbyte(f, 1);
>> -        qemu_put_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));
>> +        virtio_put_virt_queue_element(f, &req->elem);
>>          req = req->next;
>>      }
>>      qemu_put_sbyte(f, 0);
>> @@ -576,14 +576,9 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
>>  
>>      while (qemu_get_sbyte(f)) {
>>          VirtIOBlockReq *req = virtio_blk_alloc_request(s);
>> -        qemu_get_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));
>> +        virtio_get_virt_queue_element(f, &req->elem);
>>          req->next = s->rq;
>>          s->rq = req;
>> -
>> -        virtqueue_map_sg(req->elem.in_sg, req->elem.in_addr,
>> -            req->elem.in_num, 1);
>> -        virtqueue_map_sg(req->elem.out_sg, req->elem.out_addr,
>> -            req->elem.out_num, 0);
>>      }
>>  
>>      return 0;
>> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
>> index 155da58..aa1ded0 100644
>> --- a/hw/virtio-serial-bus.c
>> +++ b/hw/virtio-serial-bus.c
>> @@ -629,8 +629,7 @@ static void virtio_serial_save(QEMUFile *f, void *opaque)
>>              qemu_put_be32s(f, &port->iov_idx);
>>              qemu_put_be64s(f, &port->iov_offset);
>>  
>> -            qemu_put_buffer(f, (unsigned char *)&port->elem,
>> -                            sizeof(port->elem));
>> +            virtio_put_virt_queue_element(f, &port->elem);
>>          }
>>      }
>>  }
>> @@ -731,12 +730,7 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
>>                  qemu_get_be32s(f, &port->iov_idx);
>>                  qemu_get_be64s(f, &port->iov_offset);
>>  
>> -                qemu_get_buffer(f, (unsigned char *)&port->elem,
>> -                                sizeof(port->elem));
>> -                virtqueue_map_sg(port->elem.in_sg, port->elem.in_addr,
>> -                                 port->elem.in_num, 1);
>> -                virtqueue_map_sg(port->elem.out_sg, port->elem.out_addr,
>> -                                 port->elem.out_num, 1);
>> +                virtio_get_virt_queue_element(f, &port->elem);
>>  
>>                  /*
>>                   *  Port was throttled on source machine.  Let's
>> diff --git a/hw/virtio.c b/hw/virtio.c
>> index f40a8c5..8eb8f69 100644
>> --- a/hw/virtio.c
>> +++ b/hw/virtio.c
>> @@ -875,6 +875,19 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
>>      return 0;
>>  }
>>  
>> +void virtio_put_virt_queue_element(QEMUFile *f, const VirtQueueElement *elem)
>> +{
>> +    qemu_put_buffer(f, (unsigned char*)elem, sizeof(*elem));
>> +}
>> +
>> +void virtio_get_virt_queue_element(QEMUFile *f, VirtQueueElement *elem)
>> +{
>> +    qemu_get_buffer(f, (unsigned char *)elem, sizeof(*elem));
>> +
>> +    virtqueue_map_sg(elem->in_sg, elem->in_addr, elem->in_num, 1);
>> +    virtqueue_map_sg(elem->out_sg, elem->out_addr, elem->out_num, 0);
>> +}
>> +
>>  void virtio_cleanup(VirtIODevice *vdev)
>>  {
>>      qemu_del_vm_change_state_handler(vdev->vmstate);
>> diff --git a/hw/virtio.h b/hw/virtio.h
>> index 7c17f7b..4af8239 100644
>> --- a/hw/virtio.h
>> +++ b/hw/virtio.h
>> @@ -159,6 +159,10 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f);
>>  
>>  int virtio_load(VirtIODevice *vdev, QEMUFile *f);
>>  
>> +void virtio_put_virt_queue_element(QEMUFile *f, const VirtQueueElement *elem);
>> +
>> +void virtio_get_virt_queue_element(QEMUFile *f, VirtQueueElement *elem);
>> +
>>  void virtio_cleanup(VirtIODevice *vdev);
>>  
>>  void virtio_notify_config(VirtIODevice *vdev);
>> 

Re: [Qemu-devel] [PATCH 3/4] virtio: modify savevm to have a stable wire format

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 14/12/2012 01:57, Rusty Russell ha scritto:
>> With the new code we only need the head from that structure.
>
> We also need to do again all validation of the elements if we fetch it
> back from the data.  Sometimes the parsed data is saved elsewhere (e.g.
> in a SCSIRequest struct that is serialized by the SCSI subsystem) and
> that data may be inconsistent with whatever you read from guest memory.
>  It's a can of worms.
>
>>> I'm certainly in favor of cleaning up the savevm format and probably
>>> leaving the existing load/save functions as-is for legacy purposes.
>>> I'll leave that as an exercise for someone else though :-)
>> 
>> What is the rule about new versions?  Can we introduce a new save
>> version at any time, or only at major qemu version changes?
>
> Any time, but we provide a backwards-compatible loader for older
> versions.

Well..  if we can avoid bumping the version, we should.  Bumping the
version breaks migration from new -> old so it's preferrable that we
don't do it unless it's absolutely required.

Regards,

Anthony Liguori

>
> Paolo