Nrf security cleanups #575

SebastianBoe · 2021-11-02T17:23:41Z

Misc. cleanups done to understand nrf_security build scripts.

See commit messages for details.

nrf_security/cmake/kconfig_psa_configure.cmake

nrf_security/CMakeLists.txt

frkv · 2021-11-03T13:28:39Z

nrf_security/CMakeLists.txt

-  set(MBEDTLS_PSA_CRYPTO_KEY_ID_ENCODES_OWNER       False)
-
-  # Add a library to propagate configurations and paths to TF-M
-  add_custom_target(nrf_security_target)


I don't know all the details on the reasoning behind the nrf_security_target, but I am curious how (and if) this can be removed.

Also curious how this will run in the dual execution of the nrf_security build (once in NSPE with zephyr build system, once inside TFM/SPE build without zephyr build system)

I assume this can be cleaned up as @SebastianBoe proposes 👍

A bit of history.
The reason for this was because originally we could not pass TF-M CMake options directly to TF-M build, but only through Kconfig.
Notice this code is from March 11 (PR: #426, March 9):
ab66d4a

and the TFM_CMAKE_OPTIONS came in place in May 7:
zephyrproject-rtos/zephyr#34868

In the Kconfig indirection initial code, it was passed like this:

sdk-nrfxlib/nrf_security/Kconfig

Lines 1665 to 1667 in ab66d4a

config TFM_EXTRA_CMAKE_ARGS

string

default "-DMBEDCRYPTO_PATH=${ZEPHYR_NRFXLIB_MODULE_DIR}/nrf_security -DNRF_SECURITY_SETTINGS=$<TARGET_PROPERTY:nrf_security_target,CMAKE_ARGS>"

thus the reason for nrf_security_target.

So I think this can and should be cleaned up.

maybe part of the description I just provided above would be useful in the commit message as to why this code was looking like this, and why it can now be cleaned up.

Can help the next one going through commit history / blame to understand the history behind this cleanup.

Excellent to be able to remove complex generator expressions and additional targets!

Looking forward to trying to clean the code up more and make it more readable!

commit message re-written

tejlmand

Thanks for putting this together.

Careful on the docs though 😉

tejlmand · 2021-11-04T08:30:25Z

nrf_security/doc/prerequisites.rst

-Prerequisites
-#############


I don't think this should be deleted.

Maybe rephrased if the reason behind the guide is not clear, but the reason for this guide is to make it clear how nRF Security can be used when west is not used / installed.

For example, Zephyr has this guide on how to specify Zephyr modules when not using west:
https://docs.zephyrproject.org/latest/guides/modules.html#without-west

But the arm Mbed TLS repo is not a Zephyr module, thus if a user is not using west, then it's insufficient to add the arm Mbed TLS repo to the list of Zephyr module, cause it will not be processed as a module (it's missing zephyr/module.yml).

Therefore it's importent to inform the reader about setting ARM_MBEDTLS_PATH in this case.

Changed to retain the docs on ARM_MBEDTLS_PATH.

tejlmand · 2021-11-04T09:07:23Z

nrf_security/CMakeLists.txt

-  set(MBEDTLS_PSA_CRYPTO_KEY_ID_ENCODES_OWNER       False)
-
-  # Add a library to propagate configurations and paths to TF-M
-  add_custom_target(nrf_security_target)


I assume this can be cleaned up as @SebastianBoe proposes 👍

A bit of history.
The reason for this was because originally we could not pass TF-M CMake options directly to TF-M build, but only through Kconfig.
Notice this code is from March 11 (PR: #426, March 9):
ab66d4a

and the TFM_CMAKE_OPTIONS came in place in May 7:
zephyrproject-rtos/zephyr#34868

In the Kconfig indirection initial code, it was passed like this:

sdk-nrfxlib/nrf_security/Kconfig

Lines 1665 to 1667 in ab66d4a

config TFM_EXTRA_CMAKE_ARGS

string

default "-DMBEDCRYPTO_PATH=${ZEPHYR_NRFXLIB_MODULE_DIR}/nrf_security -DNRF_SECURITY_SETTINGS=$<TARGET_PROPERTY:nrf_security_target,CMAKE_ARGS>"

thus the reason for nrf_security_target.

So I think this can and should be cleaned up.

SebastianBoe · 2021-11-09T11:10:24Z

@tejlmand : All feedback has been addressed. Please re-assess review.

tejlmand

Thanks for putting this together 👍

A small comment to consider.

tejlmand · 2021-11-09T11:24:31Z

nrf_security/README.rst

+*************
+
+Mbed TLS is required and usually provided by west, but it can also be
+manually configured with the CMake variable ``ARM_MBEDTLS_PATH``.


Should we specify that the ARM_MBEDTLS_PATH should point to the top-level folder of the arm Mbed TLS repository ?
Or is that obvious.

@frkv thoughts ?

That is obvious IMHO.

This should be obvious, agreeing with @SebastianBoe

Don't hide the psa configs in a file. This abstraction is unnecessary. Signed-off-by: Sebastian Bøe <sebastian.boe@nordicsemi.no>

Re-use the shared code between the two macros kconfig_mbedtls_config and kconfig_mbedtls_config_val. Signed-off-by: Sebastian Bøe <sebastian.boe@nordicsemi.no>

SebastianBoe · 2021-11-15T10:35:39Z

@frkv : Can we have a +1 ?

Delete nrf_security_target as it became redundant after TFM_CMAKE_OPTIONS was introduced. Also, put the short clause in the if-else first for readability. And move the common code out of the if-else. Signed-off-by: Sebastian Bøe <sebastian.boe@nordicsemi.no>

Remove documentation of what error messages mean and inline the remaining content as it is not enough information to deserve a full chapter now. Signed-off-by: Sebastian Bøe <sebastian.boe@nordicsemi.no>

frkv

LGTM

SebastianBoe · 2021-11-15T14:02:28Z

Merged without manifest PR to save time.

greg-fer · 2021-12-07T11:54:58Z

This PR slightly broke documentation structure. Kindly please do not merge doc-required PRs without an approval from a tech writer. Follow-up task: https://projecttools.nordicsemi.no/jira/browse/NCSDK-12851

SebastianBoe · 2021-12-07T12:40:37Z

@greg-fer : Noted. Sorry.

SebastianBoe requested review from frkv and tejlmand as code owners November 2, 2021 17:23

github-actions bot added the doc-required PR must not be merged without tech writer approval. label Nov 2, 2021

SebastianBoe commented Nov 3, 2021

View reviewed changes

nrf_security/cmake/kconfig_psa_configure.cmake Show resolved Hide resolved

frkv reviewed Nov 3, 2021

View reviewed changes

nrf_security/CMakeLists.txt Show resolved Hide resolved

frkv reviewed Nov 3, 2021

View reviewed changes

tejlmand requested changes Nov 4, 2021

View reviewed changes

SebastianBoe force-pushed the nrf_security_cleanups branch from 902b7b7 to ed59c3c Compare November 9, 2021 11:08

tejlmand approved these changes Nov 9, 2021

View reviewed changes

SebastianBoe added 2 commits November 15, 2021 11:29

nrf_security: cmake: Inline kconfig_psa_configure.cmake

b930350

Don't hide the psa configs in a file. This abstraction is unnecessary. Signed-off-by: Sebastian Bøe <sebastian.boe@nordicsemi.no>

nrf_security: cmake: re-use code in kconfig_mbedtls_config

0ba5a83

Re-use the shared code between the two macros kconfig_mbedtls_config and kconfig_mbedtls_config_val. Signed-off-by: Sebastian Bøe <sebastian.boe@nordicsemi.no>

SebastianBoe force-pushed the nrf_security_cleanups branch from ed59c3c to 70a923a Compare November 15, 2021 10:34

SebastianBoe added 2 commits November 15, 2021 12:51

doc: nrf_security: Remove error message docs and inline chapter

df0d494

Remove documentation of what error messages mean and inline the remaining content as it is not enough information to deserve a full chapter now. Signed-off-by: Sebastian Bøe <sebastian.boe@nordicsemi.no>

SebastianBoe force-pushed the nrf_security_cleanups branch from 70a923a to df0d494 Compare November 15, 2021 11:52

frkv approved these changes Nov 15, 2021

View reviewed changes

SebastianBoe merged commit 043d1fd into nrfconnect:main Nov 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nrf security cleanups #575

Nrf security cleanups #575

SebastianBoe commented Nov 2, 2021

frkv Nov 3, 2021 •

edited

Loading

tejlmand Nov 4, 2021

tejlmand Nov 4, 2021 •

edited

Loading

frkv Nov 4, 2021

SebastianBoe Nov 9, 2021

tejlmand left a comment

tejlmand Nov 4, 2021

SebastianBoe Nov 9, 2021

tejlmand Nov 4, 2021

SebastianBoe commented Nov 9, 2021

tejlmand left a comment

tejlmand Nov 9, 2021

SebastianBoe Nov 9, 2021

frkv Nov 15, 2021 •

edited

Loading

SebastianBoe commented Nov 15, 2021

frkv left a comment

SebastianBoe commented Nov 15, 2021

greg-fer commented Dec 7, 2021

SebastianBoe commented Dec 7, 2021

	config TFM_EXTRA_CMAKE_ARGS
	string
	default "-DMBEDCRYPTO_PATH=${ZEPHYR_NRFXLIB_MODULE_DIR}/nrf_security -DNRF_SECURITY_SETTINGS=$<TARGET_PROPERTY:nrf_security_target,CMAKE_ARGS>"

		Prerequisites
		#############

Nrf security cleanups #575

Nrf security cleanups #575

Conversation

SebastianBoe commented Nov 2, 2021

frkv Nov 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tejlmand Nov 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tejlmand left a 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

SebastianBoe commented Nov 9, 2021

tejlmand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frkv Nov 15, 2021 • edited Loading

Choose a reason for hiding this comment

SebastianBoe commented Nov 15, 2021

frkv left a comment

Choose a reason for hiding this comment

SebastianBoe commented Nov 15, 2021

greg-fer commented Dec 7, 2021

SebastianBoe commented Dec 7, 2021

frkv Nov 3, 2021 •

edited

Loading

tejlmand Nov 4, 2021 •

edited

Loading

frkv Nov 15, 2021 •

edited

Loading