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

cmake: tfm: support for custom CMake args when building TF-M #34868

Conversation

ioannisg
Copy link
Member

@ioannisg ioannisg commented May 5, 2021

This commit allows a subsystem to specify additional CMake flags to be
given to the TF-M build.

The additional CMake flags can be provided through the TFM_CMAKE_OPTIONS
property on the zephyr_property_target.
Using the zephyr_property_target allows Zephyr modules to append extra
TFM_CMAKE_OPTIONS regardless of the CMake processing order.

It splits the ExternalProject_Add into a two step process with the CMake
invocation executed using add_custom_target() and the build process
using ExternalProject_Add(). The reason for this split is because CMake
generator expressions passed through ExternalProject_Add to CMake will
quoted so that $<TARGET_PROPERTY:<tgt>,<prop>> becomes
"-DFOO=bar -DBAR=foo" instead of -DFOO=bar -DBAR=foo which again
results in CMake failures.

Signed-off-by: Torsten Rasmussen Torsten.Rasmussen@nordicsemi.no
Signed-off-by: Ioannis Glaropoulos Ioannis.Glaropoulos@nordicsemi.no

@ioannisg ioannisg requested a review from microbuilder as a code owner May 5, 2021 15:14
@ioannisg ioannisg self-assigned this May 5, 2021
@erwango erwango self-requested a review May 5, 2021 15:49
@erwango
Copy link
Member

erwango commented May 6, 2021

@tejlmand this PR has some interest for me. Any concerns on having it for DV2.6.0?

Comment on lines 308 to 309
config TFM_EXTRA_CMAKE_ARGS
string
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not an optimal solution.
While it does solve some problems, it also carries some side-effects.

Namely that if moduleA needs to pass -DFOO=bar and thus creates:

# Kconfig.moduleA
config TFM_EXTRA_CMAKE_ARGS
    string
    default "-DFOO=bar"

and moduleB needs to pass -DBAR=foo with:

# Kconfig.moduleA
config TFM_EXTRA_CMAKE_ARGS
    string
    default "-DBAR=foo"

then only one of the values will be passed to TF-M.
Now, as long as either module A or module B is used, things will be fine, but if both are in use, this will break.

So this must be used with care.

I would prefer a cleaner and scalable solution, but could require some rework on how ExternalProject is used, cause the problem in the external project is how it handles generator expressions containing lists.

Copy link
Member

Choose a reason for hiding this comment

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

@tejlmand I'm getting your point. Though, I think even with the limitation you mention, this feature is better than having users trying to hack modules/trusted-firmware-m/CMakeLists.txt to pass their custom TFM arguments (what I did with varying results)
Isn't possible that possible to state that for now, this should be defined only in one place ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had the same argument, actually. We can keep working in the mean time to make this better. Otherwise we just keep defining explicit Kconfig options for the corresponding cmake arguments we want to pass

Copy link
Member

Choose a reason for hiding this comment

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

Please note also that, in case of the "user hacks CMakeLists.txt" case, trying to identify if the arguments are correctly passed to TFM is not the easiest thing.
So having a validated method, even with limitation, is really a thing we miss.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@tejlmand would an EXTRA_TFM_CMAKE_ARGS cmake list work for what you had in mind.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@tejlmand, done, please take a look.

Copy link
Collaborator

@frkv frkv May 6, 2021

Choose a reason for hiding this comment

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

FYI: Splitting between a "master" Kconfig and a "vendor" Kconfig has been discussed by me and @tejlmand offline... It helps a little, but it doesn't "solve it"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Changed to use a cmake list instead, so adding from multiple places should now work fine.

set_property(GLOBAL APPEND PROPERTY EXTRA_TFM_CMAKE_ARGS -DFOO=bar)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@oyvindronningstad problem with this approach is that it is introducing Zephyr module cmake order processing dependencies.

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand that this is undesirable

@oyvindronningstad oyvindronningstad force-pushed the tfm_additional_cmakelists_args branch 2 times, most recently from c132634 to 00b755d Compare May 6, 2021 13:51
@ioannisg
Copy link
Member Author

ioannisg commented May 6, 2021

@erwango can you check if this would work for your case?

@ioannisg ioannisg requested a review from tejlmand May 6, 2021 14:09
@erwango
Copy link
Member

erwango commented May 6, 2021

