All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/2] dataplane: use endian-aware memory accessors
@ 2015-01-19 17:04 Stefan Hajnoczi
  2015-01-19 17:04 ` [Qemu-devel] [PATCH 1/2] dataplane: move vring_more_avail() into vring.c Stefan Hajnoczi
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Stefan Hajnoczi @ 2015-01-19 17:04 UTC (permalink / raw)
  To: qemu-devel; +Cc: David Gibson, Stefan Hajnoczi

In commit 0f5d1d2a49778863db54b4b1ac2dc008a8f21f11 ("virtio: memory accessors
for endian-ambivalent targets") endian-aware memory accessors were added to
support bi-endian targets like recent ppc64 systems.

The dataplane vring.c code does not use these accessors and is therefore unable
to emulate virtio devices when the endianness differs between the device and
host.

These patches modify vring.c to use the endian-aware accessors.

I have tested that x86_64 guests still function correctly.

I have only compile-tested on ppc64 and require help testing that dataplane
works.  If you have access to a bi-endian system, please try:

  qemu ... -object iothread,id=iothread0 \
           -drive if=none,id=drive0,file=... \
           -device virtio-blk-pci,iothread=iothread0,drive=drive0

Stefan Hajnoczi (2):
  dataplane: move vring_more_avail() into vring.c
  dataplane: use virtio_ld/st_p() for endian-aware memory access

 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         | 69 ++++++++++++++++++++++++++-----------
 include/hw/virtio/dataplane/vring.h |  9 ++---
 6 files changed, 55 insertions(+), 31 deletions(-)

-- 
2.1.0

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [Qemu-devel] [PATCH 1/2] dataplane: move vring_more_avail() into vring.c
  2015-01-19 17:04 [Qemu-devel] [PATCH 0/2] dataplane: use endian-aware memory accessors Stefan Hajnoczi
@ 2015-01-19 17:04 ` Stefan Hajnoczi
  2015-01-20 23:56   ` David Gibson
  2015-01-21 13:25   ` Greg Kurz
  2015-01-19 17:04 ` [Qemu-devel] [PATCH 2/2] dataplane: use virtio_ld/st_p() for endian-aware memory access Stefan Hajnoczi
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 8+ messages in thread
From: Stefan Hajnoczi @ 2015-01-19 17:04 UTC (permalink / raw)
  To: qemu-devel; +Cc: David Gibson, Stefan Hajnoczi

vring_more_avail() was an inline function in vring.h.  No external
callers use it so it's not necessary to export it.

Furthermore, we'll need virtio-access.h for endian-aware memory accesses
but that only works in per-target object files (obj-y) and not
build-once object files (common-obj-y) like the virtio-blk and
virtio-scsi devices.

Move vring_more_avail() into vring.c so that virtio devices like
virtio-blk and virtio-scsi can continue to use vring.h without being
built once per target.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/dataplane/vring.c         | 6 ++++++
 include/hw/virtio/dataplane/vring.h | 6 ------
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
index 61f6d83..cb31d7f 100644
--- a/hw/virtio/dataplane/vring.c
+++ b/hw/virtio/dataplane/vring.c
@@ -21,6 +21,12 @@
 #include "hw/virtio/dataplane/vring.h"
 #include "qemu/error-report.h"
 
+/* Are there more descriptors available? */
+static inline bool vring_more_avail(Vring *vring)
+{
+    return vring->vr.avail->idx != vring->last_avail_idx;
+}
+
 /* vring_map can be coupled with vring_unmap or (if you still have the
  * value returned in *mr) memory_region_unref.
  */
diff --git a/include/hw/virtio/dataplane/vring.h b/include/hw/virtio/dataplane/vring.h
index d3e086a..1e871e6 100644
--- a/include/hw/virtio/dataplane/vring.h
+++ b/include/hw/virtio/dataplane/vring.h
@@ -36,12 +36,6 @@ 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)
 {
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [Qemu-devel] [PATCH 2/2] dataplane: use virtio_ld/st_p() for endian-aware memory access
  2015-01-19 17:04 [Qemu-devel] [PATCH 0/2] dataplane: use endian-aware memory accessors Stefan Hajnoczi
  2015-01-19 17:04 ` [Qemu-devel] [PATCH 1/2] dataplane: move vring_more_avail() into vring.c Stefan Hajnoczi
@ 2015-01-19 17:04 ` Stefan Hajnoczi
  2015-01-21  0:17   ` David Gibson
  2015-01-19 17:33 ` [Qemu-devel] [PATCH 0/2] dataplane: use endian-aware memory accessors Cornelia Huck
  2015-01-21 13:30 ` Stefan Hajnoczi
  3 siblings, 1 reply; 8+ messages in thread
