[Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Greg Kurz]

Some CPU families can dynamically change their endianness. This means we
can have little endian ppc or big endian arm guests for example. This has
an impact on legacy virtio data structures since they are target endian.
We hence introduce a new property to track the endianness of each virtio
device. It is reasonnably assumed that endianness won't change while the
device is in use : we hence capture the device endianness when it gets
reset.

Of course this property must be part of the migration stream as most of
the virtio code will depend on it. It is hence migrated in a subsection
to preserve compatibility with older migration streams. The tricky part
is that the endianness gets known quite late and we must ensure no access
is made to virtio data before that. It is for example invalid to call
vring_avail_idx() as does the actual migration code: the VQ indexes sanity
checks had to be moved from virtio_load() to virtio_load_subsections()
because of that.

In fact, we have two conditions where the endianness property is stale:
- from initialization time (virtio_init) to first reset time (virtio_reset)
- from incoming migration time (virtio_load) to the time the property
  is restored (virtio_load_device_endian)
We enforce some paranoia by making the endianness a 3-value enum so
that we can poison it when it should not be used.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 exec.c                     |    8 +-----
 hw/virtio/virtio-pci.c     |   11 +++-----
 hw/virtio/virtio.c         |   64 ++++++++++++++++++++++++++++++++++++--------
 include/exec/cpu-common.h  |    1 +
 include/hw/virtio/virtio.h |   13 +++++++++
 5 files changed, 72 insertions(+), 25 deletions(-)

diff --git a/exec.c b/exec.c
index 91513c6..4e83588 100644
--- a/exec.c
+++ b/exec.c
@@ -2740,13 +2740,7 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
 #endif
 
 #if !defined(CONFIG_USER_ONLY)
-
-/*
- * A helper function for the _utterly broken_ virtio device model to find out if
- * it's running on a big endian machine. Don't do this at home kids!
- */
-bool virtio_is_big_endian(void);
-bool virtio_is_big_endian(void)
+bool target_words_bigendian(void)
 {
 #if defined(TARGET_WORDS_BIGENDIAN)
     return true;
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index ce97514..2ffceb8 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -89,9 +89,6 @@
 /* Flags track per-device state like workarounds for quirks in older guests. */
 #define VIRTIO_PCI_FLAG_BUS_MASTER_BUG  (1 << 0)
 
-/* HACK for virtio to determine if it's running a big endian guest */
-bool virtio_is_big_endian(void);
-
 static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
                                VirtIOPCIProxy *dev);
 
