Skip to content

Conversation

gchwier
Copy link
Contributor

@gchwier gchwier commented May 26, 2025

Added automatic KMU key provisioning, when keyfile.json
file exists in the build directory.
This enables automated key provisioning during the
flashing process to enable testing nRF54L aplications using Twister.
Only applicable on nrfutil runner.

@gchwier gchwier force-pushed the grch-west-flash-with-provision branch 2 times, most recently from eb548ec to 3a452fa Compare May 27, 2025 09:26
@gchwier gchwier force-pushed the grch-west-flash-with-provision branch from 3a452fa to 47ae67c Compare May 27, 2025 10:21
nashif
nashif previously requested changes May 27, 2025
Copy link
Member

@nashif nashif left a comment

Choose a reason for hiding this comment

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

Why are we adding all this NCS stuff into Zephyr? We need to figure out a better way where Zephyr remains clean of code needed only by a downstream and supported completely there.

@gchwier
Copy link
Contributor Author

gchwier commented May 27, 2025

Why are we adding all this NCS stuff into Zephyr? We need to figure out a better way where Zephyr remains clean of code needed only by a downstream and supported completely there.

I agree with you about keeping downstream-specific code in the downstream repository, but I don't see an easy way to accomplish this (maybe to implement something like hooks that could be registered in the downstream project?)

I initially implemented this as a proof of concept in the downstream repository, but this created maintenance challenges when dealing with differences between repositories.

The primary motivation for including the NCS provisioning feature was to enable testing with Twister.
Currently, we can use pytest-harness with a multi-step process (flash with --no-reset, run provisioning, then reset the board).
There is no easy way to run tests without pytest-harness (one option might be adding post-flash scripts to the hardware map, but this also has limitations).

It's worth noting that this functionality is:

  • Limited only to the nrfutil runner
  • Contained within the nRF-specific runners files
  • Activated only when explicitly requested with the --ncs-provision flag and when the proper Kconfigs are enabled

@carlescufi
Copy link
Member

@nashif

Why are we adding all this NCS stuff into Zephyr? We need to figure out a better way where Zephyr remains clean of code needed only by a downstream and supported completely there.

Agreed, and this is already the case for almost all code in Zephyr, by extending the codebase in NCS itself.
As I see it, this one is slightly trickier than usual, because it needs to "inject" itself into the chain of operations that happens
during west flash, and the current runner infrastructure is not really extensible at all.

As @gchwier described, there are three real options in this particular case, due to the requirements of influencing the sequence of operations during flashing:

  1. Keep this in Zephyr. It is quite well contained as @gchwier described, but this is functionality that is not useful in Zephyr upstream, not in NCS
  2. Keep this whole patch in a Zephyr fork. Definitely doable, but quite ugly from a maintenance point of view
  3. Some sort of hook that is specific to this particular runner, but that is tricky because it needs to extend the command-line parameters, not just execute at a point in time. Possible, but not trivial since the extra code is using the runner's internal APIs

@bjarki-andreasen
Copy link
Contributor

bjarki-andreasen commented May 27, 2025

Looking at the PR, why is generating a keyfile put into the twister runner? writing a keyfile to a target, sure, generating it? Seems this should be handled by a downstream script invoked by CMake, and passed to the runner like any other binary files.

Case in point:

Activated only when explicitly requested with the --ncs-provision flag and when the proper Kconfigs are enabled

so, we already use Kconfigs here, just pass those to CMake instead?

@gchwier
Copy link
Contributor Author

gchwier commented May 27, 2025

Seems this should be handled by a downstream script invoked by CMake, and passed to the runner like any other binary files.

I must look into cmake scripts in sdk-nrf. I see that provisioning could be solved with:

  • adding Kconfig (e.g. SB_CONFIG_PROVISION_DEFAULT_KEYS) in downstream
  • generating keyfile during building (only downstream)
  • in Upstream - extended NrfBinaryRunner->program_hex method with self.exec_op('x-provision-keys', keyfile=str(ncs_keyfile)), when Kconfig is enabled

I will try it, thank you!

@bjarki-andreasen
Copy link
Contributor

Seems this should be handled by a downstream script invoked by CMake, and passed to the runner like any other binary files.

I must look into cmake scripts in sdk-nrf. I see that provisioning could be solved with:

  • adding Kconfig (e.g. SB_CONFIG_PROVISION_DEFAULT_KEYS) in downstream
  • generating keyfile during building (only downstream)
  • in Upstream - extended NrfBinaryRunner->program_hex method with self.exec_op('x-provision-keys', keyfile=str(ncs_keyfile)), when Kconfig is enabled

I will try it, thank you!

Check out https://github.com/zephyrproject-rtos/zephyr/tree/main/soc/nordic/nrf54h/bicr where we do something like this :)

@carlescufi
Copy link
Member

Thanks @bjarki-andreasen! If the file generation can be kept out of vanilla Zephyr then it’d be just about adding a new —provision parameter that would be compatible with both vanilla and NCS.

@gchwier
Copy link
Contributor Author

gchwier commented May 29, 2025

Thanks @bjarki-andreasen! If the file generation can be kept out of vanilla Zephyr then it’d be just about adding a new —provision parameter that would be compatible with both vanilla and NCS.

I was rather thinking to not add any new parameter. Just check the build directory and if key file exists then run key provisioning.
This is added only for nrfutil runner and does not create any dependency to KConfigs. One can provision keys for NSIB, MCUboot or later for App (TF-m?).
In sdk-nrf we can add automatic keyfile generation in build process, if proper KConfig is set.
For now we can just create key file after building, before flash with west ncs-provision --dry-run command (key file is created only once, then is used every time after flashing).

@gchwier gchwier force-pushed the grch-west-flash-with-provision branch 2 times, most recently from dcd260f to c3dbbc8 Compare May 29, 2025 14:21
Added automatic KMU key provisioning, when keyfile.json
file exists in the build directory.
This enables automated key provisioning during the
flashing process to enable testing nRF54L aplications using Twister.
Only applicable on nrfutil runner.

Signed-off-by: Grzegorz Chwierut <[email protected]>
@gchwier gchwier force-pushed the grch-west-flash-with-provision branch from c3dbbc8 to 807e0ad Compare May 29, 2025 15:31
Copy link

@gchwier
Copy link
Contributor Author

gchwier commented Jun 6, 2025

@carlescufi @nvlsianpu
Is that solution acceptable? or maybe should we add it only to Downstream as a [noup]?

I've added some changes in sdk-nrf (Kconfigs and keyfile.json generation from the build process) with some examples of usage:
nrfconnect/sdk-nrf#22516

@gchwier gchwier requested review from fabiobaltieri and nashif June 6, 2025 13:42
@nvlsianpu
Copy link
Contributor

@gchwier @carlescufi
As I understand now this runner extension is limited to action which are possible to be run in an zephyr-rtos. Users must just provide a keyfile json on them own to the build directory.
This allow to extend this functionality in NCS on nordic-own by such file delivery.

@carlescufi carlescufi changed the title west: runners: Add ncs-provision to west flash command west: runners: nrfutil: Add key file when present to west flash command Jun 10, 2025
@nashif nashif dismissed their stale review June 11, 2025 16:49

resolved

@dkalowsk dkalowsk merged commit db3c344 into zephyrproject-rtos:main Jun 11, 2025
49 checks passed
@gchwier gchwier deleted the grch-west-flash-with-provision branch June 12, 2025 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants