mcuboot.mk: refactor into a declarative-style makefile. #10494

jcarrano · 2018-11-28T11:06:41Z

Read this before reviewing

~~The first 3 commits are not part of this PR.~~

~~The first two commits come from #10461 . The first one is a throwaway commit to induce (i.e. increase the probability of triggering) certain kind of bugs. The third commit comes from #10492 .~~

~~Whise these commits are not strictly necessary for this PR, it allows me to test is under more conditions: build in docker, build in parallel, use "make clean all" etc.~~ Dependencies where merged

I think this is better reviewed commit-by-commit.

Contribution description

mcuboot.mk was not very makeish: phony targets and no explicit dependencies being the main violations. Also, the mcuboot rule was rewriting the elf file.

The main changes in this PR are:

Split the recipes into one target/recipe per file.
Add a separate target for the mcuboot-linked elf (and bin), so that the normal elf is not overwritten.
Remove the use of objcopy. The RIOT build system already contains an implicit rule for elf->bin conversion.
Make it clear that MCUBOOT_BIN (now renamed) is the booloader binary ONLY.

See the commit messages for more info.

Testing procedure

I only tested building because I do not have the hardware. Could someone with a nrf52 please test this?

$ cd tests/mcuboot
$ BUILD_IN_DOCKER=1 DOCKER="sudo docker" make -j clean all
for i in  ; do "make" -C /home/jcarrano/source/masterRIOT/pkg/$i clean ; done
rm -rf /home/jcarrano/source/masterRIOT/tests/mcuboot/bin/nrf52dk
rm -rf scan-build/nrf52dk
Launching build container using image "riot/riotbuild:latest".
sudo docker run --rm -t -u "$(id -u)" \
    -v '/home/jcarrano/source/masterRIOT:/data/riotbuild/riotbase' \
    -v '/home/jcarrano/source/masterRIOT/cpu:/data/riotbuild/riotcpu' \
    -v '/home/jcarrano/source/masterRIOT/boards:/data/riotbuild/riotboard' \
    -v '/home/jcarrano/source/masterRIOT/makefiles:/data/riotbuild/riotmake' \
    -v '/home/jcarrano/source/masterRIOT:/data/riotbuild/riotproject' \
    -v /etc/localtime:/etc/localtime:ro \
    -e 'RIOTBASE=/data/riotbuild/riotbase' \
    -e 'CCACHE_BASEDIR=/data/riotbuild/riotbase' \
    -e 'RIOTCPU=/data/riotbuild/riotcpu' \
    -e 'RIOTBOARD=/data/riotbuild/riotboard' \
    -e 'RIOTMAKE=/data/riotbuild/riotmake' \
    -e 'RIOTPROJECT=/data/riotbuild/riotproject' \
    -v /home/jcarrano/.gitcache:/data/riotbuild/gitcache -e GIT_CACHE_DIR=/data/riotbuild/gitcache -v /home/jcarrano/source/vanillaRIOT/.git:/home/jcarrano/source/vanillaRIOT/.git \
     \
    -w '/data/riotbuild/riotproject/tests/mcuboot/' \
    'riot/riotbuild:latest' make all 
"make" -C /data/riotbuild/riotbase/core
"make" -C /data/riotbuild/riotbase/drivers
"make" -C /data/riotbuild/riotbase/drivers/periph_common
"make" -C /data/riotbuild/riotbase/sys
"make" -C /data/riotbuild/riotbase/sys/auto_init
"make" -C /data/riotbuild/riotbase/sys/isrpipe
"make" -C /data/riotbuild/riotbase/sys/newlib_syscalls_default
"make" -C /data/riotbuild/riotbase/sys/stdio_uart
"make" -C /data/riotbuild/riotbase/sys/tsrb
"make" -C /data/riotbuild/riotboard/nrf52dk
"make" -C /data/riotbuild/riotboard/common/nrf52xxxdk
"make" -C /data/riotbuild/riotcpu/nrf52
"make" -C /data/riotbuild/riotcpu/cortexm_common
"make" -C /data/riotbuild/riotcpu/cortexm_common/periph
"make" -C /data/riotbuild/riotcpu/nrf52/periph
"make" -C /data/riotbuild/riotcpu/nrf5x_common
"make" -C /data/riotbuild/riotcpu/nrf5x_common/periph
Re-linking for MCUBoot at 0x8000...
Signing with /data/riotbuild/riotproject/tests/mcuboot/bin/nrf52dk/key.pem for version 1.1.1+1
Building application "tests_mcuboot" for "nrf52dk" with MCU "nrf52".

   text	   data	    bss	    dec	    hex	filename
   8104	    136	   2604	  10844	   2a5c	/data/riotbuild/riotproject/tests/mcuboot/bin/nrf52dk/tests_mcuboot.elf