@@ -409,13 +406,13 @@ static uint64_t virtio_pci_config_read(void *opaque, hwaddr addr,
         break;
     case 2:
         val = virtio_config_readw(vdev, addr);
-        if (virtio_is_big_endian()) {
+        if (virtio_is_big_endian(vdev)) {
             val = bswap16(val);
         }
         break;
     case 4:
         val = virtio_config_readl(vdev, addr);
-        if (virtio_is_big_endian()) {
+        if (virtio_is_big_endian(vdev)) {
             val = bswap32(val);
         }
         break;
@@ -443,13 +440,13 @@ static void virtio_pci_config_write(void *opaque, hwaddr addr,
         virtio_config_writeb(vdev, addr, val);
         break;
     case 2:
-        if (virtio_is_big_endian()) {
+        if (virtio_is_big_endian(vdev)) {
             val = bswap16(val);
         }
         virtio_config_writew(vdev, addr, val);
         break;
     case 4:
-        if (virtio_is_big_endian()) {
+        if (virtio_is_big_endian(vdev)) {
             val = bswap32(val);
         }
         virtio_config_writel(vdev, addr, val);
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index f4eaa3f..9018b6c 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -547,6 +547,12 @@ void virtio_reset(void *opaque)
 
     virtio_set_status(vdev, 0);
 
+    if (target_words_bigendian()) {
+        vdev->device_endian = VIRTIO_DEVICE_ENDIAN_BIG;
+    } else {
+        vdev->device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE;
+    }
+
     if (k->reset) {
         k->reset(vdev);
     }
@@ -834,12 +840,28 @@ void virtio_notify_config(VirtIODevice *vdev)
     virtio_notify_vector(vdev, vdev->config_vector);
 }
 
+static void virtio_save_device_endian(VirtIODevice *vdev, QEMUFile *f)
+{
+    qemu_put_byte(f, vdev->device_endian);
+}
+
+static int virtio_load_device_endian(VirtIODevice *vdev, QEMUFile *f)
+{
+    vdev->device_endian = qemu_get_byte(f);
+    return 0;
+}
+
 static const struct VirtIOSubsectionDescStruct {
     const char *name;
     int version_id;
     void (*save)(VirtIODevice *vdev, QEMUFile *f);
     int (*load)(VirtIODevice *vdev, QEMUFile *f);
 } virtio_subsection[] = {
+    { .name = "virtio/is_big_endian",
+      .version_id = 1,
+      .save = virtio_save_device_endian,
+      .load = virtio_load_device_endian,
+    },
     { .name = NULL }
 };
 
@@ -861,6 +883,8 @@ void virtio_save_subsections(VirtIODevice *vdev, QEMUFile *f)
 
 int virtio_load_subsections(VirtIODevice *vdev, QEMUFile *f)
 {
+    int i;
+
     while (qemu_peek_byte(f, 0) == QEMU_VM_SUBSECTION) {
         char idstr[256];
         uint8_t len, size;
@@ -895,6 +919,26 @@ int virtio_load_subsections(VirtIODevice *vdev, QEMUFile *f)
         }
     }
 
+    for (i = 0; i < VIRTIO_PCI_QUEUE_MAX; i++) {
+        if (vdev->vq[i].vring.num == 0) {
+            break;
+        }
+
+        if (vdev->vq[i].pa) {
+            uint16_t nheads;
+            nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
+            /* Check it isn't doing strange things with descriptor numbers. */
+            if (nheads > vdev->vq[i].vring.num) {
+                error_report("VQ %d size 0x%x Guest index 0x%x "
+                             "inconsistent with Host index 0x%x: delta 0x%x",
+                             i, vdev->vq[i].vring.num,
+                             vring_avail_idx(&vdev->vq[i]),
+                             vdev->vq[i].last_avail_idx, nheads);
+                return -1;
+            }
+        }
+    }
+
     return 0;
 }
 
@@ -962,6 +1006,11 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
     BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
 
+    /* We poison the endianness to ensure it does not get used before
+     * subsections have been loaded.
+     */
+    vdev->device_endian = VIRTIO_DEVICE_ENDIAN_UNKNOWN;
+
     if (k->load_config) {
         ret = k->load_config(qbus->parent, f);
         if (ret)
@@ -995,18 +1044,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
         vdev->vq[i].notification = true;
 
         if (vdev->vq[i].pa) {
-            uint16_t nheads;
             virtqueue_init(&vdev->vq[i]);
-            nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
-            /* Check it isn't doing very strange things with descriptor numbers. */
-            if (nheads > vdev->vq[i].vring.num) {
-                error_report("VQ %d size 0x%x Guest index 0x%x "
-                             "inconsistent with Host index 0x%x: delta 0x%x",
-                             i, vdev->vq[i].vring.num,
-                             vring_avail_idx(&vdev->vq[i]),
-                             vdev->vq[i].last_avail_idx, nheads);
-                return -1;
-            }
         } else if (vdev->vq[i].last_avail_idx) {
             error_report("VQ %d address 0x0 "
                          "inconsistent with Host index 0x%x",
@@ -1078,6 +1116,10 @@ void virtio_init(VirtIODevice *vdev, const char *name,
     }
     vdev->vmstate = qemu_add_vm_change_state_handler(virtio_vmstate_change,
                                                      vdev);
+    /* We poison the endianness to ensure it does not get used before
+     * the device is reset.
+     */
+    vdev->device_endian = VIRTIO_DEVICE_ENDIAN_UNKNOWN;
 }
 
 hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n)
diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index a21b65a..eb798c1 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -122,4 +122,5 @@ void qemu_ram_foreach_block(RAMBlockIterFunc func, void *opaque);
 
 #endif
 
+bool target_words_bigendian(void);
 #endif /* !CPU_COMMON_H */
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 82f852f..0012470 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -104,6 +104,12 @@ typedef struct VirtQueueElement
 #define VIRTIO_DEVICE(obj) \
         OBJECT_CHECK(VirtIODevice, (obj), TYPE_VIRTIO_DEVICE)
 
+enum virtio_device_endian {
+    VIRTIO_DEVICE_ENDIAN_UNKNOWN,
+    VIRTIO_DEVICE_ENDIAN_LITTLE,
+    VIRTIO_DEVICE_ENDIAN_BIG,
+};
+
 struct VirtIODevice
 {
     DeviceState parent_obj;
@@ -121,6 +127,7 @@ struct VirtIODevice
     bool vm_running;
     VMChangeStateEntry *vmstate;
     char *bus_name;
+    enum virtio_device_endian device_endian;
 };
 
 typedef struct VirtioDeviceClass {
@@ -257,4 +264,10 @@ void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
                                                bool set_handler);
 void virtio_queue_notify_vq(VirtQueue *vq);
 void virtio_irq(VirtQueue *vq);
+
+static inline bool virtio_is_big_endian(VirtIODevice *vdev)
+{
+    assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
+    return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
+}
 #endif

[Qemu-devel] [PATCH RFC V2 0/8] virtio: migrate new properties

[Greg Kurz]

Hi,

This patch set tries to address comments from the initial
review. For this round, I have focused on two changes:
- as suggested by Andreas, we now call the device specific
  code from the generic code to ease the implementation of
  future devices. This is achieved with the addition of
  load/save methods to VirtioDeviceClass.
- virtio subsections now implement a "needed" concept with
  the same semantics as in the VMState code.

I haven't looked at compat mode issues yet, but it is
on my TODO list.

Cheers.

---

Greg Kurz (8):
      virtio: introduce device specific migration calls
      virtio-net: implement per-device migration calls
      virtio-blk: implement per-device migration calls
      virtio-serial: implement per-device migration calls
      virtio-balloon: implement per-device migration calls
      virtio-rng: implement per-device migration calls
      virtio: add subsections to the migration stream
      virtio: add endian-ambivalent support to VirtIODevice


 exec.c                      |    8 --
 hw/block/virtio-blk.c       |   24 ++++--
 hw/char/virtio-serial-bus.c |   32 +++++---
 hw/net/virtio-net.c         |   22 ++++--
 hw/scsi/virtio-scsi.c       |    2 -
 hw/virtio/virtio-balloon.c  |   25 ++++--
 hw/virtio/virtio-pci.c      |   11 +--
 hw/virtio/virtio-rng.c      |   12 ++-
 hw/virtio/virtio.c          |  169 ++++++++++++++++++++++++++++++++++++++++---
 include/exec/cpu-common.h   |    1 
 include/hw/virtio/virtio.h  |   17 ++++
 11 files changed, 251 insertions(+), 72 deletions(-)

-- 
Greg

[Qemu-devel] [PATCH RFC 1/8] virtio: introduce device specific migration calls

[Greg Kurz]

In order to migrate virtio subsections, we need the device specific
code to be called from the common migration code. This patch introduces
load and save methods for this purpose.

Suggested-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/block/virtio-blk.c       |    2 +-
 hw/char/virtio-serial-bus.c |    2 +-
 hw/net/virtio-net.c         |    2 +-
 hw/scsi/virtio-scsi.c       |    2 +-
 hw/virtio/virtio-balloon.c  |    2 +-
 hw/virtio/virtio-rng.c      |    2 +-
 hw/virtio/virtio.c          |   13 ++++++++++++-
 include/hw/virtio/virtio.h  |    4 +++-
 8 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 8a568e5..63d4ccd 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -611,7 +611,7 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
     if (version_id != 2)
         return -EINVAL;
 
-    ret = virtio_load(vdev, f);
+    ret = virtio_load(vdev, f, version_id);
     if (ret) {
         return ret;
     }
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 2b647b6..b0f322a 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -676,7 +676,7 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
     }
 
     /* The virtio device */
-    ret = virtio_load(VIRTIO_DEVICE(s), f);
+    ret = virtio_load(VIRTIO_DEVICE(s), f, version_id);
     if (ret) {
         return ret;
     }
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index fd23c46..e0dc544 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1311,7 +1311,7 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
     if (version_id < 2 || version_id > VIRTIO_NET_VM_VERSION)
         return -EINVAL;
 
-    ret = virtio_load(vdev, f);
+    ret = virtio_load(vdev, f, version_id);
     if (ret) {
         return ret;
     }
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index b0d7517..e82e66d 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -478,7 +478,7 @@ static int virtio_scsi_load(QEMUFile *f, void *opaque, int version_id)
     VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
     int ret;
 
-    ret = virtio_load(vdev, f);
+    ret = virtio_load(vdev, f, version_id);
     if (ret) {
         return ret;
     }
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index a470a0b..6d8ec72 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -327,7 +327,7 @@ static int virtio_balloon_load(QEMUFile *f, void *opaque, int version_id)
     if (version_id != 1)
         return -EINVAL;
 
-    ret = virtio_load(vdev, f);
+    ret = virtio_load(vdev, f, version_id);
     if (ret) {
         return ret;
     }
diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index b6ab361..025de81 100644
--- a/hw/virtio/virtio-rng.c
+++ b/hw/virtio/virtio-rng.c
@@ -113,7 +113,7 @@ static int virtio_rng_load(QEMUFile *f, void *opaque, int version_id)
     if (version_id != 1) {
         return -EINVAL;
     }
-    virtio_load(vdev, f);
+    virtio_load(vdev, f, version_id);
 
     /* We may have an element ready but couldn't process it due to a quota
      * limit.  Make sure to try again after live migration when the quota may
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index aeabf3a..cf87b44 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -837,6 +837,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
 {
     BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+    VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
     int i;
 
     if (k->save_config) {
@@ -871,6 +872,10 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
             k->save_queue(qbus->parent, i, f);
         }
     }
+
+    if (vdc->save != NULL) {
+        vdc->save(vdev, f);
+    }
 }
 
 int virtio_set_features(VirtIODevice *vdev, uint32_t val)
@@ -889,13 +894,14 @@ int virtio_set_features(VirtIODevice *vdev, uint32_t val)
     return bad ? -1 : 0;
 }
 
-int virtio_load(VirtIODevice *vdev, QEMUFile *f)
+int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
 {
     int num, i, ret;
     uint32_t features;
     uint32_t supported_features;
     BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+    VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
 
     if (k->load_config) {
         ret = k->load_config(qbus->parent, f);
@@ -956,6 +962,11 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
     }
 
     virtio_notify_vector(vdev, VIRTIO_NO_VECTOR);
+
+    if (vdc->load != NULL) {
+        return vdc->load(vdev, f, version_id);
+    }
+
     return 0;
 }
 
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 3e54e90..3505ce5 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -150,6 +150,8 @@ typedef struct VirtioDeviceClass {
      * must mask in frontend instead.
      */
     void (*guest_notifier_mask)(VirtIODevice *vdev, int n, bool mask);
+    void (*save)(VirtIODevice *vdev, QEMUFile *f);
+    int (*load)(VirtIODevice *vdev, QEMUFile *f, int version_id);
 } VirtioDeviceClass;
 
 void virtio_init(VirtIODevice *vdev, const char *name,
@@ -184,7 +186,7 @@ void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);
 
 void virtio_save(VirtIODevice *vdev, QEMUFile *f);
 
-int virtio_load(VirtIODevice *vdev, QEMUFile *f);
+int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id);
 
 void virtio_notify_config(VirtIODevice *vdev);
 

[Qemu-devel] [PATCH RFC 2/8] virtio-net: implement per-device migration calls

[Greg Kurz]

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/net/virtio-net.c |   22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index e0dc544..d319ac2 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1263,7 +1263,6 @@ static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue)
 
 static void virtio_net_save(QEMUFile *f, void *opaque)
 {
-    int i;
     VirtIONet *n = opaque;
     VirtIODevice *vdev = VIRTIO_DEVICE(n);
 
@@ -1271,6 +1270,12 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
      * it might keep writing to memory. */
     assert(!n->vhost_started);
     virtio_save(vdev, f);
+}
+
+static void virtio_net_save_device(VirtIODevice *vdev, QEMUFile *f)
+{
+    VirtIONet *n = VIRTIO_NET(vdev);
+    int i;
 
     qemu_put_buffer(f, n->mac, ETH_ALEN);
     qemu_put_be32(f, n->vqs[0].tx_waiting);
@@ -1306,15 +1311,18 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
 {
     VirtIONet *n = opaque;
     VirtIODevice *vdev = VIRTIO_DEVICE(n);
-    int ret, i, link_down;
 
     if (version_id < 2 || version_id > VIRTIO_NET_VM_VERSION)
         return -EINVAL;
 
-    ret = virtio_load(vdev, f, version_id);
-    if (ret) {
-        return ret;
-    }
+    return virtio_load(vdev, f, version_id);
+}
+
+static int virtio_net_load_device(VirtIODevice *vdev, QEMUFile *f,
+                                  int version_id)
+{
+    VirtIONet *n = VIRTIO_NET(vdev);
+    int i, link_down;
 
     qemu_get_buffer(f, n->mac, ETH_ALEN);
     n->vqs[0].tx_waiting = qemu_get_be32(f);
@@ -1651,6 +1659,8 @@ 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;
 }
 
 static const TypeInfo virtio_net_info = {

[Qemu-devel] [PATCH RFC 3/8] virtio-blk: implement per-device migration calls

[Greg Kurz]

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/block/virtio-blk.c |   24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 63d4ccd..a50be54 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -588,12 +588,16 @@ static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t status)
 
 static void virtio_blk_save(QEMUFile *f, void *opaque)
 {
-    VirtIOBlock *s = opaque;
-    VirtIODevice *vdev = VIRTIO_DEVICE(s);
-    VirtIOBlockReq *req = s->rq;
+    VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
 
     virtio_save(vdev, f);
+}
     
+static void virtio_blk_save_device(VirtIODevice *vdev, QEMUFile *f)
+{
+    VirtIOBlock *s = VIRTIO_BLK(vdev);
+    VirtIOBlockReq *req = s->rq;
+
     while (req) {
         qemu_put_sbyte(f, 1);
         qemu_put_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));
@@ -606,15 +610,17 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
 {
     VirtIOBlock *s = opaque;
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
-    int ret;
 
     if (version_id != 2)
         return -EINVAL;
 
-    ret = virtio_load(vdev, f, version_id);
-    if (ret) {
-        return ret;
-    }
+    return virtio_load(vdev, f, version_id);
+}
+
+static int virtio_blk_load_device(VirtIODevice *vdev, QEMUFile *f,
+                                  int version_id)
+{
+    VirtIOBlock *s = VIRTIO_BLK(vdev);
 
     while (qemu_get_sbyte(f)) {
         VirtIOBlockReq *req = virtio_blk_alloc_request(s);
@@ -773,6 +779,8 @@ static void virtio_blk_class_init(ObjectClass *klass, void *data)
     vdc->get_features = virtio_blk_get_features;
     vdc->set_status = virtio_blk_set_status;
     vdc->reset = virtio_blk_reset;
+    vdc->save = virtio_blk_save_device;
+    vdc->load = virtio_blk_load_device;
 }
 
 static const TypeInfo virtio_device_info = {

[Qemu-devel] [PATCH RFC 4/8] virtio-serial: implement per-device migration calls

[Greg Kurz]

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/char/virtio-serial-bus.c |   32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index b0f322a..ce336a0 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -521,14 +521,17 @@ static void vser_reset(VirtIODevice *vdev)
 
 static void virtio_serial_save(QEMUFile *f, void *opaque)
 {
-    VirtIOSerial *s = VIRTIO_SERIAL(opaque);
+    /* The virtio device */
+    virtio_save(VIRTIO_DEVICE(opaque), f);
+}
+
+static void virtio_serial_save_device(VirtIODevice *vdev, QEMUFile *f)
+{
+    VirtIOSerial *s = VIRTIO_SERIAL(vdev);
     VirtIOSerialPort *port;
     uint32_t nr_active_ports;
     unsigned int i, max_nr_ports;
 
-    /* The virtio device */
-    virtio_save(VIRTIO_DEVICE(s), f);
-
     /* The config space */
     qemu_put_be16s(f, &s->config.cols);
     qemu_put_be16s(f, &s->config.rows);
@@ -666,20 +669,21 @@ static int fetch_active_ports_list(QEMUFile *f, int version_id,
 
 static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
 {
-    VirtIOSerial *s = VIRTIO_SERIAL(opaque);
-    uint32_t max_nr_ports, nr_active_ports, ports_map;
-    unsigned int i;
-    int ret;
-
     if (version_id > 3) {
         return -EINVAL;
     }
 
     /* The virtio device */
-    ret = virtio_load(VIRTIO_DEVICE(s), f, version_id);
-    if (ret) {
-        return ret;
-    }
+    return virtio_load(VIRTIO_DEVICE(opaque), f, version_id);
+}
+
+static int virtio_serial_load_device(VirtIODevice *vdev, QEMUFile *f,
+                                     int version_id)
+{
+    VirtIOSerial *s = VIRTIO_SERIAL(vdev);
+    uint32_t max_nr_ports, nr_active_ports, ports_map;
+    unsigned int i;
+    int ret;
 
     if (version_id < 2) {
         return 0;
@@ -1027,6 +1031,8 @@ static void virtio_serial_class_init(ObjectClass *klass, void *data)
     vdc->set_config = set_config;
     vdc->set_status = set_status;
     vdc->reset = vser_reset;
+    vdc->save = virtio_serial_save_device;
+    vdc->load = virtio_serial_load_device;
 }
 
 static const TypeInfo virtio_device_info = {

[Qemu-devel] [PATCH RFC 5/8] virtio-balloon: implement per-device migration calls

[Greg Kurz]

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/virtio/virtio-balloon.c |   25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 6d8ec72..a3a9ac1 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -309,10 +309,12 @@ static void virtio_balloon_to_target(void *opaque, ram_addr_t target)
 
 static void virtio_balloon_save(QEMUFile *f, void *opaque)
 {
-    VirtIOBalloon *s = VIRTIO_BALLOON(opaque);
-    VirtIODevice *vdev = VIRTIO_DEVICE(s);
+    virtio_save(VIRTIO_DEVICE(opaque), f);
+}
 
-    virtio_save(vdev, f);
+static void virtio_balloon_save_device(VirtIODevice *vdev, QEMUFile *f)
+{
+    VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
 
     qemu_put_be32(f, s->num_pages);
     qemu_put_be32(f, s->actual);
@@ -320,17 +322,16 @@ static void virtio_balloon_save(QEMUFile *f, void *opaque)
 
 static int virtio_balloon_load(QEMUFile *f, void *opaque, int version_id)
 {
-    VirtIOBalloon *s = VIRTIO_BALLOON(opaque);
-    VirtIODevice *vdev = VIRTIO_DEVICE(s);
-    int ret;
-
     if (version_id != 1)
         return -EINVAL;
 
-    ret = virtio_load(vdev, f, version_id);
-    if (ret) {
-        return ret;
-    }
+    return virtio_load(VIRTIO_DEVICE(opaque), f, version_id);
+}
+
+static int virtio_balloon_load_device(VirtIODevice *vdev, QEMUFile *f,
+                                      int version_id)
+{
+    VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
 
     s->num_pages = qemu_get_be32(f);
     s->actual = qemu_get_be32(f);
@@ -398,6 +399,8 @@ static void virtio_balloon_class_init(ObjectClass *klass, void *data)
     vdc->get_config = virtio_balloon_get_config;
     vdc->set_config = virtio_balloon_set_config;
     vdc->get_features = virtio_balloon_get_features;
+    vdc->save = virtio_balloon_save_device;
+    vdc->load = virtio_balloon_load_device;
 }
 
 static const TypeInfo virtio_balloon_info = {

[Qemu-devel] [PATCH RFC 6/8] virtio-rng: implement per-device migration calls

[Greg Kurz]

While we are here, we also check virtio_load() return value.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/virtio/virtio-rng.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index 025de81..1356aca 100644
--- a/hw/virtio/virtio-rng.c
+++ b/hw/virtio/virtio-rng.c
@@ -107,19 +107,20 @@ static void virtio_rng_save(QEMUFile *f, void *opaque)
 
 static int virtio_rng_load(QEMUFile *f, void *opaque, int version_id)
 {
-    VirtIORNG *vrng = opaque;
-    VirtIODevice *vdev = VIRTIO_DEVICE(vrng);
-
     if (version_id != 1) {
         return -EINVAL;
     }
-    virtio_load(vdev, f, version_id);
+    return virtio_load(VIRTIO_DEVICE(opaque), f, version_id);
+}
 
+static int virtio_rng_load_device(VirtIODevice *vdev, QEMUFile *f,
+                                  int version_id)
+{
     /* We may have an element ready but couldn't process it due to a quota
      * limit.  Make sure to try again after live migration when the quota may
      * have been reset.
      */
-    virtio_rng_process(vrng);
+    virtio_rng_process(VIRTIO_RNG(vdev));
 
     return 0;
 }
@@ -219,6 +220,7 @@ static void virtio_rng_class_init(ObjectClass *klass, void *data)
     vdc->realize = virtio_rng_device_realize;
     vdc->unrealize = virtio_rng_device_unrealize;
     vdc->get_features = get_features;
+    vdc->load = virtio_rng_load_device;
 }
 
 static void virtio_rng_initfn(Object *obj)

[Qemu-devel] [PATCH RFC 7/8] virtio: add subsections to the migration stream

[Greg Kurz]

There is a need to add some more fields to VirtIODevice that should be
migrated (broken status, endianness). The problem is that we do not
want to break compatibility while adding a new feature... This issue has
been addressed in the generic VMState code with the use of optional
subsections. As a *temporary* alternative to port the whole virtio
migration code to VMState, this patch mimics a similar subsectionning
ability for virtio.

Since each virtio device is streamed in its own section, the idea is to
stream subsections between the end of the device section and the start
of the next sections. This allows an older QEMU to complain and exit
when fed with subsections:

Unknown savevm section type 5
Error -22 while loading VM state

The virtio subsections honor the same protocol than the VMState ones:

 +-------------------------+
 |QEMU_VM_SUBSECTION (byte)| 1 byte
 +-------------------------+
 | subsection name length  | 1 byte
 +-------------------------+
 |                         |
 : subsection name buffer  : length bytes
 |                         |
 +-------------------------+
 |       version_id        | 4 bytes (be32)
 +-------------------------+
 |                         |
 :    subsection data      :
 |                         |
 +-------------------------+

The model differs from VMState though as we don't describe the fields in
the subsections. As a consequence, there is no "field exists" concept and
this impacts backwards migration.

Suggested-by: Alexander Graf <agraf@suse.de>
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/virtio/virtio.c |   78 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 77 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index cf87b44..7fbad29 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -19,6 +19,7 @@
 #include "hw/virtio/virtio.h"
 #include "qemu/atomic.h"
 #include "hw/virtio/virtio-bus.h"
+#include "migration/migration.h"
 
 /*
  * The alignment to use between consumer and producer parts of vring.
@@ -833,6 +834,79 @@ void virtio_notify_config(VirtIODevice *vdev)
     virtio_notify_vector(vdev, vdev->config_vector);
 }
 
+typedef struct VirtIOSubsection {
+    const char *name;
+    int version_id;
+    void (*save)(VirtIODevice *vdev, QEMUFile *f);
+    int (*load)(VirtIODevice *vdev, QEMUFile *f);
+    int (*needed)(VirtIODevice *vdev);
+} VirtIOSubsection;
+
+static const VirtIOSubsection virtio_subsection[] = {
+    { .name = NULL }
+};
+
+static void virtio_save_subsections(VirtIODevice *vdev, QEMUFile *f)
+{
+    int i;
+
+    for (i = 0; virtio_subsection[i].name; i++) {
+        VirtIOSubsection sub = virtio_subsection[i];
+
+        if (sub.needed != NULL && (*sub.needed)(vdev)) {
+            const char *name = sub.name;
+            uint8_t len = strlen(name);
+
+            qemu_put_byte(f, QEMU_VM_SUBSECTION);
+            qemu_put_byte(f, len);
+            qemu_put_buffer(f, (uint8_t *) name, len);
+            qemu_put_be32(f, sub.version_id);
+            (*sub.save)(vdev, f);
+        }
+    }
+}
+
+static int virtio_load_subsections(VirtIODevice *vdev, QEMUFile *f)
+{
+    while (qemu_peek_byte(f, 0) == QEMU_VM_SUBSECTION) {
+        char idstr[256];
+        uint8_t len, size;
+        int i;
+
+        len = qemu_peek_byte(f, 1);
+        size = qemu_peek_buffer(f, (uint8_t *) idstr, len, 2);
+        if (size != len) {
+            break;
+        }
+
+        idstr[size] = 0;
+
+        for (i = 0; virtio_subsection[i].name; i++) {
+            VirtIOSubsection sub = virtio_subsection[i];
+
+            if (strcmp(sub.name, idstr) == 0) {
+                uint32_t version_id;
+                int ret;
+
+                qemu_file_skip(f, 1);   /* subsection */
+                qemu_file_skip(f, 1);   /* len */
+                qemu_file_skip(f, len); /* idstr */
+
+                version_id = qemu_get_be32(f);
+                if (version_id > sub.version_id) {
+                    return -EINVAL;
+                }
+                ret = (*sub.load)(vdev, f);
+                if (ret) {
+                    return ret;
+                }
+            }
+        }
+    }
+
+    return 0;
+}
+
 void virtio_save(VirtIODevice *vdev, QEMUFile *f)
 {
     BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
@@ -876,6 +950,8 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
     if (vdc->save != NULL) {
         vdc->save(vdev, f);
     }
+
+    virtio_save_subsections(vdev, f);
 }
 
 int virtio_set_features(VirtIODevice *vdev, uint32_t val)
@@ -967,7 +1043,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
         return vdc->load(vdev, f, version_id);
     }
 
-    return 0;
+    return virtio_load_subsections(vdev, f);
 }
 
 void virtio_cleanup(VirtIODevice *vdev)

[Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Greg Kurz]

Some CPU families can dynamically change their endianness. This means we
can have little endian ppc or big endian arm guests for example. This has
an impact on legacy virtio data structures since they are target endian.
We hence introduce a new property to track the endianness of each virtio
device. It is reasonnably assumed that endianness won't change while the
device is in use : we hence capture the device endianness when it gets
reset.

Of course this property must be part of the migration stream as most of
the virtio code will depend on it. It is hence migrated in a subsection
to preserve compatibility with older migration streams. The tricky part
is that the endianness gets known quite late and we must ensure no access
is made to virtio data before that. It is for example invalid to call
vring_avail_idx() as does the actual migration code: the VQ indexes sanity
checks had to be moved from virtio_load() to virtio_load_subsections()
because of that.

We enforce some paranoia by making the endianness a 3-value enum so
that we can temporarily poison it while loading state.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 exec.c                     |    8 +---
 hw/virtio/virtio-pci.c     |   11 ++----
 hw/virtio/virtio.c         |   80 +++++++++++++++++++++++++++++++++++++-------
 include/exec/cpu-common.h  |    1 +
 include/hw/virtio/virtio.h |   13 +++++++
 5 files changed, 87 insertions(+), 26 deletions(-)

diff --git a/exec.c b/exec.c
index 91513c6..4e83588 100644
--- a/exec.c
+++ b/exec.c
@@ -2740,13 +2740,7 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
 #endif
 
 #if !defined(CONFIG_USER_ONLY)
-
-/*
- * A helper function for the _utterly broken_ virtio device model to find out if
- * it's running on a big endian machine. Don't do this at home kids!
- */
-bool virtio_is_big_endian(void);
-bool virtio_is_big_endian(void)
+bool target_words_bigendian(void)
 {
 #if defined(TARGET_WORDS_BIGENDIAN)
     return true;
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index ce97514..2ffceb8 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -89,9 +89,6 @@
 /* Flags track per-device state like workarounds for quirks in older guests. */
 #define VIRTIO_PCI_FLAG_BUS_MASTER_BUG  (1 << 0)
 
-/* HACK for virtio to determine if it's running a big endian guest */
-bool virtio_is_big_endian(void);
-
 static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
                                VirtIOPCIProxy *dev);
 
@@ -409,13 +406,13 @@ static uint64_t virtio_pci_config_read(void *opaque, hwaddr addr,
         break;
     case 2:
         val = virtio_config_readw(vdev, addr);
-        if (virtio_is_big_endian()) {
+        if (virtio_is_big_endian(vdev)) {
             val = bswap16(val);
         }
         break;
     case 4:
         val = virtio_config_readl(vdev, addr);
-        if (virtio_is_big_endian()) {
+        if (virtio_is_big_endian(vdev)) {
             val = bswap32(val);
         }
         break;
@@ -443,13 +440,13 @@ static void virtio_pci_config_write(void *opaque, hwaddr addr,
         virtio_config_writeb(vdev, addr, val);
         break;
     case 2:
-        if (virtio_is_big_endian()) {
+        if (virtio_is_big_endian(vdev)) {
             val = bswap16(val);
         }
         virtio_config_writew(vdev, addr, val);
         break;
     case 4:
-        if (virtio_is_big_endian()) {
+        if (virtio_is_big_endian(vdev)) {
             val = bswap32(val);
         }
         virtio_config_writel(vdev, addr, val);
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 7fbad29..6578854 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -539,6 +539,15 @@ void virtio_set_status(VirtIODevice *vdev, uint8_t val)
     vdev->status = val;
 }
 
+static void virtio_set_endian_target_default(VirtIODevice *vdev)
+{
+    if (target_words_bigendian()) {
+        vdev->device_endian = VIRTIO_DEVICE_ENDIAN_BIG;
+    } else {
+        vdev->device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE;
+    }
+}
+
 void virtio_reset(void *opaque)
 {
     VirtIODevice *vdev = opaque;
@@ -546,6 +555,7 @@ void virtio_reset(void *opaque)
     int i;
 
     virtio_set_status(vdev, 0);
+    virtio_set_endian_target_default(vdev);
 
     if (k->reset) {
         k->reset(vdev);
@@ -839,10 +849,39 @@ typedef struct VirtIOSubsection {
     int version_id;
     void (*save)(VirtIODevice *vdev, QEMUFile *f);
     int (*load)(VirtIODevice *vdev, QEMUFile *f);
-    int (*needed)(VirtIODevice *vdev);
+    bool (*needed)(VirtIODevice *vdev);
 } VirtIOSubsection;
 
+static void virtio_save_device_endian(VirtIODevice *vdev, QEMUFile *f)
+{
+    qemu_put_byte(f, vdev->device_endian);
+}
+
+static int virtio_load_device_endian(VirtIODevice *vdev, QEMUFile *f)
+{
+    vdev->device_endian = qemu_get_byte(f);
+    return 0;
+}
+
+static bool virtio_device_endian_needed(VirtIODevice *vdev)
+{
+    /* No migration is supposed to occur while we are loading state.
+     */
+    assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
+    if (target_words_bigendian()) {
+        return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_LITTLE;
+    } else {
+        return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
+    }
+}
+
 static const VirtIOSubsection virtio_subsection[] = {
+    { .name = "virtio/device_endian",
+      .version_id = 1,
+      .save = virtio_save_device_endian,
+      .load = virtio_load_device_endian,
+      .needed = virtio_device_endian_needed,
+    },
     { .name = NULL }
 };
 
@@ -868,6 +907,8 @@ static void virtio_save_subsections(VirtIODevice *vdev, QEMUFile *f)
 
 static int virtio_load_subsections(VirtIODevice *vdev, QEMUFile *f)
 {
+    int i;
+
     while (qemu_peek_byte(f, 0) == QEMU_VM_SUBSECTION) {
         char idstr[256];
         uint8_t len, size;
@@ -904,6 +945,26 @@ static int virtio_load_subsections(VirtIODevice *vdev, QEMUFile *f)
         }
     }
 
+    for (i = 0; i < VIRTIO_PCI_QUEUE_MAX; i++) {
+        if (vdev->vq[i].vring.num == 0) {
+            break;
+        }
+
+        if (vdev->vq[i].pa) {
+            uint16_t nheads;
+            nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
+            /* Check it isn't doing strange things with descriptor numbers. */
+            if (nheads > vdev->vq[i].vring.num) {
+                error_report("VQ %d size 0x%x Guest index 0x%x "
+                             "inconsistent with Host index 0x%x: delta 0x%x",
+                             i, vdev->vq[i].vring.num,
+                             vring_avail_idx(&vdev->vq[i]),
+                             vdev->vq[i].last_avail_idx, nheads);
+                return -1;
+            }
+        }
+    }
+
     return 0;
 }
 
@@ -979,6 +1040,11 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
     VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
 
+    /* We poison the endianness to ensure it does not get used before
+     * subsections have been loaded.
+     */
+    vdev->device_endian = VIRTIO_DEVICE_ENDIAN_UNKNOWN;
+
     if (k->load_config) {
         ret = k->load_config(qbus->parent, f);
         if (ret)
@@ -1012,18 +1078,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
         vdev->vq[i].notification = true;
 
         if (vdev->vq[i].pa) {
-            uint16_t nheads;
             virtqueue_init(&vdev->vq[i]);
-            nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
-            /* Check it isn't doing very strange things with descriptor numbers. */
-            if (nheads > vdev->vq[i].vring.num) {
-                error_report("VQ %d size 0x%x Guest index 0x%x "
-                             "inconsistent with Host index 0x%x: delta 0x%x",
-                             i, vdev->vq[i].vring.num,
-                             vring_avail_idx(&vdev->vq[i]),
-                             vdev->vq[i].last_avail_idx, nheads);
-                return -1;
-            }
         } else if (vdev->vq[i].last_avail_idx) {
             error_report("VQ %d address 0x0 "
                          "inconsistent with Host index 0x%x",
@@ -1100,6 +1155,7 @@ void virtio_init(VirtIODevice *vdev, const char *name,
     }
     vdev->vmstate = qemu_add_vm_change_state_handler(virtio_vmstate_change,
                                                      vdev);
+    virtio_set_endian_target_default(vdev);
 }
 
 hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n)
diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index a21b65a..eb798c1 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -122,4 +122,5 @@ void qemu_ram_foreach_block(RAMBlockIterFunc func, void *opaque);
 
 #endif
 
+bool target_words_bigendian(void);
 #endif /* !CPU_COMMON_H */
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 3505ce5..1c4a736 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -104,6 +104,12 @@ typedef struct VirtQueueElement
 #define VIRTIO_DEVICE(obj) \
         OBJECT_CHECK(VirtIODevice, (obj), TYPE_VIRTIO_DEVICE)
 
+enum virtio_device_endian {
+    VIRTIO_DEVICE_ENDIAN_UNKNOWN,
+    VIRTIO_DEVICE_ENDIAN_LITTLE,
+    VIRTIO_DEVICE_ENDIAN_BIG,
+};
+
 struct VirtIODevice
 {
     DeviceState parent_obj;
@@ -121,6 +127,7 @@ struct VirtIODevice
     bool vm_running;
     VMChangeStateEntry *vmstate;
     char *bus_name;
+    enum virtio_device_endian device_endian;
 };
 
 typedef struct VirtioDeviceClass {
@@ -255,4 +262,10 @@ void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
                                                bool set_handler);
 void virtio_queue_notify_vq(VirtQueue *vq);
 void virtio_irq(VirtQueue *vq);
+
+static inline bool virtio_is_big_endian(VirtIODevice *vdev)
+{
+    assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
+    return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
+}
 #endif

Re: [Qemu-devel] [PATCH RFC V2 0/8] virtio: migrate new properties

[Alexander Graf]

On 19.05.14 10:38, Greg Kurz wrote:
> Hi,
>
> This patch set tries to address comments from the initial
> review. For this round, I have focused on two changes:
> - as suggested by Andreas, we now call the device specific
>    code from the generic code to ease the implementation of
>    future devices. This is achieved with the addition of
>    load/save methods to VirtioDeviceClass.
> - virtio subsections now implement a "needed" concept with
>    the same semantics as in the VMState code.
>
> I haven't looked at compat mode issues yet, but it is
> on my TODO list.

If you fix up the comments to be either

   /*
    * foo
    */

or

   /* foo */

style, not

   /* foo
    */

then you get my


Acked-by: Alexander Graf <agraf@suse.de>


Alex

Re: [Qemu-devel] [PATCH RFC V2 0/8] virtio: migrate new properties

[Greg Kurz]

On Mon, 19 May 2014 14:02:39 +0200
Alexander Graf <agraf@suse.de> wrote:

> 
> On 19.05.14 10:38, Greg Kurz wrote:
> > Hi,
> >
> > This patch set tries to address comments from the initial
> > review. For this round, I have focused on two changes:
> > - as suggested by Andreas, we now call the device specific
> >    code from the generic code to ease the implementation of
> >    future devices. This is achieved with the addition of
> >    load/save methods to VirtioDeviceClass.
> > - virtio subsections now implement a "needed" concept with
> >    the same semantics as in the VMState code.
> >
> > I haven't looked at compat mode issues yet, but it is
> > on my TODO list.
> 
> If you fix up the comments to be either
> 
>    /*
>     * foo
>     */
> 
> or
> 
>    /* foo */
> 
> style, not
> 
>    /* foo
>     */
> 
> then you get my
> 
> 
> Acked-by: Alexander Graf <agraf@suse.de>
> 
> 
> Alex
> 

I'll certainly do that then ! :)

BTW, the faulty comments are in patch 8/8 that I was
sending for informational purpose only as it is part
of the bi-endian virtio serie. Should I merge the
two patch sets at some point ?

-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.

Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Greg Kurz]

On Mon, 19 May 2014 10:39:09 +0200
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> Some CPU families can dynamically change their endianness. This means we
> can have little endian ppc or big endian arm guests for example. This has
> an impact on legacy virtio data structures since they are target endian.
> We hence introduce a new property to track the endianness of each virtio
> device. It is reasonnably assumed that endianness won't change while the
> device is in use : we hence capture the device endianness when it gets
> reset.
> 
> Of course this property must be part of the migration stream as most of
> the virtio code will depend on it. It is hence migrated in a subsection
> to preserve compatibility with older migration streams. The tricky part
> is that the endianness gets known quite late and we must ensure no access
> is made to virtio data before that. It is for example invalid to call
> vring_avail_idx() as does the actual migration code: the VQ indexes sanity
> checks had to be moved from virtio_load() to virtio_load_subsections()
> because of that.
> 
> We enforce some paranoia by making the endianness a 3-value enum so
> that we can temporarily poison it while loading state.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  exec.c                     |    8 +---
>  hw/virtio/virtio-pci.c     |   11 ++----
>  hw/virtio/virtio.c         |   80 +++++++++++++++++++++++++++++++++++++-------
>  include/exec/cpu-common.h  |    1 +
>  include/hw/virtio/virtio.h |   13 +++++++
>  5 files changed, 87 insertions(+), 26 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 91513c6..4e83588 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2740,13 +2740,7 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
>  #endif
> 
>  #if !defined(CONFIG_USER_ONLY)
> -
> -/*
> - * A helper function for the _utterly broken_ virtio device model to find out if
> - * it's running on a big endian machine. Don't do this at home kids!
> - */
> -bool virtio_is_big_endian(void);
> -bool virtio_is_big_endian(void)
> +bool target_words_bigendian(void)
>  {
>  #if defined(TARGET_WORDS_BIGENDIAN)
>      return true;
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index ce97514..2ffceb8 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -89,9 +89,6 @@
>  /* Flags track per-device state like workarounds for quirks in older guests. */
>  #define VIRTIO_PCI_FLAG_BUS_MASTER_BUG  (1 << 0)
> 
> -/* HACK for virtio to determine if it's running a big endian guest */
> -bool virtio_is_big_endian(void);
> -
>  static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
>                                 VirtIOPCIProxy *dev);
> 
> @@ -409,13 +406,13 @@ static uint64_t virtio_pci_config_read(void *opaque, hwaddr addr,
>          break;
>      case 2:
>          val = virtio_config_readw(vdev, addr);
> -        if (virtio_is_big_endian()) {
> +        if (virtio_is_big_endian(vdev)) {
>              val = bswap16(val);
>          }
>          break;
>      case 4:
>          val = virtio_config_readl(vdev, addr);
> -        if (virtio_is_big_endian()) {
> +        if (virtio_is_big_endian(vdev)) {
>              val = bswap32(val);
>          }
>          break;
> @@ -443,13 +440,13 @@ static void virtio_pci_config_write(void *opaque, hwaddr addr,
>          virtio_config_writeb(vdev, addr, val);
>          break;
>      case 2:
> -        if (virtio_is_big_endian()) {
> +        if (virtio_is_big_endian(vdev)) {
>              val = bswap16(val);
>          }
>          virtio_config_writew(vdev, addr, val);
>          break;
>      case 4:
> -        if (virtio_is_big_endian()) {
> +        if (virtio_is_big_endian(vdev)) {
>              val = bswap32(val);
>          }
>          virtio_config_writel(vdev, addr, val);
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 7fbad29..6578854 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -539,6 +539,15 @@ void virtio_set_status(VirtIODevice *vdev, uint8_t val)
>      vdev->status = val;
>  }
> 
> +static void virtio_set_endian_target_default(VirtIODevice *vdev)
> +{
> +    if (target_words_bigendian()) {
> +        vdev->device_endian = VIRTIO_DEVICE_ENDIAN_BIG;
> +    } else {
> +        vdev->device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE;
> +    }
> +}
> +
>  void virtio_reset(void *opaque)
>  {
>      VirtIODevice *vdev = opaque;
> @@ -546,6 +555,7 @@ void virtio_reset(void *opaque)
>      int i;
> 
>      virtio_set_status(vdev, 0);
> +    virtio_set_endian_target_default(vdev);
> 
>      if (k->reset) {
>          k->reset(vdev);
> @@ -839,10 +849,39 @@ typedef struct VirtIOSubsection {
>      int version_id;
>      void (*save)(VirtIODevice *vdev, QEMUFile *f);
>      int (*load)(VirtIODevice *vdev, QEMUFile *f);
> -    int (*needed)(VirtIODevice *vdev);
> +    bool (*needed)(VirtIODevice *vdev);
>  } VirtIOSubsection;
> 
> +static void virtio_save_device_endian(VirtIODevice *vdev, QEMUFile *f)
> +{
> +    qemu_put_byte(f, vdev->device_endian);
> +}
> +
> +static int virtio_load_device_endian(VirtIODevice *vdev, QEMUFile *f)
> +{
> +    vdev->device_endian = qemu_get_byte(f);
> +    return 0;
> +}
> +
> +static bool virtio_device_endian_needed(VirtIODevice *vdev)
> +{
> +    /* No migration is supposed to occur while we are loading state.
> +     */
> +    assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
> +    if (target_words_bigendian()) {
> +        return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_LITTLE;
> +    } else {
> +        return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
> +    }
> +}
> +
>  static const VirtIOSubsection virtio_subsection[] = {
> +    { .name = "virtio/device_endian",

Can anyone comment the subsection name ? Is there a chance the
VMState port would come up with the same ?

> +      .version_id = 1,
> +      .save = virtio_save_device_endian,
> +      .load = virtio_load_device_endian,
> +      .needed = virtio_device_endian_needed,
> +    },
>      { .name = NULL }
>  };
> 
> @@ -868,6 +907,8 @@ static void virtio_save_subsections(VirtIODevice *vdev, QEMUFile *f)
> 
>  static int virtio_load_subsections(VirtIODevice *vdev, QEMUFile *f)
>  {
> +    int i;
> +
>      while (qemu_peek_byte(f, 0) == QEMU_VM_SUBSECTION) {
>          char idstr[256];
>          uint8_t len, size;
> @@ -904,6 +945,26 @@ static int virtio_load_subsections(VirtIODevice *vdev, QEMUFile *f)
>          }
>      }
> 
> +    for (i = 0; i < VIRTIO_PCI_QUEUE_MAX; i++) {
> +        if (vdev->vq[i].vring.num == 0) {
> +            break;
> +        }
> +
> +        if (vdev->vq[i].pa) {
> +            uint16_t nheads;
> +            nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
> +            /* Check it isn't doing strange things with descriptor numbers. */
> +            if (nheads > vdev->vq[i].vring.num) {
> +                error_report("VQ %d size 0x%x Guest index 0x%x "
> +                             "inconsistent with Host index 0x%x: delta 0x%x",
> +                             i, vdev->vq[i].vring.num,
> +                             vring_avail_idx(&vdev->vq[i]),
> +                             vdev->vq[i].last_avail_idx, nheads);
> +                return -1;
> +            }
> +        }
> +    }
> +
>      return 0;
>  }
> 
> @@ -979,6 +1040,11 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>      VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
> 
> +    /* We poison the endianness to ensure it does not get used before
> +     * subsections have been loaded.
> +     */
> +    vdev->device_endian = VIRTIO_DEVICE_ENDIAN_UNKNOWN;
> +
>      if (k->load_config) {
>          ret = k->load_config(qbus->parent, f);
>          if (ret)
> @@ -1012,18 +1078,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
>          vdev->vq[i].notification = true;
> 
>          if (vdev->vq[i].pa) {
> -            uint16_t nheads;
>              virtqueue_init(&vdev->vq[i]);
> -            nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
> -            /* Check it isn't doing very strange things with descriptor numbers. */
> -            if (nheads > vdev->vq[i].vring.num) {
> -                error_report("VQ %d size 0x%x Guest index 0x%x "
> -                             "inconsistent with Host index 0x%x: delta 0x%x",
> -                             i, vdev->vq[i].vring.num,
> -                             vring_avail_idx(&vdev->vq[i]),
> -                             vdev->vq[i].last_avail_idx, nheads);
> -                return -1;
> -            }
>          } else if (vdev->vq[i].last_avail_idx) {
>              error_report("VQ %d address 0x0 "
>                           "inconsistent with Host index 0x%x",
> @@ -1100,6 +1155,7 @@ void virtio_init(VirtIODevice *vdev, const char *name,
>      }
>      vdev->vmstate = qemu_add_vm_change_state_handler(virtio_vmstate_change,
>                                                       vdev);
> +    virtio_set_endian_target_default(vdev);
>  }
> 
>  hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n)
> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index a21b65a..eb798c1 100644
> --- a/include/exec/cpu-common.h
> +++ b/include/exec/cpu-common.h
> @@ -122,4 +122,5 @@ void qemu_ram_foreach_block(RAMBlockIterFunc func, void *opaque);
> 
>  #endif
> 
> +bool target_words_bigendian(void);
>  #endif /* !CPU_COMMON_H */
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 3505ce5..1c4a736 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -104,6 +104,12 @@ typedef struct VirtQueueElement
>  #define VIRTIO_DEVICE(obj) \
>          OBJECT_CHECK(VirtIODevice, (obj), TYPE_VIRTIO_DEVICE)
> 
> +enum virtio_device_endian {
> +    VIRTIO_DEVICE_ENDIAN_UNKNOWN,
> +    VIRTIO_DEVICE_ENDIAN_LITTLE,
> +    VIRTIO_DEVICE_ENDIAN_BIG,
> +};
> +
>  struct VirtIODevice
>  {
>      DeviceState parent_obj;
> @@ -121,6 +127,7 @@ struct VirtIODevice
>      bool vm_running;
>      VMChangeStateEntry *vmstate;
>      char *bus_name;
> +    enum virtio_device_endian device_endian;
>  };
> 
>  typedef struct VirtioDeviceClass {
> @@ -255,4 +262,10 @@ void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
>                                                 bool set_handler);
>  void virtio_queue_notify_vq(VirtQueue *vq);
>  void virtio_irq(VirtQueue *vq);
> +
> +static inline bool virtio_is_big_endian(VirtIODevice *vdev)
> +{
> +    assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
> +    return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
> +}
>  #endif
> 
> 



-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.

Re: [Qemu-devel] [PATCH RFC V2 0/8] virtio: migrate new properties

[Alexander Graf]

On 19.05.14 14:45, Greg Kurz wrote:
> On Mon, 19 May 2014 14:02:39 +0200
> Alexander Graf <agraf@suse.de> wrote:
>
>> On 19.05.14 10:38, Greg Kurz wrote:
>>> Hi,
>>>
>>> This patch set tries to address comments from the initial
>>> review. For this round, I have focused on two changes:
>>> - as suggested by Andreas, we now call the device specific
>>>     code from the generic code to ease the implementation of
>>>     future devices. This is achieved with the addition of
>>>     load/save methods to VirtioDeviceClass.
>>> - virtio subsections now implement a "needed" concept with
>>>     the same semantics as in the VMState code.
>>>
>>> I haven't looked at compat mode issues yet, but it is
>>> on my TODO list.
>> If you fix up the comments to be either
>>
>>     /*
>>      * foo
>>      */
>>
>> or
>>
>>     /* foo */
>>
>> style, not
>>
>>     /* foo
>>      */
>>
>> then you get my
>>
>>
>> Acked-by: Alexander Graf <agraf@suse.de>
>>
>>
>> Alex
>>
> I'll certainly do that then ! :)
>
> BTW, the faulty comments are in patch 8/8 that I was
> sending for informational purpose only as it is part
> of the bi-endian virtio serie. Should I merge the
> two patch sets at some point ?


Either way works for me, let's wait for virtio and migration specialists 
to comment first ;)


Alex

Re: [Qemu-devel] [PATCH RFC V2 0/8] virtio: migrate new properties

[Michael S. Tsirkin]

On Mon, May 19, 2014 at 02:02:39PM +0200, Alexander Graf wrote:
> 
> On 19.05.14 10:38, Greg Kurz wrote:
> >Hi,
> >
> >This patch set tries to address comments from the initial
> >review. For this round, I have focused on two changes:
> >- as suggested by Andreas, we now call the device specific
> >   code from the generic code to ease the implementation of
> >   future devices. This is achieved with the addition of
> >   load/save methods to VirtioDeviceClass.
> >- virtio subsections now implement a "needed" concept with
> >   the same semantics as in the VMState code.
> >
> >I haven't looked at compat mode issues yet, but it is
> >on my TODO list.
> 
> If you fix up the comments to be either
> 
>   /*
>    * foo
>    */
> 
> or
> 
>   /* foo */
> 
> style, not
> 
>   /* foo
>    */
> 
> then you get my
> 
> 
> Acked-by: Alexander Graf <agraf@suse.de>
> 
> 
> Alex

Documented anywhere?
Linux style is

        /* Always
         * like this.
         */

so it's definitely not universal.


-- 
MST

Re: [Qemu-devel] [PATCH RFC V2 0/8] virtio: migrate new properties

[Alexander Graf]

On 19.05.14 17:10, Michael S. Tsirkin wrote:
> On Mon, May 19, 2014 at 02:02:39PM +0200, Alexander Graf wrote:
>> On 19.05.14 10:38, Greg Kurz wrote:
>>> Hi,
>>>
>>> This patch set tries to address comments from the initial
>>> review. For this round, I have focused on two changes:
>>> - as suggested by Andreas, we now call the device specific
>>>    code from the generic code to ease the implementation of
>>>    future devices. This is achieved with the addition of
>>>    load/save methods to VirtioDeviceClass.
>>> - virtio subsections now implement a "needed" concept with
>>>    the same semantics as in the VMState code.
>>>
>>> I haven't looked at compat mode issues yet, but it is
>>> on my TODO list.
>> If you fix up the comments to be either
>>
>>    /*
>>     * foo
>>     */
>>
>> or
>>
>>    /* foo */
>>
>> style, not
>>
>>    /* foo
>>     */
>>
>> then you get my
>>
>>
>> Acked-by: Alexander Graf <agraf@suse.de>
>>
>>
>> Alex
> Documented anywhere?

Impressive. I thought it was, but apparently I'm wrong :). Nevermind then.

> Linux style is
>
>          /* Always
>           * like this.
>           */
>
> so it's definitely not universal.

Eh - documented anywhere? Linux usually goes with C style comments.


Alex

Re: [Qemu-devel] [PATCH RFC V2 0/8] virtio: migrate new properties

[Andreas Färber]

Am 19.05.2014 17:10, schrieb Michael S. Tsirkin:
> On Mon, May 19, 2014 at 02:02:39PM +0200, Alexander Graf wrote:
>>
>> On 19.05.14 10:38, Greg Kurz wrote:
>>> Hi,
>>>
>>> This patch set tries to address comments from the initial
>>> review. For this round, I have focused on two changes:
>>> - as suggested by Andreas, we now call the device specific
>>>   code from the generic code to ease the implementation of
>>>   future devices. This is achieved with the addition of
>>>   load/save methods to VirtioDeviceClass.
>>> - virtio subsections now implement a "needed" concept with
>>>   the same semantics as in the VMState code.
>>>
>>> I haven't looked at compat mode issues yet, but it is
>>> on my TODO list.
>>
>> If you fix up the comments to be either
>>
>>   /*
>>    * foo
>>    */
>>
>> or
>>
>>   /* foo */
>>
>> style, not
>>
>>   /* foo
>>    */
>>
>> then you get my
>>
>>
>> Acked-by: Alexander Graf <agraf@suse.de>
>>
>>
>> Alex
> 
> Documented anywhere?
> Linux style is
> 
>         /* Always
>          * like this.
>          */
> 
> so it's definitely not universal.

I could've sworn that you (mst) asked at least two people to change
comments from

/* foo
   bar */

to

/*
 * foo
 */

style...

Andreas

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

Re: [Qemu-devel] [PATCH RFC V2 0/8] virtio: migrate new properties

[Michael S. Tsirkin]

On Mon, May 19, 2014 at 05:36:38PM +0200, Alexander Graf wrote:
> 
> On 19.05.14 17:10, Michael S. Tsirkin wrote:
> >On Mon, May 19, 2014 at 02:02:39PM +0200, Alexander Graf wrote:
> >>On 19.05.14 10:38, Greg Kurz wrote:
> >>>Hi,
> >>>
> >>>This patch set tries to address comments from the initial
> >>>review. For this round, I have focused on two changes:
> >>>- as suggested by Andreas, we now call the device specific
> >>>   code from the generic code to ease the implementation of
> >>>   future devices. This is achieved with the addition of
> >>>   load/save methods to VirtioDeviceClass.
> >>>- virtio subsections now implement a "needed" concept with
> >>>   the same semantics as in the VMState code.
> >>>
> >>>I haven't looked at compat mode issues yet, but it is
> >>>on my TODO list.
> >>If you fix up the comments to be either
> >>
> >>   /*
> >>    * foo
> >>    */
> >>
> >>or
> >>
> >>   /* foo */
> >>
> >>style, not
> >>
> >>   /* foo
> >>    */
> >>
> >>then you get my
> >>
> >>
> >>Acked-by: Alexander Graf <agraf@suse.de>
> >>
> >>
> >>Alex
> >Documented anywhere?
> 
> Impressive. I thought it was, but apparently I'm wrong :). Nevermind then.
> 
> >Linux style is
> >
> >         /* Always
> >          * like this.
> >          */
> >
> >so it's definitely not universal.
> 
> Eh - documented anywhere? Linux usually goes with C style comments.
> 
> 
> Alex

Actually, what I said is only true for networking, I'm
just hacking in that field recently.
It's all in Documentation/CodingStyle:


The preferred style for long (multi-line) comments is:

        /*
         * This is the preferred style for multi-line
         * comments in the Linux kernel source code.
         * Please use it consistently.
         *
         * Description:  A column of asterisks on the left side,
         * with beginning and ending almost-blank lines.
         */

For files in net/ and drivers/net/ the preferred style for long (multi-line)
comments is a little different.

        /* The preferred comment style for files in net/ and drivers/net
         * looks like this.
         *
         * It is nearly the same as the generally preferred comment style,
         * but there is no initial almost-blank line.
         */

Re: [Qemu-devel] [PATCH RFC V2 0/8] virtio: migrate new properties

[Michael S. Tsirkin]

On Mon, May 19, 2014 at 05:40:36PM +0200, Andreas Färber wrote:
> Am 19.05.2014 17:10, schrieb Michael S. Tsirkin:
> > On Mon, May 19, 2014 at 02:02:39PM +0200, Alexander Graf wrote:
> >>
> >> On 19.05.14 10:38, Greg Kurz wrote:
> >>> Hi,
> >>>
> >>> This patch set tries to address comments from the initial
> >>> review. For this round, I have focused on two changes:
> >>> - as suggested by Andreas, we now call the device specific
> >>>   code from the generic code to ease the implementation of
> >>>   future devices. This is achieved with the addition of
> >>>   load/save methods to VirtioDeviceClass.
> >>> - virtio subsections now implement a "needed" concept with
> >>>   the same semantics as in the VMState code.
> >>>
> >>> I haven't looked at compat mode issues yet, but it is
> >>> on my TODO list.
> >>
> >> If you fix up the comments to be either
> >>
> >>   /*
> >>    * foo
> >>    */
> >>
> >> or
> >>
> >>   /* foo */
> >>
> >> style, not
> >>
> >>   /* foo
> >>    */
> >>
> >> then you get my
> >>
> >>
> >> Acked-by: Alexander Graf <agraf@suse.de>
> >>
> >>
> >> Alex
> > 
> > Documented anywhere?
> > Linux style is
> > 
> >         /* Always
> >          * like this.
> >          */
> > 
> > so it's definitely not universal.
> 
> I could've sworn that you (mst) asked at least two people to change
> comments from
> 
> /* foo
>    bar */
> 
> to
> 
> /*
>  * foo
>  */
> 
> style...
> 
> Andreas

IMHO it's the last */ on the same line as text that looks ugly.
 /*
  * foo
  */

/* foo */

/* foo
 */

all look ok I think.


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

Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Andreas Färber]

Am 19.05.2014 15:06, schrieb Greg Kurz:
> On Mon, 19 May 2014 10:39:09 +0200
> Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 7fbad29..6578854 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
[...]
>> @@ -839,10 +849,39 @@ typedef struct VirtIOSubsection {
>>      int version_id;
>>      void (*save)(VirtIODevice *vdev, QEMUFile *f);
>>      int (*load)(VirtIODevice *vdev, QEMUFile *f);
>> -    int (*needed)(VirtIODevice *vdev);
>> +    bool (*needed)(VirtIODevice *vdev);
>>  } VirtIOSubsection;
>>
>> +static void virtio_save_device_endian(VirtIODevice *vdev, QEMUFile *f)
>> +{
>> +    qemu_put_byte(f, vdev->device_endian);
>> +}
>> +
>> +static int virtio_load_device_endian(VirtIODevice *vdev, QEMUFile *f)
>> +{
>> +    vdev->device_endian = qemu_get_byte(f);
>> +    return 0;
>> +}
>> +
>> +static bool virtio_device_endian_needed(VirtIODevice *vdev)
>> +{
>> +    /* No migration is supposed to occur while we are loading state.
>> +     */
>> +    assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
>> +    if (target_words_bigendian()) {
>> +        return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_LITTLE;
>> +    } else {
>> +        return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
>> +    }
>> +}
>> +
>>  static const VirtIOSubsection virtio_subsection[] = {
>> +    { .name = "virtio/device_endian",
> 
> Can anyone comment the subsection name ? Is there a chance the
> VMState port would come up with the same ?
> 
>> +      .version_id = 1,
>> +      .save = virtio_save_device_endian,
>> +      .load = virtio_load_device_endian,
>> +      .needed = virtio_device_endian_needed,
>> +    },
>>      { .name = NULL }
>>  };
>>

Different question: With converting VirtIO to VMState in mind, why are
you not using a regular VMStateSubsection and loading/saving that as
part of the old-style load/save functions? Is an API for that missing?

Regards,
Andreas

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

Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Greg Kurz]

On Mon, 19 May 2014 19:06:39 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Am 19.05.2014 15:06, schrieb Greg Kurz:
> > On Mon, 19 May 2014 10:39:09 +0200
> > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >> index 7fbad29..6578854 100644
> >> --- a/hw/virtio/virtio.c
> >> +++ b/hw/virtio/virtio.c
> [...]
> >> @@ -839,10 +849,39 @@ typedef struct VirtIOSubsection {
> >>      int version_id;
> >>      void (*save)(VirtIODevice *vdev, QEMUFile *f);
> >>      int (*load)(VirtIODevice *vdev, QEMUFile *f);
> >> -    int (*needed)(VirtIODevice *vdev);
> >> +    bool (*needed)(VirtIODevice *vdev);
> >>  } VirtIOSubsection;
> >>
> >> +static void virtio_save_device_endian(VirtIODevice *vdev, QEMUFile *f)
> >> +{
> >> +    qemu_put_byte(f, vdev->device_endian);
> >> +}
> >> +
> >> +static int virtio_load_device_endian(VirtIODevice *vdev, QEMUFile *f)
> >> +{
> >> +    vdev->device_endian = qemu_get_byte(f);
> >> +    return 0;
> >> +}
> >> +
> >> +static bool virtio_device_endian_needed(VirtIODevice *vdev)
> >> +{
> >> +    /* No migration is supposed to occur while we are loading state.
> >> +     */
> >> +    assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
> >> +    if (target_words_bigendian()) {
> >> +        return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_LITTLE;
> >> +    } else {
> >> +        return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
> >> +    }
> >> +}
> >> +
> >>  static const VirtIOSubsection virtio_subsection[] = {
> >> +    { .name = "virtio/device_endian",
> > 
> > Can anyone comment the subsection name ? Is there a chance the
> > VMState port would come up with the same ?
> > 
> >> +      .version_id = 1,
> >> +      .save = virtio_save_device_endian,
> >> +      .load = virtio_load_device_endian,
> >> +      .needed = virtio_device_endian_needed,
> >> +    },
> >>      { .name = NULL }
> >>  };
> >>
> 
> Different question: With converting VirtIO to VMState in mind, why are
> you not using a regular VMStateSubsection and loading/saving that as
> part of the old-style load/save functions? Is an API for that missing?
> 

I guess because I haven't tried yet. :)
I'll have a closer look at this.

> Regards,
> Andreas
> 

Thanks.

-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.

Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Dr. David Alan Gilbert]

* Andreas F?rber (afaerber@suse.de) wrote:
> Am 19.05.2014 15:06, schrieb Greg Kurz:
> > On Mon, 19 May 2014 10:39:09 +0200
> > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >> index 7fbad29..6578854 100644
> >> --- a/hw/virtio/virtio.c
> >> +++ b/hw/virtio/virtio.c
> [...]
> >> @@ -839,10 +849,39 @@ typedef struct VirtIOSubsection {
> >>      int version_id;
> >>      void (*save)(VirtIODevice *vdev, QEMUFile *f);
> >>      int (*load)(VirtIODevice *vdev, QEMUFile *f);
> >> -    int (*needed)(VirtIODevice *vdev);
> >> +    bool (*needed)(VirtIODevice *vdev);
> >>  } VirtIOSubsection;
> >>
> >> +static void virtio_save_device_endian(VirtIODevice *vdev, QEMUFile *f)
> >> +{
> >> +    qemu_put_byte(f, vdev->device_endian);
> >> +}
> >> +
> >> +static int virtio_load_device_endian(VirtIODevice *vdev, QEMUFile *f)
> >> +{
> >> +    vdev->device_endian = qemu_get_byte(f);
> >> +    return 0;
> >> +}
> >> +
> >> +static bool virtio_device_endian_needed(VirtIODevice *vdev)
> >> +{
> >> +    /* No migration is supposed to occur while we are loading state.
> >> +     */
> >> +    assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
> >> +    if (target_words_bigendian()) {
> >> +        return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_LITTLE;
> >> +    } else {
> >> +        return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
> >> +    }
> >> +}
> >> +
> >>  static const VirtIOSubsection virtio_subsection[] = {
> >> +    { .name = "virtio/device_endian",
> > 
> > Can anyone comment the subsection name ? Is there a chance the
> > VMState port would come up with the same ?
> > 
> >> +      .version_id = 1,
> >> +      .save = virtio_save_device_endian,
> >> +      .load = virtio_load_device_endian,
> >> +      .needed = virtio_device_endian_needed,
> >> +    },
> >>      { .name = NULL }
> >>  };
> >>
> 
> Different question: With converting VirtIO to VMState in mind, why are
> you not using a regular VMStateSubsection and loading/saving that as
> part of the old-style load/save functions? Is an API for that missing?

There are a handful of places that call into vmstate from a non-vmstate
routine but I don't think they're using plain subsections.

hw/pci/pci.c: pci_device_save/load
hw/scsi/spapr_vscsi.c: vscsi_save_request
hw/acpi/piix4.c: acpi_load_old

Dave
> 
> Regards,
> Andreas
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Greg Kurz]

On Thu, 29 May 2014 12:16:26 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 29/05/2014 11:12, Greg Kurz ha scritto:
> > int virtio_load(VirtIODevice *vdev, QEMUFile *f)
> > {
> > [...]
> >             nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
> >                        ^^^^^^^^^^^
> >             /* Check it isn't doing very strange things with descriptor numbers. */
> >             if (nheads > vdev->vq[i].vring.num) {
> > [...]
> > }
> >
> > and
> >
> > static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
> > {
> > [...]
> >     /* The config space */
> >     qemu_get_be16s(f, &s->config.cols);
> >     qemu_get_be16s(f, &s->config.rows);
> >
> >     qemu_get_be32s(f, &max_nr_ports);
> >     tswap32s(&max_nr_ports);
> >      ^^^^^^
> >     if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
> > [...]
> > }
> >
> > If we stream subsections after the device descriptor as it is done
> > in VMState, these two will break because the device endian is stale.
> >
> > The first one can be easily dealt with: just defer the sanity check
> > to a post_load function.
> 
> Good, we're lucky here.
> 
> > The second is a bit more tricky: the
> > virtio serial migrates its config space (target endian) and the
> > active ports bitmap. The load code assumes max_nr_ports from the
> > config space tells the size of the ports bitmap... that means the
> > virtio migration protocol is also contaminated by target endianness. :-\
> 
> Ouch.
> 
> I guess we could break migration in the case of host endianness != 
> target endianness, like this:
> 
>      /* These three used to be fetched in target endianness and then
>       * stored as big endian.  It ended up as little endian if host and
>       * target endianness doesn't match.
>       *
>       * Starting with qemu 2.1, we always store as big endian.  The
>       * version wasn't bumped to avoid breaking backwards compatibility.
>       * We check the validity of max_nr_ports, and the incorrect-
>       * endianness max_nr_ports will be huge, which will abort migration
>       * anyway.
>       */
>      uint16_t cols = tswap16(s->config.cols);
>      uint16_t rows = tswap16(s->config.rows);
>      uint32_t max_nr_ports = tswap32(s->config.max_nr_ports);
> 
>      qemu_put_be16s(f, &cols);
>      qemu_put_be16s(f, &rows);
>      qemu_put_be32s(f, &max_nr_ports);
> 
> ...
> 
>      uint16_t cols, rows;
> 
>      qemu_get_be16s(f, &cols);
>      qemu_get_be16s(f, &rows);
>      qemu_get_be32s(f, &max_nr_ports);
> 
>      /* Convert back to target endianness when storing into the config
>       * space.
>       */

Paolo,

The patch set to support endian changing targets adds a device_endian
field to the VirtIODevice structure to be used instead of the default
target endianness as it happens with tswap() macros. It also introduces
virtio_tswap() helpers for this purpose, but they can only be used when
the device_endian field has been restored... in a subsection after the
device descriptor... :-\
If the scenario is ppc64le-on-ppc64: tswap() macros don't do anything
and we cannot convert back to LE...

>      s->config.cols = tswap16(cols);
>      s->config.rows = tswap16(rows);

Since cols and rows are not involved in the protocol, we can safely
defer the conversion to post load.

>      if (max_nr_ports > tswap32(s->config.max_nr_ports) {
>          ...
>      }
> 

Since we know that 0 < max_nr_ports < 32,  is it acceptable to guess
the correct endianness with a heuristic ?

if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
	max_nr_ports = bswap32(max_nr_ports);
}

if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
	return -EINVAL;
}

> > In the case the answer for above is "legacy virtio really sucks" then,
> > is it acceptable to not honor bug-compatibility with older versions and
> > fix the code ? :)
> 
> As long as the common cases don't break, yes.  The question is what are 
> the common cases.  Here I think the only non-obscure case that could 
> break is x86-on-PPC, and it's not that common.
> 
> Paolo
> 

Thanks.

-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.

Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Michael S. Tsirkin]

On Thu, Jun 12, 2014 at 09:43:51AM +0200, Greg Kurz wrote:
> On Thu, 29 May 2014 12:16:26 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> > Il 29/05/2014 11:12, Greg Kurz ha scritto:
> > > int virtio_load(VirtIODevice *vdev, QEMUFile *f)
> > > {
> > > [...]
> > >             nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
> > >                        ^^^^^^^^^^^
> > >             /* Check it isn't doing very strange things with descriptor numbers. */
> > >             if (nheads > vdev->vq[i].vring.num) {
> > > [...]
> > > }
> > >
> > > and
> > >
> > > static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
> > > {
> > > [...]
> > >     /* The config space */
> > >     qemu_get_be16s(f, &s->config.cols);
> > >     qemu_get_be16s(f, &s->config.rows);
> > >
> > >     qemu_get_be32s(f, &max_nr_ports);
> > >     tswap32s(&max_nr_ports);
> > >      ^^^^^^
> > >     if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
> > > [...]
> > > }
> > >
> > > If we stream subsections after the device descriptor as it is done
> > > in VMState, these two will break because the device endian is stale.
> > >
> > > The first one can be easily dealt with: just defer the sanity check
> > > to a post_load function.
> > 
> > Good, we're lucky here.
> > 
> > > The second is a bit more tricky: the
> > > virtio serial migrates its config space (target endian) and the
> > > active ports bitmap. The load code assumes max_nr_ports from the
> > > config space tells the size of the ports bitmap... that means the
> > > virtio migration protocol is also contaminated by target endianness. :-\
> > 
> > Ouch.
> > 
> > I guess we could break migration in the case of host endianness != 
> > target endianness, like this:
> > 
> >      /* These three used to be fetched in target endianness and then
> >       * stored as big endian.  It ended up as little endian if host and
> >       * target endianness doesn't match.
> >       *
> >       * Starting with qemu 2.1, we always store as big endian.  The
> >       * version wasn't bumped to avoid breaking backwards compatibility.
> >       * We check the validity of max_nr_ports, and the incorrect-
> >       * endianness max_nr_ports will be huge, which will abort migration
> >       * anyway.
> >       */
> >      uint16_t cols = tswap16(s->config.cols);
> >      uint16_t rows = tswap16(s->config.rows);
> >      uint32_t max_nr_ports = tswap32(s->config.max_nr_ports);
> > 
> >      qemu_put_be16s(f, &cols);
> >      qemu_put_be16s(f, &rows);
> >      qemu_put_be32s(f, &max_nr_ports);
> > 
> > ...
> > 
> >      uint16_t cols, rows;
> > 
> >      qemu_get_be16s(f, &cols);
> >      qemu_get_be16s(f, &rows);
> >      qemu_get_be32s(f, &max_nr_ports);
> > 
> >      /* Convert back to target endianness when storing into the config
> >       * space.
> >       */
> 
> Paolo,
> 
> The patch set to support endian changing targets adds a device_endian
> field to the VirtIODevice structure to be used instead of the default
> target endianness as it happens with tswap() macros. It also introduces
> virtio_tswap() helpers for this purpose, but they can only be used when
> the device_endian field has been restored... in a subsection after the
> device descriptor... :-\

Store it earlier then, using plain put/get.
You can still add a section conditionally to cause
a cleaner failure in broken cross-version scenarios.

> If the scenario is ppc64le-on-ppc64: tswap() macros don't do anything
> and we cannot convert back to LE...
> 
> >      s->config.cols = tswap16(cols);
> >      s->config.rows = tswap16(rows);
> 
> Since cols and rows are not involved in the protocol, we can safely
> defer the conversion to post load.
> 
> >      if (max_nr_ports > tswap32(s->config.max_nr_ports) {
> >          ...
> >      }
> > 
> 
> Since we know that 0 < max_nr_ports < 32,  is it acceptable to guess
> the correct endianness with a heuristic ?
> 
> if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
> 	max_nr_ports = bswap32(max_nr_ports);
> }
> 
> if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
> 	return -EINVAL;
> }
> 
> > > In the case the answer for above is "legacy virtio really sucks" then,
> > > is it acceptable to not honor bug-compatibility with older versions and
> > > fix the code ? :)
> > 
> > As long as the common cases don't break, yes.  The question is what are 
> > the common cases.  Here I think the only non-obscure case that could 
> > break is x86-on-PPC, and it's not that common.
> > 
> > Paolo
> > 
> 
> Thanks.

One starts doubting whether all this hackery is worth it.  virtio 1.0
should be out real soon now, it makes everything LE so the problem goes
away. It's not like PPC LE is so popular that we must support old drivers
at all costs.  Won't time be better spent backporting virtio 1.0 drivers?


> -- 
> Gregory Kurz                                     kurzgreg@fr.ibm.com
>                                                  gkurz@linux.vnet.ibm.com
> Software Engineer @ IBM/Meiosys                  http://www.ibm.com
> Tel +33 (0)562 165 496
> 
> "Anarchy is about taking complete responsibility for yourself."
>         Alan Moore.

Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Greg Kurz]

On Thu, 12 Jun 2014 10:54:48 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Jun 12, 2014 at 09:43:51AM +0200, Greg Kurz wrote:
> > On Thu, 29 May 2014 12:16:26 +0200
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > > Il 29/05/2014 11:12, Greg Kurz ha scritto:
> > > > int virtio_load(VirtIODevice *vdev, QEMUFile *f)
> > > > {
> > > > [...]
> > > >             nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
> > > >                        ^^^^^^^^^^^
> > > >             /* Check it isn't doing very strange things with descriptor numbers. */
> > > >             if (nheads > vdev->vq[i].vring.num) {
> > > > [...]
> > > > }
> > > >
> > > > and
> > > >
> > > > static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
> > > > {
> > > > [...]
> > > >     /* The config space */
> > > >     qemu_get_be16s(f, &s->config.cols);
> > > >     qemu_get_be16s(f, &s->config.rows);
> > > >
> > > >     qemu_get_be32s(f, &max_nr_ports);
> > > >     tswap32s(&max_nr_ports);
> > > >      ^^^^^^
> > > >     if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
> > > > [...]
> > > > }
> > > >
> > > > If we stream subsections after the device descriptor as it is done
> > > > in VMState, these two will break because the device endian is stale.
> > > >
> > > > The first one can be easily dealt with: just defer the sanity check
> > > > to a post_load function.
> > > 
> > > Good, we're lucky here.
> > > 
> > > > The second is a bit more tricky: the
> > > > virtio serial migrates its config space (target endian) and the
> > > > active ports bitmap. The load code assumes max_nr_ports from the
> > > > config space tells the size of the ports bitmap... that means the
> > > > virtio migration protocol is also contaminated by target endianness. :-\
> > > 
> > > Ouch.
> > > 
> > > I guess we could break migration in the case of host endianness != 
> > > target endianness, like this:
> > > 
> > >      /* These three used to be fetched in target endianness and then
> > >       * stored as big endian.  It ended up as little endian if host and
> > >       * target endianness doesn't match.
> > >       *
> > >       * Starting with qemu 2.1, we always store as big endian.  The
> > >       * version wasn't bumped to avoid breaking backwards compatibility.
> > >       * We check the validity of max_nr_ports, and the incorrect-
> > >       * endianness max_nr_ports will be huge, which will abort migration
> > >       * anyway.
> > >       */
> > >      uint16_t cols = tswap16(s->config.cols);
> > >      uint16_t rows = tswap16(s->config.rows);
> > >      uint32_t max_nr_ports = tswap32(s->config.max_nr_ports);
> > > 
> > >      qemu_put_be16s(f, &cols);
> > >      qemu_put_be16s(f, &rows);
> > >      qemu_put_be32s(f, &max_nr_ports);
> > > 
> > > ...
> > > 
> > >      uint16_t cols, rows;
> > > 
> > >      qemu_get_be16s(f, &cols);
> > >      qemu_get_be16s(f, &rows);
> > >      qemu_get_be32s(f, &max_nr_ports);
> > > 
> > >      /* Convert back to target endianness when storing into the config
> > >       * space.
> > >       */
> > 
> > Paolo,
> > 
> > The patch set to support endian changing targets adds a device_endian
> > field to the VirtIODevice structure to be used instead of the default
> > target endianness as it happens with tswap() macros. It also introduces
> > virtio_tswap() helpers for this purpose, but they can only be used when
> > the device_endian field has been restored... in a subsection after the
> > device descriptor... :-\
> 
> Store it earlier then, using plain put/get.

Not sure I follow... this will break compatibility, no ?

> You can still add a section conditionally to cause
> a cleaner failure in broken cross-version scenarios.
> 
> > If the scenario is ppc64le-on-ppc64: tswap() macros don't do anything
> > and we cannot convert back to LE...
> > 
> > >      s->config.cols = tswap16(cols);
> > >      s->config.rows = tswap16(rows);
> > 
> > Since cols and rows are not involved in the protocol, we can safely
> > defer the conversion to post load.
> > 
> > >      if (max_nr_ports > tswap32(s->config.max_nr_ports) {
> > >          ...
> > >      }
> > > 
> > 
> > Since we know that 0 < max_nr_ports < 32,  is it acceptable to guess
> > the correct endianness with a heuristic ?
> > 
> > if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
> > 	max_nr_ports = bswap32(max_nr_ports);
> > }
> > 
> > if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
> > 	return -EINVAL;
> > }
> > 
> > > > In the case the answer for above is "legacy virtio really sucks" then,
> > > > is it acceptable to not honor bug-compatibility with older versions and
> > > > fix the code ? :)
> > > 
> > > As long as the common cases don't break, yes.  The question is what are 
> > > the common cases.  Here I think the only non-obscure case that could 
> > > break is x86-on-PPC, and it's not that common.
> > > 
> > > Paolo
> > > 
> > 
> > Thanks.
> 
> One starts doubting whether all this hackery is worth it.  virtio 1.0
> should be out real soon now, it makes everything LE so the problem goes
> away. It's not like PPC LE is so popular that we must support old drivers
> at all costs.  Won't time be better spent backporting virtio 1.0 drivers?
> 

Hmmm... AFAIC some QEMU maintainers expressed interest in supporting legacy
virtio in the case we have endian-changing targets. Patches to run a ppc64le
guests have been accepted in KVM, Linux and QEMU... the only missing block
is virtio. I don't especially care in supporting old drivers at all cost: this
request was expressed on the list. I just want people to be able to run a ppc64le
ubuntu-14.04 (and soon other distros) guest on a ppc64 box and be able to migrate.

Would it be acceptable to break compatibility for ppc64 (and maybe ARM) only with
a target specific hook being called from the virtio code ?

> 
> > -- 
> > Gregory Kurz                                     kurzgreg@fr.ibm.com
> >                                                  gkurz@linux.vnet.ibm.com
> > Software Engineer @ IBM/Meiosys                  http://www.ibm.com
> > Tel +33 (0)562 165 496
> > 
> > "Anarchy is about taking complete responsibility for yourself."
> >         Alan Moore.
> 

Thanks.

-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.

Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Alexander Graf]

On 12.06.14 09:54, Michael S. Tsirkin wrote:
> On Thu, Jun 12, 2014 at 09:43:51AM +0200, Greg Kurz wrote:
>> On Thu, 29 May 2014 12:16:26 +0200
>> Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> Il 29/05/2014 11:12, Greg Kurz ha scritto:
>>>> int virtio_load(VirtIODevice *vdev, QEMUFile *f)
>>>> {
>>>> [...]
>>>>              nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
>>>>                         ^^^^^^^^^^^
>>>>              /* Check it isn't doing very strange things with descriptor numbers. */
>>>>              if (nheads > vdev->vq[i].vring.num) {
>>>> [...]
>>>> }
>>>>
>>>> and
>>>>
>>>> static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
>>>> {
>>>> [...]
>>>>      /* The config space */
>>>>      qemu_get_be16s(f, &s->config.cols);
>>>>      qemu_get_be16s(f, &s->config.rows);
>>>>
>>>>      qemu_get_be32s(f, &max_nr_ports);
>>>>      tswap32s(&max_nr_ports);
>>>>       ^^^^^^
>>>>      if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
>>>> [...]
>>>> }
>>>>
>>>> If we stream subsections after the device descriptor as it is done
>>>> in VMState, these two will break because the device endian is stale.
>>>>
>>>> The first one can be easily dealt with: just defer the sanity check
>>>> to a post_load function.
>>> Good, we're lucky here.
>>>
>>>> The second is a bit more tricky: the
>>>> virtio serial migrates its config space (target endian) and the
>>>> active ports bitmap. The load code assumes max_nr_ports from the
>>>> config space tells the size of the ports bitmap... that means the
>>>> virtio migration protocol is also contaminated by target endianness. :-\
>>> Ouch.
>>>
>>> I guess we could break migration in the case of host endianness !=
>>> target endianness, like this:
>>>
>>>       /* These three used to be fetched in target endianness and then
>>>        * stored as big endian.  It ended up as little endian if host and
>>>        * target endianness doesn't match.
>>>        *
>>>        * Starting with qemu 2.1, we always store as big endian.  The
>>>        * version wasn't bumped to avoid breaking backwards compatibility.
>>>        * We check the validity of max_nr_ports, and the incorrect-
>>>        * endianness max_nr_ports will be huge, which will abort migration
>>>        * anyway.
>>>        */
>>>       uint16_t cols = tswap16(s->config.cols);
>>>       uint16_t rows = tswap16(s->config.rows);
>>>       uint32_t max_nr_ports = tswap32(s->config.max_nr_ports);
>>>
>>>       qemu_put_be16s(f, &cols);
>>>       qemu_put_be16s(f, &rows);
>>>       qemu_put_be32s(f, &max_nr_ports);
>>>
>>> ...
>>>
>>>       uint16_t cols, rows;
>>>
>>>       qemu_get_be16s(f, &cols);
>>>       qemu_get_be16s(f, &rows);
>>>       qemu_get_be32s(f, &max_nr_ports);
>>>
>>>       /* Convert back to target endianness when storing into the config
>>>        * space.
>>>        */
>> Paolo,
>>
>> The patch set to support endian changing targets adds a device_endian
>> field to the VirtIODevice structure to be used instead of the default
>> target endianness as it happens with tswap() macros. It also introduces
>> virtio_tswap() helpers for this purpose, but they can only be used when
>> the device_endian field has been restored... in a subsection after the
>> device descriptor... :-\
> Store it earlier then, using plain put/get.
> You can still add a section conditionally to cause
> a cleaner failure in broken cross-version scenarios.
>
>> If the scenario is ppc64le-on-ppc64: tswap() macros don't do anything
>> and we cannot convert back to LE...
>>
>>>       s->config.cols = tswap16(cols);
>>>       s->config.rows = tswap16(rows);
>> Since cols and rows are not involved in the protocol, we can safely
>> defer the conversion to post load.
>>
>>>       if (max_nr_ports > tswap32(s->config.max_nr_ports) {
>>>           ...
>>>       }
>>>
>> Since we know that 0 < max_nr_ports < 32,  is it acceptable to guess
>> the correct endianness with a heuristic ?
>>
>> if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
>> 	max_nr_ports = bswap32(max_nr_ports);
>> }
>>
>> if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
>> 	return -EINVAL;
>> }
>>
>>>> In the case the answer for above is "legacy virtio really sucks" then,
>>>> is it acceptable to not honor bug-compatibility with older versions and
>>>> fix the code ? :)
>>> As long as the common cases don't break, yes.  The question is what are
>>> the common cases.  Here I think the only non-obscure case that could
>>> break is x86-on-PPC, and it's not that common.
>>>
>>> Paolo
>>>
>> Thanks.
> One starts doubting whether all this hackery is worth it.  virtio 1.0
> should be out real soon now, it makes everything LE so the problem goes
> away. It's not like PPC LE is so popular that we must support old drivers
> at all costs.  Won't time be better spent backporting virtio 1.0 drivers?

There are already released and working Linux distributions (Ubuntu, 
openSUSE, maybe others) that don't have virtio 1.0 drivers. Putting our 
heads into the sand is not an option ;).


Alex

Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Paolo Bonzini]

Il 12/06/2014 09:43, Greg Kurz ha scritto:
> Since we know that 0 < max_nr_ports < 32,  is it acceptable to guess
> the correct endianness with a heuristic ?
>
> if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
> 	max_nr_ports = bswap32(max_nr_ports);
> }
>
> if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
> 	return -EINVAL;
> }

