| Message ID | 20221213125218.39868-2-philmd@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/ppc: Remove tswap() calls | expand |
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~
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.
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~
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
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~
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.
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
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
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 > >
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.
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
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 --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; }
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(-)