Re: [PATCH] powerpc: e500: Fix compilation with gcc e500 compiler

["Pali Rohár" <pali@kernel.org>]

On Monday 04 July 2022 15:13:58 Pali Rohár wrote:
> On Monday 04 July 2022 14:07:10 Arnd Bergmann wrote:
> > On Mon, Jul 4, 2022 at 12:39 PM Pali Rohár <pali@kernel.org> wrote:
> > > On Monday 04 July 2022 20:23:29 Michael Ellerman wrote:
> > > > On 2 July 2022 7:44:05 pm AEST, "Pali Rohár" <pali@kernel.org> wrote:
> > > > >On Tuesday 24 May 2022 11:39:39 Pali Rohár wrote:
> > > > >> gcc e500 compiler does not support -mcpu=powerpc option. When it is
> > > > >> specified then gcc throws compile error:
> > > > >>
> > > > >>   gcc: error: unrecognized argument in option ‘-mcpu=powerpc’
> > > > >>   gcc: note: valid arguments to ‘-mcpu=’ are: 8540 8548 native
> > > > >>
> > > > >> So do not set -mcpu=powerpc option when CONFIG_E500 is set. Correct option
> > > > >> -mcpu=8540 for CONFIG_E500 is set few lines below in that Makefile.
> > > > >>
> > > > >> Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > >> Cc: stable@vger.kernel.org
> > > > >
> > > > >Michael, do you have any objections about this patch?
> > > >
> > > > I don't particularly like it :)
> > > >
> > > > From the discussion with Segher, it sounds like this is a problem with a specific build of gcc that you're using, not a general problem with gcc built with e500 support.
> > >
> > > Well, the "full" build of gcc for e500 cores with SPE does not support
> > > -mcpu=powerpc option. So I think this is a general problem. I do not
> > > think that this is "specific build" as this is the correct build of gcc
> > > for these processors with e500 cores.
> > >
> > > "stripped". build of gcc without SPE support for e500 cores does not
> > > have this problem...
> > 
> > I can see a couple of problems with the CPU selection, but I don't think
> > this is a major one, as nobody should be using those SPE compilers for
> > building the kernel. Just use a modern powerpc-gcc build.
> 
> The point is to use same compiler for building kernel as for the all
> other parts of the system.
> 
> I just do not see reason why for kernel it is needed to build completely
> different toolchain and compiler.
> 
> > > > Keying it off CONFIG_E500 means it will fix your problem, but not anyone else who has a different non-e500 compiler that also doesn't support -mcpu=powerpc (for whatever reason).
> > > >
> > > > So I wonder if a better fix is to use cc-option when setting -mcpu=powerpc.
> > > >
> > >
> > > Comment for that code which adds -mpcu=powerpc says:
> > >
> > >   they are needed to set a sane 32-bit cpu target for the 64-bit cross
> > >   compiler which may default to the wrong ISA.
> > >
> > > So I'm not sure how to handle this in other way. GCC uses -mpcu=8540
> > > option for specifying to compile code for e500 cores and seems that
> > > -mcpu=8540 is supported by all e500 compilers...
> > >
> > > Few lines below is code
> > >
> > >   CFLAGS-$(CONFIG_E500) += $(call cc-option,-mcpu=8540 -msoft-float,-mcpu=powerpc)
> > >
> > > which for e500 kernel builds user either -mcpu=8540 or -mcpu=powerpc
> > > (probably as a fallback if -mcpu=8540 is not supported).
> > 
> > The -mcpu=powerpc fallback can probably be skipped here, that must have been
> > for compilers predating the addition of -mcpu=8540, and even the oldest ones
> > support that now.
> 
> Ok, makes sense.
> 
> > > So for me it looks like that problematic code
> > >
> > >   KBUILD_CFLAGS         += -mcpu=powerpc
> > >   KBUILD_AFLAGS         += -mcpu=powerpc
> > >
> > > needs to be somehow skipped when compiling for CONFIG_E500.
> > >> My change which skips that code base on ifndef CONFIG_E500 should be
> > > fine as when CONFIG_E500 is disabled it does nothing and when it is
> > > enabled then code
> > >
> > >   CFLAGS-$(CONFIG_E500) += $(call cc-option,-mcpu=8540 -msoft-float,-mcpu=powerpc)
> > >
> > > is called which sets -mcpu option suitable for e500.
> > 
> > I think this part is indeed fishy, but adding another special case for E500
> > seems to take it in the wrong direction.
> > 
> > Nick added this in 4bf4f42a2feb ("powerpc/kbuild: Set default generic
> > machine type
> > for 32-bit compile") as a compile-time fix to prevent the default target from
> > getting used when the compiler supports both 64-bit and 32-bit. This is the
> > right idea, but it's inconsistent to pass different flags depending on the type
> > of toolchain, and it loses the more specific options.
> > 
> > Another problem I see is that a kernel that is built for both E500 and E500MC
> > uses -mcpu=e500mc and may not actually work on the older ones either
> > (even with your patch).
> 
> That is probably truth, -mcpu=8540 should have been chosen. (Anyway it
> should have been called -mcpu=e500, no idea why gcc still name it 8540.)
> 
> > I think what you actually want is to set one option for each of the
> > possible CPU types:
> > 
> > CFLAGS_CPU-$(CONFIG_PPC_BOOK3S_32) := -mcpu=powerpc
> > CFLAGS_CPU-$(CONFIG_PPC_85xx) := -mcpu=8540
> > CFLAGS_CPU-$(CONFIG_PPC8xx) := -mcpu=860
> > CFLAGS_CPU-$(CONFIG_PPC44x) := -mcpu=440
> > CFLAGS_CPU-$(CONFIG_PPC40x) := -mcpu=405
> > ifdef CONFIG_CPU_LITTLE_ENDIAN
> > CFLAGS_CPU-$(CONFIG_BOOK3S_64) := -mcpu=power8
> > else
> > CFLAGS_CPU-$(CONFIG_BOOK3S_64) := -mcpu=power5
> > endif
> > CFLAGS_CPU-$(CONFIG_BOOK3E_64) := -mcpu=powerpc64
> 
> Yes, this is something I would expect that in Makefile should be.

So what about this change?

diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index a0cd70712061..74a608b5796a 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -15,22 +15,7 @@ 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
+CFLAGS-$(CONFIG_PPC_BOOK3S_32) += -mcpu=powerpc
 
 # 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.
@@ -163,17 +148,14 @@ CFLAGS-$(CONFIG_PPC32)	+= $(call cc-option, $(MULTIPLEWORD))
 
 CFLAGS-$(CONFIG_PPC32)	+= $(call cc-option,-mno-readonly-in-sdata)
 
-ifdef CONFIG_PPC_BOOK3S_64
 ifdef CONFIG_CPU_LITTLE_ENDIAN
-CFLAGS-$(CONFIG_GENERIC_CPU) += -mcpu=power8
-CFLAGS-$(CONFIG_GENERIC_CPU) += $(call cc-option,-mtune=power9,-mtune=power8)
+CFLAGS-$(CONFIG_PPC_BOOK3S_64) += -mcpu=power8
+CFLAGS-$(CONFIG_PPC_BOOK3S_64) += $(call cc-option,-mtune=power9,-mtune=power8)
 else
-CFLAGS-$(CONFIG_GENERIC_CPU) += $(call cc-option,-mtune=power7,$(call cc-option,-mtune=power5))
-CFLAGS-$(CONFIG_GENERIC_CPU) += $(call cc-option,-mcpu=power5,-mcpu=power4)
-endif
-else ifdef CONFIG_PPC_BOOK3E_64
-CFLAGS-$(CONFIG_GENERIC_CPU) += -mcpu=powerpc64
+CFLAGS-$(CONFIG_PPC_BOOK3S_64) += $(call cc-option,-mtune=power7,$(call cc-option,-mtune=power5))
+CFLAGS-$(CONFIG_PPC_BOOK3S_64) += $(call cc-option,-mcpu=power5,-mcpu=power4)
 endif
+CFLAGS-$(CONFIG_PPC_BOOK3E_64) += -mcpu=powerpc64
 
 ifdef CONFIG_FUNCTION_TRACER
 CC_FLAGS_FTRACE := -pg
@@ -193,13 +175,8 @@ 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_PPC_E500MC) += $(call cc-option,-mcpu=e500mc,-mcpu=powerpc)
 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)
 


> But what to do with fallback value?
> 
> > For the non-generic CPU types, there is also CONFIG_TARGET_CPU,
> > and the list above could just get folded into that instead.
> > 
> >        Arnd