Yes, I guess it is acceptable.  So first you should fix the code to 
always serialize fields as big-endian, and then apply this little hack 
and virtio_tswap on top of the previous change.

Paolo

Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Alexander Graf]

On 12.06.14 10:55, Paolo Bonzini wrote:
> Il 12/06/2014 09:43, Greg Kurz ha scritto:
>> Since we know that 0 < max_nr_ports < 32,  is it acceptable to guess
>> the correct endianness with a heuristic ?
>>
>> if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
>>     max_nr_ports = bswap32(max_nr_ports);
>> }
>>
>> if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
>>     return -EINVAL;
>> }
>
> Yes, I guess it is acceptable.  So first you should fix the code to 
> always serialize fields as big-endian, and then apply this little hack 
> and virtio_tswap on top of the previous change.

Why don't we just ignore those config fields? They won't change during 
the lifetime of the guest anyway - and configuration needs to be the 
same on source and dest to work.


Alex

Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Michael S. Tsirkin]

On Thu, Jun 12, 2014 at 10:47:17AM +0200, Greg Kurz wrote:
> On Thu, 12 Jun 2014 10:54:48 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Thu, Jun 12, 2014 at 09:43:51AM +0200, Greg Kurz wrote:
> > > On Thu, 29 May 2014 12:16:26 +0200
> > > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > > > Il 29/05/2014 11:12, Greg Kurz ha scritto:
> > > > > int virtio_load(VirtIODevice *vdev, QEMUFile *f)
> > > > > {
> > > > > [...]
> > > > >             nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
> > > > >                        ^^^^^^^^^^^
> > > > >             /* Check it isn't doing very strange things with descriptor numbers. */
> > > > >             if (nheads > vdev->vq[i].vring.num) {
> > > > > [...]
> > > > > }
> > > > >
> > > > > and
> > > > >
> > > > > static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
> > > > > {
> > > > > [...]
> > > > >     /* The config space */
> > > > >     qemu_get_be16s(f, &s->config.cols);
> > > > >     qemu_get_be16s(f, &s->config.rows);
> > > > >
> > > > >     qemu_get_be32s(f, &max_nr_ports);
> > > > >     tswap32s(&max_nr_ports);
> > > > >      ^^^^^^
> > > > >     if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
> > > > > [...]
> > > > > }
> > > > >
> > > > > If we stream subsections after the device descriptor as it is done
> > > > > in VMState, these two will break because the device endian is stale.
> > > > >
> > > > > The first one can be easily dealt with: just defer the sanity check
> > > > > to a post_load function.
> > > > 
> > > > Good, we're lucky here.
> > > > 
> > > > > The second is a bit more tricky: the
> > > > > virtio serial migrates its config space (target endian) and the
> > > > > active ports bitmap. The load code assumes max_nr_ports from the
> > > > > config space tells the size of the ports bitmap... that means the
> > > > > virtio migration protocol is also contaminated by target endianness. :-\
> > > > 
> > > > Ouch.
> > > > 
> > > > I guess we could break migration in the case of host endianness != 
> > > > target endianness, like this:
> > > > 
> > > >      /* These three used to be fetched in target endianness and then
> > > >       * stored as big endian.  It ended up as little endian if host and
> > > >       * target endianness doesn't match.
> > > >       *
> > > >       * Starting with qemu 2.1, we always store as big endian.  The
> > > >       * version wasn't bumped to avoid breaking backwards compatibility.
> > > >       * We check the validity of max_nr_ports, and the incorrect-
> > > >       * endianness max_nr_ports will be huge, which will abort migration
> > > >       * anyway.
> > > >       */
> > > >      uint16_t cols = tswap16(s->config.cols);
> > > >      uint16_t rows = tswap16(s->config.rows);
> > > >      uint32_t max_nr_ports = tswap32(s->config.max_nr_ports);
> > > > 
> > > >      qemu_put_be16s(f, &cols);
> > > >      qemu_put_be16s(f, &rows);
> > > >      qemu_put_be32s(f, &max_nr_ports);
> > > > 
> > > > ...
> > > > 
> > > >      uint16_t cols, rows;
> > > > 
> > > >      qemu_get_be16s(f, &cols);
> > > >      qemu_get_be16s(f, &rows);
> > > >      qemu_get_be32s(f, &max_nr_ports);
> > > > 
> > > >      /* Convert back to target endianness when storing into the config
> > > >       * space.
> > > >       */
> > > 
> > > Paolo,
> > > 
> > > The patch set to support endian changing targets adds a device_endian
> > > field to the VirtIODevice structure to be used instead of the default
> > > target endianness as it happens with tswap() macros. It also introduces
> > > virtio_tswap() helpers for this purpose, but they can only be used when
> > > the device_endian field has been restored... in a subsection after the
> > > device descriptor... :-\
> > 
> > Store it earlier then, using plain put/get.
> 
> Not sure I follow... this will break compatibility, no ?

Do it only on the ambialent targets :)

