[Qemu-devel] [PATCH 0/7] virtio endian-ambivalent target fixes.

[Qemu-devel] [PATCH 0/7] virtio endian-ambivalent target fixes.

[Rusty Russell]

This is a re-transmit of the core of the virtio endian code.  Since
there seems to be some interest in ARM BE virtio, I've separated this from
the direct problem I was solving: PowerPC LE.

Please apply!
Rusty.

Rusty Russell (7):
  virtio_get_byteswap: function for endian-ambivalent targets using
    virtio.
  virtio: allow byte swapping for vring and config access
  hw/net/virtio-net: use virtio wrappers to access headers.
  hw/net/virtio-balloon: use virtio wrappers to access page frame
    numbers.
  hw/block/virtio-blk: use virtio wrappers to access headers.
  hw/scsi/virtio-scsi: use virtio wrappers to access headers.
  hw/char/virtio-serial-bus: use virtio wrappers to access headers.

 hw/block/virtio-blk.c             |  35 +++++-----
 hw/char/virtio-serial-bus.c       |  34 +++++-----
 hw/net/virtio-net.c               |  15 +++--
 hw/scsi/virtio-scsi.c             |  33 +++++-----
 hw/virtio/virtio-balloon.c        |   3 +-
 hw/virtio/virtio.c                |  34 ++++++----
 include/hw/virtio/virtio-access.h | 133 ++++++++++++++++++++++++++++++++++++++
 include/hw/virtio/virtio.h        |   2 +
 stubs/Makefile.objs               |   1 +
 stubs/virtio_get_byteswap.c       |   6 ++
 10 files changed, 225 insertions(+), 71 deletions(-)
 create mode 100644 include/hw/virtio/virtio-access.h
 create mode 100644 stubs/virtio_get_byteswap.c

-- 
1.8.1.2

[Qemu-devel] [PATCH 1/7] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Rusty Russell]

virtio data structures are defined as "target endian", which assumes
that's a fixed value.  In fact, that actually means it's
platform-specific.

The OASIS virtio 1.0 spec will fix this.  Meanwhile, create a hook for
little endian ppc (and potentially ARM).  This is called at device
reset time (which is done before any driver is loaded) since it
may involve a system call to get the status when running under kvm.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 hw/virtio/virtio.c                |   6 ++
 include/hw/virtio/virtio-access.h | 133 ++++++++++++++++++++++++++++++++++++++
 include/hw/virtio/virtio.h        |   2 +
 stubs/Makefile.objs               |   1 +
 stubs/virtio_get_byteswap.c       |   6 ++
 5 files changed, 148 insertions(+)
 create mode 100644 include/hw/virtio/virtio-access.h
 create mode 100644 stubs/virtio_get_byteswap.c

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 2f1e73b..7730780 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -18,6 +18,9 @@
 #include "hw/virtio/virtio.h"
 #include "qemu/atomic.h"
 #include "hw/virtio/virtio-bus.h"
+#include "hw/virtio/virtio-access.h"
+
+bool virtio_byteswap = false;
 
 /*
  * The alignment to use between consumer and producer parts of vring.
@@ -543,6 +546,9 @@ void virtio_reset(void *opaque)
 
     virtio_set_status(vdev, 0);
 
+    /* We assume all devices are the same endian. */
+    virtio_byteswap = virtio_get_byteswap();
+
     if (k->reset) {
         k->reset(vdev);
     }
diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
new file mode 100644
index 0000000..2263c9c
--- /dev/null
+++ b/include/hw/virtio/virtio-access.h
@@ -0,0 +1,133 @@
+/*
+ * Virtio Accessor Support: In case your target can change endian.
+ *
+ * Copyright IBM, Corp. 2013
+ *
+ * Authors:
+ *  Rusty Russell   <rusty@au.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ */
+#ifndef _QEMU_VIRTIO_ACCESS_H
+#define _QEMU_VIRTIO_ACCESS_H
+#include "hw/virtio/virtio.h"
+
+/* Initialized by virtio_get_byteswap() at any virtio device reset. */
+extern bool virtio_byteswap;
+
+static inline uint16_t virtio_lduw_phys(hwaddr pa)
+{
+    if (virtio_byteswap) {
+        return bswap16(lduw_phys(pa));
+    }
+    return lduw_phys(pa);
+    
+}
+
+static inline uint32_t virtio_ldl_phys(hwaddr pa)
+{
+    if (virtio_byteswap) {
+        return bswap32(ldl_phys(pa));
+    }
+    return ldl_phys(pa);
+}
+
+static inline uint64_t virtio_ldq_phys(hwaddr pa)
+{
+    if (virtio_byteswap) {
+        return bswap64(ldq_phys(pa));
+    }
+    return ldq_phys(pa);
+}
+
+static inline void virtio_stw_phys(hwaddr pa, uint16_t value)
+{
+    if (virtio_byteswap) {
+        stw_phys(pa, bswap16(value));
+    } else {
+        stw_phys(pa, value);
+    }
+}
+
+static inline void virtio_stl_phys(hwaddr pa, uint32_t value)
+{
+    if (virtio_byteswap) {
+        stl_phys(pa, bswap32(value));
+    } else {
+        stl_phys(pa, value);
+    }
+}
+
+static inline void virtio_stw_p(void *ptr, uint16_t v)
+{
+    if (virtio_byteswap) {
+        stw_p(ptr, bswap16(v));
+    } else {
+        stw_p(ptr, v);
+    }
+}
+
+static inline void virtio_stl_p(void *ptr, uint32_t v)
+{
+    if (virtio_byteswap) {
+        stl_p(ptr, bswap32(v));
+    } else {
+        stl_p(ptr, v);
+    }
+}
+
+static inline void virtio_stq_p(void *ptr, uint64_t v)
+{
+    if (virtio_byteswap) {
+        stq_p(ptr, bswap64(v));
+    } else {
+        stq_p(ptr, v);
+    }
+}
+
+static inline int virtio_lduw_p(const void *ptr)
+{
+    if (virtio_byteswap) {
+        return bswap16(lduw_p(ptr));
+    } else {
+        return lduw_p(ptr);
+    }
+}
+
+static inline int virtio_ldl_p(const void *ptr)
+{
+    if (virtio_byteswap) {
+        return bswap32(ldl_p(ptr));
+    } else {
+        return ldl_p(ptr);
+    }
+}
+
+static inline uint64_t virtio_ldq_p(const void *ptr)
+{
+    if (virtio_byteswap) {
+        return bswap64(ldq_p(ptr));
+    } else {
+        return ldq_p(ptr);
+    }
+}
+
+static inline uint32_t virtio_tswap32(uint32_t s)
+{
+    if (virtio_byteswap) {
+        return bswap32(tswap32(s));
+    } else {
+        return tswap32(s);
+    }
+}
+
+static inline void virtio_tswap32s(uint32_t *s)
+{
+    tswap32s(s);
+    if (virtio_byteswap) {
+        *s = bswap32(*s);
+    }
+}
+#endif /* _QEMU_VIRTIO_ACCESS_H */
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index a90522d..065e1d9 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -249,4 +249,6 @@ 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);
+
+extern bool virtio_get_byteswap(void);
 #endif
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index df92fe5..7e7a9c8 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -27,3 +27,4 @@ stub-obj-y += vm-stop.o
 stub-obj-y += vmstate.o
 stub-obj-$(CONFIG_WIN32) += fd-register.o
 stub-obj-y += cpus.o
+stub-obj-y += virtio_get_byteswap.o
diff --git a/stubs/virtio_get_byteswap.c b/stubs/virtio_get_byteswap.c
new file mode 100644
index 0000000..7cf764d
--- /dev/null
+++ b/stubs/virtio_get_byteswap.c
@@ -0,0 +1,6 @@
+#include "hw/virtio/virtio.h"
+
+bool virtio_get_byteswap(void)
+{
+    return false;
+}
-- 
1.8.1.2

[Qemu-devel] [PATCH 2/7] virtio: allow byte swapping for vring and config access

[Rusty Russell]

This is based on a simpler patch by Anthony Liguouri, which only handled
the vring accesses.  We also need some drivers to access these helpers,
eg. for data which contains headers.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 hw/virtio/virtio.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 7730780..bbf9f15 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -107,49 +107,49 @@ static inline uint64_t vring_desc_addr(hwaddr desc_pa, int i)
 {
     hwaddr pa;
     pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, addr);
-    return ldq_phys(pa);
+    return virtio_ldq_phys(pa);
 }
 
 static inline uint32_t vring_desc_len(hwaddr desc_pa, int i)
 {
     hwaddr pa;
     pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, len);
-    return ldl_phys(pa);
+    return virtio_ldl_phys(pa);
 }
 
 static inline uint16_t vring_desc_flags(hwaddr desc_pa, int i)
 {
     hwaddr pa;
     pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, flags);
-    return lduw_phys(pa);
+    return virtio_lduw_phys(pa);
 }
 
 static inline uint16_t vring_desc_next(hwaddr desc_pa, int i)
 {
     hwaddr pa;
     pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, next);
-    return lduw_phys(pa);
+    return virtio_lduw_phys(pa);
 }
 
 static inline uint16_t vring_avail_flags(VirtQueue *vq)
 {
     hwaddr pa;
     pa = vq->vring.avail + offsetof(VRingAvail, flags);
-    return lduw_phys(pa);
+    return virtio_lduw_phys(pa);
 }
 
 static inline uint16_t vring_avail_idx(VirtQueue *vq)
 {
     hwaddr pa;
     pa = vq->vring.avail + offsetof(VRingAvail, idx);
-    return lduw_phys(pa);
+    return virtio_lduw_phys(pa);
 }
 
 static inline uint16_t vring_avail_ring(VirtQueue *vq, int i)
 {
     hwaddr pa;
     pa = vq->vring.avail + offsetof(VRingAvail, ring[i]);
-    return lduw_phys(pa);
+    return virtio_lduw_phys(pa);
 }
 
 static inline uint16_t vring_used_event(VirtQueue *vq)
@@ -161,42 +161,42 @@ static inline void vring_used_ring_id(VirtQueue *vq, int i, uint32_t val)
 {
     hwaddr pa;
     pa = vq->vring.used + offsetof(VRingUsed, ring[i].id);
-    stl_phys(pa, val);
+    virtio_stl_phys(pa, val);
 }
 
 static inline void vring_used_ring_len(VirtQueue *vq, int i, uint32_t val)
 {
     hwaddr pa;
     pa = vq->vring.used + offsetof(VRingUsed, ring[i].len);
-    stl_phys(pa, val);
+    virtio_stl_phys(pa, val);
 }
 
 static uint16_t vring_used_idx(VirtQueue *vq)
 {
     hwaddr pa;
     pa = vq->vring.used + offsetof(VRingUsed, idx);
-    return lduw_phys(pa);
+    return virtio_lduw_phys(pa);
 }
 
 static inline void vring_used_idx_set(VirtQueue *vq, uint16_t val)
 {
     hwaddr pa;
     pa = vq->vring.used + offsetof(VRingUsed, idx);
-    stw_phys(pa, val);
+    virtio_stw_phys(pa, val);
 }
 
 static inline void vring_used_flags_set_bit(VirtQueue *vq, int mask)
 {
     hwaddr pa;
     pa = vq->vring.used + offsetof(VRingUsed, flags);
-    stw_phys(pa, lduw_phys(pa) | mask);
+    virtio_stw_phys(pa, virtio_lduw_phys(pa) | mask);
 }
 
 static inline void vring_used_flags_unset_bit(VirtQueue *vq, int mask)
 {
     hwaddr pa;
     pa = vq->vring.used + offsetof(VRingUsed, flags);
-    stw_phys(pa, lduw_phys(pa) & ~mask);
+    virtio_stw_phys(pa, virtio_lduw_phys(pa) & ~mask);
 }
 
 static inline void vring_avail_event(VirtQueue *vq, uint16_t val)
@@ -206,7 +206,7 @@ static inline void vring_avail_event(VirtQueue *vq, uint16_t val)
         return;
     }
     pa = vq->vring.used + offsetof(VRingUsed, ring[vq->vring.num]);
-    stw_phys(pa, val);
+    virtio_stw_phys(pa, val);
 }
 
 void virtio_queue_set_notification(VirtQueue *vq, int enable)
-- 
1.8.1.2

[Qemu-devel] [PATCH 3/7] hw/net/virtio-net: use virtio wrappers to access headers.

[Rusty Russell]

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/net/virtio-net.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 22dbd05..431a4b6 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -23,6 +23,7 @@
 #include "hw/virtio/virtio-bus.h"
 #include "qapi/qmp/qjson.h"
 #include "monitor/monitor.h"
+#include "hw/virtio/virtio-access.h"
 
 #define VIRTIO_NET_VM_VERSION    11
 
@@ -72,8 +73,8 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
     VirtIONet *n = VIRTIO_NET(vdev);
     struct virtio_net_config netcfg;
 
-    stw_p(&netcfg.status, n->status);
-    stw_p(&netcfg.max_virtqueue_pairs, n->max_queues);
+    virtio_stw_p(&netcfg.status, n->status);
+    virtio_stw_p(&netcfg.max_virtqueue_pairs, n->max_queues);
     memcpy(netcfg.mac, n->mac, ETH_ALEN);
     memcpy(config, &netcfg, n->config_size);
 }
@@ -618,7 +619,7 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
 
     s = iov_to_buf(iov, iov_cnt, 0, &mac_data.entries,
                    sizeof(mac_data.entries));
-    mac_data.entries = ldl_p(&mac_data.entries);
+    mac_data.entries = virtio_ldl_p(&mac_data.entries);
     if (s != sizeof(mac_data.entries)) {
         goto error;
     }
@@ -645,7 +646,7 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
 
     s = iov_to_buf(iov, iov_cnt, 0, &mac_data.entries,
                    sizeof(mac_data.entries));
-    mac_data.entries = ldl_p(&mac_data.entries);
+    mac_data.entries = virtio_ldl_p(&mac_data.entries);
     if (s != sizeof(mac_data.entries)) {
         goto error;
     }
@@ -684,7 +685,7 @@ static int virtio_net_handle_vlan_table(VirtIONet *n, uint8_t cmd,
     NetClientState *nc = qemu_get_queue(n->nic);
 
     s = iov_to_buf(iov, iov_cnt, 0, &vid, sizeof(vid));
-    vid = lduw_p(&vid);
+    vid = virtio_lduw_p(&vid);
     if (s != sizeof(vid)) {
         return VIRTIO_NET_ERR;
     }
@@ -721,7 +722,7 @@ static int virtio_net_handle_mq(VirtIONet *n, uint8_t cmd,
         return VIRTIO_NET_ERR;
     }
 
-    queues = lduw_p(&mq.virtqueue_pairs);
+    queues = virtio_lduw_p(&mq.virtqueue_pairs);
 
     if (queues < VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN ||
         queues > VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX ||
@@ -1020,7 +1021,7 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t
     }
 
     if (mhdr_cnt) {
-        stw_p(&mhdr.num_buffers, i);
+        virtio_stw_p(&mhdr.num_buffers, i);
         iov_from_buf(mhdr_sg, mhdr_cnt,
                      0,
                      &mhdr.num_buffers, sizeof mhdr.num_buffers);
-- 
1.8.1.2

[Qemu-devel] [PATCH 4/7] hw/net/virtio-balloon: use virtio wrappers to access page frame numbers.

[Rusty Russell]

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/virtio/virtio-balloon.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 9504877..97c4ac5 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -30,6 +30,7 @@
 #endif
 
 #include "hw/virtio/virtio-bus.h"
+#include "hw/virtio/virtio-access.h"
 
 static void balloon_page(void *addr, int deflate)
 {
@@ -192,7 +193,7 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
             ram_addr_t pa;
             ram_addr_t addr;
 
-            pa = (ram_addr_t)ldl_p(&pfn) << VIRTIO_BALLOON_PFN_SHIFT;
+            pa = (ram_addr_t)virtio_ldl_p(&pfn) << VIRTIO_BALLOON_PFN_SHIFT;
             offset += 4;
 
             /* FIXME: remove get_system_memory(), but how? */
-- 
1.8.1.2

[Qemu-devel] [PATCH 5/7] hw/block/virtio-blk: use virtio wrappers to access headers.

[Rusty Russell]

Note that st*_raw and ld*_raw are effectively replaced by st*_p and ld*_p.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/block/virtio-blk.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 49a23c3..111ad4b 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -26,6 +26,7 @@
 # include <scsi/sg.h>
 #endif
 #include "hw/virtio/virtio-bus.h"
+#include "hw/virtio/virtio-access.h"
 
 typedef struct VirtIOBlockReq
 {
@@ -77,7 +78,7 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
     trace_virtio_blk_rw_complete(req, ret);
 
     if (ret) {
-        bool is_read = !(ldl_p(&req->out->type) & VIRTIO_BLK_T_OUT);
+        bool is_read = !(virtio_ldl_p(&req->out->type) & VIRTIO_BLK_T_OUT);
         if (virtio_blk_handle_rw_error(req, -ret, is_read))
             return;
     }
@@ -224,12 +225,12 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
         hdr.status = CHECK_CONDITION;
     }
 
-    stl_p(&req->scsi->errors,
-          hdr.status | (hdr.msg_status << 8) |
-          (hdr.host_status << 16) | (hdr.driver_status << 24));
-    stl_p(&req->scsi->residual, hdr.resid);
-    stl_p(&req->scsi->sense_len, hdr.sb_len_wr);
-    stl_p(&req->scsi->data_len, hdr.dxfer_len);
+    virtio_stl_p(&req->scsi->errors,
+                 hdr.status | (hdr.msg_status << 8) |
+                 (hdr.host_status << 16) | (hdr.driver_status << 24));
+    virtio_stl_p(&req->scsi->residual, hdr.resid);
+    virtio_stl_p(&req->scsi->sense_len, hdr.sb_len_wr);
+    virtio_stl_p(&req->scsi->data_len, hdr.dxfer_len);
 
     virtio_blk_req_complete(req, status);
     g_free(req);
@@ -240,7 +241,7 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
 
 fail:
     /* Just put anything nonzero so that the ioctl fails in the guest.  */
-    stl_p(&req->scsi->errors, 255);
+    virtio_stl_p(&req->scsi->errors, 255);
     virtio_blk_req_complete(req, status);
     g_free(req);
 }
@@ -286,7 +287,7 @@ static void virtio_blk_handle_write(VirtIOBlockReq *req, MultiReqBuffer *mrb)
     BlockRequest *blkreq;
     uint64_t sector;
 
-    sector = ldq_p(&req->out->sector);
+    sector = virtio_ldq_p(&req->out->sector);
 
     bdrv_acct_start(req->dev->bs, &req->acct, req->qiov.size, BDRV_ACCT_WRITE);
 
@@ -320,7 +321,7 @@ static void virtio_blk_handle_read(VirtIOBlockReq *req)
 {
     uint64_t sector;
 
-    sector = ldq_p(&req->out->sector);
+    sector = virtio_ldq_p(&req->out->sector);
 
     bdrv_acct_start(req->dev->bs, &req->acct, req->qiov.size, BDRV_ACCT_READ);
 
@@ -358,7 +359,7 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req,
     req->out = (void *)req->elem.out_sg[0].iov_base;
     req->in = (void *)req->elem.in_sg[req->elem.in_num - 1].iov_base;
 
-    type = ldl_p(&req->out->type);
+    type = virtio_ldl_p(&req->out->type);
 
     if (type & VIRTIO_BLK_T_FLUSH) {
         virtio_blk_handle_flush(req, mrb);
@@ -487,12 +488,12 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
 
     bdrv_get_geometry(s->bs, &capacity);
     memset(&blkcfg, 0, sizeof(blkcfg));
-    stq_raw(&blkcfg.capacity, capacity);
-    stl_raw(&blkcfg.seg_max, 128 - 2);
-    stw_raw(&blkcfg.cylinders, s->conf->cyls);
-    stl_raw(&blkcfg.blk_size, blk_size);
-    stw_raw(&blkcfg.min_io_size, s->conf->min_io_size / blk_size);
-    stw_raw(&blkcfg.opt_io_size, s->conf->opt_io_size / blk_size);
+    virtio_stq_p(&blkcfg.capacity, capacity);
+    virtio_stl_p(&blkcfg.seg_max, 128 - 2);
+    virtio_stw_p(&blkcfg.cylinders, s->conf->cyls);
+    virtio_stl_p(&blkcfg.blk_size, blk_size);
+    virtio_stw_p(&blkcfg.min_io_size, s->conf->min_io_size / blk_size);
+    virtio_stw_p(&blkcfg.opt_io_size, s->conf->opt_io_size / blk_size);
     blkcfg.heads = s->conf->heads;
     /*
      * We must ensure that the block device capacity is a multiple of
-- 
1.8.1.2

[Qemu-devel] [PATCH 6/7] hw/scsi/virtio-scsi: use virtio wrappers to access headers.

[Rusty Russell]

Note that st*_raw and ld*_raw are effectively replaced by st*_p and ld*_p.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/scsi/virtio-scsi.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 26d95a1..e581dd0 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -18,6 +18,7 @@
 #include <hw/scsi/scsi.h>
 #include <block/scsi.h>
 #include <hw/virtio/virtio-bus.h>
+#include "hw/virtio/virtio-access.h"
 
 typedef struct VirtIOSCSIReq {
     VirtIOSCSI *dev;
@@ -309,12 +310,12 @@ static void virtio_scsi_command_complete(SCSIRequest *r, uint32_t status,
     req->resp.cmd->response = VIRTIO_SCSI_S_OK;
     req->resp.cmd->status = status;
     if (req->resp.cmd->status == GOOD) {
-        req->resp.cmd->resid = tswap32(resid);
+        req->resp.cmd->resid = virtio_tswap32(resid);
     } else {
         req->resp.cmd->resid = 0;
         sense_len = scsi_req_get_sense(r, req->resp.cmd->sense,
                                        VIRTIO_SCSI_SENSE_SIZE);
-        req->resp.cmd->sense_len = tswap32(sense_len);
+        req->resp.cmd->sense_len = virtio_tswap32(sense_len);
     }
     virtio_scsi_complete_req(req);
 }
@@ -410,16 +411,16 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
     VirtIOSCSIConfig *scsiconf = (VirtIOSCSIConfig *)config;
     VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
 
-    stl_raw(&scsiconf->num_queues, s->conf.num_queues);
-    stl_raw(&scsiconf->seg_max, 128 - 2);
-    stl_raw(&scsiconf->max_sectors, s->conf.max_sectors);
-    stl_raw(&scsiconf->cmd_per_lun, s->conf.cmd_per_lun);
-    stl_raw(&scsiconf->event_info_size, sizeof(VirtIOSCSIEvent));
-    stl_raw(&scsiconf->sense_size, s->sense_size);
-    stl_raw(&scsiconf->cdb_size, s->cdb_size);
-    stw_raw(&scsiconf->max_channel, VIRTIO_SCSI_MAX_CHANNEL);
-    stw_raw(&scsiconf->max_target, VIRTIO_SCSI_MAX_TARGET);
-    stl_raw(&scsiconf->max_lun, VIRTIO_SCSI_MAX_LUN);
+    virtio_stl_p(&scsiconf->num_queues, s->conf.num_queues);
+    virtio_stl_p(&scsiconf->seg_max, 128 - 2);
+    virtio_stl_p(&scsiconf->max_sectors, s->conf.max_sectors);
+    virtio_stl_p(&scsiconf->cmd_per_lun, s->conf.cmd_per_lun);
+    virtio_stl_p(&scsiconf->event_info_size, sizeof(VirtIOSCSIEvent));
+    virtio_stl_p(&scsiconf->sense_size, s->sense_size);
+    virtio_stl_p(&scsiconf->cdb_size, s->cdb_size);
+    virtio_stw_p(&scsiconf->max_channel, VIRTIO_SCSI_MAX_CHANNEL);
+    virtio_stw_p(&scsiconf->max_target, VIRTIO_SCSI_MAX_TARGET);
+    virtio_stl_p(&scsiconf->max_lun, VIRTIO_SCSI_MAX_LUN);
 }
 
 static void virtio_scsi_set_config(VirtIODevice *vdev,
@@ -428,14 +429,14 @@ static void virtio_scsi_set_config(VirtIODevice *vdev,
     VirtIOSCSIConfig *scsiconf = (VirtIOSCSIConfig *)config;
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
 
-    if ((uint32_t) ldl_raw(&scsiconf->sense_size) >= 65536 ||
-        (uint32_t) ldl_raw(&scsiconf->cdb_size) >= 256) {
+    if ((uint32_t) virtio_ldl_p(&scsiconf->sense_size) >= 65536 ||
+        (uint32_t) virtio_ldl_p(&scsiconf->cdb_size) >= 256) {
         error_report("bad data written to virtio-scsi configuration space");
         exit(1);
     }
 
-    vs->sense_size = ldl_raw(&scsiconf->sense_size);
-    vs->cdb_size = ldl_raw(&scsiconf->cdb_size);
+    vs->sense_size = virtio_ldl_p(&scsiconf->sense_size);
+    vs->cdb_size = virtio_ldl_p(&scsiconf->cdb_size);
 }
 
 static uint32_t virtio_scsi_get_features(VirtIODevice *vdev,
-- 
1.8.1.2

[Qemu-devel] [PATCH 7/7] hw/char/virtio-serial-bus: use virtio wrappers to access headers.

[Rusty Russell]

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/char/virtio-serial-bus.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 703f026..2dc0ccf 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -24,6 +24,7 @@
 #include "hw/sysbus.h"
 #include "trace.h"
 #include "hw/virtio/virtio-serial.h"
+#include "hw/virtio/virtio-access.h"
 
 static VirtIOSerialPort *find_port_by_id(VirtIOSerial *vser, uint32_t id)
 {
@@ -185,9 +186,9 @@ static size_t send_control_event(VirtIOSerial *vser, uint32_t port_id,
 {
     struct virtio_console_control cpkt;
 
-    stl_p(&cpkt.id, port_id);
-    stw_p(&cpkt.event, event);
-    stw_p(&cpkt.value, value);
+    virtio_stl_p(&cpkt.id, port_id);
+    virtio_stw_p(&cpkt.event, event);
+    virtio_stw_p(&cpkt.value, value);
 
     trace_virtio_serial_send_control_event(port_id, event, value);
     return send_control_msg(vser, &cpkt, sizeof(cpkt));
@@ -291,8 +292,8 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
         return;
     }
 
-    cpkt.event = lduw_p(&gcpkt->event);
-    cpkt.value = lduw_p(&gcpkt->value);
+    cpkt.event = virtio_lduw_p(&gcpkt->event);
+    cpkt.value = virtio_lduw_p(&gcpkt->value);
 
     trace_virtio_serial_handle_control_message(cpkt.event, cpkt.value);
 
@@ -312,10 +313,10 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
         return;
     }
 
-    port = find_port_by_id(vser, ldl_p(&gcpkt->id));
+    port = find_port_by_id(vser, virtio_ldl_p(&gcpkt->id));
     if (!port) {
         error_report("virtio-serial-bus: Unexpected port id %u for device %s",
-                     ldl_p(&gcpkt->id), vser->bus.qbus.name);
+                     virtio_ldl_p(&gcpkt->id), vser->bus.qbus.name);
         return;
     }
 
@@ -342,9 +343,9 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
         }
 
         if (port->name) {
-            stl_p(&cpkt.id, port->id);
-            stw_p(&cpkt.event, VIRTIO_CONSOLE_PORT_NAME);
-            stw_p(&cpkt.value, 1);
+            virtio_stl_p(&cpkt.id, port->id);
+            virtio_stw_p(&cpkt.event, VIRTIO_CONSOLE_PORT_NAME);
+            virtio_stw_p(&cpkt.value, 1);
 
             buffer_len = sizeof(cpkt) + strlen(port->name) + 1;
             buffer = g_malloc(buffer_len);
@@ -536,7 +537,7 @@ static void virtio_serial_save(QEMUFile *f, void *opaque)
     qemu_put_be32s(f, &s->config.max_nr_ports);
 
     /* The ports map */
-    max_nr_ports = tswap32(s->config.max_nr_ports);
+    max_nr_ports = virtio_tswap32(s->config.max_nr_ports);
     for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
         qemu_put_be32s(f, &s->ports_map[i]);
     }
@@ -690,8 +691,8 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
     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)) {
+    virtio_tswap32s(&max_nr_ports);
+    if (max_nr_ports > virtio_tswap32(s->config.max_nr_ports)) {
         /* Source could have had more ports than us. Fail migration. */
         return -EINVAL;
     }
@@ -760,7 +761,7 @@ static uint32_t find_free_port_id(VirtIOSerial *vser)
 {
     unsigned int i, max_nr_ports;
 
-    max_nr_ports = tswap32(vser->config.max_nr_ports);
+    max_nr_ports = virtio_tswap32(vser->config.max_nr_ports);
     for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
         uint32_t map, bit;
 
@@ -846,7 +847,7 @@ static int virtser_port_qdev_init(DeviceState *qdev)
         }
     }
 
-    max_nr_ports = tswap32(port->vser->config.max_nr_ports);
+    max_nr_ports = virtio_tswap32(port->vser->config.max_nr_ports);
     if (port->id >= max_nr_ports) {
         error_report("virtio-serial-bus: Out-of-range port id specified, max. allowed: %u",
                      max_nr_ports - 1);
@@ -946,7 +947,8 @@ static int virtio_serial_device_init(VirtIODevice *vdev)
         vser->ovqs[i] = virtio_add_queue(vdev, 128, handle_output);
     }
 
-    vser->config.max_nr_ports = tswap32(vser->serial.max_virtserial_ports);
+    vser->config.max_nr_ports =
+        virtio_tswap32(vser->serial.max_virtserial_ports);
     vser->ports_map = g_malloc0(((vser->serial.max_virtserial_ports + 31) / 32)
         * sizeof(vser->ports_map[0]));
     /*
-- 
1.8.1.2

Re: [Qemu-devel] [PATCH 0/7] virtio endian-ambivalent target fixes.

[Thomas Huth]

On Thu, 17 Oct 2013 14:23:35 +1030
Rusty Russell <rusty@rustcorp.com.au> wrote:

> This is a re-transmit of the core of the virtio endian code.  Since
> there seems to be some interest in ARM BE virtio, I've separated this from
> the direct problem I was solving: PowerPC LE.
> 
> Please apply!
> Rusty.
> 
> Rusty Russell (7):
>   virtio_get_byteswap: function for endian-ambivalent targets using
>     virtio.
>   virtio: allow byte swapping for vring and config access
>   hw/net/virtio-net: use virtio wrappers to access headers.
>   hw/net/virtio-balloon: use virtio wrappers to access page frame
>     numbers.
>   hw/block/virtio-blk: use virtio wrappers to access headers.
>   hw/scsi/virtio-scsi: use virtio wrappers to access headers.
>   hw/char/virtio-serial-bus: use virtio wrappers to access headers.

 Hi Rusty!

May I ask what's the current status of your virtio endian patches? We
likely need something similar when we enable Virtio v1.0 for S390
virtio-ccw since we then have to byteswap the virtio stuff there, too.
So I recently started to have a look at this... However, in your
patches, the byteswapping seems to be activated/disabled globally, with
the "virtio_byteswap" variable. But with Virtio v1.0, the guest can
decide on a per-device basis whether it wants to drive the device in
v1.0 mode (--> byteswap on S390) or in v0.9 legacy mode (--> no
byteswap), depending on whether it sets the VIRTIO_F_VERSION_1 feature
bit or not. I guess other architectures will have the same problem with
Virtio 1.0, too, when the guests are not running in little endian mode.
So I wonder whether it would it be feasible to change the code so that
the decision of byteswapping or not is done on a per-device basis
instead? What do you think?

 Regards,
  Thomas

Re: [Qemu-devel] [PATCH 0/7] virtio endian-ambivalent target fixes.

[Rusty Russell]

Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> On Thu, 17 Oct 2013 14:23:35 +1030
> Rusty Russell <rusty@rustcorp.com.au> wrote:
>
>> This is a re-transmit of the core of the virtio endian code.  Since
>> there seems to be some interest in ARM BE virtio, I've separated this from
>> the direct problem I was solving: PowerPC LE.
>> 
>> Please apply!
>> Rusty.
>> 
>> Rusty Russell (7):
>>   virtio_get_byteswap: function for endian-ambivalent targets using
>>     virtio.
>>   virtio: allow byte swapping for vring and config access
>>   hw/net/virtio-net: use virtio wrappers to access headers.
>>   hw/net/virtio-balloon: use virtio wrappers to access page frame
>>     numbers.
>>   hw/block/virtio-blk: use virtio wrappers to access headers.
>>   hw/scsi/virtio-scsi: use virtio wrappers to access headers.
>>   hw/char/virtio-serial-bus: use virtio wrappers to access headers.
>
>  Hi Rusty!
>
> May I ask what's the current status of your virtio endian patches? We
> likely need something similar when we enable Virtio v1.0 for S390
> virtio-ccw since we then have to byteswap the virtio stuff there, too.
> So I recently started to have a look at this... However, in your
> patches, the byteswapping seems to be activated/disabled globally, with
> the "virtio_byteswap" variable. But with Virtio v1.0, the guest can
> decide on a per-device basis whether it wants to drive the device in
> v1.0 mode (--> byteswap on S390) or in v0.9 legacy mode (--> no
> byteswap), depending on whether it sets the VIRTIO_F_VERSION_1 feature
> bit or not. I guess other architectures will have the same problem with
> Virtio 1.0, too, when the guests are not running in little endian mode.
> So I wonder whether it would it be feasible to change the code so that
> the decision of byteswapping or not is done on a per-device basis
> instead? What do you think?

Hi Thomas,

        That is definitely the end-goal: these patches are simply to
enable current legacy virtio devices.

Since we missed 1.3, we're supposed to be in 2.0.

Cheers,
Rusty.

Re: [Qemu-devel] [PATCH 0/7] virtio endian-ambivalent target fixes.

[Greg Kurz]

On Thu, 17 Oct 2013 14:23:35 +1030
Rusty Russell <rusty@rustcorp.com.au> wrote:
> This is a re-transmit of the core of the virtio endian code.  Since
> there seems to be some interest in ARM BE virtio, I've separated this from
> the direct problem I was solving: PowerPC LE.
> 
> Please apply!
> Rusty.
> 

Hi,

This serie is needed to enable current legacy virtio devices in a
cross-endian environment. Even though virtio-1.0 will address endianess
questions at the specification level, it is still in its early boot phase
and no code will be available before long (Rusty, please correct me if I am
wrong).

We have all the PPC KVM bits in 3.14 already. We have PPC QEMU patches
ready to be applied by Alexander, as soon as the common code gets in.
Anthony has already positively reviewed this serie. We have been testing
for some monthes... Now we are waiting for partners ! :)

Are the ARM people still interested in cross-endian virtio ? Are there
other people interested (Thomas for s390) ?

If so, please participate or we will have to wait for the new standard to be
finalized and implemented... :-\

Cheers.

--
Greg

> Rusty Russell (7):
>   virtio_get_byteswap: function for endian-ambivalent targets using
>     virtio.
>   virtio: allow byte swapping for vring and config access
>   hw/net/virtio-net: use virtio wrappers to access headers.
>   hw/net/virtio-balloon: use virtio wrappers to access page frame
>     numbers.
>   hw/block/virtio-blk: use virtio wrappers to access headers.
>   hw/scsi/virtio-scsi: use virtio wrappers to access headers.
>   hw/char/virtio-serial-bus: use virtio wrappers to access headers.
> 
>  hw/block/virtio-blk.c             |  35 +++++-----
>  hw/char/virtio-serial-bus.c       |  34 +++++-----
>  hw/net/virtio-net.c               |  15 +++--
>  hw/scsi/virtio-scsi.c             |  33 +++++-----
>  hw/virtio/virtio-balloon.c        |   3 +-
>  hw/virtio/virtio.c                |  34 ++++++----
>  include/hw/virtio/virtio-access.h | 133
> ++++++++++++++++++++++++++++++++++++++ include/hw/virtio/virtio.h
> |   2 + stubs/Makefile.objs               |   1 +
>  stubs/virtio_get_byteswap.c       |   6 ++
>  10 files changed, 225 insertions(+), 71 deletions(-)
>  create mode 100644 include/hw/virtio/virtio-access.h
>  create mode 100644 stubs/virtio_get_byteswap.c
> 



-- 
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 0/7] virtio endian-ambivalent target fixes.

[Andreas Färber]

Hi,

Am 14.02.2014 10:38, schrieb Greg Kurz:
> On Thu, 17 Oct 2013 14:23:35 +1030
> Rusty Russell <rusty@rustcorp.com.au> wrote:
>> This is a re-transmit of the core of the virtio endian code.  Since
>> there seems to be some interest in ARM BE virtio, I've separated this from
>> the direct problem I was solving: PowerPC LE.
>>
>> Please apply!
>> Rusty.
>>
> 
> Hi,
> 
> This serie is needed to enable current legacy virtio devices in a
> cross-endian environment. Even though virtio-1.0 will address endianess
> questions at the specification level, it is still in its early boot phase
> and no code will be available before long (Rusty, please correct me if I am
> wrong).
> 
> We have all the PPC KVM bits in 3.14 already. We have PPC QEMU patches
> ready to be applied by Alexander, as soon as the common code gets in.
> Anthony has already positively reviewed this serie. We have been testing
> for some monthes... Now we are waiting for partners ! :)
> 
> Are the ARM people still interested in cross-endian virtio ? Are there
> other people interested (Thomas for s390) ?
> 
> If so, please participate or we will have to wait for the new standard to be
> finalized and implemented... :-\

It might've helped if Rusty had actually used our
scripts/get_maintainer.pl script to CC people. While Anthony seems to
have reviewed some patches (usually Reviewed-by should be before the
final Signed-off-by fwiw), neither Stefan (virtio-net) nor Kevin
(virtio-blk) nor Paolo (virtio-scsi) were CC'ed, and recently Michael
stepped up as virtio maintainer, so maybe he can take them once ready.

1/7 looks okay to me; 3-7 are rather mechanical - people will need to
review that those changes are sufficient for the current code base.
We've since converted virtio devices to QOM realize, so a rebase is
likely needed for such an "old" series.

Regards,
Andreas

>> Rusty Russell (7):
>>   virtio_get_byteswap: function for endian-ambivalent targets using
>>     virtio.
>>   virtio: allow byte swapping for vring and config access
>>   hw/net/virtio-net: use virtio wrappers to access headers.
>>   hw/net/virtio-balloon: use virtio wrappers to access page frame
>>     numbers.
>>   hw/block/virtio-blk: use virtio wrappers to access headers.
>>   hw/scsi/virtio-scsi: use virtio wrappers to access headers.
>>   hw/char/virtio-serial-bus: use virtio wrappers to access headers.
>>
>>  hw/block/virtio-blk.c             |  35 +++++-----
>>  hw/char/virtio-serial-bus.c       |  34 +++++-----
>>  hw/net/virtio-net.c               |  15 +++--
>>  hw/scsi/virtio-scsi.c             |  33 +++++-----
>>  hw/virtio/virtio-balloon.c        |   3 +-
>>  hw/virtio/virtio.c                |  34 ++++++----
>>  include/hw/virtio/virtio-access.h | 133
>> ++++++++++++++++++++++++++++++++++++++ include/hw/virtio/virtio.h
>> |   2 + stubs/Makefile.objs               |   1 +
>>  stubs/virtio_get_byteswap.c       |   6 ++
>>  10 files changed, 225 insertions(+), 71 deletions(-)
>>  create mode 100644 include/hw/virtio/virtio-access.h
>>  create mode 100644 stubs/virtio_get_byteswap.c

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

Re: [Qemu-devel] [PATCH 0/7] virtio endian-ambivalent target fixes.

[Thomas Huth]

On Fri, 14 Feb 2014 10:38:02 +0100
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> On Thu, 17 Oct 2013 14:23:35 +1030
> Rusty Russell <rusty@rustcorp.com.au> wrote:
> > This is a re-transmit of the core of the virtio endian code.  Since
> > there seems to be some interest in ARM BE virtio, I've separated this from
> > the direct problem I was solving: PowerPC LE.
> > 
> > Please apply!
> > Rusty.
> > 
> 
> Hi,
> 
> This serie is needed to enable current legacy virtio devices in a
> cross-endian environment. Even though virtio-1.0 will address endianess
> questions at the specification level, it is still in its early boot phase
> and no code will be available before long (Rusty, please correct me if I am
> wrong).
> 
> We have all the PPC KVM bits in 3.14 already. We have PPC QEMU patches
> ready to be applied by Alexander, as soon as the common code gets in.
> Anthony has already positively reviewed this serie. We have been testing
> for some monthes... Now we are waiting for partners ! :)
> 
> Are the ARM people still interested in cross-endian virtio ? Are there
> other people interested (Thomas for s390) ?

FYI: s390 is always big-endian, so we currently do not have a problem
yet. However, as soon as the Virtio-1.0 standard materializes, we will
have to swap the ring data on s390, since it is then changed to
little-endian. That's why we are interested in this patch series, too.
So Rusty, if you (or somebody else) could post an updated version of
this patch series, that would be great!

 Regards,
  Thomas

[Qemu-devel] [PATCH 0/8] virtio endian-ambivalent target fixes (rebased)

[Greg Kurz]

On Fri, 14 Feb 2014 12:59:49 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Hi,
> 
> [...]
> 
> It might've helped if Rusty had actually used our
> scripts/get_maintainer.pl script to CC people. While Anthony seems to
> have reviewed some patches (usually Reviewed-by should be before the
> final Signed-off-by fwiw), neither Stefan (virtio-net) nor Kevin
> (virtio-blk) nor Paolo (virtio-scsi) were CC'ed, and recently Michael
> stepped up as virtio maintainer, so maybe he can take them once ready.
> 
> 1/7 looks okay to me; 3-7 are rather mechanical - people will need to
> review that those changes are sufficient for the current code base.
> We've since converted virtio devices to QOM realize, so a rebase is
> likely needed for such an "old" series.
> 
> Regards,
> Andreas

Hi,

Thank you Andreas for your support. :)

Here is a rebased patchset. Only the two first patches ("virtio_get_byteswap:"
and "virtio:") had to be updated actually. FWIW, this the very same patchset
I use, along with some ppc64 specific enablement code, to have functionnal
ppc64 LE guests.

---

Greg Kurz (1):
      hw/9pfs/virtio_9p_device: use virtio wrappers to access headers.

Rusty Russell (7):
      virtio_get_byteswap: function for endian-ambivalent targets using virtio.
      virtio: allow byte swapping for vring and config access
      hw/net/virtio-net: use virtio wrappers to access headers.
      hw/net/virtio-balloon: use virtio wrappers to access page frame numbers.
      hw/block/virtio-blk: use virtio wrappers	to access headers.
      hw/scsi/virtio-scsi: use virtio wrappers	to access headers.
      hw/char/virtio-serial-bus: use virtio wrappers to access headers.


 hw/9pfs/virtio-9p-device.c        |    3 +
 hw/block/virtio-blk.c             |   35 +++++-----
 hw/char/virtio-serial-bus.c       |   34 +++++-----
 hw/net/virtio-net.c               |   15 ++--
 hw/scsi/virtio-scsi.c             |   33 +++++----
 hw/virtio/virtio-balloon.c        |    3 +
 hw/virtio/virtio.c                |   38 ++++++-----
 include/hw/virtio/virtio-access.h |  132 +++++++++++++++++++++++++++++++++++++
 include/hw/virtio/virtio.h        |    2 +
 stubs/Makefile.objs               |    1 
 stubs/virtio_get_byteswap.c       |    6 ++
 11 files changed, 228 insertions(+), 74 deletions(-)
 create mode 100644 include/hw/virtio/virtio-access.h
 create mode 100644 stubs/virtio_get_byteswap.c

Best Regards.

-- 
Greg

[Qemu-devel] [PATCH 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Greg Kurz]

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

virtio data structures are defined as "target endian", which assumes
that's a fixed value.  In fact, that actually means it's
platform-specific.

The OASIS virtio 1.0 spec will fix this.  Meanwhile, create a hook for
little endian ppc (and potentially ARM).  This is called at device
reset time (which is done before any driver is loaded) since it
may involve a system call to get the status when running under kvm.

[ fixed checkpatch.pl error with the virtio_byteswap initialisation,
  ldq_phys() API change, Greg Kurz <gkurz@linux.vnet.ibm.com> ]
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/virtio/virtio.c                |    6 ++
 include/hw/virtio/virtio-access.h |  132 +++++++++++++++++++++++++++++++++++++
 include/hw/virtio/virtio.h        |    2 +
 stubs/Makefile.objs               |    1 
 stubs/virtio_get_byteswap.c       |    6 ++
 5 files changed, 147 insertions(+)
 create mode 100644 include/hw/virtio/virtio-access.h
 create mode 100644 stubs/virtio_get_byteswap.c

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index aeabf3a..4fd6ac2 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -19,6 +19,9 @@
 #include "hw/virtio/virtio.h"
 #include "qemu/atomic.h"
 #include "hw/virtio/virtio-bus.h"
+#include "hw/virtio/virtio-access.h"
+
+bool virtio_byteswap;
 
 /*
  * The alignment to use between consumer and producer parts of vring.
@@ -546,6 +549,9 @@ void virtio_reset(void *opaque)
 
     virtio_set_status(vdev, 0);
 
+    /* We assume all devices are the same endian. */
+    virtio_byteswap = virtio_get_byteswap();
+
     if (k->reset) {
         k->reset(vdev);
     }
diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
new file mode 100644
index 0000000..2e22a47
--- /dev/null
+++ b/include/hw/virtio/virtio-access.h
@@ -0,0 +1,132 @@
+/*
+ * Virtio Accessor Support: In case your target can change endian.
+ *
+ * Copyright IBM, Corp. 2013
+ *
+ * Authors:
+ *  Rusty Russell   <rusty@au.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ */
+#ifndef _QEMU_VIRTIO_ACCESS_H
+#define _QEMU_VIRTIO_ACCESS_H
+#include "hw/virtio/virtio.h"
+
+/* Initialized by virtio_get_byteswap() at any virtio device reset. */
+extern bool virtio_byteswap;
+
+static inline uint16_t virtio_lduw_phys(AddressSpace *as, hwaddr pa)
+{
+    if (virtio_byteswap) {
+        return bswap16(lduw_phys(as, pa));
+    }
+    return lduw_phys(as, pa);
+}
+
+static inline uint32_t virtio_ldl_phys(AddressSpace *as, hwaddr pa)
+{
+    if (virtio_byteswap) {
+        return bswap32(ldl_phys(as, pa));
+    }
+    return ldl_phys(as, pa);
+}
+
+static inline uint64_t virtio_ldq_phys(AddressSpace *as, hwaddr pa)
+{
+    if (virtio_byteswap) {
+        return bswap64(ldq_phys(as, pa));
+    }
+    return ldq_phys(as, pa);
+}
+
+static inline void virtio_stw_phys(AddressSpace *as, hwaddr pa, uint16_t value)
+{
+    if (virtio_byteswap) {
+        stw_phys(as, pa, bswap16(value));
+    } else {
+        stw_phys(as, pa, value);
+    }
+}
+
+static inline void virtio_stl_phys(AddressSpace *as, hwaddr pa, uint32_t value)
+{
+    if (virtio_byteswap) {
+        stl_phys(as, pa, bswap32(value));
+    } else {
+        stl_phys(as, pa, value);
+    }
+}
+
+static inline void virtio_stw_p(void *ptr, uint16_t v)
+{
+    if (virtio_byteswap) {
+        stw_p(ptr, bswap16(v));
+    } else {
+        stw_p(ptr, v);
+    }
+}
+
+static inline void virtio_stl_p(void *ptr, uint32_t v)
+{
+    if (virtio_byteswap) {
+        stl_p(ptr, bswap32(v));
+    } else {
+        stl_p(ptr, v);
+    }
+}
+
+static inline void virtio_stq_p(void *ptr, uint64_t v)
+{
+    if (virtio_byteswap) {
+        stq_p(ptr, bswap64(v));
+    } else {
+        stq_p(ptr, v);
+    }
+}
+
+static inline int virtio_lduw_p(const void *ptr)
+{
+    if (virtio_byteswap) {
+        return bswap16(lduw_p(ptr));
+    } else {
+        return lduw_p(ptr);
+    }
+}
+
+static inline int virtio_ldl_p(const void *ptr)
+{
+    if (virtio_byteswap) {
+        return bswap32(ldl_p(ptr));
+    } else {
+        return ldl_p(ptr);
+    }
+}
+
+static inline uint64_t virtio_ldq_p(const void *ptr)
+{
+    if (virtio_byteswap) {
+        return bswap64(ldq_p(ptr));
+    } else {
+        return ldq_p(ptr);
+    }
+}
+
+static inline uint32_t virtio_tswap32(uint32_t s)
+{
+    if (virtio_byteswap) {
+        return bswap32(tswap32(s));
+    } else {
+        return tswap32(s);
+    }
+}
+
+static inline void virtio_tswap32s(uint32_t *s)
+{
+    tswap32s(s);
+    if (virtio_byteswap) {
+        *s = bswap32(*s);
+    }
+}
+#endif /* _QEMU_VIRTIO_ACCESS_H */
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 3e54e90..5009945 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -253,4 +253,6 @@ 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);
+
+extern bool virtio_get_byteswap(void);
 #endif
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index df92fe5..7e7a9c8 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -27,3 +27,4 @@ stub-obj-y += vm-stop.o
 stub-obj-y += vmstate.o
 stub-obj-$(CONFIG_WIN32) += fd-register.o
 stub-obj-y += cpus.o
