[Qemu-devel] [PATCH 0/2] dataplane: cross-endian fixes

[Qemu-devel] [PATCH 0/2] dataplane: cross-endian fixes

[Greg Kurz]

This series fixes two issues that prevent cross-endian setups to use
dataplane. I had already posted tentative fixes, each in its own
thread of mail, but I think it is better to send them altogether.

Especially, note that patch 1/2 is the same as:

http://patchwork.ozlabs.org/patch/488482/

with some minor changes in the commit message.

This series was tested with ppc64 and ppc64le hosts. No regression was
observed on x86_64.

---

Greg Kurz (2):
      dataplane: drop copy_in_vring_desc()
      dataplane: fix cross-endian issues


 hw/virtio/dataplane/vring.c |   20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

--
Greg

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

[Greg Kurz]

During early virtio 1.0 devel, there were several proposals about how to
deal with the 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 boot in SLOF (BE client) when host is ppc64le with the following
QEMU 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 result in a smaller patch.

This patch allows SLOF to boot the OS.

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 1/2] dataplane: drop copy_in_vring_desc()

[Cornelia Huck]

On Fri, 26 Jun 2015 09:32:21 +0200
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> During early virtio 1.0 devel, there were several proposals about how to
> deal with the 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 boot in SLOF (BE client) when host is ppc64le with the following
> QEMU 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 result in a smaller patch.

I'd prefer the revert, as the resulting code is nicer IMHO.

But your second patch should apply regardless.

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

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

[Greg Kurz]

On Fri, 26 Jun 2015 12:28:45 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Fri, 26 Jun 2015 09:32:21 +0200
> Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> 
> > During early virtio 1.0 devel, there were several proposals about how to
> > deal with the 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 boot in SLOF (BE client) when host is ppc64le with the following
> > QEMU 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 result in a smaller patch.
> 
> I'd prefer the revert, as the resulting code is nicer IMHO.
> 

Agreed. It is good to clear the endianness noise out of the real code. :)

> But your second patch should apply regardless.
> 

Thanks for your feedback.

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

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

[Michael S. Tsirkin]

On Fri, Jun 26, 2015 at 02:18:38PM +0200, Greg Kurz wrote:
> On Fri, 26 Jun 2015 12:28:45 +0200
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> 
> > On Fri, 26 Jun 2015 09:32:21 +0200
> > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> > 
> > > During early virtio 1.0 devel, there were several proposals about how to
> > > deal with the 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 boot in SLOF (BE client) when host is ppc64le with the following
> > > QEMU 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 result in a smaller patch.
> > 
> > I'd prefer the revert, as the resulting code is nicer IMHO.
> > 
> 
> Agreed. It is good to clear the endianness noise out of the real code. :)

Can you please send v2 that works the way you want it?

> > But your second patch should apply regardless.
> > 
> 
> Thanks for your feedback.
> 
> > > 
> > > This patch allows SLOF to boot the OS.
> > > 
> > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > > ---
> > >  hw/virtio/dataplane/vring.c |   14 ++------------
> > >  1 file changed, 2 insertions(+), 12 deletions(-)

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

[Greg Kurz]

On Fri, 26 Jun 2015 18:28:42 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Jun 26, 2015 at 02:18:38PM +0200, Greg Kurz wrote:
> > On Fri, 26 Jun 2015 12:28:45 +0200
> > Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> > 
> > > On Fri, 26 Jun 2015 09:32:21 +0200
> > > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> > > 
> > > > During early virtio 1.0 devel, there were several proposals about how to
> > > > deal with the 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 boot in SLOF (BE client) when host is ppc64le with the following
> > > > QEMU 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 result in a smaller patch.
> > > 
> > > I'd prefer the revert, as the resulting code is nicer IMHO.
> > > 
> > 
> > Agreed. It is good to clear the endianness noise out of the real code. :)
> 
> Can you please send v2 that works the way you want it?
> 

Taken from a previous mail by Stefan:

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

I guess you just have to take Cornelia's patch + Stefan's ack,
and patch 2/2 in my series + Cornelia's rb, and we are all set. :)

FWIW I tested and it works exactly the same.

> > > But your second patch should apply regardless.
> > > 
> > 
> > Thanks for your feedback.
> > 
> > > > 
> > > > This patch allows SLOF to boot the OS.
> > > > 
> > > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > > > ---
> > > >  hw/virtio/dataplane/vring.c |   14 ++------------
> > > >  1 file changed, 2 insertions(+), 12 deletions(-)
> 

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

[Michael S. Tsirkin]

On Fri, Jun 26, 2015 at 07:05:07PM +0200, Greg Kurz wrote:
> On Fri, 26 Jun 2015 18:28:42 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Fri, Jun 26, 2015 at 02:18:38PM +0200, Greg Kurz wrote:
> > > On Fri, 26 Jun 2015 12:28:45 +0200
> > > Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> > > 
> > > > On Fri, 26 Jun 2015 09:32:21 +0200
> > > > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> > > > 
> > > > > During early virtio 1.0 devel, there were several proposals about how to
> > > > > deal with the 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 boot in SLOF (BE client) when host is ppc64le with the following
> > > > > QEMU 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 result in a smaller patch.
> > > > 
> > > > I'd prefer the revert, as the resulting code is nicer IMHO.
> > > > 
> > > 
> > > Agreed. It is good to clear the endianness noise out of the real code. :)
> > 
> > Can you please send v2 that works the way you want it?
> > 
> 
> Taken from a previous mail by Stefan:
> 
> "Cornelia already sent "[PATCH] Revert "dataplane: allow virtio-1
> devices" to revert f5a5628cf0b.  I acked it but the patch is going
> through Michael Tsirkin."
> 
> I guess you just have to take Cornelia's patch + Stefan's ack,
> and patch 2/2 in my series + Cornelia's rb, and we are all set. :)
> 
> FWIW I tested and it works exactly the same.

Sounds good.
Can you please test the pci branch from my tree, and confirm?



> > > > But your second patch should apply regardless.
> > > > 
> > > 
> > > Thanks for your feedback.
> > > 
> > > > > 
> > > > > This patch allows SLOF to boot the OS.
> > > > > 
> > > > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > > > > ---
> > > > >  hw/virtio/dataplane/vring.c |   14 ++------------
> > > > >  1 file changed, 2 insertions(+), 12 deletions(-)
> > 

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

[Greg Kurz]

On Sun, 28 Jun 2015 15:03:20 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Jun 26, 2015 at 07:05:07PM +0200, Greg Kurz wrote:
> > On Fri, 26 Jun 2015 18:28:42 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Fri, Jun 26, 2015 at 02:18:38PM +0200, Greg Kurz wrote:
> > > > On Fri, 26 Jun 2015 12:28:45 +0200
> > > > Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> > > > 
> > > > > On Fri, 26 Jun 2015 09:32:21 +0200
> > > > > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> > > > > 
> > > > > > During early virtio 1.0 devel, there were several proposals about how to
> > > > > > deal with the 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 boot in SLOF (BE client) when host is ppc64le with the following
> > > > > > QEMU 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 result in a smaller patch.
> > > > > 
> > > > > I'd prefer the revert, as the resulting code is nicer IMHO.
> > > > > 
> > > > 
> > > > Agreed. It is good to clear the endianness noise out of the real code. :)
> > > 
> > > Can you please send v2 that works the way you want it?
> > > 
> > 
> > Taken from a previous mail by Stefan:
> > 
> > "Cornelia already sent "[PATCH] Revert "dataplane: allow virtio-1
> > devices" to revert f5a5628cf0b.  I acked it but the patch is going
> > through Michael Tsirkin."
> > 
> > I guess you just have to take Cornelia's patch + Stefan's ack,
> > and patch 2/2 in my series + Cornelia's rb, and we are all set. :)
> > 
> > FWIW I tested and it works exactly the same.
> 
> Sounds good.
> Can you please test the pci branch from my tree, and confirm?
> 

Sure ! I shall do it tomorrow.

Thanks.

--
Greg

> 
> 
> > > > > But your second patch should apply regardless.
> > > > > 
> > > > 
> > > > Thanks for your feedback.
> > > > 
> > > > > > 
> > > > > > This patch allows SLOF to boot the OS.
> > > > > > 
> > > > > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > > > > > ---
> > > > > >  hw/virtio/dataplane/vring.c |   14 ++------------
> > > > > >  1 file changed, 2 insertions(+), 12 deletions(-)
> > > 
> 

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

[Greg Kurz]

