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

Bugfix/fix intrinsic header for arm architecture #614

Merged
minosgalanakis merged 1 commit from bugfix/fix-intrisic-header-armf into development 2026-01-22 11:15:30 -06:00
Conversation 15 Commits 1 Files changed 2 +20 -14
minosgalanakis commented 2025-12-18 05:12:06 -06:00 (Migrated from github.com)

Description

Currently the development branch fails to compile in arm silicone because of how the arm intrisics macros are not properly defined. This pr changes the inclusion order of tf_psa_crypto_common.h after they have been defined, in affected source files.

It also updates to the head of framework to bring in other related bugfixes in the tf-psa-crypto branch.

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 not required because: the bug that is fixed by this PR has not been released
  • framework PR not required
  • mbedtls development PR not required because: The crypto files have been moved to TF-PSA-Crypto
  • mbedtls 3.6 PR provided Mbed-TLS/mbedtls#10542
  • tests provided | not required because:
## Description Currently the development branch fails to compile in arm silicone because of how the arm intrisics macros are not properly defined. This pr changes the inclusion order of `tf_psa_crypto_common.h` after they have been defined, in affected source files. It also updates to the head of framework to bring in other related bugfixes in the tf-psa-crypto branch. ## 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. - [x] **changelog** not required because: the bug that is fixed by this PR has not been released - [x] **framework PR** not required - [x] **mbedtls development PR** not required because: The crypto files have been moved to TF-PSA-Crypto - [x] **mbedtls 3.6 PR** provided Mbed-TLS/mbedtls#10542 - **tests** provided | not required because:
ronald-cron-arm commented 2025-12-18 05:19:55 -06:00 (Migrated from github.com)

If you base your PR on current development, you won't need the framework pointer update. It is already there.

If you base your PR on current development, you won't need the framework pointer update. It is already there.
gilles-peskine-arm (Migrated from github.com) reviewed 2025-12-18 05:29:49 -06:00
drivers/builtin/src/aesce.c
gilles-peskine-arm (Migrated from github.com) commented 2025-12-18 05:29:49 -06:00

The fix for aesce.c is already approved in https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/603.

But I'd missed that sha256.c was also affected, thanks.

The fix for `aesce.c` is already approved in https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/603. But I'd missed that `sha256.c` was also affected, thanks.
minosgalanakis commented 2025-12-18 09:59:48 -06:00 (Migrated from github.com)

Latest force push was a rebase to latest development.

