Neon impl of ChaCha20 (better size & perf) #159

Merged
daverodgman merged 28 commits from chacha-neon-split2 into development 2025-10-21 09:34:08 -05:00
daverodgman commented 2025-01-15 12:24:12 -06:00 (Migrated from github.com)

Description

Neon implementation of ChaCha20 which, depending on how it's configured, is ~500b smaller than the existing impl, or up to 4.6x faster. In its default setting it has slightly better code size than the existing impl, and is 2-2.7x faster.

Tested on all combinations of Armv7 arm/thumb2; Armv8 arm/thumb2/aarch64 on clang 14, gcc 11, and some more recent versions, plus Armv8 aarch64_be on gcc 7.5.

PR checklist

Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line.
If the provided content is part of the present PR remove the # symbol.

multiblock setting size delta c.f. development (aarch64) perf delta c.f. development (aarch64)
clang 0 -84 0%
1 -548 14%
2 -344 64%
3 -144 98%
4 76 172%
5 352 161%
6 576 224%
gcc 0 -71 0%
1 -519 57%
2 -275 129%
3 1 177%
4 309 280%
5 665 273%
6 881 355%
## Description Neon implementation of ChaCha20 which, depending on how it's configured, is ~500b smaller than the existing impl, or up to 4.6x faster. In its default setting it has slightly better code size than the existing impl, and is 2-2.7x faster. Tested on all combinations of Armv7 arm/thumb2; Armv8 arm/thumb2/aarch64 on clang 14, gcc 11, and some more recent versions, plus Armv8 aarch64_be on gcc 7.5. ## PR checklist Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line. If the provided content is part of the present PR remove the # symbol. - [ ] **changelog** provided - [ ] **development PR** provided - [ ] **framework PR** not required - [ ] **3.6 PR** not required because: non-functional change - [ ] **2.28 PR** not required because: non-functional change - [ ] **tests** to follow via separate PR in mbedtls repo - see https://github.com/Mbed-TLS/mbedtls/pull/9701/commits/a8e9a884bd5582f9d5a9adf22472380b45864d2b | | multiblock setting | size delta c.f. development (aarch64) | perf delta c.f. development (aarch64) | | --- | --- | --- | --- | | clang | 0 | -84 | 0% | | | 1 | -548 | 14% | | | 2 | -344 | 64% | | | 3 | -144 | 98% | | | 4 | 76 | 172% | | | 5 | 352 | 161% | | | 6 | 576 | 224% | | | | | | | gcc | 0 | -71 | 0% | | | 1 | -519 | 57% | | | 2 | -275 | 129% | | | 3 | 1 | 177% | | | 4 | 309 | 280% | | | 5 | 665 | 273% | | | 6 | 881 | 355% |
ronald-cron-arm commented 2025-01-16 02:15:38 -06:00 (Migrated from github.com)

Thanks for moving your PR to the new repo.

Thanks for moving your PR to the new repo.
davidhorstmann-arm (Migrated from github.com) requested changes 2025-08-27 09:01:54 -05:00
davidhorstmann-arm (Migrated from github.com) left a comment

This looks good to me generally. Could you add a testcase in tests/suites/test_suite_chacha20.data that depends on MBEDTLS_HAVE_NEON_INTRINSICS? It can be a duplicate of one of the existing cases.

That way the CI should complain if we don't test this implementation at all, since any testcase that is never executed will generate an error in analyze_outcomes.py.

This looks good to me generally. Could you add a testcase in `tests/suites/test_suite_chacha20.data` that depends on `MBEDTLS_HAVE_NEON_INTRINSICS`? It can be a duplicate of one of the existing cases. That way the CI should complain if we don't test this implementation at all, since any testcase that is never executed will generate an error in `analyze_outcomes.py`.
davidhorstmann-arm (Migrated from github.com) commented 2025-08-27 06:13:09 -05:00

Note to self: The TBL instruction performed here has data-independent timing when DIT is set (source)

Note to self: The `TBL` instruction performed here has data-independent timing when DIT is set ([source](https://developer.arm.com/documentation/ddi0595/2021-06/AArch64-Registers/DIT--Data-Independent-Timing))
davidhorstmann-arm commented 2025-08-27 09:03:01 -05:00 (Migrated from github.com)

It also looks like this could do with a rebase on top of latest for the CI's benefit.

It also looks like this could do with a rebase on top of latest for the CI's benefit.
daverodgman commented 2025-08-30 07:34:29 -05:00 (Migrated from github.com)

Rebased (clean, the rebase caused no changes to the files touched in this PR). Added requested test.

If you can advise on how/where to pull in ‎tests/scripts/components-configuration-crypto.sh I'll add that too.

Rebased (clean, the rebase caused no changes to the files touched in this PR). Added requested test. If you can advise on how/where to pull in [‎tests/scripts/components-configuration-crypto.sh](https://github.com/Mbed-TLS/mbedtls/pull/9701/commits/a8e9a884bd5582f9d5a9adf22472380b45864d2b) I'll add that too.
davidhorstmann-arm commented 2025-09-01 04:20:11 -05:00 (Migrated from github.com)

If you can advise on how/where to pull in ‎tests/scripts/components-configuration-crypto.sh I'll add that too.

That would be excellent, thanks! The eventual plan is to move all of the crypto components to this repo. For now you can put this component in tests/scripts/components-configuration.sh. If you feel like rewriting it for CMake too that would be lovely, as we're planning to deprecate Make soon, but we have quite a few Make components in there already so no problem if not.

It looks like check-names is failing for reasons unrelated to this PR (there is a lot of churn at the moment). I will try to figure out what's happening there.

> If you can advise on how/where to pull in [‎tests/scripts/components-configuration-crypto.sh](https://github.com/Mbed-TLS/mbedtls/pull/9701/commits/a8e9a884bd5582f9d5a9adf22472380b45864d2b) I'll add that too. That would be excellent, thanks! The eventual plan is to move all of the crypto components to this repo. For now you can put this component in `tests/scripts/components-configuration.sh`. If you feel like rewriting it for CMake too that would be lovely, as we're planning to deprecate Make soon, but we have quite a few Make components in there already so no problem if not. It looks like check-names is failing for reasons unrelated to this PR (there is a lot of churn at the moment). I will try to figure out what's happening there.
davidhorstmann-arm (Migrated from github.com) reviewed 2025-10-08 10:55:52 -05:00
davidhorstmann-arm (Migrated from github.com) left a comment

Apologies for the delay, I think I have worked out why the CMake tests are not working (although I don't currently have an appropriate machine set up to test).

Would you be able to try out this change? I should then be able to approve.

Apologies for the delay, I think I have worked out why the CMake tests are not working (although I don't currently have an appropriate machine set up to test). Would you be able to try out this change? I should then be able to approve.
davidhorstmann-arm (Migrated from github.com) commented 2025-10-08 10:49:58 -05:00

I believe with CMake the setting of CFLAGS must be done at configure time so the following loop should work instead:


    for x in 0 1 2 3 4 5 6; do
        msg "multiblock = $x"

        cmake -DCMAKE_BUILD_TYPE:String=Release -DCMAKE_C_FLAGS="-DMBEDTLS_CHACHA20_NEON_MULTIBLOCK=$x" "$TF_PSA_CRYPTO_ROOT_DIR"
        make -C tests test_suite_chacha20 
        ./tests/test_suite_chacha20
    done
I believe with CMake the setting of `CFLAGS` must be done at configure time so the following loop should work instead: ```suggestion for x in 0 1 2 3 4 5 6; do msg "multiblock = $x" cmake -DCMAKE_BUILD_TYPE:String=Release -DCMAKE_C_FLAGS="-DMBEDTLS_CHACHA20_NEON_MULTIBLOCK=$x" "$TF_PSA_CRYPTO_ROOT_DIR" make -C tests test_suite_chacha20 ./tests/test_suite_chacha20 done ```
davidhorstmann-arm commented 2025-10-08 10:57:28 -05:00 (Migrated from github.com)

Needs: 1 small rework, final round of first reviewer and review from second reviewer.

Needs: 1 small rework, final round of first reviewer and review from second reviewer.
daverodgman commented 2025-10-09 08:16:08 -05:00 (Migrated from github.com)

Thanks @davidhorstmann-arm for help with cmake - I've verified locally that this builds the different variations as intended.

Thanks @davidhorstmann-arm for help with cmake - I've verified locally that this builds the different variations as intended.
davidhorstmann-arm (Migrated from github.com) reviewed 2025-10-09 08:26:44 -05:00
davidhorstmann-arm (Migrated from github.com) left a comment
No description provided.
LGTM, thanks!
daverodgman commented 2025-10-09 08:47:23 -05:00 (Migrated from github.com)

rebased onto latest development

rebased onto latest development
davidhorstmann-arm (Migrated from github.com) reviewed 2025-10-09 11:00:18 -05:00
davidhorstmann-arm (Migrated from github.com) left a comment
No description provided.
LGTM, thanks!
davidhorstmann-arm (Migrated from github.com) reviewed 2025-10-13 02:31:43 -05:00
davidhorstmann-arm (Migrated from github.com) left a comment
No description provided.
LGTM, thanks! CI is on hold for now for release testing but I'll kick it once we are in the clear there.
davidhorstmann-arm (Migrated from github.com) reviewed 2025-10-14 09:04:49 -05:00
davidhorstmann-arm (Migrated from github.com) left a comment
No description provided.
LGTM, thanks!
davidhorstmann-arm (Migrated from github.com) reviewed 2025-10-17 02:52:31 -05:00
davidhorstmann-arm (Migrated from github.com) left a comment
No description provided.
LGTM, thanks!
yanesca (Migrated from github.com) reviewed 2025-10-17 11:08:49 -05:00
yanesca (Migrated from github.com) left a comment

Hi Dave, thank you for the PR, I had a look and I have a few questions.

Hi Dave, thank you for the PR, I had a look and I have a few questions.
yanesca (Migrated from github.com) commented 2025-10-17 10:31:50 -05:00

I think this raises two questions:

  1. do the performance improvements persist when the DIT bit is set (I mean is this implementation faster with DIT on than the else branch without DIT set)?
  2. we haven't thought about how the library would relate to the DIT feature. Do we recommend users to set it? Are we going to add it to platform abstraction and control it ourselves? Something else? What security requirements we have for DIT code and non-DIT code? What security guarantees can we achieve and how do we communicate them?

And I am more concerned about the second point, I don't feel we are ready to integrate code for which the security relies on DIT being set.

I think this raises two questions: 1. do the performance improvements persist when the DIT bit is set (I mean is this implementation faster with DIT on than the else branch without DIT set)? 2. we haven't thought about how the library would relate to the DIT feature. Do we recommend users to set it? Are we going to add it to platform abstraction and control it ourselves? Something else? What security requirements we have for DIT code and non-DIT code? What security guarantees can we achieve and how do we communicate them? And I am more concerned about the second point, I don't feel we are ready to integrate code for which the security relies on DIT being set.
yanesca (Migrated from github.com) commented 2025-10-17 10:35:37 -05:00

I am a bit concerned on the DIT bit requirement for this to be constant time. Do you think we could go for the else branch for now and add this part back when we managed to sort out our thoughts around DIT?

I am a bit concerned on the [DIT bit](https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/159/commits/bd5c34ccf89b6c2033d9b31d5d1b52b51013a35d#r2303621644) requirement for this to be constant time. Do you think we could go for the `else` branch for now and add this part back when we managed to sort out our thoughts around DIT?
yanesca (Migrated from github.com) commented 2025-10-17 09:05:08 -05:00

I would think that on big endian machines this changes the values of the constants. What am I missing?

I would think that on big endian machines this changes the values of the constants. What am I missing?
@ -190,3 +206,4 @@
}
mbedtls_platform_zeroize(ctx->keystream8, sizeof(ctx->keystream8));
yanesca (Migrated from github.com) commented 2025-10-17 09:03:00 -05:00

nit (for readability):

    while (ctx->keystream_bytes_used < CHACHA20_BLOCK_SIZE_BYTES && size > 0) {
nit (for readability): ```suggestion while (ctx->keystream_bytes_used < CHACHA20_BLOCK_SIZE_BYTES && size > 0) { ```
@ -216,3 +235,3 @@
/* Process full blocks */
while (size >= CHACHA20_BLOCK_SIZE_BYTES) {
while (size >= MBEDTLS_CHACHA20_BLOCK_SIZE_BYTES) {
/* Generate new keystream block and increment counter */
yanesca (Migrated from github.com) commented 2025-10-17 08:35:55 -05:00

Is there a reason for doing this check in runtime?

Is there a reason for doing this check in runtime?
yanesca (Migrated from github.com) commented 2025-10-17 08:54:15 -05:00

This change is likely to make things slower if compiled with an older GCC on a platform without efficient unaligned access or if compiled with IAR on some platforms. We could prevent this if we checked for UINT_UNALIGNED and UINT_UNALIGNED_STRUCT as well.

This change is likely to make things slower if compiled with an older GCC on a platform without efficient unaligned access or if compiled with IAR on some platforms. We could prevent this if we checked for `UINT_UNALIGNED` and `UINT_UNALIGNED_STRUCT` as well.
yanesca (Migrated from github.com) commented 2025-10-17 09:00:24 -05:00

Same as above.

Same as above.
yanesca (Migrated from github.com) commented 2025-10-17 10:59:21 -05:00

I am sorry, am not very familiar with the new config, why do we need PSA_WANT_KEY_TYPE_AES and PSA_WANT_ALG_SHA_256 for ChaCha?

I am sorry, am not very familiar with the new config, why do we need `PSA_WANT_KEY_TYPE_AES` and `PSA_WANT_ALG_SHA_256` for ChaCha?
yanesca (Migrated from github.com) commented 2025-10-17 11:08:18 -05:00

I am not sure how this function is used by our test scripts, could you please help me understand where this is called from?

Never mind, fixed in later commit.

~~I am not sure how this function is used by our test scripts, could you please help me understand where this is called from?~~ Never mind, fixed in later commit.
daverodgman (Migrated from github.com) reviewed 2025-10-17 12:06:05 -05:00
@ -216,3 +235,3 @@
/* Process full blocks */
while (size >= CHACHA20_BLOCK_SIZE_BYTES) {
while (size >= MBEDTLS_CHACHA20_BLOCK_SIZE_BYTES) {
/* Generate new keystream block and increment counter */
daverodgman (Migrated from github.com) commented 2025-10-17 12:06:05 -05:00

Because MBEDTLS_IS_BIG_ENDIAN is not guaranteed to be compile-time const. On just about every reasonable compiler, it is constant, and the branch is optimised away.

Because `MBEDTLS_IS_BIG_ENDIAN` is not guaranteed to be compile-time const. On just about every reasonable compiler, it is constant, and the branch is optimised away.
daverodgman (Migrated from github.com) reviewed 2025-10-17 12:08:39 -05:00
@ -216,3 +235,3 @@
/* Process full blocks */
while (size >= CHACHA20_BLOCK_SIZE_BYTES) {
while (size >= MBEDTLS_CHACHA20_BLOCK_SIZE_BYTES) {
/* Generate new keystream block and increment counter */
daverodgman (Migrated from github.com) commented 2025-10-17 12:08:39 -05:00

I would expect memcpy (the bit I've added) to be faster than repeated use of MBEDTLS_GET_UINT32_LE.

In any case, note that this is part of mbedtls_chacha20_setkey which is not perf sensitive. It is a decent size saving though.

I would expect `memcpy` (the bit I've added) to be faster than repeated use of `MBEDTLS_GET_UINT32_LE`. In any case, note that this is part of `mbedtls_chacha20_setkey` which is not perf sensitive. It is a decent size saving though.
daverodgman (Migrated from github.com) reviewed 2025-10-17 12:20:36 -05:00
daverodgman (Migrated from github.com) commented 2025-10-17 12:20:36 -05:00

Note that the look-up table that describes how to re-order the data is a compile-time const - only the register it is shuffling is unknown. So I don't think this could result in data-dependent timing.

Note that the look-up table that describes how to re-order the data is a compile-time const - only the register it is shuffling is unknown. So I don't think this could result in data-dependent timing.
daverodgman (Migrated from github.com) reviewed 2025-10-17 12:47:13 -05:00
daverodgman (Migrated from github.com) commented 2025-10-17 12:47:13 -05:00

This was needed previously, but seems not necessary now.

This was needed previously, but seems not necessary now.
daverodgman (Migrated from github.com) reviewed 2025-10-17 13:07:55 -05:00
daverodgman (Migrated from github.com) commented 2025-10-17 13:07:55 -05:00

Hmm. In https://cr.yp.to/streamciphers/timings/estreambench/submissions/salsa20/chacha8/regs/chacha.c this is written using:

static const char sigma[16] = "expand 32-byte k";
constants = sigma;
x->input[0] = U8TO32_LITTLE(constants + 0);
x->input[1] = U8TO32_LITTLE(constants + 4);

ie treat the string as a bunch of little-endian 32-bit ints. So it will write "e" into the lowest address, then "x", and so on, regardless of endianness. So unless I've missed something, this is equivalent behaviour to memcpy.

It does look like previously, the constant would have been written differently on big-endian, which looks like a bug.

Hmm. In `https://cr.yp.to/streamciphers/timings/estreambench/submissions/salsa20/chacha8/regs/chacha.c` this is written using: ``` static const char sigma[16] = "expand 32-byte k"; constants = sigma; x->input[0] = U8TO32_LITTLE(constants + 0); x->input[1] = U8TO32_LITTLE(constants + 4); ``` ie treat the string as a bunch of little-endian 32-bit ints. So it will write "e" into the lowest address, then "x", and so on, regardless of endianness. So unless I've missed something, this is equivalent behaviour to `memcpy`. It does look like previously, the constant would have been written differently on big-endian, which looks like a bug.
daverodgman (Migrated from github.com) reviewed 2025-10-18 10:12:44 -05:00
daverodgman (Migrated from github.com) commented 2025-10-18 10:12:44 -05:00

Sorry, no, you were right. I've fixed, and tested on big-endian aarch64. On little-endian (ie the common case) the memcpy path saves 20 bytes so worth retaining this.

Sorry, no, you were right. I've fixed, and tested on big-endian aarch64. On little-endian (ie the common case) the `memcpy` path saves 20 bytes so worth retaining this.
yanesca (Migrated from github.com) reviewed 2025-10-20 08:00:35 -05:00
yanesca (Migrated from github.com) commented 2025-10-20 08:00:35 -05:00

Oh, I think I misread the code, so tbl_rotl8 actually contains the indices and v is the table from which we are performing the lookup. In that case we should indeed be safe.

Oh, I think I misread the code, so `tbl_rotl8` actually contains the indices and `v` is the table from which we are performing the lookup. In that case we should indeed be safe.
yanesca (Migrated from github.com) requested changes 2025-10-20 08:15:30 -05:00
yanesca (Migrated from github.com) left a comment

Hi Dave, thank you very much for reworking the PR, I had a look at it and I have two small last asks.

Hi Dave, thank you very much for reworking the PR, I had a look at it and I have two small last asks.
yanesca (Migrated from github.com) commented 2025-10-20 08:02:35 -05:00

Can we please rename this variable to idx_rotl8 or something similar, the naming really threw me off when reading the code.

Can we please rename this variable to `idx_rotl8` or something similar, the naming really threw me off when reading the code.
yanesca (Migrated from github.com) commented 2025-10-20 08:03:08 -05:00

Never mind, I misread the code, it should be safe.

Never mind, I misread the code, it should be safe.
yanesca (Migrated from github.com) commented 2025-10-20 08:11:45 -05:00

Do you think we could move the neon code into its own source file? It is easier to read/maintain like that and that is how we have been doing it with AES as well.

(The two implementations seem to be nicely separated, I would hope that this is not a very big change.)

Do you think we could move the neon code into its own source file? It is easier to read/maintain like that and that is how we have been doing it with AES as well. (The two implementations seem to be nicely separated, I would hope that this is not a very big change.)
davidhorstmann-arm (Migrated from github.com) reviewed 2025-10-20 11:40:44 -05:00
davidhorstmann-arm (Migrated from github.com) left a comment
No description provided.
LGTM, thanks! Happy with the small improvements, reviewed the refactor with `--color-moved`.
davidhorstmann-arm (Migrated from github.com) approved these changes 2025-10-21 04:40:47 -05:00
davidhorstmann-arm (Migrated from github.com) left a comment

LGTM, thanks!

LGTM, thanks!
yanesca (Migrated from github.com) approved these changes 2025-10-21 07:54:01 -05:00
yanesca (Migrated from github.com) left a comment

LGTM, thanks

LGTM, thanks
gilles-peskine-arm (Migrated from github.com) reviewed 2025-10-28 09:13:56 -05:00
@ -0,0 +66,4 @@
static inline uint32x4_t chacha20_neon_inc_counter(uint32x4_t v)
{
if (MBEDTLS_IS_BIG_ENDIAN) {
uint32x4_t counter_increment = { 0, 0, 0, 1 };
gilles-peskine-arm (Migrated from github.com) commented 2025-10-28 09:13:57 -05:00

This initializer is not portable. It works with GCC, Clang and IAR but not MSVC. @daverodgman would you mind reviewing my proposed fix? https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/552

[This initializer is not portable](https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/551). It works with GCC, Clang and IAR but not MSVC. @daverodgman would you mind reviewing my proposed fix? https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/552
Sign in to join this conversation.
No milestone
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-Mbed-TLS/TF-PSA-Crypto!159
No description provided.