[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: BALATON Zoltan
Subject: Re: [RFC PATCH-for-8.0 1/3] hw/ppc: Replace tswap32() by const_le32()
Date: Tue, 13 Dec 2022 19:09:51 +0100 (CET)

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:


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.


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.

-- PMM

reply via email to

[Prev in Thread] Current Thread [Next in Thread]