| Message ID | 20210315034159.315675-1-mpe@ellerman.id.au (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | powerpc/kexec: Don't use .machine ppc64 in trampoline_64.S | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (91966823812efbd175f904599e5cf2a854b39809) |
| snowpatch_ozlabs/build-ppc64le | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64be | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64e | success | Build succeeded |
| snowpatch_ozlabs/build-pmac32 | success | Build succeeded |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 7 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
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
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
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
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:
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(-)