[Qemu-devel] [PATCH 2/2] dataplane: use virtio_ld/st_p() for endian-aware memory access

[Stefan Hajnoczi]

The vring.c code was written under the assumption that virtio devices
have the same endianness as the host.

This is wrong when emulating targets that differ in endianness from the
host.

It is also wrong when emulating bi-endian targets like POWER 8
processors, which support both little- and big-endian at run-time.  In
this case the virtio device knows which endianness to use.

This change requires the virtio-access.h APIs and therefore builds
vring.o for each target (obj-y instead of common-obj-y).

Note that $(CONFIG_VIRTIO) for dataplane/ was dropped in
hw/virtio/Makefile.objs because hw/Makefile.objs conditionally includes
hw/virtio/ on $(CONFIG_VIRTIO) already.

Only a small change is needed to the vring.h interface: vring_push() now
takes a VirtIODevice *vdev argument.

Cc: Paolo Bonzini <address@hidden>
Cc: Fam Zheng <address@hidden>
Signed-off-by: Stefan Hajnoczi <address@hidden>
---
 hw/block/dataplane/virtio-blk.c     |  2 +-
 hw/scsi/virtio-scsi-dataplane.c     |  2 +-
 hw/virtio/Makefile.objs             |  2 +-
 hw/virtio/dataplane/Makefile.objs   |  2 +-
 hw/virtio/dataplane/vring.c         | 67 +++++++++++++++++++++++++------------
 include/hw/virtio/dataplane/vring.h |  3 +-
 6 files changed, 51 insertions(+), 27 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 39c5d71..aca2a8d 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -75,7 +75,7 @@ static void complete_request_vring(VirtIOBlockReq *req, 
unsigned char status)
     VirtIOBlockDataPlane *s = req->dev->dataplane;
     stb_p(&req->in->status, status);
 
