[Qemu-devel] [PATCH v2 0/1] dataplane vs. endianness

[Qemu-devel] [PATCH v2 0/1] dataplane vs. endianness

[Cornelia Huck]

Stefan:

Here's v2 of my endianness patch for dataplane, with the extraneous
vdev argument dropped from get_desc().

I orginally planned to send my virtio-1 patchset as well, but I haven't
found the time for it; therefore, I think this should be applied
independently.

David: I take it your r-b still holds?

Cornelia Huck (1):
  dataplane: endianness-aware accesses

 hw/block/dataplane/virtio-blk.c               |  4 +-
 hw/scsi/virtio-scsi-dataplane.c               |  2 +-
 hw/virtio/Makefile.objs                       |  2 +-
 hw/virtio/dataplane/Makefile.objs             |  2 +-
 hw/virtio/dataplane/vring.c                   | 53 ++++++++++++-------
 include/hw/virtio/dataplane/vring-accessors.h | 75 +++++++++++++++++++++++++++
 include/hw/virtio/dataplane/vring.h           | 14 +----
 7 files changed, 117 insertions(+), 35 deletions(-)
 create mode 100644 include/hw/virtio/dataplane/vring-accessors.h

-- 
2.1.4

[Qemu-devel] [PATCH v2 1/1] dataplane: endianness-aware accesses

[Cornelia Huck]

The vring.c code currently assumes that guest and host endianness match,
which is not true for a number of cases:

- emulating targets with a different endianness than the host
- bi-endian targets, where the correct endianness depends on the virtio
  device
- upcoming support for the virtio-1 standard mandates little-endian
  accesses even for big-endian targets and hosts

Make sure to use accessors that depend on the virtio device.

Note that dataplane now needs to be built per-target.

Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Fam Zheng <famz@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Tested-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hw/block/dataplane/virtio-blk.c               |  4 +-
 hw/scsi/virtio-scsi-dataplane.c               |  2 +-
 hw/virtio/Makefile.objs                       |  2 +-
 hw/virtio/dataplane/Makefile.objs             |  2 +-
 hw/virtio/dataplane/vring.c                   | 53 ++++++++++++-------
 include/hw/virtio/dataplane/vring-accessors.h | 75 +++++++++++++++++++++++++++
 include/hw/virtio/dataplane/vring.h           | 14 +----
 7 files changed, 117 insertions(+), 35 deletions(-)
 create mode 100644 include/hw/virtio/dataplane/vring-accessors.h

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 39c5d71..7ae4e03 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -16,7 +16,9 @@
 #include "qemu/iov.h"
 #include "qemu/thread.h"
 #include "qemu/error-report.h"
+#include "hw/virtio/virtio-access.h"
 #include "hw/virtio/dataplane/vring.h"
+#include "hw/virtio/dataplane/vring-accessors.h"
 #include "sysemu/block-backend.h"
 #include "hw/virtio/virtio-blk.h"
 #include "virtio-blk.h"
@@ -75,7 +77,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..19b224a 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-$(CONFIG_VIRTIO) += dataplane/
 
 obj-y += virtio.o virtio-balloon.o 
 obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o
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 61f6d83..87d9121 100644
--- a/hw/virtio/dataplane/vring.c
+++ b/hw/virtio/dataplane/vring.c
@@ -18,7 +18,9 @@
 #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 "hw/virtio/dataplane/vring-accessors.h"
 #include "qemu/error-report.h"
 
 /* vring_map can be coupled with vring_unmap or (if you still have the
@@ -83,7 +85,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 = vring_get_used_idx(vdev, vring);
     vring->signalled_used = 0;
     vring->signalled_used_valid = false;
 
@@ -104,7 +106,7 @@ 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;
+        vring_set_used_flags(vdev, vring, VRING_USED_F_NO_NOTIFY);
     }
 }
 
@@ -117,10 +119,10 @@ 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;
     } else {
-        vring->vr.used->flags &= ~VRING_USED_F_NO_NOTIFY;
+        vring_clear_used_flags(vdev, vring, 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() */
@@ -134,12 +136,13 @@ 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(!vring_more_avail(vdev, vring))) {
         return true;
     }
 
     if (!(vdev->guest_features & VIRTIO_RING_F_EVENT_IDX)) {
-        return !(vring->vr.avail->flags & VRING_AVAIL_F_NO_INTERRUPT);
+        return !(vring_get_avail_flags(vdev, vring) &
+                 VRING_AVAIL_F_NO_INTERRUPT);
     }
     old = vring->signalled_used;
     v = vring->signalled_used_valid;
