zephyr: refactor zephyr port #1411

gregshue · 2022-07-08T22:26:38Z

I am not yet able to verify these changes on actual hardware, so the verification has been done by inspection of the .map files.

This refactoring could go farther. I am submitting it as is to get feedback on how much farther to take it. One goal is to fully demonstrate a pattern for extending an existing repository for full integration with the Zephyr build system while minimizing the impact on existing code.

gregshue · 2022-07-11T14:57:59Z

Note: zephyrproject-rtos/zephyr#47638 is related to how this is solved.

gregshue · 2022-07-16T04:53:25Z

The workflow showed which Zephyr samples need to pass for the PR to be accepted. Some of this revealed further changes needed in this PR. Fundamentally, though, it revealed that the current configurations of zephyr/samples/subsys/mgmt/mcumgr/smp_svr/ are depending on CONFIG_MULTITHREADING=y implicitly, which is not true for NRF targets. For some reason, the current (hacky) configuration and build files implicitly generate MULTITHREADING=y for all configurations and targets but the module-structured refactoring picks up the target-specific logic and produces build failures for the miss-configuration.

The consequence: this PR will not pass the Zephyr build until the zephyr/samples/subsys/mgmt/mcumgr/smp_svr/prj.conf has been extended with CONFIG_MULTITHREADING=y. This change to Zephyr has been submitted in zephyrproject-rtos/zephyr#47896.

gregshue · 2022-07-26T23:05:22Z

The consequence: this PR will not pass the Zephyr build until the zephyr/samples/subsys/mgmt/mcumgr/smp_svr/prj.conf has been extended with CONFIG_MULTITHREADING=y. This change to Zephyr has been submitted in zephyrproject-rtos/zephyr#47896.

It turns out the MULTITHREADING default 'n' should only be applied when building in the mcuboot application. This PR has been updated with the change and should not depend on the Zephyr change anymore.

nvlsianpu · 2022-07-27T11:36:34Z

@gregshue I think this efforts should be discussed during MCUboot TSC meeting.
This might help us to understand objectives/requirements/assumptions of this refactoring.
So further what is done here, why in this way and for what it is done.

cc @d3zd3z

gregshue · 2022-07-27T16:05:21Z

I think this efforts should be discussed during MCUboot TSC meeting.

@nvlsianpu That sounds appropriate. My membership in mcuboot is in process.

Unfortunately I will be unable to attend the Aug 9 meeting, so here is the summary of what is done here and why it is this way:

Situation:
The current MCUBoot Zephyr "glue" does not seem to cleanly support configuration from other external modules. It also is not organized in a way consistent with the expectations of people working in the Zephyr ecosystem.

For context, here are some User Needs revealing the situation above:

As a downstream integrator of secure product firmware built on the Zephyr Project ecosystem, I need to be able to reuse the SHA of verified modules in my integration so I can submit any pre-generated, accessible and relevant security and safety artifacts for certification.
As a downstream integrator of secure product firmware built on the Zephyr Project ecosystem, I need to be able to generate safety and security artifacts for every module in the workspace from which the firmware image is built so that I can achieve any relevant certifications needed to sell product into markets of interest.
As a downstream integrator of secure product firmware built on the Zephyr Project ecosystem, I need module content to be configurable from out-of-module repositories so that I can have all proprietary project configurations in a module independent of any other modules.
As a downstream integrator of secure product firmware built on the Zephyr Project ecosystem I need downstream modules to provide integration "glue" in a structure consistent with the recommendations of the Zephyr Project so that I can quickly triage integration issues.
As a maintainer of a open source project that I wish to extend to also be a downstream Zephyr module, I need the Zephyr "glue" structure (e.g., build system, test structure, device tree, scripts, documentation) to not conflict with my existing structure so that I can sustain multiple integrations from the same branch of code.

Background:
Zephyr glue was added following the existing mcuboot structure. Zephyr has not provided a recommendation on how to structure downstream module "glue" for a consistent integration experience and alignment with expected required capabilities (e.g., requirements and requirement traceability, reuse and extension, aggregate documentation, aggregate test coverage and results).