From: Stefan Hajnoczi @ 2015-01-19 17:04 UTC (permalink / raw)
  To: qemu-devel; +Cc: Paolo Bonzini, David Gibson, Fam Zheng, 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 <pbonzini@redhat.com>
Cc: Fam Zheng <famz@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH 0/2] dataplane: use endian-aware memory accessors
  2015-01-19 17:04 [Qemu-devel] [PATCH 0/2] dataplane: use endian-aware memory accessors Stefan Hajnoczi
  2015-01-19 17:04 ` [Qemu-devel] [PATCH 1/2] dataplane: move vring_more_avail() into vring.c Stefan Hajnoczi
  2015-01-19 17:04 ` [Qemu-devel] [PATCH 2/2] dataplane: use virtio_ld/st_p() for endian-aware memory access Stefan Hajnoczi
@ 2015-01-19 17:33 ` Cornelia Huck
  2015-01-21 13:30 ` Stefan Hajnoczi
  3 siblings, 0 replies; 8+ messages in thread
From: Cornelia Huck @ 2015-01-19 17:33 UTC (permalink / raw)
  To: Stefan Hajnoczi; +Cc: qemu-devel, David Gibson

On Mon, 19 Jan 2015 17:04:35 +0000
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> In commit 0f5d1d2a49778863db54b4b1ac2dc008a8f21f11 ("virtio: memory accessors
> for endian-ambivalent targets") endian-aware memory accessors were added to
> support bi-endian targets like recent ppc64 systems.
> 
> The dataplane vring.c code does not use these accessors and is therefore unable
> to emulate virtio devices when the endianness differs between the device and
> host.
> 
> These patches modify vring.c to use the endian-aware accessors.

I had modified the accesses in vring.c to consider endianness for
virtio-1 support as well. I wonder whether we can find some common
ground there?

It's been a while since I have had time to work on this, but searching
for "dataplane: allow virtio-1 devices" should lead you to the relevant
patch.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] dataplane: move vring_more_avail() into vring.c
  2015-01-19 17:04 ` [Qemu-devel] [PATCH 1/2] dataplane: move vring_more_avail() into vring.c Stefan Hajnoczi
@ 2015-01-20 23:56   ` David Gibson
  2015-01-21 13:25   ` Greg Kurz
  1 sibling, 0 replies; 8+ messages in thread
From: David Gibson @ 2015-01-20 23:56 UTC (permalink / raw)
  To: Stefan Hajnoczi; +Cc: qemu-devel

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

On Mon, Jan 19, 2015 at 05:04:36PM +0000, Stefan Hajnoczi wrote:
> vring_more_avail() was an inline function in vring.h.  No external
> callers use it so it's not necessary to export it.
> 
> Furthermore, we'll need virtio-access.h for endian-aware memory accesses
> but that only works in per-target object files (obj-y) and not
> build-once object files (common-obj-y) like the virtio-blk and
> virtio-scsi devices.
> 
> Move vring_more_avail() into vring.c so that virtio devices like
> virtio-blk and virtio-scsi can continue to use vring.h without being
> built once per target.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Looks sensible to me.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
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 --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] dataplane: use virtio_ld/st_p() for endian-aware memory access
  2015-01-19 17:04 ` [Qemu-devel] [PATCH 2/2] dataplane: use virtio_ld/st_p() for endian-aware memory access Stefan Hajnoczi
@ 2015-01-21  0:17   ` David Gibson
  0 siblings, 0 replies; 8+ messages in thread
From: David Gibson @ 2015-01-21  0:17 UTC (permalink / raw)
  To: Stefan Hajnoczi; +Cc: Paolo Bonzini, Fam Zheng, qemu-devel

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

On Mon, Jan 19, 2015 at 05:04:37PM +0000, Stefan Hajnoczi wrote:
> 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 <pbonzini@redhat.com>
> Cc: Fam Zheng <famz@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Without this patch adding an iothread to a secondary virtio-blk disk
caused the kernel to jam up during boot.  With this patch it booted
up and I was able to do things with the disk.

I'm still not able to add an iothread to a primary virtio-blk device:
SLOF (guest firmware) gets errors attempting to read the kernel to
boot.  However, SLOF is always big endian, so I think that's an
unrelated bug.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
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 --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] dataplane: move vring_more_avail() into vring.c
  2015-01-19 17:04 ` [Qemu-devel] [PATCH 1/2] dataplane: move vring_more_avail() into vring.c Stefan Hajnoczi
  2015-01-20 23:56   ` David Gibson
@ 2015-01-21 13:25   ` Greg Kurz
  1 sibling, 0 replies; 8+ messages in thread
From: Greg Kurz @ 2015-01-21 13:25 UTC (permalink / raw)
  To: Stefan Hajnoczi; +Cc: qemu-devel, David Gibson

