[RFC PATCH-for-8.0 0/3] hw/ppc: Remove tswap() calls

[RFC PATCH-for-8.0 0/3] hw/ppc: Remove tswap() calls

[Philippe Mathieu-Daudé]

Hi,

I am trying to remove the tswap() API from system
emulation and replace it by more meaningful calls,
because tswap depends on the host endianness, and
this detail should be irrelevant from the system
emulation PoV.

In this RFC series I'm trying to convert the PPC
calls.

Any help in understanding what was the original
author intention is welcomed :)

Thanks,

Phil.

Philippe Mathieu-Daudé (3):
  hw/ppc: Replace tswap32() by const_le32()
  hw/ppc/spapr: Replace tswap64(HPTE) by cpu_to_be64(HPTE)
  hw/net/xilinx_ethlite: Replace tswap32() by be32_to_cpu()

 hw/net/xilinx_ethlite.c | 10 +++++-----
 hw/ppc/sam460ex.c       |  3 ++-
 hw/ppc/spapr.c          |  9 +++++----
 hw/ppc/virtex_ml507.c   |  3 ++-
 4 files changed, 14 insertions(+), 11 deletions(-)

-- 
2.38.1

[RFC PATCH-for-8.0 1/3] hw/ppc: Replace tswap32() by const_le32()

[Philippe Mathieu-Daudé]

Assuming the developers of commits 2c50e26efd ("powerpc: Add
a virtex5 ml507 refdesign board") and 4b387f9ee1 ("ppc: Add
aCube Sam460ex board") were testing on a little-endian setup,
they meant to use const_le32() instead of tswap32() here,
since tswap32() depends on the host endianness.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ppc/sam460ex.c     | 3 ++-
 hw/ppc/virtex_ml507.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 4a22ce3761..88b1480138 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -15,6 +15,7 @@
 #include "qemu/units.h"
 #include "qemu/datadir.h"
 #include "qemu/error-report.h"
+#include "qemu/bswap.h"
 #include "qapi/error.h"
 #include "hw/boards.h"
 #include "sysemu/kvm.h"
@@ -255,7 +256,7 @@ static void main_cpu_reset(void *opaque)
 
         /* Create a mapping for the kernel.  */
         mmubooke_create_initial_mapping(env, 0, 0);
-        env->gpr[6] = tswap32(EPAPR_MAGIC);
+        env->gpr[6] = const_le32(EPAPR_MAGIC);
         env->gpr[7] = (16 * MiB) - 8; /* bi->ima_size; */
 
     } else {
diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
index 13cace229b..0f282ecaa7 100644
--- a/hw/ppc/virtex_ml507.c
+++ b/hw/ppc/virtex_ml507.c
@@ -38,6 +38,7 @@
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "qemu/option.h"
+#include "qemu/bswap.h"
 
 #include "hw/intc/ppc-uic.h"
 #include "hw/ppc/ppc.h"
@@ -141,7 +142,7 @@ static void main_cpu_reset(void *opaque)
 
     /* Create a mapping for the kernel.  */
     mmubooke_create_initial_mapping(env, 0, 0);
-    env->gpr[6] = tswap32(EPAPR_MAGIC);
+    env->gpr[6] = const_le32(EPAPR_MAGIC);
     env->gpr[7] = bi->ima_size;
 }
 
-- 
2.38.1

[RFC PATCH-for-8.0 2/3] hw/ppc/spapr: Replace tswap64(HPTE) by cpu_to_be64(HPTE)

[Philippe Mathieu-Daudé]

