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

[WIP] Multi image building #430

Closed
wants to merge 2 commits into from

Conversation

SebastianBoe
Copy link
Contributor

WIP support for multi-image building.

Must be applied simultaneously with zephyrproject-rtos/zephyr#13672

See the Zephyr PR for details.

@utzig
Copy link
Member

utzig commented Mar 5, 2019

Some of those changes were already merged as part of 048168a, please rebase!

@SebastianBoe
Copy link
Contributor Author

Done

@SebastianBoe SebastianBoe changed the title [WIP] Multi image [WIP] Multi image building Mar 14, 2019
@SebastianBoe SebastianBoe force-pushed the multi_image branch 2 times, most recently from 640c107 to c57debc Compare March 14, 2019 14:57
@nvlsianpu nvlsianpu self-requested a review March 28, 2019 12:32
@@ -0,0 +1,39 @@
if(CONFIG_BOOTLOADER_MCUBOOT)
Copy link

Choose a reason for hiding this comment

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

Might want to drop a README in this directory or add a comment to this file saying that the zephyr directory is for use of mcuboot as a zephyr module (https://docs.zephyrproject.org/latest/application/index.html#modules-external-projects), and that boot/zephyr is where the Zephyr-specific application code is, to avoid confusing newcomers who don't know to look for boot/zephyr yet.

I've had to clear up confusion that CONFIG_BOOTLOADER_MCUBOOT should not be enabled when building mcuboot itself, but is rather an option that's enabled by applications using MCUboot; this file muddies that distinction a bit, so a comment would be nice to clear up that what's going on here is kind of complex:

  1. Application CMakeLists.txt
  2. Application boilerplate.cmake
  3. Application zephyr_modules.cmake
  4. This file
  5. MCUboot CMakeLists.txt (boot/zephyr/CMakeLists.txt)
  6. MCUboot boilerplate.cmake
    ...

if (${require_build})
add_subdirectory(${MCUBOOT_BASE}/boot/zephyr ${CMAKE_CURRENT_BINARY_DIR}/mcuboot)

# TODO: Assert that the bootloader and image use the same key.
Copy link

Choose a reason for hiding this comment

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

Tricky: multiple keys are supported simultaneously.

--key ${MCUBOOT_BASE}/${CONFIG_BOOT_SIGNATURE_KEY_FILE}
--header-size ${CONFIG_TEXT_SECTION_OFFSET}
--align ${DT_FLASH_WRITE_BLOCK_SIZE}
--version 0.1 # TODO: Configurable?
Copy link

Choose a reason for hiding this comment

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

TODO: Configurable?

I think so. One could set it at CMake time; maybe even let the environment override that. That'd be pretty convenient on macOS and Linux at least.

Copy link
Contributor

@hakonfam hakonfam May 20, 2019

Choose a reason for hiding this comment

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

Could you elaborate the benefits of not simply having this as a Kconfig option?

Choose a reason for hiding this comment

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

I'm thinking of things like developer workflows where you want to update this on a build-by-build basis every few minutes, without having to go through the overhead of running menuconfig. An environment variable makes that pretty easy -- not sure if it's possible to have a kconfig option fall back to an env var dynamically, but I believe not. OTOH from a reproducible builds perspective I take your point. Tradeoffs here, your call.

--header-size ${CONFIG_TEXT_SECTION_OFFSET}
--align ${DT_FLASH_WRITE_BLOCK_SIZE}
--version 0.1 # TODO: Configurable?
--slot-size 0x32000 # TODO: Configurable?
Copy link

Choose a reason for hiding this comment

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

Not configurable; this is from DT (DT_FLASH_AREA_IMAGE_0_SIZE).

Copy link
Contributor

@hakonfam hakonfam May 20, 2019

Choose a reason for hiding this comment

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

Will fix


# TODO: Assert that the bootloader and image use the same key.

set(SIGNED_IMAGE signed.hex)
Copy link

Choose a reason for hiding this comment

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

Would you mind zephyr.signed.hex, to match the default from west sign?


if MCUBOOT_BUILD_STRATEGY_USE_HEX_FILE

config MCUBOOT_HEX_FILE
Copy link

Choose a reason for hiding this comment

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

I don't think you meant this to be a choice option; did you? Shouldn't it be moved after the endchoice?

endif # BOOTLOADER_MCUBOOT

if MCUBOOT || BOOTLOADER_MCUBOOT
# TODO: Support sharing Kconfig configuration between images
Copy link

Choose a reason for hiding this comment

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

That is a really interesting idea!

@@ -60,14 +60,6 @@ config BOOT_SIGNATURE_TYPE_ECDSA_P256

endchoice

config BOOT_SIGNATURE_KEY_FILE
Copy link

Choose a reason for hiding this comment

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

Does this break the build when MCUboot is built as a single image?

# TODO: Configurable?
set_property(GLOBAL APPEND PROPERTY
HEX_FILES_TO_MERGE
${PROJECT_BINARY_DIR}/zephyr/${KERNEL_HEX_NAME}
Copy link

Choose a reason for hiding this comment

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

Why isn't zephyr_add_executable() taking care of these?

set_property(
GLOBAL
PROPERTY
KEY_FILE
Copy link

Choose a reason for hiding this comment

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

Where is this used?

@d3zd3z
Copy link
Member

d3zd3z commented Apr 18, 2019

Unless I'm misreading something, the commit is titled "dump: dump\n\ndump". Perhaps a better description is in order.

I'm also wondering if we need a better name for this than "multi image", since we are using multi image to refer to having multiple images that MCUboot knows how to load. I'm not sure what terms are best here. But, "multi-image" from MCUboot's perspective makes sense as MCUboot itself handling multiple images. Maybe "multi build"? I'm not real sure.

Migrate to support multi-image building.

Must be applied simultaneously with zephyrproject-rtos/zephyr#13672

See the Zephyr PR for details.

Signed-off-by: Sebastian Bøe <[email protected]>
@SebastianBoe
Copy link
Contributor Author

Unless I'm misreading something, the commit is titled "dump: dump\n\ndump". Perhaps a better description is in order.

The commit message has been improved.

@SebastianBoe
Copy link
Contributor Author

I'm also wondering if we need a better name for this than "multi image", since we are using multi image to refer to having multiple images that MCUboot knows how to load. I'm not sure what terms are best here. But, "multi-image" from MCUboot's perspective makes sense as MCUboot itself handling multiple images. Maybe "multi build"? I'm not real sure.

Since these are two different domains, I'm hoping it will always be obvious from context what we are talking about. It will be difficult to change this terminology as it is in wide use already. To disambiguate when not obvious from context we can try to be more verbose, e.g. multi-image building.

Signed-off-by: Håkon Øye Amundsen <[email protected]>
@utzig
Copy link
Member

utzig commented Jul 12, 2019

@SebastianBoe Please rebase on latest master!

@SebastianBoe
Copy link
Contributor Author

Closing, zephyrproject-rtos/zephyr#13672 was not accepted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants