gh-Mbed-TLS/TF-PSA-Crypto
Watch
1
0
Fork
You've already forked TF-PSA-Crypto
 0
Code Issues 167 Pull requests 92 Releases 5 Activity

Use instruction-level parallelism with AES-NI to speed up AES-CTR #573

Open
mfil wants to merge 8 commits from mfil/feature/aes_ni_parallel into development
pull from: mfil/feature/aes_ni_parallel
merge into: gh-Mbed-TLS:development
Conversation 7 Commits 8 Files changed 4 +501 -68
mfil commented 2025-11-13 06:39:33 -06:00 (Migrated from github.com)

Description

One feature of the AES-NI instructions is instruction-level parallelism, which means that, while one round of AES takes multiple clock cycles to complete, another instruction can already start executing on the next clock cycle. Only when the result of the AES round is used do we need to wait for it to finish.

In particular, this makes it possible to encrypt or decrypt multiple AES blocks in parallel which can be used to speed up AES in some modes of operation. In this commit, it is used to speed up AES-CTR. I have chosen to do up to four blocks in parallel. More would be possible, but while testing on my machine, I didn't see meaningful speed-ups when going beyond 4, and I don't want to clutter the source files with even more assembly.

I have removed the defines with the aesenc etc. opcodes assuming that we don't need to support these older versions of binutils anymore. If that's wrong, I'll put them back (and add my own macros for the registers I need to use).

I would like to go on and also use it in AES-GCM, but there I ran into a problem. GCM runs the underlying block cipher in ECB mode. Due to an inconsistency in the mbedtls_cipher API, ECB mode always encrypts exactly one block. As far as I understand, mbedtls_cipher is on its way out, so I don't know if you would want to spend time reviewing a fix for this. Please let me know what you think would be the best approach!