> > You can still add a section conditionally to cause
> > a cleaner failure in broken cross-version scenarios.
> > 
> > > If the scenario is ppc64le-on-ppc64: tswap() macros don't do anything
> > > and we cannot convert back to LE...
> > > 
> > > >      s->config.cols = tswap16(cols);
> > > >      s->config.rows = tswap16(rows);
> > > 
> > > Since cols and rows are not involved in the protocol, we can safely
> > > defer the conversion to post load.
> > > 
> > > >      if (max_nr_ports > tswap32(s->config.max_nr_ports) {
> > > >          ...
> > > >      }
> > > > 
> > > 
> > > Since we know that 0 < max_nr_ports < 32,  is it acceptable to guess
> > > the correct endianness with a heuristic ?
> > > 
> > > if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
> > > 	max_nr_ports = bswap32(max_nr_ports);
> > > }
> > > 
> > > if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
> > > 	return -EINVAL;
> > > }
> > > 
> > > > > In the case the answer for above is "legacy virtio really sucks" then,
> > > > > is it acceptable to not honor bug-compatibility with older versions and
> > > > > fix the code ? :)
> > > > 
> > > > As long as the common cases don't break, yes.  The question is what are 
> > > > the common cases.  Here I think the only non-obscure case that could 
> > > > break is x86-on-PPC, and it's not that common.
> > > > 
> > > > Paolo
> > > > 
> > > 
> > > Thanks.
> > 
> > One starts doubting whether all this hackery is worth it.  virtio 1.0
> > should be out real soon now, it makes everything LE so the problem goes
> > away. It's not like PPC LE is so popular that we must support old drivers
> > at all costs.  Won't time be better spent backporting virtio 1.0 drivers?
> > 
> 
> Hmmm... AFAIC some QEMU maintainers expressed interest in supporting legacy
> virtio in the case we have endian-changing targets. Patches to run a ppc64le
> guests have been accepted in KVM, Linux and QEMU... the only missing block
> is virtio. I don't especially care in supporting old drivers at all cost: this
> request was expressed on the list. I just want people to be able to run a ppc64le
> ubuntu-14.04 (and soon other distros) guest on a ppc64 box and be able to migrate.