Assessment:
Recently, the Zephyr TSC expressed the desire to create a recommendation for this. Though not provided as a "recommendation", a supported extensible mechanism and structure have already existed in the Zephyr ecosystem for two years - put Zephyr "glue" in a Zephyr directory structure located within the required top-level zephyr/ subdirectory that already contains module.yml. This structure is aligned with and naturally follows Zephyr ecosystem extensibility strategy. It also isolates (most?) structure differences that arise between different integration environments.

Proposal:

Refactor the Zephyr-unique "glue" into a Zephyr tree structure kept within the top-level zephyr/ subdirectory.
- Separate the Zephyr-unique project configuration from the Zephyr-unique mcuboot application glue (subsystem/service).
- Separate the Kconfig logic to support mcuboot configuration and integration by project configurations in other repositories.
Add a "west.yml" manifest file to support mcuboot development in the way Zephyr Project expects downstream modules to be developed and integrated.

See the commit comments for the easiest tracking of the refactoring changes.

gregshue · 2022-07-27T19:48:41Z

Another User Need that leads to this refactoring:
6. As a downstream integrator of secure product firmware built on the Zephyr Project ecosystem, I need all logic and data in an executable to be traceable to and verified against requirements.

The simplest way to achieve this in Zephyr is have all logic in a subsystem, resulting in an empty void main(void){}. This supports all logic (including Zephyr-unique glue logic) being testable with automated testing/mocking (i.e., ztest()). Any project-unique data (e.g., model numbers) would either be populated via Kconfigs, provided as a directly-linked symbol, or possibly registered via a SYS_INIT(). Each of these could be verified by having the empty main() conditionally replaced by test suite code.

The proposed refactoring has not been extended for automated verification of the remaining source file (keys.c - lacking logic), but doing so is directly following the same pattern.

The proposed refactoring needs guidance on location for hooks_sample.c.

The proposed refactoring naturally should relocate the remaining contents of $workspace/mcuboot/boot/zephyr/ to be in a location like $workspace/mcuboot/zephyr/tests/mcuboot_image/. This relocation was postponed waiting for alignment on the proposal.

Extend this repository to be a Zephyr application manifest-module to better support development as a downstream Zephyr module when using `west`. Verified by using `west init -m $repo --mr $branch $workspace` to initialize a west workspace, followed by a west update and a successful build of `$mcuboot/boot/zephyr` for mimxrt1064_evk. Further verified that the following testcases still pass with mcuboot options on twister: mcuboot/boot/zephyr zephyr/tests/subsys/dfu zephyr/samples/subsys/mgmt/mcumgr/smp_svr Signed-off-by: Gregory SHUE <[email protected]>

Separate `boot/zephyr/Kconfig` into the portion to be picked up through `zephyr/module.yml` (variable declarations) and the portion appropriate for a top-level Kconfig (main menu, sourcing Kconfig.zephyr). Relocate all the module-specific Kconfig content beneath `$mcuboot/zephyr/` and extend `$mcuboot/zephyr/module.yml` to reference it. Fix up mcuboot-specific internal files to get the existing builds to pass. Verified by: 1. building with `-t guiconfig` and manually verifying the relocation of mcuboot Kconfig content within the Modules:mcuboot section. 2. building a full image and verifying the .map file did not change in flash or ram size. 3. Further verified that the following testcases still pass with mcuboot options on twister: mcuboot/boot/zephyr zephyr/tests/subsys/dfu zephyr/samples/subsys/mgmt/mcumgr/smp_svr Signed-off-by: Gregory SHUE <[email protected]>

