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

[Arnd Bergmann]

On Mon, Jul 4, 2022 at 12:39 PM Pali Rohár <pali@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > >> Cc: stable@xxxxxxxxxxxxxxx
> > >
> > >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.

> > 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.

> 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).

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

For the non-generic CPU types, there is also CONFIG_TARGET_CPU,
and the list above could just get folded into that instead.

       Arnd