Message ID | 20250723012936.3799471-1-raj.khem@gmail.com |
---|---|
State | New |
Headers | show |
Series | [v3] feature-arm-crypto: Add +nocrypto to -mcpu when crypto not in features | expand |
Hi Khem, On 7/23/25 3:29 AM, Khem Raj wrote: > When crypto is not in tune features then add +nocryto to > -mcpu explicitly. This makes the behavior between clang > and gcc match. Currently -mcpu=cortex-a72 has different > behavior in clang and gcc in terms of what features are > considered default. Clang enables different set of common > features than gcc on other hand. For example clang > enables crypto with default set but gcc > does not, gcc recommends to disable unavailable extentions s/extentions/extensions/ > in -mcpu [1] explicitly. crypto is optional on cortext-a53 s/cortext/cortex/ I am not disputing this patch is needed but there's just conflicting information (to me). This says """ For example clang enables crypto with default set but gcc does not, gcc recommends to disable unavailable extentions in -mcpu explicitly """ which can be summed up to "gcc does not enable crypto by default, but recommends disabling it when unsupported", doesn't that mean it's enabled by default? And I finally managed to understand what bothered me. -mcpu is the one which enables extensions by default it seems? But the variable (TUNE_CCARGS_MARCH_OPTS) here is for -march, not -mcpu! And -march doesn't specify what it enables by default in the docs it seems like, see https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html#index-march-2 vs https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html#index-mcpu-2 So we need to modify the commit log to reflect that and not be misleading. While at it, I'm wondering if we shouldn't simply do that for all TUNE_CCARGS_MARCH_OPTS that we have today? I see: +crc, +fp, +simd, +dsp, +idiv as well that have a +no variant. +sve and +sve2 though, couldn't find them in the official docs of GCC (though https://community.arm.com/arm-community-blogs/b/tools-software-ides-blog/posts/getting-ready-for-sve-with-gcc-11 and https://github.com/gcc-mirror/gcc/blob/master/gcc/config/aarch64/aarch64-option-extensions.def define both, so I guess it's fine too). We probably should report or fix this in the GCC docs? > and cortex-a72 while, this is not as common but Broadcom "while, " is a bit odd here :) > SOCs in raspberrypi3/4 have dropped crypto for cost > reasons [2]. This results in illegal instruction > traps [3] [4] when building components e.g. chromium, > qtwebengine, weston etc. with clang using -mcpu=cortex-a72 > for rpi4 target. > > Adding +nocrypto makes it behave like gcc does today. We s/it/clang/ > do have separate tune if crypto enabled cortex-a72 cores > are to be targeted (cortexa72-cryto) as DEFAULTTUNE > > They are added to default feature file since crypto extension > is available in multiple arm architecture versions e.g. armv8, > armv9. It is optional extention as per spec [5] s/extention/extension/ Cheers, Quentin
diff --git a/meta/conf/machine/include/arm/feature-arm-crypto.inc b/meta/conf/machine/include/arm/feature-arm-crypto.inc index aade6ce08d5..8202de78e46 100644 --- a/meta/conf/machine/include/arm/feature-arm-crypto.inc +++ b/meta/conf/machine/include/arm/feature-arm-crypto.inc @@ -2,4 +2,5 @@ # armv8-a, armv8.1-a, armv8.3-a, armv8.4-a, armv8.5-a, armv8.6-a, and armv8-r TUNEVALID[crypto] = "Enable cryptographic instructions for ARMv8" -TUNE_CCARGS_MARCH_OPTS .= "${@bb.utils.contains('TUNE_FEATURES', 'crypto', '+crypto', '', d)}" +# See https://github.com/llvm/llvm-project/issues/85699 +TUNE_CCARGS_MARCH_OPTS .= "${@bb.utils.contains('TUNE_FEATURES', 'crypto', '+crypto', '+nocrypto', d)}"
When crypto is not in tune features then add +nocryto to -mcpu explicitly. This makes the behavior between clang and gcc match. Currently -mcpu=cortex-a72 has different behavior in clang and gcc in terms of what features are considered default. Clang enables different set of common features than gcc on other hand. For example clang enables crypto with default set but gcc does not, gcc recommends to disable unavailable extentions in -mcpu [1] explicitly. crypto is optional on cortext-a53 and cortex-a72 while, this is not as common but Broadcom SOCs in raspberrypi3/4 have dropped crypto for cost reasons [2]. This results in illegal instruction traps [3] [4] when building components e.g. chromium, qtwebengine, weston etc. with clang using -mcpu=cortex-a72 for rpi4 target. Adding +nocrypto makes it behave like gcc does today. We do have separate tune if crypto enabled cortex-a72 cores are to be targeted (cortexa72-cryto) as DEFAULTTUNE They are added to default feature file since crypto extension is available in multiple arm architecture versions e.g. armv8, armv9. It is optional extention as per spec [5] [1] https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html#index-mcpu-2 [2] https://forums.raspberrypi.com/viewtopic.php?f=63&t=207888#p1332960 [3] https://github.com/llvm/llvm-project/issues/85699 [4] https://github.com/llvm/llvm-project/issues/90365 [5] https://developer.arm.com/documentation/109697/2025_06/Feature-descriptions/The-Armv9-0-architecture-extension Signed-off-by: Khem Raj <raj.khem@gmail.com> Cc: Quentin Schulz <quentin.schulz@cherry.de> --- v2: Move the change to feature-arm-crypto.inc. v3: Adjust commit message for clarity on reason to be in common feature file. meta/conf/machine/include/arm/feature-arm-crypto.inc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)