@@ -202,9 +205,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,
-                        struct vring_desc *indirect)
+static int get_indirect(VirtIODevice *vdev, Vring *vring,
+                        VirtQueueElement *elem, struct vring_desc *indirect)
 {
     struct vring_desc desc;
     unsigned int i = 0, count, found = 0;
@@ -244,7 +257,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 */
@@ -320,7 +333,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 = vring_get_avail_idx(vdev, vring);
     barrier(); /* load indices now and not again later */
 
     if (unlikely((uint16_t)(avail_idx - last_avail_idx) > num)) {
@@ -341,7 +354,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 = vring_get_avail_ring(vdev, vring, last_avail_idx % num);
 
     elem->index = head;
 
@@ -365,13 +378,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;
             }
@@ -407,9 +420,9 @@ 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;
     uint16_t new;
 
@@ -422,14 +435,16 @@ 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;
+    vring_set_used_ring_id(vdev, vring, vring->last_used_idx % vring->vr.num,
+                           head);
+    vring_set_used_ring_len(vdev, vring, vring->last_used_idx % vring->vr.num,
+                            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;
+    vring_set_used_idx(vdev, vring, 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-accessors.h b/include/hw/virtio/dataplane/vring-accessors.h
new file mode 100644
index 0000000..b508b87
--- /dev/null
+++ b/include/hw/virtio/dataplane/vring-accessors.h
@@ -0,0 +1,75 @@
+#ifndef VRING_ACCESSORS_H
+#define VRING_ACCESSORS_H
+
+#include "hw/virtio/virtio_ring.h"
+#include "hw/virtio/virtio.h"
+#include "hw/virtio/virtio-access.h"
+
+static inline uint16_t vring_get_used_idx(VirtIODevice *vdev, Vring *vring)
+{
+    return virtio_tswap16(vdev, vring->vr.used->idx);
+}
+
+static inline void vring_set_used_idx(VirtIODevice *vdev, Vring *vring,
+                                      uint16_t idx)
+{
+    vring->vr.used->idx = virtio_tswap16(vdev, idx);
+}
+
+static inline uint16_t vring_get_avail_idx(VirtIODevice *vdev, Vring *vring)
+{
+    return virtio_tswap16(vdev, vring->vr.avail->idx);
+}
+
+static inline uint16_t vring_get_avail_ring(VirtIODevice *vdev, Vring *vring,
+                                            int i)
+{
+    return virtio_tswap16(vdev, vring->vr.avail->ring[i]);
+}
+
+static inline void vring_set_used_ring_id(VirtIODevice *vdev, Vring *vring,
+                                          int i, uint32_t id)
+{
+    vring->vr.used->ring[i].id = virtio_tswap32(vdev, id);
+}
+
+static inline void vring_set_used_ring_len(VirtIODevice *vdev, Vring *vring,
+                                          int i, uint32_t len)
+{
+    vring->vr.used->ring[i].len = virtio_tswap32(vdev, len);
+}
+
+static inline uint16_t vring_get_used_flags(VirtIODevice *vdev, Vring *vring)
+{
+    return virtio_tswap16(vdev, vring->vr.used->flags);
+}
+
+static inline uint16_t vring_get_avail_flags(VirtIODevice *vdev, Vring *vring)
+{
+    return virtio_tswap16(vdev, vring->vr.avail->flags);
+}
+
+static inline void vring_set_used_flags(VirtIODevice *vdev, Vring *vring,
+                                        uint16_t flags)
+{
+    vring->vr.used->flags |= virtio_tswap16(vdev, flags);
+}
+
+static inline void vring_clear_used_flags(VirtIODevice *vdev, Vring *vring,
+                                          uint16_t flags)
+{
+    vring->vr.used->flags &= virtio_tswap16(vdev, ~flags);
+}
+
+static inline unsigned int vring_get_num(Vring *vring)
+{
+    return vring->vr.num;
+}
+
+/* Are there more descriptors available? */
+static inline bool vring_more_avail(VirtIODevice *vdev, Vring *vring)
+{
+    return vring_get_avail_idx(vdev, vring) != vring->last_avail_idx;
+}
+
+#endif
diff --git a/include/hw/virtio/dataplane/vring.h b/include/hw/virtio/dataplane/vring.h
index d3e086a..e42c0fc 100644
--- a/include/hw/virtio/dataplane/vring.h
+++ b/include/hw/virtio/dataplane/vring.h
@@ -31,17 +31,6 @@ typedef struct {
     bool broken;                    /* was there a fatal error? */
 } Vring;
 
-static inline unsigned int vring_get_num(Vring *vring)
-{
-    return vring->vr.num;
-}
-
-/* Are there more descriptors available? */
-static inline bool vring_more_avail(Vring *vring)
-{
-    return vring->vr.avail->idx != vring->last_avail_idx;
-}
-
 /* Fail future vring_pop() and vring_push() calls until reset */
 static inline void vring_set_broken(Vring *vring)
 {
@@ -54,6 +43,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.4

Re: [Qemu-devel] [PATCH v2 0/1] dataplane vs. endianness

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 719 bytes --]

On Mon, Jan 26, 2015 at 05:26:41PM +0100, Cornelia Huck wrote:
> Stefan:
> 
> Here's v2 of my endianness patch for dataplane, with the extraneous
> vdev argument dropped from get_desc().
> 
> I orginally planned to send my virtio-1 patchset as well, but I haven't
> found the time for it; therefore, I think this should be applied
> independently.
> 
> David: I take it your r-b still holds?

Yes.  I also retested this version and it still works fine.

Tested-by: David Gibson <david@gibson.dropbear.id.au>

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v2 0/1] dataplane vs. endianness

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 848 bytes --]

On Tue, Jan 27, 2015 at 04:15:23PM +1100, David Gibson wrote:
> On Mon, Jan 26, 2015 at 05:26:41PM +0100, Cornelia Huck wrote:
> > Stefan:
> > 
> > Here's v2 of my endianness patch for dataplane, with the extraneous
> > vdev argument dropped from get_desc().
> > 
> > I orginally planned to send my virtio-1 patchset as well, but I haven't
> > found the time for it; therefore, I think this should be applied
> > independently.
> > 
> > David: I take it your r-b still holds?
> 
> Yes.  I also retested this version and it still works fine.
> 
> Tested-by: David Gibson <david@gibson.dropbear.id.au>

Any word on getting this merged?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v2 0/1] dataplane vs. endianness

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1194 bytes --]