@erwango can you check if this would work for your case?

This is working but definitely requires some documentation.

Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

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

Approving with the expectation documentation will come later.

@tejlmand tejlmand force-pushed the tfm_additional_cmakelists_args branch from 00b755d to 7515a77 Compare May 6, 2021 14:57
@tejlmand
Copy link
Collaborator

tejlmand commented May 6, 2021

@erwango Please re-visit.

@ioannisg @oyvindronningstad I have pushed a proper solution which:

  • is not suffering the Kconfig issue.
  • is not CMake configure time process order dependent

Any CMake file can now append additional CMake arguments to the TF-M build by appending to the TFM_CMAKE_OPTIONS property on the zephyr_property_target.

This can be done as:

set_property(TARGET zephyr_property_target
  APPEND PROPERTY TFM_CMAKE_OPTIONS
  -DFOO=bar
  -DBAR=foo
  -DGENEX=$<1:Genex_are_supported>
)

@erwango
Copy link
Member

erwango commented May 6, 2021

@tejlmand Equally working and as average user I'd still need extra documentation

@tejlmand tejlmand force-pushed the tfm_additional_cmakelists_args branch from 7515a77 to a7f4b30 Compare May 6, 2021 15:23
@ioannisg ioannisg requested a review from nashif as a code owner May 6, 2021 15:23
@tejlmand
Copy link
Collaborator

tejlmand commented May 6, 2021

@tejlmand Equally working and as average user I'd still need extra documentation

@erwango and that has just been pushed 😄

@erwango
Copy link
Member

erwango commented May 6, 2021

@tejlmand Equally working and as average user I'd still need extra documentation

@erwango and that has just been pushed

Awesome. I'd approve twice if I could !

@erwango erwango added this to the v2.6.0 milestone May 6, 2021
@erwango
Copy link
Member

erwango commented May 6, 2021

@tejlmand Maybe I've been too fast.
With this new version, I'm getting an error:

[14/163] cd /local/mcu/zephyrproject/zephyr/samples/tfm_integration/psa_level_1/build/nucleo_l552ze_q_ns/tfm && make install && /local/home/frq07517/.local/lib/python3.6/site-packages/cmake/data/bin/cmake -E touch /local/mcu/zephyrproject/zephyr/samples/tfm_integration/psa_level_1/build/nucleo_l552ze_q_ns/modules/trusted-firmware-m/tfm-prefix/src/tfm-stamp/tfm-install
FAILED: modules/trusted-firmware-m/tfm-prefix/src/tfm-stamp/tfm-install 
cd /local/mcu/zephyrproject/zephyr/samples/tfm_integration/psa_level_1/build/nucleo_l552ze_q_ns/tfm && make install && /local/home/frq07517/.local/lib/python3.6/site-packages/cmake/data/bin/cmake -E touch /local/mcu/zephyrproject/zephyr/samples/tfm_integration/psa_level_1/build/nucleo_l552ze_q_ns/modules/trusted-firmware-m/tfm-prefix/src/tfm-stamp/tfm-install
make: *** No rule to make target 'install'.  Stop.
ninja: build stopped: subcommand failed.
FATAL ERROR: command exited with status 1: /local/home/frq07517/.local/bin/cmake --build /local/mcu/zephyrproject/zephyr/samples/tfm_integration/psa_level_1/build/nucleo_l552ze_q_ns -- -v

Is it me only ?

I'm running:

/local/mcu/zephyrproject/zephyr/samples/tfm_integration/psa_level_1
$ west -v build -b nucleo_l552ze_q_ns

Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

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

There's definitely something going not well with this last version.
Cf reported error
(Everything works fine for me on main branch)

@tejlmand
Copy link
Collaborator

tejlmand commented May 7, 2021

@oyvindronningstad I experiences CI failures here:

INFO - 308/314 mps2_an521_nonsecure samples/tfm_integration/tfm_psa_test/sample.tfm.psa_internal_trusted_storage_test FAILED Build failure (qemu)

but if I go back to ef5ed3f and build the same sample, it already fails there:

