CMakeLists.txt: Provide common routine for post-build commands #14

anangl · 2020-09-30T09:03:22Z

... so that those commands are not duplicated in particular boards that
support building with TF-M.

Signed-off-by: Andrzej Głąbek andrzej.glabek@nordicsemi.no

... so that those commands are not duplicated in particular boards that support building with TF-M. Signed-off-by: Andrzej Głąbek <andrzej.glabek@nordicsemi.no>

ioannisg · 2020-10-01T10:39:01Z

@erwango @oyvindronningstad FYI.

This is the tf-m module counterpart patch of #28806. It only groups together the post-build actions when we build a "non-secure" platform with tfm support.

Tested on mps2 and stm32 platforms.

microbuilder · 2020-10-01T13:30:00Z

@agansari Do you mind having a look at this for the LPC while I'm out? Thanks!

agansari · 2020-10-01T19:30:45Z

@agansari Do you mind having a look at this for the LPC while I'm out? Thanks!

Tested on LPC, works OK.

galak

This feels a little backwards to me, while I get the benefit of having this in common, is it possible that common cmake code can live in Zephyr?

galak · 2020-10-02T13:32:13Z

@tejlmand any thoughts on how this might live purely in the zephyr tree in a common place? Would it make sense to have modules/tfm.cmake and place this in there?

tejlmand · 2020-10-02T13:52:06Z

CMakeLists.txt

+ ${ADD_NS_IMAGE_MIN_VER}
+ ${ADD_SECURITY_COUNTER_S}


Those two variables may not be set, that depends on current variables in the scope of the caller, and may lead to unexpected behavior depending on the location from where this function is called.

Probably this should be arguments to the function.

This was copied from the mps2_an521 board's CMakeLists.txt. Looking at it now, I don't know how it was supposed to work. Perhaps I should add here:

if (TFM_NS_IMAGE_MIN_VER) set(ADD_NS_IMAGE_MIN_VER "-d \"(1,${TFM_NS_IMAGE_MIN_VER})\"") else() set(ADD_NS_IMAGE_MIN_VER "") endif() if (TFM_SECURITY_COUNTER_S) set(ADD_SECURITY_COUNTER_S "-s ${TFM_SECURITY_COUNTER_S}") else() set(ADD_SECURITY_COUNTER_S "") endif()

basing on what is done in MCUBoot.cmake:

trusted-firmware-m/trusted-firmware-m/bl2/ext/mcuboot/MCUBoot.cmake

Lines 127 to 131 in 143df67

if (NS_IMAGE_MIN_VER)

set(ADD_NS_IMAGE_MIN_VER "-d \"(1,${NS_IMAGE_MIN_VER})\"")

else()

set(ADD_NS_IMAGE_MIN_VER "")

endif()

trusted-firmware-m/trusted-firmware-m/bl2/ext/mcuboot/MCUBoot.cmake

Lines 92 to 96 in 143df67

if (SECURITY_COUNTER_S)

set(ADD_SECURITY_COUNTER_S "-s ${SECURITY_COUNTER_S}")

else()

set(ADD_SECURITY_COUNTER_S "")

endif()

@microbuilder you took over from @karl-zh, do you have any comments on this ?

For reference: https://github.com/zephyrproject-rtos/zephyr/blame/12e1fd653deab52cef5b1679ba631c35b2a55286/boards/arm/mps2_an521/CMakeLists.txt#L45-L46

tejlmand · 2020-10-02T13:54:52Z

CMakeLists.txt

+
+ # Copy mcuboot.bin
+ COMMAND ${CMAKE_COMMAND} -E copy
+ ${CMAKE_BINARY_DIR}/tfm/install/outputs/${TFM_TARGET_PLATFORM}/mcuboot.bin


The TFM_TARGET_PLATFORM variable may not be set, that depends on current scope of the caller.
This may lead to unexpected behavior depending on the location from where this function is called.
Same comment applies to L133.

Probably this should be an argument to the function.

This variable is supposed to be set in the respective board definition, for example in mps2_an521/board.cmake:

set(TFM_TARGET_PLATFORM "AN521")

How about adding here a check like:

if(NOT DEFINED TFM_TARGET_PLATFORM) message(FATAL_ERROR "TFM_TARGET_PLATFORM is not set") endif()

We are having CMake cached variables and CONFIG_<name> Kconfig variables available as globals.
If we start having other variables semi globally available throughout modules, then things becomes much harder to keep track of.

