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 <th...@redhat.com>
>>
>> Reviewed-by: Michael S. Tsirkin <m...@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
