diff mbox series

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

Message ID 20221213125218.39868-2-philmd@linaro.org (mailing list archive)
State New, archived
Headers show
Series hw/ppc: Remove tswap() calls | expand

Commit Message

Philippe Mathieu-Daudé Dec. 13, 2022, 12:52 p.m. UTC
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(-)

Comments

Richard Henderson Dec. 13, 2022, 4 p.m. UTC | #1
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~
Philippe Mathieu-Daudé Dec. 13, 2022, 4:10 p.m. UTC | #2
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.
Richard Henderson Dec. 13, 2022, 4:14 p.m. UTC | #3
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~
Peter Maydell Dec. 13, 2022, 4:21 p.m. UTC | #4
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
Richard Henderson Dec. 13, 2022, 4:27 p.m. UTC | #5
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~
Cédric Le Goater Dec. 13, 2022, 4:53 p.m. UTC | #6
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.
Peter Maydell Dec. 13, 2022, 5:23 p.m. UTC | #7
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
Edgar E. Iglesias Dec. 13, 2022, 5:51 p.m. UTC | #8
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
BALATON Zoltan Dec. 13, 2022, 6:09 p.m. UTC | #9
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
>
>
Cédric Le Goater Dec. 13, 2022, 6:13 p.m. UTC | #10
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.
Peter Maydell Dec. 13, 2022, 9:37 p.m. UTC | #11
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
Thomas Huth May 17, 2023, 10:51 a.m. UTC | #12
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
diff mbox series

Patch

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