So unless this is Kconfig setting, I would like it to be passed by the caller.

tejlmand · 2020-10-02T18:23:34Z

CMakeLists.txt

+
+# Gets a list of commands to be performed after a successful Zephyr build that
+# involves trusted-firmware-m.
+function(trusted_firmware_get_post_build_commands out_commands)


Not sure exactly where / which target will call this function, but I think it would be better to have this as a macro which actually adds this as a real post build step, like:

macro(sign_trusted_firmware target <other args>) if(NOT CONFIG_TFM_BL2_FALSE) <existing code> add_custom_command(TARGET ${target} POST_BUILD COMMAND ${PYTHON_EXECUTABLE} ${TFM_MCUBOOT_DIR}/scripts/imgtool.py sign <existing additional args> ) <and so on> endif() endmacro()

Then users won't need to handle a out_commands variable and decide what to do with it.

The reason to suggest a macro here, instead of a function, has to do with the add_custom_commnad(... POST_BUILD ..) must be called in same CMakeLists directory as the target.

Also, feel free to find a better name.
Not sure if: sign_trusted_firmware is the best name.

If this is purely a Zephyr macro, then maybe it should be called zephyr_<something>.

Not sure exactly where / which target will call this function, but I think it would be better to have this as a macro which actually adds this as a real post build step, like:

I should of course look here:
https://github.com/zephyrproject-rtos/zephyr/pull/28806/files

and that actually means this will be hard to change, due to the nature of:
extra_post_build_commands

If changing that, then we are looking into a completely different task.

tejlmand · 2020-10-02T22:06:50Z

This feels a little backwards to me, while I get the benefit of having this in common, is it possible that common cmake code can live in Zephyr?

This code only makes sense together with TFM.
So I think this is at the right place.

Of course, one could consider to have a <tfm_module>/cmake/extensions.cmake for module functions, but then that should be a general principle for all modules.
But I would do such cleanups if we move Zephyr module code into Zephyr, as discussed here:
zephyrproject-rtos/zephyr#27185

tejlmand · 2020-10-05T10:43:17Z

CMakeLists.txt

+ --layout ${PREPROCESSED_FILE}_s.c
+ -s ${CMAKE_BINARY_DIR}/tfm_s_signed.bin
+ -n ${CMAKE_BINARY_DIR}/zephyr_ns_signed.bin
+ -o ${CMAKE_BINARY_DIR}/tfm_sign.bin


I discovered the tfm_sign.bin is used here:

trusted-firmware-m/CMakeLists.txt

Line 156 in 7316751

-o ${CMAKE_BINARY_DIR}/tfm_sign.bin

we should not have such a hard coded dependency between module code and CMake code in Zephyr, as that will break if out files are renamed in one place, but not the other.

Instead tfm_sign.bin should be returned as an out value (or provided as argument).
Note: I have not checked if there are more such names.

galak · 2020-10-05T13:42:16Z

This code only makes sense together with TFM.
So I think this is at the right place.

Of course, one could consider to have a <tfm_module>/cmake/extensions.cmake for module functions, but then that should be a general principle for all modules.
But I would do such cleanups if we move Zephyr module code into Zephyr, as discussed here:
zephyrproject-rtos/zephyr#27185

There was a general feeling in the process group about trying to have glue code, etc for modules live in the zephyr code base instead of with the module. I get that the cmake integration isn't exactly glue code, and I've got no issue with it being in the module repo. But wanted to ask the question.

tejlmand · 2020-10-06T08:29:18Z

There was a general feeling in the process group about trying to have glue code, etc for modules live in the zephyr code base instead of with the module. I get that the cmake integration isn't exactly glue code, and I've got no issue with it being in the module repo. But wanted to ask the question.

I know, just think that cleanup is outside the scope of this PR, and we should take a look at the exact structure we want in Zephyr for such code.
Some modules could end up with a lot a CMake code in Zephyr, if the end goal is to have modules with no to minimal Zephyr specific code in the module repo.

ioannisg · 2021-04-19T07:36:19Z

@tejlmand @anangl @oyvindronningstad @galak is this old PR still applicable ?

anangl · 2021-04-19T09:02:04Z

@tejlmand @anangl @oyvindronningstad @galak is this old PR still applicable ?

No. At least the PR here does not make much sense, as the modified code has been moved to the main Zephyr repository.