Sigh. So ubuntu ships known-broken virtio drivers in their LTS release
and now we are trying to make them work.
There's no working qemu configuration at the moment, do we really
have to build new hardware around known-broken drivers
and then maintain it all for years?
Why not build good hardware and backport new drivers?


> Would it be acceptable to break compatibility for ppc64 (and maybe ARM) only with
> a target specific hook being called from the virtio code ?

I think so.
Can we just add a function that tells us whether target is ambivalent?
Only migrate new stuff if that's the case.

> > 
> > > -- 
> > > Gregory Kurz                                     kurzgreg@fr.ibm.com
> > >                                                  gkurz@linux.vnet.ibm.com
> > > Software Engineer @ IBM/Meiosys                  http://www.ibm.com
> > > Tel +33 (0)562 165 496
> > > 
> > > "Anarchy is about taking complete responsibility for yourself."
> > >         Alan Moore.
> > 
> 
> Thanks.
> 
> -- 
> Gregory Kurz                                     kurzgreg@fr.ibm.com
>                                                  gkurz@linux.vnet.ibm.com
> Software Engineer @ IBM/Meiosys                  http://www.ibm.com
> Tel +33 (0)562 165 496
> 
> "Anarchy is about taking complete responsibility for yourself."
>         Alan Moore.

Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Greg Kurz]

