SECLIB-667: Accelerate SHA-512 with A64 crypto extensions #5632

tom-cosgrove-arm · 2022-03-15T10:56:20Z

Description

Provide an additional pair of #defines, MBEDTLS_SHA512_USE_A64_CRYPTO_IF_PRESENT and MBEDTLS_SHA512_USE_A64_CRYPTO_ONLY. At most one of them may be specified. If used, it is necessary to compile with -march=armv8.2-a+sha3.

The MBEDTLS_SHA512_PROCESS_ALT and MBEDTLS_SHA512_ALT mechanisms continue to work, and are mutually exclusive with A64_CRYPTO.

There should be minimal code size impact if no A64_CRYPTO option is set.

Note that the Neon-accelerated implementation was originally written by Simon Tatham for PuTTY, under the MIT licence; it is dual-licensed as Apache 2 with his kind permission. I have restructured it to match the Mbed TLS implementation; any mistakes are mine, not his.

Status

READY

Requires Backporting

NO

Additional comments

The coding standards say "a preferred maximum line length of 80 characters" rather than "a maximum line length of 80 characters" - in a very few places I had exceeded 80 characters by a small amount to improve readability (especially where a sequence of consecutive lines are similar).

Steps to test or reproduce

Define either MBEDTLS_SHA512_USE_A64_CRYPTO_IF_PRESENT or MBEDTLS_SHA512_USE_A64_CRYPTO_ONLY in the configuration, and compile with -march=armv8.2-a+sha3. *IF_PRESENT will work on systems (such as Raspberry Pi) that don't have the crypto extensions; *ONLY will work on most (other) Aarch64 systems.

tom-cosgrove-arm · 2022-03-15T17:34:48Z

library/sha256.c

-        uint32x4_t sched1 = vld1q_u32( (const uint32_t *)( msg + 16 * 1 ) );
-        uint32x4_t sched2 = vld1q_u32( (const uint32_t *)( msg + 16 * 2 ) );
-        uint32x4_t sched3 = vld1q_u32( (const uint32_t *)( msg + 16 * 3 ) );
+        uint32x4_t sched0 = (uint32x4_t) vld1q_u8( msg + 16 * 0 );


Doing it this way avoids increases required alignment when compiled with -Wcast-align

sgtatham · 2022-03-16T09:38:10Z

Note that the Neon-accelerated implementation was originally written by Simon Tatham for PuTTY, under the MIT licence; it is dual-licensed as Apache 2 with his kind permission.

For the record, yes, I confirm that I did give permission for that. Glad it's useful!

tom-cosgrove-arm · 2022-03-16T15:54:13Z

For those wondering: my latest push removed a commit that added a build_neon_intrinsics component to all.sh. Since there aren't any images that could run this component (it needed aarch64 and a gcc >= 8 or clang >= 7), the builds would always fail.

Provide an additional pair of #defines, MBEDTLS_SHA512_USE_A64_CRYPTO_IF_PRESENT and MBEDTLS_SHA512_USE_A64_CRYPTO_ONLY. At most one of them may be specified. If used, it is necessary to compile with -march=armv8.2-a+sha3. The MBEDTLS_SHA512_PROCESS_ALT and MBEDTLS_SHA512_ALT mechanisms continue to work, and are mutually exclusive with SHA512_USE_A64_CRYPTO. There should be minimal code size impact if no A64_CRYPTO option is set. The SHA-512 implementation was originally written by Simon Tatham for PuTTY, under the MIT licence; dual-licensed as Apache 2 with his kind permission. Signed-off-by: Tom Cosgrove <[email protected]>

Signed-off-by: Tom Cosgrove <[email protected]>

Was getting 'dd: unknown operand status' Signed-off-by: Tom Cosgrove <[email protected]>

tom-cosgrove-arm · 2022-03-23T21:45:17Z

Minor update to the detection of which clang requires inline assembler rather than being able to use the intrinsics: from further testing, it seems that as well as < 13, it's only == 13.0.0. that has this problem - 13.0.1 (what you get when asking for 13.0 on Ubuntu) and 13.1 on macOS have the intrinsics and __ARM_FEATURE_SHA512

