Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow armv8-asm on ARMv7-A with -mthumb-interwork #8035

Merged
merged 1 commit into from
Oct 10, 2024

Conversation

danielinux
Copy link
Member

@danielinux danielinux commented Oct 2, 2024

ARMv7-A (e.g. Cortex-A5) support calls between ARM and Thumb mode. When this is enabled (via -mthumb -mthumb-interwork, some functions in the amrv8- ARMASM port are guarded out via && !defined(__thumb__) statement.

Description

This patch removes the !__thumb__ dependency from ARMASM functions to allow compiling with -mthumb-interwork.

This compile flag is required in wolfBoot, as some bring-up procedures are written in thumb assembly, and this MPU allows calling between ARM and THUMB code when this option is enabled.

Testing

Attempting to enable ARMASM to speed up wolfBoot integrity check on SAMA5D3.

Added defines:
-DWOLFSSL_SP_ARM32_ASM -DWOLFSSL_ARMASM -DWOLFSSL_ARMASM_NO_HW_CRYPTO -DWOLFSSL_ARM_ARCH=7 -DWOLFSSL_ARMASM_INLINE -DWOLFSSL_ARMASM_NO_NEON

Relevant gcc flags, turning on both arm and thumb at the same time:
gcc -mcpu=cortex-a5 -mtune=cortex-a5 -mthumb -mthumb-interwork

Current code leaves out Transform_Sha256_Len (unresolved symbol while linking sha256 support).

After removing the dependency on !__thumb__ , the module compiles and works as expected.

@SparkiDev
Copy link
Contributor

SparkiDev commented Oct 2, 2024

If a customer includes both the ARM32 and the Thumb files, won't this cause a problem?
Maybe a new define is required to exclude the ARM32 when Thumb2 and vice-versa.

@SparkiDev SparkiDev assigned danielinux and unassigned SparkiDev Oct 2, 2024
@SparkiDev
Copy link
Contributor

The code is generated so I'll need to modify the scripts to produce the final result.

@danielinux
Copy link
Member Author

https://developer.arm.com/documentation/dui0376/c/Compiler-specific-Features/Compiler-predefines/Predefined-macros?lang=en

Only ARMv6-M and v7-M (armthumb and cortex-m) seem to be thumb-only

@SparkiDev
Copy link
Contributor

thumb gets defined in configure.ac.
How about thumb2 instead?

@danielinux
Copy link
Member Author

I'm not sure what you mean here.
I'm not using autoconf. Are you suggesting to use a different set of defines to activate ASM optimizations on this Cortex-A?

@danielinux
Copy link
Member Author

please note that once compiled, the code works fine on this A5 (at least the SHA I've tested). I'm actually surprised it still needs WOLFSSL_ARMASM_NO_NEON since the chip should support NEON (v1) instructions, but maybe I'm missing some flags to activate SIMD

@danielinux
Copy link
Member Author

Ah I see now. No, it's not our configuration defining __thumb__, it gets enabled by the compiler depending on -mcpu and on -mthumb/-mthumb-interwork.

test-thumb-arm.c:

#ifdef __thumb__
#warning "Thumb mode ON"

#else
#warning "Thumb mode OFF"
#endif

#ifdef __arm__
#warning "ARM mode ON"
#else
#warning "ARM mode OFF"
#endif
arm-none-eabi-gcc -c test-thumb-arm.c -mcpu=cortex-a5 
test-thumb-arm.c:5:2: warning: #warning "Thumb mode OFF" [-Wcpp]
    5 | #warning "Thumb mode OFF"
      |  ^~~~~~~
test-thumb-arm.c:9:2: warning: #warning "ARM mode ON" [-Wcpp]
    9 | #warning "ARM mode ON"
arm-none-eabi-gcc -c test-thumb-arm.c -mcpu=cortex-a5 -mthumb -mthumb-interwork
    2 | #warning "Thumb mode ON"
      |  ^~~~~~~
test-thumb-arm.c:9:2: warning: #warning "ARM mode ON" [-Wcpp]
    9 | #warning "ARM mode ON"
      |  ^~~~~~~

@danielinux
Copy link
Member Author

If a customer includes both the ARM32 and the Thumb files, won't this cause a problem?

I understand this is limiting for eclipse users, perhaps we need a a set of defines that mutually excludes the two ports?

ARMCC offers the following to discriminate the ISA:

image

Gcc has defines like __ARM_ARCH_8A__ __ARM_ARCH_6M__ __THUMB_INTERWORK__ etc.

  • __arm__ is always defined across all ARM targets on all families, regardless if thumb2-only
  • __thumb__ ...is complicated. ARMCC has this:

image

@danielinux
Copy link
Member Author

It looks like it's not just my ARMv7-A, as all platform including newer ARMv8-A can compile with -mthumb when needed, so in conclusion I think we should not be excluding the ARMASM optimizations where supported.
!__thumb__ just tells you the option was not set at compile time

@danielinux
Copy link
Member Author

@SparkiDev I'm keeping this open as a reminder. I still think this should be addressed, although in the asm generator. I have a wolfboot port pending that currently depends on this workaround.

@danielinux
Copy link
Member Author

I have tested and pushed a different suggestion for the assembly with checks for (!defined(__thumb__) || defined(__THUMB_INTERWORK__)) instead. This should work fine and in theory avoid accidental inclusion for non-arm32 speaking targets. Builds fine with cortex-A + interwork. Could this be an acceptable approach?

@SparkiDev
Copy link
Contributor

retest this please

@SparkiDev SparkiDev merged commit 9c4960f into wolfSSL:master Oct 10, 2024
140 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants