Neon impl of ChaCha20 (better size & perf) #159
No reviewers
Labels
No labels
api-break
approved
bug
component-docs
component-platform
component-test
dependencies
deprecation
DO-NOT-MERGE
enhancement
good first issue
help wanted
needs-adoption
needs-backports
needs-ci
needs-design-approval
needs-info
needs-preceding-pr
needs-review
needs-reviewer
needs-work
priority-high
priority-low
priority-medium
priority-scheduled
priority-very-high
resolved-after-1.1
single-reviewer
size-l
size-m
size-optimisation
size-s
size-xs
No milestone
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gh-Mbed-TLS/TF-PSA-Crypto!159
Loading…
Reference in a new issue
No description provided.
Delete branch "chacha-neon-split2"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.
Thanks for moving your PR to the new repo.
This looks good to me generally. Could you add a testcase in
tests/suites/test_suite_chacha20.datathat depends onMBEDTLS_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.Note to self: The
TBLinstruction performed here has data-independent timing when DIT is set (source)It also looks like this could do with a rebase on top of latest for the CI's benefit.
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.
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.
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.
I believe with CMake the setting of
CFLAGSmust be done at configure time so the following loop should work instead:Needs: 1 small rework, final round of first reviewer and review from second reviewer.
Thanks @davidhorstmann-arm for help with cmake - I've verified locally that this builds the different variations as intended.
rebased onto latest development
Hi Dave, thank you for the PR, I had a look and I have a few questions.
I think this raises two questions:
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 am a bit concerned on the DIT bit requirement for this to be constant time. Do you think we could go for the
elsebranch for now and add this part back when we managed to sort out our thoughts around DIT?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));nit (for readability):
@ -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 */Is there a reason for doing this check in runtime?
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_UNALIGNEDandUINT_UNALIGNED_STRUCTas well.Same as above.
I am sorry, am not very familiar with the new config, why do we need
PSA_WANT_KEY_TYPE_AESandPSA_WANT_ALG_SHA_256for ChaCha?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.
@ -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 */Because
MBEDTLS_IS_BIG_ENDIANis not guaranteed to be compile-time const. On just about every reasonable compiler, it is constant, and the branch is optimised away.@ -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 */I would expect
memcpy(the bit I've added) to be faster than repeated use ofMBEDTLS_GET_UINT32_LE.In any case, note that this is part of
mbedtls_chacha20_setkeywhich is not perf sensitive. It is a decent size saving though.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.
This was needed previously, but seems not necessary now.
Hmm. In
https://cr.yp.to/streamciphers/timings/estreambench/submissions/salsa20/chacha8/regs/chacha.cthis is written using: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.
Sorry, no, you were right. I've fixed, and tested on big-endian aarch64. On little-endian (ie the common case) the
memcpypath saves 20 bytes so worth retaining this.Oh, I think I misread the code, so
tbl_rotl8actually contains the indices andvis the table from which we are performing the lookup. In that case we should indeed be safe.Hi Dave, thank you very much for reworking the PR, I had a look at it and I have two small last asks.
Can we please rename this variable to
idx_rotl8or something similar, the naming really threw me off when reading the code.Never mind, I misread the code, it should be safe.
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.)
LGTM, thanks!
LGTM, thanks
@ -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 };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