Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TF-M: Add path for MBEDCRYPTO for TF-M builds without pulling external trees #34263

Closed

Conversation

ioannisg
Copy link
Member

@ioannisg ioannisg commented Apr 14, 2021

Fixes #32252

TF-M uses mbedtls version v.2.25.0, which is different from
the one Zephyr uses, so we add an entry in the default manifest
for TF-M's mbedtls. west update will checkout the upstream
mbedtls version in a directory that is used explicitly by TF-M.

Signed-off-by: Ioannis Glaropoulos <Ioannis.Glaropoulos@nordicsemi.no>
Define the MBEDCRYPTO_PATH variable for the TF-M build,
so the latter can used a checked-out version of mbedtls,
instead of pulling an external tree during build time.
This will make Zephyr builds with TF-M must faster.

Signed-off-by: Ioannis Glaropoulos <Ioannis.Glaropoulos@nordicsemi.no>
@ioannisg ioannisg changed the title Tfm mbedcrypto path TF-M: Add path for MBEDCRYPTO for TF-M builds without pulling external trees Apr 14, 2021
@github-actions
Copy link

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
tfm-mbedcrypto N/A v2.25.0 N/A

Note: This message is automatically posted and updated by the Manifest GitHub Action.

Copy link
Collaborator

@oyvindronningstad oyvindronningstad left a comment

Choose a reason for hiding this comment

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

I discussed this with @tejlmand earlier, and this solution doesn't take into account the patches that TFM needs to add to mbedtls (see lib/ext/mbedcrypto/*.patch). We now have another option upstream: MBEDCRYPTO_GIT_REMOTE, which we can use to tell TFM to pull the files locally (e.g. from modules/crypto/mbedtls/.git), instead of from the web, after which it automatically applies the patches.

@tejlmand
Copy link
Collaborator

We now have another option upstream: MBEDCRYPTO_GIT_REMOTE, which we can use to tell TFM to pull the files locally (e.g. from modules/crypto/mbedtls/.git), instead of from the web

but that requires that the local remote contains the SHA / tag that TF-M will be using, which again puts some requirements to the maintainer of https://github.com/zephyrproject-rtos/mbedtls fork.

Also it still has the slight annoyance of cloning mbedtls into each build folder 😞

But for sure a step forward compared to today's solution.

Another option could be to create a TF-M patched branch in https://github.com/zephyrproject-rtos/mbedtls and then have a SHA from that branch in the manifest file and use -DMBEDCRYPTO_PATH as proposed in this PR.

@ioannisg ioannisg added the Needs review This PR needs attention from Zephyr's maintainers label Apr 15, 2021
@tejlmand tejlmand requested a review from frkv April 16, 2021 08:25
@@ -150,6 +150,7 @@ function(trusted_firmware_build)
${TFM_PROFILE_ARG}
${MCUBOOT_IMAGE_NUM_ARG}
-DTFM_TEST_REPO_PATH=${ZEPHYR_TRUSTED_FIRMWARE_M_MODULE_DIR}/tf-m-tests
-DMBEDCRYPTO_PATH=${ZEPHYR_TRUSTED_FIRMWARE_M_MODULE_DIR}/../tfm-mbedcrypto
Copy link
Collaborator

@frkv frkv Apr 16, 2021

Choose a reason for hiding this comment

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

This may collide with an externally set -DMBEDCRYPTO_PATH
We set this already downstream for nrf_security in nRF Connect SDK

Could this be handled by a specific variable instead of using a relative/full path so downstream users can inject their own settings?

@ioannisg
Copy link
Member Author

Has been addressed via the mbedtls update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Modules manifest manifest-tfm-mbedcrypto Needs review This PR needs attention from Zephyr's maintainers
Projects
None yet
4 participants