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 <minos.galanakis@arm.com>
…o cmake

Optimization for size (-Os) is required.

Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
…r_accel_ec to cmake

Compilation flags, and spe include directories have been adjusted

Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
…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 <minos.galanakis@arm.com>
…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 <minos.galanakis@arm.com>
…d_light_only to cmake

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

Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
…_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 <minos.galanakis@arm.com>
Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
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 <minos.galanakis@arm.com>
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 <minos.galanakis@arm.com>
Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
…needed.

Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
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 <minos.galanakis@arm.com>
…f_source_directory for test_crypto_full_md_light_only

Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
@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.

…BUILD_TYPE=None

Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
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