Remove encoding width suffix from Arm bignum assembly #6090

hanno-becker · 2022-07-15T11:14:20Z

Within the M-profile of the Arm architecture, some instructions admit both a 16-bit and a 32-bit encoding. For those instructions,
some assemblers support the use of the .n (narrow) and .w (wide) suffixes to force a choice of instruction encoding width.
Forcing the size of encodings may be useful to ensure alignment of code, which can have a significant performance impact on some microarchitectures.

It is for this reason that a previous commit introduced explicit .w suffixes into what was believed to be M-profile only assembly
in library/bn_mul.h.

This change, however, introduced two issues:

First, the assembly block in question is used also for Armv7-A systems, on which the .n/.w distinction is not meaningful (all instructions are 32-bit). This was reported in Building v3.2.1 on ARM fails #6089.
Second, compiler support for .n/.w suffixes appears patchy in the first place, leading to compilation failures even when building for M-profile targets that don't appear trivial to resolve.

This PR removes the .w annotations in order to restore working code, deferring controlled re-introduction for the sake of performance.

It also modifies all.sh to make sure that Arm inline assembly is at least built.

Backport: no

Fixes #6089.

Within the M-profile of the Arm architecture, some instructions admit both a 16-bit and a 32-bit encoding. For those instructions, some assemblers support the use of the .n (narrow) and .w (wide) suffixes to force a choice of instruction encoding width. Forcing the size of encodings may be useful to ensure alignment of code, which can have a significant performance impact on some microarchitectures. It is for this reason that a previous commit introduced explicit .w suffixes into what was believed to be M-profile only assembly in library/bn_mul.h. This change, however, introduced two issues: - First, the assembly block in question is used also for Armv7-A systems, on which the .n/.w distinction is not meaningful (all instructions are 32-bit). - Second, compiler support for .n/.w suffixes appears patchy, leading to compilation failures even when building for M-profile targets. This commit removes the .w annotations in order to restore working code, deferring controlled re-introduction for the sake of performance. Fixes Mbed-TLS#6089. Signed-off-by: Hanno Becker <[email protected]>

The test script all.sh contains the component component_build_armcc testing that Mbed TLS builds using Arm Compiler 5 and 6, on a variety of platforms. However, the component does not exercise inline assembly for Arm, since - MBEDTLS_HAVE_ASM is unset, and - Some Arm inline assembly is only used if the level of optimization is not 0. This commit changes the test component to ensure that inline assembly is built by setting MBEDTLS_HAVE_ASM as well as enabling optimization level 1 (-O1). Signed-off-by: Hanno Becker <[email protected]>

Aethedor · 2022-07-15T11:36:16Z

The changes to bn_mul.h is what I mentioned in my original report. That works for me, so good to hear that it's indeed the right patch.

mpg

Thanks for the fix and test! Looks good to me, but shouldn't we also build that arm asm with gcc? We already have components that build with arm-none-eabi-gcc and arm-linux-gnueabihf-gcc (IIRC), they should probably be extended as well?

Also, I think this is a correct fix, and it's good for now, but in the long run I wonder if a better solution could be found. I thought the point of Arm's Unified Assembler Language was precisely what we shouldn't need to use different syntax from M-class (Thumb) and A-class (Arm). And it seems to me that GNU asm supports it with .syntax unified. But I'm not familiar enough with the details to know if this would work, so I'm very happy to merge the obvious fix for now and leave that for later - I just wanted to share the idea.

hanno-becker · 2022-07-19T08:04:20Z

@mpg You may very well be right, and if you have time, I'd encourage and appreciate further investigation. My impression was that even though in principle, unified assembler is what we want here, compiler support is lacking. For example, you would want .w suffices to be completely ignored if (a) compiling for A-class, or (b) when they're used with instructions on M-class which are 32-bit only -- but that, apparently, is not the case.

mpg · 2022-07-19T08:13:29Z

Yes, I have no practical experience with that, so it's entirely possible that it doesn't work in practice. I'm also unlikely to have time to investigate, which is why I'm happy approving the present fix - as soon as the question of whether we should also extend the components that build with GCC for Arm is resolved.

