Skip to content

[tf-psa-crypto] Move benchmark program to TF-PSA-Crypto#176

Merged
ronald-cron-arm merged 6 commits intoMbed-TLS:developmentfrom
valeriosetti:issue9971-tfpsacrypto
Feb 21, 2025
Merged

[tf-psa-crypto] Move benchmark program to TF-PSA-Crypto#176
ronald-cron-arm merged 6 commits intoMbed-TLS:developmentfrom
valeriosetti:issue9971-tfpsacrypto

Conversation

@valeriosetti
Copy link
Copy Markdown
Contributor

@valeriosetti valeriosetti commented Feb 17, 2025 •
edited by ronald-cron-arm
Loading

Description

This is part of Mbed-TLS/mbedtls#9986

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.

@valeriosetti valeriosetti mentioned this pull request Feb 17, 2025
Merged
6 tasks
@valeriosetti valeriosetti self-assigned this Feb 17, 2025
@valeriosetti valeriosetti added 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 priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most) needs-ci Needs to pass CI tests labels Feb 17, 2025
Harry-Ramsey
Harry-Ramsey previously approved these changes Feb 17, 2025
View reviewed changes
@valeriosetti
Copy link
Copy Markdown
Contributor Author

valeriosetti commented Feb 17, 2025

Failures in the CI are due to the fact that tf-psa-crypto checks out main repo at development, but in that branch the benchmark program is still built from the main repo, so this results in duplication

CMake Error at programs/test/CMakeLists.txt:85 (add_executable):
  add_executable cannot create target "benchmark" because another target with
  the same name already exists.  The existing target is an executable created
  in source directory "/var/lib/build/tf-psa-crypto/programs/test".

I suggest to use CI of Mbed-TLS/mbedtls#9986 as reference.
IMO this means that we should merge Mbed-TLS/mbedtls#9986 first and then this one, but the problem is that Mbed-TLS/mbedtls#9986 also updates the tf-psa-crypto reference. What's the suggested way to proceed here?

@ronald-cron-arm
Copy link
Copy Markdown
Contributor

ronald-cron-arm commented Feb 17, 2025

Our usual procedure: validate this PR with the CI of Mbed-TLS/mbedtls#9986, merge this PR, update the TF-PSA-Crypto pointer in Mbed-TLS/mbedtls#9986, merge the Mbed TLS PR.

@valeriosetti
Copy link
Copy Markdown
Contributor Author

valeriosetti commented Feb 18, 2025

Our usual procedure: validate this PR with the CI of Mbed-TLS/mbedtls#9986, merge this PR, update the TF-PSA-Crypto pointer in Mbed-TLS/mbedtls#9986, merge the Mbed TLS PR.

OK, so we can override the CI and forcibly merge the PR also with some failure, good to know. Since Mbed-TLS/mbedtls#9986 has a fully green CI, I think this PR is ready for reviews then :)

@valeriosetti valeriosetti removed the needs-ci Needs to pass CI tests label Feb 18, 2025
ronald-cron-arm
ronald-cron-arm requested changes Feb 19, 2025
View reviewed changes
@valeriosetti
programs: move benchmark to tf-psa-crypto
23f5211 
The full history of the file is still available via 'git log --follow'.

This commit also add the CMakeList.txt file necessary to build the
benchmark program. This is mostly copied from
"programs/psa/CMakeLists.txt" and allows other test programs to be
easily added in "programs/test" folder.

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
@valeriosetti
programs: update .gitignore
645ba3b 
Add a new entry for benchmark program since it was moved from the
main repo to this one.

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
@valeriosetti
scripts: copy ecc-heap.sh from main repo
94156ed 
Now that the benchmark program has been moved to tf-psa-crypto, also
ecc-heap.sh can be moved in this repo.

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
@valeriosetti
scripts: adapt ecc-heap to tf-psa-crypto repo
f072f58 
Fix paths after the file was moved in this repo.

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
@valeriosetti
scripts: ecc-heap: add missing defines
3ab90d6 
In TF-PSA-Crypto MBEDTLS_USE_PSA_CRYPTO is assumed to be always enabled.
This means that a crypto backend such as MBEDTLS_PSA_CRYPTO_C must be
defined in order to build successfully. CRYPTO_C brings in other
requirements for what concerns entropy generator.

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
@valeriosetti
scripts: ecc-heap: fix script permissions
5785c87 
Signed-off-by: Valerio Setti <vsetti@baylibre.com>
ronald-cron-arm
ronald-cron-arm approved these changes Feb 20, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ronald-cron-arm
Copy link
Copy Markdown
Contributor

ronald-cron-arm commented Feb 21, 2025

This PR cannot be validated by the TF-PSA-Crypto CI as it is incompatible with mbedtls:development. It has been validated by the CI of Mbed-TLS/mbedtls#9986 thus I am merging it.

@ronald-cron-arm ronald-cron-arm merged commit 2cfed8e into Mbed-TLS:development Feb 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ronald-cron-arm ronald-cron-arm ronald-cron-arm approved these changes

+1 more reviewer

@Harry-Ramsey Harry-Ramsey Harry-Ramsey approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@valeriosetti valeriosetti

Labels

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 priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most)

Projects

Roadmap pull requests (new board)
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@valeriosetti @ronald-cron-arm @Harry-Ramsey