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