On Mon, 19 Jan 2015 17:04:36 +0000
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> vring_more_avail() was an inline function in vring.h.  No external
> callers use it so it's not necessary to export it.
> 
> Furthermore, we'll need virtio-access.h for endian-aware memory accesses
> but that only works in per-target object files (obj-y) and not
> build-once object files (common-obj-y) like the virtio-blk and
> virtio-scsi devices.
> 
> Move vring_more_avail() into vring.c so that virtio devices like
> virtio-blk and virtio-scsi can continue to use vring.h without being
> built once per target.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---

Yes, this function should definitely be declared in more appropriate scope.
And BTW I remember Cornelia had the same concern in the virtio-1 serie. :)

Reviewed-by: Greg Kurz <gkurz@linux.vnet.ibm.com>

>  hw/virtio/dataplane/vring.c         | 6 ++++++
>  include/hw/virtio/dataplane/vring.h | 6 ------
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
> index 61f6d83..cb31d7f 100644
> --- a/hw/virtio/dataplane/vring.c
> +++ b/hw/virtio/dataplane/vring.c
> @@ -21,6 +21,12 @@
>  #include "hw/virtio/dataplane/vring.h"
>  #include "qemu/error-report.h"
> 
> +/* Are there more descriptors available? */
> +static inline bool vring_more_avail(Vring *vring)
> +{
> +    return vring->vr.avail->idx != vring->last_avail_idx;
> +}
> +
>  /* vring_map can be coupled with vring_unmap or (if you still have the
>   * value returned in *mr) memory_region_unref.
>   */
> diff --git a/include/hw/virtio/dataplane/vring.h b/include/hw/virtio/dataplane/vring.h
> index d3e086a..1e871e6 100644
> --- a/include/hw/virtio/dataplane/vring.h
> +++ b/include/hw/virtio/dataplane/vring.h
> @@ -36,12 +36,6 @@ 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)
>  {

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH 0/2] dataplane: use endian-aware memory accessors
  2015-01-19 17:04 [Qemu-devel] [PATCH 0/2] dataplane: use endian-aware memory accessors Stefan Hajnoczi
                   ` (2 preceding siblings ...)
  2015-01-19 17:33 ` [Qemu-devel] [PATCH 0/2] dataplane: use endian-aware memory accessors Cornelia Huck
@ 2015-01-21 13:30 ` Stefan Hajnoczi
  3 siblings, 0 replies; 8+ messages in thread
From: Stefan Hajnoczi @ 2015-01-21 13:30 UTC (permalink / raw)
  To: Stefan Hajnoczi; +Cc: qemu-devel, David Gibson

On Mon, Jan 19, 2015 at 5:04 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> In commit 0f5d1d2a49778863db54b4b1ac2dc008a8f21f11 ("virtio: memory accessors
> for endian-ambivalent targets") endian-aware memory accessors were added to
> support bi-endian targets like recent ppc64 systems.
>
> The dataplane vring.c code does not use these accessors and is therefore unable
> to emulate virtio devices when the endianness differs between the device and
> host.
>
> These patches modify vring.c to use the endian-aware accessors.
>
> I have tested that x86_64 guests still function correctly.
>
> I have only compile-tested on ppc64 and require help testing that dataplane
> works.  If you have access to a bi-endian system, please try:
>
>   qemu ... -object iothread,id=iothread0 \
>            -drive if=none,id=drive0,file=... \
>            -device virtio-blk-pci,iothread=iothread0,drive=drive0
>
> Stefan Hajnoczi (2):
>   dataplane: move vring_more_avail() into vring.c
>   dataplane: use virtio_ld/st_p() for endian-aware memory access
>
>  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         | 69 ++++++++++++++++++++++++++-----------
>  include/hw/virtio/dataplane/vring.h |  9 ++---
>  6 files changed, 55 insertions(+), 31 deletions(-)

NACK

Cornelia Huck already has an equivalent patch.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2015-01-21 13:30 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-19 17:04 [Qemu-devel] [PATCH 0/2] dataplane: use endian-aware memory accessors Stefan Hajnoczi
2015-01-19 17:04 ` [Qemu-devel] [PATCH 1/2] dataplane: move vring_more_avail() into vring.c Stefan Hajnoczi
2015-01-20 23:56   ` David Gibson
2015-01-21 13:25   ` Greg Kurz
2015-01-19 17:04 ` [Qemu-devel] [PATCH 2/2] dataplane: use virtio_ld/st_p() for endian-aware memory access Stefan Hajnoczi
2015-01-21  0:17   ` David Gibson
2015-01-19 17:33 ` [Qemu-devel] [PATCH 0/2] dataplane: use endian-aware memory accessors Cornelia Huck
2015-01-21 13:30 ` Stefan Hajnoczi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.