[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: |
Edgar E. Iglesias |
Subject: |
Re: [RFC PATCH-for-8.0 1/3] hw/ppc: Replace tswap32() by const_le32() |
Date: |
Tue, 13 Dec 2022 18:51:46 +0100 |
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
- [RFC PATCH-for-8.0 0/3] hw/ppc: Remove tswap() calls, Philippe Mathieu-Daudé, 2022/12/13
- [RFC PATCH-for-8.0 1/3] hw/ppc: Replace tswap32() by const_le32(), Philippe Mathieu-Daudé, 2022/12/13
- Re: [RFC PATCH-for-8.0 1/3] hw/ppc: Replace tswap32() by const_le32(), Richard Henderson, 2022/12/13
- Re: [RFC PATCH-for-8.0 1/3] hw/ppc: Replace tswap32() by const_le32(), Philippe Mathieu-Daudé, 2022/12/13
- Re: [RFC PATCH-for-8.0 1/3] hw/ppc: Replace tswap32() by const_le32(), Richard Henderson, 2022/12/13
- Re: [RFC PATCH-for-8.0 1/3] hw/ppc: Replace tswap32() by const_le32(), Peter Maydell, 2022/12/13
- Re: [RFC PATCH-for-8.0 1/3] hw/ppc: Replace tswap32() by const_le32(), Richard Henderson, 2022/12/13
- Re: [RFC PATCH-for-8.0 1/3] hw/ppc: Replace tswap32() by const_le32(), Cédric Le Goater, 2022/12/13
- Re: [RFC PATCH-for-8.0 1/3] hw/ppc: Replace tswap32() by const_le32(), Peter Maydell, 2022/12/13
- Re: [RFC PATCH-for-8.0 1/3] hw/ppc: Replace tswap32() by const_le32(),
Edgar E. Iglesias <=
- Re: [RFC PATCH-for-8.0 1/3] hw/ppc: Replace tswap32() by const_le32(), BALATON Zoltan, 2022/12/13
- Re: [RFC PATCH-for-8.0 1/3] hw/ppc: Replace tswap32() by const_le32(), Peter Maydell, 2022/12/13
- Re: [RFC PATCH-for-8.0 1/3] hw/ppc: Replace tswap32() by const_le32(), Cédric Le Goater, 2022/12/13
[RFC PATCH-for-8.0 2/3] hw/ppc/spapr: Replace tswap64(HPTE) by cpu_to_be64(HPTE), Philippe Mathieu-Daudé, 2022/12/13
- Re: [RFC PATCH-for-8.0 2/3] hw/ppc/spapr: Replace tswap64(HPTE) by cpu_to_be64(HPTE), Peter Maydell, 2022/12/13
- Re: [RFC PATCH-for-8.0 2/3] hw/ppc/spapr: Replace tswap64(HPTE) by cpu_to_be64(HPTE), Daniel Henrique Barboza, 2022/12/16
- Re: [RFC PATCH-for-8.0 2/3] hw/ppc/spapr: Replace tswap64(HPTE) by cpu_to_be64(HPTE), Peter Maydell, 2022/12/16
- Re: [RFC PATCH-for-8.0 2/3] hw/ppc/spapr: Replace tswap64(HPTE) by cpu_to_be64(HPTE), David Gibson, 2022/12/19
- Re: [RFC PATCH-for-8.0 2/3] hw/ppc/spapr: Replace tswap64(HPTE) by cpu_to_be64(HPTE), Peter Maydell, 2022/12/19
- Re: [RFC PATCH-for-8.0 2/3] hw/ppc/spapr: Replace tswap64(HPTE) by cpu_to_be64(HPTE), David Gibson, 2022/12/20