An alternative I can see would be to use CTR mode (making sure that the 32-bit counter in GCM doesn't overflow) inside of GCM, if available.

PR checklist

  • changelog provided
  • framework PR not required
  • mbedtls development PR not required because: No changes in mbedtls are needed to make use of this. The speed-up will take effect as soon as the submodule is updated. (Should I make a pull request just to update the submodule?)
  • mbedtls 3.6 PR not required because: It's a new feature, not a bugfix. It doesn't have to be backported. (As far as I'm concerned.)
  • tests not required because: No changes in the output are intended, only performance.
## Description One feature of the AES-NI instructions is instruction-level parallelism, which means that, while one round of AES takes multiple clock cycles to complete, another instruction can already start executing on the next clock cycle. Only when the result of the AES round is used do we need to wait for it to finish. In particular, this makes it possible to encrypt or decrypt multiple AES blocks in parallel which can be used to speed up AES in some modes of operation. In this commit, it is used to speed up AES-CTR. I have chosen to do up to four blocks in parallel. More would be possible, but while testing on my machine, I didn't see meaningful speed-ups when going beyond 4, and I don't want to clutter the source files with even more assembly. I have removed the `defines` with the `aesenc` etc. opcodes assuming that we don't need to support these older versions of binutils anymore. If that's wrong, I'll put them back (and add my own macros for the registers I need to use). I would like to go on and also use it in AES-GCM, but there I ran into a problem. GCM runs the underlying block cipher in ECB mode. Due to an inconsistency in the `mbedtls_cipher` API, ECB mode always encrypts exactly one block. As far as I understand, `mbedtls_cipher` is on its way out, so I don't know if you would want to spend time reviewing a fix for this. Please let me know what you think would be the best approach! An alternative I can see would be to use CTR mode (making sure that the 32-bit counter in GCM doesn't overflow) inside of GCM, if available. ## PR checklist - [x] **changelog** provided - [x] **framework PR** not required - [x] **mbedtls development PR** not required because: No changes in mbedtls are needed to make use of this. The speed-up will take effect as soon as the submodule is updated. (Should I make a pull request just to update the submodule?) - [x] **mbedtls 3.6 PR** not required because: It's a new feature, not a bugfix. It doesn't have to be backported. (As far as I'm concerned.) - **tests** not required because: No changes in the output are intended, only performance.
mfil commented 2025-11-18 06:00:09 -06:00 (Migrated from github.com)

It says

TF OpenCI: Interface stability tests — Pre-test checks failed.

but when I click the link to view details, I see a green check on archive-timestamps and everything else is blank.

It says > [TF OpenCI: Interface stability tests](https://mbedtls.trustedfirmware.org/job/mbed-tls-tf-psa-crypto-multibranch/job/PR-573-merge/5/display/redirect) — Pre-test checks failed. but when I click the link to view details, I see a green check on archive-timestamps and everything else is blank.
bjwtaylor commented 2025-11-18 07:43:44 -06:00 (Migrated from github.com)

Hi @mfil, thanks for submitting this PR. It looks like that job has failed due to infrastructure issues, so I would ignore them for the time being. I'm trying to get them resolved and will let you know when it's working again.

Hi @mfil, thanks for submitting this PR. It looks like that job has failed due to infrastructure issues, so I would ignore them for the time being. I'm trying to get them resolved and will let you know when it's working again.
mfil commented 2025-11-18 13:24:23 -06:00 (Migrated from github.com)

Hi @bjwtaylor ! Thanks, I'll wait.

In the meantime, I found that the functions with the intrinsics may not actually improve performance depending on the compiler. What I hoped would happen is that with optimizations turned on, the compiler would unroll the loops and then pipeline the aesenc instructions. Clang 18.1.3 does so, GCC 13.3.0 does not.

I tried hand-unrolling the loops, and then I get the speed-ups also with GCC. Should I commit that?

Hi @bjwtaylor ! Thanks, I'll wait. In the meantime, I found that the functions with the intrinsics may not actually improve performance depending on the compiler. What I *hoped* would happen is that with optimizations turned on, the compiler would unroll the loops and then pipeline the `aesenc` instructions. Clang 18.1.3 does so, GCC 13.3.0 does not. I tried hand-unrolling the loops, and then I get the speed-ups also with GCC. Should I commit that?
bjwtaylor commented 2025-11-19 02:36:07 -06:00 (Migrated from github.com)

@mfil, the PR will need to go through a full review process. In the meantime though I would add anything you think is useful. I've now resolved some of the CI issues, please ignore the OpenCI issues and any failures in the NewCI should be related to your changes.

@mfil, the PR will need to go through a full review process. In the meantime though I would add anything you think is useful. I've now resolved some of the CI issues, please ignore the OpenCI issues and any failures in the NewCI should be related to your changes.
mfil commented 2025-11-19 08:56:02 -06:00 (Migrated from github.com)

Ok, I added another commit that unrolls the "4blocks" function.

Ok, I added another commit that unrolls the "4blocks" function.
mfil commented 2025-11-20 12:18:27 -06:00 (Migrated from github.com)

I fixed the error in tf_psa_crypto_check_changelog.

I figured out what's going wrong in test_aesni, but not what would be a good way to fix it. It checks that the binary aes.o contains the string mbedtls_internal_aes_encrypt. That function is declared static, and my changes in aes.c seem to cause the compiler in your test container to inline it.

For the failure on aarch64, it's not obvious to me what's going wrong. I guess I have to go play with QEMU.

I fixed the error in `tf_psa_crypto_check_changelog`. I figured out what's going wrong in `test_aesni`, but not what would be a good way to fix it. It checks that the binary `aes.o` contains the string `mbedtls_internal_aes_encrypt`. That function is declared static, and my changes in `aes.c` seem to cause the compiler in your test container to inline it. For the failure on aarch64, it's not obvious to me what's going wrong. I guess I have to go play with QEMU.
mfil commented 2025-11-20 12:42:01 -06:00 (Migrated from github.com)

After looking at it for the 10th time, I think I spotted the bug with aarch64.

After looking at it for the 10th time, I think I spotted the bug with aarch64.
This pull request is broken due to missing fork information.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin 'mfil/feature/aes_ni_parallel:mfil/feature/aes_ni_parallel'
git switch 'mfil/feature/aes_ni_parallel'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'development'
git merge --no-ff 'mfil/feature/aes_ni_parallel'
git switch 'mfil/feature/aes_ni_parallel'
git rebase 'development'
git switch 'development'
git merge --ff-only 'mfil/feature/aes_ni_parallel'
git switch 'mfil/feature/aes_ni_parallel'
git rebase 'development'
git switch 'development'
git merge --no-ff 'mfil/feature/aes_ni_parallel'
git switch 'development'
git merge --squash 'mfil/feature/aes_ni_parallel'
git switch 'development'
git merge --ff-only 'mfil/feature/aes_ni_parallel'
git switch 'development'
git merge 'mfil/feature/aes_ni_parallel'
git push origin 'development'
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
api-break
This issue/PR breaks the API and must wait for a new major version

  
approved
Design and code approved - may be waiting for CI or backports

  
bug
Something isn't working

  
component-docs
Docs / website issues filed here for tracking

  
component-platform
Portability layer and build scripts

  
component-test
Test framework and CI scripts

  
dependencies
Pull requests that update a dependency file

  
deprecation
needs deprecation announcement

  
DO-NOT-MERGE

   
enhancement
New feature or request

  
good first issue
Good for newcomers

  
help wanted
This issue is not being actively worked on, but PRs welcome.

  
needs-adoption
stalled PR that someone should pick up and complete

  
needs-backports
Backports are missing or are pending review and approval.

  
needs-ci
Needs to pass CI tests

  
needs-design-approval
Needs design discussion / approval

  
needs-info
An issue or PR which needs further info from the reporter / author

  
needs-preceding-pr
Requires another PR to be merged first

  
needs-review
Every commit must be reviewed by at least two team members

  
needs-reviewer
This PR needs someone to pick it up for review

  
needs-work

   
priority-high
High priority - will be reviewed soon

  
priority-low
Low priority - this may not receive review soon

  
priority-medium
Medium priority - this can be reviewed as time permits

  
priority-scheduled
This PR is big - it will require time to be scheduled for review

  
priority-very-high
Highest priority - prioritise this over other review work

  
resolved-after-1.1
The issue is resolved in development, but still present in 1.1 LTS.

  
single-reviewer
This PR qualifies for having only one reviewer

  
size-l
Estimated task size: large (2w+)

  
size-m
Estimated task size: medium (~1w)

  
size-optimisation

   
size-s
Estimated task size: small (~2d)

  
size-xs
Estimated task size: extra small (a few hours at most)

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
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
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!573
No description provided.