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

west: bootloader integration for MCUboot #12903

Merged
merged 3 commits into from
Feb 7, 2019

Conversation

mbolivar
Copy link
Contributor

@mbolivar mbolivar commented Jan 31, 2019

This pull request makes it significantly easier for users to build Zephyr applications as signed MCUboot payloads. The main addition is a new extension command, imgtool-sign. The rest is just moving code around.

Assuming that there is an MCUboot repository in ~/src/mcuboot, this series makes it possible to build and flash MCUboot itself, as well as Zephyr's hello_world app as an MCUboot signed image, with the following commands (for nrf52840_pca10056, though other boards should work as well):

# Build and flash MCUboot for nrf52840_pca10056:
west build -b nrf52840_pca10056 -s ~/src/mcuboot/boot/zephyr -d build-mcuboot
west flash -d build-mcuboot

# Build and flash Zephyr hello_world as a child image to be loaded by
# MCUboot:
west build -b nrf52840_pca10056 -s zephyr/samples/hello_world -d build-hello \
           -- -DCONFIG_BOOTLOADER_MCUBOOT=y
west sign -t imgtool -d build-hello -- --key ~/src/mcuboot/root-rsa-2048.pem
west flash -d build-hello --hex-file zephyr.signed.hex

If doing this for the first time, it's smart to completely erase the flash on the chip, e.g. with nrfjprog --eraseall.

@mbolivar mbolivar added DNM This PR should not be merged (Do Not Merge) area: West West utility labels Jan 31, 2019
@mbolivar mbolivar added this to the v1.14.0 milestone Jan 31, 2019
@codecov-io
Copy link

codecov-io commented Jan 31, 2019

Codecov Report

Merging #12903 into master will increase coverage by 0.15%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12903      +/-   ##
==========================================
+ Coverage   48.62%   48.78%   +0.15%     
==========================================
  Files         313      313              
  Lines       46484    46484              
  Branches    10728    10728              
