Skip to content

Conversation

@minosgalanakis
Copy link
Contributor

Description

Migrates a series of components from the configuration-crypto on mbedtls to cmake. One of many pr's required for #10472

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: Changes the testing framework not the code itself
  • development PR provided here
  • TF-PSA-Crypto PR provided # TODO
  • framework PR not required
  • 3.6 PR provided # | not required because: Will not be backported
  • tests provided

@minosgalanakis minosgalanakis added needs-work needs-ci Needs to pass CI tests size-s Estimated task size: small (~2d) labels Nov 13, 2025
@ronald-cron-arm ronald-cron-arm changed the title Migrate componenets for configuration-crypto to cmake Migrate components for configuration-crypto to cmake Nov 17, 2025
@ronald-cron-arm ronald-cron-arm changed the title Migrate components for configuration-crypto to cmake Migrate configuration-crypto components to cmake Nov 17, 2025
@minosgalanakis minosgalanakis force-pushed the rework/component-configuration-crypto-cmake branch 2 times, most recently from 78aec57 to a39dc79 Compare November 19, 2025 11:24
@minosgalanakis minosgalanakis force-pushed the rework/component-configuration-crypto-cmake branch 3 times, most recently from 9a7e3aa to 006d758 Compare December 1, 2025 12:47
@minosgalanakis
Copy link
Contributor Author

minosgalanakis commented Dec 1, 2025

Force push to latest development --> diff

Squashing and grouping the commits for easier review --> diff to pre-rebase base

@minosgalanakis minosgalanakis force-pushed the rework/component-configuration-crypto-cmake branch 2 times, most recently from 35af1d8 to dab67bd Compare December 2, 2025 12:11
@minosgalanakis minosgalanakis 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 and removed needs-work needs-ci Needs to pass CI tests labels Dec 2, 2025
@minosgalanakis minosgalanakis force-pushed the rework/component-configuration-crypto-cmake branch from 51266d8 to 61f5ab1 Compare December 4, 2025 00:08
@ronald-cron-arm ronald-cron-arm self-requested a review December 9, 2025 10:37
@ronald-cron-arm ronald-cron-arm removed the needs-reviewer This PR needs someone to pick it up for review label Dec 9, 2025
@ronald-cron-arm ronald-cron-arm moved this from In Development to In Review in Roadmap pull requests (new board) Dec 11, 2025
Copy link
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.

This looks quit good to me. I have a few questions and suggestions. Regarding completeness, although we discuss to discard it at some point, we should be able to migrate test_psa_crypto_drivers as well I'd say.

@github-project-automation github-project-automation bot moved this from In Review to In Development in Roadmap pull requests (new board) Dec 12, 2025
@minosgalanakis
Copy link
Contributor Author

This looks quit good to me. I have a few questions and suggestions. Regarding completeness, although we discuss to discard it at some point, we should be able to migrate test_psa_crypto_drivers as well I'd say.

@ronald-cron-arm This is possible but not as straighforward right now.

As you have discovered in your own pr https://github.com/Mbed-TLS/mbedtls-framework/pull/243/files#diff-778df0488579c2de20bb280c3c142e099f065b8a3887e31ce3ed4462a06ff0af those utiility functions demonstrated in my now void pr need to be static, because the [-Werror,-Wmissing-prototypes] was not consistently propagated in the old build system but the new one with the cmake does it correctly.

To migrate the component_test_psa_crypto_drivers to cmake should be as simple as this
CC=$ASAN_CC cmake -D CMAKE_BUILD_TYPE:String=Asan -D CMAKE_C_FLAGS:STRING="${loc_cflags}" .
but now it needs:

The gains of migrating one component do not outweight the cost of trying to target a moving chain of commits.

ps. Currently tested with the latest mbedtls pointing to the latest tf-psa-crypto with the framework pointer and it does not work.

Alternatively I can write the cmake component now, and explicitly disable all the compilation flags that scream e.g loc_cflags="${loc_cflags} -Wno-error=missing-prototypes -Wno-missing-prototypes -Wno-error" and we clean it up in the future. wdyt?

@ronald-cron-arm
Copy link
Contributor

ronald-cron-arm commented Dec 18, 2025