Issues/PRs references

Depends on ~~#10461~~ and ~~#10492.~~

makefiles/mcuboot.mk

jcarrano · 2018-12-05T16:55:34Z

I rebased now that #10492 is merged. I also removed the dependency on #10461.

aabadie · 2018-12-10T09:05:24Z

I tested this PR and I confirm it fixes build problems when building tests/mcuboot using the docker image.
In master, you need arm-none-eabi-gcc installed on the build host to have a successful build. The re-link of the mcuboot firmware is also called twice in this case. This PR fixes that.

I also tested on a nrf52dk, but I have no idea what I should expect from the test. When I flash the mcuboot on the board, I have nothing on the terminal, either if using master or this PR.

jcarrano · 2018-12-10T10:44:01Z

I have no idea what I should expect from the test.

Me neither. To be honest, I don't know what anything of this is supposed to do. Maybe @kYc0o has something to say.

cladmi · 2018-12-10T12:54:37Z

I am currently not sure where the docker handling should be done. Because if you type make mcuboot it does make in docker. It comes form our current docker handling and integration.

BUILD_IN_DOCKER=1 DOCKER="sudo docker" make  -C tests/mcuboot/ mcuboot
make: Entering directory '/home/harter/work/git/RIOT/tests/mcuboot'
Launching build container using image "riot/riotbuild:latest".
sudo docker run --rm -t -u "$(id -u)" \
    -v '/home/harter/work/git/RIOT:/data/riotbuild/riotbase' \
    -v '/home/harter/work/git/RIOT/cpu:/data/riotbuild/riotcpu' \
    -v '/home/harter/work/git/RIOT/boards:/data/riotbuild/riotboard' \
    -v '/home/harter/work/git/RIOT/makefiles:/data/riotbuild/riotmake' \
    -v '/home/harter/work/git/RIOT:/data/riotbuild/riotproject' \
    -v /etc/localtime:/etc/localtime:ro \
    -e 'RIOTBASE=/data/riotbuild/riotbase' \
    -e 'CCACHE_BASEDIR=/data/riotbuild/riotbase' \
    -e 'RIOTCPU=/data/riotbuild/riotcpu' \
    -e 'RIOTBOARD=/data/riotbuild/riotboard' \
    -e 'RIOTMAKE=/data/riotbuild/riotmake' \
    -e 'RIOTPROJECT=/data/riotbuild/riotproject' \
    -v /home/harter/.gitcache:/data/riotbuild/gitcache -e GIT_CACHE_DIR=/data/riotbuild/gitcache  \
     \
    -w '/data/riotbuild/riotproject/tests/mcuboot/' \
    'riot/riotbuild:latest' make  
[sudo] password for harter:

To at least verify what happens in master, I will do a sha1 based comparison. If it produces the same checksum, the creation targets are good.

cladmi · 2018-12-10T13:38:25Z

Unfortunately, the sign file creation produces different sha1 when executed multiple times.

I used a fixed key.pem file to try getting a deterministic output, but it did not :/

My test command was the following, I first copied one generated key.pem file to tests/mcuboot and executed the following:

m -rf bin; mkdir -p bin/nrf52dk; cp -p key.pem bin/nrf52dk/key.pem; RIOT_CI_BUILD=1 make

The sha1 outputs: For the pr I created multiple signed files one after the other so using the same command and you can see the different output.