The Zephyr build system exposes to module CMakeLists.txt files the following variables: ZEPHYR_${MODULE_NAME}_MODULE_DIR ZEPHYR_${MODULE_NAME}_CMAKE_DIR where MODULE_NAME is the uppercase version of the current module. This variable is set based on the `name:` field in `module.yml`. If the `name:` field is not present, then it picks up the name from the manifest file if present, and follows other defaulting logic if not. In order to enable other modules to reference overlays in this module the module name must be fixed by setting it in `module.yml`. Verified by: 1. setting the name to the value "mcuboot2" and verifying the build artifacts produced `$build/modules/mcuboot2`. 2. setting the name to the value "mcuboot" and verifying the build artifacts produced `$buildmodules/mcuboot` 3. Further verified that the following testcases still pass with mcuboot options on twister: mcuboot/boot/zephyr zephyr/tests/subsys/dfu zephyr/samples/subsys/mgmt/mcumgr/smp_svr Signed-off-by: Gregory SHUE <[email protected]>

In the Zephyr directory structure the module-level include is located as a subdirectory of the module-level CMakeLists.txt. For a general pattern suitable for extending existing repositories the Zephyr directory structure must exist within the `zephyr/` directory along with `module.yml`. This is necessary to avoid conflicts with existing repository directory structures and build files. Verified by: 1. Building `boot/zephyr` for mimxrt1064_evk and verifying that the zephyr image did not change in sizes. 2. Further verified that the following testcases still pass with mcuboot options on twister: mcuboot/boot/zephyr zephyr/tests/subsys/dfu zephyr/samples/subsys/mgmt/mcumgr/smp_svr Signed-off-by: Gregory SHUE <[email protected]>

