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

Complete the move of driver wrappers to the dispatch/ directory #809

Open
asmellby wants to merge 16 commits from asmellby/feature/dispatch-library into development
pull from: asmellby/feature/dispatch-library
merge into: gh-Mbed-TLS:development
Conversation 27 Commits 16 Files changed 31 +322 -53
asmellby commented 2026-06-11 06:48:06 -05:00 (Migrated from github.com)

Description

The TF-PSA-Crypto 1.1 release introduced a dispatch directory with the following description:

A new directory dispatch has been added at the root of the repository
to eventually host all code that dispatches cryptographic operations to
drivers, such as psa_crypto_driver_wrappers_no_static.c. For the time
being, it only contains psa_crypto_driver_wrappers_no_static.h.

This PR completes the move of generated driver wrapper files and API headers to the dispatch directory.

Resolves https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/833

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 provided
  • framework PR provided Mbed-TLS/mbedtls-framework#307
  • TF-PSA-Crypto development PR provided (this PR)
  • TF-PSA-Crypto 1.1 PR not required because: not a bugfix, repository structure change is not entirely backwards compatible
  • mbedtls development PR provided Mbed-TLS/mbedtls#10780
  • mbedtls 4.1 PR not required because: does not affect mbedtls
  • mbedtls 3.6 PR not required because: does not affect mbedtls
  • tests provided
## Description The TF-PSA-Crypto 1.1 release introduced a `dispatch` directory with the following description: > A new directory `dispatch` has been added at the root of the repository to eventually host all code that dispatches cryptographic operations to drivers, such as `psa_crypto_driver_wrappers_no_static.c`. For the time being, it only contains `psa_crypto_driver_wrappers_no_static.h`. This PR completes the move of generated driver wrapper files and API headers to the dispatch directory. Resolves https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/833 ## 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** provided - [x] **framework PR** provided Mbed-TLS/mbedtls-framework#307 - [x] **TF-PSA-Crypto development PR** provided (this PR) - [x] **TF-PSA-Crypto 1.1 PR** not required because: not a bugfix, repository structure change is not entirely backwards compatible - [x] **mbedtls development PR** provided Mbed-TLS/mbedtls#10780 - [x] **mbedtls 4.1 PR** not required because: does not affect mbedtls - [x] **mbedtls 3.6 PR** not required because: does not affect mbedtls - [x] **tests** provided
gilles-peskine-arm (Migrated from github.com) requested changes 2026-06-11 12:10:17 -05:00
gilles-peskine-arm (Migrated from github.com) left a comment

I think this is moving in the direction we want, thanks. But I've asked Ronald to review as well, as he's the architect of the directory structure and build scripts. We're currently focusing on some security fixes for our next release, so it might be a while before he can review.

I'm not sure what users are supposed to be able to do. Whatever it is, it needs to be documented and tested.

I reviewed 56449b86ac563df1daa921a7a929d522ecfc8256

I think this is moving in the direction we want, thanks. But I've asked Ronald to review as well, as he's the architect of the directory structure and build scripts. We're currently focusing on some security fixes for our next release, so it might be a while before he can review. I'm not sure what users are supposed to be able to do. Whatever it is, it needs to be documented and tested. I reviewed 56449b86ac563df1daa921a7a929d522ecfc8256
CMakeLists.txt
@ -68,3 +76,4 @@
# Put the version numbers into relevant files
set(version_number_files
doxygen/input/doc_mainpage.h
gilles-peskine-arm (Migrated from github.com) commented 2026-06-11 11:01:10 -05:00

As I understand it, TF_PSA_CRYPTO_DISPATCH_DIR is now an option that can be set when running CMake. Please document it.

As I understand it, `TF_PSA_CRYPTO_DISPATCH_DIR` is now an option that can be set when running CMake. Please document it.
CMakeLists.txt
gilles-peskine-arm (Migrated from github.com) commented 2026-06-11 11:08:25 -05:00

From prior experience, I think CMakeLists.txt in Mbed TLS will need to know about the new location to populate TF_PSA_CRYPTO_PRIVATE_INCLUDE_DIRS. It isn't properly extracting the information from TF-PSA-Crypto.

From prior experience, I think `CMakeLists.txt` in Mbed TLS will need to know about the new location to populate `TF_PSA_CRYPTO_PRIVATE_INCLUDE_DIRS`. It isn't properly extracting the information from TF-PSA-Crypto.
CMakeLists.txt
@ -415,3 +425,4 @@
${TF_PSA_CRYPTO_DISPATCH_DIR}
${CMAKE_CURRENT_SOURCE_DIR}/extras
${CMAKE_CURRENT_SOURCE_DIR}/platform
${CMAKE_CURRENT_SOURCE_DIR}/utilities
gilles-peskine-arm (Migrated from github.com) commented 2026-06-11 11:33:43 -05:00

My understanding is that the point of having TF_PSA_CRYPTO_DISPATCH_DIR as a variable is that users can pass an out-of-tree directory to be used instead of dispatch, but it doesn't work.

$ cmake -DTF_PSA_CRYPTO_DISPATCH_DIR=/tmp/custom-dispatch-56449b86ac563df1daa921a7a929d522ecfc8256 -B build-customdispatch-cmake
-- The C compiler identification is GNU 13.3.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Found Python3: /home/gilpes01/Packages/_/x86.64_Linux/bin/python3 (found version "3.12.3") found components: Interpreter 
-- Performing Test C_COMPILER_SUPPORTS_WFORMAT_SIGNEDNESS
-- Performing Test C_COMPILER_SUPPORTS_WFORMAT_SIGNEDNESS - Success
CMake Error at CMakeLists.txt:437 (add_subdirectory):
  add_subdirectory not given a binary directory but the given source
  directory "/tmp/custom-dispatch-56449b86ac563df1daa921a7a929d522ecfc8256"
  is not a subdirectory of
  "/tmp/reviewing/mbedtls+tf-psa-crypto-809-feature%dispatch-library/tf-psa-crypto".
  When specifying an out-of-tree source a binary directory must be explicitly
  specified.


-- Configuring incomplete, errors occurred!
  1. Please either fix, or clarify the intended semantics.
  2. Please add a test in tests/scripts/components-build-system.sh that demonstrates the intended feature.
My understanding is that the point of having `TF_PSA_CRYPTO_DISPATCH_DIR` as a variable is that users can pass an out-of-tree directory to be used instead of `dispatch`, but it doesn't work. ``` $ cmake -DTF_PSA_CRYPTO_DISPATCH_DIR=/tmp/custom-dispatch-56449b86ac563df1daa921a7a929d522ecfc8256 -B build-customdispatch-cmake -- The C compiler identification is GNU 13.3.0 -- Detecting C compiler ABI info -- Detecting C compiler ABI info - done -- Check for working C compiler: /usr/bin/cc - skipped -- Detecting C compile features -- Detecting C compile features - done -- Found Python3: /home/gilpes01/Packages/_/x86.64_Linux/bin/python3 (found version "3.12.3") found components: Interpreter -- Performing Test C_COMPILER_SUPPORTS_WFORMAT_SIGNEDNESS -- Performing Test C_COMPILER_SUPPORTS_WFORMAT_SIGNEDNESS - Success CMake Error at CMakeLists.txt:437 (add_subdirectory): add_subdirectory not given a binary directory but the given source directory "/tmp/custom-dispatch-56449b86ac563df1daa921a7a929d522ecfc8256" is not a subdirectory of "/tmp/reviewing/mbedtls+tf-psa-crypto-809-feature%dispatch-library/tf-psa-crypto". When specifying an out-of-tree source a binary directory must be explicitly specified. -- Configuring incomplete, errors occurred! ``` 1. Please either fix, or clarify the intended semantics. 2. Please add a test in `tests/scripts/components-build-system.sh` that demonstrates the intended feature.
core/CMakeLists.txt
gilles-peskine-arm (Migrated from github.com) commented 2026-06-11 10:59:23 -05:00

Please add a changelog entry file (see ChangeLog.d/00README.md) that briefly explains the user-visible aspect of the changes.

Please add a changelog entry file (see `ChangeLog.d/00README.md`) that briefly explains the user-visible aspect of the changes.
dispatch/CMakeLists.txt
@ -0,0 +1,52 @@
# SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later
gilles-peskine-arm (Migrated from github.com) commented 2026-06-11 11:46:55 -05:00

I asked Codex to review and it tells me:

The generated wrapper rule can run twice in parallel CMake Makefile builds

dispatch/CMakeLists.txt creates a custom target for the generated wrapper
files, and also lists the same generated outputs as sources of the dispatch
object library. In a clean Makefile build:

cmake -S . -B /tmp/tf-psa-review-default \
    -DENABLE_TESTING=OFF \
    -DENABLE_PROGRAMS=OFF
cmake --build /tmp/tf-psa-review-default --parallel 16

the output showed generate_driver_wrappers.py running twice for the same
dispatch/psa_crypto_driver_wrappers.* outputs:

Generating psa_crypto_driver_wrappers.h, psa_crypto_driver_wrappers_no_static.c
Generating psa_crypto_driver_wrappers.h, psa_crypto_driver_wrappers_no_static.c

That is a race risk under recursive Makefile generation. The generated files
should have a single owning custom command path, with other targets depending on
that owner rather than independently triggering the same outputs.

This looks superficially legitimate to me, but I don't know much about CMake.

I asked Codex to review and it tells me: > The generated wrapper rule can run twice in parallel CMake Makefile builds > > `dispatch/CMakeLists.txt` creates a custom target for the generated wrapper > files, and also lists the same generated outputs as sources of the dispatch > object library. In a clean Makefile build: > > ```sh > cmake -S . -B /tmp/tf-psa-review-default \ > -DENABLE_TESTING=OFF \ > -DENABLE_PROGRAMS=OFF > cmake --build /tmp/tf-psa-review-default --parallel 16 > ``` > > the output showed `generate_driver_wrappers.py` running twice for the same > `dispatch/psa_crypto_driver_wrappers.*` outputs: > > ```text > Generating psa_crypto_driver_wrappers.h, psa_crypto_driver_wrappers_no_static.c > Generating psa_crypto_driver_wrappers.h, psa_crypto_driver_wrappers_no_static.c > ``` > > That is a race risk under recursive Makefile generation. The generated files > should have a single owning custom command path, with other targets depending on > that owner rather than independently triggering the same outputs. This looks superficially legitimate to me, but I don't know much about CMake.
asmellby (Migrated from github.com) reviewed 2026-06-12 08:08:15 -05:00
core/CMakeLists.txt
asmellby (Migrated from github.com) commented 2026-06-12 08:08:15 -05:00

Added a changelog entry.

Added a changelog entry.
asmellby (Migrated from github.com) reviewed 2026-06-12 08:08:43 -05:00
CMakeLists.txt
@ -68,3 +76,4 @@
# Put the version numbers into relevant files
set(version_number_files
doxygen/input/doc_mainpage.h
asmellby (Migrated from github.com) commented 2026-06-12 08:08:42 -05:00

Added documentation to the top of CMakeLists.txt, where the existing CMake variables were documented.

Added documentation to the top of CMakeLists.txt, where the existing CMake variables were documented.
asmellby (Migrated from github.com) reviewed 2026-06-12 08:09:00 -05:00
CMakeLists.txt
@ -415,3 +425,4 @@
${TF_PSA_CRYPTO_DISPATCH_DIR}
${CMAKE_CURRENT_SOURCE_DIR}/extras
${CMAKE_CURRENT_SOURCE_DIR}/platform
${CMAKE_CURRENT_SOURCE_DIR}/utilities
asmellby (Migrated from github.com) commented 2026-06-12 08:09:00 -05:00

Good catch! Fixed and added tests.

Good catch! Fixed and added tests.
asmellby (Migrated from github.com) reviewed 2026-06-12 08:09:12 -05:00
dispatch/CMakeLists.txt
@ -0,0 +1,52 @@
# SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later
asmellby (Migrated from github.com) commented 2026-06-12 08:09:12 -05:00

Fixed!

Fixed!
asmellby (Migrated from github.com) reviewed 2026-06-12 08:09:42 -05:00
gilles-peskine-arm (Migrated from github.com) requested changes 2026-06-12 10:46:40 -05:00
gilles-peskine-arm (Migrated from github.com) left a comment

Looks good to me, except I'd like one point clarified in the documentation. I don't know much about CMake, so my review of CMake code is superficial.

Our CI appears to be stuck today (ticket filed), so results might not be available until Monday.

Looks good to me, except I'd like one point clarified in the documentation. I don't know much about CMake, so my review of CMake code is superficial. Our CI appears to be stuck today (ticket filed), so results might not be available until Monday.
CMakeLists.txt
@ -14,6 +14,11 @@
# calling CMake in order to avoid target name clashes, via the use of
gilles-peskine-arm (Migrated from github.com) commented 2026-06-12 10:13:01 -05:00

Please state that this must be an absolute path.

Please state that this must be an absolute path.
gilles-peskine-arm commented 2026-06-15 03:58:00 -05:00 (Migrated from github.com)

Note that the CI isn't happy. There's at least a missing include directory in CMake builds.

Note that the CI isn't happy. There's at least a missing include directory in CMake builds.
asmellby (Migrated from github.com) reviewed 2026-06-18 08:41:44 -05:00
CMakeLists.txt
asmellby (Migrated from github.com) commented 2026-06-18 08:41:44 -05:00

Opened Mbed-TLS/mbedtls#10780

Opened Mbed-TLS/mbedtls#10780
asmellby (Migrated from github.com) reviewed 2026-06-18 08:41:52 -05:00
CMakeLists.txt
@ -14,6 +14,11 @@
# calling CMake in order to avoid target name clashes, via the use of
asmellby (Migrated from github.com) commented 2026-06-18 08:41:52 -05:00

Done.

Done.
valeriosetti commented 2026-07-01 07:41:46 -05:00 (Migrated from github.com)

I manually kicked the CI because it didn't automatically start after the last push

I manually kicked the CI because it didn't automatically start after the last push
ronald-cron-arm commented 2026-07-08 03:10:48 -05:00 (Migrated from github.com)

@asmellby Sorry for the delay.

I've started looking at this PR. It seems to me that it addresses two separate things:

  1. Completing the move of the generated driver wrapper files and API headers to the dispatch directory.
  2. Adding support for selecting a dispatch directory different from the repository's dispatch directory.

I would prefer to handle (1) first and (2) in a follow-up PR. Would you be able to split the changes accordingly?

@asmellby Sorry for the delay. I've started looking at this PR. It seems to me that it addresses two separate things: 1. Completing the move of the generated driver wrapper files and API headers to the `dispatch` directory. 2. Adding support for selecting a dispatch directory different from the repository's `dispatch` directory. I would prefer to handle (1) first and (2) in a follow-up PR. Would you be able to split the changes accordingly?
asmellby commented 2026-07-08 03:44:29 -05:00 (Migrated from github.com)

I would prefer to handle (1) first and (2) in a follow-up PR. Would you be able to split the changes accordingly?

I could do that, but that will require a rebase, which the contribution guidelines tell me not to do without approval from reviewers. So I'll need some guidance: To what degree should I clean up the commits when I rebase? Am I allowed to merge fixup commits into the initial commits to create a more reviewable changeset, or should I keep them separate to preserve the (now no longer entirely real) history?

> I would prefer to handle (1) first and (2) in a follow-up PR. Would you be able to split the changes accordingly? I could do that, but that will require a rebase, which the contribution guidelines tell me not to do without approval from reviewers. So I'll need some guidance: To what degree should I clean up the commits when I rebase? Am I allowed to merge fixup commits into the initial commits to create a more reviewable changeset, or should I keep them separate to preserve the (now no longer entirely real) history?
gilles-peskine-arm commented 2026-07-08 03:52:18 -05:00 (Migrated from github.com)

I would prefer to keep (1) and (2) in the same pull request, because (2) checks that (1) has been done right.

But if (1) and (2) are separate, please provide histories that help me review incrementally:

  • A history with the same commit structure, so I can review with git range-diff development 1c6cde11420cf17340a385dfb43b9453dc211c44 head1.
  • A restructured history with a step step1 in one PR, and a step step2 such that head1 and step2 have the same content.

(Where rebases get really bad is if there's both a change of base and a change of commit structure.)

I would prefer to keep (1) and (2) in the same pull request, because (2) checks that (1) has been done right. But if (1) and (2) are separate, please provide histories that help me review incrementally: * A history with the same commit structure, so I can review with `git range-diff development 1c6cde11420cf17340a385dfb43b9453dc211c44 head1`. * A restructured history with a step `step1` in one PR, and a step `step2` such that `head1` and `step2` have the same content. (Where rebases get really bad is if there's both a change of base and a change of commit structure.)
gilles-peskine-arm commented 2026-07-08 03:54:01 -05:00 (Migrated from github.com)

By the way, for reviewing, the merge conflict on the framework submodule is not a concern. However, I haven't made this review a priority due to the CI failure.

Note, I will be offline from tomorrow to 19 July.

By the way, for reviewing, the merge conflict on the framework submodule is not a concern. However, I haven't made this review a priority due to the CI failure. Note, I will be offline from tomorrow to 19 July.
asmellby commented 2026-07-08 03:57:00 -05:00 (Migrated from github.com)

By the way, for reviewing, the merge conflict on the framework submodule is not a concern. However, I haven't made this review a priority due to the CI failure.

CI passes for the TF-PSA-Crypto tests as far as I can tell. The CI failure is on the Mbed TLS tests, which needs the corresponding change in https://github.com/Mbed-TLS/mbedtls/pull/10780 to pass. How can I tell the TF-PSA-Crypto CI to use that Mbed TLS PR?

> By the way, for reviewing, the merge conflict on the framework submodule is not a concern. However, I haven't made this review a priority due to the CI failure. CI passes for the TF-PSA-Crypto tests as far as I can tell. The CI failure is on the Mbed TLS tests, which needs the corresponding change in https://github.com/Mbed-TLS/mbedtls/pull/10780 to pass. How can I tell the TF-PSA-Crypto CI to use that Mbed TLS PR?
ronald-cron-arm commented 2026-07-08 04:00:55 -05:00 (Migrated from github.com)

By the way, for reviewing, the merge conflict on the framework submodule is not a concern. However, I haven't made this review a priority due to the CI failure.

CI passes for the TF-PSA-Crypto tests as far as I can tell. The CI failure is on the Mbed TLS tests, which needs the corresponding change in Mbed-TLS/mbedtls#10780 to pass. How can I tell the TF-PSA-Crypto CI to use that Mbed TLS PR?

You cannot. We will need to merge the Mbed TLS PR first.

> > By the way, for reviewing, the merge conflict on the framework submodule is not a concern. However, I haven't made this review a priority due to the CI failure. > > CI passes for the TF-PSA-Crypto tests as far as I can tell. The CI failure is on the Mbed TLS tests, which needs the corresponding change in [Mbed-TLS/mbedtls#10780](https://github.com/Mbed-TLS/mbedtls/pull/10780) to pass. How can I tell the TF-PSA-Crypto CI to use that Mbed TLS PR? You cannot. We will need to merge the Mbed TLS PR first.
ronald-cron-arm commented 2026-07-08 04:04:50 -05:00 (Migrated from github.com)

I would prefer to keep (1) and (2) in the same pull request, because (2) checks that (1) has been done right.

@gilles-peskine-arm I am not sure what do you mean by that. Could you elaborate?

> I would prefer to keep (1) and (2) in the same pull request, because (2) checks that (1) has been done right. @gilles-peskine-arm I am not sure what do you mean by that. Could you elaborate?
gilles-peskine-arm commented 2026-07-08 04:10:16 -05:00 (Migrated from github.com)

If we've moved too few files or too many files, the test with an alternative dispatch directory has a good change of telling us. Without this test, nothing will tell us.

If we've moved too few files or too many files, the test with an alternative dispatch directory has a good change of telling us. Without this test, nothing will tell us.
ronald-cron-arm commented 2026-07-08 10:06:49 -05:00 (Migrated from github.com)

If we've moved too few files or too many files, the test with an alternative dispatch directory has a good change of telling us. Without this test, nothing will tell us.

The tests with an alternative dispatch directory mostly reproduce the content of the dispatch directory into a custom directory, so I am not sure how they would help check whether we have moved too few or too many files into that directory.

More generally, I may have missed some discussion, but I am not sure what we are trying to achieve with the TF_PSA_CRYPTO_DISPATCH_DIR option. What are the intended use cases for this option?

> If we've moved too few files or too many files, the test with an alternative dispatch directory has a good change of telling us. Without this test, nothing will tell us. The tests with an alternative dispatch directory mostly reproduce the content of the `dispatch` directory into a `custom` directory, so I am not sure how they would help check whether we have moved too few or too many files into that directory. More generally, I may have missed some discussion, but I am not sure what we are trying to achieve with the `TF_PSA_CRYPTO_DISPATCH_DIR` option. What are the intended use cases for this option?
gilles-peskine-arm commented 2026-07-08 12:01:23 -05:00 (Migrated from github.com)

The tests with an alternative dispatch directory mostly reproduce the content of the dispatch directory into a custom directory, so I am not sure how they would help check whether we have moved too few or too many files into that directory.

Ah, yes, they wouldn't really help with that, my bad.

More generally, I may have missed some discussion, but I am not sure what we are trying to achieve with the TF_PSA_CRYPTO_DISPATCH_DIR option. What are the intended use cases for this option?

Ah, my bad, I've mentioned it before but I hadn't put it in writing. I've filed https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/833.

> The tests with an alternative dispatch directory mostly reproduce the content of the dispatch directory into a custom directory, so I am not sure how they would help check whether we have moved too few or too many files into that directory. Ah, yes, they wouldn't really help with that, my bad. > More generally, I may have missed some discussion, but I am not sure what we are trying to achieve with the TF_PSA_CRYPTO_DISPATCH_DIR option. What are the intended use cases for this option? Ah, my bad, I've mentioned it before but I hadn't put it in writing. I've filed https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/833.
gilles-peskine-arm (Migrated from github.com) requested changes 2026-07-08 12:16:02 -05:00
gilles-peskine-arm (Migrated from github.com) left a comment

At 7655efb69f, LGTM except that I don't understand 2de8d4e61c.

At 7655efb69f881912911083c8ff96650f9961f3cb, LGTM except that I don't understand 2de8d4e61c9d9fbb60461fb3ed578077191b3e65.
dispatch/CMakeLists.txt
@ -0,0 +29,4 @@
EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/psa_crypto_driver_wrappers_no_static.c")
# Some test cases (tf_psa_crypto_test_memsan_constant_flow_psa) rely on
# reusing a generated version of the driver wrapper from a previous
# CMake invocation. If the driver wrapper doesn't exist in the source
gilles-peskine-arm (Migrated from github.com) commented 2026-07-08 12:07:04 -05:00

This should be an elseif.

This should be an elseif.
gilles-peskine-arm (Migrated from github.com) commented 2026-07-08 12:09:35 -05:00

I don't understand why this condition is needed now. Is it fixing a preexisting bug?

We run our CI with GEN_FILES=ON. It's supposed to work with GEN_FILES=OFF, but that's not something we test.

I don't understand why this condition is needed now. Is it fixing a preexisting bug? We run our CI with `GEN_FILES=ON`. It's supposed to work with `GEN_FILES=OFF`, but that's not something we test.
dispatch/CMakeLists.txt
@ -0,0 +30,4 @@
# Some test cases (tf_psa_crypto_test_memsan_constant_flow_psa) rely on
# reusing a generated version of the driver wrapper from a previous
# CMake invocation. If the driver wrapper doesn't exist in the source
# directory but does exist in the build directory, use the file from
gilles-peskine-arm (Migrated from github.com) commented 2026-07-08 12:06:13 -05:00

Not “test cases” (this would be a stanza in tests/suites/*.data, but “test components”.

But it's weird to justify a feature by a test component. I don't quite understand why this is needed.

Not “test cases” (this would be a stanza in `tests/suites/*.data`, but “test components”. But it's weird to justify a feature by a test component. I don't quite understand why this is needed.
asmellby (Migrated from github.com) reviewed 2026-07-08 14:13:07 -05:00
dispatch/CMakeLists.txt
@ -0,0 +29,4 @@
EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/psa_crypto_driver_wrappers_no_static.c")
# Some test cases (tf_psa_crypto_test_memsan_constant_flow_psa) rely on
# reusing a generated version of the driver wrapper from a previous
# CMake invocation. If the driver wrapper doesn't exist in the source
asmellby (Migrated from github.com) commented 2026-07-08 14:13:07 -05:00

The test component tf_psa_crypto_test_memsan_constant_flow_psa runs CMake twice -- first with GEN_FILES=ON to generate the driver wrapper. Then, it runs again with GEN_FILES=OFF and CMAKE_BUILD_TYPE=MemSan, relying on the previous invocation's copy of the generated driver wrapper to still exist and be picked up by the build system.

Introduced in github.com/Mbed-TLS/TF-PSA-Crypto@93a8114276

This creates a weird scenario where the build system on that second pass needs to be able to pick up the file from the generation directory, despite GEN_FILES=OFF. The previous implementation in core handled this by using a relative path, which makes CMake check both the source and build directory: https://github.com/Mbed-TLS/TF-PSA-Crypto/blob/development/core/CMakeLists.txt#L3-L14
I can simplify this to match the way it was done in core before, they should be equivalent if my reading of https://cmake.org/cmake/help/v3.31/prop_tgt/SOURCES.html is correct -- what I did is just more explicit about the intention.

The test component `tf_psa_crypto_test_memsan_constant_flow_psa` runs CMake twice -- first with `GEN_FILES=ON` to generate the driver wrapper. Then, it runs again with `GEN_FILES=OFF` and `CMAKE_BUILD_TYPE=MemSan`, relying on the previous invocation's copy of the generated driver wrapper to still exist and be picked up by the build system. Introduced in https://github.com/Mbed-TLS/TF-PSA-Crypto/commit/93a811427694293d75d032878d6c8384e544b5ce This creates a weird scenario where the build system on that second pass needs to be able to pick up the file from the generation directory, despite `GEN_FILES=OFF`. The previous implementation in `core` handled this by using a relative path, which makes CMake check both the source and build directory: https://github.com/Mbed-TLS/TF-PSA-Crypto/blob/development/core/CMakeLists.txt#L3-L14 I can simplify this to match the way it was done in `core` before, they should be equivalent if my reading of https://cmake.org/cmake/help/v3.31/prop_tgt/SOURCES.html is correct -- what I did is just more explicit about the intention.
asmellby (Migrated from github.com) reviewed 2026-07-08 14:21:16 -05:00
dispatch/CMakeLists.txt
@ -0,0 +29,4 @@
EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/psa_crypto_driver_wrappers_no_static.c")
# Some test cases (tf_psa_crypto_test_memsan_constant_flow_psa) rely on
# reusing a generated version of the driver wrapper from a previous
# CMake invocation. If the driver wrapper doesn't exist in the source
asmellby (Migrated from github.com) commented 2026-07-08 14:21:16 -05:00

https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/135#discussion_r1946824568 contains some discussion of why it's done this way for that test component.

https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/135#discussion_r1946824568 contains some discussion of why it's done this way for that test component.
asmellby (Migrated from github.com) reviewed 2026-07-08 14:22:57 -05:00
dispatch/CMakeLists.txt
@ -0,0 +30,4 @@
# Some test cases (tf_psa_crypto_test_memsan_constant_flow_psa) rely on
# reusing a generated version of the driver wrapper from a previous
# CMake invocation. If the driver wrapper doesn't exist in the source
# directory but does exist in the build directory, use the file from
asmellby (Migrated from github.com) commented 2026-07-08 14:22:57 -05:00

Agree that it's weird. Added a comment below about why it's needed, but that it could be made less explicit by relying on implicit CMake search paths for relative paths, which was what the core did before. That would avoid explicitly calling out the test component -- the build system behavior would be exactly the same, though.

Agree that it's weird. Added a comment below about why it's needed, but that it could be made less explicit by relying on implicit CMake search paths for relative paths, which was what the core did before. That would avoid explicitly calling out the test component -- the build system behavior would be exactly the same, though.
gilles-peskine-arm (Migrated from github.com) reviewed 2026-07-08 15:34:40 -05:00
dispatch/CMakeLists.txt
@ -0,0 +29,4 @@
EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/psa_crypto_driver_wrappers_no_static.c")
# Some test cases (tf_psa_crypto_test_memsan_constant_flow_psa) rely on
# reusing a generated version of the driver wrapper from a previous
# CMake invocation. If the driver wrapper doesn't exist in the source
gilles-peskine-arm (Migrated from github.com) commented 2026-07-08 15:34:39 -05:00

Thanks for the explanation.

I'm ok with this code if you improve the comments explaining why, mainly that it's about GEN_FILES=ON followed by GEN_FILES=OFF. I don't understand why we did that in 93a8114276, as opposed to e.g. framework/scripts/make_generated_files.py or just directly running the build without touching GEN_FILES.

But I'm a CMake novice. Please wait for someone who is more experienced with CMake (Ronald) to give their opinion.

Thanks for the explanation. I'm ok with this code if you improve the comments explaining why, mainly that it's about `GEN_FILES=ON` followed by `GEN_FILES=OFF`. I don't understand why we did that in 93a811427694293d75d032878d6c8384e544b5ce, as opposed to e.g. `framework/scripts/make_generated_files.py` or just directly running the build without touching `GEN_FILES`. But I'm a CMake novice. Please wait for someone who is more experienced with CMake (Ronald) to give their opinion.
ronald-cron-arm (Migrated from github.com) requested changes 2026-09-25 02:59:40 -05:00
ronald-cron-arm (Migrated from github.com) left a comment

Sorry for the delay. This looks quite good to me. There are a few things I would like to change/improve.

Sorry for the delay. This looks quite good to me. There are a few things I would like to change/improve.
dispatch/CMakeLists.txt
@ -0,0 +1,52 @@
# SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later
ronald-cron-arm (Migrated from github.com) commented 2026-09-25 02:28:19 -05:00

My understanding is that the commit "Remove intermediate generated dispatch target" is the fix. This commit fixes the race between libtfpsacrypto_generated_dispatch_target and the dispatch object target. But there is still a race condition when both the shared and static libraries are built. Both dispatch_target and dispatch_static_target targets have the driver wrapper in their list of sources. The two target builds which can happen in parallel trigger the custom command to generate the driver wrapper.

I would restore the libtfpsacrypto_generated_dispatch_target custom target and make both dispatch_target and dispatch_static_target depends on it. Then as it is already the case now I think, the driver targets depend on the dispatch ones.

My understanding is that the commit "Remove intermediate generated dispatch target" is the fix. This commit fixes the race between libtfpsacrypto_generated_dispatch_target and the dispatch object target. But there is still a race condition when both the shared and static libraries are built. Both `dispatch_target` and `dispatch_static_target` targets have the driver wrapper in their list of sources. The two target builds which can happen in parallel trigger the custom command to generate the driver wrapper. I would restore the `libtfpsacrypto_generated_dispatch_target` custom target and make both `dispatch_target` and `dispatch_static_target` depends on it. Then as it is already the case now I think, the driver targets depend on the dispatch ones.
dispatch/CMakeLists.txt
@ -0,0 +29,4 @@
EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/psa_crypto_driver_wrappers_no_static.c")
# Some test cases (tf_psa_crypto_test_memsan_constant_flow_psa) rely on
# reusing a generated version of the driver wrapper from a previous
# CMake invocation. If the driver wrapper doesn't exist in the source
ronald-cron-arm (Migrated from github.com) commented 2026-09-25 02:37:51 -05:00

I haven't dug into the history but as of today I think we should change the test component to something like:

component_tf_psa_crypto_test_memsan_constant_flow_psa () {                      
    # This tests both (1) accesses to undefined memory, and (2) branches or     
    # memory access depending on secret values. To distinguish between those:   
    # - unset MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN - does the failure persist?     
    # - or alternatively, change the build type to MemSanDbg, which enables     
    # origin tracking and nicer stack traces (which are useful for debugging    
    # anyway), and check if the origin was TEST_CF_SECRET() or something else.  
    msg "build: cmake MSan (clang), full config with constant flow testing"     
    scripts/config.py full                                                      
    scripts/config.py unset MBEDTLS_AESNI_C # memsan doesn't grok asm           
    scripts/config.py unset MBEDTLS_HAVE_ASM                                    
    framework/scripts/make_generated_files.py
                                       
    scripts/config.py set MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN             
    cd $OUT_OF_SOURCE_DIR                                                       
    cmake -DCMAKE_C_COMPILER=clang -DGEN_FILES=OFF -DCMAKE_BUILD_TYPE:String=MemSan "$TF_PSA_CRYPTO_ROOT_DIR"
    make                                                                        
                                                                                
    msg "test: main suites (Msan + constant flow)"                              
    make test                                                                   
}

and remove the if(EXISTS ... code in question in this series of comments.

With the MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN config option the generation of some files fails thus we generate them in the source tree (like in releases) and then we build the library with the desired option. There may be other all.sh components to update.

I haven't dug into the history but as of today I think we should change the test component to something like: ``` component_tf_psa_crypto_test_memsan_constant_flow_psa () { # This tests both (1) accesses to undefined memory, and (2) branches or # memory access depending on secret values. To distinguish between those: # - unset MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN - does the failure persist? # - or alternatively, change the build type to MemSanDbg, which enables # origin tracking and nicer stack traces (which are useful for debugging # anyway), and check if the origin was TEST_CF_SECRET() or something else. msg "build: cmake MSan (clang), full config with constant flow testing" scripts/config.py full scripts/config.py unset MBEDTLS_AESNI_C # memsan doesn't grok asm scripts/config.py unset MBEDTLS_HAVE_ASM framework/scripts/make_generated_files.py scripts/config.py set MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN cd $OUT_OF_SOURCE_DIR cmake -DCMAKE_C_COMPILER=clang -DGEN_FILES=OFF -DCMAKE_BUILD_TYPE:String=MemSan "$TF_PSA_CRYPTO_ROOT_DIR" make msg "test: main suites (Msan + constant flow)" make test } ``` and remove the ```if(EXISTS ...``` code in question in this series of comments. With the MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN config option the generation of some files fails thus we generate them in the source tree (like in releases) and then we build the library with the desired option. There may be other all.sh components to update.
programs/test/cmake_package_install_custom_dispatch/CMakeLists.txt
@ -0,0 +1,41 @@
cmake_minimum_required(VERSION 3.5.1)
ronald-cron-arm (Migrated from github.com) commented 2026-09-25 02:57:42 -05:00

Rather than duplicate and then adapt cmake_package_install I would prefer to just adapt it with a TF_PSA_CRYPTO_DISPATCH_DIR option handled by this CMakeLists.txt file and passed to TF-PSA-Crypto CMake build.

Ditto for cmake_subproject_custom_dispatch().

The all.sh component would then look something like:

component_test_tf_psa_crypto_cmake_as_package_install_custom_dispatch () {      
    msg "build: custom dispatch with cmake 'as-installed-package' build"        
    cp -r dispatch/. programs/test/cmake_package_install/custom/                
    cp programs/test/cmake_package_install/custom.CMakeLists.txt \              
        programs/test/cmake_package_install/custom/CMakeLists.txt               
    scripts/generate_driver_wrappers.py programs/test/cmake_package_install/custom
    cd programs/test/cmake_package_install                                      
    cmake -DTF_PSA_CRYPTO_DISPATCH_DIR="$PWD/custom" .                          
    make                                                                        
    ./cmake_package_install                                                     
}

The logic to copy the content of dispatch is a bit different to what we have currently to know as little as possible about its content.

Rather than duplicate and then adapt `cmake_package_install` I would prefer to just adapt it with a TF_PSA_CRYPTO_DISPATCH_DIR option handled by this CMakeLists.txt file and passed to TF-PSA-Crypto CMake build. Ditto for `cmake_subproject_custom_dispatch()`. The all.sh component would then look something like: ``` component_test_tf_psa_crypto_cmake_as_package_install_custom_dispatch () { msg "build: custom dispatch with cmake 'as-installed-package' build" cp -r dispatch/. programs/test/cmake_package_install/custom/ cp programs/test/cmake_package_install/custom.CMakeLists.txt \ programs/test/cmake_package_install/custom/CMakeLists.txt scripts/generate_driver_wrappers.py programs/test/cmake_package_install/custom cd programs/test/cmake_package_install cmake -DTF_PSA_CRYPTO_DISPATCH_DIR="$PWD/custom" . make ./cmake_package_install } ``` The logic to copy the content of `dispatch` is a bit different to what we have currently to know as little as possible about its content.
This pull request has changes conflicting with the target branch.
  • CMakeLists.txt
  • framework
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin 'asmellby/feature/dispatch-library:asmellby/feature/dispatch-library'
git switch 'asmellby/feature/dispatch-library'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'development'
git merge --no-ff 'asmellby/feature/dispatch-library'
git switch 'asmellby/feature/dispatch-library'
git rebase 'development'
git switch 'development'
git merge --ff-only 'asmellby/feature/dispatch-library'
git switch 'asmellby/feature/dispatch-library'
git rebase 'development'
git switch 'development'
git merge --no-ff 'asmellby/feature/dispatch-library'
git switch 'development'
git merge --squash 'asmellby/feature/dispatch-library'
git switch 'development'
git merge --ff-only 'asmellby/feature/dispatch-library'
git switch 'development'
git merge 'asmellby/feature/dispatch-library'
git push origin 'development'
Sign in to join this conversation.
Reviewers
No reviewers
gilles-peskine-arm
ronald-cron-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!809
No description provided.