$ git checkout ef5ed3f88bb
$ west update
$ ./scripts/twister -p mps2_an521_nonsecure  -N  --inline-logs -s samples/tfm_integration/tfm_psa_test/sample.tfm.psa_internal_trusted_storage_test
INFO    - Zephyr version: zephyr-v2.5.0-2414-gef5ed3f88bb9
INFO    - JOBS: 4
...
ninja: build stopped: subcommand failed.
FAILED: modules/trusted-firmware-m/tfm-prefix/src/tfm-stamp/tfm-build tfm/secure_fw/s_veneers.o tfm/app/libtfm_api_ns.a tfm/generated/interface/include/psa_manifest/sid.h tfm/app/psa_api_tests/val/val_nspe.a tfm/app/psa_api_tests/platform/pal_nspe.a tfm/app/psa_api_tests/dev_apis/storage/test_combine.a tfm/platform/libplatform_ns.a tfm/bin/bl2.bin tfm/bin/bl2.hex tfm/bin/tfm_s.bin tfm/bin/tfm_s.hex tfm/bin/tfm_ns.bin tfm/bin/tfm_ns.hex tfm/bin/tfm_s_signed.bin tfm/bin/tfm_ns_signed.bin tfm/bin/tfm_s_ns_signed.bin 
cd /projects/github/ncs/zephyr/twister-out/mps2_an521_nonsecure/samples/tfm_integration/tfm_psa_test/sample.tfm.psa_internal_trusted_storage_test/tfm && /opt/cmake-3.19.1-Linux-x86_64/bin/cmake --build .
ninja: build stopped: subcommand failed.

INFO    - /projects/github/ncs/zephyr/twister-out/mps2_an521_nonsecure/samples/tfm_integration/tfm_psa_test/sample.tfm.psa_internal_trusted_storage_test/build.log
INFO    - Total complete:    1/   1  100%  skipped:    0, failed:    1
...

So seems the CI failure is unrelated to this PR.

mps2_an521_nonsecure is valid board:

platform_allow: mps2_an521_nonsecure v2m_musca_s1_nonsecure

@tejlmand tejlmand force-pushed the tfm_additional_cmakelists_args branch from ee53f63 to f00e51b Compare May 7, 2021 07:30
@tejlmand tejlmand changed the title Kconfig: tfm: support for custom CMake args when building TF-M cmake: tfm: support for custom CMake args when building TF-M May 7, 2021
@tejlmand
Copy link
Collaborator

tejlmand commented May 7, 2021

Maybe change the commit message header since kconfig is no longer touched.

Done