0b149b54e40e8f3e9e25ea0dc47cfe37ce55d12d  bin_ma/nrf52dk/key.pem
0b149b54e40e8f3e9e25ea0dc47cfe37ce55d12d  bin_pr/nrf52dk/key.pem
0c965daf110ccf743a34c8bb43a37b4658ca894f  bin_pr/nrf52dk/tests_mcuboot.elf
520cd8557c9fa9198947d78409762bd58f9acfed  bin_pr/nrf52dk/signed-tests_mcuboot.bin
6e7a2d0cc36677071ac5b0006589aaadaf59f775  bin_pr/nrf52dk/signed-tests_mcuboot.bin.2.bin
78da0aaf36b7a8186aa9bdc9c1013741ab29da50  bin_ma/nrf52dk/tests_mcuboot.elf
78da0aaf36b7a8186aa9bdc9c1013741ab29da50  bin_pr/nrf52dk/tests_mcuboot.mcuboot.elf
bf357b4e69a1bfb5e25d4eda00dc26366e56cdcc  bin_pr/nrf52dk/signed-tests_mcuboot.bin.3.bin
c07e503f7f0d984356557f1bcc64843e61ef2931  bin_pr/nrf52dk/signed-tests_mcuboot.bin.4.bin
c2206f002199578b9b6781d6b030fbabe68fa440  bin_pr/nrf52dk/tests_mcuboot.bin
c42f0032fe41a4401e703f361611171dbafb72d2  bin_ma/nrf52dk/signed-tests_mcuboot.bin
e0246e81ed090a15aafbe052bf637b541fe6327f  bin_ma/nrf52dk/tests_mcuboot.bin
e0246e81ed090a15aafbe052bf637b541fe6327f  bin_pr/nrf52dk/tests_mcuboot.mcuboot.bin

With this I can see the name changes and that at least here, tests_mcuboot.elf is not overwritten.

And the output is the same for the elf files master: tests_mcuboot.elf and pr: tests_mcuboot.mcuboot.elf and then also for the corresponding .bin files.

One thing to pay attention while reviewing, MCUBOOT_BIN is now referencing a different file.
Same for the

jcarrano · 2018-12-10T13:44:43Z

The signature process may involve random data.

kYc0o · 2018-12-10T13:44:59Z

Me neither. To be honest, I don't know what anything of this is supposed to do. Maybe @kYc0o has something to say.

The only thing that can be tested is an actual boot. If there's nothing even after a reboot, then it didn't succeed and something went wrong. Can you test in previous releases?

kYc0o · 2018-12-10T13:45:39Z

Unfortunately, the sign file creation produces different sha1 when executed multiple times.

Anyways, the pre-compiled binary doesn't check any signature, but only the hash of the image.

cladmi · 2018-12-10T14:12:31Z

For me, when flashed with mcuboot-flash it works both for master and for this PR.

2018-12-10 15:02:16,606 - INFO # main(): This is RIOT! (Version: 2018.10-RC1-457-gbf1e8-pr/riot/10494/refactor_into_a_declarative_style_makefile_)
2018-12-10 15:02:16,607 - INFO # Hello MCUBoot!
2018-12-10 15:02:16,611 - INFO # You are running RIOT on a(n) nrf52dk board.
2018-12-10 15:02:16,614 - INFO # This board features a(n) nrf52 MCU.
2018-12-10 15:02:16,617 - INFO # The startup address is: 0x8200

When using flash it only uses the normal firmware and startup address is: 0.

So for me it's good on the testing for the normal out of docker functionality.
For the rest I still need to do a proper review.

cladmi · 2018-12-10T14:24:02Z

makefiles/mcuboot.mk

+MCUBOOT_ELF ?= $(ELFFILE:.elf=.mcuboot.elf)
+MCUBOOT_BIN ?= $(MCUBOOT_ELF:.elf=.bin)
+
+MCUBOOT_LOADER_BIN ?= $(BINDIR)/mcuboot.bin


I prefered MCUBOOT_BIN has it is the mcuboot binary file. For me it was completely clear. Here loader does not mean anything to me, at least bootloader would be clear.

