[Qemu-devel] [PATCH for-2.11] hw/misc/ivshmem: Fix ivshmem_recv_msg() to also work on big endian systems

[Qemu-devel] [PATCH for-2.11] hw/misc/ivshmem: Fix ivshmem_recv_msg() to also work on big endian systems

[Thomas Huth]

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)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH for-2.11] hw/misc/ivshmem: Fix ivshmem_recv_msg() to also work on big endian systems

[Cornelia Huck]

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>

Re: [Qemu-devel] [PATCH for-2.11] hw/misc/ivshmem: Fix ivshmem_recv_msg() to also work on big endian systems

[Thomas Huth]

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

Re: [Qemu-devel] [PATCH for-2.11] hw/misc/ivshmem: Fix ivshmem_recv_msg() to also work on big endian systems

[Marc-André Lureau]

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

Re: [Qemu-devel] [PATCH for-2.11] hw/misc/ivshmem: Fix ivshmem_recv_msg() to also work on big endian systems

[Philippe Mathieu-Daudé]

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)
> 

Re: [Qemu-devel] [PATCH for-2.11] hw/misc/ivshmem: Fix ivshmem_recv_msg() to also work on big endian systems

[Thomas Huth]

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

Re: [Qemu-devel] [PATCH for-2.11] hw/misc/ivshmem: Fix ivshmem_recv_msg() to also work on big endian systems

[Cornelia Huck]

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.

Re: [Qemu-devel] [PATCH for-2.11] hw/misc/ivshmem: Fix ivshmem_recv_msg() to also work on big endian systems

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH for-2.11] hw/misc/ivshmem: Fix ivshmem_recv_msg() to also work on big endian systems

[David Gibson]

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

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.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH for-2.11] hw/misc/ivshmem: Fix ivshmem_recv_msg() to also work on big endian systems

[Philippe Mathieu-Daudé]

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

Re: [Qemu-devel] [PATCH for-2.11] hw/misc/ivshmem: Fix ivshmem_recv_msg() to also work on big endian systems

[Thomas Huth]

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

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


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

Re: [Qemu-devel] [PATCH for-2.11] hw/misc/ivshmem: Fix ivshmem_recv_msg() to also work on big endian systems

[Cornelia Huck]

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' :)

Re: [Qemu-devel] [PATCH for-2.11] hw/misc/ivshmem: Fix ivshmem_recv_msg() to also work on big endian systems

[Michael Tokarev]

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