| Message ID | 1510867014-13423-1-git-send-email-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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;
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(-)