Re: [PATCH] powerpc/boot: Build wrapper for an appropriate CPU

["Murilo Opsfelder Araújo" <mopsfelder@gmail.com>]

Hi, Joel.

On 3/31/22 02:01, Joel Stanley wrote:
> On Thu, 31 Mar 2022 at 02:05, Murilo Opsfelder Araújo
> <mopsfelder@gmail.com> wrote:
>>
>> Hi, Joel.
>>
>> On 3/30/22 08:24, Joel Stanley wrote:
>>> Currently the boot wrapper lacks a -mcpu option, so it will be built for
>>> the toolchain's default cpu. This is a problem if the toolchain defaults
>>> to a cpu with newer instructions.
>>>
>>> We could wire in TARGET_CPU but instead use the oldest supported option
>>> so the wrapper runs anywhere.
>>>
>>> The GCC documentation stays that -mcpu=powerpc64le will give us a
>>> generic 64 bit powerpc machine:
>>>
>>>    -mcpu=powerpc, -mcpu=powerpc64, and -mcpu=powerpc64le specify pure
>>>    32-bit PowerPC (either endian), 64-bit big endian PowerPC and 64-bit
>>>    little endian PowerPC architecture machine types, with an appropriate,
>>>    generic processor model assumed for scheduling purposes.
>>>
>>> So do that for each of the three machines.
>>>
>>> This bug was found when building the kernel with a toolchain that
>>> defaulted to powre10, resulting in a pcrel enabled wrapper which fails
>>> to link:
>>>
>>>    arch/powerpc/boot/wrapper.a(crt0.o): in function `p_base':
>>>    (.text+0x150): call to `platform_init' lacks nop, can't restore toc; (toc save/adjust stub)
>>>    (.text+0x154): call to `start' lacks nop, can't restore toc; (toc save/adjust stub)
>>>    powerpc64le-buildroot-linux-gnu-ld: final link failed: bad value
>>>
>>> Even with tha bug worked around the resulting kernel would crash on a
>>> power9 box:
>>>
>>>    $ qemu-system-ppc64 -nographic -nodefaults -M powernv9 -kernel arch/powerpc/boot/zImage.epapr -serial mon:stdio
>>>    [    7.069331356,5] INIT: Starting kernel at 0x20010020, fdt at 0x3068c628 25694 bytes
>>>    [    7.130374661,3] ***********************************************
>>>    [    7.131072886,3] Fatal Exception 0xe40 at 00000000200101e4    MSR 9000000000000001
>>>    [    7.131290613,3] CFAR : 000000002001027c MSR  : 9000000000000001
>>>    [    7.131433759,3] SRR0 : 0000000020010050 SRR1 : 9000000000000001
>>>    [    7.131577775,3] HSRR0: 00000000200101e4 HSRR1: 9000000000000001
>>>    [    7.131733687,3] DSISR: 00000000         DAR  : 0000000000000000
>>>    [    7.131905162,3] LR   : 0000000020010280 CTR  : 0000000000000000
>>>    [    7.132068356,3] CR   : 44002004         XER  : 00000000
>>>
>>> Link: https://github.com/linuxppc/issues/issues/400
>>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>>> ---
>>> Tested:
>>>
>>>    - ppc64le_defconfig
>>>    - pseries and powernv qemu, for power8, power9, power10 cpus
>>>    - buildroot compiler that defaults to -mcpu=power10 (gcc 10.3.0, ld 2.36.1)
>>>    -  RHEL9 cross compilers (gcc 11.2.1-1, ld 2.35.2-17.el9)
>>>
>>> All decompressed and made it into the kernel ok.
>>>
>>> ppc64_defconfig did not work, as we've got a regression when the wrapper
>>> is built for big endian. It hasn't worked for zImage.pseries for a long
>>> time (at least v4.14), and broke some time between v5.4 and v5.17 for
>>> zImage.epapr.
>>>
>>>    arch/powerpc/boot/Makefile | 8 ++++++--
>>>    1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/powerpc/boot/Makefile b/arch/powerpc/boot/Makefile
>>> index 9993c6256ad2..1f5cc401bfc0 100644
>>> --- a/arch/powerpc/boot/Makefile
>>> +++ b/arch/powerpc/boot/Makefile
>>> @@ -38,9 +38,13 @@ BOOTCFLAGS    := -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs \
>>>                 $(LINUXINCLUDE)
>>>
>>>    ifdef CONFIG_PPC64_BOOT_WRAPPER
>>> -BOOTCFLAGS   += -m64
>>> +ifdef CONFIG_CPU_LITTLE_ENDIAN
>>> +BOOTCFLAGS   += -m64 -mcpu=powerpc64le
>>>    else
>>> -BOOTCFLAGS   += -m32
>>> +BOOTCFLAGS   += -m64 -mcpu=powerpc64
>>> +endif
>>> +else
>>> +BOOTCFLAGS   += -m32 -mcpu=powerpc
>>>    endif
>>>
>>>    BOOTCFLAGS  += -isystem $(shell $(BOOTCC) -print-file-name=include)
>>
>> I think it was a fortunate coincidence that the default cpu type of your gcc is
>> compatible with your system.  If the distro gcc moves its default to a newer cpu
>> type than your system, this bug would happen again.
> 
> Perhaps I needed to be clear in my commit message: that's the exact
> bug I'm looking to avoid. I have a buildroot toolchain that was built
> for -mcpu=power10.
> 
> I think you're suggesting the -mcpu=powerpc64 option will change it 's
> behavior depending on the default. From my reading of the man page, I
> don't think that's true.

Looking at GCC source code (gcc/config/rs6000/rs6000.h:287), -mcpu=powerpc64 seems
to select "rs64" as the default cpu type.

     284 /* Define generic processor types based upon current deployment.  */
     285 #define PROCESSOR_COMMON    PROCESSOR_PPC601
     286 #define PROCESSOR_POWERPC   PROCESSOR_PPC604
     287 #define PROCESSOR_POWERPC64 PROCESSOR_RS64A

Then in gcc/config/rs6000/linux64.h:77 it sets the 64-bit default with power8.

     74 #undef  PROCESSOR_DEFAULT
     75 #define PROCESSOR_DEFAULT PROCESSOR_POWER7
     76 #undef  PROCESSOR_DEFAULT64
     77 #define PROCESSOR_DEFAULT64 PROCESSOR_POWER8

My understanding is that the default cpu type for -mcpu=powerpc64 can change.
If that change is unlikely to happen, that's a separate discussion.

> 
> I did a little test using my buildroot compiler which has
> with-cpu=power10. I used the presence of PCREL relocations as evidence
> that it was build for power10.
> 
> $ powerpc64le-buildroot-linux-gnu-gcc -mcpu=power10 -c test.c
> $ readelf -r test.o |grep -c PCREL
> 24

It respected the -mcpu=power10 you provided.

> $ powerpc64le-buildroot-linux-gnu-gcc -c test.c
> $ readelf -r test.o |grep -c PCREL
> 24

You didn't provide -mcpu, the default --with-cpu=power10 was used.

> $ powerpc64le-buildroot-linux-gnu-gcc -mcpu=powerpc64le -c test.c
> $ readelf -r test.o |grep -c PCREL
> 0

It likely defaulted to power8.

And that's my concern.  We're relying on the compiler default cpu type.

If gcc defaults -mcpu=powerpc64le to power10, you're going to have
the same problem again.  It happens that the power8 default cpu type
is compatible to your system by coincidence.

> 
>>
>> The command "gcc -v |& grep with-cpu" will show you the default cpu type for 32
>> and 64-bit that gcc was configured.
> 
> Just a headss up: this gives me no output for the 64 bit compilers on my laptop:
> 
> $ powerpc64le-linux-gnu-gcc -v |& grep  with-cpu
> $ echo $?
> 1
> 
> $ powerpc64-linux-gnu-gcc -v |& grep  with-cpu
> $ echo $?
> 1
> 
> It reports --with-cpu=default32 for the 32 bit compiler.
> 

This is what native gcc version 11.2.1 reports on Fedora 35 ppc64le:

     --with-cpu-32=power8 --with-cpu-64=power8

>>
>> Considering the CONFIG_TARGET_CPU for BOOTCFLAGS would bring some level of
>> consistency between CFLAGS and BOOTCFLAGS regarding -mcpu value.
>>
>> We could mimic the behaviour from arch/powerpc/Makefile:
> 
> This was the inspiration for my change. I first took it verbatim, and
> then did a bit of reading about what -mcpu actually sets. Reading the
> GCC source it seems powerpc64le is equivalent to power8. powerpc64 is
> less clear.

In gcc/config/rs6000/rs6000-cpus.def, they are set to different processors:

     254 RS6000_CPU ("powerpc64", PROCESSOR_POWERPC64, MASK_PPC_GFXOPT | MASK_POWERPC64)
     255 RS6000_CPU ("powerpc64le", PROCESSOR_POWER8, MASK_POWERPC64 | ISA_2_7_MASKS_SERVER

> 
> So I a agree with your suggestion. Hopefully my patch has the equivalent result.

My suggestion was to explicitly set -mcpu=power8 instead of -mcpu=powerpc64le.

We can set -mcpu=power8 for generic cpu, and then override it with target cpu.

That would be consistent with arch/powerpc/Makefile.

> 
> 
>>
>>       166 ifdef config_ppc_book3s_64
>>       167 ifdef config_cpu_little_endian
>>       168 cflags-$(config_generic_cpu) += -mcpu=power8
>>       169 cflags-$(config_generic_cpu) += $(call cc-option,-mtune=power9,-mtune=power8)
>>       170 else
>>       171 cflags-$(config_generic_cpu) += $(call cc-option,-mtune=power7,$(call cc-option,-mtune=power5))
>>       172 cflags-$(config_generic_cpu) += $(call cc-option,-mcpu=power5,-mcpu=power4)
>>       173 endif
>>       174 else ifdef config_ppc_book3e_64
>>       175 cflags-$(config_generic_cpu) += -mcpu=powerpc64
>>       176 endif
>>       ...
>>       185 CFLAGS-$(CONFIG_TARGET_CPU_BOOL) += $(call cc-option,-mcpu=$(CONFIG_TARGET_CPU))
>>
>> Cheers!
>>
>> --
>> Murilo

Cheers!

-- 
Murilo