| Message ID | 20200325114110.23491-1-broonie@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | arm64: Open code .arch_extension | expand |
On Wed, 25 Mar 2020 at 12:41, Mark Brown <broonie@kernel.org> wrote: > > Currently several assembler files override the default architecture to > enable extensions in order to allow them to implement optimised routines > for systems with those extensions. Since inserting BTI landing pads into > assembler functions will require us to change the default architecture we > need a way to enable extensions without hard coding the architecture. > The assembler has the .arch_extension feature but this was introduced > for arm64 in gas 2.26 which is too modern for us to rely on it. > > We could just update the base architecture used by these assembler files > but this would mean the assembler would no longer catch attempts to use > newer instructions so instead introduce a macro which sets the default > architecture centrally. Doing this will also make our use of .arch and > .cpu to select the base architecture more consistent. > > Mark Brown (3): > arm64: asm: Provide macro to control enabling architecture extensions > arm64: lib: Use ARM64_EXTENSIONS() > arm64: crypto: Use ARM64_EXTENSIONS() > Hi Mark, I don't think this is the right fix. What is wrong with keeping these .cpu and .arch directives in the .S files, and simply make SYM_FUNC_START() expand to something that includes .arch_extension pac or .arch_extension bti when needed? That way, we only use .arch_extension when we know the assembler supports it (given that .arch_extension support itself should predate BTI or PAC support in GAS or Clang) > arch/arm64/crypto/aes-ce-ccm-core.S | 3 ++- > arch/arm64/crypto/aes-ce-core.S | 2 +- > arch/arm64/crypto/aes-ce.S | 2 +- > arch/arm64/crypto/crct10dif-ce-core.S | 3 ++- > arch/arm64/crypto/ghash-ce-core.S | 3 ++- > arch/arm64/crypto/sha1-ce-core.S | 3 ++- > arch/arm64/crypto/sha2-ce-core.S | 3 ++- > arch/arm64/include/asm/linkage.h | 6 ++++++ > arch/arm64/lib/crc32.S | 2 +- > 9 files changed, 19 insertions(+), 8 deletions(-) > > -- > 2.20.1 >
On Wed, Mar 25, 2020 at 12:45:11PM +0100, Ard Biesheuvel wrote: > I don't think this is the right fix. What is wrong with keeping these > .cpu and .arch directives in the .S files, and simply make > SYM_FUNC_START() expand to something that includes .arch_extension pac > or .arch_extension bti when needed? That way, we only use > .arch_extension when we know the assembler supports it (given that > .arch_extension support itself should predate BTI or PAC support in > GAS or Clang) Since BTI is a mandatory feature of v8.5 there is no BTI arch_extension, you can only enable it by moving the base architecture to v8.5. You'd need to use .arch and that feels likely to find us sharp edges to run into.
On Wed, 25 Mar 2020 at 12:50, Mark Brown <broonie@kernel.org> wrote: > > On Wed, Mar 25, 2020 at 12:45:11PM +0100, Ard Biesheuvel wrote: > > > I don't think this is the right fix. What is wrong with keeping these > > .cpu and .arch directives in the .S files, and simply make > > SYM_FUNC_START() expand to something that includes .arch_extension pac > > or .arch_extension bti when needed? That way, we only use > > .arch_extension when we know the assembler supports it (given that > > .arch_extension support itself should predate BTI or PAC support in > > GAS or Clang) > > Since BTI is a mandatory feature of v8.5 there is no BTI arch_extension, > you can only enable it by moving the base architecture to v8.5. You'd > need to use .arch and that feels likely to find us sharp edges to run > into. I think we should talk to the toolchain folks about this. Even if .arch_extension today does not support the 'bti' argument, it *is* most definitely an architecture extension, even it it is mandatory in v8.5 (given that v8.5 is itself an architecture extension).
On Wed, Mar 25, 2020 at 12:54:10PM +0100, Ard Biesheuvel wrote: > On Wed, 25 Mar 2020 at 12:50, Mark Brown <broonie@kernel.org> wrote: > > Since BTI is a mandatory feature of v8.5 there is no BTI arch_extension, > > you can only enable it by moving the base architecture to v8.5. You'd > > need to use .arch and that feels likely to find us sharp edges to run > > into. > I think we should talk to the toolchain folks about this. Even if > .arch_extension today does not support the 'bti' argument, it *is* > most definitely an architecture extension, even it it is mandatory in > v8.5 (given that v8.5 is itself an architecture extension). I agree entirely, the current behaviour is surprising and doesn't really map onto how the architecture is described - my first thought was similar to yours. It won't help us right now but it would help for future architecture extensions and for other projects.
On Wed, Mar 25, 2020 at 12:54:10PM +0100, Ard Biesheuvel wrote: > On Wed, 25 Mar 2020 at 12:50, Mark Brown <broonie@kernel.org> wrote: > > Since BTI is a mandatory feature of v8.5 there is no BTI arch_extension, > > you can only enable it by moving the base architecture to v8.5. You'd > > need to use .arch and that feels likely to find us sharp edges to run > > into. > > I think we should talk to the toolchain folks about this. Even if > .arch_extension today does not support the 'bti' argument, it *is* > most definitely an architecture extension, even it it is mandatory in > v8.5 (given that v8.5 is itself an architecture extension). It certianly seems unfortunate, as it goes against the premise of having HINT space instructions. Most software will want to enable HINT space instructions from ARMv8.x but nothing else to ensure binary compatibility with existing hardware. I see the same is true for pointer authentication judging by: https://sourceware.org/binutils/docs/as/AArch64-Extensions.html#AArch64-Extensions ... so worth raising with toolchain folk as a general principle even if we have to bodge around it for now. Thanks, Mark.
On 2020-03-25 12:03 pm, Mark Rutland wrote: > On Wed, Mar 25, 2020 at 12:54:10PM +0100, Ard Biesheuvel wrote: >> On Wed, 25 Mar 2020 at 12:50, Mark Brown <broonie@kernel.org> wrote: >>> Since BTI is a mandatory feature of v8.5 there is no BTI arch_extension, >>> you can only enable it by moving the base architecture to v8.5. You'd >>> need to use .arch and that feels likely to find us sharp edges to run >>> into. >> >> I think we should talk to the toolchain folks about this. Even if >> .arch_extension today does not support the 'bti' argument, it *is* >> most definitely an architecture extension, even it it is mandatory in >> v8.5 (given that v8.5 is itself an architecture extension). > > It certianly seems unfortunate, as it goes against the premise of having > HINT space instructions. Most software will want to enable HINT space > instructions from ARMv8.x but nothing else to ensure binary > compatibility with existing hardware. > > I see the same is true for pointer authentication judging by: > > https://sourceware.org/binutils/docs/as/AArch64-Extensions.html#AArch64-Extensions > > ... so worth raising with toolchain folk as a general principle even if > we have to bodge around it for now. Indeed, in general, just because a feature is mandatory in v8.n doesn't necessarily mean it can't be included in a v8.(n-1) implementation which may not have the *other* mandatory parts of v8.n which ".arch armv8.<n>-a" would let through. Robin.
On Wed, Mar 25, 2020 at 11:41:07AM +0000, Mark Brown wrote: > Since inserting BTI landing pads into assembler functions will require > us to change the default architecture we need a way to enable > extensions without hard coding the architecture. Assuming we'll poke the toolchain folk, let's consider alternative ways around this in the mean time. Is there anything akin to push/pop versions of .arch directitves that we can use around the BTI instructions specifically? ... or could we encode the BTI instructions with a .inst, and wrap those in macros so that GAS won't complain (like we do for mrs_s and friends)? ... does asking GCC to use BTI for C code make the default arch v8.5 for inline asm, or does it do something special to allow BTI instructions in specific locations? Thanks, Mark.
, On Wed, 25 Mar 2020 at 13:32, Mark Rutland <mark.rutland@arm.com> wrote: > > On Wed, Mar 25, 2020 at 11:41:07AM +0000, Mark Brown wrote: > > Since inserting BTI landing pads into assembler functions will require > > us to change the default architecture we need a way to enable > > extensions without hard coding the architecture. > > Assuming we'll poke the toolchain folk, let's consider alternative ways > around this in the mean time. > > Is there anything akin to push/pop versions of .arch directitves that we > can use around the BTI instructions specifically? > > ... or could we encode the BTI instructions with a .inst, and wrap those > in macros so that GAS won't complain (like we do for mrs_s and friends)? > > ... does asking GCC to use BTI for C code make the default arch v8.5 for > inline asm, or does it do something special to allow BTI instructions in > specific locations? > I think using macros wrapping .inst directives is the most hassle free way to achieve this, assuming there is no need to encode registers or immediates (which makes it slightly messy - refer to arch/arm64/crypto/sm3-ce-core.S for an example)
On Wed, Mar 25, 2020 at 12:31:59PM +0000, Mark Rutland wrote: > Is there anything akin to push/pop versions of .arch directitves that we > can use around the BTI instructions specifically? Not that I can see. > ... or could we encode the BTI instructions with a .inst, and wrap those > in macros so that GAS won't complain (like we do for mrs_s and friends)? That should work, I think it's a taste thing which is better. For me it was a combination of there being a small number of files that were affected, the fact that even within that small set there was divergence in how they were doing this and the fact that neither solution is a thing of great beauty. > ... does asking GCC to use BTI for C code make the default arch v8.5 for > inline asm, or does it do something special to allow BTI instructions in > specific locations? This is only an issue for freestanding assembly files as far as I can see.
On Wed, Mar 25, 2020 at 02:26:49PM +0100, Ard Biesheuvel wrote: > I think using macros wrapping .inst directives is the most hassle free > way to achieve this, assuming there is no need to encode registers or > immediates (which makes it slightly messy - refer to > arch/arm64/crypto/sm3-ce-core.S for an example) There isn't - you just have to encode the four target classes, of which we only use one.
On Wed, Mar 25, 2020 at 11:50:38AM +0000, Mark Brown wrote: > On Wed, Mar 25, 2020 at 12:45:11PM +0100, Ard Biesheuvel wrote: > > I don't think this is the right fix. What is wrong with keeping these > > .cpu and .arch directives in the .S files, and simply make > > SYM_FUNC_START() expand to something that includes .arch_extension pac > > or .arch_extension bti when needed? That way, we only use > > .arch_extension when we know the assembler supports it (given that > > .arch_extension support itself should predate BTI or PAC support in > > GAS or Clang) > > Since BTI is a mandatory feature of v8.5 there is no BTI arch_extension, > you can only enable it by moving the base architecture to v8.5. You'd > need to use .arch and that feels likely to find us sharp edges to run > into. For MTE, .arch armv8-a+memtag won't work since this is only available with armv8.5-a. My preference would be to have the highest arch version supported by the kernel in the assembler.h file, i.e. ".arch armv8.5-a" followed by .arch_extension in each .S file, as needed. Forcing .S files to armv8.5 would not cause any problems with the base armv8.0 that the kernel image support since it shouldn't change the opcodes gas generates. The .S files would use alternatives anyway (or simply have code not called). The inline asm is slightly more problematic, especially with the clang builtin assembler which goes in a single pass. But we could do something similar to what we did with the LSE atomics and raising the base of the inline asm to armv8.5 (or 8.6 etc., whatever we need in the future).
On Wed, Apr 22, 2020 at 07:00:28PM +0100, Catalin Marinas wrote: > On Wed, Mar 25, 2020 at 11:50:38AM +0000, Mark Brown wrote: > > Since BTI is a mandatory feature of v8.5 there is no BTI arch_extension, > > you can only enable it by moving the base architecture to v8.5. You'd > > need to use .arch and that feels likely to find us sharp edges to run > > into. > For MTE, .arch armv8-a+memtag won't work since this is only available > with armv8.5-a. My preference would be to have the highest arch version > supported by the kernel in the assembler.h file, i.e. ".arch armv8.5-a" > followed by .arch_extension in each .S file, as needed. I think we decided that .arch_extension was too new to be used for things like the crypto stuff where we still support older toolchains? > Forcing .S files to armv8.5 would not cause any problems with > the base armv8.0 that the kernel image support since it shouldn't change > the opcodes gas generates. The .S files would use alternatives anyway > (or simply have code not called). We do loose the checking that the assembler does that nobody used a newer feature by mistake but yeah, shouldn't affect the output. > The inline asm is slightly more problematic, especially with the clang > builtin assembler which goes in a single pass. But we could do something > similar to what we did with the LSE atomics and raising the base of the > inline asm to armv8.5 (or 8.6 etc., whatever we need in the future). FWIW I did something different to this for BTI so I wasn't using the instructions directly so I was going to abandon this series.
On Thu, Apr 23, 2020 at 12:18:03PM +0100, Mark Brown wrote: > On Wed, Apr 22, 2020 at 07:00:28PM +0100, Catalin Marinas wrote: > > On Wed, Mar 25, 2020 at 11:50:38AM +0000, Mark Brown wrote: > > > Since BTI is a mandatory feature of v8.5 there is no BTI arch_extension, > > > you can only enable it by moving the base architecture to v8.5. You'd > > > need to use .arch and that feels likely to find us sharp edges to run > > > into. > > > For MTE, .arch armv8-a+memtag won't work since this is only available > > with armv8.5-a. My preference would be to have the highest arch version > > supported by the kernel in the assembler.h file, i.e. ".arch armv8.5-a" > > followed by .arch_extension in each .S file, as needed. > > I think we decided that .arch_extension was too new to be used for > things like the crypto stuff where we still support older toolchains? .arch_extension would be issued conditionally only for features like CONFIG_ARM64_MTE which already have a dependency on a newer toolchain. However, '.arch_extension memtag' is not sufficient for MTE, it needs a prior '.arch armv8.5-a'. > > Forcing .S files to armv8.5 would not cause any problems with > > the base armv8.0 that the kernel image support since it shouldn't change > > the opcodes gas generates. The .S files would use alternatives anyway > > (or simply have code not called). > > We do loose the checking that the assembler does that nobody used a > newer feature by mistake but yeah, shouldn't affect the output. We may need some push/pop_arch macros to contain the supported features. The gas documentation says that .arch_extension may be used multiple times to add or remove extensions. However, I couldn't find a way to remove memtag after adding it (tried -memtag, !memtag, empty string). So I may go with a '.arch armv8.0-a' as a base, followed by temporary setting of '.arch armv8.5-a+memtag' (and hope we don't need combinations of such extensions). > > The inline asm is slightly more problematic, especially with the clang > > builtin assembler which goes in a single pass. But we could do something > > similar to what we did with the LSE atomics and raising the base of the > > inline asm to armv8.5 (or 8.6 etc., whatever we need in the future). > > FWIW I did something different to this for BTI so I wasn't using the > instructions directly so I was going to abandon this series. I can't work around this easily for MTE, there are more instructions with register encoding. I'll see if the push/pop idea works or just leave it to whoever does the next feature, figure out how it interacts with MTE ;).
On Thu, Apr 23, 2020 at 12:59:05PM +0100, Catalin Marinas wrote: > On Thu, Apr 23, 2020 at 12:18:03PM +0100, Mark Brown wrote: > > On Wed, Apr 22, 2020 at 07:00:28PM +0100, Catalin Marinas wrote: > > > Forcing .S files to armv8.5 would not cause any problems with > > > the base armv8.0 that the kernel image support since it shouldn't change > > > the opcodes gas generates. The .S files would use alternatives anyway > > > (or simply have code not called). > > > > We do loose the checking that the assembler does that nobody used a > > newer feature by mistake but yeah, shouldn't affect the output. > > We may need some push/pop_arch macros to contain the supported features. > > The gas documentation says that .arch_extension may be used multiple > times to add or remove extensions. However, I couldn't find a way to > remove memtag after adding it (tried -memtag, !memtag, empty string). So > I may go with a '.arch armv8.0-a' as a base, followed by temporary > setting of '.arch armv8.5-a+memtag' (and hope we don't need combinations > of such extensions). Quick attempt at this on top of the MTE patches: diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h index e7338e129dfd..6180ac605406 100644 --- a/arch/arm64/include/asm/assembler.h +++ b/arch/arm64/include/asm/assembler.h @@ -24,10 +24,18 @@ #include <asm/sysreg.h> #include <asm/thread_info.h> -#ifdef CONFIG_ARM64_MTE - .arch armv8.5-a - .arch_extension memtag -#endif + /* Base architecture version for the .S files */ + .arch armv8-a + + .macro arm64_set_arch, arch, enable = 1 + .if \enable + .arch \arch + .endif + .endm + + .macro arm64_reset_arch + .arch armv8-a + .endm .macro save_and_disable_daif, flags mrs \flags, daif diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S index e4ab82e543cf..df2037fc431b 100644 --- a/arch/arm64/kernel/entry.S +++ b/arch/arm64/kernel/entry.S @@ -148,6 +148,7 @@ alternative_cb_end /* Check for MTE asynchronous tag check faults */ .macro check_mte_async_tcf, flgs, tmp #ifdef CONFIG_ARM64_MTE + arm64_set_arch armv8.5-a+memtag alternative_if_not ARM64_MTE b 1f alternative_else_nop_endif @@ -158,16 +159,19 @@ alternative_else_nop_endif str \flgs, [tsk, #TSK_TI_FLAGS] msr_s SYS_TFSRE0_EL1, xzr 1: + arm64_reset_arch #endif .endm /* Clear the MTE asynchronous tag check faults */ .macro clear_mte_async_tcf #ifdef CONFIG_ARM64_MTE + arm64_set_arch armv8.5-a+memtag alternative_if ARM64_MTE dsb ish msr_s SYS_TFSRE0_EL1, xzr alternative_else_nop_endif + arm64_reset_arch #endif .endm diff --git a/arch/arm64/lib/clear_page.S b/arch/arm64/lib/clear_page.S index 9f85a4cf9568..a8e26f232502 100644 --- a/arch/arm64/lib/clear_page.S +++ b/arch/arm64/lib/clear_page.S @@ -22,8 +22,10 @@ SYM_FUNC_START(clear_page) mov x2, #4 lsl x1, x2, x1 1: + arm64_set_arch armv8.5-a+memtag, IS_ENABLED(CONFIG_ARM64_MTE) alternative_insn "dc zva, x0", "stzgm xzr, [x0]", \ ARM64_MTE, IS_ENABLED(CONFIG_ARM64_MTE), 1 + arm64_reset_arch add x0, x0, x1 tst x0, #(PAGE_SIZE - 1) b.ne 1b diff --git a/arch/arm64/lib/copy_page.S b/arch/arm64/lib/copy_page.S index c3234175efe0..8322043e75e6 100644 --- a/arch/arm64/lib/copy_page.S +++ b/arch/arm64/lib/copy_page.S @@ -26,6 +26,7 @@ alternative_if ARM64_HAS_NO_HW_PREFETCH alternative_else_nop_endif #ifdef CONFIG_ARM64_MTE + arm64_set_arch armv8.5-a+memtag alternative_if_not ARM64_MTE b 2f alternative_else_nop_endif @@ -46,6 +47,7 @@ alternative_else_nop_endif tst x2, #(PAGE_SIZE - 1) b.ne 1b 2: + arm64_reset_arch #endif ldp x2, x3, [x1] diff --git a/arch/arm64/lib/mte.S b/arch/arm64/lib/mte.S index 45be04a8c73c..8f824fc62ad4 100644 --- a/arch/arm64/lib/mte.S +++ b/arch/arm64/lib/mte.S @@ -7,6 +7,8 @@ #include <asm/assembler.h> #include <asm/mte.h> + arm64_set_arch armv8.5-a+memtag + /* * Compare tags of two pages * x0 - page1 address