Refactor `boot/zephyr/CMakeLists.txt` and `boot/bootutil/zephyr/CMakeLists.txt` into more standard subsystems beneath `$mcuboot/zephyr/`. This required changing which CMake file was pointed to by `module.yml`. Verified by: 1. building `$mcuboot/boot/zephyr` for mimxrt1064_evk and verifying the map file reported the same output and the same functions exist. (Since they are linked in a different order the sizes may be different by a few padding bytes. 2. Further verified that the following testcases still pass with mcuboot options on twister: mcuboot/boot/zephyr zephyr/tests/subsys/dfu zephyr/samples/subsys/mgmt/mcumgr/smp_svr Signed-off-by: Gregory SHUE <[email protected]>

Relocate `$mcuboot/boot/zephyr/*.c` files to `$mcuboot/zephyr/subsys/mcuboot_svc/` to become a reusable subsystem. NOTE: keys.c was not put into `mcuboot_svc` as this will need to be project-specific and may need to be kept in a different module. Verified by: 1. Building `$mcuboot/boot/zephyr` and verifying the zephyr.map files showed the same functions and that the flash an ram usage was the same. 2. Further verified that the following testcases still pass with mcuboot options on twister: mcuboot/boot/zephyr zephyr/tests/subsys/dfu zephyr/samples/subsys/mgmt/mcumgr/smp_svr Resolve mcu-tools#1410 Signed-off-by: Gregory SHUE <[email protected]>

nvlsianpu

I'm just on review beginning.

I don't fully understand what this PR is doing with current port nor what is improved.
Brief explanation is welcome ( saw lot of cases described).

nvlsianpu · 2022-10-07T15:20:53Z

west.yml

@@ -0,0 +1,32 @@
+# Copyright (c) 2022 Legrand North America, LLC.


Why it is done?
In zephyr project the MCUboot is just one of submodule. So zephyr-rtos manifest points given MCUboot version.

This PR is to resolve MCUBoot issue #1410.

The problem is the Zephyr port of MCUBoot does not allow me to configure and build an MCUBoot image from my proprietary workspace application module. This is different behavior from most(?) other Zephyr (sub)modules and prevents me from keeping all of my project configurations and builds in one repository. This refactoring aligns the Zephyr port with the Zephyr configuration and build system modularity and enables standard Zephyr module behavior.

This PR is to resolve MCUBoot issue #1410.

The problem is the Zephyr port of MCUBoot does not allow me to configure and build an MCUBoot image from my proprietary workspace application module. This is different behavior from most(?) other Zephyr (sub)modules and prevents me from keeping all of my project configurations and builds in one repository.

I believe this is solved by zephyrproject-rtos/zephyr#51166

@tejlmand I tried your suggestion but it does not solve my scenario. It missed picking up boards/ configs within APPLICATION_CONFIG_DIR.

nvlsianpu · 2022-10-07T15:27:58Z

boot/bootutil/zephyr/CMakeLists.txt

 zephyr_interface_library_named(MCUBOOT_BOOTUTIL)

-target_include_directories(MCUBOOT_BOOTUTIL INTERFACE
+zephyr_include_directories(MCUBOOT_BOOTUTIL INTERFACE


why this change?

Because the repartitioned build system couldn't see this otherwise, and this is a zephyr-specific build file.

nvlsianpu · 2022-10-07T15:31:15Z

zephyr/module.yml

 samples:
  - boot/zephyr
 build:
+  kconfig: zephyr/Kconfig


Why can't refer to ../boot/zephyr/Kconfig ?
Was this move neccessery?

In the original implementation the boot/zephyr/Kconfig file mixed two layers into one file. First, the top-level (executable-specific) build Kconfig declarations, which is optional and usually does little more than sources "Kconfig.zephyr". Second, the module-level Kconfig declarations, which declare controls for configuring subsystems. Combining them caused problems when configuring and building my MCUBoot executable from outside of the MCUBoot repository.

nvlsianpu · 2022-10-07T15:50:14Z

cc @tejlmand

gregshue · 2022-10-07T23:47:44Z

Much of this refactoring was to separate the Zephyr-specific MCUBoot application (subsystem/service) away from the executable configuration to become aligned with the composability inherent in Zephyr.

I chose to refactor the glue into the top-level zephyr/ directory tree for several reasons:

Align the source entry point structure with what Zephyr users are familiar with.
Follow the standard Zephyr tree needed for unit-testing the Zephyr port glue code.
Set up to follow future Zephyr processes and structure for safety/security certification readiness.
Establish a demonstration and test case in zephyrproject-rtos for extending pre-existing repositories with top-level directories that conflict with the Zephyr directory structure in Zephyr's example-application. This helps fulfill @nashif's request last summer for a recommended pattern for downstream modules to follow.

tejlmand · 2022-10-13T06:50:36Z

Haven't looked at all those changes, but based on the description:

The problem is the Zephyr port of MCUBoot does not allow me to configure and build an MCUBoot image from my proprietary workspace application module. This is different behavior from most(?) other Zephyr (sub)modules and prevents me from keeping all of my project configurations and builds in one repository.

and #1410:

The MCUboot project is an application repository downstream of the Zephyr repository, but it does not support being built and configured from application modules that are further downstream. This requires clearly separating the project configuration portion of boot/zephyr/ from the reusable, configurable "application" service (i.e., subsystem).

I believe this is solved by zephyrproject-rtos/zephyr#51166 and I don't see the need for this PR.
To my understanding, this PR is similar to proposing changes to samples/hello_world to be more configurable downstream.

Also note, Zephyr, and thus MCUboot supports the use of APPLICATION_CONFIG_DIR which exactly provides the functionality requested in this PR.

Further remember, MCUboot is not the only sample that might need such reconfiguration, so could OpenAMP and RPMsg which are samples often used in multi core SoCs.
But I don't believe it's feasible to do same amount of work in every case as is proposed in this PR.

tejlmand

So far not convinced, see #1411 (comment)

But if @nvlsianpu thinks this change is considered important I can lift the -1 and do a proper in detailed review.

gregshue · 2022-10-13T13:26:28Z

I believe this is solved by zephyrproject-rtos/zephyr#51166

I am not building this as an extras image, and I expect to build and configure this in exactly the same way as other images. Having the option to use sysbuild is fine. Not being able to configure & build this in exactly the same way is a problem.

Also note, Zephyr, and thus MCUboot supports the use of APPLICATION_CONFIG_DIR which exactly provides the functionality requested in this PR.

APPLICATION_CONFIG_DIR does support referencing configuration files from multiple other directories, so it doesn't address the full scope of the problem. I also don't know if it expands ZEPHYR_${MODULE_NAME_UPPER}_MODULE_DIR, needed for reliably referencing directories in other modules.

This PR solves multiple problems. Only one was entered as a top-level issue. See items 1-6 above. Sysbuild cannot address all of these (e.g., #4, #5).

Perhaps I need to enter the other items as issues and reference fixing them also in this PR?

gregshue · 2022-10-13T17:28:18Z

In PR #1489 (#1489) I have taken advantage of the just-merged of the fix for Zephyr's issue #41830 and the APPLICATION_CONFIG_DIR. These reduced that solution to 17 files (touched and created).

gregshue · 2022-10-19T19:39:27Z

@tejlmand I discovered that APPLICATION_CONFIG_DIR does not solve the problem when board overlay files are also needed. It seems the build generates an incorrect configuration when APPLICATION_CONFIG_DIR is used and CONF_FILE is set before find_package(Zephyr).

When I tried list(APPEND CONF_FILE ${APPLICATION_CONFIG_DIR}/prj.conf) the build no longer picked up the CONFIG_NRFX_QSPI=n from the board-specific file under ${APPLICATION_CONFIG_DIR}/boards/.

gregshue · 2022-10-25T23:46:15Z

This PR is superseded by #1489. Reviews and discussions should continue there.

gregshue force-pushed the feature/1410_refactor_zephyr_port branch 2 times, most recently from 932aa0e to 70cc399 Compare July 8, 2022 22:50

gregshue changed the title ~~Feature/1410 refactor zephyr port~~ zephyr: refactor zephyr port Jul 11, 2022

gregshue force-pushed the feature/1410_refactor_zephyr_port branch from 70cc399 to e18513d Compare July 16, 2022 04:37

gustavonihei added the area: zephyr Affects the Zephyr port label Jul 26, 2022

gregshue force-pushed the feature/1410_refactor_zephyr_port branch from e18513d to 0fc1c9f Compare July 26, 2022 17:45

gregshue force-pushed the feature/1410_refactor_zephyr_port branch from 0fc1c9f to a1cf15b Compare July 27, 2022 04:34

nvlsianpu self-requested a review July 27, 2022 06:03

gregshue force-pushed the feature/1410_refactor_zephyr_port branch from a1cf15b to d802988 Compare July 27, 2022 14:29

gregshue force-pushed the feature/1410_refactor_zephyr_port branch 5 times, most recently from 989c115 to 054faa6 Compare September 30, 2022 19:08

legrand-gregshue added 6 commits October 4, 2022 16:29

gregshue force-pushed the feature/1410_refactor_zephyr_port branch from 054faa6 to ac37fb2 Compare October 4, 2022 23:53

nvlsianpu requested a review from d3zd3z October 7, 2022 15:22

nvlsianpu reviewed Oct 7, 2022

View reviewed changes

nvlsianpu requested a review from nordicjm October 10, 2022 13:11

tejlmand suggested changes Oct 13, 2022

View reviewed changes

gregshue closed this Oct 25, 2022

		@@ -0,0 +1,32 @@
		# Copyright (c) 2022 Legrand North America, LLC.

zephyr: refactor zephyr port #1411

zephyr: refactor zephyr port #1411

Uh oh!

Conversation

gregshue commented Jul 8, 2022

Uh oh!

gregshue commented Jul 11, 2022

Uh oh!

gregshue commented Jul 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gregshue commented Jul 26, 2022

Uh oh!

nvlsianpu commented Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gregshue commented Jul 27, 2022

Uh oh!

gregshue commented Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nvlsianpu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nvlsianpu commented Oct 7, 2022

Uh oh!

gregshue commented Oct 7, 2022

Uh oh!

tejlmand commented Oct 13, 2022

Uh oh!

tejlmand left a comment

Choose a reason for hiding this comment

Uh oh!

gregshue commented Oct 13, 2022

Uh oh!

gregshue commented Oct 13, 2022

Uh oh!

gregshue commented Oct 19, 2022

Uh oh!

gregshue commented Oct 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gregshue commented Jul 16, 2022 •

edited

Loading

nvlsianpu commented Jul 27, 2022 •

edited

Loading

gregshue commented Jul 27, 2022 •

edited

Loading