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 <c...@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