| Message ID | 20221213125218.39868-4-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/ppc: Remove tswap() calls | expand |
On Tue, 13 Dec 2022 at 12:52, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > This partly revert commit d48751ed4f ("xilinx-ethlite: > Simplify byteswapping to/from brams") which states the > packet data is stored in big-endian. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > @@ -102,8 +102,8 @@ eth_read(void *opaque, hwaddr addr, unsigned int size) > D(qemu_log("%s " TARGET_FMT_plx "=%x\n", __func__, addr * 4, r)); > break; > > - default: > - r = tswap32(s->regs[addr]); > + default: /* Packet data */ > + r = be32_to_cpu(s->regs[addr]); > break; > } > return r; > @@ -160,8 +160,8 @@ eth_write(void *opaque, hwaddr addr, > s->regs[addr] = value; > break; > > - default: > - s->regs[addr] = tswap32(value); > + default: /* Packet data */ > + s->regs[addr] = cpu_to_be32(value); > break; > } > } This is a change of behaviour for this device in the qemu-system-microblazeel petalogix-s3adsp1800 board, because previously on that system the bytes of the rx buffer would appear in the registers in little-endian order and now they will appear in big-endian order. Edgar, do you know what the real hardware does here ? thanks -- PMM
On Tue, Dec 13, 2022 at 01:53:15PM +0000, Peter Maydell wrote: > On Tue, 13 Dec 2022 at 12:52, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > > > This partly revert commit d48751ed4f ("xilinx-ethlite: > > Simplify byteswapping to/from brams") which states the > > packet data is stored in big-endian. > > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > > @@ -102,8 +102,8 @@ eth_read(void *opaque, hwaddr addr, unsigned int size) > > D(qemu_log("%s " TARGET_FMT_plx "=%x\n", __func__, addr * 4, r)); > > break; > > > > - default: > > - r = tswap32(s->regs[addr]); > > + default: /* Packet data */ > > + r = be32_to_cpu(s->regs[addr]); > > break; > > } > > return r; > > @@ -160,8 +160,8 @@ eth_write(void *opaque, hwaddr addr, > > s->regs[addr] = value; > > break; > > > > - default: > > - s->regs[addr] = tswap32(value); > > + default: /* Packet data */ > > + s->regs[addr] = cpu_to_be32(value); > > break; > > } > > } > > This is a change of behaviour for this device in the > qemu-system-microblazeel petalogix-s3adsp1800 board, because > previously on that system the bytes of the rx buffer would > appear in the registers in little-endian order and now they > will appear in big-endian order. > > Edgar, do you know what the real hardware does here ? > Yeah, I think these tx/rx buffers (the default case with tswap32) should be modelled as plain RAM's (they are just RAM's on real HW). Because we're modeling as MMIO regs, I think we get into endianness trouble when the ethernet output logic treats the content as a blob (thus the need for byteswaps). Does that make sense? Cheers, Edgar
On Tue, 13 Dec 2022 at 14:14, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > > On Tue, Dec 13, 2022 at 01:53:15PM +0000, Peter Maydell wrote: > > On Tue, 13 Dec 2022 at 12:52, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > > > > > This partly revert commit d48751ed4f ("xilinx-ethlite: > > > Simplify byteswapping to/from brams") which states the > > > packet data is stored in big-endian. > > > > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > > > > @@ -102,8 +102,8 @@ eth_read(void *opaque, hwaddr addr, unsigned int size) > > > D(qemu_log("%s " TARGET_FMT_plx "=%x\n", __func__, addr * 4, r)); > > > break; > > > > > > - default: > > > - r = tswap32(s->regs[addr]); > > > + default: /* Packet data */ > > > + r = be32_to_cpu(s->regs[addr]); > > > break; > > > } > > > return r; > > > @@ -160,8 +160,8 @@ eth_write(void *opaque, hwaddr addr, > > > s->regs[addr] = value; > > > break; > > > > > > - default: > > > - s->regs[addr] = tswap32(value); > > > + default: /* Packet data */ > > > + s->regs[addr] = cpu_to_be32(value); > > > break; > > > } > > > } > > > > This is a change of behaviour for this device in the > > qemu-system-microblazeel petalogix-s3adsp1800 board, because > > previously on that system the bytes of the rx buffer would > > appear in the registers in little-endian order and now they > > will appear in big-endian order. > > > > Edgar, do you know what the real hardware does here ? > Yeah, I think these tx/rx buffers (the default case with tswap32) > should be modelled as plain RAM's (they are just RAM's on real HW). > Because we're modeling as MMIO regs, I think we get into endianness > trouble when the ethernet output logic treats the content as a blob > (thus the need for byteswaps). Does that make sense? As a concrete question: if I do a 32-bit load from the buffer register into a CPU register, do I get a different value on the BE microblaze hardware vs LE microblaze ? thanks -- PMM
On Tue, Dec 13, 2022 at 02:18:42PM +0000, Peter Maydell wrote: > On Tue, 13 Dec 2022 at 14:14, Edgar E. Iglesias > <edgar.iglesias@gmail.com> wrote: > > > > On Tue, Dec 13, 2022 at 01:53:15PM +0000, Peter Maydell wrote: > > > On Tue, 13 Dec 2022 at 12:52, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > > > > > > > This partly revert commit d48751ed4f ("xilinx-ethlite: > > > > Simplify byteswapping to/from brams") which states the > > > > packet data is stored in big-endian. > > > > > > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > > > > > > @@ -102,8 +102,8 @@ eth_read(void *opaque, hwaddr addr, unsigned int size) > > > > D(qemu_log("%s " TARGET_FMT_plx "=%x\n", __func__, addr * 4, r)); > > > > break; > > > > > > > > - default: > > > > - r = tswap32(s->regs[addr]); > > > > + default: /* Packet data */ > > > > + r = be32_to_cpu(s->regs[addr]); > > > > break; > > > > } > > > > return r; > > > > @@ -160,8 +160,8 @@ eth_write(void *opaque, hwaddr addr, > > > > s->regs[addr] = value; > > > > break; > > > > > > > > - default: > > > > - s->regs[addr] = tswap32(value); > > > > + default: /* Packet data */ > > > > + s->regs[addr] = cpu_to_be32(value); > > > > break; > > > > } > > > > } > > > > > > This is a change of behaviour for this device in the > > > qemu-system-microblazeel petalogix-s3adsp1800 board, because > > > previously on that system the bytes of the rx buffer would > > > appear in the registers in little-endian order and now they > > > will appear in big-endian order. > > > > > > Edgar, do you know what the real hardware does here ? > > > Yeah, I think these tx/rx buffers (the default case with tswap32) > > should be modelled as plain RAM's (they are just RAM's on real HW). > > Because we're modeling as MMIO regs, I think we get into endianness > > trouble when the ethernet output logic treats the content as a blob > > (thus the need for byteswaps). Does that make sense? > > As a concrete question: if I do a 32-bit load from the buffer > register into a CPU register, do I get a different value > on the BE microblaze hardware vs LE microblaze ? Yes, I beleive so. If the CPU stores the value and reads it back, you get the same. But the representation on the RAM's is different between LE/BE. But if the Ethernet logic writes Ethernet packet data into the buffer, LE and BE MicroBlazes will read differient values from the buffers. These buffer "registers" are just RAM's I beleive. Best regards, Edgar
On Tue, 13 Dec 2022 at 14:23, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > > On Tue, Dec 13, 2022 at 02:18:42PM +0000, Peter Maydell wrote: > > On Tue, 13 Dec 2022 at 14:14, Edgar E. Iglesias > > <edgar.iglesias@gmail.com> wrote: > > > > > > On Tue, Dec 13, 2022 at 01:53:15PM +0000, Peter Maydell wrote: > > > > On Tue, 13 Dec 2022 at 12:52, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > > > > > > > > > This partly revert commit d48751ed4f ("xilinx-ethlite: > > > > > Simplify byteswapping to/from brams") which states the > > > > > packet data is stored in big-endian. > > > > This is a change of behaviour for this device in the > > > > qemu-system-microblazeel petalogix-s3adsp1800 board, because > > > > previously on that system the bytes of the rx buffer would > > > > appear in the registers in little-endian order and now they > > > > will appear in big-endian order. > > > > > > > > Edgar, do you know what the real hardware does here ? > > > > > Yeah, I think these tx/rx buffers (the default case with tswap32) > > > should be modelled as plain RAM's (they are just RAM's on real HW). > > > Because we're modeling as MMIO regs, I think we get into endianness > > > trouble when the ethernet output logic treats the content as a blob > > > (thus the need for byteswaps). Does that make sense? > > > > As a concrete question: if I do a 32-bit load from the buffer > > register into a CPU register, do I get a different value > > on the BE microblaze hardware vs LE microblaze ? > > Yes, I beleive so. > > If the CPU stores the value and reads it back, you get the same. But > the representation on the RAM's is different between LE/BE. > But if the Ethernet logic writes Ethernet packet data into the buffer, > LE and BE MicroBlazes will read differient values from the buffers. > These buffer "registers" are just RAM's I beleive. Thanks. That suggests that the current code for this device is correct, and we would be breaking it on the LE platform if we applied this patch. I don't suppose you have a guest image for the boards which uses ethernet ? -- PMM
On Tue, Dec 13, 2022 at 03:22:54PM +0000, Peter Maydell wrote: > On Tue, 13 Dec 2022 at 14:23, Edgar E. Iglesias > <edgar.iglesias@gmail.com> wrote: > > > > On Tue, Dec 13, 2022 at 02:18:42PM +0000, Peter Maydell wrote: > > > On Tue, 13 Dec 2022 at 14:14, Edgar E. Iglesias > > > <edgar.iglesias@gmail.com> wrote: > > > > > > > > On Tue, Dec 13, 2022 at 01:53:15PM +0000, Peter Maydell wrote: > > > > > On Tue, 13 Dec 2022 at 12:52, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > > > > > > > > > > > This partly revert commit d48751ed4f ("xilinx-ethlite: > > > > > > Simplify byteswapping to/from brams") which states the > > > > > > packet data is stored in big-endian. > > > > > > This is a change of behaviour for this device in the > > > > > qemu-system-microblazeel petalogix-s3adsp1800 board, because > > > > > previously on that system the bytes of the rx buffer would > > > > > appear in the registers in little-endian order and now they > > > > > will appear in big-endian order. > > > > > > > > > > Edgar, do you know what the real hardware does here ? > > > > > > > Yeah, I think these tx/rx buffers (the default case with tswap32) > > > > should be modelled as plain RAM's (they are just RAM's on real HW). > > > > Because we're modeling as MMIO regs, I think we get into endianness > > > > trouble when the ethernet output logic treats the content as a blob > > > > (thus the need for byteswaps). Does that make sense? > > > > > > As a concrete question: if I do a 32-bit load from the buffer > > > register into a CPU register, do I get a different value > > > on the BE microblaze hardware vs LE microblaze ? > > > > Yes, I beleive so. > > > > If the CPU stores the value and reads it back, you get the same. But > > the representation on the RAM's is different between LE/BE. > > But if the Ethernet logic writes Ethernet packet data into the buffer, > > LE and BE MicroBlazes will read differient values from the buffers. > > These buffer "registers" are just RAM's I beleive. > > Thanks. That suggests that the current code for this device > is correct, and we would be breaking it on the LE platform > if we applied this patch. > > I don't suppose you have a guest image for the boards which > uses ethernet ? Yes, I do, both little and big endian with ethlite working. Do you have a suggestion where to upload? Best regards, Edgar
On 13/12/22 14:53, Peter Maydell wrote: > On Tue, 13 Dec 2022 at 12:52, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >> >> This partly revert commit d48751ed4f ("xilinx-ethlite: >> Simplify byteswapping to/from brams") which states the >> packet data is stored in big-endian. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > >> @@ -102,8 +102,8 @@ eth_read(void *opaque, hwaddr addr, unsigned int size) >> D(qemu_log("%s " TARGET_FMT_plx "=%x\n", __func__, addr * 4, r)); >> break; >> >> - default: >> - r = tswap32(s->regs[addr]); >> + default: /* Packet data */ >> + r = be32_to_cpu(s->regs[addr]); >> break; >> } >> return r; >> @@ -160,8 +160,8 @@ eth_write(void *opaque, hwaddr addr, >> s->regs[addr] = value; >> break; >> >> - default: >> - s->regs[addr] = tswap32(value); >> + default: /* Packet data */ >> + s->regs[addr] = cpu_to_be32(value); >> break; >> } >> } > > This is a change of behaviour for this device in the > qemu-system-microblazeel petalogix-s3adsp1800 board, because > previously on that system the bytes of the rx buffer would > appear in the registers in little-endian order and now they > will appear in big-endian order. Maybe to simplify we could choose to only model the Big Endian variant of this device? -- >8 -- @@ -169,7 +169,7 @@ eth_write(void *opaque, hwaddr addr, static const MemoryRegionOps eth_ops = { .read = eth_read, .write = eth_write, - .endianness = DEVICE_NATIVE_ENDIAN, + .endianness = DEVICE_BIG_ENDIAN, .valid = { .min_access_size = 4, .max_access_size = 4 ---
diff --git a/hw/net/xilinx_ethlite.c b/hw/net/xilinx_ethlite.c index 6e09f7e422..efe627d734 100644 --- a/hw/net/xilinx_ethlite.c +++ b/hw/net/xilinx_ethlite.c @@ -24,8 +24,8 @@ #include "qemu/osdep.h" #include "qemu/module.h" +#include "qemu/bswap.h" #include "qom/object.h" -#include "cpu.h" /* FIXME should not use tswap* */ #include "hw/sysbus.h" #include "hw/irq.h" #include "hw/qdev-properties.h" @@ -102,8 +102,8 @@ eth_read(void *opaque, hwaddr addr, unsigned int size) D(qemu_log("%s " TARGET_FMT_plx "=%x\n", __func__, addr * 4, r)); break; - default: - r = tswap32(s->regs[addr]); + default: /* Packet data */ + r = be32_to_cpu(s->regs[addr]); break; } return r; @@ -160,8 +160,8 @@ eth_write(void *opaque, hwaddr addr, s->regs[addr] = value; break; - default: - s->regs[addr] = tswap32(value); + default: /* Packet data */ + s->regs[addr] = cpu_to_be32(value); break; } }
This partly revert commit d48751ed4f ("xilinx-ethlite: Simplify byteswapping to/from brams") which states the packet data is stored in big-endian. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/net/xilinx_ethlite.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)