| Message ID | 20200323191807.3864-1-broonie@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] arm64: lib: Use .arch_extension | expand |
On Mon, Mar 23, 2020 at 07:18:06PM +0000, Mark Brown wrote: > Currently when implementing optimised assembler routines using > architecture extensions we override the base architecture along with > enabling the new extensions, causing problems for in kernel BTI support > which needs to raise the base architecture level for assembler files in > order to generate BTI landing pads. We did this due to a lack of > support for the .arch_extension gas feature in older versions of the > clang built in assembler but since current versions of clang now have > support for .arch_extension we can use that. > > Signed-off-by: Mark Brown <broonie@kernel.org> I'll queue this through the arm64 tree. Thanks.
On Mon, 23 Mar 2020 at 20:18, Mark Brown <broonie@kernel.org> wrote: > > Currently when implementing optimised assembler routines using > architecture extensions we override the base architecture along with > enabling the new extensions, causing problems for in kernel BTI support > which needs to raise the base architecture level for assembler files in > order to generate BTI landing pads. We did this due to a lack of > support for the .arch_extension gas feature in older versions of the > clang built in assembler but since current versions of clang now have > support for .arch_extension we can use that. This is not 100% accurate. Support for .arch_extension was added to GAS/aarch64 much later, so we should at least double check that it works on the oldest binutils that we do support. > > Signed-off-by: Mark Brown <broonie@kernel.org> > --- > arch/arm64/lib/crc32.S | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/lib/crc32.S b/arch/arm64/lib/crc32.S > index 243e107e9896..7420dea6afc1 100644 > --- a/arch/arm64/lib/crc32.S > +++ b/arch/arm64/lib/crc32.S > @@ -9,7 +9,7 @@ > #include <asm/alternative.h> > #include <asm/assembler.h> > > - .cpu generic+crc > + .arch_extension crc > > .macro __crc32, c > cmp x2, #16 > -- > 2.20.1 >
On Tue, Mar 24, 2020 at 07:19:17PM +0100, Ard Biesheuvel wrote: > On Mon, 23 Mar 2020 at 20:18, Mark Brown <broonie@kernel.org> wrote: > > order to generate BTI landing pads. We did this due to a lack of > > support for the .arch_extension gas feature in older versions of the > > clang built in assembler but since current versions of clang now have > > support for .arch_extension we can use that. > This is not 100% accurate. Support for .arch_extension was added to > GAS/aarch64 much later, so we should at least double check that it > works on the oldest binutils that we do support. Ah, OK - the information I figured out from history was misleading sorry. We've already got quite a lot of usages of .arch_extension in the kernel for arm and a couple for arm64 (one in TF and another for LSE). It looks like the feature was added in binutils 2.26 which is newer than the current advertised minimum binutils version of 2.21 but dates from 2016 (the code was added in 2014 and looks like it might've appeared in downstream releases earlier) so is pretty old in arm64 terms. If that's not OK I've got an open coded version for BTI that does this with macros but it's obviously not as nice. I do wish the binutils docs included information on when features were added, it'd make life easier :/
On Tue, 24 Mar 2020 at 19:58, Mark Brown <broonie@kernel.org> wrote: > > On Tue, Mar 24, 2020 at 07:19:17PM +0100, Ard Biesheuvel wrote: > > On Mon, 23 Mar 2020 at 20:18, Mark Brown <broonie@kernel.org> wrote: > > > > order to generate BTI landing pads. We did this due to a lack of > > > support for the .arch_extension gas feature in older versions of the > > > clang built in assembler but since current versions of clang now have > > > support for .arch_extension we can use that. > > > This is not 100% accurate. Support for .arch_extension was added to > > GAS/aarch64 much later, so we should at least double check that it > > works on the oldest binutils that we do support. > > Ah, OK - the information I figured out from history was misleading > sorry. > > We've already got quite a lot of usages of .arch_extension in the kernel > for arm and a couple for arm64 (one in TF and another for LSE). ARM had support for this way before it was added to AArch64 as well. In the LSE case, it doesn't matter since LSE itself was not implemented by the assembler before that. For other things, we should really avoid .arch_extension as long as we still support binutils < 2.26 > It > looks like the feature was added in binutils 2.26 which is newer than > the current advertised minimum binutils version of 2.21 but dates from > 2016 (the code was added in 2014 and looks like it might've appeared in > downstream releases earlier) so is pretty old in arm64 terms. If that's > not OK I've got an open coded version for BTI that does this with macros > but it's obviously not as nice. > > I do wish the binutils docs included information on when features were > added, it'd make life easier :/
On Tue, Mar 24, 2020 at 11:17:05PM +0100, Ard Biesheuvel wrote: > On Tue, 24 Mar 2020 at 19:58, Mark Brown <broonie@kernel.org> wrote: > > > > On Tue, Mar 24, 2020 at 07:19:17PM +0100, Ard Biesheuvel wrote: > > > On Mon, 23 Mar 2020 at 20:18, Mark Brown <broonie@kernel.org> wrote: > > > > > > order to generate BTI landing pads. We did this due to a lack of > > > > support for the .arch_extension gas feature in older versions of the > > > > clang built in assembler but since current versions of clang now have > > > > support for .arch_extension we can use that. > > > > > This is not 100% accurate. Support for .arch_extension was added to > > > GAS/aarch64 much later, so we should at least double check that it > > > works on the oldest binutils that we do support. > > > > Ah, OK - the information I figured out from history was misleading > > sorry. > > > > We've already got quite a lot of usages of .arch_extension in the kernel > > for arm and a couple for arm64 (one in TF and another for LSE). > > ARM had support for this way before it was added to AArch64 as well. > In the LSE case, it doesn't matter since LSE itself was not > implemented by the assembler before that. For other things, we should > really avoid .arch_extension as long as we still support binutils < > 2.26 Good point. So we can add .arch_extension for new features which are already dependent on newer gas options. I'll drop this patch for now but we should try to find a solution because of the overriding effect of .arch. In self-contained .S files (like crypto) that's not an issue but once we start adding more architecture feature in a single file, things will get more complicated.
On Wed, Mar 25, 2020 at 11:05:04AM +0000, Catalin Marinas wrote: > I'll drop this patch for now but we should try to find a solution > because of the overriding effect of .arch. In self-contained .S files > (like crypto) that's not an issue but once we start adding more > architecture feature in a single file, things will get more complicated. It's fine I think, you can open code .arch_extension in the assembler.
diff --git a/arch/arm64/lib/crc32.S b/arch/arm64/lib/crc32.S index 243e107e9896..7420dea6afc1 100644 --- a/arch/arm64/lib/crc32.S +++ b/arch/arm64/lib/crc32.S @@ -9,7 +9,7 @@ #include <asm/alternative.h> #include <asm/assembler.h> - .cpu generic+crc + .arch_extension crc .macro __crc32, c cmp x2, #16
Currently when implementing optimised assembler routines using architecture extensions we override the base architecture along with enabling the new extensions, causing problems for in kernel BTI support which needs to raise the base architecture level for assembler files in order to generate BTI landing pads. We did this due to a lack of support for the .arch_extension gas feature in older versions of the clang built in assembler but since current versions of clang now have support for .arch_extension we can use that. Signed-off-by: Mark Brown <broonie@kernel.org> --- arch/arm64/lib/crc32.S | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)