+stub-obj-y += virtio_get_byteswap.o
diff --git a/stubs/virtio_get_byteswap.c b/stubs/virtio_get_byteswap.c
new file mode 100644
index 0000000..7cf764d
--- /dev/null
+++ b/stubs/virtio_get_byteswap.c
@@ -0,0 +1,6 @@
+#include "hw/virtio/virtio.h"
+
+bool virtio_get_byteswap(void)
+{
+    return false;
+}

[Qemu-devel] [PATCH 2/8] virtio: allow byte swapping for vring and config access

[Greg Kurz]

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

This is based on a simpler patch by Anthony Liguouri, which only handled
the vring accesses.  We also need some drivers to access these helpers,
eg. for data which contains headers.

[ ldq_phys() API change, Greg Kurz <gkurz@linux.vnet.ibm.com> ]
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/virtio/virtio.c |   32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 4fd6ac2..c2c3a7c 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -108,49 +108,49 @@ static inline uint64_t vring_desc_addr(hwaddr desc_pa, int i)
 {
     hwaddr pa;
     pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, addr);
-    return ldq_phys(&address_space_memory, pa);
+    return virtio_ldq_phys(&address_space_memory, pa);
 }
 
 static inline uint32_t vring_desc_len(hwaddr desc_pa, int i)
 {
     hwaddr pa;
     pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, len);
-    return ldl_phys(&address_space_memory, pa);
+    return virtio_ldl_phys(&address_space_memory, pa);
 }
 
 static inline uint16_t vring_desc_flags(hwaddr desc_pa, int i)
 {
     hwaddr pa;
     pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, flags);
-    return lduw_phys(&address_space_memory, pa);
+    return virtio_lduw_phys(&address_space_memory, pa);
 }
 
 static inline uint16_t vring_desc_next(hwaddr desc_pa, int i)
 {
     hwaddr pa;
     pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, next);