On Thu, 12 Jun 2014 10:55:42 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 12/06/2014 09:43, Greg Kurz ha scritto:
> > Since we know that 0 < max_nr_ports < 32,  is it acceptable to guess
> > the correct endianness with a heuristic ?
> >
> > if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
> > 	max_nr_ports = bswap32(max_nr_ports);
> > }
> >
> > if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
> > 	return -EINVAL;
> > }
> 
> Yes, I guess it is acceptable.  So first you should fix the code to 
> always serialize fields as big-endian, and then apply this little hack 
> and virtio_tswap on top of the previous change.
> 
> Paolo
> 

BTW, can someone explain why we stream the device config ?

-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.

Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Alexander Graf]

On 12.06.14 10:47, Greg Kurz wrote:
> On Thu, 12 Jun 2014 10:54:48 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> On Thu, Jun 12, 2014 at 09:43:51AM +0200, Greg Kurz wrote:
>>> On Thu, 29 May 2014 12:16:26 +0200
>>> Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>> Il 29/05/2014 11:12, Greg Kurz ha scritto:
>>>>> int virtio_load(VirtIODevice *vdev, QEMUFile *f)
>>>>> {
>>>>> [...]
>>>>>              nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
>>>>>                         ^^^^^^^^^^^
>>>>>              /* Check it isn't doing very strange things with descriptor numbers. */
>>>>>              if (nheads > vdev->vq[i].vring.num) {
>>>>> [...]
>>>>> }
>>>>>
>>>>> and
>>>>>
>>>>> static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
>>>>> {
>>>>> [...]
>>>>>      /* The config space */
>>>>>      qemu_get_be16s(f, &s->config.cols);
>>>>>      qemu_get_be16s(f, &s->config.rows);
>>>>>
>>>>>      qemu_get_be32s(f, &max_nr_ports);
>>>>>      tswap32s(&max_nr_ports);
>>>>>       ^^^^^^
>>>>>      if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
>>>>> [...]
>>>>> }
>>>>>
>>>>> If we stream subsections after the device descriptor as it is done
>>>>> in VMState, these two will break because the device endian is stale.
>>>>>
>>>>> The first one can be easily dealt with: just defer the sanity check
>>>>> to a post_load function.
>>>> Good, we're lucky here.
>>>>
>>>>> The second is a bit more tricky: the
>>>>> virtio serial migrates its config space (target endian) and the
>>>>> active ports bitmap. The load code assumes max_nr_ports from the
>>>>> config space tells the size of the ports bitmap... that means the
>>>>> virtio migration protocol is also contaminated by target endianness. :-\
>>>> Ouch.
>>>>
>>>> I guess we could break migration in the case of host endianness !=
>>>> target endianness, like this:
>>>>
>>>>       /* These three used to be fetched in target endianness and then
>>>>        * stored as big endian.  It ended up as little endian if host and
>>>>        * target endianness doesn't match.
>>>>        *
>>>>        * Starting with qemu 2.1, we always store as big endian.  The
>>>>        * version wasn't bumped to avoid breaking backwards compatibility.
>>>>        * We check the validity of max_nr_ports, and the incorrect-
>>>>        * endianness max_nr_ports will be huge, which will abort migration
>>>>        * anyway.
>>>>        */
>>>>       uint16_t cols = tswap16(s->config.cols);
>>>>       uint16_t rows = tswap16(s->config.rows);
>>>>       uint32_t max_nr_ports = tswap32(s->config.max_nr_ports);
>>>>
>>>>       qemu_put_be16s(f, &cols);
>>>>       qemu_put_be16s(f, &rows);
>>>>       qemu_put_be32s(f, &max_nr_ports);
>>>>
>>>> ...
>>>>
>>>>       uint16_t cols, rows;
>>>>
>>>>       qemu_get_be16s(f, &cols);
>>>>       qemu_get_be16s(f, &rows);
>>>>       qemu_get_be32s(f, &max_nr_ports);
>>>>
>>>>       /* Convert back to target endianness when storing into the config
>>>>        * space.
>>>>        */
>>> Paolo,
>>>
>>> The patch set to support endian changing targets adds a device_endian
>>> field to the VirtIODevice structure to be used instead of the default
>>> target endianness as it happens with tswap() macros. It also introduces
>>> virtio_tswap() helpers for this purpose, but they can only be used when
>>> the device_endian field has been restored... in a subsection after the
>>> device descriptor... :-\
>> Store it earlier then, using plain put/get.
> Not sure I follow... this will break compatibility, no ?
>
>> You can still add a section conditionally to cause
>> a cleaner failure in broken cross-version scenarios.
>>
>>> If the scenario is ppc64le-on-ppc64: tswap() macros don't do anything
>>> and we cannot convert back to LE...
>>>
>>>>       s->config.cols = tswap16(cols);
>>>>       s->config.rows = tswap16(rows);
>>> Since cols and rows are not involved in the protocol, we can safely
>>> defer the conversion to post load.
>>>
>>>>       if (max_nr_ports > tswap32(s->config.max_nr_ports) {
>>>>           ...
>>>>       }
>>>>
>>> Since we know that 0 < max_nr_ports < 32,  is it acceptable to guess
>>> the correct endianness with a heuristic ?
>>>
>>> if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
>>> 	max_nr_ports = bswap32(max_nr_ports);
>>> }
>>>
>>> if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
>>> 	return -EINVAL;
>>> }
>>>
>>>>> In the case the answer for above is "legacy virtio really sucks" then,
>>>>> is it acceptable to not honor bug-compatibility with older versions and
>>>>> fix the code ? :)
>>>> As long as the common cases don't break, yes.  The question is what are
>>>> the common cases.  Here I think the only non-obscure case that could
>>>> break is x86-on-PPC, and it's not that common.
>>>>
>>>> Paolo
>>>>
>>> Thanks.
>> One starts doubting whether all this hackery is worth it.  virtio 1.0
>> should be out real soon now, it makes everything LE so the problem goes
>> away. It's not like PPC LE is so popular that we must support old drivers
>> at all costs.  Won't time be better spent backporting virtio 1.0 drivers?
>>
> Hmmm... AFAIC some QEMU maintainers expressed interest in supporting legacy
> virtio in the case we have endian-changing targets. Patches to run a ppc64le
> guests have been accepted in KVM, Linux and QEMU... the only missing block
> is virtio. I don't especially care in supporting old drivers at all cost: this
> request was expressed on the list. I just want people to be able to run a ppc64le
> ubuntu-14.04 (and soon other distros) guest on a ppc64 box and be able to migrate.
>
> Would it be acceptable to break compatibility for ppc64 (and maybe ARM) only with
> a target specific hook being called from the virtio code ?

I don't see the problem. Let's just make the config fields UNUSED in 
vmstate and keep the virtio-serial config space in good endian locally. 
Then you don't care about ordering of sections anymore.

Then we only need the subsection code for "endian is not target endian" 
which makes migration backwards compatible with non-switchable QEMU 
versions and everyone's happy. End of story :).


Alex

Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Michael S. Tsirkin]

On Thu, Jun 12, 2014 at 10:55:40AM +0200, Alexander Graf wrote:
> 
> On 12.06.14 09:54, Michael S. Tsirkin wrote:
> >On Thu, Jun 12, 2014 at 09:43:51AM +0200, Greg Kurz wrote:
> >>On Thu, 29 May 2014 12:16:26 +0200
> >>Paolo Bonzini <pbonzini@redhat.com> wrote:
> >>>Il 29/05/2014 11:12, Greg Kurz ha scritto:
> >>>>int virtio_load(VirtIODevice *vdev, QEMUFile *f)
> >>>>{
> >>>>[...]
> >>>>             nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
> >>>>                        ^^^^^^^^^^^
> >>>>             /* Check it isn't doing very strange things with descriptor numbers. */
> >>>>             if (nheads > vdev->vq[i].vring.num) {
> >>>>[...]
> >>>>}
> >>>>
> >>>>and
> >>>>
> >>>>static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
> >>>>{
> >>>>[...]
> >>>>     /* The config space */
> >>>>     qemu_get_be16s(f, &s->config.cols);
> >>>>     qemu_get_be16s(f, &s->config.rows);
> >>>>
> >>>>     qemu_get_be32s(f, &max_nr_ports);
> >>>>     tswap32s(&max_nr_ports);
> >>>>      ^^^^^^
> >>>>     if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
> >>>>[...]
> >>>>}
> >>>>
> >>>>If we stream subsections after the device descriptor as it is done
> >>>>in VMState, these two will break because the device endian is stale.
> >>>>
> >>>>The first one can be easily dealt with: just defer the sanity check
> >>>>to a post_load function.
> >>>Good, we're lucky here.
> >>>
> >>>>The second is a bit more tricky: the
> >>>>virtio serial migrates its config space (target endian) and the
> >>>>active ports bitmap. The load code assumes max_nr_ports from the
> >>>>config space tells the size of the ports bitmap... that means the
> >>>>virtio migration protocol is also contaminated by target endianness. :-\
> >>>Ouch.
> >>>
> >>>I guess we could break migration in the case of host endianness !=
> >>>target endianness, like this:
> >>>
> >>>      /* These three used to be fetched in target endianness and then
> >>>       * stored as big endian.  It ended up as little endian if host and
> >>>       * target endianness doesn't match.
> >>>       *
> >>>       * Starting with qemu 2.1, we always store as big endian.  The
> >>>       * version wasn't bumped to avoid breaking backwards compatibility.
> >>>       * We check the validity of max_nr_ports, and the incorrect-
> >>>       * endianness max_nr_ports will be huge, which will abort migration
> >>>       * anyway.
> >>>       */
> >>>      uint16_t cols = tswap16(s->config.cols);
> >>>      uint16_t rows = tswap16(s->config.rows);
> >>>      uint32_t max_nr_ports = tswap32(s->config.max_nr_ports);
> >>>
> >>>      qemu_put_be16s(f, &cols);
> >>>      qemu_put_be16s(f, &rows);
> >>>      qemu_put_be32s(f, &max_nr_ports);
> >>>
> >>>...
> >>>
> >>>      uint16_t cols, rows;
> >>>
> >>>      qemu_get_be16s(f, &cols);
> >>>      qemu_get_be16s(f, &rows);
> >>>      qemu_get_be32s(f, &max_nr_ports);
> >>>
> >>>      /* Convert back to target endianness when storing into the config
> >>>       * space.
> >>>       */
> >>Paolo,
> >>
> >>The patch set to support endian changing targets adds a device_endian
> >>field to the VirtIODevice structure to be used instead of the default
> >>target endianness as it happens with tswap() macros. It also introduces
> >>virtio_tswap() helpers for this purpose, but they can only be used when
> >>the device_endian field has been restored... in a subsection after the
> >>device descriptor... :-\
> >Store it earlier then, using plain put/get.
> >You can still add a section conditionally to cause
> >a cleaner failure in broken cross-version scenarios.
> >
> >>If the scenario is ppc64le-on-ppc64: tswap() macros don't do anything
> >>and we cannot convert back to LE...
> >>
> >>>      s->config.cols = tswap16(cols);
> >>>      s->config.rows = tswap16(rows);
> >>Since cols and rows are not involved in the protocol, we can safely
> >>defer the conversion to post load.
> >>
> >>>      if (max_nr_ports > tswap32(s->config.max_nr_ports) {
> >>>          ...
> >>>      }
> >>>
> >>Since we know that 0 < max_nr_ports < 32,  is it acceptable to guess
> >>the correct endianness with a heuristic ?
> >>
> >>if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
> >>	max_nr_ports = bswap32(max_nr_ports);
> >>}
> >>
> >>if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
> >>	return -EINVAL;
> >>}
> >>
> >>>>In the case the answer for above is "legacy virtio really sucks" then,
> >>>>is it acceptable to not honor bug-compatibility with older versions and
> >>>>fix the code ? :)
> >>>As long as the common cases don't break, yes.  The question is what are
> >>>the common cases.  Here I think the only non-obscure case that could
> >>>break is x86-on-PPC, and it's not that common.
> >>>
> >>>Paolo
> >>>
> >>Thanks.
> >One starts doubting whether all this hackery is worth it.  virtio 1.0
> >should be out real soon now, it makes everything LE so the problem goes
> >away. It's not like PPC LE is so popular that we must support old drivers
> >at all costs.  Won't time be better spent backporting virtio 1.0 drivers?
> 
> There are already released and working Linux distributions (Ubuntu,
> openSUSE, maybe others) that don't have virtio 1.0 drivers. Putting our
> heads into the sand is not an option ;).
> 
> 
> Alex

I don't get it. Does virtio work there at the moment?

-- 
MST

Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Alexander Graf]

