[Qemu-devel] [PATCH] dataplane: drop copy_in_vring_desc()

[Qemu-devel] [PATCH] dataplane: drop copy_in_vring_desc()

[Greg Kurz]

During early virtio 1.0 devel, there were several proposals about how to
deal endianness of the vring descriptor fields:
- convert the decriptor to host endianness in a single place, and use its
  fields directly in the code
- keep the descriptor untouched and use virtio memory helpers to access its
  fields with the appropriate endianness

It seems like both approaches got merged: commit f5a5628cf0b6 introduces
an extra swap that negates the one brought by commit b0e5d90ebc3e. This
breaks cross-endian setups with the following error:

Failed to map descriptor addr 0x18e2517e00000000 len 268435456

A solution could be to revert f5a5628cf0b6, but dropping copy_in_vring_desc()
is equivalent and gives a smaller patch.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/virtio/dataplane/vring.c |   14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
index 35891856ee06..3fa421b9d773 100644
--- a/hw/virtio/dataplane/vring.c
+++ b/hw/virtio/dataplane/vring.c
@@ -207,16 +207,6 @@ static int get_desc(VirtIODevice *vdev, 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(VirtIODevice *vdev, Vring *vring,
                         VirtQueueElement *elem, struct vring_desc *indirect)
@@ -261,7 +251,7 @@ static int get_indirect(VirtIODevice *vdev, Vring *vring,
             vring->broken = true;
             return -EFAULT;
         }
-        copy_in_vring_desc(vdev, desc_ptr, &desc);
+        desc = *desc_ptr;
         memory_region_unref(mr);
 
         /* Ensure descriptor has been loaded before accessing fields */
@@ -383,7 +373,7 @@ int vring_pop(VirtIODevice *vdev, Vring *vring,
             ret = -EFAULT;
             goto out;
         }
-        copy_in_vring_desc(vdev, &vring->vr.desc[i], &desc);
+        desc = vring->vr.desc[i];
 
         /* Ensure descriptor is loaded before accessing fields */
         barrier();

Re: [Qemu-devel] [PATCH] dataplane: drop copy_in_vring_desc()

[Stefan Hajnoczi]

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

On Thu, Jun 25, 2015 at 05:26:09PM +0200, Greg Kurz wrote:
> During early virtio 1.0 devel, there were several proposals about how to
> deal endianness of the vring descriptor fields:
> - convert the decriptor to host endianness in a single place, and use its
>   fields directly in the code
> - keep the descriptor untouched and use virtio memory helpers to access its
>   fields with the appropriate endianness
> 
> It seems like both approaches got merged: commit f5a5628cf0b6 introduces
> an extra swap that negates the one brought by commit b0e5d90ebc3e. This
> breaks cross-endian setups with the following error:
> 
> Failed to map descriptor addr 0x18e2517e00000000 len 268435456
> 
> A solution could be to revert f5a5628cf0b6, but dropping copy_in_vring_desc()
> is equivalent and gives a smaller patch.

Cornelia already sent "[PATCH] Revert "dataplane: allow virtio-1
devices" to revert f5a5628cf0b.  I acked it but the patch is going
through Michael Tsirkin.

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

Re: [Qemu-devel] [PATCH] dataplane: drop copy_in_vring_desc()

[Greg Kurz]

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

On Fri, 26 Jun 2015 11:00:45 +0100
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Thu, Jun 25, 2015 at 05:26:09PM +0200, Greg Kurz wrote:
> > During early virtio 1.0 devel, there were several proposals about how to
> > deal endianness of the vring descriptor fields:
> > - convert the decriptor to host endianness in a single place, and use its
> >   fields directly in the code
> > - keep the descriptor untouched and use virtio memory helpers to access its
> >   fields with the appropriate endianness
> > 
> > It seems like both approaches got merged: commit f5a5628cf0b6 introduces
> > an extra swap that negates the one brought by commit b0e5d90ebc3e. This
> > breaks cross-endian setups with the following error:
> > 
> > Failed to map descriptor addr 0x18e2517e00000000 len 268435456
> > 
> > A solution could be to revert f5a5628cf0b6, but dropping copy_in_vring_desc()
> > is equivalent and gives a smaller patch.
> 
> Cornelia already sent "[PATCH] Revert "dataplane: allow virtio-1
> devices" to revert f5a5628cf0b.  I acked it but the patch is going
> through Michael Tsirkin.

Indeed... my bad, I overlooked that thread...

--
G

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]