diff mbox series

[RFC,PATCH-for-8.0,3/3] hw/net/xilinx_ethlite: Replace tswap32() by be32_to_cpu()

Message ID 20221213125218.39868-4-philmd@linaro.org (mailing list archive)
State New, archived
Headers show
Series hw/ppc: Remove tswap() calls | expand

Commit Message

Philippe Mathieu-Daudé Dec. 13, 2022, 12:52 p.m. UTC
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(-)

Comments

Peter Maydell Dec. 13, 2022, 1:53 p.m. UTC | #1
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
Edgar E. Iglesias Dec. 13, 2022, 1:54 p.m. UTC | #2
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
Peter Maydell Dec. 13, 2022, 2:18 p.m. UTC | #3
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
Edgar E. Iglesias Dec. 13, 2022, 2:23 p.m. UTC | #4
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
Peter Maydell Dec. 13, 2022, 3:22 p.m. UTC | #5
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
Edgar E. Iglesias Dec. 13, 2022, 3:41 p.m. UTC | #6
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
Philippe Mathieu-Daudé April 22, 2024, 12:46 p.m. UTC | #7
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 mbox series

Patch

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;
     }
 }