(Also rebased onto latest development - if you Compare, you'll just want to look at check_config.h and sha512.c)

tom-daubney-arm

I reviewed this in the same manner as the SHA-256 PR (#5547).

My review has focused on the following areas:

Style and naming conventions --> all good
Self test --> passes
Unit tests --> passes
Compilation with different combinations of new config options --> Works as expected
Performance gains --> I compiled and ran on an Aarch64 machine and can report that the performance gain is around a 4.4x increase (for both new config options) when compared to not using the CE, which is worthwhile.

LGTM.

P.S. There were a couple of (IMO) missing full stops on some of the multi-line comments and I have highlighted this in my comments. This is so minor that I am not going to let it halt approval and I leave it to @tom-cosgrove-arm to fix if deemed worth it.

tom-daubney-arm · 2022-03-24T21:22:45Z

include/mbedtls/mbedtls_config.h

+ * an illegal instruction fault will occur).
+ *
+ * \note This allows builds with a smaller code size than with
+ * MBEDTLS_SHA512_USE_A64_CRYPTO_IF_PRESENT


Missing full stop. (To keep it consistent with how you have written the rest of the comments.)

tom-daubney-arm · 2022-03-24T21:23:34Z

library/sha512.c

+#if defined(MBEDTLS_SHA512_USE_A64_CRYPTO_IF_PRESENT)
+/*
+ * Capability detection code comes early, so we can disable
+ * MBEDTLS_SHA512_USE_A64_CRYPTO_IF_PRESENT if no detection mechanism found


Missing full stop

tests/scripts/all.sh

library/sha512.c

xkqian

LGTM

daverodgman · 2022-03-29T14:35:31Z

Fixes #5029

tom-cosgrove-arm requested a review from tom-daubney-arm March 15, 2022 10:58

tom-cosgrove-arm added mbed TLS team needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests labels Mar 15, 2022

tom-cosgrove-arm requested a review from xkqian March 15, 2022 11:06

tom-cosgrove-arm force-pushed the seclib-667-sha512-acceleration-mbedtls-internal branch 6 times, most recently from 59bd283 to f284765 Compare March 15, 2022 17:33

tom-cosgrove-arm commented Mar 15, 2022

View reviewed changes

tom-cosgrove-arm force-pushed the seclib-667-sha512-acceleration-mbedtls-internal branch 3 times, most recently from 6132fa0 to 2785928 Compare March 16, 2022 15:23

tom-cosgrove-arm added 3 commits March 23, 2022 21:40

Minor changes to sha256.c to bring it in line with sha512.c

b7f5b97

Signed-off-by: Tom Cosgrove <[email protected]>

Fix running of all.sh on macOS

226aca1

Was getting 'dd: unknown operand status' Signed-off-by: Tom Cosgrove <[email protected]>

tom-cosgrove-arm force-pushed the seclib-667-sha512-acceleration-mbedtls-internal branch from 2785928 to 226aca1 Compare March 23, 2022 21:41

tom-daubney-arm removed the needs-ci Needs to pass CI tests label Mar 24, 2022

tom-daubney-arm approved these changes Mar 24, 2022

View reviewed changes

xkqian reviewed Mar 29, 2022

View reviewed changes

tests/scripts/all.sh Show resolved Hide resolved

library/sha512.c Show resolved Hide resolved

library/sha512.c Show resolved Hide resolved

library/sha512.c Show resolved Hide resolved

xkqian reviewed Mar 29, 2022

View reviewed changes

xkqian approved these changes Mar 29, 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 Mar 29, 2022

daverodgman merged commit 1c41501 into Mbed-TLS:development Mar 29, 2022

gilles-peskine-arm mentioned this pull request Jun 9, 2022

Changelog improvements before the 3.2 release #5905

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SECLIB-667: Accelerate SHA-512 with A64 crypto extensions #5632

SECLIB-667: Accelerate SHA-512 with A64 crypto extensions #5632

tom-cosgrove-arm commented Mar 15, 2022 •

edited

Loading

tom-cosgrove-arm Mar 15, 2022

sgtatham commented Mar 16, 2022

tom-cosgrove-arm commented Mar 16, 2022 •

edited

Loading

tom-cosgrove-arm commented Mar 23, 2022 •

edited

Loading

tom-daubney-arm left a comment •

edited

Loading

tom-daubney-arm Mar 24, 2022

tom-daubney-arm Mar 24, 2022

xkqian left a comment

daverodgman commented Mar 29, 2022

SECLIB-667: Accelerate SHA-512 with A64 crypto extensions #5632

SECLIB-667: Accelerate SHA-512 with A64 crypto extensions #5632

Conversation

tom-cosgrove-arm commented Mar 15, 2022 • edited Loading

Description

Status

Requires Backporting

Additional comments

Steps to test or reproduce

tom-cosgrove-arm Mar 15, 2022

Choose a reason for hiding this comment

sgtatham commented Mar 16, 2022

tom-cosgrove-arm commented Mar 16, 2022 • edited Loading

tom-cosgrove-arm commented Mar 23, 2022 • edited Loading

tom-daubney-arm left a comment • edited Loading

Choose a reason for hiding this comment

tom-daubney-arm Mar 24, 2022

Choose a reason for hiding this comment

tom-daubney-arm Mar 24, 2022

Choose a reason for hiding this comment

xkqian left a comment

Choose a reason for hiding this comment

daverodgman commented Mar 29, 2022

tom-cosgrove-arm commented Mar 15, 2022 •

edited

Loading

tom-cosgrove-arm commented Mar 16, 2022 •

edited

Loading

tom-cosgrove-arm commented Mar 23, 2022 •

edited

Loading

tom-daubney-arm left a comment •

edited

Loading