However renaming the file to keep the full mynewt.mcuboot.bin name would prevent possible issues with the filename in bin.

cladmi · 2018-12-10T14:32:20Z

makefiles/mcuboot.mk


-export IMAGE_HDR_SIZE ?= 512
+MCUBOOT_ELF ?= $(ELFFILE:.elf=.mcuboot.elf)


This is not a mcuboot_elf specific thing it is only a normal RIOT firmware but linked for slot0.
In the bootloader PR it is $(BINDIR_APP)-slot0.elf.
So maybe something like ELFFILE_MCUBOOT_SLOT0 = $(ELFFILE:.elf=.mcuboot-slot0.elf)

"normal RIOT firmware": it also has space for the header. I saw the message "Re-linking for MCUBoot at..." and assumed it was a MCUBoot-specific thing.

cladmi · 2018-12-10T14:35:28Z

makefiles/mcuboot.mk

 ifdef MCUBOOT_SLOT0_SIZE

 IMGTOOL ?= $(RIOTTOOLS)/mcuboot/imgtool.py
-override IMGTOOL := $(abspath $(IMGTOOL))
+IMGTOOL_KEYFLAGS ?= keygen -t rsa-2048


Currently I do not find necessary to add the KEYFLAGS. If it was imgtool.py specific I would not put keygen in it.
And if supposed to be generic, here you do not include the -k so cannot be replaced by an other tool anyway.

Maybe keep this change for when there is a need.

I can remove that commit.

cladmi · 2018-12-10T14:38:56Z

makefiles/mcuboot.mk

@@ -55,3 +58,4 @@ mcuboot:
 $(Q)echo "error: mcuboot not supported on board $(BOARD)!"
 $(Q)false
 endif # MCUBOOT_SLOT0_SIZE
+


Newline added here.

I'll remove it.

cladmi · 2018-12-10T14:50:00Z

makefiles/mcuboot.mk

+ $(MCUBOOT_IMAGE_ALIGN) -H $(IMAGE_HDR_SIZE) $(MCUBOOT_BIN) $(SIGN_BINFILE)
+
+ifeq ($(BUILD_IN_DOCKER),1)
+mcuboot: ..in-docker-container


This currently only works in the tests/mcuboot application as it will not pass mcuboot to the docker make command.
A solution for this is to add DOCKER_MAKECMDGOALS_POSSIBLE += mcuboot.
However, IMAGE_VERSION must be set in the Makefile and not command line.
So could add a test for this to at least not try to compile.

I will find something for this.

cladmi · 2018-12-10T15:04:00Z

For the docker thing, I did not manage to have the full behavior that I expect.

I manage to make it work when doing make mcuboot and make mcuboot mcuboot-flash but not when only doing make mcuboot-flash as can be expected.

