Clean up build_info.h after 1.0 #571
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gilles-peskine-arm
added
size-s
github-project-automation
bot
moved this to In Development
in Roadmap pull requests (new board)
3 tasks
gilles-peskine-arm
force-pushed
the
build_info-cleanup-1.0
branch
4 times, most recently
from
d353f00 to
1f55061
Compare
gilles-peskine-arm
added
needs-review
ronald-cron-arm
removed
the
needs-reviewer
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
ronald-cron-arm
requested changes
Nov 19, 2025
Contributor
ronald-cron-arm
left a comment
ronald-cron-arm
left a comment
There was a problem hiding this comment.
Thanks for this nice clean-up. Thanks for the commit message that helped a lot reviewing the changes. I have some minor comments/questions.
include/tf-psa-crypto/private/crypto_adjust_config_dependencies.h
Outdated
Show resolved
Hide resolved
drivers/builtin/include/mbedtls/private/crypto_adjust_config_enable_builtins.h
Show resolved
Hide resolved
drivers/builtin/include/mbedtls/private/crypto_adjust_config_tweak_builtins.h
Show resolved
Hide resolved
Contributor
|
One minor comment remaining. Otherwise it looks good to me. |
github-project-automation
bot
moved this from In Development
to Has Approval
in Roadmap pull requests (new board)
ronald-cron-arm
added
approved
…nyms.h Signed-off-by: Gilles Peskine <[email protected]>
Individual headers or C files are supposed to include what they need. Signed-off-by: Gilles Peskine <[email protected]>
This is changing PSA configurations, based on conditions that are not driver-specific, so keep it in `/include`. The file has guards based on whether some key derivation algorithms are accelerated, but these guards are currently irrelevant since key derivation drivers are not supported yet. This is not meant to be included directly by applications, so move it under `.../private`. Signed-off-by: Gilles Peskine <[email protected]>
This is at the PSA level, not driver-specific, so keep it in `/include`. This is not meant to be included directly by applications, so move it under `.../private`. Signed-off-by: Gilles Peskine <[email protected]>
This commit changes the location of the file in the inclusion order, to group it with other headers that tweak the PSA configuration. The former order also worked because the macros that it references aren't used in other configuration-tweaking headers at this time (and have never been, I believe), so this is not a bug fix, just a readability and robustness improvement. This is at the PSA level, not driver-specific, so keep it in `/include`. This is not meant to be included directly by applications, so move it under `.../private`. Rename the file to match the pattern followed by similar files (I had forgotten a word when I created that file initially). Signed-off-by: Gilles Peskine <[email protected]>
This commit changes the location of the file in the inclusion order, to group it with other headers that tweak the PSA configuration. Specificaly, this header is now included before `config_adjust_legacy_from_psa.h` and `config_adjust_test_accelerators.h`. Both of these set driver-specific macros based on `PSA_WANT` macros, whereas `crypto_adjust_config_derived.h` does not query driver-specific macros, so the new order is no worse. In fact the new order more correct because it would make sense for `config_adjust_legacy_from_psa.h` to query macros set by `crypto_adjust_config_derived.h`. However, it currently doesn't, so this is not a bug fix, just a robustness improvement. This is at the PSA level, not driver-specific, so keep it in `/include`. This is not meant to be included directly by applications, so move it under `.../private`. Rename the file to match the pattern followed by similar files (I had forgotten a word when I created that file initially). Signed-off-by: Gilles Peskine <[email protected]>
Historically, we did config ajustments in a separate header `config_psa.h`. Then we split `config_psa.h` into pieces. Today, having `config_psa.h` as a layer of indirection in header inclusions is no longer useful. So remove it. Signed-off-by: Gilles Peskine <[email protected]>
Rename the file to match the pattern followed by similar files, and to better reflect the role of the file in the context of TF-PSA-Crypto. It's no longer about enabling legacy interfaces, since low-level crypto is mostly no longer part of a public interface, but about enabling internal interfaces that implement actual cryptography. Signed-off-by: Gilles Peskine <[email protected]>
This is specific to modules that are under `/drivers`, so keep it under `/drivers`. Some of these modules are also public interfaces that are used in builds with only third-party drivers, so the corresponding macros should be outside of `/drivers`; this will be handled in a subsequent commit. This is not meant to be included directly by applications, so move it under `.../private`. Rename the file to match the pattern followed by similar files, and to better reflect its current role: it's tweaking modules that implement the built-in cryptography, which were formerly legacy public interfaces but are now internal. Signed-off-by: Gilles Peskine <[email protected]>
This one comes after both the PSA feature set and the feature set of built-in crypto have been determined. This commit creates an empty file. It will be populated in a subsequent commit. Signed-off-by: Gilles Peskine <[email protected]>
Split out the parts of `crypto_adjust_config_tweak_builtins.h` that are actually about generic support modules (platform, ASN.1, MD, ...), and move them outside `/drivers`, into `crypto_adjust_config_support.h`. One macro only depends on PSA options, so move it to `crypto_adjust_config_derived.h` instead. Signed-off-by: Gilles Peskine <[email protected]>
Install all headers without hard-coding which sub(sub)directories exist. Don't use file(GLOB), following CMake best practices (the list might not be updated if a source file is added or removed when the build tree already exists). It isn't actually useful anyway. To keep things simple, don't permissions: we just want the default permissions. Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
This was done too late, so it could have led to mysterious build errors if PKCS5 was the only thing that requires MD. Signed-off-by: Gilles Peskine <[email protected]>
We added `#error` directives to prevent some headers from being included directly because those headers were not clearly indicated as public (and, in the case of `check_config.h`, were formerly public). Headers under `.../private` have no history of being public and are obviously not intended for direct consumption, so remove the enforcement. Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
gilles-peskine-arm
dismissed stale reviews from minosgalanakis and ronald-cron-arm
via
d33a85e
gilles-peskine-arm
force-pushed
the
build_info-cleanup-1.0
branch
from
b2bd8e8 to
d33a85e
Compare
Contributor
Author
|
I rebased to include the merge of the framework prerequisite and to resolve the framework conflict. The previous version is in https://github.com/gilles-peskine-arm/TF-PSA-Crypto/tree/build_info-cleanup-1.0-4 |
gilles-peskine-arm
added
needs-review
ronald-cron-arm
approved these changes
Nov 20, 2025
Contributor
ronald-cron-arm
left a comment
ronald-cron-arm
left a comment
There was a problem hiding this comment.
Rebase and framework update look good to me.
github-project-automation
bot
moved this from Has Approval
to Done
in Roadmap pull requests (new board)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Clean up some things in
build_info.hand the header that it includes:.../private— this had only been partially done in 1.0./includeand which ones belong under/drivers/builtin/include.Needs preceding PR: framework Mbed-TLS/mbedtls-framework#236
I used (and even wrote) Mbed-TLS/mbedtls-docs#193 to check for mistakes when reordering parts of
build_info.h.PR checklist