-    vring_push(&req->dev->dataplane->vring, &req->elem,
+    vring_push(s->vdev, &req->dev->dataplane->vring, &req->elem,
                req->qiov.size + sizeof(*req->in));
 
     /* Suppress notification to guest by BH and its scheduled
diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index 03a1e8c..418d73b 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -94,7 +94,7 @@ void virtio_scsi_vring_push_notify(VirtIOSCSIReq *req)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(req->vring->parent);
 
-    vring_push(&req->vring->vring, &req->elem,
+    vring_push(vdev, &req->vring->vring, &req->elem,
                req->qsgl.size + req->resp_iov.size);
 
     if (vring_should_notify(vdev, &req->vring->vring)) {
diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
index d21c397..3e93a3a 100644
--- a/hw/virtio/Makefile.objs
+++ b/hw/virtio/Makefile.objs
@@ -2,7 +2,7 @@ common-obj-y += virtio-rng.o
 common-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
 common-obj-y += virtio-bus.o
 common-obj-y += virtio-mmio.o
-common-obj-$(CONFIG_VIRTIO) += dataplane/
 
 obj-y += virtio.o virtio-balloon.o 
 obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o
+obj-y += dataplane/
diff --git a/hw/virtio/dataplane/Makefile.objs 
b/hw/virtio/dataplane/Makefile.objs
index 9a8cfc0..753a9ca 100644
--- a/hw/virtio/dataplane/Makefile.objs
+++ b/hw/virtio/dataplane/Makefile.objs
@@ -1 +1 @@
-common-obj-y += vring.o
+obj-y += vring.o
diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
index cb31d7f..a36dc19 100644
--- a/hw/virtio/dataplane/vring.c
+++ b/hw/virtio/dataplane/vring.c
@@ -18,13 +18,14 @@
 #include "hw/hw.h"
 #include "exec/memory.h"
 #include "exec/address-spaces.h"
+#include "hw/virtio/virtio-access.h"
 #include "hw/virtio/dataplane/vring.h"
 #include "qemu/error-report.h"
 
 /* Are there more descriptors available? */
-static inline bool vring_more_avail(Vring *vring)
+static inline bool vring_more_avail(VirtIODevice *vdev, Vring *vring)
 {
-    return vring->vr.avail->idx != vring->last_avail_idx;
+    return virtio_lduw_p(vdev, &vring->vr.avail->idx) != vring->last_avail_idx;
 }
 
 /* vring_map can be coupled with vring_unmap or (if you still have the
@@ -89,7 +90,7 @@ bool vring_setup(Vring *vring, VirtIODevice *vdev, int n)
     vring_init(&vring->vr, virtio_queue_get_num(vdev, n), vring_ptr, 4096);
 
     vring->last_avail_idx = virtio_queue_get_last_avail_idx(vdev, n);
-    vring->last_used_idx = vring->vr.used->idx;
+    vring->last_used_idx = virtio_lduw_p(vdev, &vring->vr.used->idx);
     vring->signalled_used = 0;
     vring->signalled_used_valid = false;
 
@@ -110,7 +111,9 @@ void vring_teardown(Vring *vring, VirtIODevice *vdev, int n)
 void vring_disable_notification(VirtIODevice *vdev, Vring *vring)
 {
     if (!(vdev->guest_features & (1 << VIRTIO_RING_F_EVENT_IDX))) {
-        vring->vr.used->flags |= VRING_USED_F_NO_NOTIFY;
+        uint16_t flags = virtio_lduw_p(vdev, &vring->vr.used->flags);
+        virtio_stw_p(vdev, &vring->vr.used->flags,
+                     flags | VRING_USED_F_NO_NOTIFY);
     }
 }
 
@@ -121,18 +124,21 @@ void vring_disable_notification(VirtIODevice *vdev, Vring 
*vring)
 bool vring_enable_notification(VirtIODevice *vdev, Vring *vring)
 {
     if (vdev->guest_features & (1 << VIRTIO_RING_F_EVENT_IDX)) {
-        vring_avail_event(&vring->vr) = vring->vr.avail->idx;
+        virtio_stw_p(vdev, &vring_avail_event(&vring->vr),
+                     virtio_lduw_p(vdev, &vring->vr.avail->idx));
     } else {
-        vring->vr.used->flags &= ~VRING_USED_F_NO_NOTIFY;
+        uint16_t flags = virtio_lduw_p(vdev, &vring->vr.used->flags);
+        virtio_stw_p(vdev, &vring->vr.used->flags,
+                     flags & ~VRING_USED_F_NO_NOTIFY);
     }
     smp_mb(); /* ensure update is seen before reading avail_idx */
-    return !vring_more_avail(vring);
+    return !vring_more_avail(vdev, vring);
 }
 
 /* This is stolen from linux/drivers/vhost/vhost.c:vhost_notify() */
 bool vring_should_notify(VirtIODevice *vdev, Vring *vring)
 {
-    uint16_t old, new;
+    uint16_t used_event_idx, old, new;
     bool v;
     /* Flush out used index updates. This is paired
      * with the barrier that the Guest executes when enabling
@@ -140,12 +146,14 @@ bool vring_should_notify(VirtIODevice *vdev, Vring *vring)
     smp_mb();
 
     if ((vdev->guest_features & VIRTIO_F_NOTIFY_ON_EMPTY) &&
-        unlikely(vring->vr.avail->idx == vring->last_avail_idx)) {
+        unlikely(virtio_lduw_p(vdev, &vring->vr.avail->idx) ==
+                 vring->last_avail_idx)) {
         return true;
     }
 
     if (!(vdev->guest_features & VIRTIO_RING_F_EVENT_IDX)) {
-        return !(vring->vr.avail->flags & VRING_AVAIL_F_NO_INTERRUPT);
+        uint16_t flags = virtio_lduw_p(vdev, &vring->vr.avail->flags);
+        return !(flags & VRING_AVAIL_F_NO_INTERRUPT);
     }
     old = vring->signalled_used;
     v = vring->signalled_used_valid;
@@ -156,7 +164,8 @@ bool vring_should_notify(VirtIODevice *vdev, Vring *vring)
         return true;
     }
 
-    return vring_need_event(vring_used_event(&vring->vr), new, old);
+    used_event_idx = virtio_lduw_p(vdev, &vring_used_event(&vring->vr));
+    return vring_need_event(used_event_idx, new, old);
 }
 
 
@@ -208,8 +217,19 @@ static int get_desc(Vring *vring, VirtQueueElement *elem,
     return 0;
 }
 
+static void copy_in_vring_desc(VirtIODevice *vdev,
+                               const struct vring_desc *guest,
+                               struct vring_desc *host)
+{
+    host->addr = virtio_ldq_p(vdev, &guest->addr);
+    host->len = virtio_ldl_p(vdev, &guest->len);
+    host->flags = virtio_lduw_p(vdev, &guest->flags);
+    host->next = virtio_lduw_p(vdev, &guest->next);
+}
+
 /* This is stolen from linux/drivers/vhost/vhost.c. */
-static int get_indirect(Vring *vring, VirtQueueElement *elem,
+static int get_indirect(VirtIODevice *vdev, Vring *vring,
+                        VirtQueueElement *elem,
                         struct vring_desc *indirect)
 {
     struct vring_desc desc;
@@ -250,7 +270,7 @@ static int get_indirect(Vring *vring, VirtQueueElement 
*elem,
             vring->broken = true;
             return -EFAULT;
         }
-        desc = *desc_ptr;
+        copy_in_vring_desc(vdev, desc_ptr, &desc);
         memory_region_unref(mr);
 
         /* Ensure descriptor has been loaded before accessing fields */
@@ -326,7 +346,7 @@ int vring_pop(VirtIODevice *vdev, Vring *vring,
 
     /* Check it isn't doing very strange things with descriptor numbers. */
     last_avail_idx = vring->last_avail_idx;
-    avail_idx = vring->vr.avail->idx;
+    avail_idx = virtio_lduw_p(vdev, &vring->vr.avail->idx);
     barrier(); /* load indices now and not again later */
 
     if (unlikely((uint16_t)(avail_idx - last_avail_idx) > num)) {
@@ -347,7 +367,7 @@ int vring_pop(VirtIODevice *vdev, Vring *vring,
 
     /* Grab the next descriptor number they're advertising, and increment
      * the index we've seen. */
-    head = vring->vr.avail->ring[last_avail_idx % num];
+    head = virtio_lduw_p(vdev, &vring->vr.avail->ring[last_avail_idx % num]);
 
     elem->index = head;
 
@@ -371,13 +391,13 @@ int vring_pop(VirtIODevice *vdev, Vring *vring,
             ret = -EFAULT;
             goto out;
         }
-        desc = vring->vr.desc[i];
+        copy_in_vring_desc(vdev, &vring->vr.desc[i], &desc);
 
         /* Ensure descriptor is loaded before accessing fields */
         barrier();
 
         if (desc.flags & VRING_DESC_F_INDIRECT) {
-            ret = get_indirect(vring, elem, &desc);
+            ret = get_indirect(vdev, vring, elem, &desc);
             if (ret < 0) {
                 goto out;
             }
@@ -395,7 +415,8 @@ int vring_pop(VirtIODevice *vdev, Vring *vring,
     /* On success, increment avail index. */
     vring->last_avail_idx++;
     if (vdev->guest_features & (1 << VIRTIO_RING_F_EVENT_IDX)) {
-        vring_avail_event(&vring->vr) = vring->last_avail_idx;
+        virtio_stw_p(vdev, &vring_avail_event(&vring->vr),
+                     vring->last_avail_idx);
     }
 
     return head;
@@ -413,7 +434,8 @@ out:
  *
  * Stolen from linux/drivers/vhost/vhost.c.
  */
-void vring_push(Vring *vring, VirtQueueElement *elem, int len)
+void vring_push(VirtIODevice *vdev, Vring *vring,
+                VirtQueueElement *elem, int len)
 {
     struct vring_used_elem *used;
     unsigned int head = elem->index;
@@ -429,13 +451,14 @@ void vring_push(Vring *vring, VirtQueueElement *elem, int 
len)
     /* The virtqueue contains a ring of used buffers.  Get a pointer to the
      * next entry in that used ring. */
     used = &vring->vr.used->ring[vring->last_used_idx % vring->vr.num];
-    used->id = head;
-    used->len = len;
+    virtio_stl_p(vdev, &used->id, head);
+    virtio_stl_p(vdev, &used->len, len);
 
     /* Make sure buffer is written before we update index. */
     smp_wmb();
 
-    new = vring->vr.used->idx = ++vring->last_used_idx;
+    new = ++vring->last_used_idx;
+    virtio_stw_p(vdev, &vring->vr.used->idx, new);
     if (unlikely((int16_t)(new - vring->signalled_used) < (uint16_t)1)) {
         vring->signalled_used_valid = false;
     }
diff --git a/include/hw/virtio/dataplane/vring.h 
b/include/hw/virtio/dataplane/vring.h
index 1e871e6..38e581b 100644
--- a/include/hw/virtio/dataplane/vring.h
+++ b/include/hw/virtio/dataplane/vring.h
@@ -48,6 +48,7 @@ void vring_disable_notification(VirtIODevice *vdev, Vring 
*vring);
 bool vring_enable_notification(VirtIODevice *vdev, Vring *vring);
 bool vring_should_notify(VirtIODevice *vdev, Vring *vring);
 int vring_pop(VirtIODevice *vdev, Vring *vring, VirtQueueElement *elem);
-void vring_push(Vring *vring, VirtQueueElement *elem, int len);
+void vring_push(VirtIODevice *vdev, Vring *vring,
+                VirtQueueElement *elem, int len);
 
 #endif /* VRING_H */
-- 
2.1.0