With this, it currently works with make mcuboot or make mcuboot mcuboot-flash from any application directory and provides an error message when `

My test command is:

BUILD_IN_DOCKER=1 DOCKER="sudo docker" BOARD=nrf52dk make -C examples/hello-world/ mcuboot  mcuboot-flash

diff --git a/makefiles/mcuboot.mk b/makefiles/mcuboot.mk
index 9b0e7eac8..666e9b2f4 100644
--- a/makefiles/mcuboot.mk
+++ b/makefiles/mcuboot.mk
@@ -16,6 +16,10 @@ MCUBOOT_LOADER_BIN_MD5 ?= 0c71a0589bd3709fc2d90f07a0035ce7

 IMAGE_HDR_SIZE ?= 512

+ifeq (,$(IMAGE_VERSION))
+  $(error IMAGE_VERSION not set)
+endif
+
 $(MCUBOOT_KEYFILE) $(MCUBOOT_LOADER_BIN): $(filter clean, $(MAKECMDGOALS))

 mcuboot-create-key: $(MCUBOOT_KEYFILE)
@@ -39,6 +43,11 @@ $(SIGN_BINFILE): $(MCUBOOT_BIN) $(MCUBOOT_KEYFILE)

 ifeq ($(BUILD_IN_DOCKER),1)
 mcuboot: ..in-docker-container
+DOCKER_MAKECMDGOALS_POSSIBLE += mcuboot
+  ifneq (,$(filter environment command,$(origin IMAGE_VERSION)))
+    $(warning Setting IMAGE_VERSION from env/command line not supported in docker)
+    $(error IMAGE_VERSION should be set from the application Makefile)
+  endif
 else
 mcuboot: $(SIGN_BINFILE)
 endif # BUILD_IN_DOCKER

cladmi · 2018-12-10T15:23:34Z

Another solution for the docker integration, as there are other limitations too, is to only do in in the tests/mcuboot makefile maybe around all: mcuboot, and a comment in mcuboot.mk that it is not fully integrated in docker if configuration is changed.

Also, it was not working properly in docker before as the files were also built on the local machine where they should not. So I could split this part in a separate PR anyway.

cladmi · 2018-12-10T16:31:58Z

When using flash it only uses the normal firmware and startup address is: 0.

Actually, in master make flash flashes the firmware linked with an offset without the bootloader and signature. The result may still work as you have pre-existing data on the node.
This PR changed this behavior at least.

Not sure what the real behavior that is wanted in that case.
We could make that make flash flasher a combined file like what will be done in the riot-bootloader.
This would allow testing it in CI.

cladmi · 2019-03-04T11:00:09Z

Could you currently remove the renaming of the variables from this PR ? Going for file targets is already something big enough on its own and does not need to include naming changes/other refactoring.
Also the DOCKER handling has still issues so should be removed from the moment. It was broken and will still be.

A solution for the docker handling will be to use FLASHFILE but requires to have the file targets first.

The previous definition of IMGTOOL was unnecessarily complicated. "abspath" is not needed. Also, the export for IMAGE_HDR_SIZE is not necessary.

There is no point on defining BINFILE again. It is already done by Makefile.include.

The name MCUBOOT_BIN is confusing and not clear. It suggests it is the application binary when in reality it is just the mcuboot bootloader image.

Add rule for MCUBOOT_ELF (do not overwrite normal elf.) Similar to ELFFILE, this depends on BASELIBS. MCUBOOT_BIN is done automatically from MCUBOOT_ELF by an implicit rule. SIGN_BINFILE (the signed binary) depends on MCUBOOT_KEYFILE and not on a phony rule. All phony rules are listed at the beginning.

After the restructuring of mcuboot.mk building in docker was broken. This makes it work again (but only if on uses the mcuboot target.)

Making the rule definition conditional does not make sense, as it should not conflict if the user overrides MCUBOOT_KEYFILE (the rule will just not get used). Factor keygen arguments into IMGTOOL_KEYFLAGS. In the future a flag can be added to generate a determnistic key for reproducibility on CI builds.

jcarrano · 2019-04-02T14:18:42Z

I rebased this on top of master.

@cladmi what exactly is broken with docker?

cladmi · 2019-04-04T10:44:45Z

For docker it magically works because there is all: mcuboot in the tests/mcuboot.
I would rather not touch it for the moment and fix it in a separate PR.

There are also still other unrelated changes in this one.

stale · 2019-10-06T11:39:19Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

jcarrano requested a review from cladmi November 28, 2018 11:06

jcarrano commented Nov 28, 2018

View reviewed changes

makefiles/mcuboot.mk Outdated Show resolved Hide resolved

miri64 added Area: build system Area: Build system Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: OTA Area: Over-the-air updates labels Nov 30, 2018

jcarrano added State: waiting for other PR State: The PR requires another PR to be merged first CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable labels Dec 4, 2018

jcarrano commented Dec 4, 2018

View reviewed changes

makefiles/mcuboot.mk Outdated Show resolved Hide resolved

jcarrano force-pushed the mcuboot.mk-fixes branch from d53a60e to 85f0864 Compare December 5, 2018 16:50

jcarrano removed the State: waiting for other PR State: The PR requires another PR to be merged first label Dec 5, 2018

cladmi suggested changes Dec 10, 2018

View reviewed changes

cladmi mentioned this pull request Dec 10, 2018

bootloader: add riotboot minimal bootloader #10065

Merged

3 tasks

This was referenced Mar 4, 2019

Makefiles: add support to generate both .hex and .bin file and add FLASHFILE variable #8838

Merged

makefiles/mcuboot: prepare for when FLASHFILE is used #11112

Merged

tests/mcuboot: handle building in docker #11083

Merged

jcarrano force-pushed the mcuboot.mk-fixes branch from 193e649 to dfed9e9 Compare April 2, 2019 14:15

jcarrano removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Apr 2, 2019

jcarrano added 6 commits April 2, 2019 16:17

mcuboot.mk: remove unnecessary override, export and :=.

d2bc904

The previous definition of IMGTOOL was unnecessarily complicated. "abspath" is not needed. Also, the export for IMAGE_HDR_SIZE is not necessary.

mcuboot.mk: Do not define BINFILE.

1c7c365

There is no point on defining BINFILE again. It is already done by Makefile.include.

mcuboot.mk: replace MCUBOOT_BIN by MCUBOOT_LOADER_BIN.

38b3cbd

The name MCUBOOT_BIN is confusing and not clear. It suggests it is the application binary when in reality it is just the mcuboot bootloader image.

mcuboot.mk: support in-docker-container for mcuboot target.

b439fd9

After the restructuring of mcuboot.mk building in docker was broken. This makes it work again (but only if on uses the mcuboot target.)

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Oct 6, 2019

stale bot closed this Nov 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mcuboot.mk: refactor into a declarative-style makefile. #10494

mcuboot.mk: refactor into a declarative-style makefile. #10494

jcarrano commented Nov 28, 2018 •

edited

Loading

jcarrano commented Dec 5, 2018

aabadie commented Dec 10, 2018

jcarrano commented Dec 10, 2018

cladmi commented Dec 10, 2018 •

edited

Loading

cladmi commented Dec 10, 2018

jcarrano commented Dec 10, 2018

kYc0o commented Dec 10, 2018

kYc0o commented Dec 10, 2018

cladmi commented Dec 10, 2018

cladmi Dec 10, 2018

cladmi Dec 10, 2018

jcarrano Dec 10, 2018

cladmi Dec 10, 2018

jcarrano Dec 10, 2018

cladmi Dec 10, 2018

jcarrano Dec 11, 2018

cladmi Dec 10, 2018

cladmi commented Dec 10, 2018

cladmi commented Dec 10, 2018

cladmi commented Dec 10, 2018 •

edited

Loading

cladmi commented Mar 4, 2019

jcarrano commented Apr 2, 2019

cladmi commented Apr 4, 2019

stale bot commented Oct 6, 2019


		export IMAGE_HDR_SIZE ?= 512
		MCUBOOT_ELF ?= $(ELFFILE:.elf=.mcuboot.elf)

mcuboot.mk: refactor into a declarative-style makefile. #10494

mcuboot.mk: refactor into a declarative-style makefile. #10494

Conversation

jcarrano commented Nov 28, 2018 • edited Loading

Read this before reviewing

Contribution description

Testing procedure

Issues/PRs references

jcarrano commented Dec 5, 2018

aabadie commented Dec 10, 2018

jcarrano commented Dec 10, 2018

cladmi commented Dec 10, 2018 • edited Loading

cladmi commented Dec 10, 2018

jcarrano commented Dec 10, 2018

kYc0o commented Dec 10, 2018

kYc0o commented Dec 10, 2018

cladmi commented Dec 10, 2018

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

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

cladmi commented Dec 10, 2018

cladmi commented Dec 10, 2018

cladmi commented Dec 10, 2018 • edited Loading

cladmi commented Mar 4, 2019

jcarrano commented Apr 2, 2019

cladmi commented Apr 4, 2019

stale bot commented Oct 6, 2019

jcarrano commented Nov 28, 2018 •

edited

Loading

cladmi commented Dec 10, 2018 •

edited

Loading

cladmi commented Dec 10, 2018 •

edited

Loading