David: I saw you tested Stefan's endianness patch.
Could you also give my alternative implementation a try? Thanks!
On Tue, 20 Jan 2015 17:27:50 +0100
Cornelia Huck <cornelia.h...@de.ibm.com> wrote:
> 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 <stefa...@redhat.com>
> Cc: Paolo Bonzini <pbonz...@redhat.com>
> Cc: Fam Zheng <f...@redhat.com>
> Signed-off-by: Cornelia Huck <cornelia.h...@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 | 59 +++++++++++++--------
> include/hw/virtio/dataplane/vring-accessors.h | 75
> +++++++++++++++++++++++++++
> include/hw/virtio/dataplane/vring.h | 14 +----
> 7 files changed, 120 insertions(+), 38 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..bf2122b 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;
> @@ -154,7 +157,7 @@ bool vring_should_notify(VirtIODevice *vdev, Vring *vring)
> }
>
>
> -static int get_desc(Vring *vring, VirtQueueElement *elem,
> +static int get_desc(VirtIODevice *vdev, Vring *vring, VirtQueueElement *elem,
> struct vring_desc *desc)
> {
> unsigned *num;
> @@ -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 */
> @@ -263,7 +276,7 @@ static int get_indirect(Vring *vring, VirtQueueElement
> *elem,
> return -EFAULT;
> }
>
> - ret = get_desc(vring, elem, &desc);
> + ret = get_desc(vdev, vring, elem, &desc);
> if (ret < 0) {
> vring->broken |= (ret == -EFAULT);
> return ret;
> @@ -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,20 +378,20 @@ 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;
> }
> continue;
> }
>
> - ret = get_desc(vring, elem, &desc);
> + ret = get_desc(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 */