Latest force [push](https://github.com/Mbed-TLS/TF-PSA-Crypto/compare/3429951e008f6df1a4190b8fff620fa03dcfe184..d17897bac6e9c4aa5d570f1f9d07011b263ce2d8) was a rebase to latest development.
ronald-cron-arm (Migrated from github.com) requested changes 2025-12-18 11:01:03 -06:00
drivers/builtin/src/sha512.c
@ -27,1 +22,3 @@
* at the top of this file, before any includes.
* at the top of this file, before any includes but after the intrinsic
* declaration. This is necessary with
* Clang <=15.x. With Clang 16.0 and above, these macro definitions are
ronald-cron-arm (Migrated from github.com) commented 2025-12-18 11:00:20 -06:00

"at the top of this file, before any includes."

That's not true. #include "tf_psa_crypto_common.h" is before.

"at the top of this file, before any includes." That's not true. `#include "tf_psa_crypto_common.h"` is before.
gilles-peskine-arm (Migrated from github.com) requested changes 2025-12-22 14:04:30 -06:00
gilles-peskine-arm (Migrated from github.com) left a comment

sha512.c has the same problem as sha256.c.

Maybe a better fix would be to move the #define __ARM_FEATURE_xxx definitions to tf_psa_crypto_common.h (common.h in 3.6), before it includes <arm_neon.h>? That would seem like the natural approach to me, but I don't know why it wasn't done that way originally.

`sha512.c` has the same problem as `sha256.c`. Maybe a better fix would be to move the `#define __ARM_FEATURE_xxx` definitions to `tf_psa_crypto_common.h` (`common.h` in 3.6), before it includes `<arm_neon.h>`? That would seem like the natural approach to me, but I don't know why it wasn't done that way originally.
drivers/builtin/src/sha512.c
@ -27,1 +22,3 @@
* at the top of this file, before any includes.
* at the top of this file, before any includes but after the intrinsic
* declaration. This is necessary with
* Clang <=15.x. With Clang 16.0 and above, these macro definitions are
gilles-peskine-arm (Migrated from github.com) commented 2025-12-22 14:02:21 -06:00

But it shouldn't be, that's the point.

But it shouldn't be, that's the point.
minosgalanakis (Migrated from github.com) reviewed 2025-12-23 14:46:21 -06:00
minosgalanakis commented 2025-12-23 14:48:31 -06:00 (Migrated from github.com)

sha512.c has the same problem as sha256.c.

Maybe a better fix would be to move the #define __ARM_FEATURE_xxx definitions to tf_psa_crypto_common.h (common.h in 3.6), before it includes <arm_neon.h>? That would seem like the natural approach to me, but I don't know why it wasn't done that way originally.

I have moved the include in sha512 for now and rebase on top of latest development to pick up your changes. I am preparing a patch to test moving it to tf_psa_crypto_common but I could not get it to work on my computer as of now. I will update the pr as soon as its done.

> `sha512.c` has the same problem as `sha256.c`. > > Maybe a better fix would be to move the `#define __ARM_FEATURE_xxx` definitions to `tf_psa_crypto_common.h` (`common.h` in 3.6), before it includes `<arm_neon.h>`? That would seem like the natural approach to me, but I don't know why it wasn't done that way originally. I have moved the include in sha512 for now and rebase on top of latest development to pick up your changes. I am preparing a patch to test moving it to tf_psa_crypto_common but I could not get it to work on my computer as of now. I will update the pr as soon as its done.
minosgalanakis commented 2026-01-09 10:12:22 -06:00 (Migrated from github.com)

I have done another re-base and no other changes since the previous one. Changes github.com/Mbed-TLS/TF-PSA-Crypto@3aa5836893..577f51c846 are rebased content.

I will be following up with some changes as requested above

I have done another re-base and no other changes since the previous one. Changes https://github.com/Mbed-TLS/TF-PSA-Crypto/compare/3aa58368936dde5001015db92a1d8a22c7bfa478..577f51c846987b786601c42adecf5791565976b5 are rebased content. I will be following up with some changes as requested above
minosgalanakis commented 2026-01-12 10:01:13 -06:00 (Migrated from github.com)

Push to github.com/Mbed-TLS/TF-PSA-Crypto@e48c6ec38c is a simple rebase.

Push to https://github.com/Mbed-TLS/TF-PSA-Crypto/commit/e48c6ec38c433e30696a34f5b079f93473fd44b1 is a simple rebase.
gilles-peskine-arm (Migrated from github.com) reviewed 2026-01-13 09:17:15 -06:00
gilles-peskine-arm (Migrated from github.com) left a comment
No description provided.
LGTM
ronald-cron-arm (Migrated from github.com) reviewed 2026-01-14 09:01:02 -06:00
ronald-cron-arm (Migrated from github.com) left a comment
No description provided.
LGTM
minosgalanakis commented 2026-01-15 02:59:44 -06:00 (Migrated from github.com)

Added commit https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/614/commits/a1d8494067c904b44c5325991c9b9b7b141436eb which is implementing what @gilles-peskine-arm was asking at https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/614#pullrequestreview-3605551266.

I clean-up the the include logic, but this commit is a quality of life improvement and I could undo it if you do not feel it is necessary.

Added commit https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/614/commits/a1d8494067c904b44c5325991c9b9b7b141436eb which is implementing what @gilles-peskine-arm was asking at https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/614#pullrequestreview-3605551266. I clean-up the the include logic, but this commit is a quality of life improvement and I could undo it if you do not feel it is necessary.
minosgalanakis commented 2026-01-15 08:51:38 -06:00 (Migrated from github.com)

Latest head is identical to the previously approved head -> link . You can check the comparison -> link

The proposed change by github.com/Mbed-TLS/TF-PSA-Crypto@93153586af will be moved to a separate pr because it needs a design-aproved change on how we handle check-names.py

I also cleaned up the commit stack since the change is minimal.

Latest head is identical to the previously approved head -> [link ](https://github.com/Mbed-TLS/TF-PSA-Crypto/commit/c9a17193020cdda59109b2a1e57b946ad1a8b4e2) . You can check the comparison -> [link](https://github.com/Mbed-TLS/TF-PSA-Crypto/compare/c9a17193020cdda59109b2a1e57b946ad1a8b4e2..c8f1f771ae61079b70955f8b661d6c612afedd93) The proposed change by https://github.com/Mbed-TLS/TF-PSA-Crypto/commit/93153586af016cf7316d625c754db94d2fa3e792 will be moved to a separate pr because it needs a design-aproved change on how we handle `check-names.py` I also cleaned up the commit stack since the change is minimal.
ronald-cron-arm (Migrated from github.com) approved these changes 2026-01-19 05:32:48 -06:00
ronald-cron-arm (Migrated from github.com) left a comment

LGTM

LGTM
gilles-peskine-arm (Migrated from github.com) approved these changes 2026-01-19 11:36:35 -06:00
gilles-peskine-arm (Migrated from github.com) left a comment

LGTM (identical content to c9a17193020cdda59109b2a1e57b946ad1a8b4e2 which I previously approved)

LGTM (identical content to c9a17193020cdda59109b2a1e57b946ad1a8b4e2 which I previously approved)
Sign in to join this conversation.
Reviewers
No reviewers
ronald-cron-arm
gilles-peskine-arm
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!614
No description provided.