This looks quit good to me. I have a few questions and suggestions. Regarding completeness, although we discuss to discard it at some point, we should be able to migrate test_psa_crypto_drivers as well I'd say.

@ronald-cron-arm This is possible but not as straighforward right now.

As you have discovered in your own pr https://github.com/Mbed-TLS/mbedtls-framework/pull/243/files#diff-778df0488579c2de20bb280c3c142e099f065b8a3887e31ce3ed4462a06ff0af those utiility functions demonstrated in my now void pr need to be static, because the [-Werror,-Wmissing-prototypes] was not consistently propagated in the old build system but the new one with the cmake does it correctly.

To migrate the component_test_psa_crypto_drivers to cmake should be as simple as this CC=$ASAN_CC cmake -D CMAKE_BUILD_TYPE:String=Asan -D CMAKE_C_FLAGS:STRING="${loc_cflags}" . but now it needs:

* A compatible tf-psa-crypto upate pointing to the framework at the [Add support for TF-PSA-Crypto test driver mbedtls-framework#243](https://github.com/Mbed-TLS/mbedtls-framework/pull/243)

* A compatible mbedtls update commit that is pointing at the tf-psa-crypto created above

* A rebase of this pr on that.

The gains of migrating one component do not outweight the cost of trying to target a moving chain of commits.

ps. Currently tested with the latest mbedtls pointing to the latest tf-psa-crypto with the framework pointer and it does not work.

Alternatively I can write the cmake component now, and explicitly disable all the compilation flags that scream e.g loc_cflags="${loc_cflags} -Wno-error=missing-prototypes -Wno-missing-prototypes -Wno-error" and we clean it up in the future. wdyt?

If you need to update the tf-psa-crypto and framework pointers and to rebase, please do it.
If after that we have an issue not related to your PR, then we have a problem that we will have to face sooner or later anyway.

Copy link
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.

This looks almost good to me. A few final comments.

msg "all loops unrolled"
$MAKE_COMMAND clean
make -C tests ../tf-psa-crypto/tests/test_suite_shax CFLAGS="-DMBEDTLS_SHA3_THETA_UNROLL=1 -DMBEDTLS_SHA3_PI_UNROLL=1 -DMBEDTLS_SHA3_CHI_UNROLL=1 -DMBEDTLS_SHA3_RHO_UNROLL=1"
CFLAGS="-DMBEDTLS_SHA3_THETA_UNROLL=1 -DMBEDTLS_SHA3_PI_UNROLL=1 -DMBEDTLS_SHA3_CHI_UNROLL=1 -DMBEDTLS_SHA3_RHO_UNROLL=1" cmake -D CMAKE_BUILD_TYPE:String=None .
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
CFLAGS="-DMBEDTLS_SHA3_THETA_UNROLL=1 -DMBEDTLS_SHA3_PI_UNROLL=1 -DMBEDTLS_SHA3_CHI_UNROLL=1 -DMBEDTLS_SHA3_RHO_UNROLL=1" cmake -D CMAKE_BUILD_TYPE:String=None .
CFLAGS="-DMBEDTLS_SHA3_THETA_UNROLL=1 -DMBEDTLS_SHA3_PI_UNROLL=1 -DMBEDTLS_SHA3_CHI_UNROLL=1 -DMBEDTLS_SHA3_RHO_UNROLL=1" cmake .

We have started to remove some "-D CMAKE_BUILD_TYPE:String=None", what about to remove them consistently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am happy to do it but again I need to reiterate that the decision tree choice to use CMAKE_BUILD_TYPE:String=None is when we want to test fine grained flag combinations. This component will work either way but felt like it was doing that.

@minosgalanakis minosgalanakis added the priority-high High priority - will be reviewed soon label Dec 18, 2025
@minosgalanakis minosgalanakis added the needs-reviewer This PR needs someone to pick it up for review label Dec 18, 2025
@minosgalanakis
Copy link
Contributor Author

If you need to update the tf-psa-crypto and framework pointers and to rebase, please do it. If after that we have an issue not related to your PR, then we have a problem that we will have to face sooner or later anyway.

So the answer to that is a yes & no. Yes we have an issue, but no our CI will not catch it, but my computer will. Also need to update the framework pointer in tf-psa-crypto to bring the changes that I discussed above.

Raised a PR that does both here Mbed-TLS/TF-PSA-Crypto#614

Migrate all straightfoward components from using $ASAN_CFLAGS
to CMAKE_BUILD_TYPE:String=Asan

Signed-off-by: Minos Galanakis <[email protected]>
…o cmake

Optimization for size (-Os) is required.

Signed-off-by: Minos Galanakis <[email protected]>
…r_accel_ec to cmake

Compilation flags, and spe include directories have been adjusted

Signed-off-by: Minos Galanakis <[email protected]>
…onents to cmake

- By default all unspecified build-type components should be release
- CMAKE_BUILD_TYPE:String=Release enables the following
  CFLAGS: "-O2 -Werror -Wall -Wextra"

Signed-off-by: Minos Galanakis <[email protected]>
…se components to cmake

Moved the following components to CMAKE_BUILD_TYPE:String=Release
and adjusted  the include paths for cmake:
* component_build_psa_crypto_spm
* component_test_tfm_config_no_p256m

Signed-off-by: Minos Galanakis <[email protected]>
…d_light_only to cmake

Use compilation directory for object discovery in out-of-source CMake builds.

Signed-off-by: Minos Galanakis <[email protected]>
…_BUILD_TYPE:String=None

Improve compilation flag granularity by disabling CMAKE_BUILD_TYPE defaults
and asserting test-specific flags manually.

Signed-off-by: Minos Galanakis <[email protected]>
The original make -C tests, contains a perl inliner
to generate the alt-headers. Replicated that logic in
sed regex.

Signed-off-by: Minos Galanakis <[email protected]>
Update the previously modified component to use
consistent syntax:
* make -> cmake --build .
* make test -> ctest
* Removed redudant CC=$ASAN_CC for BUILD_TYPE:String=Asan

Signed-off-by: Minos Galanakis <[email protected]>
build_psa_alt_headers will now generate the headers at
./tests/include/alt-dummy instead of
./framework/tests/include/alt-extra.

Signed-off-by: Minos Galanakis <[email protected]>
…f_source_directory for test_crypto_full_md_light_only

Signed-off-by: Minos Galanakis <[email protected]>
@minosgalanakis minosgalanakis force-pushed the rework/component-configuration-crypto-cmake branch from 1025f2b to c0fbb16 Compare December 18, 2025 17:39
@minosgalanakis
Copy link
Contributor Author

Addressed @ronald-cron-arm's comments at 1025f2b and then rebased on top of head of public/development.

The only outstandading comment which I did not push an update for is the discussion about CMAKE_BUILD_TYPE:String=Release vs None mentioned 1, 2

@ronald-cron-arm
Copy link
Contributor

ronald-cron-arm commented Dec 19, 2025

Addressed @ronald-cron-arm's comments at 1025f2b and then rebased on top of head of public/development.

The only outstandading comment which I did not push an update for is the discussion about CMAKE_BUILD_TYPE:String=Release vs None mentioned 1, 2

My last #10507 (comment) was not about cmake -DCMAKE_BUILD_TYPE:String=Release vs cmake -DCMAKE_BUILD_TYPE:String=None but cmake -DCMAKE_BUILD_TYPE:String=None vs cmake. cmake -DCMAKE_BUILD_TYPE:String=None and just cmake are equivalent and cmake is mostly used in this PR, like in CFLAGS="-O1" cmake ., or CFLAGS="-O1 -I$PWD/framework/tests/include/baremetal-override/" cmake . ... Thus I was proposing to replace the cmake -DCMAKE_BUILD_TYPE:String=None we introduce in this PR by just "cmake".

@ronald-cron-arm ronald-cron-arm self-requested a review December 19, 2025 13:23
Copy link
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.

Thanks for addressing my comments. My last comment #10507 (comment) is not a blocker thus I am approving as it is. Regarding test_psa_crypto_drivers that was discussed at some point I can see that it would need some changes in the framework and TF-PSA-Crypto. Thus better to address that one separately.

@minosgalanakis
Copy link
Contributor Author

Thanks @ronald-cron-arm for the approval and the clarification for the 1025f2b comment. I will prepare a fix and upload it later today if it has no been reviewed yet, or I will move it to one of the follow up prs, if this one gets approved today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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-s Estimated task size: small (~2d)

Projects

Development

Successfully merging this pull request may close these issues.

2 participants