Skip to contentSkip to conversation

lib/crc: arm64: use .arch_extension to enable CRC and crypto

PATCH In review Emanuele Rocca 3 participants 5 messages Nov 5 – Nov 7

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 lib/crc: arm64: use .arch_extension to enable CRC and crypto

Instead of using ".cpu generic+crc+crypto", use ".arch_extension crc" and ".arch_extension crypto" to enable the required instructions.

The ".cpu generic+crc+crypto" directive replaces the assembler's current feature set with generic+crc+crypto, dropping any other features that were previously enabled, for example by -march or by a toolchain configured with --with-arch=armv9-a. In contrast, ".arch_extension" enables the requested extensions while preserving the current feature set.

This is not a functional change: crc32-core.S only uses base A64 instructions plus CRC32 and PMULL, which remain available, and BTI landing pads are emitted as HINT instructions. However, using .arch_extension avoids the pitfall of silently disabling features. See commit 9142c2f8f84b ("octeontx2: use arch_extension to enable LSE").

---
 lib/crc/arm64/crc32-core.S | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
lib/crc/arm64/crc32-core.S +2 -1
index 49d02cc485b3..0877d07ed1bf 100644
@@ -11,7 +11,8 @@
 #include <linux/linkage.h>
 #include <asm/assembler.h>
 
-	.cpu		generic+crc+crypto
+	.arch_extension	crc
+	.arch_extension	crypto
 
 	.macro		bitle, reg
 	.endm
Signature · 1 line
2.55.0
maintainer
#1 lore raw
Quoted 9 lines · Emanuele Rocca

Instead of using ".cpu generic+crc+crypto", use ".arch_extension crc" and ".arch_extension crypto" to enable the required instructions.

The ".cpu generic+crc+crypto" directive replaces the assembler's current feature set with generic+crc+crypto, dropping any other features that were previously enabled, for example by -march or by a toolchain configured with --with-arch=armv9-a. In contrast, ".arch_extension" enables the requested extensions while preserving the current feature set.

Quoted 7 lines

This is not a functional change: crc32-core.S only uses base A64 instructions plus CRC32 and PMULL, which remain available, and BTI landing pads are emitted as HINT instructions. However, using .arch_extension avoids the pitfall of silently disabling features. See commit 9142c2f8f84b ("octeontx2: use arch_extension to enable LSE").

Thanks. Just to confirm, all supported versions of binutils and clang support this? (As per Documentation/process/changes.rst, the kernel currently supports binutils 2.30 and later, and clang 17.0.1 and later.)

A lot of the arm64 crypto code uses ".arch armv8-a+crypto". Should
those be replaced with ".arch_extension crypto" for the same reason?

- Eric

#2 lore raw

Hi Eric,

Quoted 3 lines · Eric Biggers

Thanks. Just to confirm, all supported versions of binutils and clang support this? (As per Documentation/process/changes.rst, the kernel currently supports binutils 2.30 and later, and clang 17.0.1 and later.)

Yes, binutils supports .arch_extension since 2.26:
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=ae527cd876fe35de72d876d3484e194ccc66232f

$ git tag --contains ae527cd876fe35de72d876d3484e194ccc66232f | head -1
binutils-2_26

clang added support in 8.0.0:
https://releases.llvm.org/8.0.0/docs/ReleaseNotes.html#changes-to-the-aarch64-target

Quoted 2 lines

A lot of the arm64 crypto code uses ".arch armv8-a+crypto". Should
those be replaced with ".arch_extension crypto" for the same reason?

I think so, yes. Let me look more closely, I'll send another series for those if they need changing as well.

Thanks!
Emanuele

#3 lore raw
In reply to Emanuele Rocca

Hello Emanuele,

Quoted 9 lines · Emanuele Rocca

Instead of using ".cpu generic+crc+crypto", use ".arch_extension crc" and ".arch_extension crypto" to enable the required instructions.

The ".cpu generic+crc+crypto" directive replaces the assembler's current feature set with generic+crc+crypto, dropping any other features that were previously enabled, for example by -march or by a toolchain configured with --with-arch=armv9-a. In contrast, ".arch_extension" enables the requested extensions while preserving the current feature set.

Quoted 6 lines

This is not a functional change: crc32-core.S only uses base A64 instructions plus CRC32 and PMULL, which remain available, and BTI landing pads are emitted as HINT instructions. However, using .arch_extension avoids the pitfall of silently disabling features. See commit 9142c2f8f84b ("octeontx2: use arch_extension to enable LSE").

I don't mind the patch, but I will point out that issues such as these can never occur in .S sources. The emitted instructions are always the same, and the only reason we need the .arch is so that the assembler permits the use of the special ones.

Commit 9142c2f8f84b is different because it uses .arch in inline asm, where it switches the assembler into a different mode behind the back of the compiler, leading it to reject instructions generated by the compiler.

maintainer
#4 lore raw
Quoted 19 lines · Ard Biesheuvel

Hello Emanuele,

Quoted 9 lines · Emanuele Rocca

Instead of using ".cpu generic+crc+crypto", use ".arch_extension crc" and ".arch_extension crypto" to enable the required instructions.

The ".cpu generic+crc+crypto" directive replaces the assembler's current feature set with generic+crc+crypto, dropping any other features that were previously enabled, for example by -march or by a toolchain configured with --with-arch=armv9-a. In contrast, ".arch_extension" enables the requested extensions while preserving the current feature set.

Quoted 6 lines

This is not a functional change: crc32-core.S only uses base A64 instructions plus CRC32 and PMULL, which remain available, and BTI landing pads are emitted as HINT instructions. However, using .arch_extension avoids the pitfall of silently disabling features. See commit 9142c2f8f84b ("octeontx2: use arch_extension to enable LSE").

Quoted 9 lines

I don't mind the patch, but I will point out that issues such as these can never occur in .S sources. The emitted instructions are always the same, and the only reason we need the .arch is so that the assembler permits the use of the special ones.

Commit 9142c2f8f84b is different because it uses .arch in inline asm, where it switches the assembler into a different mode behind the back of the compiler, leading it to reject instructions generated by the compiler.

Thanks. Emanuele, could you maybe note that in the commit message?

Also, this patch is not complete even for lib/crc/arm64/, since lib/crc/arm64/crc-t10dif-core.S still uses "armv8-a+crypto". If we're bothering with this, I would suggest just taking care of both crc32-core.S and crc-t10dif-core.S in one patch. (The crypto files I mentioned before should be a different patch.)

- Eric