On Mon, Jan 26, 2015 at 05:26:41PM +0100, Cornelia Huck wrote:
> Stefan:
> 
> Here's v2 of my endianness patch for dataplane, with the extraneous
> vdev argument dropped from get_desc().
> 
> I orginally planned to send my virtio-1 patchset as well, but I haven't
> found the time for it; therefore, I think this should be applied
> independently.
> 
> David: I take it your r-b still holds?
> 
> Cornelia Huck (1):
>   dataplane: endianness-aware accesses
> 
>  hw/block/dataplane/virtio-blk.c               |  4 +-
>  hw/scsi/virtio-scsi-dataplane.c               |  2 +-
>  hw/virtio/Makefile.objs                       |  2 +-
>  hw/virtio/dataplane/Makefile.objs             |  2 +-
>  hw/virtio/dataplane/vring.c                   | 53 ++++++++++++-------
>  include/hw/virtio/dataplane/vring-accessors.h | 75 +++++++++++++++++++++++++++
>  include/hw/virtio/dataplane/vring.h           | 14 +----
>  7 files changed, 117 insertions(+), 35 deletions(-)
>  create mode 100644 include/hw/virtio/dataplane/vring-accessors.h

I will merge this into the block tree for the Friday, 13th of February
pull request.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v2 0/1] dataplane vs. endianness

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1174 bytes --]

On Mon, Jan 26, 2015 at 05:26:41PM +0100, Cornelia Huck wrote:
> Stefan:
> 
> Here's v2 of my endianness patch for dataplane, with the extraneous
> vdev argument dropped from get_desc().
> 
> I orginally planned to send my virtio-1 patchset as well, but I haven't
> found the time for it; therefore, I think this should be applied
> independently.
> 
> David: I take it your r-b still holds?
> 
> Cornelia Huck (1):
>   dataplane: endianness-aware accesses
> 
>  hw/block/dataplane/virtio-blk.c               |  4 +-
>  hw/scsi/virtio-scsi-dataplane.c               |  2 +-
>  hw/virtio/Makefile.objs                       |  2 +-
>  hw/virtio/dataplane/Makefile.objs             |  2 +-
>  hw/virtio/dataplane/vring.c                   | 53 ++++++++++++-------
>  include/hw/virtio/dataplane/vring-accessors.h | 75 +++++++++++++++++++++++++++
>  include/hw/virtio/dataplane/vring.h           | 14 +----
>  7 files changed, 117 insertions(+), 35 deletions(-)
>  create mode 100644 include/hw/virtio/dataplane/vring-accessors.h
> 
> -- 
> 2.1.4
> 
> 

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]