diff mbox series

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

Message ID 20250724170600.1113476-1-raj.khem@gmail.com
State New
Headers show
Series [v4] feature-arm-crypto: Add +nocrypto to -mcpu when crypto not in features | expand

Commit Message

Khem Raj July 24, 2025, 5:06 p.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 extensions
in -mcpu [1] explicitly. crypto is optional on cortex-a53
and cortex-a72. 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 clang 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 extension as per spec [5]

Extensions can be enabled and disabled with -mcpu using the same
syntax as with -march, and have same effect thats why it is
intrumented via TUNE_CCARGS_MARCH_OPTS

[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.
v4: Address commit msg comments, add section on why use TUNE_CCARGS_MARCH_OPTS

 meta/conf/machine/include/arm/feature-arm-crypto.inc | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Quentin Schulz July 24, 2025, 5:09 p.m. UTC | #1
Hi Khem

On 7/24/25 7:06 PM, 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 extensions
> in -mcpu [1] explicitly. crypto is optional on cortex-a53
> and cortex-a72. 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 clang 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 extension as per spec [5]
> 
> Extensions can be enabled and disabled with -mcpu using the same
> syntax as with -march, and have same effect thats why it is
> intrumented via TUNE_CCARGS_MARCH_OPTS
> 

I believe this is incorrect. TUNE_CCARGS_MARCH_OPTS applies to -march, 
not -mcpu.

If that is correct, then The whole explanation should be targeting march 
and not mcpu as we're changing march and not mcpu (though I assume they 
trickle from march to mcpu inside the toolchain).

Cheers,
Quentin
Khem Raj July 24, 2025, 6:32 p.m. UTC | #2
On Thu, Jul 24, 2025 at 10:10 AM Quentin Schulz
<quentin.schulz@cherry.de> wrote:
>
> Hi Khem
>
> On 7/24/25 7:06 PM, 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 extensions
> > in -mcpu [1] explicitly. crypto is optional on cortex-a53
> > and cortex-a72. 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 clang 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 extension as per spec [5]
> >
> > Extensions can be enabled and disabled with -mcpu using the same
> > syntax as with -march, and have same effect thats why it is
> > intrumented via TUNE_CCARGS_MARCH_OPTS
> >
>
> I believe this is incorrect. TUNE_CCARGS_MARCH_OPTS applies to -march,
> not -mcpu.

Well, it applied to both in cpu specific tune files ( e.g.
tune-cortexa55.inc ) we override
arch specific TUNE_FEATURES and replace with then CPU specific TUNE_FEATURES
 This results in  TUNE_CCARGS_MARCH being empty and TUNE_CCARGS_MARCH_OPTS
are still valid and they can appended to TUNE_CCARGS

in conf/machine/include/arm/arch-arm.inc we have
TUNE_CCARGS .= "${TUNE_CCARGS_MARCH}${TUNE_CCARGS_MARCH_OPTS
}"

Due to include order this append appears after the TUNE_CCARGS is
appended with -mcpu option in a mcpu tune file e.g. tune-cortexa55.inc

TUNE_CCARGS .= "${@bb.utils.contains('TUNE_FEATURES', 'cortexa55', '
-mcpu=cortex-a55', '', d)}"

so in the end we get -mcpu=cortex-a55+nocrypto or
-mcpu=cortex-a55+crypto in this case
if you were optimizing widely for an arch then the effect will be via
TUNE_CCARGS_MARCH and
TUNE_CCARGS_MARCH_OPTS

I do wonder whether renaming both TUNE_CCARGS_MARCH and
TUNE_CCARGS_MARCH_OPTS to something more generic could help lessen the
confusion. It's there because we have -march and
introduced -mcpu options later on.

>
> If that is correct, then The whole explanation should be targeting march
> and not mcpu as we're changing march and not mcpu (though I assume they
> trickle from march to mcpu inside the toolchain).
>

your assumption is right. Although, now thinking of this a bit more. I
think we can rename the
TUNE_CCARGS_MARCH_OPTS as I suggested in prior email to something like
TUNE_CCARGS_CPU_EXTENSIONS but in the end they are march options from
gcc/clang's
perspective, its just they are instrumented via either march or mcpu

hope that helps.
> 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)}"