The tswap64() calls introduced in commit 4be21d561d ("pseries:
savevm support for pseries machine") are used to store the HTAB
in the migration stream (see savevm_htab_handlers) and are in
big-endian format.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ppc/spapr.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 66b414d2e9..8b1d5689d2 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -39,6 +39,7 @@
 #include "sysemu/reset.h"
 #include "sysemu/runstate.h"
 #include "qemu/log.h"
+#include "qemu/bswap.h"
 #include "hw/fw-path-provider.h"
 #include "elf.h"
 #include "net/net.h"
@@ -1357,10 +1358,10 @@ static bool spapr_get_pate(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu,
 }
 
 #define HPTE(_table, _i)   (void *)(((uint64_t *)(_table)) + ((_i) * 2))
-#define HPTE_VALID(_hpte)  (tswap64(*((uint64_t *)(_hpte))) & HPTE64_V_VALID)
-#define HPTE_DIRTY(_hpte)  (tswap64(*((uint64_t *)(_hpte))) & HPTE64_V_HPTE_DIRTY)
-#define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= tswap64(~HPTE64_V_HPTE_DIRTY))
-#define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= tswap64(HPTE64_V_HPTE_DIRTY))
+#define HPTE_VALID(_hpte)  (be64_to_cpu(*((uint64_t *)(_hpte))) & HPTE64_V_VALID)
+#define HPTE_DIRTY(_hpte)  (be64_to_cpu(*((uint64_t *)(_hpte))) & HPTE64_V_HPTE_DIRTY)
+#define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= cpu_to_be64(~HPTE64_V_HPTE_DIRTY))
+#define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= cpu_to_be64(HPTE64_V_HPTE_DIRTY))
 
 /*
  * Get the fd to access the kernel htab, re-opening it if necessary
-- 
2.38.1

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

[Philippe Mathieu-Daudé]

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(-)

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

Re: [RFC PATCH-for-8.0 2/3] hw/ppc/spapr: Replace tswap64(HPTE) by cpu_to_be64(HPTE)

[Peter Maydell]

On Tue, 13 Dec 2022 at 12:52, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> The tswap64() calls introduced in commit 4be21d561d ("pseries:
> savevm support for pseries machine") are used to store the HTAB
> in the migration stream (see savevm_htab_handlers) and are in
> big-endian format.

I think they're reading the run-time spapr->htab data structure
(some of which is stuck onto the wire as a stream-of-bytes buffer
and some of which is not). But either way, it's a target-endian
data structure, because the code in hw/ppc/spapr_softmmu.c which
reads and writes entries in it is using ldq_p() and stq_p(),
and the current in-tree version of these macros is doing a
"read host 64-bit and convert to/from target endianness wih tswap64".

>  #define HPTE(_table, _i)   (void *)(((uint64_t *)(_table)) + ((_i) * 2))
> -#define HPTE_VALID(_hpte)  (tswap64(*((uint64_t *)(_hpte))) & HPTE64_V_VALID)
> -#define HPTE_DIRTY(_hpte)  (tswap64(*((uint64_t *)(_hpte))) & HPTE64_V_HPTE_DIRTY)
> -#define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= tswap64(~HPTE64_V_HPTE_DIRTY))
> -#define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= tswap64(HPTE64_V_HPTE_DIRTY))
> +#define HPTE_VALID(_hpte)  (be64_to_cpu(*((uint64_t *)(_hpte))) & HPTE64_V_VALID)
> +#define HPTE_DIRTY(_hpte)  (be64_to_cpu(*((uint64_t *)(_hpte))) & HPTE64_V_HPTE_DIRTY)
> +#define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= cpu_to_be64(~HPTE64_V_HPTE_DIRTY))
> +#define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= cpu_to_be64(HPTE64_V_HPTE_DIRTY))

This means we now have one file that's accessing this data structure
as "this is target-endian", and one file that's accessing it as
"this is big-endian". It happens that that ends up meaning the same
thing because PPC is always TARGET_BIG_ENDIAN, but it seems a bit
inconsistent.

We should decide whether we're thinking of the data structure
as target-endian or big-endian and change all the accessors
appropriately (or none of them -- currently we're completely
consistent about treating it as "target endian", I think).

I also think that "cast a pointer into a byte-array to uint64_t*
and then dereference it" is less preferable than using ldq_p()
and stq_p(), but that's arguably a separate thing.

thanks
-- PMM

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

[Peter Maydell]

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

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

[Edgar E. Iglesias]

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

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

[Peter Maydell]

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

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

[Edgar E. Iglesias]

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

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

[Peter Maydell]

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

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

[Edgar E. Iglesias]

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

Re: [RFC PATCH-for-8.0 1/3] hw/ppc: Replace tswap32() by const_le32()

[Richard Henderson]

On 12/13/22 06:52, Philippe Mathieu-Daudé wrote:
> Assuming the developers of commits 2c50e26efd ("powerpc: Add
> a virtex5 ml507 refdesign board") and 4b387f9ee1 ("ppc: Add
> aCube Sam460ex board") were testing on a little-endian setup,
> they meant to use const_le32() instead of tswap32() here,
> since tswap32() depends on the host endianness.

tswap depends on target endianness.


> @@ -255,7 +256,7 @@ static void main_cpu_reset(void *opaque)
>   
>           /* Create a mapping for the kernel.  */
>           mmubooke_create_initial_mapping(env, 0, 0);
> -        env->gpr[6] = tswap32(EPAPR_MAGIC);
> +        env->gpr[6] = const_le32(EPAPR_MAGIC);

I think this is probably wrong.


r~

Re: [RFC PATCH-for-8.0 1/3] hw/ppc: Replace tswap32() by const_le32()

[Philippe Mathieu-Daudé]

On 13/12/22 17:00, Richard Henderson wrote:
> On 12/13/22 06:52, Philippe Mathieu-Daudé wrote:
>> Assuming the developers of commits 2c50e26efd ("powerpc: Add
>> a virtex5 ml507 refdesign board") and 4b387f9ee1 ("ppc: Add
>> aCube Sam460ex board") were testing on a little-endian setup,
>> they meant to use const_le32() instead of tswap32() here,
>> since tswap32() depends on the host endianness.
> 
> tswap depends on target endianness.

Yes, it depends on both host/target endianness. What I meant
is we shouldn't have system code depending on host endianness.

(I'm trying to reduce it to semihosting / user-emulation).

>> @@ -255,7 +256,7 @@ static void main_cpu_reset(void *opaque)
>>           /* Create a mapping for the kernel.  */
>>           mmubooke_create_initial_mapping(env, 0, 0);
>> -        env->gpr[6] = tswap32(EPAPR_MAGIC);
>> +        env->gpr[6] = const_le32(EPAPR_MAGIC);
> 
> I think this is probably wrong.

Since this is used for the kernel, I'll try to get its endianness
from the load_kernel() calls.

Re: [RFC PATCH-for-8.0 1/3] hw/ppc: Replace tswap32() by const_le32()

[Richard Henderson]

On 12/13/22 10:10, Philippe Mathieu-Daudé wrote:
> On 13/12/22 17:00, Richard Henderson wrote:
>> On 12/13/22 06:52, Philippe Mathieu-Daudé wrote:
>>> Assuming the developers of commits 2c50e26efd ("powerpc: Add
>>> a virtex5 ml507 refdesign board") and 4b387f9ee1 ("ppc: Add
>>> aCube Sam460ex board") were testing on a little-endian setup,
>>> they meant to use const_le32() instead of tswap32() here,
>>> since tswap32() depends on the host endianness.
>>
>> tswap depends on target endianness.
> 
> Yes, it depends on both host/target endianness. What I meant
> is we shouldn't have system code depending on host endianness.

It compares host vs target endianness, to determine if a swap is needed.  But the real 
dependency is target endianness.


r~

Re: [RFC PATCH-for-8.0 1/3] hw/ppc: Replace tswap32() by const_le32()

[Peter Maydell]

On Tue, 13 Dec 2022 at 16:14, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 12/13/22 10:10, Philippe Mathieu-Daudé wrote:
> > On 13/12/22 17:00, Richard Henderson wrote:
> >> On 12/13/22 06:52, Philippe Mathieu-Daudé wrote:
> >>> Assuming the developers of commits 2c50e26efd ("powerpc: Add
> >>> a virtex5 ml507 refdesign board") and 4b387f9ee1 ("ppc: Add
> >>> aCube Sam460ex board") were testing on a little-endian setup,
> >>> they meant to use const_le32() instead of tswap32() here,
> >>> since tswap32() depends on the host endianness.
> >>
> >> tswap depends on target endianness.
> >
> > Yes, it depends on both host/target endianness. What I meant
> > is we shouldn't have system code depending on host endianness.
>
> It compares host vs target endianness, to determine if a swap is needed.  But the real
> dependency is target endianness.

It does seem odd, though. We have a value in host endianness
(the EPAPR_MAGIC constant, which is host-endian by virtue of
being a C constant). But we're storing it to env->gpr[], which
is to say the CPUPPCState general purpose register array. Isn't
that array *also* kept in host endianness order?

If so, then the right thing here is "don't swap at all",
i.e. just "env->gpr[6] = EPAPR_MAGIC;". But that would imply
that the current code is setting the wrong value for the GPR
on little-endian hosts, which seems a bit unlikely...

thanks
-- PMM

Re: [RFC PATCH-for-8.0 1/3] hw/ppc: Replace tswap32() by const_le32()

[Richard Henderson]

On 12/13/22 10:21, Peter Maydell wrote:
> On Tue, 13 Dec 2022 at 16:14, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 12/13/22 10:10, Philippe Mathieu-Daudé wrote:
>>> On 13/12/22 17:00, Richard Henderson wrote:
>>>> On 12/13/22 06:52, Philippe Mathieu-Daudé wrote:
>>>>> Assuming the developers of commits 2c50e26efd ("powerpc: Add
>>>>> a virtex5 ml507 refdesign board") and 4b387f9ee1 ("ppc: Add
>>>>> aCube Sam460ex board") were testing on a little-endian setup,
>>>>> they meant to use const_le32() instead of tswap32() here,
>>>>> since tswap32() depends on the host endianness.
>>>>
>>>> tswap depends on target endianness.
>>>
>>> Yes, it depends on both host/target endianness. What I meant
>>> is we shouldn't have system code depending on host endianness.
>>
>> It compares host vs target endianness, to determine if a swap is needed.  But the real
>> dependency is target endianness.
> 
> It does seem odd, though. We have a value in host endianness
> (the EPAPR_MAGIC constant, which is host-endian by virtue of
> being a C constant). But we're storing it to env->gpr[], which
> is to say the CPUPPCState general purpose register array. Isn't
> that array *also* kept in host endianness order?

Yes indeed.

> If so, then the right thing here is "don't swap at all",

So it would seem...

> i.e. just "env->gpr[6] = EPAPR_MAGIC;". But that would imply
> that the current code is setting the wrong value for the GPR
> on little-endian hosts, which seems a bit unlikely...

... unless this board has only been tested on matching hosts.


r~

Re: [RFC PATCH-for-8.0 1/3] hw/ppc: Replace tswap32() by const_le32()

[Cédric Le Goater]

On 12/13/22 17:27, Richard Henderson wrote:
> On 12/13/22 10:21, Peter Maydell wrote:
>> On Tue, 13 Dec 2022 at 16:14, Richard Henderson
>> <richard.henderson@linaro.org> wrote:
>>>
>>> On 12/13/22 10:10, Philippe Mathieu-Daudé wrote:
>>>> On 13/12/22 17:00, Richard Henderson wrote:
>>>>> On 12/13/22 06:52, Philippe Mathieu-Daudé wrote:
>>>>>> Assuming the developers of commits 2c50e26efd ("powerpc: Add
>>>>>> a virtex5 ml507 refdesign board") and 4b387f9ee1 ("ppc: Add
>>>>>> aCube Sam460ex board") were testing on a little-endian setup,
>>>>>> they meant to use const_le32() instead of tswap32() here,
>>>>>> since tswap32() depends on the host endianness.
>>>>>
>>>>> tswap depends on target endianness.
>>>>
>>>> Yes, it depends on both host/target endianness. What I meant
>>>> is we shouldn't have system code depending on host endianness.
>>>
>>> It compares host vs target endianness, to determine if a swap is needed.  But the real
>>> dependency is target endianness.
>>
>> It does seem odd, though. We have a value in host endianness
>> (the EPAPR_MAGIC constant, which is host-endian by virtue of
>> being a C constant). But we're storing it to env->gpr[], which
>> is to say the CPUPPCState general purpose register array. Isn't
>> that array *also* kept in host endianness order?
> 
> Yes indeed.
> 
>> If so, then the right thing here is "don't swap at all",
> 
> So it would seem...
> 
>> i.e. just "env->gpr[6] = EPAPR_MAGIC;". But that would imply
>> that the current code is setting the wrong value for the GPR
>> on little-endian hosts, which seems a bit unlikely...
> 
> ... unless this board has only been tested on matching hosts.

But these are register default values. Endianness doesn't apply
there. Doesn't it ?

C.

Re: [RFC PATCH-for-8.0 0/3] hw/ppc: Remove tswap() calls

[Cédric Le Goater]

On 12/13/22 13:52, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> I am trying to remove the tswap() API from system
> emulation and replace it by more meaningful calls,
> because tswap depends on the host endianness, and
> this detail should be irrelevant from the system
> emulation PoV.
> 
> In this RFC series I'm trying to convert the PPC
> calls.

Here are some simple images for tests:

   https://github.com/legoater/qemu-ppc-boot/tree/main/buildroot

Cheers,

C.


> 
> Any help in understanding what was the original
> author intention is welcomed :)
> 
> Thanks,
> 
> Phil.
> 
> Philippe Mathieu-Daudé (3):
>    hw/ppc: Replace tswap32() by const_le32()
>    hw/ppc/spapr: Replace tswap64(HPTE) by cpu_to_be64(HPTE)
>    hw/net/xilinx_ethlite: Replace tswap32() by be32_to_cpu()
> 
>   hw/net/xilinx_ethlite.c | 10 +++++-----
>   hw/ppc/sam460ex.c       |  3 ++-
>   hw/ppc/spapr.c          |  9 +++++----
>   hw/ppc/virtex_ml507.c   |  3 ++-
>   4 files changed, 14 insertions(+), 11 deletions(-)
> 

Re: [RFC PATCH-for-8.0 1/3] hw/ppc: Replace tswap32() by const_le32()

[Peter Maydell]

On Tue, 13 Dec 2022 at 16:53, Cédric Le Goater <clg@kaod.org> wrote:
>
> On 12/13/22 17:27, Richard Henderson wrote:
> > On 12/13/22 10:21, Peter Maydell wrote:
> >> It does seem odd, though. We have a value in host endianness
> >> (the EPAPR_MAGIC constant, which is host-endian by virtue of
> >> being a C constant). But we're storing it to env->gpr[], which
> >> is to say the CPUPPCState general purpose register array. Isn't
> >> that array *also* kept in host endianness order?
> >
> > Yes indeed.
> >
> >> If so, then the right thing here is "don't swap at all",
> >
> > So it would seem...
> >
> >> i.e. just "env->gpr[6] = EPAPR_MAGIC;". But that would imply
> >> that the current code is setting the wrong value for the GPR
> >> on little-endian hosts, which seems a bit unlikely...
> >
> > ... unless this board has only been tested on matching hosts.
>
> But these are register default values. Endianness doesn't apply
> there. Doesn't it ?

Any time you have a value that's more than 1 byte wide,
endianness applies in some sense :-) We choose for our
emulated CPUs typically to keep register values in struct
fields and variables in the C code in host endianness. This
is the "obvious" choice given that you want to be able to
do things like do a simple host add to emulate a guest CPU
add, but in theory you could store the values the other
way around if you wanted (then "store register into RAM"
would be trivial, and "add 1 to register" would require
extra effort; currently it's the other way round.)

Anyway, I think that in the virtex_ml507 and sam460ex code
the use of tswap32() should be removed. In hw/ppc/e500.c
we get this right:
    env->gpr[6] = EPAPR_MAGIC;

We have a Linux kernel boot test in the avocado tests for
virtex_ml507 -- we really do set up this magic value wrongly
afaict, but it seems like the kernel doesn't check it (the
test passes regardless of whether we swap the value or not).

I think what has happened here is that this bit of code is
setting up CPU registers for an EPAPR style boot, but the
test kernel at least doesn't expect that. It boots via the
code in arch/powerpc/kernel/head_44x.S. That file claims
in a comment that it expects
 *   r3 - Board info structure pointer (DRAM, frequency, MAC address, etc.)
 *   r4 - Starting address of the init RAM disk
 *   r5 - Ending address of the init RAM disk
 *   r6 - Start of kernel command line string (e.g. "mem=128")
 *   r7 - End of kernel command line string

but actually it only cares that r3 == device-tree-blob.

Documentation/powerpc/booting.rst says the expectation
(for a non-OpenFirmware boot) is:
                r3 : physical pointer to the device-tree block
                (defined in chapter II) in RAM

                r4 : physical pointer to the kernel itself. This is
                used by the assembly code to properly disable the MMU
                in case you are entering the kernel with MMU enabled
                and a non-1:1 mapping.

                r5 : NULL (as to differentiate with method a)

which isn't the same as what the kernel code actually cares about
or what the kernel's comment says it cares about...

So my guess about what's happening here is that the intention
was that these boards should be able to boot both kernels built
to be entered directly in the way booting.rst says, and also
kernels and other guest programs built to assume boot by
EPAPR firmware, but this bug means that we're only currently
supporting the first of these two categories. The reason nobody's
noticed before is presumably that in practice nobody's trying to
boot the "built to boot from EPAPR firmware" type binary on
these two boards.

TLDR: we should drop the "tswap32()" entirely from both files.

thanks
-- PMM

Re: [RFC PATCH-for-8.0 1/3] hw/ppc: Replace tswap32() by const_le32()

[Edgar E. Iglesias]

On Tue, Dec 13, 2022 at 05:23:06PM +0000, Peter Maydell wrote:
> On Tue, 13 Dec 2022 at 16:53, Cédric Le Goater <clg@kaod.org> wrote:
> >
> > On 12/13/22 17:27, Richard Henderson wrote:
> > > On 12/13/22 10:21, Peter Maydell wrote:
> > >> It does seem odd, though. We have a value in host endianness
> > >> (the EPAPR_MAGIC constant, which is host-endian by virtue of
> > >> being a C constant). But we're storing it to env->gpr[], which
> > >> is to say the CPUPPCState general purpose register array. Isn't
> > >> that array *also* kept in host endianness order?
> > >
> > > Yes indeed.
> > >
> > >> If so, then the right thing here is "don't swap at all",
> > >
> > > So it would seem...
> > >
> > >> i.e. just "env->gpr[6] = EPAPR_MAGIC;". But that would imply
> > >> that the current code is setting the wrong value for the GPR
> > >> on little-endian hosts, which seems a bit unlikely...
> > >
> > > ... unless this board has only been tested on matching hosts.
> >
> > But these are register default values. Endianness doesn't apply
> > there. Doesn't it ?
> 
> Any time you have a value that's more than 1 byte wide,
> endianness applies in some sense :-) We choose for our
> emulated CPUs typically to keep register values in struct
> fields and variables in the C code in host endianness. This
> is the "obvious" choice given that you want to be able to
> do things like do a simple host add to emulate a guest CPU
> add, but in theory you could store the values the other
> way around if you wanted (then "store register into RAM"
> would be trivial, and "add 1 to register" would require
> extra effort; currently it's the other way round.)
> 
> Anyway, I think that in the virtex_ml507 and sam460ex code
> the use of tswap32() should be removed. In hw/ppc/e500.c
> we get this right:
>     env->gpr[6] = EPAPR_MAGIC;
> 
> We have a Linux kernel boot test in the avocado tests for
> virtex_ml507 -- we really do set up this magic value wrongly
> afaict, but it seems like the kernel doesn't check it (the
> test passes regardless of whether we swap the value or not).
> 
> I think what has happened here is that this bit of code is
> setting up CPU registers for an EPAPR style boot, but the
> test kernel at least doesn't expect that. It boots via the
> code in arch/powerpc/kernel/head_44x.S. That file claims
> in a comment that it expects
>  *   r3 - Board info structure pointer (DRAM, frequency, MAC address, etc.)
>  *   r4 - Starting address of the init RAM disk
>  *   r5 - Ending address of the init RAM disk
>  *   r6 - Start of kernel command line string (e.g. "mem=128")
>  *   r7 - End of kernel command line string
> 
> but actually it only cares that r3 == device-tree-blob.
> 
> Documentation/powerpc/booting.rst says the expectation
> (for a non-OpenFirmware boot) is:
>                 r3 : physical pointer to the device-tree block
>                 (defined in chapter II) in RAM
> 
>                 r4 : physical pointer to the kernel itself. This is
>                 used by the assembly code to properly disable the MMU
>                 in case you are entering the kernel with MMU enabled
>                 and a non-1:1 mapping.
> 
>                 r5 : NULL (as to differentiate with method a)
> 
> which isn't the same as what the kernel code actually cares about
> or what the kernel's comment says it cares about...
> 
> So my guess about what's happening here is that the intention
> was that these boards should be able to boot both kernels built
> to be entered directly in the way booting.rst says, and also
> kernels and other guest programs built to assume boot by
> EPAPR firmware, but this bug means that we're only currently
> supporting the first of these two categories. The reason nobody's
> noticed before is presumably that in practice nobody's trying to
> boot the "built to boot from EPAPR firmware" type binary on
> these two boards.
> 
> TLDR: we should drop the "tswap32()" entirely from both files.
>

Sounds reasonable to me!

Best regards,
Edgar
 

Re: [RFC PATCH-for-8.0 1/3] hw/ppc: Replace tswap32() by const_le32()

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 4712 bytes --]

On Tue, 13 Dec 2022, Peter Maydell wrote:
> On Tue, 13 Dec 2022 at 16:53, Cédric Le Goater <clg@kaod.org> wrote:
>> On 12/13/22 17:27, Richard Henderson wrote:
>>> On 12/13/22 10:21, Peter Maydell wrote:
>>>> It does seem odd, though. We have a value in host endianness
>>>> (the EPAPR_MAGIC constant, which is host-endian by virtue of
>>>> being a C constant). But we're storing it to env->gpr[], which
>>>> is to say the CPUPPCState general purpose register array. Isn't
>>>> that array *also* kept in host endianness order?
>>>
>>> Yes indeed.
>>>
>>>> If so, then the right thing here is "don't swap at all",
>>>
>>> So it would seem...
>>>
>>>> i.e. just "env->gpr[6] = EPAPR_MAGIC;". But that would imply
>>>> that the current code is setting the wrong value for the GPR
>>>> on little-endian hosts, which seems a bit unlikely...
>>>
>>> ... unless this board has only been tested on matching hosts.

I can't remember the details but I think I've had no tswap in sam460ex 
first but that did not work and had to add it but I've probably looked at 
other examples and did not really understand why this was needed. I tested 
on x86_64 so not matching host. The board firmware has some reference to 
this magic value in:

qemu/roms/u-boot-sam460ex/arch/powerpc/lib/bootm.c::boot_jump_linux()

I don't know what it does with it but I think kernel expects it in big 
endian and what we put there should match what U-Boor does (if this is 
actually used on sam460ex which I'm not sure about). The Linux kernel I've 
tested with back then was probably from Ubuntu_16.04 or Debian Jessie 
which supported this board. Not sure this helps but that's all I can 
gather now but I may remember wrong.

Regards,
BALATON Zoltan

>> But these are register default values. Endianness doesn't apply
>> there. Doesn't it ?
>
> Any time you have a value that's more than 1 byte wide,
> endianness applies in some sense :-) We choose for our
> emulated CPUs typically to keep register values in struct
> fields and variables in the C code in host endianness. This
> is the "obvious" choice given that you want to be able to
> do things like do a simple host add to emulate a guest CPU
> add, but in theory you could store the values the other
> way around if you wanted (then "store register into RAM"
> would be trivial, and "add 1 to register" would require
> extra effort; currently it's the other way round.)
>
> Anyway, I think that in the virtex_ml507 and sam460ex code
> the use of tswap32() should be removed. In hw/ppc/e500.c
> we get this right:
>    env->gpr[6] = EPAPR_MAGIC;
>
> We have a Linux kernel boot test in the avocado tests for
> virtex_ml507 -- we really do set up this magic value wrongly
> afaict, but it seems like the kernel doesn't check it (the
> test passes regardless of whether we swap the value or not).
>
> I think what has happened here is that this bit of code is
> setting up CPU registers for an EPAPR style boot, but the
> test kernel at least doesn't expect that. It boots via the
> code in arch/powerpc/kernel/head_44x.S. That file claims
> in a comment that it expects
> *   r3 - Board info structure pointer (DRAM, frequency, MAC address, etc.)
> *   r4 - Starting address of the init RAM disk
> *   r5 - Ending address of the init RAM disk
> *   r6 - Start of kernel command line string (e.g. "mem=128")
> *   r7 - End of kernel command line string
>
> but actually it only cares that r3 == device-tree-blob.
>
> Documentation/powerpc/booting.rst says the expectation
> (for a non-OpenFirmware boot) is:
>                r3 : physical pointer to the device-tree block
>                (defined in chapter II) in RAM
>
>                r4 : physical pointer to the kernel itself. This is
>                used by the assembly code to properly disable the MMU
>                in case you are entering the kernel with MMU enabled
>                and a non-1:1 mapping.
>
>                r5 : NULL (as to differentiate with method a)
>
> which isn't the same as what the kernel code actually cares about
> or what the kernel's comment says it cares about...
>
> So my guess about what's happening here is that the intention
> was that these boards should be able to boot both kernels built
> to be entered directly in the way booting.rst says, and also
> kernels and other guest programs built to assume boot by
> EPAPR firmware, but this bug means that we're only currently
> supporting the first of these two categories. The reason nobody's
> noticed before is presumably that in practice nobody's trying to
> boot the "built to boot from EPAPR firmware" type binary on
> these two boards.
>
> TLDR: we should drop the "tswap32()" entirely from both files.
>
> thanks
> -- PMM
>
>

Re: [RFC PATCH-for-8.0 1/3] hw/ppc: Replace tswap32() by const_le32()

[Cédric Le Goater]

On 12/13/22 18:23, Peter Maydell wrote:
> On Tue, 13 Dec 2022 at 16:53, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> On 12/13/22 17:27, Richard Henderson wrote:
>>> On 12/13/22 10:21, Peter Maydell wrote:
>>>> It does seem odd, though. We have a value in host endianness
>>>> (the EPAPR_MAGIC constant, which is host-endian by virtue of
>>>> being a C constant). But we're storing it to env->gpr[], which
>>>> is to say the CPUPPCState general purpose register array. Isn't
>>>> that array *also* kept in host endianness order?
>>>
>>> Yes indeed.
>>>
>>>> If so, then the right thing here is "don't swap at all",
>>>
>>> So it would seem...
>>>
>>>> i.e. just "env->gpr[6] = EPAPR_MAGIC;". But that would imply
>>>> that the current code is setting the wrong value for the GPR
>>>> on little-endian hosts, which seems a bit unlikely...
>>>
>>> ... unless this board has only been tested on matching hosts.
>>
>> But these are register default values. Endianness doesn't apply
>> there. Doesn't it ?
> 
> Any time you have a value that's more than 1 byte wide,
> endianness applies in some sense :-) We choose for our
> emulated CPUs typically to keep register values in struct
> fields and variables in the C code in host endianness. This
> is the "obvious" choice given that you want to be able to
> do things like do a simple host add to emulate a guest CPU
> add, but in theory you could store the values the other
> way around if you wanted (then "store register into RAM"
> would be trivial, and "add 1 to register" would require
> extra effort; currently it's the other way round.)
> 
> Anyway, I think that in the virtex_ml507 and sam460ex code
> the use of tswap32() should be removed. In hw/ppc/e500.c
> we get this right:
>      env->gpr[6] = EPAPR_MAGIC;
> 
> We have a Linux kernel boot test in the avocado tests for
> virtex_ml507 -- we really do set up this magic value wrongly
> afaict, but it seems like the kernel doesn't check it (the
> test passes regardless of whether we swap the value or not).
> 
> I think what has happened here is that this bit of code is
> setting up CPU registers for an EPAPR style boot, but the
> test kernel at least doesn't expect that. It boots via the
> code in arch/powerpc/kernel/head_44x.S. That file claims
> in a comment that it expects
>   *   r3 - Board info structure pointer (DRAM, frequency, MAC address, etc.)
>   *   r4 - Starting address of the init RAM disk
>   *   r5 - Ending address of the init RAM disk
>   *   r6 - Start of kernel command line string (e.g. "mem=128")
>   *   r7 - End of kernel command line string
> 
> but actually it only cares that r3 == device-tree-blob.
> 
> Documentation/powerpc/booting.rst says the expectation
> (for a non-OpenFirmware boot) is:
>                  r3 : physical pointer to the device-tree block
>                  (defined in chapter II) in RAM
> 
>                  r4 : physical pointer to the kernel itself. This is
>                  used by the assembly code to properly disable the MMU
>                  in case you are entering the kernel with MMU enabled
>                  and a non-1:1 mapping.
> 
>                  r5 : NULL (as to differentiate with method a)
> 
> which isn't the same as what the kernel code actually cares about
> or what the kernel's comment says it cares about...
> 
> So my guess about what's happening here is that the intention
> was that these boards should be able to boot both kernels built
> to be entered directly in the way booting.rst says, and also
> kernels and other guest programs built to assume boot by
> EPAPR firmware, but this bug means that we're only currently
> supporting the first of these two categories. The reason nobody's
> noticed before is presumably that in practice nobody's trying to
> boot the "built to boot from EPAPR firmware" type binary on
> these two boards.
> 
> TLDR: we should drop the "tswap32()" entirely from both files.
Yes. I think so too.

Here are the specs :

    https://web.archive.org/web/20120419173345/https://www.power.org/resources/downloads/Power_ePAPR_APPROVED_v1.1.pdf

See 5.4.1 Boot CPU Initial Register State

Register	Value

MSR		PR=0 supervisor state
		EE=0 interrupts disabled
		ME=0 machine check interrupt disabled
		IP=0 interrupt prefix-- low memory
		IR=0,DR=0 real mode (see note 1)
		IS=0,DS=0 address space 0 (see note 1)
		SF=0, CM=0, ICM=0 32-bit mode
		The state of any additional MSR bits is defined in the
		applicable processor supplement specification.
R3		Effective address of the device tree image.
		Note: This address shall be 8 bytes aligned in memory.
R4		0
R5		0
R6		ePAPR magic value—to distinguish from non-ePAPR-
		compliant firmware
		• For Book III-E CPUs shall be 0x45504150
		• For non-Book III-E CPUs shall be 0x65504150
R7		shall be the size of the boot IMA in bytes
R8		0
R9		0
TCR		WRC=0, no watchdog timer reset will occur (see note 2)
other registers implementation dependent


Thanks,

C.

Re: [RFC PATCH-for-8.0 1/3] hw/ppc: Replace tswap32() by const_le32()

[Peter Maydell]

On Tue, 13 Dec 2022 at 18:11, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> I can't remember the details but I think I've had no tswap in sam460ex
> first but that did not work and had to add it but I've probably looked at
> other examples and did not really understand why this was needed. I tested
> on x86_64 so not matching host. The board firmware has some reference to
> this magic value in:
>
> qemu/roms/u-boot-sam460ex/arch/powerpc/lib/bootm.c::boot_jump_linux()
>
> I don't know what it does with it but I think kernel expects it in big
> endian and what we put there should match what U-Boor does (if this is
> actually used on sam460ex which I'm not sure about).

Thanks. That u-boot code uses the same value for EPAPR_MAGIC
as we do (0x45504150), and it puts it in r6 (by doing a function
call), and being native code the register will get that exact
value, not a byte-swapped version.

So to match that we should delete the tswap32() that we
currently have in our hw/ppc/sam460ex.c code.

My guess is that (as with the virtex kernel in our test suite)
the Debian/Ubuntu kernel you tested with worked because it
doesn't actually check the value of the magic number, it only
cares that it gets the FDT address in r3. The giveaway that
the tswap32() is wrong is that we're only swapping one of the
4 things we pass to the guest code in registers -- either
we should need to swap all of them, or none (unless our
magic number value was pre-byteswapped, which it isn't).

-- PMM

Re: [RFC PATCH-for-8.0 2/3] hw/ppc/spapr: Replace tswap64(HPTE) by cpu_to_be64(HPTE)

[Daniel Henrique Barboza]

On 12/13/22 10:51, Peter Maydell wrote:
> On Tue, 13 Dec 2022 at 12:52, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> The tswap64() calls introduced in commit 4be21d561d ("pseries:
>> savevm support for pseries machine") are used to store the HTAB
>> in the migration stream (see savevm_htab_handlers) and are in
>> big-endian format.
> 
> I think they're reading the run-time spapr->htab data structure
> (some of which is stuck onto the wire as a stream-of-bytes buffer
> and some of which is not). But either way, it's a target-endian
> data structure, because the code in hw/ppc/spapr_softmmu.c which
> reads and writes entries in it is using ldq_p() and stq_p(),
> and the current in-tree version of these macros is doing a
> "read host 64-bit and convert to/from target endianness wih tswap64".
> 
>>   #define HPTE(_table, _i)   (void *)(((uint64_t *)(_table)) + ((_i) * 2))
>> -#define HPTE_VALID(_hpte)  (tswap64(*((uint64_t *)(_hpte))) & HPTE64_V_VALID)
>> -#define HPTE_DIRTY(_hpte)  (tswap64(*((uint64_t *)(_hpte))) & HPTE64_V_HPTE_DIRTY)
>> -#define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= tswap64(~HPTE64_V_HPTE_DIRTY))
>> -#define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= tswap64(HPTE64_V_HPTE_DIRTY))
>> +#define HPTE_VALID(_hpte)  (be64_to_cpu(*((uint64_t *)(_hpte))) & HPTE64_V_VALID)
>> +#define HPTE_DIRTY(_hpte)  (be64_to_cpu(*((uint64_t *)(_hpte))) & HPTE64_V_HPTE_DIRTY)
>> +#define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= cpu_to_be64(~HPTE64_V_HPTE_DIRTY))
>> +#define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= cpu_to_be64(HPTE64_V_HPTE_DIRTY))
> 
> This means we now have one file that's accessing this data structure
> as "this is target-endian", and one file that's accessing it as
> "this is big-endian". It happens that that ends up meaning the same
> thing because PPC is always TARGET_BIG_ENDIAN, but it seems a bit
> inconsistent.
> 
> We should decide whether we're thinking of the data structure
> as target-endian or big-endian and change all the accessors
> appropriately (or none of them -- currently we're completely
> consistent about treating it as "target endian", I think).

Yes, most if not all accesses are being handled as "target endian", even
though the target is always big endian.

IIUC the idea behind Phil's cleanups is exactly to replace uses of
"target-something" if the endianess of the host is irrelevant, which
is the case for ppc64. We would then change the semantics of the code
gradually to make it consistent again.

However, I don't feel comfortable acking this patch alone since 4be21d561d
is from David and I don't know if there's a great design behind the use of
tswap64() to manipulate the hpte. David, would you care to comment?



Daniel

  

> 
> I also think that "cast a pointer into a byte-array to uint64_t*
> and then dereference it" is less preferable than using ldq_p()
> and stq_p(), but that's arguably a separate thing.
> 
> thanks
> -- PMM

Re: [RFC PATCH-for-8.0 2/3] hw/ppc/spapr: Replace tswap64(HPTE) by cpu_to_be64(HPTE)

[Peter Maydell]

On Fri, 16 Dec 2022 at 19:11, Daniel Henrique Barboza
<danielhb413@gmail.com> wrote:
>
>
>
> On 12/13/22 10:51, Peter Maydell wrote:
> > On Tue, 13 Dec 2022 at 12:52, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >>
> >> The tswap64() calls introduced in commit 4be21d561d ("pseries:
> >> savevm support for pseries machine") are used to store the HTAB
> >> in the migration stream (see savevm_htab_handlers) and are in
> >> big-endian format.
> >
> > I think they're reading the run-time spapr->htab data structure
> > (some of which is stuck onto the wire as a stream-of-bytes buffer
> > and some of which is not). But either way, it's a target-endian
> > data structure, because the code in hw/ppc/spapr_softmmu.c which
> > reads and writes entries in it is using ldq_p() and stq_p(),
> > and the current in-tree version of these macros is doing a
> > "read host 64-bit and convert to/from target endianness wih tswap64".
> >
> >>   #define HPTE(_table, _i)   (void *)(((uint64_t *)(_table)) + ((_i) * 2))
> >> -#define HPTE_VALID(_hpte)  (tswap64(*((uint64_t *)(_hpte))) & HPTE64_V_VALID)
> >> -#define HPTE_DIRTY(_hpte)  (tswap64(*((uint64_t *)(_hpte))) & HPTE64_V_HPTE_DIRTY)
> >> -#define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= tswap64(~HPTE64_V_HPTE_DIRTY))
> >> -#define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= tswap64(HPTE64_V_HPTE_DIRTY))
> >> +#define HPTE_VALID(_hpte)  (be64_to_cpu(*((uint64_t *)(_hpte))) & HPTE64_V_VALID)
> >> +#define HPTE_DIRTY(_hpte)  (be64_to_cpu(*((uint64_t *)(_hpte))) & HPTE64_V_HPTE_DIRTY)
> >> +#define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= cpu_to_be64(~HPTE64_V_HPTE_DIRTY))
> >> +#define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= cpu_to_be64(HPTE64_V_HPTE_DIRTY))
> >
> > This means we now have one file that's accessing this data structure
> > as "this is target-endian", and one file that's accessing it as
> > "this is big-endian". It happens that that ends up meaning the same
> > thing because PPC is always TARGET_BIG_ENDIAN, but it seems a bit
> > inconsistent.
> >
> > We should decide whether we're thinking of the data structure
> > as target-endian or big-endian and change all the accessors
> > appropriately (or none of them -- currently we're completely
> > consistent about treating it as "target endian", I think).
>
> Yes, most if not all accesses are being handled as "target endian", even
> though the target is always big endian.
>
> IIUC the idea behind Phil's cleanups is exactly to replace uses of
> "target-something" if the endianess of the host is irrelevant, which
> is the case for ppc64. We would then change the semantics of the code
> gradually to make it consistent again.

I would be happier if we just did all the functions that read and
write this byte array at once -- there are not many of them.

thanks
-- PMM

Re: [RFC PATCH-for-8.0 2/3] hw/ppc/spapr: Replace tswap64(HPTE) by cpu_to_be64(HPTE)

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4409 bytes --]

On Fri, Dec 16, 2022 at 09:39:19PM +0000, Peter Maydell wrote:
> On Fri, 16 Dec 2022 at 19:11, Daniel Henrique Barboza
> <danielhb413@gmail.com> wrote:
> >
> >
> >
> > On 12/13/22 10:51, Peter Maydell wrote:
> > > On Tue, 13 Dec 2022 at 12:52, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> > >>
> > >> The tswap64() calls introduced in commit 4be21d561d ("pseries:
> > >> savevm support for pseries machine") are used to store the HTAB
> > >> in the migration stream (see savevm_htab_handlers) and are in
> > >> big-endian format.
> > >
> > > I think they're reading the run-time spapr->htab data structure
> > > (some of which is stuck onto the wire as a stream-of-bytes buffer
> > > and some of which is not). But either way, it's a target-endian
> > > data structure, because the code in hw/ppc/spapr_softmmu.c which
> > > reads and writes entries in it is using ldq_p() and stq_p(),
> > > and the current in-tree version of these macros is doing a
> > > "read host 64-bit and convert to/from target endianness wih tswap64".
> > >
> > >>   #define HPTE(_table, _i)   (void *)(((uint64_t *)(_table)) + ((_i) * 2))
> > >> -#define HPTE_VALID(_hpte)  (tswap64(*((uint64_t *)(_hpte))) & HPTE64_V_VALID)
> > >> -#define HPTE_DIRTY(_hpte)  (tswap64(*((uint64_t *)(_hpte))) & HPTE64_V_HPTE_DIRTY)
> > >> -#define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= tswap64(~HPTE64_V_HPTE_DIRTY))
> > >> -#define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= tswap64(HPTE64_V_HPTE_DIRTY))
> > >> +#define HPTE_VALID(_hpte)  (be64_to_cpu(*((uint64_t *)(_hpte))) & HPTE64_V_VALID)
> > >> +#define HPTE_DIRTY(_hpte)  (be64_to_cpu(*((uint64_t *)(_hpte))) & HPTE64_V_HPTE_DIRTY)
> > >> +#define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= cpu_to_be64(~HPTE64_V_HPTE_DIRTY))
> > >> +#define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= cpu_to_be64(HPTE64_V_HPTE_DIRTY))
> > >
> > > This means we now have one file that's accessing this data structure
> > > as "this is target-endian", and one file that's accessing it as
> > > "this is big-endian". It happens that that ends up meaning the same
> > > thing because PPC is always TARGET_BIG_ENDIAN, but it seems a bit
> > > inconsistent.
> > >
> > > We should decide whether we're thinking of the data structure
> > > as target-endian or big-endian and change all the accessors
> > > appropriately (or none of them -- currently we're completely
> > > consistent about treating it as "target endian", I think).
> >
> > Yes, most if not all accesses are being handled as "target endian", even
> > though the target is always big endian.

So "target is always big endian" is pretty misleading for POWER.  We
always define "TARGET_BIG_ENDIAN" in qemu, but for at least 10 years
the CPUs have been capable of running in either big endian or little
endian mode (selected at runtime).  Some variants can choose
endianness on a per-page basis.  Since the creation of the ISA it's
had "byte reversed" load and store instructions that let it use little
endian for specific memory accesses.

Really the whole notion of an ISA having an "endianness" doesn't make
a lot of sense - it's an individual load or store to memory that has
an endianness which can depend on a bunch of factors.  When these
macros were created, an ISA nearly always used the same endianness,
but that's not really true any more - not just for POWER, but for a
bunch of targets.  So from that point of view, I think getting rid of
tswap() - particularly one that has compile time semantics, rather
than behaviour which can depend on cpu mode/state is a good idea.

I believe that even when running in little-endian mode, the hash page
tables are encoded in big-endian, so I think the proposed change makes
sense.

> > IIUC the idea behind Phil's cleanups is exactly to replace uses of
> > "target-something" if the endianess of the host is irrelevant, which
> > is the case for ppc64. We would then change the semantics of the code
> > gradually to make it consistent again.
> 
> I would be happier if we just did all the functions that read and
> write this byte array at once -- there are not many of them.
> 
> thanks
> -- PMM
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH-for-8.0 2/3] hw/ppc/spapr: Replace tswap64(HPTE) by cpu_to_be64(HPTE)

[Peter Maydell]

On Mon, 19 Dec 2022 at 06:35, David Gibson <david@gibson.dropbear.id.au> wrote:
>
> On Fri, Dec 16, 2022 at 09:39:19PM +0000, Peter Maydell wrote:
> > On Fri, 16 Dec 2022 at 19:11, Daniel Henrique Barboza
> > <danielhb413@gmail.com> wrote:
> > >
> > >
> > >
> > > On 12/13/22 10:51, Peter Maydell wrote:
> > > Yes, most if not all accesses are being handled as "target endian", even
> > > though the target is always big endian.
>
> So "target is always big endian" is pretty misleading for POWER.  We
> always define "TARGET_BIG_ENDIAN" in qemu, but for at least 10 years
> the CPUs have been capable of running in either big endian or little
> endian mode (selected at runtime).  Some variants can choose
> endianness on a per-page basis.  Since the creation of the ISA it's
> had "byte reversed" load and store instructions that let it use little
> endian for specific memory accesses.

Yeah, this is like Arm (and for the purposes of this thread
I meant essentially "TARGET_BIG_ENDIAN is always defined").

> Really the whole notion of an ISA having an "endianness" doesn't make
> a lot of sense - it's an individual load or store to memory that has
> an endianness which can depend on a bunch of factors.  When these
> macros were created, an ISA nearly always used the same endianness,
> but that's not really true any more - not just for POWER, but for a
> bunch of targets.  So from that point of view, I think getting rid of
> tswap() - particularly one that has compile time semantics, rather
> than behaviour which can depend on cpu mode/state is a good idea.

I tend to think of the TARGET_BIG_ENDIAN/not setting as being
something like "CPU bus endianness". At least for Arm, when you
put the CPU into BE mode it pretty much means "the CPU byteswaps
the data when it comes in/out", AIUI.

> I believe that even when running in little-endian mode, the hash page
> tables are encoded in big-endian, so I think the proposed change makes
> sense.

OK. I still think we should consistently change all the places that are
accessing this data structure, though, not just half of them.

thanks
-- PMM

Re: [RFC PATCH-for-8.0 2/3] hw/ppc/spapr: Replace tswap64(HPTE) by cpu_to_be64(HPTE)

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2979 bytes --]

On Mon, Dec 19, 2022 at 10:39:40AM +0000, Peter Maydell wrote:
> On Mon, 19 Dec 2022 at 06:35, David Gibson <david@gibson.dropbear.id.au> wrote:
> >
> > On Fri, Dec 16, 2022 at 09:39:19PM +0000, Peter Maydell wrote:
> > > On Fri, 16 Dec 2022 at 19:11, Daniel Henrique Barboza
> > > <danielhb413@gmail.com> wrote:
> > > >
> > > >
> > > >
> > > > On 12/13/22 10:51, Peter Maydell wrote:
> > > > Yes, most if not all accesses are being handled as "target endian", even
> > > > though the target is always big endian.
> >
> > So "target is always big endian" is pretty misleading for POWER.  We
> > always define "TARGET_BIG_ENDIAN" in qemu, but for at least 10 years
> > the CPUs have been capable of running in either big endian or little
> > endian mode (selected at runtime).  Some variants can choose
> > endianness on a per-page basis.  Since the creation of the ISA it's
> > had "byte reversed" load and store instructions that let it use little
> > endian for specific memory accesses.
> 
> Yeah, this is like Arm (and for the purposes of this thread
> I meant essentially "TARGET_BIG_ENDIAN is always defined").

Ok.

> > Really the whole notion of an ISA having an "endianness" doesn't make
> > a lot of sense - it's an individual load or store to memory that has
> > an endianness which can depend on a bunch of factors.  When these
> > macros were created, an ISA nearly always used the same endianness,
> > but that's not really true any more - not just for POWER, but for a
> > bunch of targets.  So from that point of view, I think getting rid of
> > tswap() - particularly one that has compile time semantics, rather
> > than behaviour which can depend on cpu mode/state is a good idea.
> 
> I tend to think of the TARGET_BIG_ENDIAN/not setting as being
> something like "CPU bus endianness". At least for Arm, when you
> put the CPU into BE mode it pretty much means "the CPU byteswaps
> the data when it comes in/out", AIUI.

Hmm, I guess.  We're not really modelling down to the level of bus
byte lanes, though, so I'm not really convinced that's a meaningful
definition in the context of qemu.

> > I believe that even when running in little-endian mode, the hash page
> > tables are encoded in big-endian, so I think the proposed change makes
> > sense.
> 
> OK. I still think we should consistently change all the places that are
> accessing this data structure, though, not just half of them.

Yes, that makes sense.  Although what exactly constitutes "this data
structure" is a bit complex here.  If we mean just the spapr specific
"external HPT", then there are only a few more references to it.  If
we mean all instances of a powerpc hashed page table, then there are a
bunch more in the cpu target code.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH-for-8.0 2/3] hw/ppc/spapr: Replace tswap64(HPTE) by cpu_to_be64(HPTE)

[Peter Maydell]

On Wed, 21 Dec 2022 at 01:35, David Gibson <david@gibson.dropbear.id.au> wrote:
> On Mon, Dec 19, 2022 at 10:39:40AM +0000, Peter Maydell wrote:
> > OK. I still think we should consistently change all the places that are
> > accessing this data structure, though, not just half of them.
>
> Yes, that makes sense.  Although what exactly constitutes "this data
> structure" is a bit complex here.  If we mean just the spapr specific
> "external HPT", then there are only a few more references to it.  If
> we mean all instances of a powerpc hashed page table, then there are a
> bunch more in the cpu target code.

I had in mind "places where we write this specific array of bytes
spapr->htab".

thanks
-- PMM

Re: [RFC PATCH-for-8.0 2/3] hw/ppc/spapr: Replace tswap64(HPTE) by cpu_to_be64(HPTE)

[Cédric Le Goater]

On 12/21/22 13:33, Peter Maydell wrote:
> On Wed, 21 Dec 2022 at 01:35, David Gibson <david@gibson.dropbear.id.au> wrote:
>> On Mon, Dec 19, 2022 at 10:39:40AM +0000, Peter Maydell wrote:
>>> OK. I still think we should consistently change all the places that are
>>> accessing this data structure, though, not just half of them.
>>
>> Yes, that makes sense.  Although what exactly constitutes "this data
>> structure" is a bit complex here.  If we mean just the spapr specific
>> "external HPT", then there are only a few more references to it.  If
>> we mean all instances of a powerpc hashed page table, then there are a
>> bunch more in the cpu target code.
> 
> I had in mind "places where we write this specific array of bytes
> spapr->htab".


spapr_store_hpte() seems to be the most annoying part. It is used
by hcalls h_enter, h_remove, h_protect. Reworking the interface
to present pte0/pte1 as BE variables means reworking the whole
hw/ppc/spapr_softmmu.c file. That's feasible but not a small task
since the changes will root down in the target hash mmu code which
is shared by all platforms ... :/

spapr_hpte_set_c() are spapr_hpte_set_r() are of a different kind.

C.

Re: [RFC PATCH-for-8.0 2/3] hw/ppc/spapr: Replace tswap64(HPTE) by cpu_to_be64(HPTE)

[Peter Maydell]

On Wed, 21 Dec 2022 at 16:03, Cédric Le Goater <clg@kaod.org> wrote:
>
> On 12/21/22 13:33, Peter Maydell wrote:
> > On Wed, 21 Dec 2022 at 01:35, David Gibson <david@gibson.dropbear.id.au> wrote:
> >> On Mon, Dec 19, 2022 at 10:39:40AM +0000, Peter Maydell wrote:
> >>> OK. I still think we should consistently change all the places that are
> >>> accessing this data structure, though, not just half of them.
> >>
> >> Yes, that makes sense.  Although what exactly constitutes "this data
> >> structure" is a bit complex here.  If we mean just the spapr specific
> >> "external HPT", then there are only a few more references to it.  If
> >> we mean all instances of a powerpc hashed page table, then there are a
> >> bunch more in the cpu target code.
> >
> > I had in mind "places where we write this specific array of bytes
> > spapr->htab".
>
>
> spapr_store_hpte() seems to be the most annoying part. It is used
> by hcalls h_enter, h_remove, h_protect. Reworking the interface
> to present pte0/pte1 as BE variables means reworking the whole
> hw/ppc/spapr_softmmu.c file. That's feasible but not a small task
> since the changes will root down in the target hash mmu code which
> is shared by all platforms ... :/

Don't you just need to change spapr_store_hpte() to use stq_be_p()
instead of stq_p() ?

> spapr_hpte_set_c() are spapr_hpte_set_r() are of a different kind.

That code seems to suggest we already implicitly assume that
spapr->htab fields have a given endianness...

thanks
-- PMM

Re: [RFC PATCH-for-8.0 2/3] hw/ppc/spapr: Replace tswap64(HPTE) by cpu_to_be64(HPTE)

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2109 bytes --]

On Wed, Dec 21, 2022 at 10:15:28PM +0000, Peter Maydell wrote:
> On Wed, 21 Dec 2022 at 16:03, Cédric Le Goater <clg@kaod.org> wrote:
> >
> > On 12/21/22 13:33, Peter Maydell wrote:
> > > On Wed, 21 Dec 2022 at 01:35, David Gibson <david@gibson.dropbear.id.au> wrote:
> > >> On Mon, Dec 19, 2022 at 10:39:40AM +0000, Peter Maydell wrote:
> > >>> OK. I still think we should consistently change all the places that are
> > >>> accessing this data structure, though, not just half of them.
> > >>
> > >> Yes, that makes sense.  Although what exactly constitutes "this data
> > >> structure" is a bit complex here.  If we mean just the spapr specific
> > >> "external HPT", then there are only a few more references to it.  If
> > >> we mean all instances of a powerpc hashed page table, then there are a
> > >> bunch more in the cpu target code.
> > >
> > > I had in mind "places where we write this specific array of bytes
> > > spapr->htab".

Seems a reasonable amount to tackle for now.

> > spapr_store_hpte() seems to be the most annoying part. It is used
> > by hcalls h_enter, h_remove, h_protect. Reworking the interface
> > to present pte0/pte1 as BE variables means reworking the whole
> > hw/ppc/spapr_softmmu.c file. That's feasible but not a small task
> > since the changes will root down in the target hash mmu code which
> > is shared by all platforms ... :/
> 
> Don't you just need to change spapr_store_hpte() to use stq_be_p()
> instead of stq_p() ?

I think Peter is right.  The values passed to the function are "host
endian" (really, they don't have an endianness since they'll be in
registers).

> > spapr_hpte_set_c() are spapr_hpte_set_r() are of a different kind.
> 
> That code seems to suggest we already implicitly assume that
> spapr->htab fields have a given endianness...

Yes, we absolutely do.  We rely on the HPTE always being big-endian.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH-for-8.0 1/3] hw/ppc: Replace tswap32() by const_le32()

