2022-07-11 14:49:18

by Christophe Leroy

[permalink] [raw]
Subject: [PATCH v1 2/5] powerpc/32: Don't always pass -mcpu=powerpc to the compiler

Since commit 4bf4f42a2feb ("powerpc/kbuild: Set default generic
machine type for 32-bit compile"), when building a 32 bits kernel
with a bi-arch version of GCC, or when building a book3s/32 kernel,
the option -mcpu=powerpc is passed to GCC at all time, relying on it
being eventually overriden by a subsequent -mcpu=xxxx.

But when building the same kernel with a 32 bits only version of GCC,
that is not done, relying on gcc being built with the expected default
CPU.

This logic has two problems. First, it is a bit fragile to rely on
whether the GCC version is bi-arch or not, because today we can have
bi-arch versions of GCC configured with a 32 bits default. Second,
there are some versions of GCC which don't support -mcpu=powerpc,
for instance for e500 SPE-only versions.

So, stop relying on this approximative logic and allow the user to
decide whether he/she wants to use the toolchain's default CPU or if
he/she wants to set one, and allow only possible CPUs based on the
selected target.

Reported-by: Pali Rohár <[email protected]>
Tested-by: Pali Rohár <[email protected]>
Reviewed-by: Arnd Bergmann <[email protected]>
Cc: Segher Boessenkool <[email protected]>
Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/Makefile | 26 +-------------------------
arch/powerpc/platforms/Kconfig.cputype | 21 ++++++++++++++++++---
2 files changed, 19 insertions(+), 28 deletions(-)

diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index a0cd70712061..d54e1fe03551 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -15,23 +15,6 @@ HAS_BIARCH := $(call cc-option-yn, -m32)
# Set default 32 bits cross compilers for vdso and boot wrapper
CROSS32_COMPILE ?=

-ifeq ($(HAS_BIARCH),y)
-ifeq ($(CROSS32_COMPILE),)
-ifdef CONFIG_PPC32
-# These options will be overridden by any -mcpu option that the CPU
-# or platform code sets later on the command line, but they are needed
-# to set a sane 32-bit cpu target for the 64-bit cross compiler which
-# may default to the wrong ISA.
-KBUILD_CFLAGS += -mcpu=powerpc
-KBUILD_AFLAGS += -mcpu=powerpc
-endif
-endif
-endif
-
-ifdef CONFIG_PPC_BOOK3S_32
-KBUILD_CFLAGS += -mcpu=powerpc
-endif
-
# If we're on a ppc/ppc64/ppc64le machine use that defconfig, otherwise just use
# ppc64_defconfig because we have nothing better to go on.
uname := $(shell uname -m)
@@ -183,6 +166,7 @@ endif
endif

CFLAGS-$(CONFIG_TARGET_CPU_BOOL) += $(call cc-option,-mcpu=$(CONFIG_TARGET_CPU))
+AFLAGS-$(CONFIG_TARGET_CPU_BOOL) += $(call cc-option,-mcpu=$(CONFIG_TARGET_CPU))

# Altivec option not allowed with e500mc64 in GCC.
ifdef CONFIG_ALTIVEC
@@ -193,14 +177,6 @@ endif
CFLAGS-$(CONFIG_E5500_CPU) += $(E5500_CPU)
CFLAGS-$(CONFIG_E6500_CPU) += $(call cc-option,-mcpu=e6500,$(E5500_CPU))

-ifdef CONFIG_PPC32
-ifdef CONFIG_PPC_E500MC
-CFLAGS-y += $(call cc-option,-mcpu=e500mc,-mcpu=powerpc)
-else
-CFLAGS-$(CONFIG_E500) += $(call cc-option,-mcpu=8540 -msoft-float,-mcpu=powerpc)
-endif
-endif
-
asinstr := $(call as-instr,lis 9$(comma)foo@high,-DHAVE_AS_ATHIGH=1)

KBUILD_CPPFLAGS += -I $(srctree)/arch/$(ARCH) $(asinstr)
diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platforms/Kconfig.cputype
index 9805a2c717b9..bfea5d8452f8 100644
--- a/arch/powerpc/platforms/Kconfig.cputype
+++ b/arch/powerpc/platforms/Kconfig.cputype
@@ -135,9 +135,9 @@ config GENERIC_CPU
select ARCH_HAS_FAST_MULTIPLIER
select PPC_64S_HASH_MMU

-config GENERIC_CPU
+config POWERPC_CPU
bool "Generic 32 bits powerpc"
- depends on PPC32 && !PPC_8xx
+ depends on PPC32 && !PPC_8xx && !PPC_85xx

config CELL_CPU
bool "Cell Broadband Engine"
@@ -196,11 +196,23 @@ config G4_CPU
depends on PPC_BOOK3S_32
select ALTIVEC

+config E500_CPU
+ bool "e500 (8540)"
+ depends on PPC_85xx && !PPC_E500MC
+
+config E500MC_CPU
+ bool "e500mc"
+ depends on PPC_85xx && PPC_E500MC
+
+config TOOLCHAIN_DEFAULT_CPU
+ bool "Rely on the toolchain's implicit default CPU"
+ depends on PPC32
+
endchoice

config TARGET_CPU_BOOL
bool
- default !GENERIC_CPU
+ default !GENERIC_CPU && !TOOLCHAIN_DEFAULT_CPU

config TARGET_CPU
string
@@ -215,6 +227,9 @@ config TARGET_CPU
default "e300c2" if E300C2_CPU
default "e300c3" if E300C3_CPU
default "G4" if G4_CPU
+ default "8540" if E500_CPU
+ default "e500mc" if E500MC_CPU
+ default "powerpc" if POWERPC_CPU

config PPC_BOOK3S
def_bool y
--
2.36.1


2022-07-11 17:47:11

by Segher Boessenkool

[permalink] [raw]
Subject: Re: [PATCH v1 2/5] powerpc/32: Don't always pass -mcpu=powerpc to the compiler

On Mon, Jul 11, 2022 at 04:19:30PM +0200, Christophe Leroy wrote:
> Since commit 4bf4f42a2feb ("powerpc/kbuild: Set default generic
> machine type for 32-bit compile"), when building a 32 bits kernel
> with a bi-arch version of GCC, or when building a book3s/32 kernel,
> the option -mcpu=powerpc is passed to GCC at all time, relying on it
> being eventually overriden by a subsequent -mcpu=xxxx.
>
> But when building the same kernel with a 32 bits only version of GCC,
> that is not done, relying on gcc being built with the expected default
> CPU.
>
> This logic has two problems. First, it is a bit fragile to rely on
> whether the GCC version is bi-arch or not, because today we can have
> bi-arch versions of GCC configured with a 32 bits default. Second,
> there are some versions of GCC which don't support -mcpu=powerpc,
> for instance for e500 SPE-only versions.

More fundamentally, the *only* thing you should check biarch for is
for determining if you can use both -m32 and -m64 with the same
compiler. Everything behaves identically in a biarch and a non-biarch
compiler, other than the latter screams bloody murder if you try to
change the architecture from 32 to 64 bit or vice versa.

> So, stop relying on this approximative logic and allow the user to
> decide whether he/she wants to use the toolchain's default CPU or if
> he/she wants to set one, and allow only possible CPUs based on the
> selected target.
>
> Reported-by: Pali Roh?r <[email protected]>
> Tested-by: Pali Roh?r <[email protected]>
> Reviewed-by: Arnd Bergmann <[email protected]>
> Cc: Segher Boessenkool <[email protected]>
> Signed-off-by: Christophe Leroy <[email protected]>

Reviewed-by: Segher Boessenkool <[email protected]>

Looks good to me. This untangles/demystifies quite some code :-)


Segher

2022-08-18 18:10:53

by Pali Rohár

[permalink] [raw]
Subject: Re: [PATCH v1 2/5] powerpc/32: Don't always pass -mcpu=powerpc to the compiler

On Monday 11 July 2022 16:19:30 Christophe Leroy wrote:
> Since commit 4bf4f42a2feb ("powerpc/kbuild: Set default generic
> machine type for 32-bit compile"), when building a 32 bits kernel
> with a bi-arch version of GCC, or when building a book3s/32 kernel,
> the option -mcpu=powerpc is passed to GCC at all time, relying on it
> being eventually overriden by a subsequent -mcpu=xxxx.
>
> But when building the same kernel with a 32 bits only version of GCC,
> that is not done, relying on gcc being built with the expected default
> CPU.
>
> This logic has two problems. First, it is a bit fragile to rely on
> whether the GCC version is bi-arch or not, because today we can have
> bi-arch versions of GCC configured with a 32 bits default. Second,
> there are some versions of GCC which don't support -mcpu=powerpc,
> for instance for e500 SPE-only versions.
>
> So, stop relying on this approximative logic and allow the user to
> decide whether he/she wants to use the toolchain's default CPU or if
> he/she wants to set one, and allow only possible CPUs based on the
> selected target.

Hello! Exactly same issue is still in file arch/powerpc/boot/Makefile:

ifdef CONFIG_PPC64_BOOT_WRAPPER
ifdef CONFIG_CPU_LITTLE_ENDIAN
BOOTCFLAGS += -m64 -mcpu=powerpc64le
else
BOOTCFLAGS += -m64 -mcpu=powerpc64
endif
else
BOOTCFLAGS += -m32 -mcpu=powerpc
endif

It cause compile error:

make ARCH=powerpc CROSS_COMPILE=powerpc-linux-gnuspe- mpc85xx_smp_defconfig uImage
...
BOOTAS arch/powerpc/boot/crt0.o
powerpc-linux-gnuspe-gcc: error: unrecognized argument in option ‘-mcpu=powerpc’
powerpc-linux-gnuspe-gcc: note: valid arguments to ‘-mcpu=’ are: 8540 8548 native
make[1]: *** [arch/powerpc/boot/Makefile:231: arch/powerpc/boot/crt0.o] Error 1

> Reported-by: Pali Rohár <[email protected]>
> Tested-by: Pali Rohár <[email protected]>
> Reviewed-by: Arnd Bergmann <[email protected]>
> Cc: Segher Boessenkool <[email protected]>
> Signed-off-by: Christophe Leroy <[email protected]>
> ---
> arch/powerpc/Makefile | 26 +-------------------------
> arch/powerpc/platforms/Kconfig.cputype | 21 ++++++++++++++++++---
> 2 files changed, 19 insertions(+), 28 deletions(-)

2022-08-20 11:12:58

by Pali Rohár

[permalink] [raw]
Subject: Re: [PATCH v1 2/5] powerpc/32: Don't always pass -mcpu=powerpc to the compiler

On Thursday 18 August 2022 19:46:34 Pali Rohár wrote:
> On Monday 11 July 2022 16:19:30 Christophe Leroy wrote:
> > Since commit 4bf4f42a2feb ("powerpc/kbuild: Set default generic
> > machine type for 32-bit compile"), when building a 32 bits kernel
> > with a bi-arch version of GCC, or when building a book3s/32 kernel,
> > the option -mcpu=powerpc is passed to GCC at all time, relying on it
> > being eventually overriden by a subsequent -mcpu=xxxx.
> >
> > But when building the same kernel with a 32 bits only version of GCC,
> > that is not done, relying on gcc being built with the expected default
> > CPU.
> >
> > This logic has two problems. First, it is a bit fragile to rely on
> > whether the GCC version is bi-arch or not, because today we can have
> > bi-arch versions of GCC configured with a 32 bits default. Second,
> > there are some versions of GCC which don't support -mcpu=powerpc,
> > for instance for e500 SPE-only versions.
> >
> > So, stop relying on this approximative logic and allow the user to
> > decide whether he/she wants to use the toolchain's default CPU or if
> > he/she wants to set one, and allow only possible CPUs based on the
> > selected target.
>
> Hello! Exactly same issue is still in file arch/powerpc/boot/Makefile:
>
> ifdef CONFIG_PPC64_BOOT_WRAPPER
> ifdef CONFIG_CPU_LITTLE_ENDIAN
> BOOTCFLAGS += -m64 -mcpu=powerpc64le
> else
> BOOTCFLAGS += -m64 -mcpu=powerpc64
> endif
> else
> BOOTCFLAGS += -m32 -mcpu=powerpc
> endif
>
> It cause compile error:
>
> make ARCH=powerpc CROSS_COMPILE=powerpc-linux-gnuspe- mpc85xx_smp_defconfig uImage
> ...
> BOOTAS arch/powerpc/boot/crt0.o
> powerpc-linux-gnuspe-gcc: error: unrecognized argument in option ‘-mcpu=powerpc’
> powerpc-linux-gnuspe-gcc: note: valid arguments to ‘-mcpu=’ are: 8540 8548 native
> make[1]: *** [arch/powerpc/boot/Makefile:231: arch/powerpc/boot/crt0.o] Error 1

Now I have sent patch for this issue:
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/[email protected]/

> > Reported-by: Pali Rohár <[email protected]>
> > Tested-by: Pali Rohár <[email protected]>
> > Reviewed-by: Arnd Bergmann <[email protected]>
> > Cc: Segher Boessenkool <[email protected]>
> > Signed-off-by: Christophe Leroy <[email protected]>
> > ---
> > arch/powerpc/Makefile | 26 +-------------------------
> > arch/powerpc/platforms/Kconfig.cputype | 21 ++++++++++++++++++---
> > 2 files changed, 19 insertions(+), 28 deletions(-)