==========================================
+ Hits        22604    22678      +74     
+ Misses      19413    19310     -103     
- Partials     4467     4496      +29
Impacted Files Coverage Δ
include/logging/log_backend.h 45.65% <0%> (ø) ⬆️
subsys/logging/log_output.c 47.58% <0%> (+0.37%) ⬆️
drivers/usb/device/usb_dc_native_posix.c 12.09% <0%> (+1.39%) ⬆️
subsys/usb/usb_descriptor.c 53.84% <0%> (+53.84%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a447c8...b02f5d0. Read the comment docs.

@SebastianBoe
Copy link
Collaborator

SebastianBoe commented Jan 31, 2019

At a glance, this looks reasonable.

I'm sure you would appreciate being aware that there exists a prototype that solves the same problem, but from CMake instead of from west.

There, to get exactly the same flash contents as demonstrated above you do:

# Build and flash both MCUboot and the signed child image hello_world:
cmake -H$ZEPHYR_BASE/samples/hello_world -DCONFIG_BOOTLOADER_MCUBOOT=y -Bbuild
ninja -C build flash

But of course, this requires a big overhaul, and is therefore taking some time to get ready for merging:

master...hakonfam:mulit_image_mcuboot_support

As discussed a while back, this is not a problem, as by-design and agreement we will have duplicated features across west and CMake.

@mbolivar mbolivar removed the DNM This PR should not be merged (Do Not Merge) label Jan 31, 2019
@mbolivar
Copy link
Contributor Author

I've removed the DNM now that the dependency is merged -- @carlescufi @tejlmand mind taking a look?

@SebastianBoe:

I'm sure you would appreciate being aware that there exists a prototype that solves the same problem, but from CMake instead of from west.

I do appreciate it! Thank you for letting me know. Perhaps we have an opportunity to collaborate by bringing mcuboot in as a west project in the zephyr manifest and sharing the codebase.

@carlescufi
Copy link
Member

I haven't reviewed this yet, but I have a comment after a quick chat with @nashif. Doesn't imgtool_sign.py really belong in MCUboot? now that we have support for external commands, we could contribute this to upstream MCUboot?

@mbolivar
Copy link
Contributor Author

mbolivar commented Jan 31, 2019

Doesn't imgtool_sign.py really belong in MCUboot? now that we have support for external commands, we could contribute this to upstream MCUboot?

I thought about that. The answer for now is "no", because this relies on code in the runners package to do build system inspection, which cannot be easily or cleanly imported from an extension command in another package project. And it's pretty zephyr specific anyway, so I'd rather have it be something we control and can manage atomically with build system changes.

Long term if we split some of this code into a zephyr_rtos package we can upload to PyPI and import from code in MCUboot (assuming MCUboot is part of the Zephyr manifest as a project), then we can make that happen. But that would imply freezing a fair number of implementation details about our build system so I think it would be a mistake at this point.

@SebastianBoe
Copy link
Collaborator

I'd also say no.

imgtool is a script that is run on the Zephyr application, it modifies the application such that it can be booted by MCUBoot, just like CONFIG_BOOTLOADER_MCUBOOT does:

https://github.com/zephyrproject-rtos/zephyr/blob/master/Kconfig.zephyr#L342

Marti Bolivar added 3 commits February 6, 2019 17:23
This is a prep work patch for adding another command.  Refactor
build.py to use a new Forceable superclass and find_build_dir() helper
routine. Fix a help string while we are here.

Signed-off-by: Marti Bolivar <[email protected]>
Pull out some more common functionality we will need elsewhere into a
separate file.

Signed-off-by: Marti Bolivar <[email protected]>
This command is useful for signing binaries for loading by a
bootloader. At present, only MCUboot's "imgtool" is supported, but it
would be straightforward to add support for additional tools.

Using this command instead of "plain" imgtool avoids looking up any
numbers for the flash write block size, text section offset, or slot
size to get a signed binary. All users need to specify is the location
of the signing key.

This greatly improves usability for those unfamiliar with MCUboot, or
even experienced users who have to deal with multiple flash partition
layouts, boards, etc.

The command works by inspecting state in the Zephyr build system, some
of which is also provided by the runner package.

Signed-off-by: Marti Bolivar <[email protected]>
@mbolivar
Copy link
Contributor Author

mbolivar commented Feb 7, 2019

@nashif @carlescufi please take a look again?

As discussed elsewhere, I made this a generic command which takes a backend tool as argument. It should be suitable for extension with additional backends, such as the nrf tooling for the non-mcuboot bootloader.

Here is the updated usage showing how it works now:

$ west build -b nrf52840_pca10056 -s ~/src/mcuboot/boot/zephyr -d build-mcuboot
$ west flash -d build-mcuboot
$ west build -b nrf52840_pca10056 -s zephyr/samples/hello_world -d build-hello -- -DCONFIG_BOOTLOADER_MCUBOOT=y
$ west sign -t imgtool -d build-hello -- --key ~/src/mcuboot/root-rsa-2048.pem
$ west flash -d build-hello --hex-file zephyr.signed.hex

@vikrant8052
Copy link
Contributor

vikrant8052 commented Feb 7, 2019

@mbolivar
it will be great if background script assume some default location of #mcuboot directory.
If it is possible then we could make single command using which we could flash " bootloader + signed.hex " in one shot. Isn't it ?

@vikrant8052
Copy link
Contributor

vikrant8052 commented Feb 7, 2019

  1. west build
  2. west bflash

bflash -> (background Linux bash script implementation just for e.g.)

MCUBOOT="/home/user/projects/mcuboot"
MCUBOOT_HEX="$MCUBOOT/boot/zephyr/build/zephyr/zephyr.hex"
MCUBOOT_IMGTOOL="$MCUBOOT/scripts/imgtool.py"
MCUBOOT_KEY="$MCUBOOT/root-rsa-2048.pem"

nrfjprog --eraseall -f nrf52

nrfjprog --program $MCUBOOT_HEX -f nrf52

$MCUBOOT_IMGTOOL sign --key $MCUBOOT_KEY --header-size 0x200 \
--align 8 --version 1.0  --slot-size 0x69000 \
$PWD/zephyr/zephyr.hex $PWD/zephyr/signed.hex

#signed.bin is required for #DFU_OTA via nRFConnect Android App

$MCUBOOT_IMGTOOL sign --key $MCUBOOT_KEY --header-size 0x200 \
--align 8 --version 1.0  --slot-size 0x69000 \
$PWD/zephyr/zephyr.bin $PWD/zephyr/signed.bin

nrfjprog --program $PWD/zephyr/signed.hex -f nrf52

nrfjprog --reset -f nrf52

@carlescufi
Copy link
Member

@hakonfam @SebastianBoe could you please re-review this one? It's been made more generic by renaming the command west sign and adding a -t,--tool option that one needs to specify in order to select a signing tool. For now the only one supported is -t imgtool as per MCUboot's imgtool

@carlescufi
Copy link
Member

@Vikrant8051 Flashing the bootloader + signed hex is an interesting idea but ultimately I think this might be pushing it at this point, because the west commands today only deal with the current build, which corresponds to the app, since it parses the CMakeCache in a build folder to achieve that.
I think automating this via a shell script or similar on top of west is the right way to go at this point.

Copy link
Member

@carlescufi carlescufi left a comment

Choose a reason for hiding this comment

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

LGTM in the current state

@carlescufi carlescufi requested a review from nashif February 7, 2019 13:15
@nashif nashif merged commit d1780aa into zephyrproject-rtos:master Feb 7, 2019
@mbolivar
Copy link
Contributor Author

mbolivar commented Feb 7, 2019

it will be great if background script assume some default location of #mcuboot directory.
If it is possible then we could make single command using which we could flash " bootloader + signed.hex " in one shot. Isn't it ?

@Vikrant8051 I agree but we need to wait to put the mcuboot repository into the west manifest to really get that done. Let's start here for LTS and make it better during the next development cycle?

You may also be interested in the work that @SebastianBoe is working on: master...hakonfam:mulit_image_mcuboot_support

@vikrant8052
Copy link
Contributor

@mbolivar sure !!

@mbolivar
Copy link
Contributor Author

mbolivar commented Feb 7, 2019

@mbolivar sure !!

Great, thanks!

@vikrant8052
Copy link
Contributor

vikrant8052 commented Feb 14, 2019

@mbolivar
Not able to execute
west sign -t imgtool -d build-app -- --key /home/vikrant/zephyrproject/mcuboot/root-rsa-2048.pem

I also tried ...
west sign -t /home/vikrant/zephyrproject/mcuboot/scripts/imgtool -d build-app -- --key /home/vikrant/zephyrproject/mcuboot/root-rsa-2048.pem

but no success.

Error:


usage: west sign [-h] [-d BUILD_DIR] [-f] [-t {imgtool}] [-p TOOL_PATH]
                 [--bin] [-B BIN] [--hex] [-H HEX]
                 [tool_opt [tool_opt ...]]
west sign: error: argument -t/--tool: invalid choice: '/home/vikrant/zephyrproject/mcuboot/scripts/imgtool' (choose from 'imgtool')

@SebastianBoe
Copy link
Collaborator

pip3 install imgtool

west sign -t imgtool -d build-app -- --key ~/src/mcuboot/root-rsa-2048.pem

@vikrant8052
Copy link
Contributor

@SebastianBoe Thanks for such prompt response. Now I can do that.

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

Successfully merging this pull request may close these issues.

6 participants