hanno-becker · 2022-07-19T08:17:36Z

@tom-cosgrove-arm Do you have time to look into build tests using GCC?

tom-cosgrove-arm · 2022-07-19T08:29:41Z

@hanno-arm Sorry, no, I am somewhat overcommitted at the moment :(

tom-cosgrove-arm · 2022-07-20T09:07:54Z

Note that with this PR, I am concerned that the all.sh updates are just changing existing builds, which means, although we will now be testing stuff we want to, we will no longer be testing stuff we used to. I don't know all.sh well enough to know if this is a problem

daverodgman · 2022-08-16T13:09:38Z

@lhuang04 confirms that this PR addresses the problem for his use-case

daverodgman · 2022-08-16T15:56:19Z

Note that with this PR, I am concerned that the all.sh updates are just changing existing builds, which means, although we will now be testing stuff we want to, we will no longer be testing stuff we used to. I don't know all.sh well enough to know if this is a problem

Given that this test is very quick, I suggest we add an additional couple of lines to retain coverage of -O0 / assembly disabled

tests/scripts/all.sh

hanno-becker · 2022-08-16T15:58:17Z

We do have tests for ASM disabled: https://github.com/Mbed-TLS/mbedtls/blob/development/tests/scripts/all.sh#L2811 for 32-bit and https://github.com/Mbed-TLS/mbedtls/blob/development/tests/scripts/all.sh#L2822 for 64-bit.

daverodgman

LGTM but suggest we retain some coverage of non-assembly builds for armcc?

gilles-peskine-arm · 2022-08-16T16:50:25Z

@daverodgman We test with assembly disabled. That's important because there could be a bug in the C code. However, there's no reason to test with multiple compilers. (There are reasons to test with different endiannesses — which we don't do at all at the moment — and with different word sizes, which we do.)

It is useful to test assembly code with different compilers because they might optimize differently and this can reveal bugs at the asm/c interface. For example recently we had code that was missing a clobber declaration and this worked fine with GCC but led Clang to produce non-working code.

gilles-peskine-arm

LGTM for the assembly change (I agree with worrying about portability first and compiler- and CPU-specific microoptimization second). But please keep the plain v7m build.

gilles-peskine-arm · 2022-08-16T16:51:40Z

tests/scripts/all.sh

@@ -2870,6 +2870,7 @@ component_build_armcc () {
    scripts/config.py baremetal
    # armc[56] don't support SHA-512 intrinsics
    scripts/config.py unset MBEDTLS_SHA512_USE_A64_CRYPTO_IF_PRESENT
+    scripts/config.py set MBEDTLS_HAVE_ASM


It's on by default anyway, but I have nothing against explicitly enabling it.

gilles-peskine-arm · 2022-08-16T16:52:24Z

tests/scripts/all.sh


    # ARM Compiler 6 - Target ARMv7-M
-    armc6_build_test "--target=arm-arm-none-eabi -march=armv7-m"
+    armc6_build_test "--target=arm-arm-none-eabi -O1 -march=armv7-m+dsp"


Blocker: please keep a build with a plain arch=armv7-m as well.

gilles-peskine-arm · 2022-08-16T16:55:31Z

tests/scripts/all.sh

    # ARM Compiler 6 - Target ARMv7-A
-    armc6_build_test "--target=arm-arm-none-eabi -march=armv7-a"
+    armc6_build_test "--target=arm-arm-none-eabi -O1 -march=armv7-a"


Minor: I'd prefer to have --target and -march next to each other since they're strongly related.

Signed-off-by: Dave Rodgman <[email protected]>

daverodgman · 2022-08-17T13:42:08Z

(marking single reviewer as it's been double-reviewed up to the last, trivial commit)

tom-cosgrove-arm

LGTM

peter-toft-greve · 2022-08-24T06:10:58Z

Good stuff! Fixes ARMv7l build fails

Remove encoding width suffix from Arm bignum assembly

nasli · 2022-11-22T11:25:38Z

Thanks for the fix!!
Is there an ETA for the next release @gilles-peskine-arm ? Thanks!

gilles-peskine-arm · 2022-11-22T11:27:44Z

@nasli We're planning a release mid-December.

nasli · 2022-11-22T16:25:53Z

Great! thank you for the update!

* Added mbedcrypto changes * More changes * fix warning * added path for RSA wrapped keys * update for new key id argument * Updated export size macro to return value for wrapped keys also fixed missing argument * exposed new ecc static fns * ECP macro updates for wrapped keys * ECP key import/export updates * Fixed CMAC and GCM usage * added macro to exclude vendor path if RSA/ECCis not enabled * removed test code * fixed else condition * remove commented code 2 * Undo persistent check * review comments * removed commented code * Added check for MBEDTLS_CIPHER_ALT * type update * updates for 2.4 * updated RSA wrapped key type check * macro condition fix * retained changes from older release * Added HW acceleration segment * ECP HW acceleration * HW acceleration updates. * made vendor related changes * fix for persistent key support. * updated unpadding of rsa pkcs * changes in slot management src code * AES hardware acceleration enabled. * fix for vendor key import * SWFLEX-4447: Updated dlot management src code * remove unused definitions * Vendor related changes * Temporary change * test change for IAR compiler * reverted test changes * test change to fix ac6 error * Merge pull request Mbed-TLS#6090 from hanno-arm/fix_bnmul_arm_v7a Remove encoding width suffix from Arm bignum assembly * Updated crypto cipher file to support XTS mode * tfm-1-7-0 specific patch * tfm patch on driver wrapper key agreement * Vendor related changes * change in ecp file * Updated psa_util * FSPRA-754 Updated header files * FSPRA-754 Updated aead src file * FSPRA-754 Reverted the changes of constant_time_interval * FSPRA-754 Reverted back the changes of crypto aead * FSPRA-754 Fixed build error * tfm builtin key retrieve * Updated crypto cipher src * Updated vendor related code * Vendor code update * Fixed build err * build err * Vendor code update * Deleting unwanted change * Updated vendor changes * Updated magic numbers * Updated crypto mac * Updated mac * Alignment update * Resolved llvm error on trng and aes engines * Updated ecp file * vendor related change * Vendor related changes * Updated constant-time src file * Updated crypto_sizes header * Build failure fix * Test change * Fixed build error * LLVM error fix --------- Co-authored-by: Michael Thomas <[email protected]> Co-authored-by: michaelthomasj <[email protected]> Co-authored-by: aniruddhabajaj-renesas <[email protected]> Co-authored-by: Ajay N <[email protected]> Co-authored-by: aniruddha.bajaj <[email protected]> Co-authored-by: Varun.C <[email protected]> Co-authored-by: Dave Rodgman <[email protected]> Co-authored-by: lavanya.selvaraju.cj <[email protected]>

* Fspra 2646 mbedtls update 3 5 0 (#21) * Added mbedcrypto changes * More changes * fix warning * added path for RSA wrapped keys * update for new key id argument * Updated export size macro to return value for wrapped keys also fixed missing argument * exposed new ecc static fns * ECP macro updates for wrapped keys * ECP key import/export updates * Fixed CMAC and GCM usage * added macro to exclude vendor path if RSA/ECCis not enabled * removed test code * fixed else condition * remove commented code 2 * Undo persistent check * review comments * removed commented code * Added check for MBEDTLS_CIPHER_ALT * type update * updates for 2.4 * updated RSA wrapped key type check * macro condition fix * retained changes from older release * Added HW acceleration segment * ECP HW acceleration * HW acceleration updates. * made vendor related changes * fix for persistent key support. * updated unpadding of rsa pkcs * changes in slot management src code * AES hardware acceleration enabled. * fix for vendor key import * SWFLEX-4447: Updated dlot management src code * remove unused definitions * Vendor related changes * Temporary change * test change for IAR compiler * reverted test changes * test change to fix ac6 error * Merge pull request Mbed-TLS#6090 from hanno-arm/fix_bnmul_arm_v7a Remove encoding width suffix from Arm bignum assembly * Updated crypto cipher file to support XTS mode * tfm-1-7-0 specific patch * tfm patch on driver wrapper key agreement * Vendor related changes * change in ecp file * Updated psa_util * FSPRA-754 Updated header files * FSPRA-754 Updated aead src file * FSPRA-754 Reverted the changes of constant_time_interval * FSPRA-754 Reverted back the changes of crypto aead * FSPRA-754 Fixed build error * tfm builtin key retrieve * Updated crypto cipher src * Updated vendor related code * Vendor code update * Fixed build err * build err * Vendor code update * Deleting unwanted change * Updated vendor changes * Updated magic numbers * Updated crypto mac * Updated mac * Alignment update * Resolved llvm error on trng and aes engines * Updated ecp file * vendor related change * Vendor related changes * Updated constant-time src file * Updated crypto_sizes header * Build failure fix * Test change * Fixed build error * LLVM error fix --------- Co-authored-by: Michael Thomas <[email protected]> Co-authored-by: michaelthomasj <[email protected]> Co-authored-by: aniruddhabajaj-renesas <[email protected]> Co-authored-by: Ajay N <[email protected]> Co-authored-by: aniruddha.bajaj <[email protected]> Co-authored-by: Varun.C <[email protected]> Co-authored-by: Dave Rodgman <[email protected]> Co-authored-by: lavanya.selvaraju.cj <[email protected]> * Patch for TFM 2.0.0 (#23) Co-authored-by: lavanya.selvaraju.cj <[email protected]> * Add header to include acceleration header file * Fspra 2555 psa ed25519 3.4.0 renesas.7 (#22) * Added mbedcrypto changes * Vendor related changes * Updated ecp file * Fixed err * Updated psa_crypto_slot_management * Fixed errors * Fixed IAR error * Updated src file * RSA file update * Test change * Added comment --------- Co-authored-by: Michael Thomas <[email protected]> Co-authored-by: michaelthomasj <[email protected]> Co-authored-by: aniruddhabajaj-renesas <[email protected]> Co-authored-by: Ajay N <[email protected]> Co-authored-by: aniruddha.bajaj <[email protected]> Co-authored-by: Varun.C <[email protected]> Co-authored-by: Dave Rodgman <[email protected]> Co-authored-by: lavanya.selvaraju.cj <[email protected]> Co-authored-by: hienthehuynh <[email protected]>

* Fspra 3246 mbedtls update 3 6 0 (#25) * Fspra 2646 mbedtls update 3 5 0 (#21) * Added mbedcrypto changes * More changes * fix warning * added path for RSA wrapped keys * update for new key id argument * Updated export size macro to return value for wrapped keys also fixed missing argument * exposed new ecc static fns * ECP macro updates for wrapped keys * ECP key import/export updates * Fixed CMAC and GCM usage * added macro to exclude vendor path if RSA/ECCis not enabled * removed test code * fixed else condition * remove commented code 2 * Undo persistent check * review comments * removed commented code * Added check for MBEDTLS_CIPHER_ALT * type update * updates for 2.4 * updated RSA wrapped key type check * macro condition fix * retained changes from older release * Added HW acceleration segment * ECP HW acceleration * HW acceleration updates. * made vendor related changes * fix for persistent key support. * updated unpadding of rsa pkcs * changes in slot management src code * AES hardware acceleration enabled. * fix for vendor key import * SWFLEX-4447: Updated dlot management src code * remove unused definitions * Vendor related changes * Temporary change * test change for IAR compiler * reverted test changes * test change to fix ac6 error * Merge pull request Mbed-TLS#6090 from hanno-arm/fix_bnmul_arm_v7a Remove encoding width suffix from Arm bignum assembly * Updated crypto cipher file to support XTS mode * tfm-1-7-0 specific patch * tfm patch on driver wrapper key agreement * Vendor related changes * change in ecp file * Updated psa_util * FSPRA-754 Updated header files * FSPRA-754 Updated aead src file * FSPRA-754 Reverted the changes of constant_time_interval * FSPRA-754 Reverted back the changes of crypto aead * FSPRA-754 Fixed build error * tfm builtin key retrieve * Updated crypto cipher src * Updated vendor related code * Vendor code update * Fixed build err * build err * Vendor code update * Deleting unwanted change * Updated vendor changes * Updated magic numbers * Updated crypto mac * Updated mac * Alignment update * Resolved llvm error on trng and aes engines * Updated ecp file * vendor related change * Vendor related changes * Updated constant-time src file * Updated crypto_sizes header * Build failure fix * Test change * Fixed build error * LLVM error fix --------- Co-authored-by: Michael Thomas <[email protected]> Co-authored-by: michaelthomasj <[email protected]> Co-authored-by: aniruddhabajaj-renesas <[email protected]> Co-authored-by: Ajay N <[email protected]> Co-authored-by: aniruddha.bajaj <[email protected]> Co-authored-by: Varun.C <[email protected]> Co-authored-by: Dave Rodgman <[email protected]> Co-authored-by: lavanya.selvaraju.cj <[email protected]> * Patch for TFM 2.0.0 (#23) Co-authored-by: lavanya.selvaraju.cj <[email protected]> * Add header to include acceleration header file * Fspra 2555 psa ed25519 3.4.0 renesas.7 (#22) * Added mbedcrypto changes * Vendor related changes * Updated ecp file * Fixed err * Updated psa_crypto_slot_management * Fixed errors * Fixed IAR error * Updated src file * RSA file update * Test change * Added comment --------- Co-authored-by: Michael Thomas <[email protected]> Co-authored-by: michaelthomasj <[email protected]> Co-authored-by: aniruddhabajaj-renesas <[email protected]> Co-authored-by: Ajay N <[email protected]> Co-authored-by: aniruddha.bajaj <[email protected]> Co-authored-by: Varun.C <[email protected]> Co-authored-by: Dave Rodgman <[email protected]> Co-authored-by: lavanya.selvaraju.cj <[email protected]> Co-authored-by: hienthehuynh <[email protected]> * Fspra 3448 TFM 2 .1 patch update (#26) * TFM Patch * Updated psa crypto file * psa crypto file update * Reverted psa crypto change --------- Co-authored-by: lavanya.selvaraju.cj <[email protected]> * Fixed build error * Test change * Revert "Test change" This reverts commit 2cb2836. --------- Co-authored-by: Michael Thomas <[email protected]> Co-authored-by: michaelthomasj <[email protected]> Co-authored-by: aniruddhabajaj-renesas <[email protected]> Co-authored-by: Ajay N <[email protected]> Co-authored-by: aniruddha.bajaj <[email protected]> Co-authored-by: Varun.C <[email protected]> Co-authored-by: Dave Rodgman <[email protected]> Co-authored-by: lavanya.selvaraju.cj <[email protected]> Co-authored-by: hienthehuynh <[email protected]>

Hanno Becker added 2 commits July 15, 2022 12:00

hanno-becker added bug component-crypto Crypto primitives and low-level interfaces needs-reviewer This PR needs someone to pick it up for review labels Jul 15, 2022

hanno-becker mentioned this pull request Jul 15, 2022

Building v3.2.1 on ARM fails #6089

Closed

mpg self-requested a review July 18, 2022 14:20

mpg reviewed Jul 19, 2022

View reviewed changes

daverodgman added the priority-high High priority - will be reviewed soon label Aug 16, 2022

daverodgman reviewed Aug 16, 2022

View reviewed changes

tests/scripts/all.sh Show resolved Hide resolved

daverodgman requested changes Aug 16, 2022

View reviewed changes

gilles-peskine-arm requested changes Aug 16, 2022

View reviewed changes

Add armcc plain armv7-m target; tidy up arg order

03f7a6e

Signed-off-by: Dave Rodgman <[email protected]>

daverodgman self-assigned this Aug 17, 2022

daverodgman added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests single-reviewer This PR qualifies for having only one reviewer and removed needs-reviewer This PR needs someone to pick it up for review labels Aug 17, 2022

tom-cosgrove-arm approved these changes Aug 17, 2022

View reviewed changes

tom-cosgrove-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Aug 17, 2022

gilles-peskine-arm approved these changes Aug 17, 2022

View reviewed changes

gilles-peskine-arm removed the single-reviewer This PR qualifies for having only one reviewer label Aug 17, 2022

daverodgman removed the needs-ci Needs to pass CI tests label Aug 17, 2022

daverodgman merged commit 92cd864 into Mbed-TLS:development Aug 18, 2022

tom-cosgrove-arm mentioned this pull request Aug 20, 2022

Can't compile 3.2.1 on xcode xcrun --sdk iphoneos15.5 clang++ #6217

Closed

michaelthomasj pushed a commit to renesas/mbedtls that referenced this pull request Oct 5, 2022

Merge pull request Mbed-TLS#6090 from hanno-arm/fix_bnmul_arm_v7a

e12c8dc

Remove encoding width suffix from Arm bignum assembly

gilles-peskine-arm mentioned this pull request Oct 22, 2022

MULADDC_X2_CORE ARM crashes the linker (ld) (Android build) #6472

Closed

daverodgman mentioned this pull request Dec 8, 2022

Add Changelog for Arm compile fix #6752

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove encoding width suffix from Arm bignum assembly #6090

Remove encoding width suffix from Arm bignum assembly #6090

hanno-becker commented Jul 15, 2022 •

edited by daverodgman

Loading

Aethedor commented Jul 15, 2022

mpg left a comment

hanno-becker commented Jul 19, 2022

mpg commented Jul 19, 2022

hanno-becker commented Jul 19, 2022

tom-cosgrove-arm commented Jul 19, 2022

tom-cosgrove-arm commented Jul 20, 2022

daverodgman commented Aug 16, 2022

daverodgman commented Aug 16, 2022

hanno-becker commented Aug 16, 2022

daverodgman left a comment •

edited

Loading

gilles-peskine-arm commented Aug 16, 2022

gilles-peskine-arm left a comment

gilles-peskine-arm Aug 16, 2022

gilles-peskine-arm Aug 16, 2022

gilles-peskine-arm Aug 16, 2022

daverodgman commented Aug 17, 2022

tom-cosgrove-arm left a comment

peter-toft-greve commented Aug 24, 2022

nasli commented Nov 22, 2022

gilles-peskine-arm commented Nov 22, 2022

nasli commented Nov 22, 2022

Remove encoding width suffix from Arm bignum assembly #6090

Remove encoding width suffix from Arm bignum assembly #6090

Conversation

hanno-becker commented Jul 15, 2022 • edited by daverodgman Loading

Aethedor commented Jul 15, 2022

mpg left a comment

Choose a reason for hiding this comment

hanno-becker commented Jul 19, 2022

mpg commented Jul 19, 2022

hanno-becker commented Jul 19, 2022

tom-cosgrove-arm commented Jul 19, 2022

tom-cosgrove-arm commented Jul 20, 2022

daverodgman commented Aug 16, 2022

daverodgman commented Aug 16, 2022

hanno-becker commented Aug 16, 2022

daverodgman left a comment • edited Loading

Choose a reason for hiding this comment

gilles-peskine-arm commented Aug 16, 2022

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm Aug 16, 2022

Choose a reason for hiding this comment

gilles-peskine-arm Aug 16, 2022

Choose a reason for hiding this comment

gilles-peskine-arm Aug 16, 2022

Choose a reason for hiding this comment

daverodgman commented Aug 17, 2022

tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

peter-toft-greve commented Aug 24, 2022

nasli commented Nov 22, 2022

gilles-peskine-arm commented Nov 22, 2022

nasli commented Nov 22, 2022

hanno-becker commented Jul 15, 2022 •

edited by daverodgman

Loading

daverodgman left a comment •

edited

Loading