@@ -36,6 +36,11 @@ set(TFM_VALID_PARTITIONS
# TF-M regression tests
# BL2: Boolean if the TF-M build uses MCUboot. Default: True
# ENABLED_PARTITIONS: List of TFM partitions to enable.
# CMAKE_ARGS: Additional CMake flags to be used when building TF-M
Copy link
Member Author

Choose a reason for hiding this comment

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

@tejlmand @erwango would this be better to be called CMAKE_ADDITIONAL_ARGS, or CMAKE_CUSTOM_ARGS, to reflect better what it is used for?

Copy link
Collaborator

Choose a reason for hiding this comment

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

well, this is the description of the function: trusted_firmware_build()

So from the point of view of that function, it's just CMake args.

You could ideally use that function from other locations.

But when called from the internal Zephyr build here: https://github.com/ioannisg/zephyr/blob/b6678723a282a99e62aebbc62570e37748c03df9/modules/trusted-firmware-m/CMakeLists.txt#L321

    CMAKE_ARGS $<GENEX_EVAL:$<TARGET_PROPERTY:zephyr_property_target,TFM_CMAKE_OPTIONS>>

you may consider it custom args.

@tejlmand tejlmand force-pushed the tfm_additional_cmakelists_args branch 2 times, most recently from 9f8bc71 to b667872 Compare May 7, 2021 08:27
@tejlmand
Copy link
Collaborator

tejlmand commented May 7, 2021

@oyvindronningstad I experiences CI failures here:

Fixed. There was both lost arguments from conflict resolving, and then a bad gnuarmemb compiler causing local failures.

This commit allows a subsystem to specify additional CMake flags to be
given to the TF-M build.

The additional CMake flags can be provided through the TFM_CMAKE_OPTIONS
property on the zephyr_property_target.
Using the zephyr_property_target allows Zephyr modules to append extra
TFM_CMAKE_OPTIONS regardless of the CMake processing order.

It splits the ExternalProject_Add into a two step process with the CMake
invocation executed using add_custom_target() and the build process
using ExternalProject_Add(). The reason for this split is because CMake
generator expressions passed through ExternalProject_Add to CMake will
quoted so that `$<TARGET_PROPERTY:<tgt>,<prop>>` becomes
`"-DFOO=bar -DBAR=foo"` instead of `-DFOO=bar -DBAR=foo` which again
results in CMake failures.

Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>
@tejlmand tejlmand force-pushed the tfm_additional_cmakelists_args branch from b667872 to 1e72fdc Compare May 7, 2021 08:50
@microbuilder
Copy link
Member

@oyvindronningstad I experiences CI failures here:

Fixed. There was both lost arguments from conflict resolving, and then a bad gnuarmemb compiler causing local failures.

I'm seeing the same here, with the latest code. It doesn't find GCC, and seems to fall back to the default C compiler on my system (using OS X here): /Library/Developer/CommandLineTools/usr/bin/cc ... I've run source zephyr-env.sh and west update and have the right setup in $HOME/.zephyrrc.

$ ./scripts/twister -p mps2_an521_nonsecure -N --inline-logs -s samples/tfm_integration/tfm_psa_test/sample.tfm.psa_internal_trusted_storage_test
...
[2/5] Building C object CMakeFiles/TargetConfigGen.dir/Users/kevin/Linaro/zephyr/zephyr/twister-out/mps2_an521_nonsecure/samples/tfm_integration/tfm_psa_test/sample.tfm.psa_internal_trusted_storage_test/tfm/app/psa_api_tests/targetConfigGen.c.o
FAILED: CMakeFiles/TargetConfigGen.dir/Users/kevin/Linaro/zephyr/zephyr/twister-out/mps2_an521_nonsecure/samples/tfm_integration/tfm_psa_test/sample.tfm.psa_internal_trusted_storage_test/tfm/app/psa_api_tests/targetConfigGen.c.o 
/Library/Developer/CommandLineTools/usr/bin/cc  -I/Users/kevin/Linaro/zephyr/modules/tee/tfm/psa-arch-tests/api-tests/val/nspe -I/Users/kevin/Linaro/zephyr/modules/tee/tfm/psa-arch-tests/api-tests/val/common -I/Users/kevin/Linaro/zephyr/modules/tee/tfm/psa-arch-tests/api-tests/platform/targets/common/nspe -I/Users/kevin/Linaro/zephyr/modules/tee/tfm/psa-arch-tests/api-tests/platform/targets/common/nspe/crypto -I/Users/kevin/Linaro/zephyr/modules/tee/tfm/psa-arch-tests/api-tests/platform/targets/tgt_dev_apis_tfm_an521/nspe -arch arm64 -o CMakeFiles/TargetConfigGen.dir/Users/kevin/Linaro/zephyr/zephyr/twister-out/mps2_an521_nonsecure/samples/tfm_integration/tfm_psa_test/sample.tfm.psa_internal_trusted_storage_test/tfm/app/psa_api_tests/targetConfigGen.c.o -c /Users/kevin/Linaro/zephyr/zephyr/twister-out/mps2_an521_nonsecure/samples/tfm_integration/tfm_psa_test/sample.tfm.psa_internal_trusted_storage_test/tfm/app/psa_api_tests/targetConfigGen.c
/Users/kevin/Linaro/zephyr/zephyr/twister-out/mps2_an521_nonsecure/samples/tfm_integration/tfm_psa_test/sample.tfm.psa_internal_trusted_storage_test/tfm/app/psa_api_tests/targetConfigGen.c:1:10: fatal error: 'stdio.h' file not found
#include <stdio.h>
         ^~~~~~~~~

You're using the zephyr SDK on Linux, I imagine?

@microbuilder
Copy link
Member

The compiler reference issue seems to be specific to tfm_psa_test run with twister. Running a sample via west works as expected, and finds the right compiler. Perhaps the test projects are looking for arm-zephyr-eabi-gcc from the Zephyr SDK, and not finding it since other systems will only have zephyr-none-eabi-gcc.

I get the same behaviour on master, though, so it isn't specific to this PR.

@tejlmand
Copy link
Collaborator

tejlmand commented May 7, 2021

@oyvindronningstad I experiences CI failures here:

Fixed. There was both lost arguments from conflict resolving, and then a bad gnuarmemb compiler causing local failures.

I'm seeing the same here, with the latest code. It doesn't find GCC, and seems to fall back to the default C compiler on my system (using OS X here): /Library/Developer/CommandLineTools/usr/bin/cc ...

yes, the use of the host compiler is caused by this:
https://github.com/zephyrproject-rtos/trusted-firmware-m/blob/master/psa-arch-tests/api-tests/CMakeLists.txt#L415-L436

Where the compiler is not passed on, but a C compiler is being looked up here:
https://github.com/zephyrproject-rtos/trusted-firmware-m/blob/51cdecd6f9e6b0aa66da45db22f4b478183d472f/psa-arch-tests/api-tests/tools/scripts/target_cfg/CMakeLists.txt#L29

project(TargetConfigGen LANGUAGES C)

so that actually mean there is some undocumented requirements when using TF-M.
May not be a problem on Linux or MacOS, but this could make it harder for Windows users.

@microbuilder
Copy link
Member

@tejlmand The psa-arch-tests problem is a separate issue, so I'm OK with this PR as is, but we will need to resolve the cmake issue with the arch tests. Thanks for pointing the source of the problem out.

@microbuilder
Copy link
Member

@tejlmand Filed an issue here, since this should really be fixed upstream, but this gives us something to track: #34962

tejlmand added a commit to tejlmand/zephyr that referenced this pull request May 18, 2021
Follow-up: zephyrproject-rtos#34868

The CMAKE_ARGS was accidentally lost during work on zephyrproject-rtos#34868.
This commit fixes that by re-adding `CMAKE_ARGS` as multi value arg.

Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>
ioannisg pushed a commit that referenced this pull request May 18, 2021
Follow-up: #34868

The CMAKE_ARGS was accidentally lost during work on #34868.
This commit fixes that by re-adding `CMAKE_ARGS` as multi value arg.

Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>
andvib pushed a commit to andvib/zephyr that referenced this pull request May 31, 2021
... trusted_firmware_build

Follow-up: zephyrproject-rtos#34868

The CMAKE_ARGS was accidentally lost during work on zephyrproject-rtos#34868.
This commit fixes that by re-adding `CMAKE_ARGS` as multi value arg.

Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>
tejlmand added a commit to tejlmand/zephyr that referenced this pull request Jun 11, 2021
Fixes: zephyrproject-rtos#36101

The move of CMake invocation to a dedicated custom target, see zephyrproject-rtos#34868
results in tfm_cmake to always be considered out-of-date, causing CMake
to be reinvoked in the TF-M Binary dir, which again results in the
build command to rebuild.

This commit moves the invocation to a custom command with the
CMakeCache.txt as output. The custom target tfm_cmake is updated to
depend on CMakeCache.txt.

This mean that CMake for TF-M will only be invoked inside the Zephyr
build command if that file is missing.

If the CMakeCache.txt file is updated or TF-M CMake or source code is
modified, then the build command inside the TF-M build folder will
ensure correct re-run of CMake from within the TF-M build folder.

This ensures that TF-M will still rebuild if TF-M code is modified,
while at the same time avoid unnecessary rebuilds of TF-M code.

Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>
galak pushed a commit that referenced this pull request Jun 11, 2021
Fixes: #36101

The move of CMake invocation to a dedicated custom target, see #34868
results in tfm_cmake to always be considered out-of-date, causing CMake
to be reinvoked in the TF-M Binary dir, which again results in the
build command to rebuild.

This commit moves the invocation to a custom command with the
CMakeCache.txt as output. The custom target tfm_cmake is updated to
depend on CMakeCache.txt.

This mean that CMake for TF-M will only be invoked inside the Zephyr
build command if that file is missing.

If the CMakeCache.txt file is updated or TF-M CMake or source code is
modified, then the build command inside the TF-M build folder will
ensure correct re-run of CMake from within the TF-M build folder.

This ensures that TF-M will still rebuild if TF-M code is modified,
while at the same time avoid unnecessary rebuilds of TF-M code.

Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>
jori-nordic pushed a commit to jori-nordic/zephyr that referenced this pull request Jun 15, 2021
... trusted_firmware_build

Follow-up: zephyrproject-rtos#34868

The CMAKE_ARGS was accidentally lost during work on zephyrproject-rtos#34868.
This commit fixes that by re-adding `CMAKE_ARGS` as multi value arg.

Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>
(cherry picked from commit b8ad3c4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants