[riot-notifications] [RIOT-OS/RIOT] boards/frdm-kw41z: add riotboot (#11562)

Gaëtan Harter notifications at github.com
Wed May 22 12:13:29 CEST 2019


cladmi requested changes on this pull request.



> @@ -31,8 +31,11 @@ endif
 # Configuration for OpenOCD v0.10.0 and newer
 export OPENOCD_CONFIG ?= $(RIOTBOARD)/common/frdm/dist/openocd-$(CPU_FAMILY).cfg
 
-# Check the flash configuration field before flashing
+ifneq (,$(filter riotboot/%, $(MAKECMDGOALS)))
+export PRE_FLASH_CHECK_SCRIPT = $(RIOTCPU)/$(CPU)/dist/check-fcfield-bin.sh

For this, to allow doing `FLASHFILE=the_binary` which is done in `tests/riotboot` for example, it would be better to have global script that handles if its a binary/hex/elf.

Or could be deduced from the file extension by using maybe `$(suffix $(FLASHFILE))`

> +# unified OpenOCD script (dist/tools/openocd/openocd.sh).
+#
+# Syntax: check-fcfield-hex.sh $HEXFILE
+#
+# @author       Jonas Remmert <j.remmert at phytec.de>
+# @author       Johann Fischer <j.fischer at phytec.de>
+# @author       Joakim Nohlgård <joakim.nohlgard at eistec.se>
+# @author       Francisco Molina <francisco.molina at inria.fr>
+
+if [ $# -ne 1 ]; then
+    echo "Usage: $0 BINFILE"
+    echo "Checks the flash configuration field protection bits to avoid flashing a locked image to a Kinetis MCU (protection against accidental bricking)."
+    exit 2
+fi
+
+if [ -n "${IMAGE_OFFSET}" ]; then

I am not sure it makes sense to do it if `IMAGE_OFFSET` is set.
As it is basically the bootloader jumping to an address, it could jump to the wrong address anyway and then what you flashed in rom is useless.

I do not really know what the `fcfield` is doing and how it is used, so its hard to really give info here.
Can you really brick a board if you jump to an invalid address as the next reboot will execute the "valid" bootloader?

> +#
+# Syntax: check-fcfield-hex.sh $HEXFILE
+#
+# @author       Jonas Remmert <j.remmert at phytec.de>
+# @author       Johann Fischer <j.fischer at phytec.de>
+# @author       Joakim Nohlgård <joakim.nohlgard at eistec.se>
+# @author       Francisco Molina <francisco.molina at inria.fr>
+
+if [ $# -ne 1 ]; then
+    echo "Usage: $0 BINFILE"
+    echo "Checks the flash configuration field protection bits to avoid flashing a locked image to a Kinetis MCU (protection against accidental bricking)."
+    exit 2
+fi
+
+if [ -n "${IMAGE_OFFSET}" ]; then
+    FCFIELD=$((${RIOTBOOT_HDR_LEN}+${VTOR_OFFSET}))

Somehow here, I do not really like that it is hardwritten `RIOTBOOT_HDR_LEN`. I know it was in `mcuboot` but do not like it. It is in theory for the script more a "firmware_offset_inside_the_binary" which may be set globally.

But depends if we really need to check anyway.

> @@ -90,7 +90,7 @@ test-offset_%: $(BINDIR)/$(APPLICATION)_offset_%.elf
 	$(Q)echo -n "Test compilation with offset $*: "
 	$(Q)\
 	TEST_START_ADDR=$$($(PREFIX)readelf --section-headers $^ 2>/dev/null | awk '/.text/{printf "0x%s\n", $$5}'); \
-	EXPECT_START_ADDR=$$(printf "0x%08x" $$(( $(ROM_START_ADDR) + $* ))); \
+	EXPECT_START_ADDR=$$(printf "0x%08x" $$(( $(ROM_START_ADDR) + $(VTOR_OFFSET) + $(FCFIELD_OFFSET) + $* ))); \

I find it strange to have here the specific `VTOR_OFFSET` and `FCFIELD_OFFSET`.

Maybe here the issue is to look for `.text` and taking only for the first `PROGBITS` section could work. (output for `tests_cortexm_common_ldscript_offset_0x1000.elf`)

For the `frdm-kw41z`

```
[ 0]                   NULL            00000000 000000 000000 00      0   0  0
[ 1] .vector           PROGBITS        00001000 001000 000400 00   A  0   0  4
```

And for `iotlab-m3`:

```
  [ 0]                   NULL            00000000 000000 000000 00      0   0  0
  [ 1] .text             PROGBITS        08001000 001000 001ef8 00  AX  0   0  4
```

If they are not needed anymore in this file, they do not need to be set in the Makefile.include at all which is somehow better I think as it is really linkerscript internal specific.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/11562#pullrequestreview-240524840
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190522/340ec09c/attachment-0001.html>


More information about the notifications mailing list