-    return lduw_phys(&address_space_memory, pa);
+    return virtio_lduw_phys(&address_space_memory, pa);
 }
 
 static inline uint16_t vring_avail_flags(VirtQueue *vq)
 {
     hwaddr pa;
     pa = vq->vring.avail + offsetof(VRingAvail, flags);
-    return lduw_phys(&address_space_memory, pa);
+    return virtio_lduw_phys(&address_space_memory, pa);
 }
 
 static inline uint16_t vring_avail_idx(VirtQueue *vq)
 {
     hwaddr pa;
     pa = vq->vring.avail + offsetof(VRingAvail, idx);
-    return lduw_phys(&address_space_memory, pa);
+    return virtio_lduw_phys(&address_space_memory, pa);
 }
 
 static inline uint16_t vring_avail_ring(VirtQueue *vq, int i)
 {
     hwaddr pa;
     pa = vq->vring.avail + offsetof(VRingAvail, ring[i]);
-    return lduw_phys(&address_space_memory, pa);
+    return virtio_lduw_phys(&address_space_memory, pa);
 }
 
 static inline uint16_t vring_used_event(VirtQueue *vq)
@@ -162,44 +162,44 @@ static inline void vring_used_ring_id(VirtQueue *vq, int i, uint32_t val)
 {
     hwaddr pa;
     pa = vq->vring.used + offsetof(VRingUsed, ring[i].id);
-    stl_phys(&address_space_memory, pa, val);
+    virtio_stl_phys(&address_space_memory, pa, val);
 }
 
 static inline void vring_used_ring_len(VirtQueue *vq, int i, uint32_t val)
 {
     hwaddr pa;
     pa = vq->vring.used + offsetof(VRingUsed, ring[i].len);
-    stl_phys(&address_space_memory, pa, val);
+    virtio_stl_phys(&address_space_memory, pa, val);
 }
 
 static uint16_t vring_used_idx(VirtQueue *vq)
 {
     hwaddr pa;
     pa = vq->vring.used + offsetof(VRingUsed, idx);
-    return lduw_phys(&address_space_memory, pa);
+    return virtio_lduw_phys(&address_space_memory, pa);
 }
 
 static inline void vring_used_idx_set(VirtQueue *vq, uint16_t val)
 {
     hwaddr pa;
     pa = vq->vring.used + offsetof(VRingUsed, idx);
-    stw_phys(&address_space_memory, pa, val);
+    virtio_stw_phys(&address_space_memory, pa, val);
 }
 
 static inline void vring_used_flags_set_bit(VirtQueue *vq, int mask)
 {
     hwaddr pa;
     pa = vq->vring.used + offsetof(VRingUsed, flags);
-    stw_phys(&address_space_memory,
-             pa, lduw_phys(&address_space_memory, pa) | mask);
+    virtio_stw_phys(&address_space_memory,
+             pa, virtio_lduw_phys(&address_space_memory, pa) | mask);
 }
 
 static inline void vring_used_flags_unset_bit(VirtQueue *vq, int mask)
 {
     hwaddr pa;
     pa = vq->vring.used + offsetof(VRingUsed, flags);
-    stw_phys(&address_space_memory,
-             pa, lduw_phys(&address_space_memory, pa) & ~mask);
+    virtio_stw_phys(&address_space_memory,
+                    pa, lduw_phys(&address_space_memory, pa) & ~mask);
 }
 
 static inline void vring_avail_event(VirtQueue *vq, uint16_t val)
@@ -209,7 +209,7 @@ static inline void vring_avail_event(VirtQueue *vq, uint16_t val)
         return;
     }
     pa = vq->vring.used + offsetof(VRingUsed, ring[vq->vring.num]);
-    stw_phys(&address_space_memory, pa, val);
+    virtio_stw_phys(&address_space_memory, pa, val);
 }
 
 void virtio_queue_set_notification(VirtQueue *vq, int enable)

[Qemu-devel] [PATCH 3/8] hw/net/virtio-net: use virtio wrappers to access headers.

[Greg Kurz]

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

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/net/virtio-net.c |   15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 3626608..34d6b48 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -23,6 +23,7 @@
 #include "hw/virtio/virtio-bus.h"
 #include "qapi/qmp/qjson.h"
 #include "monitor/monitor.h"
+#include "hw/virtio/virtio-access.h"
 
 #define VIRTIO_NET_VM_VERSION    11
 
@@ -72,8 +73,8 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
     VirtIONet *n = VIRTIO_NET(vdev);
     struct virtio_net_config netcfg;
 
-    stw_p(&netcfg.status, n->status);
-    stw_p(&netcfg.max_virtqueue_pairs, n->max_queues);
+    virtio_stw_p(&netcfg.status, n->status);
+    virtio_stw_p(&netcfg.max_virtqueue_pairs, n->max_queues);
     memcpy(netcfg.mac, n->mac, ETH_ALEN);
     memcpy(config, &netcfg, n->config_size);
 }
@@ -618,7 +619,7 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
 
     s = iov_to_buf(iov, iov_cnt, 0, &mac_data.entries,
                    sizeof(mac_data.entries));
-    mac_data.entries = ldl_p(&mac_data.entries);
+    mac_data.entries = virtio_ldl_p(&mac_data.entries);
     if (s != sizeof(mac_data.entries)) {
         goto error;
     }
@@ -645,7 +646,7 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
 
     s = iov_to_buf(iov, iov_cnt, 0, &mac_data.entries,
                    sizeof(mac_data.entries));
-    mac_data.entries = ldl_p(&mac_data.entries);
+    mac_data.entries = virtio_ldl_p(&mac_data.entries);
     if (s != sizeof(mac_data.entries)) {
         goto error;
     }
