Re: [PATCH] powerpc/kexec: Don't use .machine ppc64 in trampoline_64.S

[Michael Ellerman <mpe@ellerman.id.au>]

Segher Boessenkool <segher@kernel.crashing.org> writes:
> Hi!
>
> On Mon, Mar 15, 2021 at 02:41:59PM +1100, Michael Ellerman wrote:
>> The ".machine" directive allows changing the machine for which code is
>> being generated. It's equivalent to passing an -mcpu option on the
>> command line.
>> 
>> Although it can be useful, it's generally a bad idea because it adds
>> another way to influence code generation separate from the flags
>> passed via the build system. ie. if we need to build different pieces
>> of code with different flags we should do that via our Makefiles, not
>> using ".machine".
>
> It does not influence code generation.  It says which instructions are
> valid, instead.  There are a few cases where the same mnemonic will
> generate a different binary encoding depending on machine selected,
> maybe you mean that?

Yeah that's what I was referring to. Which is code generation in my
mind, but I guess that's probably not the right terminology to use
around compiler people :)

And I guess you're right, the more common case is that the mnemonics are
just not valid for other machines and wouldn't assemble at all.

I'll reword it.

> It is *normal* to use .machine push/pop and a specific .machine around
> instructions that require a machine other than what you are building
> for.  The compiler does this itself, and it is the recommended way to
> use "foreign" instructions in inline assembler.

Right, but it also makes it easy to build code that won't run :) So we'd
like to avoid it.

We had that in the past where we were building the power7-only memcpy
routines for Book3E 64. They weren't being used due to runtime patching
stuff, but it's better to not build them in the first place for those
CPUs because they could never work.

> That said...
>
>> However as best as I can tell the ".machine" directive in
>> trampoline_64.S is not necessary at all.
>> 
>> It was added in commit 0d97631392c2 ("powerpc: Add purgatory for
>> kexec_file_load() implementation."), which created the file based on
>> the kexec-tools purgatory. It may be/have-been necessary in the
>> kexec-tools version, but we have a completely different build system,
>> and we already pass the desired CPU flags, eg:
>> 
>>   gcc ... -m64 -Wl,-a64 -mabi=elfv2 -Wa,-maltivec -Wa,-mpower4 -Wa,-many
>>   ... arch/powerpc/purgatory/trampoline_64.S
>> 
>> So drop the ".machine" directive and rely on the assembler flags.
>
>> -	.machine ppc64
>
> Please make sure to test this on a big endian config.

Done.

> A ppc64le-linux assembler defaults to power8.  A ppc64-linux assembler
> defaults to power3 (that is the same as .machine ppc64).  Or maybe it
> makes it power4?  I get lost :-)

For book3s64 we always specify -mpower4 since 15a3204d24a3
("powerpc/64s: Set assembler machine type to POWER4") (Apr 2018).

That does leave 64-bit book3e, but I just tested that and it also builds
fine.

> It certainly *should* work, but, test please :-)
>
> (And with a *default* powerpc64-linux config, not one that defaults to
> power7 or power8 or similar!  Arnd's toolchains at
> <https://mirrors.edge.kernel.org/pub/tools/crosstool/>
> are fine for this.)

Yep, I used Arnd's 10.1.0.

> Reviewed-by: Segher Boessenkool <segher@kernel.crashing.org>

Thanks.

cheers