[Qemu-devel] [PATCH for-2.11] hw/net/eepro100: Fix endianness problem on big endian hosts

Thomas Huth posted 1 patch 6 years, 4 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1510867014-13423-1-git-send-email-thuth@redhat.com
Test checkpatch passed
Test docker passed
Test ppc passed
Test s390x passed
hw/net/eepro100.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
[Qemu-devel] [PATCH for-2.11] hw/net/eepro100: Fix endianness problem on big endian hosts
Posted by Thomas Huth 6 years, 4 months ago
Since commit 1865e288a823c764cd4344d ("Fix eepro100 simple transmission
mode"), the test/pxe-test is broken for the eepro100 device on big
endian hosts. However, it seems like that commit did not introduce the
problem, but just uncovered it: The EEPRO100State->tx.tbd_array_addr and
EEPRO100State->tx.tcb_bytes fields are already in host byte order, since
they have already been byte-swapped in the read_cb() function.
Thus byte-swapping them in tx_command() again results in the wrong
endianness. Removing the byte-swapping here fixes the pxe-test.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/net/eepro100.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index a63ed2c..03e00f7 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -756,8 +756,8 @@ static void read_cb(EEPRO100State *s)
 
 static void tx_command(EEPRO100State *s)
 {
-    uint32_t tbd_array = le32_to_cpu(s->tx.tbd_array_addr);
-    uint16_t tcb_bytes = (le16_to_cpu(s->tx.tcb_bytes) & 0x3fff);
+    uint32_t tbd_array = s->tx.tbd_array_addr;
+    uint16_t tcb_bytes = s->tx.tcb_bytes & 0x3fff;
     /* Sends larger than MAX_ETH_FRAME_SIZE are allowed, up to 2600 bytes. */
     uint8_t buf[2600];
     uint16_t size = 0;
-- 
1.8.3.1


Re: [Qemu-devel] [PATCH for-2.11] hw/net/eepro100: Fix endianness problem on big endian hosts
Posted by Jason Wang 6 years, 4 months ago

On 2017年11月17日 05:16, Thomas Huth wrote:
> Since commit 1865e288a823c764cd4344d ("Fix eepro100 simple transmission
> mode"), the test/pxe-test is broken for the eepro100 device on big
> endian hosts. However, it seems like that commit did not introduce the
> problem, but just uncovered it: The EEPRO100State->tx.tbd_array_addr and
> EEPRO100State->tx.tcb_bytes fields are already in host byte order, since
> they have already been byte-swapped in the read_cb() function.
> Thus byte-swapping them in tx_command() again results in the wrong
> endianness. Removing the byte-swapping here fixes the pxe-test.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   hw/net/eepro100.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
> index a63ed2c..03e00f7 100644
> --- a/hw/net/eepro100.c
> +++ b/hw/net/eepro100.c
> @@ -756,8 +756,8 @@ static void read_cb(EEPRO100State *s)
>   
>   static void tx_command(EEPRO100State *s)
>   {
> -    uint32_t tbd_array = le32_to_cpu(s->tx.tbd_array_addr);
> -    uint16_t tcb_bytes = (le16_to_cpu(s->tx.tcb_bytes) & 0x3fff);
> +    uint32_t tbd_array = s->tx.tbd_array_addr;
> +    uint16_t tcb_bytes = s->tx.tcb_bytes & 0x3fff;
>       /* Sends larger than MAX_ETH_FRAME_SIZE are allowed, up to 2600 bytes. */
>       uint8_t buf[2600];
>       uint16_t size = 0;

Applied.

Thanks

Re: [Qemu-devel] [PATCH for-2.11] hw/net/eepro100: Fix endianness problem on big endian hosts
Posted by Michael S. Tsirkin 6 years, 4 months ago
On Thu, Nov 16, 2017 at 10:16:54PM +0100, Thomas Huth wrote:
> Since commit 1865e288a823c764cd4344d ("Fix eepro100 simple transmission
> mode"), the test/pxe-test is broken for the eepro100 device on big
> endian hosts. However, it seems like that commit did not introduce the
> problem, but just uncovered it: The EEPRO100State->tx.tbd_array_addr and
> EEPRO100State->tx.tcb_bytes fields are already in host byte order, since
> they have already been byte-swapped in the read_cb() function.
> Thus byte-swapping them in tx_command() again results in the wrong
> endianness. Removing the byte-swapping here fixes the pxe-test.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>


Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Thomas, how about adding sparse endian-ness annotations
a la le32 in Linux? We could then use static checkers to
catch these bugs at build time.

> ---
>  hw/net/eepro100.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
> index a63ed2c..03e00f7 100644
> --- a/hw/net/eepro100.c
> +++ b/hw/net/eepro100.c
> @@ -756,8 +756,8 @@ static void read_cb(EEPRO100State *s)
>  
>  static void tx_command(EEPRO100State *s)
>  {
> -    uint32_t tbd_array = le32_to_cpu(s->tx.tbd_array_addr);
> -    uint16_t tcb_bytes = (le16_to_cpu(s->tx.tcb_bytes) & 0x3fff);
> +    uint32_t tbd_array = s->tx.tbd_array_addr;
> +    uint16_t tcb_bytes = s->tx.tcb_bytes & 0x3fff;
>      /* Sends larger than MAX_ETH_FRAME_SIZE are allowed, up to 2600 bytes. */
>      uint8_t buf[2600];
>      uint16_t size = 0;
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH for-2.11] hw/net/eepro100: Fix endianness problem on big endian hosts
Posted by Stefan Weil 6 years, 4 months ago
Am 17.11.2017 um 05:14 schrieb Michael S. Tsirkin:
> On Thu, Nov 16, 2017 at 10:16:54PM +0100, Thomas Huth wrote:
>> Since commit 1865e288a823c764cd4344d ("Fix eepro100 simple transmission
>> mode"), the test/pxe-test is broken for the eepro100 device on big
>> endian hosts. However, it seems like that commit did not introduce the
>> problem, but just uncovered it: The EEPRO100State->tx.tbd_array_addr and
>> EEPRO100State->tx.tcb_bytes fields are already in host byte order, since
>> they have already been byte-swapped in the read_cb() function.
>> Thus byte-swapping them in tx_command() again results in the wrong
>> endianness. Removing the byte-swapping here fixes the pxe-test.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> 
> 
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Thomas, how about adding sparse endian-ness annotations
> a la le32 in Linux? We could then use static checkers to
> catch these bugs at build time.

My main problem is that running big endian tests are
much more complex and time consuming simply because
my only big endian machines are QEMU virtual machines
(PPC* or MIPS*), so a test requires running QEMU
inside QEMU.

Nevertheless I had run such tests with network boot
and Linux guests as test cases, and they worked
(see comments in the header of eepro100.c).

So I wonder how the tests have to be enhanced to cover
more cases.

Stefan

Re: [Qemu-devel] [PATCH for-2.11] hw/net/eepro100: Fix endianness problem on big endian hosts
Posted by Thomas Huth 6 years, 4 months ago
On 17.11.2017 06:35, Stefan Weil wrote:
> Am 17.11.2017 um 05:14 schrieb Michael S. Tsirkin:
>> On Thu, Nov 16, 2017 at 10:16:54PM +0100, Thomas Huth wrote:
>>> Since commit 1865e288a823c764cd4344d ("Fix eepro100 simple transmission
>>> mode"), the test/pxe-test is broken for the eepro100 device on big
>>> endian hosts. However, it seems like that commit did not introduce the
>>> problem, but just uncovered it: The EEPRO100State->tx.tbd_array_addr and
>>> EEPRO100State->tx.tcb_bytes fields are already in host byte order, since
>>> they have already been byte-swapped in the read_cb() function.
>>> Thus byte-swapping them in tx_command() again results in the wrong
>>> endianness. Removing the byte-swapping here fixes the pxe-test.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>
>> Thomas, how about adding sparse endian-ness annotations
>> a la le32 in Linux? We could then use static checkers to
>> catch these bugs at build time.

Sounds like a good idea - but also like a bigger project. I don't have
time for that right now, so if somebody wants to have a look, you're
very welcome! (but I'll also put it on my way-too-big todo-list.txt, so
in case nobody else picks this up, maybe I can have a look at it in a
couple of months...)

> My main problem is that running big endian tests are
> much more complex and time consuming simply because
> my only big endian machines are QEMU virtual machines
> (PPC* or MIPS*), so a test requires running QEMU
> inside QEMU.
> 
> Nevertheless I had run such tests with network boot
> and Linux guests as test cases, and they worked
> (see comments in the header of eepro100.c).
> 
> So I wonder how the tests have to be enhanced to cover
> more cases.

I think this specific problem was "hidden" by the code that has been
removed with commit 1865e288a823c7, so there was no chance that you
could detect this with tests. As I wrote in the description, the
pxe-test worked fine in rc0, it just got broken in rc1.

But if you want to increase automatic test coverage, I think you should
try to improve tests/eepro100-test.c with some more code, e.g. something
similar as it is done in tests/e1000e-test.c.

 Thomas