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

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

[Michael Ellerman]

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

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.

Reported-by: Daniel Axtens <dja@axtens.net>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/purgatory/trampoline_64.S | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/powerpc/purgatory/trampoline_64.S b/arch/powerpc/purgatory/trampoline_64.S
index d956b8a35fd1..b35837c13852 100644
--- a/arch/powerpc/purgatory/trampoline_64.S
+++ b/arch/powerpc/purgatory/trampoline_64.S
@@ -12,7 +12,6 @@
 #include <asm/asm-compat.h>
 #include <asm/crashdump-ppc64.h>
 
-	.machine ppc64
 	.balign 256
 	.globl purgatory_start
 purgatory_start:
-- 
2.25.1

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

[Segher Boessenkool]

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?

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.

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.

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

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


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


Segher

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

[Michael Ellerman]

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

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

[Michael Ellerman]

On Mon, 15 Mar 2021 14:41:59 +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".
> 
> [...]

Applied to powerpc/next.

[1/1] powerpc/kexec: Don't use .machine ppc64 in trampoline_64.S
      https://git.kernel.org/powerpc/c/acd4dfeb49c8ec1071b1e67683c5779e97fdc5b9

cheers