@@ -690,7 +691,7 @@ static int virtio_net_handle_vlan_table(VirtIONet *n, uint8_t cmd,
     NetClientState *nc = qemu_get_queue(n->nic);
 
     s = iov_to_buf(iov, iov_cnt, 0, &vid, sizeof(vid));
-    vid = lduw_p(&vid);
+    vid = virtio_lduw_p(&vid);
     if (s != sizeof(vid)) {
         return VIRTIO_NET_ERR;
     }
@@ -727,7 +728,7 @@ static int virtio_net_handle_mq(VirtIONet *n, uint8_t cmd,
         return VIRTIO_NET_ERR;
     }
 
-    queues = lduw_p(&mq.virtqueue_pairs);
+    queues = virtio_lduw_p(&mq.virtqueue_pairs);
 
     if (queues < VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN ||
         queues > VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX ||
@@ -1026,7 +1027,7 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t
     }
 
     if (mhdr_cnt) {
-        stw_p(&mhdr.num_buffers, i);
+        virtio_stw_p(&mhdr.num_buffers, i);
         iov_from_buf(mhdr_sg, mhdr_cnt,
                      0,
                      &mhdr.num_buffers, sizeof mhdr.num_buffers);

[Qemu-devel] [PATCH 4/8] hw/net/virtio-balloon: use virtio wrappers to access page frame numbers.

[Greg Kurz]

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

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/virtio/virtio-balloon.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index a470a0b..5e7f26f 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -30,6 +30,7 @@
 #endif
 
 #include "hw/virtio/virtio-bus.h"
+#include "hw/virtio/virtio-access.h"
 
 static void balloon_page(void *addr, int deflate)
 {
@@ -192,7 +193,7 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
             ram_addr_t pa;
             ram_addr_t addr;
 
-            pa = (ram_addr_t)ldl_p(&pfn) << VIRTIO_BALLOON_PFN_SHIFT;
+            pa = (ram_addr_t)virtio_ldl_p(&pfn) << VIRTIO_BALLOON_PFN_SHIFT;
             offset += 4;
 
             /* FIXME: remove get_system_memory(), but how? */

[Qemu-devel] [PATCH 5/8] hw/block/virtio-blk: use virtio wrappers to access headers.

[Greg Kurz]

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

Note that st*_raw and ld*_raw are effectively replaced by st*_p and ld*_p.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/block/virtio-blk.c |   35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 8a568e5..f55f89c 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -26,6 +26,7 @@
 # include <scsi/sg.h>
 #endif
 #include "hw/virtio/virtio-bus.h"
+#include "hw/virtio/virtio-access.h"
 
 typedef struct VirtIOBlockReq
 {
@@ -77,7 +78,7 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
     trace_virtio_blk_rw_complete(req, ret);
 
     if (ret) {
-        bool is_read = !(ldl_p(&req->out->type) & VIRTIO_BLK_T_OUT);
+        bool is_read = !(virtio_ldl_p(&req->out->type) & VIRTIO_BLK_T_OUT);
         if (virtio_blk_handle_rw_error(req, -ret, is_read))
             return;
     }
@@ -224,12 +225,12 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
         hdr.status = CHECK_CONDITION;
     }
 
-    stl_p(&req->scsi->errors,
-          hdr.status | (hdr.msg_status << 8) |
-          (hdr.host_status << 16) | (hdr.driver_status << 24));
-    stl_p(&req->scsi->residual, hdr.resid);
-    stl_p(&req->scsi->sense_len, hdr.sb_len_wr);
-    stl_p(&req->scsi->data_len, hdr.dxfer_len);
+    virtio_stl_p(&req->scsi->errors,
+                 hdr.status | (hdr.msg_status << 8) |
+                 (hdr.host_status << 16) | (hdr.driver_status << 24));
+    virtio_stl_p(&req->scsi->residual, hdr.resid);
+    virtio_stl_p(&req->scsi->sense_len, hdr.sb_len_wr);
+    virtio_stl_p(&req->scsi->data_len, hdr.dxfer_len);
 
     virtio_blk_req_complete(req, status);
     g_free(req);
@@ -240,7 +241,7 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
 
 fail:
     /* Just put anything nonzero so that the ioctl fails in the guest.  */
-    stl_p(&req->scsi->errors, 255);
+    virtio_stl_p(&req->scsi->errors, 255);
     virtio_blk_req_complete(req, status);
     g_free(req);
 }
@@ -286,7 +287,7 @@ static void virtio_blk_handle_write(VirtIOBlockReq *req, MultiReqBuffer *mrb)
     BlockRequest *blkreq;
     uint64_t sector;
 
-    sector = ldq_p(&req->out->sector);
+    sector = virtio_ldq_p(&req->out->sector);
 
     bdrv_acct_start(req->dev->bs, &req->acct, req->qiov.size, BDRV_ACCT_WRITE);
 
@@ -320,7 +321,7 @@ static void virtio_blk_handle_read(VirtIOBlockReq *req)
 {
     uint64_t sector;
 
-    sector = ldq_p(&req->out->sector);
+    sector = virtio_ldq_p(&req->out->sector);
 
     bdrv_acct_start(req->dev->bs, &req->acct, req->qiov.size, BDRV_ACCT_READ);
 
@@ -358,7 +359,7 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req,
     req->out = (void *)req->elem.out_sg[0].iov_base;
     req->in = (void *)req->elem.in_sg[req->elem.in_num - 1].iov_base;
 
-    type = ldl_p(&req->out->type);
+    type = virtio_ldl_p(&req->out->type);
 
     if (type & VIRTIO_BLK_T_FLUSH) {
         virtio_blk_handle_flush(req, mrb);
@@ -487,12 +488,12 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
 
     bdrv_get_geometry(s->bs, &capacity);
     memset(&blkcfg, 0, sizeof(blkcfg));
-    stq_raw(&blkcfg.capacity, capacity);
-    stl_raw(&blkcfg.seg_max, 128 - 2);
-    stw_raw(&blkcfg.cylinders, s->conf->cyls);
-    stl_raw(&blkcfg.blk_size, blk_size);
-    stw_raw(&blkcfg.min_io_size, s->conf->min_io_size / blk_size);
-    stw_raw(&blkcfg.opt_io_size, s->conf->opt_io_size / blk_size);
+    virtio_stq_p(&blkcfg.capacity, capacity);
+    virtio_stl_p(&blkcfg.seg_max, 128 - 2);
+    virtio_stw_p(&blkcfg.cylinders, s->conf->cyls);
+    virtio_stl_p(&blkcfg.blk_size, blk_size);
+    virtio_stw_p(&blkcfg.min_io_size, s->conf->min_io_size / blk_size);
+    virtio_stw_p(&blkcfg.opt_io_size, s->conf->opt_io_size / blk_size);
     blkcfg.heads = s->conf->heads;
     /*
      * We must ensure that the block device capacity is a multiple of

[Qemu-devel] [PATCH 6/8] hw/scsi/virtio-scsi: use virtio wrappers to access headers.

[Greg Kurz]

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

Note that st*_raw and ld*_raw are effectively replaced by st*_p and ld*_p.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/scsi/virtio-scsi.c |   33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 6610b3a..df6c0e2 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -18,6 +18,7 @@
 #include <hw/scsi/scsi.h>
 #include <block/scsi.h>
 #include <hw/virtio/virtio-bus.h>
+#include "hw/virtio/virtio-access.h"
 
 typedef struct VirtIOSCSIReq {
     VirtIOSCSI *dev;
@@ -313,12 +314,12 @@ static void virtio_scsi_command_complete(SCSIRequest *r, uint32_t status,
     req->resp.cmd->response = VIRTIO_SCSI_S_OK;
     req->resp.cmd->status = status;
     if (req->resp.cmd->status == GOOD) {
-        req->resp.cmd->resid = tswap32(resid);
+        req->resp.cmd->resid = virtio_tswap32(resid);
     } else {
         req->resp.cmd->resid = 0;
         sense_len = scsi_req_get_sense(r, req->resp.cmd->sense,
                                        VIRTIO_SCSI_SENSE_SIZE);
-        req->resp.cmd->sense_len = tswap32(sense_len);
+        req->resp.cmd->sense_len = virtio_tswap32(sense_len);
     }
     virtio_scsi_complete_req(req);
 }
@@ -414,16 +415,16 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
     VirtIOSCSIConfig *scsiconf = (VirtIOSCSIConfig *)config;
     VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
 
-    stl_raw(&scsiconf->num_queues, s->conf.num_queues);
-    stl_raw(&scsiconf->seg_max, 128 - 2);
-    stl_raw(&scsiconf->max_sectors, s->conf.max_sectors);
-    stl_raw(&scsiconf->cmd_per_lun, s->conf.cmd_per_lun);
-    stl_raw(&scsiconf->event_info_size, sizeof(VirtIOSCSIEvent));
-    stl_raw(&scsiconf->sense_size, s->sense_size);
-    stl_raw(&scsiconf->cdb_size, s->cdb_size);
-    stw_raw(&scsiconf->max_channel, VIRTIO_SCSI_MAX_CHANNEL);
-    stw_raw(&scsiconf->max_target, VIRTIO_SCSI_MAX_TARGET);
-    stl_raw(&scsiconf->max_lun, VIRTIO_SCSI_MAX_LUN);
+    virtio_stl_p(&scsiconf->num_queues, s->conf.num_queues);
+    virtio_stl_p(&scsiconf->seg_max, 128 - 2);
+    virtio_stl_p(&scsiconf->max_sectors, s->conf.max_sectors);
+    virtio_stl_p(&scsiconf->cmd_per_lun, s->conf.cmd_per_lun);
+    virtio_stl_p(&scsiconf->event_info_size, sizeof(VirtIOSCSIEvent));
+    virtio_stl_p(&scsiconf->sense_size, s->sense_size);
+    virtio_stl_p(&scsiconf->cdb_size, s->cdb_size);
+    virtio_stw_p(&scsiconf->max_channel, VIRTIO_SCSI_MAX_CHANNEL);
+    virtio_stw_p(&scsiconf->max_target, VIRTIO_SCSI_MAX_TARGET);
+    virtio_stl_p(&scsiconf->max_lun, VIRTIO_SCSI_MAX_LUN);
 }
 
 static void virtio_scsi_set_config(VirtIODevice *vdev,
@@ -432,14 +433,14 @@ static void virtio_scsi_set_config(VirtIODevice *vdev,
     VirtIOSCSIConfig *scsiconf = (VirtIOSCSIConfig *)config;
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
 
-    if ((uint32_t) ldl_raw(&scsiconf->sense_size) >= 65536 ||
-        (uint32_t) ldl_raw(&scsiconf->cdb_size) >= 256) {
+    if ((uint32_t) virtio_ldl_p(&scsiconf->sense_size) >= 65536 ||
+        (uint32_t) virtio_ldl_p(&scsiconf->cdb_size) >= 256) {
         error_report("bad data written to virtio-scsi configuration space");
         exit(1);
     }
 
-    vs->sense_size = ldl_raw(&scsiconf->sense_size);
-    vs->cdb_size = ldl_raw(&scsiconf->cdb_size);
+    vs->sense_size = virtio_ldl_p(&scsiconf->sense_size);
+    vs->cdb_size = virtio_ldl_p(&scsiconf->cdb_size);
 }
 
 static uint32_t virtio_scsi_get_features(VirtIODevice *vdev,

[Qemu-devel] [PATCH 7/8] hw/char/virtio-serial-bus: use virtio wrappers to access headers.

[Greg Kurz]

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

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/char/virtio-serial-bus.c |   34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 226e9f9..243bf31 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -24,6 +24,7 @@
 #include "hw/sysbus.h"
 #include "trace.h"
 #include "hw/virtio/virtio-serial.h"
+#include "hw/virtio/virtio-access.h"
 
 static VirtIOSerialPort *find_port_by_id(VirtIOSerial *vser, uint32_t id)
 {
@@ -185,9 +186,9 @@ static size_t send_control_event(VirtIOSerial *vser, uint32_t port_id,
 {
     struct virtio_console_control cpkt;
 
-    stl_p(&cpkt.id, port_id);
-    stw_p(&cpkt.event, event);
-    stw_p(&cpkt.value, value);
+    virtio_stl_p(&cpkt.id, port_id);
+    virtio_stw_p(&cpkt.event, event);
+    virtio_stw_p(&cpkt.value, value);
 
     trace_virtio_serial_send_control_event(port_id, event, value);
     return send_control_msg(vser, &cpkt, sizeof(cpkt));
@@ -291,8 +292,8 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
         return;
     }
 
-    cpkt.event = lduw_p(&gcpkt->event);
-    cpkt.value = lduw_p(&gcpkt->value);
+    cpkt.event = virtio_lduw_p(&gcpkt->event);
+    cpkt.value = virtio_lduw_p(&gcpkt->value);
 
     trace_virtio_serial_handle_control_message(cpkt.event, cpkt.value);
 
@@ -312,10 +313,10 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
         return;
     }
 
-    port = find_port_by_id(vser, ldl_p(&gcpkt->id));
+    port = find_port_by_id(vser, virtio_ldl_p(&gcpkt->id));
     if (!port) {
         error_report("virtio-serial-bus: Unexpected port id %u for device %s",
-                     ldl_p(&gcpkt->id), vser->bus.qbus.name);
+                     virtio_ldl_p(&gcpkt->id), vser->bus.qbus.name);
         return;
     }
 
@@ -342,9 +343,9 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
         }
 
         if (port->name) {
-            stl_p(&cpkt.id, port->id);
-            stw_p(&cpkt.event, VIRTIO_CONSOLE_PORT_NAME);
-            stw_p(&cpkt.value, 1);
+            virtio_stl_p(&cpkt.id, port->id);
+            virtio_stw_p(&cpkt.event, VIRTIO_CONSOLE_PORT_NAME);
+            virtio_stw_p(&cpkt.value, 1);
 
             buffer_len = sizeof(cpkt) + strlen(port->name) + 1;
             buffer = g_malloc(buffer_len);
@@ -536,7 +537,7 @@ static void virtio_serial_save(QEMUFile *f, void *opaque)
     qemu_put_be32s(f, &s->config.max_nr_ports);
 
     /* The ports map */
-    max_nr_ports = tswap32(s->config.max_nr_ports);
+    max_nr_ports = virtio_tswap32(s->config.max_nr_ports);
     for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
         qemu_put_be32s(f, &s->ports_map[i]);
     }
@@ -690,8 +691,8 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
     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)) {
+    virtio_tswap32s(&max_nr_ports);
+    if (max_nr_ports > virtio_tswap32(s->config.max_nr_ports)) {
         /* Source could have had more ports than us. Fail migration. */
         return -EINVAL;
     }
@@ -760,7 +761,7 @@ static uint32_t find_free_port_id(VirtIOSerial *vser)
 {
     unsigned int i, max_nr_ports;
 
-    max_nr_ports = tswap32(vser->config.max_nr_ports);
+    max_nr_ports = virtio_tswap32(vser->config.max_nr_ports);
     for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
         uint32_t map, bit;
 
@@ -846,7 +847,7 @@ static int virtser_port_qdev_init(DeviceState *qdev)
         }
     }
 
-    max_nr_ports = tswap32(port->vser->config.max_nr_ports);
+    max_nr_ports = virtio_tswap32(port->vser->config.max_nr_ports);
     if (port->id >= max_nr_ports) {
         error_report("virtio-serial-bus: Out-of-range port id specified, max. allowed: %u",
                      max_nr_ports - 1);
@@ -949,7 +950,8 @@ static void virtio_serial_device_realize(DeviceState *dev, Error **errp)
         vser->ovqs[i] = virtio_add_queue(vdev, 128, handle_output);
     }
 
-    vser->config.max_nr_ports = tswap32(vser->serial.max_virtserial_ports);
+    vser->config.max_nr_ports =
+        virtio_tswap32(vser->serial.max_virtserial_ports);
     vser->ports_map = g_malloc0(((vser->serial.max_virtserial_ports + 31) / 32)
         * sizeof(vser->ports_map[0]));
     /*

[Qemu-devel] [PATCH 8/8] hw/9pfs/virtio_9p_device: use virtio wrappers to access headers.

[Greg Kurz]

Note that st*_raw and ld*_raw are effectively replaced by st*_p and ld*_p.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/9pfs/virtio-9p-device.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index 15a4983..b3161ec 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -19,6 +19,7 @@
 #include "fsdev/qemu-fsdev.h"
 #include "virtio-9p-xattr.h"
 #include "virtio-9p-coth.h"
+#include "hw/virtio/virtio-access.h"
 
 static uint32_t virtio_9p_get_features(VirtIODevice *vdev, uint32_t features)
 {
@@ -34,7 +35,7 @@ static void virtio_9p_get_config(VirtIODevice *vdev, uint8_t *config)
 
     len = strlen(s->tag);
     cfg = g_malloc0(sizeof(struct virtio_9p_config) + len);
-    stw_raw(&cfg->tag_len, len);
+    virtio_stw_p(&cfg->tag_len, len);
     /* We don't copy the terminating null to config space */
     memcpy(cfg->tag, s->tag, len);
     memcpy(config, cfg, s->config_size);

Re: [Qemu-devel] [PATCH 2/8] virtio: allow byte swapping for vring and config access

[Cornelia Huck]

On Tue, 18 Feb 2014 13:38:54 +0100
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> From: Rusty Russell <rusty@rustcorp.com.au>
> 
> This is based on a simpler patch by Anthony Liguouri, which only handled
> the vring accesses.  We also need some drivers to access these helpers,
> eg. for data which contains headers.
> 
> [ ldq_phys() API change, Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  hw/virtio/virtio.c |   32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 

>  static inline void vring_used_flags_unset_bit(VirtQueue *vq, int mask)
>  {
>      hwaddr pa;
>      pa = vq->vring.used + offsetof(VRingUsed, flags);
> -    stw_phys(&address_space_memory,
> -             pa, lduw_phys(&address_space_memory, pa) & ~mask);
> +    virtio_stw_phys(&address_space_memory,
> +                    pa, lduw_phys(&address_space_memory, pa) & ~mask);
>  }

This needs to be virtio_lduw_phys(), no?

Re: [Qemu-devel] [PATCH 2/8] virtio: allow byte swapping for vring and config access

[Greg Kurz]

On Tue, 18 Feb 2014 14:08:35 +0100
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> On Tue, 18 Feb 2014 13:38:54 +0100
> Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> 
> > From: Rusty Russell <rusty@rustcorp.com.au>
> > 
> > This is based on a simpler patch by Anthony Liguouri, which only handled
> > the vring accesses.  We also need some drivers to access these helpers,
> > eg. for data which contains headers.
> > 
> > [ ldq_phys() API change, Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> >  hw/virtio/virtio.c |   32 ++++++++++++++++----------------
> >  1 file changed, 16 insertions(+), 16 deletions(-)
> > 
> 
> >  static inline void vring_used_flags_unset_bit(VirtQueue *vq, int mask)
> >  {
> >      hwaddr pa;
> >      pa = vq->vring.used + offsetof(VRingUsed, flags);
> > -    stw_phys(&address_space_memory,
> > -             pa, lduw_phys(&address_space_memory, pa) & ~mask);
> > +    virtio_stw_phys(&address_space_memory,
> > +                    pa, lduw_phys(&address_space_memory, pa) & ~mask);
> >  }
> 
> This needs to be virtio_lduw_phys(), no?

Oops yes it should be... my mistake. :) 

-- 
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 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Alexander Graf]

On 02/18/2014 01:38 PM, Greg Kurz wrote:
> From: Rusty Russell <rusty@rustcorp.com.au>
>
> virtio data structures are defined as "target endian", which assumes
> that's a fixed value.  In fact, that actually means it's
> platform-specific.
>
> The OASIS virtio 1.0 spec will fix this.  Meanwhile, create a hook for
> little endian ppc (and potentially ARM).  This is called at device
> reset time (which is done before any driver is loaded) since it
> may involve a system call to get the status when running under kvm.
>
> [ fixed checkpatch.pl error with the virtio_byteswap initialisation,
>    ldq_phys() API change, Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>   hw/virtio/virtio.c                |    6 ++
>   include/hw/virtio/virtio-access.h |  132 +++++++++++++++++++++++++++++++++++++
>   include/hw/virtio/virtio.h        |    2 +
>   stubs/Makefile.objs               |    1
>   stubs/virtio_get_byteswap.c       |    6 ++
>   5 files changed, 147 insertions(+)
>   create mode 100644 include/hw/virtio/virtio-access.h
>   create mode 100644 stubs/virtio_get_byteswap.c
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index aeabf3a..4fd6ac2 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -19,6 +19,9 @@
>   #include "hw/virtio/virtio.h"
>   #include "qemu/atomic.h"
>   #include "hw/virtio/virtio-bus.h"
> +#include "hw/virtio/virtio-access.h"
> +
> +bool virtio_byteswap;

Could this be a virtio object property rather than a global? Imagine an 
AMP guest system with a BE and an LE system running in parallel 
accessing two separate virtio devices. With a single global that would 
break.


Alex

Re: [Qemu-devel] [PATCH 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Alexander Graf]

On 02/18/2014 04:03 PM, Michael S. Tsirkin wrote:
> On Tue, Feb 18, 2014 at 03:48:38PM +0100, Alexander Graf wrote:
>> On 02/18/2014 01:38 PM, Greg Kurz wrote:
>>> From: Rusty Russell <rusty@rustcorp.com.au>
>>>
>>> virtio data structures are defined as "target endian", which assumes
>>> that's a fixed value.  In fact, that actually means it's
>>> platform-specific.
>>>
>>> The OASIS virtio 1.0 spec will fix this.  Meanwhile, create a hook for
>>> little endian ppc (and potentially ARM).  This is called at device
>>> reset time (which is done before any driver is loaded) since it
>>> may involve a system call to get the status when running under kvm.
>>>
>>> [ fixed checkpatch.pl error with the virtio_byteswap initialisation,
>>>    ldq_phys() API change, Greg Kurz <gkurz@linux.vnet.ibm.com> ]
>>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
>>> ---
>>>   hw/virtio/virtio.c                |    6 ++
>>>   include/hw/virtio/virtio-access.h |  132 +++++++++++++++++++++++++++++++++++++
>>>   include/hw/virtio/virtio.h        |    2 +
>>>   stubs/Makefile.objs               |    1
>>>   stubs/virtio_get_byteswap.c       |    6 ++
>>>   5 files changed, 147 insertions(+)
>>>   create mode 100644 include/hw/virtio/virtio-access.h
>>>   create mode 100644 stubs/virtio_get_byteswap.c
>>>
>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>> index aeabf3a..4fd6ac2 100644
>>> --- a/hw/virtio/virtio.c
>>> +++ b/hw/virtio/virtio.c
>>> @@ -19,6 +19,9 @@
>>>   #include "hw/virtio/virtio.h"
>>>   #include "qemu/atomic.h"
>>>   #include "hw/virtio/virtio-bus.h"
>>> +#include "hw/virtio/virtio-access.h"
>>> +
>>> +bool virtio_byteswap;
>> Could this be a virtio object property rather than a global? Imagine
>> an AMP guest system with a BE and an LE system running in parallel
>> accessing two separate virtio devices. With a single global that
>> would break.
>>
>>
>> Alex
> Well, how does a device know which CPU uses it?
> I suspect we are better off waiting for 1.0 with this one.

Good point. It just feels like a heavy layering violation to have a 
global variable for all virtio devices. What if we start mixing legacy 
and 1.0 devices? We could reuse the same flag, but it would be 
initialized differently per device.


Alex

Re: [Qemu-devel] [PATCH 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Michael S. Tsirkin]

On Tue, Feb 18, 2014 at 03:48:38PM +0100, Alexander Graf wrote:
> On 02/18/2014 01:38 PM, Greg Kurz wrote:
> >From: Rusty Russell <rusty@rustcorp.com.au>
> >
> >virtio data structures are defined as "target endian", which assumes
> >that's a fixed value.  In fact, that actually means it's
> >platform-specific.
> >
> >The OASIS virtio 1.0 spec will fix this.  Meanwhile, create a hook for
> >little endian ppc (and potentially ARM).  This is called at device
> >reset time (which is done before any driver is loaded) since it
> >may involve a system call to get the status when running under kvm.
> >
> >[ fixed checkpatch.pl error with the virtio_byteswap initialisation,
> >   ldq_phys() API change, Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> >Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> >Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> >---
> >  hw/virtio/virtio.c                |    6 ++
> >  include/hw/virtio/virtio-access.h |  132 +++++++++++++++++++++++++++++++++++++
> >  include/hw/virtio/virtio.h        |    2 +
> >  stubs/Makefile.objs               |    1
> >  stubs/virtio_get_byteswap.c       |    6 ++
> >  5 files changed, 147 insertions(+)
> >  create mode 100644 include/hw/virtio/virtio-access.h
> >  create mode 100644 stubs/virtio_get_byteswap.c
> >
> >diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >index aeabf3a..4fd6ac2 100644
> >--- a/hw/virtio/virtio.c
> >+++ b/hw/virtio/virtio.c
> >@@ -19,6 +19,9 @@
> >  #include "hw/virtio/virtio.h"
> >  #include "qemu/atomic.h"
> >  #include "hw/virtio/virtio-bus.h"
> >+#include "hw/virtio/virtio-access.h"
> >+
> >+bool virtio_byteswap;
> 
> Could this be a virtio object property rather than a global? Imagine
> an AMP guest system with a BE and an LE system running in parallel
> accessing two separate virtio devices. With a single global that
> would break.
> 
> 
> Alex

Well, how does a device know which CPU uses it?
I suspect we are better off waiting for 1.0 with this one.

-- 
MST

Re: [Qemu-devel] [PATCH 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Michael S. Tsirkin]

On Tue, Feb 18, 2014 at 05:03:27PM +0200, Michael S. Tsirkin wrote:
> On Tue, Feb 18, 2014 at 03:48:38PM +0100, Alexander Graf wrote:
> > On 02/18/2014 01:38 PM, Greg Kurz wrote:
> > >From: Rusty Russell <rusty@rustcorp.com.au>
> > >
> > >virtio data structures are defined as "target endian", which assumes
> > >that's a fixed value.  In fact, that actually means it's
> > >platform-specific.
> > >
> > >The OASIS virtio 1.0 spec will fix this.  Meanwhile, create a hook for
> > >little endian ppc (and potentially ARM).  This is called at device
> > >reset time (which is done before any driver is loaded) since it
> > >may involve a system call to get the status when running under kvm.
> > >
> > >[ fixed checkpatch.pl error with the virtio_byteswap initialisation,
> > >   ldq_phys() API change, Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> > >Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> > >Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > >---
> > >  hw/virtio/virtio.c                |    6 ++
> > >  include/hw/virtio/virtio-access.h |  132 +++++++++++++++++++++++++++++++++++++
> > >  include/hw/virtio/virtio.h        |    2 +
> > >  stubs/Makefile.objs               |    1
> > >  stubs/virtio_get_byteswap.c       |    6 ++
> > >  5 files changed, 147 insertions(+)
> > >  create mode 100644 include/hw/virtio/virtio-access.h
> > >  create mode 100644 stubs/virtio_get_byteswap.c
> > >
> > >diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > >index aeabf3a..4fd6ac2 100644
> > >--- a/hw/virtio/virtio.c
> > >+++ b/hw/virtio/virtio.c
> > >@@ -19,6 +19,9 @@
> > >  #include "hw/virtio/virtio.h"
> > >  #include "qemu/atomic.h"
> > >  #include "hw/virtio/virtio-bus.h"
> > >+#include "hw/virtio/virtio-access.h"
> > >+
> > >+bool virtio_byteswap;
> > 
> > Could this be a virtio object property rather than a global? Imagine
> > an AMP guest system with a BE and an LE system running in parallel
> > accessing two separate virtio devices. With a single global that
> > would break.
> > 
> > 
> > Alex
> 
> Well, how does a device know which CPU uses it?
> I suspect we are better off waiting for 1.0 with this one.

I mean an AMP guest system with a BE and an LE system here.
Not this patchset itself.

> -- 
> MST

Re: [Qemu-devel] [PATCH 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Alexander Graf]

On 02/18/2014 04:11 PM, Michael S. Tsirkin wrote:
> On Tue, Feb 18, 2014 at 04:02:18PM +0100, Alexander Graf wrote:
>> On 02/18/2014 04:03 PM, Michael S. Tsirkin wrote:
>>> On Tue, Feb 18, 2014 at 03:48:38PM +0100, Alexander Graf wrote:
>>>> On 02/18/2014 01:38 PM, Greg Kurz wrote:
>>>>> From: Rusty Russell <rusty@rustcorp.com.au>
>>>>>
>>>>> virtio data structures are defined as "target endian", which assumes
>>>>> that's a fixed value.  In fact, that actually means it's
>>>>> platform-specific.
>>>>>
>>>>> The OASIS virtio 1.0 spec will fix this.  Meanwhile, create a hook for
>>>>> little endian ppc (and potentially ARM).  This is called at device
>>>>> reset time (which is done before any driver is loaded) since it
>>>>> may involve a system call to get the status when running under kvm.
>>>>>
>>>>> [ fixed checkpatch.pl error with the virtio_byteswap initialisation,
>>>>>    ldq_phys() API change, Greg Kurz <gkurz@linux.vnet.ibm.com> ]
>>>>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>>>>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
>>>>> ---
>>>>>   hw/virtio/virtio.c                |    6 ++
>>>>>   include/hw/virtio/virtio-access.h |  132 +++++++++++++++++++++++++++++++++++++
>>>>>   include/hw/virtio/virtio.h        |    2 +
>>>>>   stubs/Makefile.objs               |    1
>>>>>   stubs/virtio_get_byteswap.c       |    6 ++
>>>>>   5 files changed, 147 insertions(+)
>>>>>   create mode 100644 include/hw/virtio/virtio-access.h
>>>>>   create mode 100644 stubs/virtio_get_byteswap.c
>>>>>
>>>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>>>> index aeabf3a..4fd6ac2 100644
>>>>> --- a/hw/virtio/virtio.c
>>>>> +++ b/hw/virtio/virtio.c
>>>>> @@ -19,6 +19,9 @@
>>>>>   #include "hw/virtio/virtio.h"
>>>>>   #include "qemu/atomic.h"
>>>>>   #include "hw/virtio/virtio-bus.h"
>>>>> +#include "hw/virtio/virtio-access.h"
>>>>> +
>>>>> +bool virtio_byteswap;
>>>> Could this be a virtio object property rather than a global? Imagine
>>>> an AMP guest system with a BE and an LE system running in parallel
>>>> accessing two separate virtio devices. With a single global that
>>>> would break.
>>>>
>>>>
>>>> Alex
>>> Well, how does a device know which CPU uses it?
>>> I suspect we are better off waiting for 1.0 with this one.
>> Good point. It just feels like a heavy layering violation to have a
>> global variable for all virtio devices. What if we start mixing
>> legacy and 1.0 devices? We could reuse the same flag, but it would
>> be initialized differently per device.
>>
>>
>> Alex
> 1.0 won't use the flag, it's all LE.

At which point we need another flag indicating that the fields are 
always LE which means we can reuse this flag, no?


Alex

Re: [Qemu-devel] [PATCH 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Michael S. Tsirkin]

On Tue, Feb 18, 2014 at 04:02:18PM +0100, Alexander Graf wrote:
> On 02/18/2014 04:03 PM, Michael S. Tsirkin wrote:
> >On Tue, Feb 18, 2014 at 03:48:38PM +0100, Alexander Graf wrote:
> >>On 02/18/2014 01:38 PM, Greg Kurz wrote:
> >>>From: Rusty Russell <rusty@rustcorp.com.au>
> >>>
> >>>virtio data structures are defined as "target endian", which assumes
> >>>that's a fixed value.  In fact, that actually means it's
> >>>platform-specific.
> >>>
> >>>The OASIS virtio 1.0 spec will fix this.  Meanwhile, create a hook for
> >>>little endian ppc (and potentially ARM).  This is called at device
> >>>reset time (which is done before any driver is loaded) since it
> >>>may involve a system call to get the status when running under kvm.
> >>>
> >>>[ fixed checkpatch.pl error with the virtio_byteswap initialisation,
> >>>   ldq_phys() API change, Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> >>>Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> >>>Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> >>>---
> >>>  hw/virtio/virtio.c                |    6 ++
> >>>  include/hw/virtio/virtio-access.h |  132 +++++++++++++++++++++++++++++++++++++
> >>>  include/hw/virtio/virtio.h        |    2 +
> >>>  stubs/Makefile.objs               |    1
> >>>  stubs/virtio_get_byteswap.c       |    6 ++
> >>>  5 files changed, 147 insertions(+)
> >>>  create mode 100644 include/hw/virtio/virtio-access.h
> >>>  create mode 100644 stubs/virtio_get_byteswap.c
> >>>
> >>>diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >>>index aeabf3a..4fd6ac2 100644
> >>>--- a/hw/virtio/virtio.c
> >>>+++ b/hw/virtio/virtio.c
> >>>@@ -19,6 +19,9 @@
> >>>  #include "hw/virtio/virtio.h"
> >>>  #include "qemu/atomic.h"
> >>>  #include "hw/virtio/virtio-bus.h"
> >>>+#include "hw/virtio/virtio-access.h"
> >>>+
> >>>+bool virtio_byteswap;
> >>Could this be a virtio object property rather than a global? Imagine
> >>an AMP guest system with a BE and an LE system running in parallel
> >>accessing two separate virtio devices. With a single global that
> >>would break.
> >>
> >>
> >>Alex
> >Well, how does a device know which CPU uses it?
> >I suspect we are better off waiting for 1.0 with this one.
> 
> Good point. It just feels like a heavy layering violation to have a
> global variable for all virtio devices. What if we start mixing
> legacy and 1.0 devices? We could reuse the same flag, but it would
> be initialized differently per device.
> 
> 
> Alex

1.0 won't use the flag, it's all LE.

Re: [Qemu-devel] [PATCH 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Cornelia Huck]

On Tue, 18 Feb 2014 17:03:27 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Feb 18, 2014 at 03:48:38PM +0100, Alexander Graf wrote:
> > On 02/18/2014 01:38 PM, Greg Kurz wrote:
> > >From: Rusty Russell <rusty@rustcorp.com.au>
> > >
> > >virtio data structures are defined as "target endian", which assumes
> > >that's a fixed value.  In fact, that actually means it's
> > >platform-specific.
> > >
> > >The OASIS virtio 1.0 spec will fix this.  Meanwhile, create a hook for
> > >little endian ppc (and potentially ARM).  This is called at device
> > >reset time (which is done before any driver is loaded) since it
> > >may involve a system call to get the status when running under kvm.
> > >
> > >[ fixed checkpatch.pl error with the virtio_byteswap initialisation,
> > >   ldq_phys() API change, Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> > >Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> > >Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > >---
> > >  hw/virtio/virtio.c                |    6 ++
> > >  include/hw/virtio/virtio-access.h |  132 +++++++++++++++++++++++++++++++++++++
> > >  include/hw/virtio/virtio.h        |    2 +
> > >  stubs/Makefile.objs               |    1
> > >  stubs/virtio_get_byteswap.c       |    6 ++
> > >  5 files changed, 147 insertions(+)
> > >  create mode 100644 include/hw/virtio/virtio-access.h
> > >  create mode 100644 stubs/virtio_get_byteswap.c
> > >
> > >diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > >index aeabf3a..4fd6ac2 100644
> > >--- a/hw/virtio/virtio.c
> > >+++ b/hw/virtio/virtio.c
> > >@@ -19,6 +19,9 @@
> > >  #include "hw/virtio/virtio.h"
> > >  #include "qemu/atomic.h"
> > >  #include "hw/virtio/virtio-bus.h"
> > >+#include "hw/virtio/virtio-access.h"
> > >+
> > >+bool virtio_byteswap;
> > 
> > Could this be a virtio object property rather than a global? Imagine
> > an AMP guest system with a BE and an LE system running in parallel
> > accessing two separate virtio devices. With a single global that
> > would break.
> > 
> > 
> > Alex
> 
> Well, how does a device know which CPU uses it?
> I suspect we are better off waiting for 1.0 with this one.
> 

1.0 makes this a bit more complex, no?

virtio-endian accessors are defined by the endianness of host and guest
(doing a bswap depends on the host/guest combination). This needs to be
per qemu instance. (ioctl under kvm? machine option?)

For 1.0, we'll have everything le, so a be host will always do a bswap
(as will a be guest). But whether a device is 1.0 or legacy is not
something that can be decided globally, or we can't have transitional
devices with qemu.

Re: [Qemu-devel] [PATCH 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Cornelia Huck]

On Tue, 18 Feb 2014 16:12:08 +0100
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Tue, 18 Feb 2014 17:03:27 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Feb 18, 2014 at 03:48:38PM +0100, Alexander Graf wrote:
> > > On 02/18/2014 01:38 PM, Greg Kurz wrote:
> > > >From: Rusty Russell <rusty@rustcorp.com.au>
> > > >
> > > >virtio data structures are defined as "target endian", which assumes
> > > >that's a fixed value.  In fact, that actually means it's
> > > >platform-specific.
> > > >
> > > >The OASIS virtio 1.0 spec will fix this.  Meanwhile, create a hook for
> > > >little endian ppc (and potentially ARM).  This is called at device
> > > >reset time (which is done before any driver is loaded) since it
> > > >may involve a system call to get the status when running under kvm.
> > > >
> > > >[ fixed checkpatch.pl error with the virtio_byteswap initialisation,
> > > >   ldq_phys() API change, Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> > > >Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> > > >Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > > >---
> > > >  hw/virtio/virtio.c                |    6 ++
> > > >  include/hw/virtio/virtio-access.h |  132 +++++++++++++++++++++++++++++++++++++
> > > >  include/hw/virtio/virtio.h        |    2 +
> > > >  stubs/Makefile.objs               |    1
> > > >  stubs/virtio_get_byteswap.c       |    6 ++
> > > >  5 files changed, 147 insertions(+)
> > > >  create mode 100644 include/hw/virtio/virtio-access.h
> > > >  create mode 100644 stubs/virtio_get_byteswap.c
> > > >
> > > >diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > >index aeabf3a..4fd6ac2 100644
> > > >--- a/hw/virtio/virtio.c
> > > >+++ b/hw/virtio/virtio.c
> > > >@@ -19,6 +19,9 @@
> > > >  #include "hw/virtio/virtio.h"
> > > >  #include "qemu/atomic.h"
> > > >  #include "hw/virtio/virtio-bus.h"
> > > >+#include "hw/virtio/virtio-access.h"
> > > >+
> > > >+bool virtio_byteswap;
> > > 
> > > Could this be a virtio object property rather than a global? Imagine
> > > an AMP guest system with a BE and an LE system running in parallel
> > > accessing two separate virtio devices. With a single global that
> > > would break.
> > > 
> > > 
> > > Alex
> > 
> > Well, how does a device know which CPU uses it?
> > I suspect we are better off waiting for 1.0 with this one.
> > 
> 
> 1.0 makes this a bit more complex, no?
> 
> virtio-endian accessors are defined by the endianness of host and guest
> (doing a bswap depends on the host/guest combination). This needs to be
> per qemu instance. (ioctl under kvm? machine option?)
> 
> For 1.0, we'll have everything le, so a be host will always do a bswap
> (as will a be guest). But whether a device is 1.0 or legacy is not
> something that can be decided globally, or we can't have transitional
> devices with qemu.
> 

So here are two stupid tables on who needs to do byteswaps, one for
legacy devices, one for 1.0 devices:

legacy devices:

            host
       be        le

g be  host no    host yes
u     guest no   guest no
e
s le  host yes   host no
t     guest no   guest no



virtio 1.0 devices:

            host
       be        le

g be  host yes   host no
u     guest yes  guest yes
e
s le  host yes   host no
t     guest no   guest no


This means byteswaps in qemu always depend on guest-endianness for
legacy and on host-endianness for 1.0. If we want to support
transitional devices with a mixture of legacy/1.0, we'll need both a
per-machine and per-device swap flag:

virtio_whatever(device, parameters...)
{
    if (device->legacy) {
        if (guest_needs_byteswap) {
            whatever_byteswap(parameters...);
        } else {
            whatever(parameters...);
        }
    } else { /* 1.0 */
        whatever_le(parameters...);
    }
}

Comments?

Re: [Qemu-devel] [PATCH 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Alexander Graf]

> Am 18.02.2014 um 16:45 schrieb Cornelia Huck <cornelia.huck@de.ibm.com>:
> 
> On Tue, 18 Feb 2014 16:12:08 +0100
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> 
>> On Tue, 18 Feb 2014 17:03:27 +0200
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> 
>>>> On Tue, Feb 18, 2014 at 03:48:38PM +0100, Alexander Graf wrote:
>>>>> On 02/18/2014 01:38 PM, Greg Kurz wrote:
>>>>> From: Rusty Russell <rusty@rustcorp.com.au>
>>>>> 
>>>>> virtio data structures are defined as "target endian", which assumes
>>>>> that's a fixed value.  In fact, that actually means it's
>>>>> platform-specific.
>>>>> 
>>>>> The OASIS virtio 1.0 spec will fix this.  Meanwhile, create a hook for
>>>>> little endian ppc (and potentially ARM).  This is called at device
>>>>> reset time (which is done before any driver is loaded) since it
>>>>> may involve a system call to get the status when running under kvm.
>>>>> 
>>>>> [ fixed checkpatch.pl error with the virtio_byteswap initialisation,
>>>>>  ldq_phys() API change, Greg Kurz <gkurz@linux.vnet.ibm.com> ]
>>>>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>>>>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
>>>>> ---
>>>>> hw/virtio/virtio.c                |    6 ++
>>>>> include/hw/virtio/virtio-access.h |  132 +++++++++++++++++++++++++++++++++++++
>>>>> include/hw/virtio/virtio.h        |    2 +
>>>>> stubs/Makefile.objs               |    1
>>>>> stubs/virtio_get_byteswap.c       |    6 ++
>>>>> 5 files changed, 147 insertions(+)
>>>>> create mode 100644 include/hw/virtio/virtio-access.h
>>>>> create mode 100644 stubs/virtio_get_byteswap.c
>>>>> 
>>>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>>>> index aeabf3a..4fd6ac2 100644
>>>>> --- a/hw/virtio/virtio.c
>>>>> +++ b/hw/virtio/virtio.c
>>>>> @@ -19,6 +19,9 @@
>>>>> #include "hw/virtio/virtio.h"
>>>>> #include "qemu/atomic.h"
>>>>> #include "hw/virtio/virtio-bus.h"
>>>>> +#include "hw/virtio/virtio-access.h"
>>>>> +
>>>>> +bool virtio_byteswap;
>>>> 
>>>> Could this be a virtio object property rather than a global? Imagine
>>>> an AMP guest system with a BE and an LE system running in parallel
>>>> accessing two separate virtio devices. With a single global that
>>>> would break.
>>>> 
>>>> 
>>>> Alex
>>> 
>>> Well, how does a device know which CPU uses it?
>>> I suspect we are better off waiting for 1.0 with this one.
>> 
>> 1.0 makes this a bit more complex, no?
>> 
>> virtio-endian accessors are defined by the endianness of host and guest
>> (doing a bswap depends on the host/guest combination). This needs to be
>> per qemu instance. (ioctl under kvm? machine option?)
>> 
>> For 1.0, we'll have everything le, so a be host will always do a bswap
>> (as will a be guest). But whether a device is 1.0 or legacy is not
>> something that can be decided globally, or we can't have transitional
>> devices with qemu.
> 
> So here are two stupid tables on who needs to do byteswaps, one for
> legacy devices, one for 1.0 devices:
> 
> legacy devices:
> 
>            host
>       be        le
> 
> g be  host no    host yes
> u     guest no   guest no
> e
> s le  host yes   host no
> t     guest no   guest no
> 
> 
> 
> virtio 1.0 devices:
> 
>            host
>       be        le
> 
> g be  host yes   host no
> u     guest yes  guest yes
> e
> s le  host yes   host no
> t     guest no   guest no
> 
> 
> This means byteswaps in qemu always depend on guest-endianness for
> legacy and on host-endianness for 1.0. If we want to support
> transitional devices with a mixture of legacy/1.0, we'll need both a
> per-machine and per-device swap flag:
> 
> virtio_whatever(device, parameters...)
> {
>    if (device->legacy) {
>        if (guest_needs_byteswap) {
>            whatever_byteswap(parameters...);
>        } else {
>            whatever(parameters...);
>        }
>    } else { /* 1.0 */
>        whatever_le(parameters...);
>    }
> }
> 
> Comments?

Yes. My point was that we could move all of the ifery above into the device reset function and from then on only use the swap bool property.

Alex

> 

Re: [Qemu-devel] [PATCH 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Cornelia Huck]

On Tue, 18 Feb 2014 17:02:18 +0100
Alexander Graf <agraf@suse.de> wrote:

> 
> 
> > Am 18.02.2014 um 16:45 schrieb Cornelia Huck <cornelia.huck@de.ibm.com>:
> > 
> > On Tue, 18 Feb 2014 16:12:08 +0100
> > Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> > 
> >> On Tue, 18 Feb 2014 17:03:27 +0200
> >> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> 
> >>>> On Tue, Feb 18, 2014 at 03:48:38PM +0100, Alexander Graf wrote:
> >>>>> On 02/18/2014 01:38 PM, Greg Kurz wrote:
> >>>>> From: Rusty Russell <rusty@rustcorp.com.au>
> >>>>> 
> >>>>> virtio data structures are defined as "target endian", which assumes
> >>>>> that's a fixed value.  In fact, that actually means it's
> >>>>> platform-specific.
> >>>>> 
> >>>>> The OASIS virtio 1.0 spec will fix this.  Meanwhile, create a hook for
> >>>>> little endian ppc (and potentially ARM).  This is called at device
> >>>>> reset time (which is done before any driver is loaded) since it
> >>>>> may involve a system call to get the status when running under kvm.
> >>>>> 
> >>>>> [ fixed checkpatch.pl error with the virtio_byteswap initialisation,
> >>>>>  ldq_phys() API change, Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> >>>>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> >>>>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> >>>>> ---
> >>>>> hw/virtio/virtio.c                |    6 ++
> >>>>> include/hw/virtio/virtio-access.h |  132 +++++++++++++++++++++++++++++++++++++
> >>>>> include/hw/virtio/virtio.h        |    2 +
> >>>>> stubs/Makefile.objs               |    1
> >>>>> stubs/virtio_get_byteswap.c       |    6 ++
> >>>>> 5 files changed, 147 insertions(+)
> >>>>> create mode 100644 include/hw/virtio/virtio-access.h
> >>>>> create mode 100644 stubs/virtio_get_byteswap.c
> >>>>> 
> >>>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >>>>> index aeabf3a..4fd6ac2 100644
> >>>>> --- a/hw/virtio/virtio.c
> >>>>> +++ b/hw/virtio/virtio.c
> >>>>> @@ -19,6 +19,9 @@
> >>>>> #include "hw/virtio/virtio.h"
> >>>>> #include "qemu/atomic.h"
> >>>>> #include "hw/virtio/virtio-bus.h"
> >>>>> +#include "hw/virtio/virtio-access.h"
> >>>>> +
> >>>>> +bool virtio_byteswap;
> >>>> 
> >>>> Could this be a virtio object property rather than a global? Imagine
> >>>> an AMP guest system with a BE and an LE system running in parallel
> >>>> accessing two separate virtio devices. With a single global that
> >>>> would break.
> >>>> 
> >>>> 
> >>>> Alex
> >>> 
> >>> Well, how does a device know which CPU uses it?
> >>> I suspect we are better off waiting for 1.0 with this one.
> >> 
> >> 1.0 makes this a bit more complex, no?
> >> 
> >> virtio-endian accessors are defined by the endianness of host and guest
> >> (doing a bswap depends on the host/guest combination). This needs to be
> >> per qemu instance. (ioctl under kvm? machine option?)
> >> 
> >> For 1.0, we'll have everything le, so a be host will always do a bswap
> >> (as will a be guest). But whether a device is 1.0 or legacy is not
> >> something that can be decided globally, or we can't have transitional
> >> devices with qemu.
> > 
> > So here are two stupid tables on who needs to do byteswaps, one for
> > legacy devices, one for 1.0 devices:
> > 
> > legacy devices:
> > 
> >            host
> >       be        le
> > 
> > g be  host no    host yes
> > u     guest no   guest no
> > e
> > s le  host yes   host no
> > t     guest no   guest no
> > 
> > 
> > 
> > virtio 1.0 devices:
> > 
> >            host
> >       be        le
> > 
> > g be  host yes   host no
> > u     guest yes  guest yes
> > e
> > s le  host yes   host no
> > t     guest no   guest no
> > 
> > 
> > This means byteswaps in qemu always depend on guest-endianness for
> > legacy and on host-endianness for 1.0. If we want to support
> > transitional devices with a mixture of legacy/1.0, we'll need both a
> > per-machine and per-device swap flag:
> > 
> > virtio_whatever(device, parameters...)
> > {
> >    if (device->legacy) {
> >        if (guest_needs_byteswap) {
> >            whatever_byteswap(parameters...);
> >        } else {
> >            whatever(parameters...);
> >        }
> >    } else { /* 1.0 */
> >        whatever_le(parameters...);
> >    }
> > }
> > 
> > Comments?
> 
> Yes. My point was that we could move all of the ifery above into the device reset function and from then on only use the swap bool property.
> 
> Alex
> 
Hm. So whatever_le for 1.0 devices, and virtio_whatever (checking the
byteswap value) for legacy devices? The device implementation will be
aware of the virtio version anyway.

(Btw., can some of those architectures supporting both le/be run with
mixed le/be cpus? That would be a mess for legacy devices.)

Re: [Qemu-devel] [PATCH 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Alexander Graf]

On 02/18/2014 05:17 PM, Cornelia Huck wrote:
> On Tue, 18 Feb 2014 17:02:18 +0100
> Alexander Graf <agraf@suse.de> wrote:
>
>>
>>> Am 18.02.2014 um 16:45 schrieb Cornelia Huck <cornelia.huck@de.ibm.com>:
>>>
>>> On Tue, 18 Feb 2014 16:12:08 +0100
>>> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
>>>
>>>> On Tue, 18 Feb 2014 17:03:27 +0200
>>>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>>>
>>>>>> On Tue, Feb 18, 2014 at 03:48:38PM +0100, Alexander Graf wrote:
>>>>>>> On 02/18/2014 01:38 PM, Greg Kurz wrote:
>>>>>>> From: Rusty Russell <rusty@rustcorp.com.au>
>>>>>>>
>>>>>>> virtio data structures are defined as "target endian", which assumes
>>>>>>> that's a fixed value.  In fact, that actually means it's
>>>>>>> platform-specific.
>>>>>>>
>>>>>>> The OASIS virtio 1.0 spec will fix this.  Meanwhile, create a hook for
>>>>>>> little endian ppc (and potentially ARM).  This is called at device
>>>>>>> reset time (which is done before any driver is loaded) since it
>>>>>>> may involve a system call to get the status when running under kvm.
>>>>>>>
>>>>>>> [ fixed checkpatch.pl error with the virtio_byteswap initialisation,
>>>>>>>   ldq_phys() API change, Greg Kurz <gkurz@linux.vnet.ibm.com> ]
>>>>>>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>>>>>>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
>>>>>>> ---
>>>>>>> hw/virtio/virtio.c                |    6 ++
>>>>>>> include/hw/virtio/virtio-access.h |  132 +++++++++++++++++++++++++++++++++++++
>>>>>>> include/hw/virtio/virtio.h        |    2 +
>>>>>>> stubs/Makefile.objs               |    1
>>>>>>> stubs/virtio_get_byteswap.c       |    6 ++
>>>>>>> 5 files changed, 147 insertions(+)
>>>>>>> create mode 100644 include/hw/virtio/virtio-access.h
>>>>>>> create mode 100644 stubs/virtio_get_byteswap.c
>>>>>>>
>>>>>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>>>>>> index aeabf3a..4fd6ac2 100644
>>>>>>> --- a/hw/virtio/virtio.c
>>>>>>> +++ b/hw/virtio/virtio.c
>>>>>>> @@ -19,6 +19,9 @@
>>>>>>> #include "hw/virtio/virtio.h"
>>>>>>> #include "qemu/atomic.h"
>>>>>>> #include "hw/virtio/virtio-bus.h"
>>>>>>> +#include "hw/virtio/virtio-access.h"
>>>>>>> +
>>>>>>> +bool virtio_byteswap;
>>>>>> Could this be a virtio object property rather than a global? Imagine
>>>>>> an AMP guest system with a BE and an LE system running in parallel
>>>>>> accessing two separate virtio devices. With a single global that
>>>>>> would break.
>>>>>>
>>>>>>
>>>>>> Alex
>>>>> Well, how does a device know which CPU uses it?
>>>>> I suspect we are better off waiting for 1.0 with this one.
>>>> 1.0 makes this a bit more complex, no?
>>>>
>>>> virtio-endian accessors are defined by the endianness of host and guest
>>>> (doing a bswap depends on the host/guest combination). This needs to be
>>>> per qemu instance. (ioctl under kvm? machine option?)
>>>>
>>>> For 1.0, we'll have everything le, so a be host will always do a bswap
>>>> (as will a be guest). But whether a device is 1.0 or legacy is not
>>>> something that can be decided globally, or we can't have transitional
>>>> devices with qemu.
>>> So here are two stupid tables on who needs to do byteswaps, one for
>>> legacy devices, one for 1.0 devices:
>>>
>>> legacy devices:
>>>
>>>             host
>>>        be        le
>>>
>>> g be  host no    host yes
>>> u     guest no   guest no
>>> e
>>> s le  host yes   host no
>>> t     guest no   guest no
>>>
>>>
>>>
>>> virtio 1.0 devices:
>>>
>>>             host
>>>        be        le
>>>
>>> g be  host yes   host no
>>> u     guest yes  guest yes
>>> e
>>> s le  host yes   host no
>>> t     guest no   guest no
>>>
>>>
>>> This means byteswaps in qemu always depend on guest-endianness for
>>> legacy and on host-endianness for 1.0. If we want to support
>>> transitional devices with a mixture of legacy/1.0, we'll need both a
>>> per-machine and per-device swap flag:
>>>
>>> virtio_whatever(device, parameters...)
>>> {
>>>     if (device->legacy) {
>>>         if (guest_needs_byteswap) {
>>>             whatever_byteswap(parameters...);
>>>         } else {
>>>             whatever(parameters...);
>>>         }
>>>     } else { /* 1.0 */
>>>         whatever_le(parameters...);
>>>     }
>>> }
>>>
>>> Comments?
>> Yes. My point was that we could move all of the ifery above into the device reset function and from then on only use the swap bool property.
>>
>> Alex
>>
> Hm. So whatever_le for 1.0 devices, and virtio_whatever (checking the
> byteswap value) for legacy devices? The device implementation will be
> aware of the virtio version anyway.

Yeah, but I would hope we want to share as much code as possible here, 
so that config accessors can be shared between legacy virtio and 1.0 
virtio. And in that case we want to have a generic helper to read/write 
pieces of that config space - which this patch introduces for us :).

> (Btw., can some of those architectures supporting both le/be run with
> mixed le/be cpus? That would be a mess for legacy devices.)

Yes, they can. No, we don't care :).


Alex

Re: [Qemu-devel] [PATCH 0/8] virtio endian-ambivalent target fixes (rebased)

[Andreas Färber]

Am 18.02.2014 13:38, schrieb Greg Kurz:
> On Fri, 14 Feb 2014 12:59:49 +0100
> Andreas Färber <afaerber@suse.de> wrote:
> 
>> Hi,
>>
>> [...]
>>
>> It might've helped if Rusty had actually used our
>> scripts/get_maintainer.pl script to CC people. While Anthony seems to
>> have reviewed some patches (usually Reviewed-by should be before the
>> final Signed-off-by fwiw), neither Stefan (virtio-net) nor Kevin
>> (virtio-blk) nor Paolo (virtio-scsi) were CC'ed, and recently Michael
>> stepped up as virtio maintainer, so maybe he can take them once ready.
>>
>> 1/7 looks okay to me; 3-7 are rather mechanical - people will need to
>> review that those changes are sufficient for the current code base.
>> We've since converted virtio devices to QOM realize, so a rebase is
>> likely needed for such an "old" series.

Hi,

> Thank you Andreas for your support. :)
> 
> Here is a rebased patchset. Only the two first patches ("virtio_get_byteswap:"
> and "virtio:") had to be updated actually. FWIW, this the very same patchset
> I use, along with some ppc64 specific enablement code, to have functionnal
> ppc64 LE guests.

Some more support: ;) When you resend with Connie's fix, please send as
top-level patchset, and labeled "PATCH v5" (or whatever the count ... I
counted the one you replied to as v3). Please also don't forget to sign
off all patches you send out, even if unchanged. Your [ ... ] comments
should go right before your Sob (unless Rusty did sign them off).

> 
> ---
> 
> Greg Kurz (1):
>       hw/9pfs/virtio_9p_device: use virtio wrappers to access headers.

"virtio-9p-device" (or just "virtio-9p")

> 
> Rusty Russell (7):
>       virtio_get_byteswap: function for endian-ambivalent targets using virtio.
>       virtio: allow byte swapping for vring and config access
>       hw/net/virtio-net: use virtio wrappers to access headers.
>       hw/net/virtio-balloon: use virtio wrappers to access page frame numbers.
>       hw/block/virtio-blk: use virtio wrappers	to access headers.
>       hw/scsi/virtio-scsi: use virtio wrappers	to access headers.
>       hw/char/virtio-serial-bus: use virtio wrappers to access headers.

While at it, some things you've copied...

Usually subjects don't end in a full stop, similar to English headings.
Opinions are divided on whether they should start with a capital letter.

Generally, hw/foo/virtio-bar is redundant and could be just virtio-bar.
http://git.qemu.org/?p=qemu.git&a=search&h=HEAD&st=commit&s=virtio

But it's not wrong and terribly important either. :)

Regards,
Andreas

> 
> 
>  hw/9pfs/virtio-9p-device.c        |    3 +
>  hw/block/virtio-blk.c             |   35 +++++-----
>  hw/char/virtio-serial-bus.c       |   34 +++++-----
>  hw/net/virtio-net.c               |   15 ++--
>  hw/scsi/virtio-scsi.c             |   33 +++++----
>  hw/virtio/virtio-balloon.c        |    3 +
>  hw/virtio/virtio.c                |   38 ++++++-----
>  include/hw/virtio/virtio-access.h |  132 +++++++++++++++++++++++++++++++++++++
>  include/hw/virtio/virtio.h        |    2 +
>  stubs/Makefile.objs               |    1 
>  stubs/virtio_get_byteswap.c       |    6 ++
>  11 files changed, 228 insertions(+), 74 deletions(-)
>  create mode 100644 include/hw/virtio/virtio-access.h
>  create mode 100644 stubs/virtio_get_byteswap.c
> 
> Best Regards.
> 


-- 
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 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Andreas Färber]

Am 18.02.2014 13:38, schrieb Greg Kurz:
> diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
> new file mode 100644
> index 0000000..2e22a47
> --- /dev/null
> +++ b/include/hw/virtio/virtio-access.h
> @@ -0,0 +1,132 @@
> +/*
> + * Virtio Accessor Support: In case your target can change endian.
> + *
> + * Copyright IBM, Corp. 2013
> + *
> + * Authors:
> + *  Rusty Russell   <rusty@au.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
[snip]

I notice that this has been GPL-2.0 from Rusty's first series on. Is
there a reason not to make the new file GPL-2.0+?

Cf. http://wiki.qemu.org/Relicensing

Thanks,
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 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Andreas Färber]

Am 18.02.2014 15:48, schrieb Alexander Graf:
> On 02/18/2014 01:38 PM, Greg Kurz wrote:
>> From: Rusty Russell <rusty@rustcorp.com.au>
>>
>> virtio data structures are defined as "target endian", which assumes
>> that's a fixed value.  In fact, that actually means it's
>> platform-specific.
>>
>> The OASIS virtio 1.0 spec will fix this.  Meanwhile, create a hook for
>> little endian ppc (and potentially ARM).  This is called at device
>> reset time (which is done before any driver is loaded) since it
>> may involve a system call to get the status when running under kvm.
>>
>> [ fixed checkpatch.pl error with the virtio_byteswap initialisation,
>>    ldq_phys() API change, Greg Kurz <gkurz@linux.vnet.ibm.com> ]
>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
>> ---
>>   hw/virtio/virtio.c                |    6 ++
>>   include/hw/virtio/virtio-access.h |  132
>> +++++++++++++++++++++++++++++++++++++
>>   include/hw/virtio/virtio.h        |    2 +
>>   stubs/Makefile.objs               |    1
>>   stubs/virtio_get_byteswap.c       |    6 ++
>>   5 files changed, 147 insertions(+)
>>   create mode 100644 include/hw/virtio/virtio-access.h
>>   create mode 100644 stubs/virtio_get_byteswap.c
>>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index aeabf3a..4fd6ac2 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -19,6 +19,9 @@
>>   #include "hw/virtio/virtio.h"
>>   #include "qemu/atomic.h"
>>   #include "hw/virtio/virtio-bus.h"
>> +#include "hw/virtio/virtio-access.h"
>> +
>> +bool virtio_byteswap;
> 
> Could this be a virtio object property rather than a global? Imagine an
> AMP guest system with a BE and an LE system running in parallel
> accessing two separate virtio devices. With a single global that would
> break.

Déjà vu... ;)

http://lists.nongnu.org/archive/html/qemu-devel/2013-08/msg01043.html

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 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Greg Kurz]

On Tue, 18 Feb 2014 20:25:15 +0100
Andreas Färber <afaerber@suse.de> wrote:
> Am 18.02.2014 13:38, schrieb Greg Kurz:
> > diff --git a/include/hw/virtio/virtio-access.h
> > b/include/hw/virtio/virtio-access.h new file mode 100644
> > index 0000000..2e22a47
> > --- /dev/null
> > +++ b/include/hw/virtio/virtio-access.h
> > @@ -0,0 +1,132 @@
> > +/*
> > + * Virtio Accessor Support: In case your target can change endian.
> > + *
> > + * Copyright IBM, Corp. 2013
> > + *
> > + * Authors:
> > + *  Rusty Russell   <rusty@au.ibm.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2.
> > See
> > + * the COPYING file in the top-level directory.
> [snip]
> 
> I notice that this has been GPL-2.0 from Rusty's first series on. Is
> there a reason not to make the new file GPL-2.0+?
> 
> Cf. http://wiki.qemu.org/Relicensing
> 
> Thanks,
> Andreas
> 

Rusty ? It is your call. :)

-- 
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 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Rusty Russell]

Greg Kurz <gkurz@linux.vnet.ibm.com> writes:
> On Tue, 18 Feb 2014 20:25:15 +0100
> Andreas Färber <afaerber@suse.de> wrote:
>> Am 18.02.2014 13:38, schrieb Greg Kurz:
>> > diff --git a/include/hw/virtio/virtio-access.h
>> > b/include/hw/virtio/virtio-access.h new file mode 100644
>> > index 0000000..2e22a47
>> > --- /dev/null
>> > +++ b/include/hw/virtio/virtio-access.h
>> > @@ -0,0 +1,132 @@
>> > +/*
>> > + * Virtio Accessor Support: In case your target can change endian.
>> > + *
>> > + * Copyright IBM, Corp. 2013
>> > + *
>> > + * Authors:
>> > + *  Rusty Russell   <rusty@au.ibm.com>
>> > + *
>> > + * This work is licensed under the terms of the GNU GPL, version 2.
>> > See
>> > + * the COPYING file in the top-level directory.
>> [snip]
>> 
>> I notice that this has been GPL-2.0 from Rusty's first series on. Is
>> there a reason not to make the new file GPL-2.0+?
>> 
>> Cf. http://wiki.qemu.org/Relicensing
>> 
>> Thanks,
>> Andreas
>> 
>
> Rusty ? It is your call. :)

I cut & paste that header.  I always prefer 2+ to 2 anyway.

Please fix:

 * This program is free software; you can redistribute it and/or modify
 * it under the terms of the GNU General Public License as published by
 * the Free Software Foundation, either version 2 of the License, or
 * (at your option) any later version.

Thanks,
Rusty.

Re: [Qemu-devel] [PATCH 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Rusty Russell]

Alexander Graf <agraf@suse.de> writes:
> On 02/18/2014 05:17 PM, Cornelia Huck wrote:
>> Hm. So whatever_le for 1.0 devices, and virtio_whatever (checking the
>> byteswap value) for legacy devices? The device implementation will be
>> aware of the virtio version anyway.
>
> Yeah, but I would hope we want to share as much code as possible here, 
> so that config accessors can be shared between legacy virtio and 1.0 
> virtio. And in that case we want to have a generic helper to read/write 
> pieces of that config space - which this patch introduces for us :).
>
>> (Btw., can some of those architectures supporting both le/be run with
>> mixed le/be cpus? That would be a mess for legacy devices.)
>
> Yes, they can. No, we don't care :).

There's per-cpu (what endian) and per-device (legacy or no).  To do this
Right, we'd need to consult both, but we don't always have that
information.  So this the minimal viable solution.

We'll need a per-device legacy flag eventually (there are other changes
than just endian).  But getting the wrappers in place is a good first
step.

Cheers,
Rusty.

Re: [Qemu-devel] [PATCH 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Rusty Russell]

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> On Mon, 2013-08-12 at 17:29 +0930, Rusty Russell wrote:
>> virtio data structures are defined as "target endian", which assumes
>> that's a fixed value.  In fact, that actually means it's
>> platform-specific.
>> 
>> Hopefully the OASIS virtio 1.0 spec will fix this.  Meanwhile, create
>> a hook for little endian ppc.
>
> Ok, sorry if I missed a previous debate on that one but why do you do a
> call-out to architecture specific stuff (that is not even inline) on
> every access ?
>
> If we consider that virtio byte order is global, can't you make it a
> global that is *set* by the architecture rather than *polled* by
> virtio ?

OK, so after some more offline discussion it turns out these patches
won't really work reliably, since powerpc kvm doesn't reflect the
register into qemu.

Mikey N has promised me a KVM_GET_ONE_REG to get the register, and I'll
rework on top of that: we will query that whenever a device is reset
(which Linux does on every device init, so it captures the kexec case
too).

Cheers,
Rusty.

Re: [Qemu-devel] [PATCH 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Rusty Russell]

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> On Tue, 2013-08-13 at 13:50 +0930, Rusty Russell wrote:
>> We can have it call once (eg. when the first and storing the status
>> word) and store the result.
>
> And fail with kexec of a different endian kernel :-) Let's not bother
> yet. Merge it and then we see if we can optimize.

Yeah, my code actually did it every status bye write, which
unintentionally solved this.

But agreed, let's let these patches stand...

Cheers,
Rusty.

Re: [Qemu-devel] [PATCH 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Benjamin Herrenschmidt]

On Tue, 2013-08-13 at 13:50 +0930, Rusty Russell wrote:
> We can have it call once (eg. when the first and storing the status
> word) and store the result.

And fail with kexec of a different endian kernel :-) Let's not bother
yet. Merge it and then we see if we can optimize.

Cheers,
Ben.

Re: [Qemu-devel] [PATCH 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Rusty Russell]

Anthony Liguori <anthony@codemonkey.ws> writes:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
>
>> On Mon, 2013-08-12 at 17:29 +0930, Rusty Russell wrote:
>>> virtio data structures are defined as "target endian", which assumes
>>> that's a fixed value.  In fact, that actually means it's
>>> platform-specific.
>>> 
>>> Hopefully the OASIS virtio 1.0 spec will fix this.  Meanwhile, create
>>> a hook for little endian ppc.
>>
>> Ok, sorry if I missed a previous debate on that one but why do you do a
>> call-out to architecture specific stuff (that is not even inline) on
>> every access ?

Anthony said he wanted it that way: my initial patch was more optimized.

> Let's focus on getting something merged.  Then we can muck around with
> it down the road.
>
> Having target-ppc call into virtio is a layering violation.  This
> approach keeps the dependencies cleaner.

We can have it call once (eg. when the first and storing the status
word) and store the result.

Cheers,
Rusty.

Re: [Qemu-devel] [PATCH 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Anthony Liguori]

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

> On Mon, 2013-08-12 at 17:29 +0930, Rusty Russell wrote:
>> virtio data structures are defined as "target endian", which assumes
>> that's a fixed value.  In fact, that actually means it's
>> platform-specific.
>> 
>> Hopefully the OASIS virtio 1.0 spec will fix this.  Meanwhile, create
>> a hook for little endian ppc.
>
> Ok, sorry if I missed a previous debate on that one but why do you do a
> call-out to architecture specific stuff (that is not even inline) on
> every access ?

Let's focus on getting something merged.  Then we can muck around with
it down the road.

Having target-ppc call into virtio is a layering violation.  This
approach keeps the dependencies cleaner.

Regards,

Anthony Liguori

>
> If we consider that virtio byte order is global, can't you make it a
> global that is *set* by the architecture rather than *polled* by
> virtio ?
>
> We have explicit knowledge of when our endianness change (we get the
> hcall or a write to some SPR), we can call virtio *then* to adjust the
> endianness rather than having a call-out to the platform on every
> access.
>
> Ben.

Re: [Qemu-devel] [PATCH 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Peter Maydell]

On 12 August 2013 10:56, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Mon, 2013-08-12 at 10:52 +0100, Peter Maydell wrote:
>> On 12 August 2013 10:50, Benjamin Herrenschmidt
>> <benh@kernel.crashing.org> wrote:
>> > I must be confused ... you mentioned in a previous discussion around
>> > endianness that on some ARM cores at least, when changing the OS
>> > endianness, you had to configure a different lane swapping in the bridge
>> > to the the IO devices (AXI ?)
>>
>> No, that's just the implementation -- the bit in the control
>> register is effectively controlling whether there is byte lane
>> swapping in the part of the CPU which is the data path between
>> it and its bus to the outside world.
>
> I find it amazing that an OS can touch that without hitting the
> hypervisor :-)

It's no different to having a userspace process able to have
a different setting from the OS, really. (There is an equivalent
bit in another register that controls what endianness we
use if we trap to hyp mode.)

> Anyway, ok, we do need to poll from virtio then, but we
> probably need to cache as well, no ?
>
> When do you sample it in qemu ?

It's a bit theoretical at the moment since QEMU's ARM code
kind of assumes little endian. I would expect that at the
point when virtio was in an MMIO callback the CPUState
struct would have been updated via the usual sync process in
kvm_arch_get_registers().

-- PMM

Re: [Qemu-devel] [PATCH 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Benjamin Herrenschmidt]

On Mon, 2013-08-12 at 10:52 +0100, Peter Maydell wrote:
> On 12 August 2013 10:50, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > I must be confused ... you mentioned in a previous discussion around
> > endianness that on some ARM cores at least, when changing the OS
> > endianness, you had to configure a different lane swapping in the bridge
> > to the the IO devices (AXI ?)
> 
> No, that's just the implementation -- the bit in the control
> register is effectively controlling whether there is byte lane
> swapping in the part of the CPU which is the data path between
> it and its bus to the outside world.

I find it amazing that an OS can touch that without hitting the
hypervisor :-) Anyway, ok, we do need to poll from virtio then, but we
probably need to cache as well, no ?

When do you sample it in qemu ?

Cheers,
Ben.

Re: [Qemu-devel] [PATCH 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Peter Maydell]

On 12 August 2013 10:50, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> I must be confused ... you mentioned in a previous discussion around
> endianness that on some ARM cores at least, when changing the OS
> endianness, you had to configure a different lane swapping in the bridge
> to the the IO devices (AXI ?)

No, that's just the implementation -- the bit in the control
register is effectively controlling whether there is byte lane
swapping in the part of the CPU which is the data path between
it and its bus to the outside world.

-- PMM

Re: [Qemu-devel] [PATCH 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Benjamin Herrenschmidt]

On Mon, 2013-08-12 at 10:45 +0100, Peter Maydell wrote:
> On 12 August 2013 10:43, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > On Mon, 2013-08-12 at 10:39 +0100, Peter Maydell wrote:
> >> ARM doesn't -- I wouldn't expect changing the endianness of
> >> exceptions via writing to the SCTLR to trap to the hypervisor
> >> (and in any case it certainly won't result in a return to
> >> userspace).
> >
> > But don't you need to reconfigure the bridge (as per our previous
> > discussion) ? In that case you do need to call out to qemu ...
> 
> Bridge? You've lost me, I'm afraid.

I must be confused ... you mentioned in a previous discussion around
endianness that on some ARM cores at least, when changing the OS
endianness, you had to configure a different lane swapping in the bridge
to the the IO devices (AXI ?)

But I might be confusing with something else.

Ben.

Re: [Qemu-devel] [PATCH 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Peter Maydell]

On 12 August 2013 10:43, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Mon, 2013-08-12 at 10:39 +0100, Peter Maydell wrote:
>> ARM doesn't -- I wouldn't expect changing the endianness of
>> exceptions via writing to the SCTLR to trap to the hypervisor
>> (and in any case it certainly won't result in a return to
>> userspace).
>
> But don't you need to reconfigure the bridge (as per our previous
> discussion) ? In that case you do need to call out to qemu ...

Bridge? You've lost me, I'm afraid.

-- PMM

Re: [Qemu-devel] [PATCH 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Benjamin Herrenschmidt]

On Mon, 2013-08-12 at 10:39 +0100, Peter Maydell wrote:
> On 12 August 2013 10:28, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > We have explicit knowledge of when our endianness change (we get the
> > hcall or a write to some SPR), we can call virtio *then* to adjust the
> > endianness rather than having a call-out to the platform on every
> > access.
> 
> ARM doesn't -- I wouldn't expect changing the endianness of
> exceptions via writing to the SCTLR to trap to the hypervisor
> (and in any case it certainly won't result in a return to
> userspace).

But don't you need to reconfigure the bridge (as per our previous
discussion) ? In that case you do need to call out to qemu ...

Cheers,
Ben.

Re: [Qemu-devel] [PATCH 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Peter Maydell]

On 12 August 2013 10:28, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> We have explicit knowledge of when our endianness change (we get the
> hcall or a write to some SPR), we can call virtio *then* to adjust the
> endianness rather than having a call-out to the platform on every
> access.

ARM doesn't -- I wouldn't expect changing the endianness of
exceptions via writing to the SCTLR to trap to the hypervisor
(and in any case it certainly won't result in a return to
userspace).

-- PMM

Re: [Qemu-devel] [PATCH 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Benjamin Herrenschmidt]

On Mon, 2013-08-12 at 17:29 +0930, Rusty Russell wrote:
> virtio data structures are defined as "target endian", which assumes
> that's a fixed value.  In fact, that actually means it's
> platform-specific.
> 
> Hopefully the OASIS virtio 1.0 spec will fix this.  Meanwhile, create
> a hook for little endian ppc.

Ok, sorry if I missed a previous debate on that one but why do you do a
call-out to architecture specific stuff (that is not even inline) on
every access ?

If we consider that virtio byte order is global, can't you make it a
global that is *set* by the architecture rather than *polled* by
virtio ?

We have explicit knowledge of when our endianness change (we get the
hcall or a write to some SPR), we can call virtio *then* to adjust the
endianness rather than having a call-out to the platform on every
access.

Ben.

[Qemu-devel] [PATCH 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Rusty Russell]

virtio data structures are defined as "target endian", which assumes
that's a fixed value.  In fact, that actually means it's
platform-specific.

Hopefully the OASIS virtio 1.0 spec will fix this.  Meanwhile, create
a hook for little endian ppc.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 include/hw/virtio/virtio-access.h | 130 ++++++++++++++++++++++++++++++++++++++
 include/hw/virtio/virtio.h        |   2 +
 stubs/Makefile.objs               |   1 +
 stubs/virtio_get_byteswap.c       |   6 ++
 4 files changed, 139 insertions(+)
 create mode 100644 include/hw/virtio/virtio-access.h
 create mode 100644 stubs/virtio_get_byteswap.c

diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
new file mode 100644
index 0000000..01d7dd6
--- /dev/null
+++ b/include/hw/virtio/virtio-access.h
@@ -0,0 +1,130 @@
+/*
+ * Virtio Accessor Support: In case your target can change endian.
+ *
+ * Copyright IBM, Corp. 2013
+ *
+ * Authors:
+ *  Rusty Russell   <rusty@au.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ */
+#ifndef _QEMU_VIRTIO_ACCESS_H
+#define _QEMU_VIRTIO_ACCESS_H
+#include "hw/virtio/virtio.h"
+
+static inline uint16_t virtio_lduw_phys(hwaddr pa)
+{
+    if (virtio_get_byteswap()) {
+        return bswap16(lduw_phys(pa));
+    }
+    return lduw_phys(pa);
+    
+}
+
+static inline uint32_t virtio_ldl_phys(hwaddr pa)
+{
+    if (virtio_get_byteswap()) {
+        return bswap32(ldl_phys(pa));
+    }
+    return ldl_phys(pa);
+}
+
+static inline uint64_t virtio_ldq_phys(hwaddr pa)
+{
+    if (virtio_get_byteswap()) {
+        return bswap64(ldq_phys(pa));
+    }
+    return ldq_phys(pa);
+}
+
+static inline void virtio_stw_phys(hwaddr pa, uint16_t value)
+{
+    if (virtio_get_byteswap()) {
+        stw_phys(pa, bswap16(value));
+    } else {
+        stw_phys(pa, value);
+    }
+}
+
+static inline void virtio_stl_phys(hwaddr pa, uint32_t value)
+{
+    if (virtio_get_byteswap()) {
+        stl_phys(pa, bswap32(value));
+    } else {
+        stl_phys(pa, value);
+    }
+}
+
+static inline void virtio_stw_p(void *ptr, uint16_t v)
+{
+    if (virtio_get_byteswap()) {
+        stw_p(ptr, bswap16(v));
+    } else {
+        stw_p(ptr, v);
+    }
+}
+
+static inline void virtio_stl_p(void *ptr, uint32_t v)
+{
+    if (virtio_get_byteswap()) {
+        stl_p(ptr, bswap32(v));
+    } else {
+        stl_p(ptr, v);
+    }
+}
+
+static inline void virtio_stq_p(void *ptr, uint64_t v)
+{
+    if (virtio_get_byteswap()) {
+        stq_p(ptr, bswap64(v));
+    } else {
+        stq_p(ptr, v);
+    }
+}
+
+static inline int virtio_lduw_p(const void *ptr)
+{
+    if (virtio_get_byteswap()) {
+        return bswap16(lduw_p(ptr));
+    } else {
+        return lduw_p(ptr);
+    }
+}
+
+static inline int virtio_ldl_p(const void *ptr)
+{
+    if (virtio_get_byteswap()) {
+        return bswap32(ldl_p(ptr));
+    } else {
+        return ldl_p(ptr);
+    }
+}
+
+static inline uint64_t virtio_ldq_p(const void *ptr)
+{
+    if (virtio_get_byteswap()) {
+        return bswap64(ldq_p(ptr));
+    } else {
+        return ldq_p(ptr);
+    }
+}
+
+static inline uint32_t virtio_tswap32(uint32_t s)
+{
+    if (virtio_get_byteswap()) {
+        return bswap32(tswap32(s));
+    } else {
+        return tswap32(s);
+    }
+}
+
+static inline void virtio_tswap32s(uint32_t *s)
+{
+    tswap32s(s);
+    if (virtio_get_byteswap()) {
+        *s = bswap32(*s);
+    }
+}
+#endif /* _QEMU_VIRTIO_ACCESS_H */
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index d7e9e0f..18ca725 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -248,4 +248,6 @@ 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);
+
+extern bool virtio_get_byteswap(void);
 #endif
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index f306cba..05f7bab 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -26,3 +26,4 @@ stub-obj-y += vm-stop.o
 stub-obj-y += vmstate.o
 stub-obj-$(CONFIG_WIN32) += fd-register.o
 stub-obj-y += cpus.o
+stub-obj-y += virtio_get_byteswap.o
diff --git a/stubs/virtio_get_byteswap.c b/stubs/virtio_get_byteswap.c
new file mode 100644
index 0000000..7cf764d
--- /dev/null
+++ b/stubs/virtio_get_byteswap.c
@@ -0,0 +1,6 @@
+#include "hw/virtio/virtio.h"
+
+bool virtio_get_byteswap(void)
+{
+    return false;
+}
-- 
1.8.1.2