On 12.06.14 11:07, Michael S. Tsirkin wrote:
> On Thu, Jun 12, 2014 at 10:55:40AM +0200, Alexander Graf wrote:
>> On 12.06.14 09:54, Michael S. Tsirkin wrote:
>>> On Thu, Jun 12, 2014 at 09:43:51AM +0200, Greg Kurz wrote:
>>>> On Thu, 29 May 2014 12:16:26 +0200
>>>> Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>> Il 29/05/2014 11:12, Greg Kurz ha scritto:
>>>>>> int virtio_load(VirtIODevice *vdev, QEMUFile *f)
>>>>>> {
>>>>>> [...]
>>>>>>              nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
>>>>>>                         ^^^^^^^^^^^
>>>>>>              /* Check it isn't doing very strange things with descriptor numbers. */
>>>>>>              if (nheads > vdev->vq[i].vring.num) {
>>>>>> [...]
>>>>>> }
>>>>>>
>>>>>> and
>>>>>>
>>>>>> static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
>>>>>> {
>>>>>> [...]
>>>>>>      /* The config space */
>>>>>>      qemu_get_be16s(f, &s->config.cols);
>>>>>>      qemu_get_be16s(f, &s->config.rows);
>>>>>>
>>>>>>      qemu_get_be32s(f, &max_nr_ports);
>>>>>>      tswap32s(&max_nr_ports);
>>>>>>       ^^^^^^
>>>>>>      if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
>>>>>> [...]
>>>>>> }
>>>>>>
>>>>>> If we stream subsections after the device descriptor as it is done
>>>>>> in VMState, these two will break because the device endian is stale.
>>>>>>
>>>>>> The first one can be easily dealt with: just defer the sanity check
>>>>>> to a post_load function.
>>>>> Good, we're lucky here.
>>>>>
>>>>>> The second is a bit more tricky: the
>>>>>> virtio serial migrates its config space (target endian) and the
>>>>>> active ports bitmap. The load code assumes max_nr_ports from the
>>>>>> config space tells the size of the ports bitmap... that means the
>>>>>> virtio migration protocol is also contaminated by target endianness. :-\
>>>>> Ouch.
>>>>>
>>>>> I guess we could break migration in the case of host endianness !=
>>>>> target endianness, like this:
>>>>>
>>>>>       /* These three used to be fetched in target endianness and then
>>>>>        * stored as big endian.  It ended up as little endian if host and
>>>>>        * target endianness doesn't match.
>>>>>        *
>>>>>        * Starting with qemu 2.1, we always store as big endian.  The
>>>>>        * version wasn't bumped to avoid breaking backwards compatibility.
>>>>>        * We check the validity of max_nr_ports, and the incorrect-
>>>>>        * endianness max_nr_ports will be huge, which will abort migration
>>>>>        * anyway.
>>>>>        */
>>>>>       uint16_t cols = tswap16(s->config.cols);
>>>>>       uint16_t rows = tswap16(s->config.rows);
>>>>>       uint32_t max_nr_ports = tswap32(s->config.max_nr_ports);
>>>>>
>>>>>       qemu_put_be16s(f, &cols);
>>>>>       qemu_put_be16s(f, &rows);
>>>>>       qemu_put_be32s(f, &max_nr_ports);
>>>>>
>>>>> ...
>>>>>
>>>>>       uint16_t cols, rows;
>>>>>
>>>>>       qemu_get_be16s(f, &cols);
>>>>>       qemu_get_be16s(f, &rows);
>>>>>       qemu_get_be32s(f, &max_nr_ports);
>>>>>
>>>>>       /* Convert back to target endianness when storing into the config
>>>>>        * space.
>>>>>        */
>>>> Paolo,
>>>>
>>>> The patch set to support endian changing targets adds a device_endian
>>>> field to the VirtIODevice structure to be used instead of the default
>>>> target endianness as it happens with tswap() macros. It also introduces
>>>> virtio_tswap() helpers for this purpose, but they can only be used when
>>>> the device_endian field has been restored... in a subsection after the
>>>> device descriptor... :-\
>>> Store it earlier then, using plain put/get.
>>> You can still add a section conditionally to cause
>>> a cleaner failure in broken cross-version scenarios.
>>>
>>>> If the scenario is ppc64le-on-ppc64: tswap() macros don't do anything
>>>> and we cannot convert back to LE...
>>>>
>>>>>       s->config.cols = tswap16(cols);
>>>>>       s->config.rows = tswap16(rows);
>>>> Since cols and rows are not involved in the protocol, we can safely
>>>> defer the conversion to post load.
>>>>
>>>>>       if (max_nr_ports > tswap32(s->config.max_nr_ports) {
>>>>>           ...
>>>>>       }
>>>>>
>>>> Since we know that 0 < max_nr_ports < 32,  is it acceptable to guess
>>>> the correct endianness with a heuristic ?
>>>>
>>>> if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
>>>> 	max_nr_ports = bswap32(max_nr_ports);
>>>> }
>>>>
>>>> if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
>>>> 	return -EINVAL;
>>>> }
>>>>
>>>>>> In the case the answer for above is "legacy virtio really sucks" then,
>>>>>> is it acceptable to not honor bug-compatibility with older versions and
>>>>>> fix the code ? :)
>>>>> As long as the common cases don't break, yes.  The question is what are
>>>>> the common cases.  Here I think the only non-obscure case that could
>>>>> break is x86-on-PPC, and it's not that common.
>>>>>
>>>>> Paolo
>>>>>
>>>> Thanks.
>>> One starts doubting whether all this hackery is worth it.  virtio 1.0
>>> should be out real soon now, it makes everything LE so the problem goes
>>> away. It's not like PPC LE is so popular that we must support old drivers
>>> at all costs.  Won't time be better spent backporting virtio 1.0 drivers?
>> There are already released and working Linux distributions (Ubuntu,
>> openSUSE, maybe others) that don't have virtio 1.0 drivers. Putting our
>> heads into the sand is not an option ;).
>>
>>
>> Alex
> I don't get it. Does virtio work there at the moment?

With the LE enable patch, yes, sure. Imagine the same would happen for 
Windows and e1000 - would you still argue the same way?


Alex

Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Paolo Bonzini]

Il 12/06/2014 11:06, Greg Kurz ha scritto:
> On Thu, 12 Jun 2014 10:55:42 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>> Il 12/06/2014 09:43, Greg Kurz ha scritto:
>>> Since we know that 0 < max_nr_ports < 32,  is it acceptable to guess
>>> the correct endianness with a heuristic ?
>>>
>>> if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
>>> 	max_nr_ports = bswap32(max_nr_ports);
>>> }
>>>
>>> if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
>>> 	return -EINVAL;
>>> }
>>
>> Yes, I guess it is acceptable.  So first you should fix the code to
>> always serialize fields as big-endian, and then apply this little hack
>> and virtio_tswap on top of the previous change.
>
> BTW, can someone explain why we stream the device config ?

For max_nr_ports it is useless indeed.  Let's keep storing it for 
backwards compatibility, but you can remove its load.

The cols and rows values should be defined by the host and thus could 
even change on migration, there is no need to store them.  As of now, 
however, they are unused in QEMU and should always be zero, because 
VIRTIO_CONSOLE_F_SIZE is not supported.

Paolo

Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Michael S. Tsirkin]

On Thu, Jun 12, 2014 at 11:19:47AM +0200, Paolo Bonzini wrote:
> Il 12/06/2014 11:06, Greg Kurz ha scritto:
> >On Thu, 12 Jun 2014 10:55:42 +0200
> >Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> >>Il 12/06/2014 09:43, Greg Kurz ha scritto:
> >>>Since we know that 0 < max_nr_ports < 32,  is it acceptable to guess
> >>>the correct endianness with a heuristic ?
> >>>
> >>>if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
> >>>	max_nr_ports = bswap32(max_nr_ports);
> >>>}
> >>>
> >>>if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
> >>>	return -EINVAL;
> >>>}
> >>
> >>Yes, I guess it is acceptable.  So first you should fix the code to
> >>always serialize fields as big-endian, and then apply this little hack
> >>and virtio_tswap on top of the previous change.
> >
> >BTW, can someone explain why we stream the device config ?
> 
> For max_nr_ports it is useless indeed.  Let's keep storing it for backwards
> compatibility, but you can remove its load.
> 
> The cols and rows values should be defined by the host and thus could even
> change on migration, there is no need to store them.  As of now, however,
> they are unused in QEMU and should always be zero, because
> VIRTIO_CONSOLE_F_SIZE is not supported.
> 
> Paolo

Maybe just drop unnecessary stuff for new machine types?
Then we won't need hacks to migrate it.

-- 
MST

Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Paolo Bonzini]

Il 12/06/2014 11:37, Michael S. Tsirkin ha scritto:
> Maybe just drop unnecessary stuff for new machine types?
> Then we won't need hacks to migrate it.

For any machine type it's trivial to migrate it.  All machine types 
including old ones can disregard the migrated values.

Paolo

Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Alexander Graf]

On 12.06.14 11:39, Paolo Bonzini wrote:
> Il 12/06/2014 11:37, Michael S. Tsirkin ha scritto:
>> Maybe just drop unnecessary stuff for new machine types?
>> Then we won't need hacks to migrate it.
>
> For any machine type it's trivial to migrate it.  All machine types 
> including old ones can disregard the migrated values.

How about a patch like this before the actual LE awareness ones? With 
this we should make virtio-serial config space completely independent of 
live migration.

Also since QEMU versions that do read these swapped values during 
migration are not bi-endian aware, we can never get into a case where a 
cross-endian save needs to be considered ;).


Alex


diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 2b647b6..73cb9b7 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -670,6 +670,7 @@ static int virtio_serial_load(QEMUFile *f, void 
*opaque, int version_id)
      uint32_t max_nr_ports, nr_active_ports, ports_map;
      unsigned int i;
      int ret;
+    uint32_t tmp;

      if (version_id > 3) {
          return -EINVAL;
@@ -685,17 +686,12 @@ static int virtio_serial_load(QEMUFile *f, void 
*opaque, int version_id)
          return 0;
      }

-    /* The config space */
-    qemu_get_be16s(f, &s->config.cols);
-    qemu_get_be16s(f, &s->config.rows);
-
-    qemu_get_be32s(f, &max_nr_ports);
-    tswap32s(&max_nr_ports);
-    if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
-        /* Source could have had more ports than us. Fail migration. */
-        return -EINVAL;
-    }
+    /* Unused */
+    qemu_get_be16s(f, &tmp);
+    qemu_get_be16s(f, &tmp);
+    qemu_get_be32s(f, &tmp);

+    max_nr_ports = tswap32(s->config.max_nr_ports);
      for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
          qemu_get_be32s(f, &ports_map);

Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Greg Kurz]

On Thu, 12 Jun 2014 11:43:20 +0200
Alexander Graf <agraf@suse.de> wrote:

> 
> On 12.06.14 11:39, Paolo Bonzini wrote:
> > Il 12/06/2014 11:37, Michael S. Tsirkin ha scritto:
> >> Maybe just drop unnecessary stuff for new machine types?
> >> Then we won't need hacks to migrate it.
> >
> > For any machine type it's trivial to migrate it.  All machine types 
> > including old ones can disregard the migrated values.
> 
> How about a patch like this before the actual LE awareness ones? With 
> this we should make virtio-serial config space completely independent of 
> live migration.
> 
> Also since QEMU versions that do read these swapped values during 
> migration are not bi-endian aware, we can never get into a case where a 
> cross-endian save needs to be considered ;).
> 
> 
> Alex
> 
> 
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index 2b647b6..73cb9b7 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -670,6 +670,7 @@ static int virtio_serial_load(QEMUFile *f, void 
> *opaque, int version_id)
>       uint32_t max_nr_ports, nr_active_ports, ports_map;
>       unsigned int i;
>       int ret;
> +    uint32_t tmp;
> 
>       if (version_id > 3) {
>           return -EINVAL;
> @@ -685,17 +686,12 @@ static int virtio_serial_load(QEMUFile *f, void 
> *opaque, int version_id)
>           return 0;
>       }
> 
> -    /* The config space */
> -    qemu_get_be16s(f, &s->config.cols);
> -    qemu_get_be16s(f, &s->config.rows);
> -
> -    qemu_get_be32s(f, &max_nr_ports);
> -    tswap32s(&max_nr_ports);
> -    if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
> -        /* Source could have had more ports than us. Fail migration. */
> -        return -EINVAL;
> -    }
> +    /* Unused */
> +    qemu_get_be16s(f, &tmp);
> +    qemu_get_be16s(f, &tmp);
> +    qemu_get_be32s(f, &tmp);
> 
> +    max_nr_ports = tswap32(s->config.max_nr_ports);
>       for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
>           qemu_get_be32s(f, &ports_map);
> 
> 

For the moment, we have 0 < max_nr_ports < 32 so the source
machine only stores a single 32 bit value... If this limit
gets raised, we can end up sending more than that... and
only the source machine max_nr_ports value can give the
information...

-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.

Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Alexander Graf]

> Am 12.06.2014 um 12:14 schrieb Greg Kurz <gkurz@linux.vnet.ibm.com>:
> 
> On Thu, 12 Jun 2014 11:43:20 +0200
> Alexander Graf <agraf@suse.de> wrote:
> 
>> 
>>> On 12.06.14 11:39, Paolo Bonzini wrote:
>>> Il 12/06/2014 11:37, Michael S. Tsirkin ha scritto:
>>>> Maybe just drop unnecessary stuff for new machine types?
>>>> Then we won't need hacks to migrate it.
>>> 
>>> For any machine type it's trivial to migrate it.  All machine types 
>>> including old ones can disregard the migrated values.
>> 
>> How about a patch like this before the actual LE awareness ones? With 
>> this we should make virtio-serial config space completely independent of 
>> live migration.
>> 
>> Also since QEMU versions that do read these swapped values during 
>> migration are not bi-endian aware, we can never get into a case where a 
>> cross-endian save needs to be considered ;).
>> 
>> 
>> Alex
>> 
>> 
>> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
>> index 2b647b6..73cb9b7 100644
>> --- a/hw/char/virtio-serial-bus.c
>> +++ b/hw/char/virtio-serial-bus.c
>> @@ -670,6 +670,7 @@ static int virtio_serial_load(QEMUFile *f, void 
>> *opaque, int version_id)
>>      uint32_t max_nr_ports, nr_active_ports, ports_map;
>>      unsigned int i;
>>      int ret;
>> +    uint32_t tmp;
>> 
>>      if (version_id > 3) {
>>          return -EINVAL;
>> @@ -685,17 +686,12 @@ static int virtio_serial_load(QEMUFile *f, void 
>> *opaque, int version_id)
>>          return 0;
>>      }
>> 
>> -    /* The config space */
>> -    qemu_get_be16s(f, &s->config.cols);
>> -    qemu_get_be16s(f, &s->config.rows);
>> -
>> -    qemu_get_be32s(f, &max_nr_ports);
>> -    tswap32s(&max_nr_ports);
>> -    if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
>> -        /* Source could have had more ports than us. Fail migration. */
>> -        return -EINVAL;
>> -    }
>> +    /* Unused */
>> +    qemu_get_be16s(f, &tmp);
>> +    qemu_get_be16s(f, &tmp);
>> +    qemu_get_be32s(f, &tmp);
>> 
>> +    max_nr_ports = tswap32(s->config.max_nr_ports);
>>      for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
>>          qemu_get_be32s(f, &ports_map);
> 
> For the moment, we have 0 < max_nr_ports < 32 so the source
> machine only stores a single 32 bit value... If this limit
> gets raised, we can end up sending more than that... and
> only the source machine max_nr_ports value can give the
> information...

Why? This value only ever gets set in realize, so it will not change during the lifetime of the device - which means we don't need to migrate it.


Alex

Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Greg Kurz]

On Thu, 12 Jun 2014 12:39:27 +0200
Alexander Graf <agraf@suse.de> wrote:

> 
> 
> > Am 12.06.2014 um 12:14 schrieb Greg Kurz <gkurz@linux.vnet.ibm.com>:
> > 
> > On Thu, 12 Jun 2014 11:43:20 +0200
> > Alexander Graf <agraf@suse.de> wrote:
> > 
> >> 
> >>> On 12.06.14 11:39, Paolo Bonzini wrote:
> >>> Il 12/06/2014 11:37, Michael S. Tsirkin ha scritto:
> >>>> Maybe just drop unnecessary stuff for new machine types?
> >>>> Then we won't need hacks to migrate it.
> >>> 
> >>> For any machine type it's trivial to migrate it.  All machine types 
> >>> including old ones can disregard the migrated values.
> >> 
> >> How about a patch like this before the actual LE awareness ones? With 
> >> this we should make virtio-serial config space completely independent of 
> >> live migration.
> >> 
> >> Also since QEMU versions that do read these swapped values during 
> >> migration are not bi-endian aware, we can never get into a case where a 
> >> cross-endian save needs to be considered ;).
> >> 
> >> 
> >> Alex
> >> 
> >> 
> >> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> >> index 2b647b6..73cb9b7 100644
> >> --- a/hw/char/virtio-serial-bus.c
> >> +++ b/hw/char/virtio-serial-bus.c
> >> @@ -670,6 +670,7 @@ static int virtio_serial_load(QEMUFile *f, void 
> >> *opaque, int version_id)
> >>      uint32_t max_nr_ports, nr_active_ports, ports_map;
> >>      unsigned int i;
> >>      int ret;
> >> +    uint32_t tmp;
> >> 
> >>      if (version_id > 3) {
> >>          return -EINVAL;
> >> @@ -685,17 +686,12 @@ static int virtio_serial_load(QEMUFile *f, void 
> >> *opaque, int version_id)
> >>          return 0;
> >>      }
> >> 
> >> -    /* The config space */
> >> -    qemu_get_be16s(f, &s->config.cols);
> >> -    qemu_get_be16s(f, &s->config.rows);
> >> -
> >> -    qemu_get_be32s(f, &max_nr_ports);
> >> -    tswap32s(&max_nr_ports);
> >> -    if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
> >> -        /* Source could have had more ports than us. Fail migration. */
> >> -        return -EINVAL;
> >> -    }
> >> +    /* Unused */
> >> +    qemu_get_be16s(f, &tmp);
> >> +    qemu_get_be16s(f, &tmp);
> >> +    qemu_get_be32s(f, &tmp);
> >> 
> >> +    max_nr_ports = tswap32(s->config.max_nr_ports);
> >>      for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
> >>          qemu_get_be32s(f, &ports_map);
> > 
> > For the moment, we have 0 < max_nr_ports < 32 so the source
> > machine only stores a single 32 bit value... If this limit
> > gets raised, we can end up sending more than that... and
> > only the source machine max_nr_ports value can give the
> > information...
> 
> Why? This value only ever gets set in realize, so it will not change during the lifetime of the device - which means we don't need to migrate it.
> 