ioannisg · 2021-04-19T09:07:42Z

@tejlmand @anangl @oyvindronningstad @galak is this old PR still applicable ?

No. At least the PR here does not make much sense, as the modified code has been moved to the main Zephyr repository.

But how about doing this work in the main repository? Is this applicable?

oyvindronningstad · 2021-04-19T09:28:31Z

@tejlmand @anangl @oyvindronningstad @galak is this old PR still applicable ?

No. At least the PR here does not make much sense, as the modified code has been moved to the main Zephyr repository.

But how about doing this work in the main repository? Is this applicable?

It's done already

CMakeLists.txt: Provide common routine for post-build commands

7316751

... so that those commands are not duplicated in particular boards that support building with TF-M. Signed-off-by: Andrzej Głąbek <andrzej.glabek@nordicsemi.no>

anangl mentioned this pull request Sep 30, 2020

tfm_integration: Use common routine for post-build commands zephyrproject-rtos/zephyr#28806

Closed

anangl marked this pull request as ready for review September 30, 2020 10:24

ioannisg requested review from microbuilder and tejlmand October 1, 2020 10:32

ioannisg approved these changes Oct 1, 2020

View reviewed changes

erwango approved these changes Oct 1, 2020

View reviewed changes

agansari approved these changes Oct 1, 2020

View reviewed changes

oyvindronningstad approved these changes Oct 2, 2020

View reviewed changes

galak reviewed Oct 2, 2020

View reviewed changes

tejlmand suggested changes Oct 2, 2020

View reviewed changes

tejlmand suggested changes Oct 5, 2020

View reviewed changes

oyvindronningstad mentioned this pull request Jan 22, 2021

boards: Clean up TF-M signing code for nrf53 and nrf91 zephyrproject-rtos/zephyr#31456

Merged

anangl closed this Apr 19, 2021

anangl deleted the use_common_tfm_images_signing branch April 19, 2021 09:34

etienne-lms mentioned this pull request Jul 14, 2025

BL2: Add mcuboot patch to prevent build warning on FIH_PANIC #143

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

CMakeLists.txt: Provide common routine for post-build commands #14

CMakeLists.txt: Provide common routine for post-build commands #14

Uh oh!

anangl commented Sep 30, 2020

ioannisg commented Oct 1, 2020

microbuilder commented Oct 1, 2020

agansari commented Oct 1, 2020

galak left a comment

galak commented Oct 2, 2020

tejlmand Oct 2, 2020

anangl Oct 5, 2020

tejlmand Oct 5, 2020

tejlmand Oct 5, 2020

tejlmand Oct 2, 2020

anangl Oct 5, 2020

tejlmand Oct 5, 2020

tejlmand Oct 2, 2020

tejlmand Oct 5, 2020

tejlmand commented Oct 2, 2020 •

edited

Loading

tejlmand Oct 5, 2020

galak commented Oct 5, 2020

tejlmand commented Oct 6, 2020

ioannisg commented Apr 19, 2021

anangl commented Apr 19, 2021

ioannisg commented Apr 19, 2021

oyvindronningstad commented Apr 19, 2021

Labels

8 participants

	if (NS_IMAGE_MIN_VER)
	set(ADD_NS_IMAGE_MIN_VER "-d \"(1,${NS_IMAGE_MIN_VER})\"")
	else()
	set(ADD_NS_IMAGE_MIN_VER "")
	endif()

	if (SECURITY_COUNTER_S)
	set(ADD_SECURITY_COUNTER_S "-s ${SECURITY_COUNTER_S}")
	else()
	set(ADD_SECURITY_COUNTER_S "")
	endif()

CMakeLists.txt: Provide common routine for post-build commands #14

CMakeLists.txt: Provide common routine for post-build commands #14

Uh oh!

Conversation

anangl commented Sep 30, 2020

ioannisg commented Oct 1, 2020

microbuilder commented Oct 1, 2020

agansari commented Oct 1, 2020

galak left a comment

Choose a reason for hiding this comment

galak commented Oct 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tejlmand commented Oct 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

galak commented Oct 5, 2020

tejlmand commented Oct 6, 2020

ioannisg commented Apr 19, 2021

anangl commented Apr 19, 2021

ioannisg commented Apr 19, 2021

oyvindronningstad commented Apr 19, 2021

Labels

8 participants

tejlmand commented Oct 2, 2020 •

edited

Loading