Re: [Qemu-devel] [PATCH 1/2] dataplane: drop copy_in_vring_desc()
On Sun, 28 Jun 2015 15:03:20 +0200 Michael S. Tsirkin m...@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 m...@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.h...@de.ibm.com wrote: On Fri, 26 Jun 2015 09:32:21 +0200 Greg Kurz gk...@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 0x18e2517e 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 gk...@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()
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 m...@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.h...@de.ibm.com wrote: On Fri, 26 Jun 2015 09:32:21 +0200 Greg Kurz gk...@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 0x18e2517e 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 gk...@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()
On Sun, 28 Jun 2015 15:03:20 +0200 Michael S. Tsirkin m...@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 m...@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.h...@de.ibm.com wrote: On Fri, 26 Jun 2015 09:32:21 +0200 Greg Kurz gk...@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 0x18e2517e 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 gk...@linux.vnet.ibm.com --- hw/virtio/dataplane/vring.c | 14 ++ 1 file changed, 2 insertions(+), 12 deletions(-)
[Qemu-devel] [PATCH 1/2] dataplane: drop copy_in_vring_desc()
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 0x18e2517e 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 gk...@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()
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.h...@de.ibm.com wrote: On Fri, 26 Jun 2015 09:32:21 +0200 Greg Kurz gk...@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 0x18e2517e 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 gk...@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()
On Fri, 26 Jun 2015 18:28:42 +0200 Michael S. Tsirkin m...@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.h...@de.ibm.com wrote: On Fri, 26 Jun 2015 09:32:21 +0200 Greg Kurz gk...@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 0x18e2517e 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 gk...@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()
On Fri, 26 Jun 2015 12:28:45 +0200 Cornelia Huck cornelia.h...@de.ibm.com wrote: On Fri, 26 Jun 2015 09:32:21 +0200 Greg Kurz gk...@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 0x18e2517e 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 gk...@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()
On Fri, 26 Jun 2015 09:32:21 +0200 Greg Kurz gk...@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 0x18e2517e 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 gk...@linux.vnet.ibm.com --- hw/virtio/dataplane/vring.c | 14 ++ 1 file changed, 2 insertions(+), 12 deletions(-)