I agree with the fact that the value does not change and should not be migrated in the first place.
I am just worried about the size of the active ports bitmap that is streamed in the for loop... it
is only 32 bit as of today, because we are limited to 32 ports. How would this work if the limit is
raised ? How can the destination machine know how many bits have to be read ?

-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.

Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Michael S. Tsirkin]

On Thu, Jun 12, 2014 at 11:39:42AM +0200, Paolo Bonzini wrote:
> Il 12/06/2014 11:37, Michael S. Tsirkin ha scritto:
> >Maybe just drop unnecessary stuff for new machine types?
> >Then we won't need hacks to migrate it.
> 
> For any machine type it's trivial to migrate it.  All machine types
> including old ones can disregard the migrated values.
> 
> Paolo

Ah, good idea, so simply disregard the values in load.

-- 
MST

Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Michael S. Tsirkin]

On Thu, Jun 12, 2014 at 11:43:20AM +0200, Alexander Graf wrote:
> 
> On 12.06.14 11:39, Paolo Bonzini wrote:
> >Il 12/06/2014 11:37, Michael S. Tsirkin ha scritto:
> >>Maybe just drop unnecessary stuff for new machine types?
> >>Then we won't need hacks to migrate it.
> >
> >For any machine type it's trivial to migrate it.  All machine types
> >including old ones can disregard the migrated values.
> 
> How about a patch like this before the actual LE awareness ones? With this
> we should make virtio-serial config space completely independent of live
> migration.
> 
> Also since QEMU versions that do read these swapped values during migration
> are not bi-endian aware, we can never get into a case where a cross-endian
> save needs to be considered ;).
> 
> 
> Alex
> 
> 
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index 2b647b6..73cb9b7 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -670,6 +670,7 @@ static int virtio_serial_load(QEMUFile *f, void *opaque,
> int version_id)
>      uint32_t max_nr_ports, nr_active_ports, ports_map;
>      unsigned int i;
>      int ret;
> +    uint32_t tmp;
> 
>      if (version_id > 3) {
>          return -EINVAL;
> @@ -685,17 +686,12 @@ static int virtio_serial_load(QEMUFile *f, void
> *opaque, int version_id)
>          return 0;
>      }
> 
> -    /* The config space */
> -    qemu_get_be16s(f, &s->config.cols);
> -    qemu_get_be16s(f, &s->config.rows);
> -
> -    qemu_get_be32s(f, &max_nr_ports);
> -    tswap32s(&max_nr_ports);
> -    if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
> -        /* Source could have had more ports than us. Fail migration. */
> -        return -EINVAL;
> -    }
> +    /* Unused */
> +    qemu_get_be16s(f, &tmp);
> +    qemu_get_be16s(f, &tmp);
> +    qemu_get_be32s(f, &tmp);
> 
> +    max_nr_ports = tswap32(s->config.max_nr_ports);
>      for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
>          qemu_get_be32s(f, &ports_map);
>

Exactly.
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
 

Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Michael S. Tsirkin]

On Thu, Jun 12, 2014 at 12:14:40PM +0200, Greg Kurz wrote:
> On Thu, 12 Jun 2014 11:43:20 +0200
> Alexander Graf <agraf@suse.de> wrote:
> 
> > 
> > On 12.06.14 11:39, Paolo Bonzini wrote:
> > > Il 12/06/2014 11:37, Michael S. Tsirkin ha scritto:
> > >> Maybe just drop unnecessary stuff for new machine types?
> > >> Then we won't need hacks to migrate it.
> > >
> > > For any machine type it's trivial to migrate it.  All machine types 
> > > including old ones can disregard the migrated values.
> > 
> > How about a patch like this before the actual LE awareness ones? With 
> > this we should make virtio-serial config space completely independent of 
> > live migration.
> > 
> > Also since QEMU versions that do read these swapped values during 
> > migration are not bi-endian aware, we can never get into a case where a 
> > cross-endian save needs to be considered ;).
> > 
> > 
> > Alex
> > 
> > 
> > diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> > index 2b647b6..73cb9b7 100644
> > --- a/hw/char/virtio-serial-bus.c
> > +++ b/hw/char/virtio-serial-bus.c
> > @@ -670,6 +670,7 @@ static int virtio_serial_load(QEMUFile *f, void 
> > *opaque, int version_id)
> >       uint32_t max_nr_ports, nr_active_ports, ports_map;
> >       unsigned int i;
> >       int ret;
> > +    uint32_t tmp;
> > 
> >       if (version_id > 3) {
> >           return -EINVAL;
> > @@ -685,17 +686,12 @@ static int virtio_serial_load(QEMUFile *f, void 
> > *opaque, int version_id)
> >           return 0;
> >       }
> > 
> > -    /* The config space */
> > -    qemu_get_be16s(f, &s->config.cols);
> > -    qemu_get_be16s(f, &s->config.rows);
> > -
> > -    qemu_get_be32s(f, &max_nr_ports);
> > -    tswap32s(&max_nr_ports);
> > -    if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
> > -        /* Source could have had more ports than us. Fail migration. */
> > -        return -EINVAL;
> > -    }
> > +    /* Unused */
> > +    qemu_get_be16s(f, &tmp);
> > +    qemu_get_be16s(f, &tmp);
> > +    qemu_get_be32s(f, &tmp);
> > 
> > +    max_nr_ports = tswap32(s->config.max_nr_ports);
> >       for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
> >           qemu_get_be32s(f, &ports_map);
> > 
> > 
> 
> For the moment, we have 0 < max_nr_ports < 32 so the source
> machine only stores a single 32 bit value... If this limit
> gets raised, we can end up sending more than that... and
> only the source machine max_nr_ports value can give the
> information...

I don't think we need to worry.
We won't be able to change max_nr_ports in compat machine types.


> -- 
> Gregory Kurz                                     kurzgreg@fr.ibm.com
>                                                  gkurz@linux.vnet.ibm.com
> Software Engineer @ IBM/Meiosys                  http://www.ibm.com
> Tel +33 (0)562 165 496
> 
> "Anarchy is about taking complete responsibility for yourself."
>         Alan Moore.

Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Alexander Graf]

> Am 12.06.2014 um 12:50 schrieb Greg Kurz <gkurz@linux.vnet.ibm.com>:
> 
> On Thu, 12 Jun 2014 12:39:27 +0200
> Alexander Graf <agraf@suse.de> wrote:
> 
>> 
>> 
>>> Am 12.06.2014 um 12:14 schrieb Greg Kurz <gkurz@linux.vnet.ibm.com>:
>>> 
>>> On Thu, 12 Jun 2014 11:43:20 +0200
>>> Alexander Graf <agraf@suse.de> wrote:
>>> 
>>>> 
>>>>>> On 12.06.14 11:39, Paolo Bonzini wrote:
>>>>>> Il 12/06/2014 11:37, Michael S. Tsirkin ha scritto:
>>>>>> Maybe just drop unnecessary stuff for new machine types?
>>>>>> Then we won't need hacks to migrate it.
>>>>> 
>>>>> For any machine type it's trivial to migrate it.  All machine types 
>>>>> including old ones can disregard the migrated values.
>>>> 
>>>> How about a patch like this before the actual LE awareness ones? With 
>>>> this we should make virtio-serial config space completely independent of 
>>>> live migration.
>>>> 
>>>> Also since QEMU versions that do read these swapped values during 
>>>> migration are not bi-endian aware, we can never get into a case where a 
>>>> cross-endian save needs to be considered ;).
>>>> 
>>>> 
>>>> Alex
>>>> 
>>>> 
>>>> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
>>>> index 2b647b6..73cb9b7 100644
>>>> --- a/hw/char/virtio-serial-bus.c
>>>> +++ b/hw/char/virtio-serial-bus.c
>>>> @@ -670,6 +670,7 @@ static int virtio_serial_load(QEMUFile *f, void 
>>>> *opaque, int version_id)
>>>>     uint32_t max_nr_ports, nr_active_ports, ports_map;
>>>>     unsigned int i;
>>>>     int ret;
>>>> +    uint32_t tmp;
>>>> 
>>>>     if (version_id > 3) {
>>>>         return -EINVAL;
>>>> @@ -685,17 +686,12 @@ static int virtio_serial_load(QEMUFile *f, void 
>>>> *opaque, int version_id)
>>>>         return 0;
>>>>     }
>>>> 
>>>> -    /* The config space */
>>>> -    qemu_get_be16s(f, &s->config.cols);
>>>> -    qemu_get_be16s(f, &s->config.rows);
>>>> -
>>>> -    qemu_get_be32s(f, &max_nr_ports);
>>>> -    tswap32s(&max_nr_ports);
>>>> -    if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
>>>> -        /* Source could have had more ports than us. Fail migration. */
>>>> -        return -EINVAL;
>>>> -    }
>>>> +    /* Unused */
>>>> +    qemu_get_be16s(f, &tmp);
>>>> +    qemu_get_be16s(f, &tmp);
>>>> +    qemu_get_be32s(f, &tmp);
>>>> 
>>>> +    max_nr_ports = tswap32(s->config.max_nr_ports);
>>>>     for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
>>>>         qemu_get_be32s(f, &ports_map);
>>> 
>>> For the moment, we have 0 < max_nr_ports < 32 so the source
>>> machine only stores a single 32 bit value... If this limit
>>> gets raised, we can end up sending more than that... and
>>> only the source machine max_nr_ports value can give the
>>> information...
>> 
>> Why? This value only ever gets set in realize, so it will not change during the lifetime of the device - which means we don't need to migrate it.
> 
> I agree with the fact that the value does not change and should not be migrated in the first place.
> I am just worried about the size of the active ports bitmap that is streamed in the for loop... it
> is only 32 bit as of today, because we are limited to 32 ports. How would this work if the limit is
> raised ? How can the destination machine know how many bits have to be read ?

The destination has be be executed in compat mode to the older qemu version via a versioned machine type. This should ensure that limits are kept.


Alex

Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Michael S. Tsirkin]

On Thu, Jun 12, 2014 at 12:50:56PM +0200, Greg Kurz wrote:
> On Thu, 12 Jun 2014 12:39:27 +0200
> Alexander Graf <agraf@suse.de> wrote:
> 
> > 
> > 
> > > Am 12.06.2014 um 12:14 schrieb Greg Kurz <gkurz@linux.vnet.ibm.com>:
> > > 
> > > On Thu, 12 Jun 2014 11:43:20 +0200
> > > Alexander Graf <agraf@suse.de> wrote:
> > > 
> > >> 
> > >>> On 12.06.14 11:39, Paolo Bonzini wrote:
> > >>> Il 12/06/2014 11:37, Michael S. Tsirkin ha scritto:
> > >>>> Maybe just drop unnecessary stuff for new machine types?
> > >>>> Then we won't need hacks to migrate it.
> > >>> 
> > >>> For any machine type it's trivial to migrate it.  All machine types 
> > >>> including old ones can disregard the migrated values.
> > >> 
> > >> How about a patch like this before the actual LE awareness ones? With 
> > >> this we should make virtio-serial config space completely independent of 
> > >> live migration.
> > >> 
> > >> Also since QEMU versions that do read these swapped values during 
> > >> migration are not bi-endian aware, we can never get into a case where a 
> > >> cross-endian save needs to be considered ;).
> > >> 
> > >> 
> > >> Alex
> > >> 
> > >> 
> > >> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> > >> index 2b647b6..73cb9b7 100644
> > >> --- a/hw/char/virtio-serial-bus.c
> > >> +++ b/hw/char/virtio-serial-bus.c
> > >> @@ -670,6 +670,7 @@ static int virtio_serial_load(QEMUFile *f, void 
> > >> *opaque, int version_id)
> > >>      uint32_t max_nr_ports, nr_active_ports, ports_map;
> > >>      unsigned int i;
> > >>      int ret;
> > >> +    uint32_t tmp;
> > >> 
> > >>      if (version_id > 3) {
> > >>          return -EINVAL;
> > >> @@ -685,17 +686,12 @@ static int virtio_serial_load(QEMUFile *f, void 
> > >> *opaque, int version_id)
> > >>          return 0;
> > >>      }
> > >> 
> > >> -    /* The config space */
> > >> -    qemu_get_be16s(f, &s->config.cols);
> > >> -    qemu_get_be16s(f, &s->config.rows);
> > >> -
> > >> -    qemu_get_be32s(f, &max_nr_ports);
> > >> -    tswap32s(&max_nr_ports);
> > >> -    if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
> > >> -        /* Source could have had more ports than us. Fail migration. */
> > >> -        return -EINVAL;
> > >> -    }
> > >> +    /* Unused */
> > >> +    qemu_get_be16s(f, &tmp);
> > >> +    qemu_get_be16s(f, &tmp);
> > >> +    qemu_get_be32s(f, &tmp);
> > >> 
> > >> +    max_nr_ports = tswap32(s->config.max_nr_ports);
> > >>      for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
> > >>          qemu_get_be32s(f, &ports_map);
> > > 
> > > For the moment, we have 0 < max_nr_ports < 32 so the source
> > > machine only stores a single 32 bit value... If this limit
> > > gets raised, we can end up sending more than that... and
> > > only the source machine max_nr_ports value can give the
> > > information...
> > 
> > Why? This value only ever gets set in realize, so it will not change during the lifetime of the device - which means we don't need to migrate it.
> > 
> 
> I agree with the fact that the value does not change and should not be migrated in the first place.
> I am just worried about the size of the active ports bitmap that is streamed in the for loop... it
> is only 32 bit as of today, because we are limited to 32 ports. How would this work if the limit is
> raised ? How can the destination machine know how many bits have to be read ?

When the destination machine is started with -M 2.1, it
knows that it has to read 32 bit.
If started with -M 3.0 it reads in 42 bits :)


> -- 
> Gregory Kurz                                     kurzgreg@fr.ibm.com
>                                                  gkurz@linux.vnet.ibm.com
> Software Engineer @ IBM/Meiosys                  http://www.ibm.com
> Tel +33 (0)562 165 496
> 
> "Anarchy is about taking complete responsibility for yourself."
>         Alan Moore.

Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Greg Kurz]

On Thu, 12 Jun 2014 13:59:59 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Jun 12, 2014 at 12:50:56PM +0200, Greg Kurz wrote:
> > On Thu, 12 Jun 2014 12:39:27 +0200
> > Alexander Graf <agraf@suse.de> wrote:
> > 
> > > 
> > > 
> > > > Am 12.06.2014 um 12:14 schrieb Greg Kurz <gkurz@linux.vnet.ibm.com>:
> > > > 
> > > > On Thu, 12 Jun 2014 11:43:20 +0200
> > > > Alexander Graf <agraf@suse.de> wrote:
> > > > 
> > > >> 
> > > >>> On 12.06.14 11:39, Paolo Bonzini wrote:
> > > >>> Il 12/06/2014 11:37, Michael S. Tsirkin ha scritto:
> > > >>>> Maybe just drop unnecessary stuff for new machine types?
> > > >>>> Then we won't need hacks to migrate it.
> > > >>> 
> > > >>> For any machine type it's trivial to migrate it.  All machine types 
> > > >>> including old ones can disregard the migrated values.
> > > >> 
> > > >> How about a patch like this before the actual LE awareness ones? With 
> > > >> this we should make virtio-serial config space completely independent of 
> > > >> live migration.
> > > >> 
> > > >> Also since QEMU versions that do read these swapped values during 
> > > >> migration are not bi-endian aware, we can never get into a case where a 
> > > >> cross-endian save needs to be considered ;).
> > > >> 
> > > >> 
> > > >> Alex
> > > >> 
> > > >> 
> > > >> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> > > >> index 2b647b6..73cb9b7 100644
> > > >> --- a/hw/char/virtio-serial-bus.c
> > > >> +++ b/hw/char/virtio-serial-bus.c
> > > >> @@ -670,6 +670,7 @@ static int virtio_serial_load(QEMUFile *f, void 
> > > >> *opaque, int version_id)
> > > >>      uint32_t max_nr_ports, nr_active_ports, ports_map;
> > > >>      unsigned int i;
> > > >>      int ret;
> > > >> +    uint32_t tmp;
> > > >> 
> > > >>      if (version_id > 3) {
> > > >>          return -EINVAL;
> > > >> @@ -685,17 +686,12 @@ static int virtio_serial_load(QEMUFile *f, void 
> > > >> *opaque, int version_id)
> > > >>          return 0;
> > > >>      }
> > > >> 
> > > >> -    /* The config space */
> > > >> -    qemu_get_be16s(f, &s->config.cols);
> > > >> -    qemu_get_be16s(f, &s->config.rows);
> > > >> -
> > > >> -    qemu_get_be32s(f, &max_nr_ports);
> > > >> -    tswap32s(&max_nr_ports);
> > > >> -    if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
> > > >> -        /* Source could have had more ports than us. Fail migration. */
> > > >> -        return -EINVAL;
> > > >> -    }
> > > >> +    /* Unused */
> > > >> +    qemu_get_be16s(f, &tmp);
> > > >> +    qemu_get_be16s(f, &tmp);
> > > >> +    qemu_get_be32s(f, &tmp);
> > > >> 
> > > >> +    max_nr_ports = tswap32(s->config.max_nr_ports);
> > > >>      for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
> > > >>          qemu_get_be32s(f, &ports_map);
> > > > 
> > > > For the moment, we have 0 < max_nr_ports < 32 so the source
> > > > machine only stores a single 32 bit value... If this limit
> > > > gets raised, we can end up sending more than that... and
> > > > only the source machine max_nr_ports value can give the
> > > > information...
> > > 
> > > Why? This value only ever gets set in realize, so it will not change during the lifetime of the device - which means we don't need to migrate it.
> > > 
> > 
> > I agree with the fact that the value does not change and should not be migrated in the first place.
> > I am just worried about the size of the active ports bitmap that is streamed in the for loop... it
> > is only 32 bit as of today, because we are limited to 32 ports. How would this work if the limit is
> > raised ? How can the destination machine know how many bits have to be read ?
> 
> When the destination machine is started with -M 2.1, it
> knows that it has to read 32 bit.
> If started with -M 3.0 it reads in 42 bits :)
> 

Okay ! I was completely missing this point... now things make sense at last ! :)
About Alex's patch, will you or Amit or Anthony apply it directly or should
I send it along with my patches for a full review ?

Thanks for your time. 

-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.