| Message ID | 20150626073221.26832.58745.stgit@bahia.huguette.org |
|---|---|
| State | New |
| Headers | show |
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(-)
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(-)
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(-)
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(-) >
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(-) > >
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(-) > > > >
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(-) > > > >
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();
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(-)