[Thomas Huth]

On 13/12/2022 19.13, Cédric Le Goater wrote:
> On 12/13/22 18:23, Peter Maydell wrote:
>> On Tue, 13 Dec 2022 at 16:53, Cédric Le Goater <clg@kaod.org> wrote:
>>>
>>> On 12/13/22 17:27, Richard Henderson wrote:
>>>> On 12/13/22 10:21, Peter Maydell wrote:
>>>>> It does seem odd, though. We have a value in host endianness
>>>>> (the EPAPR_MAGIC constant, which is host-endian by virtue of
>>>>> being a C constant). But we're storing it to env->gpr[], which
>>>>> is to say the CPUPPCState general purpose register array. Isn't
>>>>> that array *also* kept in host endianness order?
>>>>
>>>> Yes indeed.
>>>>
>>>>> If so, then the right thing here is "don't swap at all",
>>>>
>>>> So it would seem...
>>>>
>>>>> i.e. just "env->gpr[6] = EPAPR_MAGIC;". But that would imply
>>>>> that the current code is setting the wrong value for the GPR
>>>>> on little-endian hosts, which seems a bit unlikely...
>>>>
>>>> ... unless this board has only been tested on matching hosts.
>>>
>>> But these are register default values. Endianness doesn't apply
>>> there. Doesn't it ?
>>
>> Any time you have a value that's more than 1 byte wide,
>> endianness applies in some sense :-) We choose for our
>> emulated CPUs typically to keep register values in struct
>> fields and variables in the C code in host endianness. This
>> is the "obvious" choice given that you want to be able to
>> do things like do a simple host add to emulate a guest CPU
>> add, but in theory you could store the values the other
>> way around if you wanted (then "store register into RAM"
>> would be trivial, and "add 1 to register" would require
>> extra effort; currently it's the other way round.)
>>
>> Anyway, I think that in the virtex_ml507 and sam460ex code
>> the use of tswap32() should be removed. In hw/ppc/e500.c
>> we get this right:
>>      env->gpr[6] = EPAPR_MAGIC;
>>
>> We have a Linux kernel boot test in the avocado tests for
>> virtex_ml507 -- we really do set up this magic value wrongly
>> afaict, but it seems like the kernel doesn't check it (the
>> test passes regardless of whether we swap the value or not).
>>
>> I think what has happened here is that this bit of code is
>> setting up CPU registers for an EPAPR style boot, but the
>> test kernel at least doesn't expect that. It boots via the
>> code in arch/powerpc/kernel/head_44x.S. That file claims
>> in a comment that it expects
>>   *   r3 - Board info structure pointer (DRAM, frequency, MAC address, etc.)
>>   *   r4 - Starting address of the init RAM disk
>>   *   r5 - Ending address of the init RAM disk
>>   *   r6 - Start of kernel command line string (e.g. "mem=128")
>>   *   r7 - End of kernel command line string
>>
>> but actually it only cares that r3 == device-tree-blob.
>>
>> Documentation/powerpc/booting.rst says the expectation
>> (for a non-OpenFirmware boot) is:
>>                  r3 : physical pointer to the device-tree block
>>                  (defined in chapter II) in RAM
>>
>>                  r4 : physical pointer to the kernel itself. This is
>>                  used by the assembly code to properly disable the MMU
>>                  in case you are entering the kernel with MMU enabled
>>                  and a non-1:1 mapping.
>>
>>                  r5 : NULL (as to differentiate with method a)
>>
>> which isn't the same as what the kernel code actually cares about
>> or what the kernel's comment says it cares about...
>>
>> So my guess about what's happening here is that the intention
>> was that these boards should be able to boot both kernels built
>> to be entered directly in the way booting.rst says, and also
>> kernels and other guest programs built to assume boot by
>> EPAPR firmware, but this bug means that we're only currently
>> supporting the first of these two categories. The reason nobody's
>> noticed before is presumably that in practice nobody's trying to
>> boot the "built to boot from EPAPR firmware" type binary on
>> these two boards.
>>
>> TLDR: we should drop the "tswap32()" entirely from both files.
> Yes. I think so too.

I agree. Philippe, could you please respin this patch 1 here accordingly?

  Thomas

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

[Philippe Mathieu-Daudé]

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