| Message ID | 1504100343-26607-1-git-send-email-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [for-2.11] hw/misc/ivshmem: Fix ivshmem_recv_msg() to also work on big endian systems | expand |
On Wed, 30 Aug 2017 15:39:03 +0200 Thomas Huth <thuth@redhat.com> wrote: > The "slow" ivshmem-tests currently fail when they are running on a > big endian host: > > $ uname -m > ppc64 > $ V=1 QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 tests/ivshmem-test -m slow > /x86_64/ivshmem/single: OK > /x86_64/ivshmem/hotplug: OK > /x86_64/ivshmem/memdev: OK > /x86_64/ivshmem/pair: OK > /x86_64/ivshmem/server-msi: qemu-system-x86_64: > -device ivshmem-doorbell,chardev=chr0,vectors=2: server sent invalid ID message > Broken pipe > > The problem is that the server side code in ivshmem_server_send_one_msg() > correctly translates all messages IDs into little endian 64-bit values, > but the client side code in the ivshmem_recv_msg() function does not swap > the byte order back. Fix it by passing the value through le64_to_cpu(). > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > hw/misc/ivshmem.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c > index 47a015f..b3ef3ec 100644 > --- a/hw/misc/ivshmem.c > +++ b/hw/misc/ivshmem.c > @@ -653,7 +653,7 @@ static int64_t ivshmem_recv_msg(IVShmemState *s, int *pfd, Error **errp) > } while (n < sizeof(msg)); > > *pfd = qemu_chr_fe_get_msgfd(&s->server_chr); > - return msg; > + return le64_to_cpu(msg); > } > > static void ivshmem_recv_setup(IVShmemState *s, Error **errp) This fixes the "invalid ID message" problem on s390x for me as well, and I run now into the same error as on x86 (which you also have a fix for IIRC), so I guess this is Tested-by: Cornelia Huck <cohuck@redhat.com>
Hi On Wed, Aug 30, 2017 at 3:43 PM Thomas Huth <thuth@redhat.com> wrote: > The "slow" ivshmem-tests currently fail when they are running on a > big endian host: > > $ uname -m > ppc64 > $ V=1 QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 > tests/ivshmem-test -m slow > /x86_64/ivshmem/single: OK > /x86_64/ivshmem/hotplug: OK > /x86_64/ivshmem/memdev: OK > /x86_64/ivshmem/pair: OK > /x86_64/ivshmem/server-msi: qemu-system-x86_64: > -device ivshmem-doorbell,chardev=chr0,vectors=2: server sent invalid ID > message > Broken pipe > > The problem is that the server side code in ivshmem_server_send_one_msg() > correctly translates all messages IDs into little endian 64-bit values, > but the client side code in the ivshmem_recv_msg() function does not swap > the byte order back. Fix it by passing the value through le64_to_cpu(). > > ivshmem_read() is correct though Signed-off-by: Thomas Huth <thuth@redhat.com> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- > hw/misc/ivshmem.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c > index 47a015f..b3ef3ec 100644 > --- a/hw/misc/ivshmem.c > +++ b/hw/misc/ivshmem.c > @@ -653,7 +653,7 @@ static int64_t ivshmem_recv_msg(IVShmemState *s, int > *pfd, Error **errp) > } while (n < sizeof(msg)); > > *pfd = qemu_chr_fe_get_msgfd(&s->server_chr); > - return msg; > + return le64_to_cpu(msg); > } > > static void ivshmem_recv_setup(IVShmemState *s, Error **errp) > -- > 1.8.3.1 > > > -- Marc-André Lureau
On 30.08.2017 16:13, Cornelia Huck wrote: > On Wed, 30 Aug 2017 15:39:03 +0200 > Thomas Huth <thuth@redhat.com> wrote: > >> The "slow" ivshmem-tests currently fail when they are running on a >> big endian host: >> >> $ uname -m >> ppc64 >> $ V=1 QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 tests/ivshmem-test -m slow >> /x86_64/ivshmem/single: OK >> /x86_64/ivshmem/hotplug: OK >> /x86_64/ivshmem/memdev: OK >> /x86_64/ivshmem/pair: OK >> /x86_64/ivshmem/server-msi: qemu-system-x86_64: >> -device ivshmem-doorbell,chardev=chr0,vectors=2: server sent invalid ID message >> Broken pipe >> >> The problem is that the server side code in ivshmem_server_send_one_msg() >> correctly translates all messages IDs into little endian 64-bit values, >> but the client side code in the ivshmem_recv_msg() function does not swap >> the byte order back. Fix it by passing the value through le64_to_cpu(). >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> [...] > This fixes the "invalid ID message" problem on s390x for me as well, > and I run now into the same error as on x86 (which you also have a fix > for IIRC) Yes, you also need my other patch, look for "tests: Fix broken ivshmem-server-msi/-irq tests" on the list. Thomas
On 08/30/2017 10:39 AM, Thomas Huth wrote: > The "slow" ivshmem-tests currently fail when they are running on a > big endian host: > > $ uname -m > ppc64 > $ V=1 QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 tests/ivshmem-test -m slow > /x86_64/ivshmem/single: OK > /x86_64/ivshmem/hotplug: OK > /x86_64/ivshmem/memdev: OK > /x86_64/ivshmem/pair: OK > /x86_64/ivshmem/server-msi: qemu-system-x86_64: > -device ivshmem-doorbell,chardev=chr0,vectors=2: server sent invalid ID message > Broken pipe > > The problem is that the server side code in ivshmem_server_send_one_msg() > correctly translates all messages IDs into little endian 64-bit values, > but the client side code in the ivshmem_recv_msg() function does not swap > the byte order back. Fix it by passing the value through le64_to_cpu(). Yes, we lack BE testing :( I read we can run docker on s390x, I wonder if we can ask for a FOSS account on some to run the test suite. > > Signed-off-by: Thomas Huth <thuth@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/misc/ivshmem.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c > index 47a015f..b3ef3ec 100644 > --- a/hw/misc/ivshmem.c > +++ b/hw/misc/ivshmem.c > @@ -653,7 +653,7 @@ static int64_t ivshmem_recv_msg(IVShmemState *s, int *pfd, Error **errp) > } while (n < sizeof(msg)); > > *pfd = qemu_chr_fe_get_msgfd(&s->server_chr); > - return msg; > + return le64_to_cpu(msg); > } > > static void ivshmem_recv_setup(IVShmemState *s, Error **errp) >
On 30.08.2017 16:53, Philippe Mathieu-Daudé wrote: > On 08/30/2017 10:39 AM, Thomas Huth wrote: >> The "slow" ivshmem-tests currently fail when they are running on a >> big endian host: >> >> $ uname -m >> ppc64 >> $ V=1 QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 >> tests/ivshmem-test -m slow >> /x86_64/ivshmem/single: OK >> /x86_64/ivshmem/hotplug: OK >> /x86_64/ivshmem/memdev: OK >> /x86_64/ivshmem/pair: OK >> /x86_64/ivshmem/server-msi: qemu-system-x86_64: >> -device ivshmem-doorbell,chardev=chr0,vectors=2: server sent invalid >> ID message >> Broken pipe >> >> The problem is that the server side code in ivshmem_server_send_one_msg() >> correctly translates all messages IDs into little endian 64-bit values, >> but the client side code in the ivshmem_recv_msg() function does not swap >> the byte order back. Fix it by passing the value through le64_to_cpu(). > > Yes, we lack BE testing :( As far as I know, some people are already running the tests on s390x and ppc64 ... the problem is that apparently nobody is running with SPEED=slow there - that's why this problem slipped through so far. Maybe we should switch to the SPEED=slow by default in the Makefile? Thomas
On 30 August 2017 at 15:53, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > On 08/30/2017 10:39 AM, Thomas Huth wrote: >> The problem is that the server side code in ivshmem_server_send_one_msg() >> correctly translates all messages IDs into little endian 64-bit values, >> but the client side code in the ivshmem_recv_msg() function does not swap >> the byte order back. Fix it by passing the value through le64_to_cpu(). > > > Yes, we lack BE testing :( My pre-pull-request test set includes s390 and ppc64 bigendian. This one was just missed because the 'slow' tests aren't in 'make check'. thanks -- PMM
On Wed, 30 Aug 2017 16:59:03 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 30.08.2017 16:53, Philippe Mathieu-Daudé wrote: > > On 08/30/2017 10:39 AM, Thomas Huth wrote: > >> The "slow" ivshmem-tests currently fail when they are running on a > >> big endian host: > >> > >> $ uname -m > >> ppc64 > >> $ V=1 QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 > >> tests/ivshmem-test -m slow > >> /x86_64/ivshmem/single: OK > >> /x86_64/ivshmem/hotplug: OK > >> /x86_64/ivshmem/memdev: OK > >> /x86_64/ivshmem/pair: OK > >> /x86_64/ivshmem/server-msi: qemu-system-x86_64: > >> -device ivshmem-doorbell,chardev=chr0,vectors=2: server sent invalid > >> ID message > >> Broken pipe > >> > >> The problem is that the server side code in ivshmem_server_send_one_msg() > >> correctly translates all messages IDs into little endian 64-bit values, > >> but the client side code in the ivshmem_recv_msg() function does not swap > >> the byte order back. Fix it by passing the value through le64_to_cpu(). > > > > Yes, we lack BE testing :( > > As far as I know, some people are already running the tests on s390x and > ppc64 ... the problem is that apparently nobody is running with > SPEED=slow there - that's why this problem slipped through so far. > Maybe we should switch to the SPEED=slow by default in the Makefile? Not sure whether that is a good idea. /me adding this to my s390x tests should help, though.
On Wed, Aug 30, 2017 at 03:59:07PM +0100, Peter Maydell wrote: > On 30 August 2017 at 15:53, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > On 08/30/2017 10:39 AM, Thomas Huth wrote: > >> The problem is that the server side code in ivshmem_server_send_one_msg() > >> correctly translates all messages IDs into little endian 64-bit values, > >> but the client side code in the ivshmem_recv_msg() function does not swap > >> the byte order back. Fix it by passing the value through le64_to_cpu(). > > > > > > Yes, we lack BE testing :( > > My pre-pull-request test set includes s390 and ppc64 bigendian. > This one was just missed because the 'slow' tests aren't in > 'make check'. I'm not what makes sense for staging this fix. I could take it through my tree, but it's not an obvious match, since this isn't really related to ppc at all.
On 09/10/2017 12:24 AM, David Gibson wrote: > I'm not what makes sense for staging this fix. I could take it > through my tree, but it's not an obvious match, since this isn't > really related to ppc at all. Paolo's misc? :D
On 10.09.2017 05:24, David Gibson wrote: > On Wed, Aug 30, 2017 at 03:59:07PM +0100, Peter Maydell wrote: >> On 30 August 2017 at 15:53, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >>> On 08/30/2017 10:39 AM, Thomas Huth wrote: >>>> The problem is that the server side code in ivshmem_server_send_one_msg() >>>> correctly translates all messages IDs into little endian 64-bit values, >>>> but the client side code in the ivshmem_recv_msg() function does not swap >>>> the byte order back. Fix it by passing the value through le64_to_cpu(). >>> >>> >>> Yes, we lack BE testing :( >> >> My pre-pull-request test set includes s390 and ppc64 bigendian. >> This one was just missed because the 'slow' tests aren't in >> 'make check'. > > I'm not what makes sense for staging this fix. I could take it > through my tree, but it's not an obvious match, since this isn't > really related to ppc at all. There does not seem to be maintainer for ivshmem, as far as I can see, so I'd say any tree that is distantly related is fine. So I think you could either take it through your ppc tree (since the bug affects big endian ppc machines), or maybe Cornelia could take it through the s390x tree (since we've seen the problem on the big endian s390x machines, too). Otherwise I have to wait and see whether it goes through misc or trivial, I guess... Thomas
On Mon, 11 Sep 2017 04:35:08 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 10.09.2017 05:24, David Gibson wrote: > > On Wed, Aug 30, 2017 at 03:59:07PM +0100, Peter Maydell wrote: > >> On 30 August 2017 at 15:53, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > >>> On 08/30/2017 10:39 AM, Thomas Huth wrote: > >>>> The problem is that the server side code in ivshmem_server_send_one_msg() > >>>> correctly translates all messages IDs into little endian 64-bit values, > >>>> but the client side code in the ivshmem_recv_msg() function does not swap > >>>> the byte order back. Fix it by passing the value through le64_to_cpu(). > >>> > >>> > >>> Yes, we lack BE testing :( > >> > >> My pre-pull-request test set includes s390 and ppc64 bigendian. > >> This one was just missed because the 'slow' tests aren't in > >> 'make check'. > > > > I'm not what makes sense for staging this fix. I could take it > > through my tree, but it's not an obvious match, since this isn't > > really related to ppc at all. > > There does not seem to be maintainer for ivshmem, as far as I can see, > so I'd say any tree that is distantly related is fine. So I think you > could either take it through your ppc tree (since the bug affects big > endian ppc machines), or maybe Cornelia could take it through the s390x > tree (since we've seen the problem on the big endian s390x machines, > too). Otherwise I have to wait and see whether it goes through misc or > trivial, I guess... I'll queue it; and whoever sends the first pull request, 'wins' :)
30.08.2017 16:39, Thomas Huth пишет: > The "slow" ivshmem-tests currently fail when they are running on a > big endian host: Applied to -trivial, thanks! /mjt
diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c index 47a015f..b3ef3ec 100644 --- a/hw/misc/ivshmem.c +++ b/hw/misc/ivshmem.c @@ -653,7 +653,7 @@ static int64_t ivshmem_recv_msg(IVShmemState *s, int *pfd, Error **errp) } while (n < sizeof(msg)); *pfd = qemu_chr_fe_get_msgfd(&s->server_chr); - return msg; + return le64_to_cpu(msg); } static void ivshmem_recv_setup(IVShmemState *s, Error **errp)
The "slow" ivshmem-tests currently fail when they are running on a big endian host: $ uname -m ppc64 $ V=1 QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 tests/ivshmem-test -m slow /x86_64/ivshmem/single: OK /x86_64/ivshmem/hotplug: OK /x86_64/ivshmem/memdev: OK /x86_64/ivshmem/pair: OK /x86_64/ivshmem/server-msi: qemu-system-x86_64: -device ivshmem-doorbell,chardev=chr0,vectors=2: server sent invalid ID message Broken pipe The problem is that the server side code in ivshmem_server_send_one_msg() correctly translates all messages IDs into little endian 64-bit values, but the client side code in the ivshmem_recv_msg() function does not swap the byte order back. Fix it by passing the value through le64_to_cpu(). Signed-off-by: Thomas Huth <thuth@redhat.com> --- hw/misc/ivshmem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)