On Sun, 28 Jun 2015 15:03:20 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Jun 26, 2015 at 07:05:07PM +0200, Greg Kurz wrote:
> > On Fri, 26 Jun 2015 18:28:42 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Fri, Jun 26, 2015 at 02:18:38PM +0200, Greg Kurz wrote:
> > > > On Fri, 26 Jun 2015 12:28:45 +0200
> > > > Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> > > > 
> > > > > On Fri, 26 Jun 2015 09:32:21 +0200
> > > > > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> > > > > 
> > > > > > During early virtio 1.0 devel, there were several proposals about how to
> > > > > > deal with the 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 boot in SLOF (BE client) when host is ppc64le with the following
> > > > > > QEMU 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 result in a smaller patch.
> > > > > 
> > > > > I'd prefer the revert, as the resulting code is nicer IMHO.
> > > > > 
> > > > 
> > > > Agreed. It is good to clear the endianness noise out of the real code. :)
> > > 
> > > Can you please send v2 that works the way you want it?
> > > 
> > 
> > Taken from a previous mail by Stefan:
> > 
> > "Cornelia already sent "[PATCH] Revert "dataplane: allow virtio-1
> > devices" to revert f5a5628cf0b.  I acked it but the patch is going
> > through Michael Tsirkin."
> > 
> > I guess you just have to take Cornelia's patch + Stefan's ack,
> > and patch 2/2 in my series + Cornelia's rb, and we are all set. :)
> > 
> > FWIW I tested and it works exactly the same.
> 
> Sounds good.
> Can you please test the pci branch from my tree, and confirm?
> 

I have tested both ppc64/ppc64le and pp64le/ppc64 setups. I confirm
your pci branch supports cross-endian dataplane (legacy only) as
expected.

Cheers.

--
Greg

> 
> 
> > > > > But your second patch should apply regardless.
> > > > > 
> > > > 
> > > > Thanks for your feedback.
> > > > 
> > > > > > 
> > > > > > This patch allows SLOF to boot the OS.
> > > > > > 
> > > > > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > > > > > ---
> > > > > >  hw/virtio/dataplane/vring.c |   14 ++------------
> > > > > >  1 file changed, 2 insertions(+), 12 deletions(-)
> > > 
> 

[Qemu-devel] [PATCH 2/2] dataplane: fix cross-endian issues

[Greg Kurz]

Accesses to vring_avail_event and vring_used_event must honor the queue
endianness.

This patch allows cross-endian setups to use dataplane (tested with ppc64
on ppc64le, and vice-versa).

Suggested-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/virtio/dataplane/vring.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
index 3fa421b9d773..67d411919d22 100644
--- a/hw/virtio/dataplane/vring.c
+++ b/hw/virtio/dataplane/vring.c
@@ -153,7 +153,8 @@ bool vring_should_notify(VirtIODevice *vdev, Vring *vring)
         return true;
     }
 
-    return vring_need_event(vring_used_event(&vring->vr), new, old);
+    return vring_need_event(virtio_tswap16(vdev, vring_used_event(&vring->vr)),
+                            new, old);
 }
 
 
@@ -397,7 +398,8 @@ int vring_pop(VirtIODevice *vdev, Vring *vring,
     /* On success, increment avail index. */
     vring->last_avail_idx++;
     if (virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
-        vring_avail_event(&vring->vr) = vring->last_avail_idx;
+        vring_avail_event(&vring->vr) =
+            virtio_tswap16(vdev, vring->last_avail_idx);
     }
 
     return head;

Re: [Qemu-devel] [PATCH 2/2] dataplane: fix cross-endian issues

[Cornelia Huck]

On Fri, 26 Jun 2015 09:32:28 +0200
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> Accesses to vring_avail_event and vring_used_event must honor the queue
> endianness.
> 
> This patch allows cross-endian setups to use dataplane (tested with ppc64
> on ppc64le, and vice-versa).
> 
> Suggested-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  hw/virtio/dataplane/vring.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [Qemu-devel] [PATCH 0/2] dataplane: cross-endian fixes

[Stefan Hajnoczi]

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

On Fri, Jun 26, 2015 at 09:32:12AM +0200, Greg Kurz wrote:
> This series fixes two issues that prevent cross-endian setups to use
> dataplane. I had already posted tentative fixes, each in its own
> thread of mail, but I think it is better to send them altogether.
> 
> Especially, note that patch 1/2 is the same as:
> 
> http://patchwork.ozlabs.org/patch/488482/
> 
> with some minor changes in the commit message.
> 
> This series was tested with ppc64 and ppc64le hosts. No regression was
> observed on x86_64.
> 
> ---
> 
> Greg Kurz (2):
>       dataplane: drop copy_in_vring_desc()
>       dataplane: fix cross-endian issues
> 
> 
>  hw/virtio/dataplane/vring.c |   20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)

This can go via Michael Tsirkin like the virtio-1 work.

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