diff mbox series

[v3] feature-arm-crypto: Add +nocrypto to -mcpu when crypto not in features

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

Commit Message

Khem Raj July 23, 2025, 1:29 a.m. UTC
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(-)

Comments

Quentin Schulz July 23, 2025, 9:06 a.m. UTC | #1
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
Khem Raj July 24, 2025, 6:13 p.m. UTC | #2
On Wed, Jul 23, 2025 at 2:06 AM Quentin Schulz <quentin.schulz@cherry.de> wrote:
>
> 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?

yes

>
> 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

Well, extensions can be enabled/disabled either with -mcpu or with -march and
they should have the same effect, but they are not documented as the
same set between mcpu and march in gcc manual atleast, maybe it could
be made better.
Perhaps renaming the variable to something like  TUNE_CCARGS_CPU_EXTENTIONS
might be appropriate since it is currently used for ARM architecture
only, I don't see
a reason why it can not be used for other architectures. For RISCV e.g. the
infra to compute the march/mcpu options is different. Perhaps some
logic could be
made common

see below, the list is identical between mcpu/march.

aarch64-linux-gnu-gcc -mcpu=cortex-a72+predres1 ~/tests/a.c
cc1: error: invalid feature modifier 'predres1' in '-mcpu=cortex-a72+predres1'
cc1: note: valid arguments are: rng flagm flagm2 lse fp simd dotprod
sm4 rdma crc sha2 aes crypto sha3 fp16 fp16fml jscvt fcma rcpc rcpc2
rcpc3 frintts i8mm bf16 sve sve-b16b16 f32mm f64mm sve2 sve2-aes
sve2-bitperm sve2-sha3 sve2-sm4 sve2p1 sme memtag sb predres ssbs
profile tme pauth ls64 wfxt xs sme-f64f64 sme-i16i64 sme2 sme2p1
sme-b16b16 sme-f16f16 mops cssc lse128 d128 the gcs fp8 fp8fma
ssve-fp8fma faminmax fp8dot4 ssve-fp8dot4 fp8dot2 ssve-fp8dot2 lut
cpa; did you mean 'predres'?


aarch64-linux-gnu-gcc -march=armv8.1-a+predres1 ~/tests/a.c
cc1: error: invalid feature modifier 'predres1' in '-march=armv8.1-a+predres1'
cc1: note: valid arguments are: rng flagm flagm2 lse fp simd dotprod
sm4 rdma crc sha2 aes crypto sha3 fp16 fp16fml jscvt fcma rcpc rcpc2
rcpc3 frintts i8mm bf16 sve sve-b16b16 f32mm f64mm sve2 sve2-aes
sve2-bitperm sve2-sha3 sve2-sm4 sve2p1 sme memtag sb predres ssbs
profile tme pauth ls64 wfxt xs sme-f64f64 sme-i16i64 sme2 sme2p1
sme-b16b16 sme-f16f16 mops cssc lse128 d128 the gcs fp8 fp8fma
ssve-fp8fma faminmax fp8dot4 ssve-fp8dot4 fp8dot2 ssve-fp8dot2 lut
cpa; did you mean 'predres'?

>
> 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?
>

Perhaps yes

> 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/
>

I have tried to address typos and thinkos in v4

> Cheers,
> Quentin
diff mbox series

Patch

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)}"