From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36096) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XtKLY-0001Qq-K9 for qemu-devel@nongnu.org; Tue, 25 Nov 2014 12:56:25 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XtKLP-0002WN-88 for qemu-devel@nongnu.org; Tue, 25 Nov 2014 12:56:16 -0500 Received: from e06smtp16.uk.ibm.com ([195.75.94.112]:48847) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XtKLO-0002VV-Tj for qemu-devel@nongnu.org; Tue, 25 Nov 2014 12:56:07 -0500 Received: from /spool/local by e06smtp16.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 25 Nov 2014 17:56:03 -0000 Received: from b06cxnps4076.portsmouth.uk.ibm.com (d06relay13.portsmouth.uk.ibm.com [9.149.109.198]) by d06dlp02.portsmouth.uk.ibm.com (Postfix) with ESMTP id 159232190045 for ; Tue, 25 Nov 2014 17:55:33 +0000 (GMT) Received: from d06av03.portsmouth.uk.ibm.com (d06av03.portsmouth.uk.ibm.com [9.149.37.213]) by b06cxnps4076.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id sAPHu0mm61014100 for ; Tue, 25 Nov 2014 17:56:00 GMT Received: from d06av03.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av03.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id sAPHtxQX010747 for ; Tue, 25 Nov 2014 10:56:00 -0700 Date: Tue, 25 Nov 2014 18:55:56 +0100 From: Greg Kurz Message-ID: <20141125185556.44e23979@bahia.local> In-Reply-To: <1416921863-16523-8-git-send-email-cornelia.huck@de.ibm.com> References: <1416921863-16523-1-git-send-email-cornelia.huck@de.ibm.com> <1416921863-16523-8-git-send-email-cornelia.huck@de.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH RFC v2 07/12] dataplane: allow virtio-1 devices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cornelia Huck Cc: thuth@linux.vnet.ibm.com, mst@redhat.com, qemu-devel@nongnu.org, kvm@vger.kernel.org, virtualization@lists.linux-foundation.org On Tue, 25 Nov 2014 14:24:18 +0100 Cornelia Huck wrote: > Handle endianness conversion for virtio-1 virtqueues correctly. >=20 > Note that dataplane now needs to be built per-target. >=20 > Signed-off-by: Cornelia Huck > --- We still have the same error as in your previous post... In file included from include/hw/virtio/dataplane/vring.h:23:0, from include/hw/virtio/virtio-scsi.h:21, from hw/virtio/virtio-pci.c:24: include/hw/virtio/virtio-access.h: In function =E2=80=98virtio_access_is_bi= g_endian=E2=80=99: include/hw/virtio/virtio-access.h:28:15: error: attempt to use poisoned "TA= RGET_WORDS_BIGENDIAN" #elif defined(TARGET_WORDS_BIGENDIAN) Either build virtio-pci per-target or probably better move the target taint= ed accessors to another file. > hw/block/dataplane/virtio-blk.c | 3 +- > hw/scsi/virtio-scsi-dataplane.c | 2 +- > hw/virtio/Makefile.objs | 2 +- > hw/virtio/dataplane/Makefile.objs | 2 +- > hw/virtio/dataplane/vring.c | 85 +++++++++++++++++++----------= ------ > include/hw/virtio/dataplane/vring.h | 64 ++++++++++++++++++++++++-- > 6 files changed, 113 insertions(+), 45 deletions(-) >=20 > diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-= blk.c > index 1222a37..c25878c 100644 > --- a/hw/block/dataplane/virtio-blk.c > +++ b/hw/block/dataplane/virtio-blk.c > @@ -16,6 +16,7 @@ > #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 "sysemu/block-backend.h" > #include "hw/virtio/virtio-blk.h" > @@ -75,7 +76,7 @@ static void complete_request_vring(VirtIOBlockReq *req,= unsigned char status) > VirtIOBlockDataPlane *s =3D req->dev->dataplane; > stb_p(&req->in->status, status); >=20 > - vring_push(&req->dev->dataplane->vring, &req->elem, > + vring_push(s->vdev, &req->dev->dataplane->vring, &req->elem, > req->qiov.size + sizeof(*req->in)); >=20 > /* Suppress notification to guest by BH and its scheduled > diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-datapl= ane.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 =3D VIRTIO_DEVICE(req->vring->parent); >=20 > - vring_push(&req->vring->vring, &req->elem, > + vring_push(vdev, &req->vring->vring, &req->elem, > req->qsgl.size + req->resp_iov.size); >=20 > 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 +=3D virtio-rng.o > common-obj-$(CONFIG_VIRTIO_PCI) +=3D virtio-pci.o > common-obj-y +=3D virtio-bus.o > common-obj-y +=3D virtio-mmio.o > -common-obj-$(CONFIG_VIRTIO) +=3D dataplane/ > +obj-$(CONFIG_VIRTIO) +=3D dataplane/ >=20 > obj-y +=3D virtio.o virtio-balloon.o=20 > obj-$(CONFIG_LINUX) +=3D vhost.o vhost-backend.o vhost-user.o > diff --git a/hw/virtio/dataplane/Makefile.objs b/hw/virtio/dataplane/Make= file.objs > index 9a8cfc0..753a9ca 100644 > --- a/hw/virtio/dataplane/Makefile.objs > +++ b/hw/virtio/dataplane/Makefile.objs > @@ -1 +1 @@ > -common-obj-y +=3D vring.o > +obj-y +=3D vring.o > diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c > index a051775..69809ff 100644 > --- a/hw/virtio/dataplane/vring.c > +++ b/hw/virtio/dataplane/vring.c > @@ -18,6 +18,7 @@ > #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" >=20 > @@ -83,7 +84,7 @@ bool vring_setup(Vring *vring, VirtIODevice *vdev, int = n) > vring_init(&vring->vr, virtio_queue_get_num(vdev, n), vring_ptr, 409= 6); >=20 > vring->last_avail_idx =3D virtio_queue_get_last_avail_idx(vdev, n); > - vring->last_used_idx =3D vring->vr.used->idx; > + vring->last_used_idx =3D vring_get_used_idx(vdev, vring); > vring->signalled_used =3D 0; > vring->signalled_used_valid =3D false; >=20 > @@ -104,7 +105,7 @@ void vring_teardown(Vring *vring, VirtIODevice *vdev,= int n) > void vring_disable_notification(VirtIODevice *vdev, Vring *vring) > { > if (!(vdev->guest_features[0] & (1 << VIRTIO_RING_F_EVENT_IDX))) { > - vring->vr.used->flags |=3D VRING_USED_F_NO_NOTIFY; > + vring_set_used_flags(vdev, vring, VRING_USED_F_NO_NOTIFY); > } > } >=20 > @@ -117,10 +118,10 @@ bool vring_enable_notification(VirtIODevice *vdev, = Vring *vring) > if (vdev->guest_features[0] & (1 << VIRTIO_RING_F_EVENT_IDX)) { > vring_avail_event(&vring->vr) =3D vring->vr.avail->idx; > } else { > - vring->vr.used->flags &=3D ~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); > } >=20 > /* This is stolen from linux/drivers/vhost/vhost.c:vhost_notify() */ > @@ -134,12 +135,13 @@ bool vring_should_notify(VirtIODevice *vdev, Vring = *vring) > smp_mb(); >=20 > if ((vdev->guest_features[0] & VIRTIO_F_NOTIFY_ON_EMPTY) && > - unlikely(vring->vr.avail->idx =3D=3D vring->last_avail_idx)) { > + unlikely(!vring_more_avail(vdev, vring))) { > return true; > } >=20 > if (!(vdev->guest_features[0] & 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 =3D vring->signalled_used; > v =3D vring->signalled_used_valid; > @@ -154,15 +156,18 @@ bool vring_should_notify(VirtIODevice *vdev, Vring = *vring) > } >=20 >=20 > -static int get_desc(Vring *vring, VirtQueueElement *elem, > +static int get_desc(VirtIODevice *vdev, Vring *vring, VirtQueueElement *= elem, > struct vring_desc *desc) > { > unsigned *num; > struct iovec *iov; > hwaddr *addr; > MemoryRegion *mr; > + int is_write =3D virtio_tswap16(vdev, desc->flags) & VRING_DESC_F_WR= ITE; > + uint32_t len =3D virtio_tswap32(vdev, desc->len); > + uint64_t desc_addr =3D virtio_tswap64(vdev, desc->addr); >=20 > - if (desc->flags & VRING_DESC_F_WRITE) { > + if (is_write) { > num =3D &elem->in_num; > iov =3D &elem->in_sg[*num]; > addr =3D &elem->in_addr[*num]; > @@ -186,44 +191,45 @@ static int get_desc(Vring *vring, VirtQueueElement = *elem, > } >=20 > /* TODO handle non-contiguous memory across region boundaries */ > - iov->iov_base =3D vring_map(&mr, desc->addr, desc->len, > - desc->flags & VRING_DESC_F_WRITE); > + iov->iov_base =3D vring_map(&mr, desc_addr, len, is_write); > if (!iov->iov_base) { > error_report("Failed to map descriptor addr %#" PRIx64 " len %u", > - (uint64_t)desc->addr, desc->len); > + (uint64_t)desc_addr, len); > return -EFAULT; > } >=20 > /* The MemoryRegion is looked up again and unref'ed later, leave the > * ref in place. */ > - iov->iov_len =3D desc->len; > - *addr =3D desc->addr; > + iov->iov_len =3D len; > + *addr =3D desc_addr; > *num +=3D 1; > return 0; > } >=20 > /* 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 *indir= ect) > { > struct vring_desc desc; > unsigned int i =3D 0, count, found =3D 0; > int ret; > + uint32_t len =3D virtio_tswap32(vdev, indirect->len); > + uint64_t addr =3D virtio_tswap64(vdev, indirect->addr); >=20 > /* Sanity check */ > - if (unlikely(indirect->len % sizeof(desc))) { > + if (unlikely(len % sizeof(desc))) { > error_report("Invalid length in indirect descriptor: " > "len %#x not multiple of %#zx", > - indirect->len, sizeof(desc)); > + len, sizeof(desc)); > vring->broken =3D true; > return -EFAULT; > } >=20 > - count =3D indirect->len / sizeof(desc); > + count =3D len / sizeof(desc); > /* Buffers are chained via a 16 bit next field, so > * we can have at most 2^16 of these. */ > if (unlikely(count > USHRT_MAX + 1)) { > - error_report("Indirect buffer length too big: %d", indirect->len= ); > + error_report("Indirect buffer length too big: %d", len); > vring->broken =3D true; > return -EFAULT; > } > @@ -234,12 +240,12 @@ static int get_indirect(Vring *vring, VirtQueueElem= ent *elem, >=20 > /* Translate indirect descriptor */ > desc_ptr =3D vring_map(&mr, > - indirect->addr + found * sizeof(desc), > + addr + found * sizeof(desc), > sizeof(desc), false); > if (!desc_ptr) { > error_report("Failed to map indirect descriptor " > "addr %#" PRIx64 " len %zu", > - (uint64_t)indirect->addr + found * sizeof(desc), > + (uint64_t)addr + found * sizeof(desc), > sizeof(desc)); > vring->broken =3D true; > return -EFAULT; > @@ -257,19 +263,20 @@ static int get_indirect(Vring *vring, VirtQueueElem= ent *elem, > return -EFAULT; > } >=20 > - if (unlikely(desc.flags & VRING_DESC_F_INDIRECT)) { > + if (unlikely(virtio_tswap16(vdev, desc.flags) > + & VRING_DESC_F_INDIRECT)) { > error_report("Nested indirect descriptor"); > vring->broken =3D true; > return -EFAULT; > } >=20 > - ret =3D get_desc(vring, elem, &desc); > + ret =3D get_desc(vdev, vring, elem, &desc); > if (ret < 0) { > vring->broken |=3D (ret =3D=3D -EFAULT); > return ret; > } > - i =3D desc.next; > - } while (desc.flags & VRING_DESC_F_NEXT); > + i =3D virtio_tswap16(vdev, desc.next); > + } while (virtio_tswap16(vdev, desc.flags) & VRING_DESC_F_NEXT); > return 0; > } >=20 > @@ -320,7 +327,7 @@ int vring_pop(VirtIODevice *vdev, Vring *vring, >=20 > /* Check it isn't doing very strange things with descriptor numbers.= */ > last_avail_idx =3D vring->last_avail_idx; > - avail_idx =3D vring->vr.avail->idx; > + avail_idx =3D vring_get_avail_idx(vdev, vring); > barrier(); /* load indices now and not again later */ >=20 > if (unlikely((uint16_t)(avail_idx - last_avail_idx) > num)) { > @@ -341,7 +348,7 @@ int vring_pop(VirtIODevice *vdev, Vring *vring, >=20 > /* Grab the next descriptor number they're advertising, and increment > * the index we've seen. */ > - head =3D vring->vr.avail->ring[last_avail_idx % num]; > + head =3D vring_get_avail_ring(vdev, vring, last_avail_idx % num); >=20 > elem->index =3D head; >=20 > @@ -370,21 +377,21 @@ int vring_pop(VirtIODevice *vdev, Vring *vring, > /* Ensure descriptor is loaded before accessing fields */ > barrier(); >=20 > - if (desc.flags & VRING_DESC_F_INDIRECT) { > - ret =3D get_indirect(vring, elem, &desc); > + if (virtio_tswap16(vdev, desc.flags) & VRING_DESC_F_INDIRECT) { > + ret =3D get_indirect(vdev, vring, elem, &desc); > if (ret < 0) { > goto out; > } > continue; > } >=20 > - ret =3D get_desc(vring, elem, &desc); > + ret =3D get_desc(vdev, vring, elem, &desc); > if (ret < 0) { > goto out; > } >=20 > - i =3D desc.next; > - } while (desc.flags & VRING_DESC_F_NEXT); > + i =3D virtio_tswap16(vdev, desc.next); > + } while (virtio_tswap16(vdev, desc.flags) & VRING_DESC_F_NEXT); >=20 > /* On success, increment avail index. */ > vring->last_avail_idx++; > @@ -407,9 +414,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 =3D elem->index; > uint16_t new; >=20 > @@ -422,14 +429,16 @@ void vring_push(Vring *vring, VirtQueueElement *ele= m, int len) >=20 > /* The virtqueue contains a ring of used buffers. Get a pointer to = the > * next entry in that used ring. */ > - used =3D &vring->vr.used->ring[vring->last_used_idx % vring->vr.num]; > - used->id =3D head; > - used->len =3D 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->v= r.num, > + len); >=20 > /* Make sure buffer is written before we update index. */ > smp_wmb(); >=20 > - new =3D vring->vr.used->idx =3D ++vring->last_used_idx; > + new =3D ++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 =3D false; > } > diff --git a/include/hw/virtio/dataplane/vring.h b/include/hw/virtio/data= plane/vring.h > index d3e086a..fde15f3 100644 > --- a/include/hw/virtio/dataplane/vring.h > +++ b/include/hw/virtio/dataplane/vring.h > @@ -20,6 +20,7 @@ > #include "qemu-common.h" > #include "hw/virtio/virtio_ring.h" > #include "hw/virtio/virtio.h" > +#include "hw/virtio/virtio-access.h" >=20 > typedef struct { > MemoryRegion *mr; /* memory region containing the vrin= g */ > @@ -31,15 +32,71 @@ typedef struct { > bool broken; /* was there a fatal error? */ > } Vring; >=20 > +static inline uint16_t vring_get_used_idx(VirtIODevice *vdev, Vring *vri= ng) > +{ > + 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 =3D virtio_tswap16(vdev, idx); > +} > + > +static inline uint16_t vring_get_avail_idx(VirtIODevice *vdev, Vring *vr= ing) > +{ > + return virtio_tswap16(vdev, vring->vr.avail->idx); > +} > + > +static inline uint16_t vring_get_avail_ring(VirtIODevice *vdev, Vring *v= ring, > + int i) > +{ > + return virtio_tswap16(vdev, vring->vr.avail->ring[i]); > +} > + > +static inline void vring_set_used_ring_id(VirtIODevice *vdev, Vring *vri= ng, > + int i, uint32_t id) > +{ > + vring->vr.used->ring[i].id =3D virtio_tswap32(vdev, id); > +} > + > +static inline void vring_set_used_ring_len(VirtIODevice *vdev, Vring *vr= ing, > + int i, uint32_t len) > +{ > + vring->vr.used->ring[i].len =3D virtio_tswap32(vdev, len); > +} > + > +static inline uint16_t vring_get_used_flags(VirtIODevice *vdev, Vring *v= ring) > +{ > + 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 |=3D virtio_tswap16(vdev, flags); > +} > + > +static inline void vring_clear_used_flags(VirtIODevice *vdev, Vring *vri= ng, > + uint16_t flags) > +{ > + vring->vr.used->flags &=3D virtio_tswap16(vdev, ~flags); > +} > + > static inline unsigned int vring_get_num(Vring *vring) > { > return vring->vr.num; > } >=20 > /* 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 !=3D vring->last_avail_idx; > + return vring_get_avail_idx(vdev, vring) !=3D vring->last_avail_idx; > } >=20 > /* Fail future vring_pop() and vring_push() calls until reset */ > @@ -54,6 +111,7 @@ void vring_disable_notification(VirtIODevice *vdev, Vr= ing *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); >=20 > #endif /* VRING_H */