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

[Thomas Huth]

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 <address@hidden>
>>
>> Reviewed-by: Michael S. Tsirkin <address@hidden>
>>
>> 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