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

bootloader: add riotboot minimal bootloader #10065

Merged
merged 8 commits into from
Dec 18, 2018

Conversation

kYc0o
Copy link
Contributor

@kYc0o kYc0o commented Sep 27, 2018

Contribution description

RIOT image header and "riotboot" bootloader

Overview

riotboot is the name of a minimal bootloader application and infrastructure.
It consists of

  • the application "riotboot" in the bootloaders folder which serves as minimal bootloader
  • the module "riot_hdr" used to recognise RIOT firmware images which riotboot can boot.
  • the module "slot_util" used to manage the images (slots) with a RIOT header attached to them
  • a tool in dist/tools/riot_hdr for header generation.
  • several make targets to glue everything together
Concept

riotboot expects the flash to be separated in slots: at CPU_FLASH_BASE address resides the bootloader, which is followed by a slot 0 with a firmware image.

A RIOT image with a single slot looks like:

|------------------------------- FLASH -------------------------------------|
|----- RIOTBOOT_LEN ----|----------- RIOTBOOT_SLOT_SIZE (slot 0) -----------|
                        |--- RIOTBOOT_HDR_LEN ----|
 ---------------------------------------------------------------------------
|       bootloader      | riot_hdr_t + filler (0) |     RIOT firmware       |
 ---------------------------------------------------------------------------

Please note that RIOTBOOT_HDR_LEN depends on the architecture of the MCU, since it needs to be aligned to 256B. This is fixed regardless of sizeof(riot_hdr_t).

The bootloader will, on reset, verify the checksum of the first slot, then boot it. If the slot doesn't have a valid checksum, no image will be booted and the bootloader will enter while(1);.

The header contains

/**
 * @brief Structure to store image header - All members are little endian
 * @{
 */
typedef struct {
    uint32_t magic_number;         /**< header magic_number (always "RIOT")    */
    uint32_t version;              /**< Integer representing firmware version  */
    uint32_t start_addr;           /**< Start address in flash                 */
    uint32_t chksum;               /**< checksum of riot_hdr                   */
} riot_hdr_t;
/** @} */

The bootloader "riotboot" only cares about checksum.

Testing procedure

There's a test provided in tests/riotboot. This has only tested for now in:

  • samr21-xpro
  • iotlab-m3 (desk)
  • nucleo-l152re

Please add above the boards that you have tested and worked.

Issues/PRs references

#9342
#9969

@kYc0o kYc0o added Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: OTA Area: Over-the-air updates labels Sep 27, 2018
@kYc0o kYc0o added this to the Release 2018.10 milestone Sep 27, 2018
@kYc0o kYc0o force-pushed the pr/riotboot branch 4 times, most recently from a6721e5 to 0d2a562 Compare September 27, 2018 18:50
@kaspar030
Copy link
Contributor

Why did you cut out the slot selection logic?

@bergzand
Copy link
Member

Why did you cut out the slot selection logic?

From the description it looks like this is a minimal version that only supports a single firmware slot.

@kaspar030
Copy link
Contributor

From the description it looks like this is a minimal version that only supports a single firmware slot.

Yup, but my original commit contained like, ~15 lines more, but did support multiple slots.

IMO, going through all the bootloader hoops to verify a firmware's metadata hash seems a bit pointless.

@kYc0o
Copy link
Contributor Author

kYc0o commented Sep 28, 2018

Yup, but my original commit contained like, ~15 lines more, but did support multiple slots.

IMO, going through all the bootloader hoops to verify a firmware's metadata hash seems a bit pointless.

I wanted to provide the minimal as possible and then build on top of it the extensions, like support for two firmware slots and flashing. I think I can re-add this logic since it doesn't hurt, but it will look like we're looking for inexistent slots (there are no support for second slot here).

Anyways, there are some current problems to be solved first, after solve them I can get back into this question.

@emmanuelsearch
Copy link
Member

@kYc0o any eta on when you will push the changes?

@kYc0o
Copy link
Contributor Author

kYc0o commented Oct 1, 2018

Anyways, there are some current problems to be solved first, after solve them I can get back into this question.

Marking WIP because of this.

@kYc0o any eta on when you will push the changes?

Should be done by tomorrow at latest.

@kYc0o kYc0o added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR labels Oct 1, 2018
@waehlisch waehlisch mentioned this pull request Oct 2, 2018
@kYc0o kYc0o force-pushed the pr/riotboot branch 2 times, most recently from 584dfa6 to fcec3c6 Compare October 2, 2018 16:37
@kYc0o kYc0o removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Oct 2, 2018
@kYc0o
Copy link
Contributor Author

kYc0o commented Oct 2, 2018

Not WIP anymore.

I changed several conceptual things on this PR and remove some awful dependencies which were not needed.

Now, slot 0 represents the first firmware image, while the bootloader represents no slot. This change was to be aligned on other bootloaders (e.g. mcuboot) slot concepts, and actually having slot 0 as the bootloader wasn't really useful.

RIOT_HDR_SIZE is now removed and it's not anymore a dependency for the header generator, it's now rather passed as an argument.

The compilation of genhdr is now automatic, but only depends on the existence of the binary, so any modification to the code of riot_hdr is not tracked. @cladmi do you have an idea to do it without importing all the files from riot_hdr?

Finally, I don't know how to get rid of the cppcheck error, in my system I don't have it, most likely because I have a more recent cppcheck version.

@kYc0o kYc0o force-pushed the pr/riotboot branch 2 times, most recently from 5b924f3 to dbb74e3 Compare October 4, 2018 15:38
@kYc0o
Copy link
Contributor Author

kYc0o commented Oct 4, 2018

Pushed some changes to get rid of the cppcheck error. Although, it was a false negative IMHO so we should update Murdock to avoid those.

@danpetry danpetry added Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines labels Dec 12, 2018
@jcarrano
Copy link
Contributor

@kYc0o

Can you find at least one point of failure?
Not of failure, but those mechanisms in make that depend on order tend to be be fragile. In any case, I wrote that as a comment and not as a change request because I realize it's not an issue of this PR, and there's not much you can do about it.

@emmanuelsearch
Copy link
Member

@cladmi must the docker aspect you mention be part of this PR (or can it be a subsequent PR)?

@emmanuelsearch
Copy link
Member

@jcarrano what is blocking (if anything) in your comments, from your point of view?

@jcarrano jcarrano dismissed their stale review December 13, 2018 14:23

It's easier to fix it myself in a PR later.

@emmanuelsearch
Copy link
Member

ping @cladmi

@cladmi
Copy link
Contributor

cladmi commented Dec 17, 2018

@emmanuelsearch issue is also present for mcuboot integration so not a new issue.
I will try to fix it for both for the release.

@cladmi cladmi dismissed their stale review December 17, 2018 19:12

Not much time to review the details, but test works and also helped with some of the implementation.

@danpetry
Copy link
Contributor

Please squash and rebase. @kaspar030 do you need any more changes before merging?

@kaspar030
Copy link
Contributor

IMO this is good to go. Please squash!

kYc0o and others added 8 commits December 18, 2018 19:31
This module adds some helper function to manage slots
created with a riotboot_hdr header.

Co-authored-by: Kaspar Schleiser <kaspar@schleiser.de>
In order to use the RIOT bootloader (riotboot) a header needs to
be created and placed before the firmware. This tool generates
such a header with the expected information by the bootloader.

Co-authored-by: Kaspar Schleiser <kaspar@schleiser.de>
riotboot is introduced here and makes use of riotboot_hdr,
which indentifies the images encapsulated as slots.
The slot size and offset is configurable, which makes
slots extendable if needed, e.g. 2 or more slots can be
transparently added.

Co-authored-by: Kaspar Schleiser <kaspar@schleiser.de>
Co-authored-by: Gaëtan Harter <gaetan.harter@fu-berlin.de>
This new function allows to jump to another execution
environment (VTOR) located at a certain (aligned) address.
It's used to boot firmwares at another location than
`CPU_FLASH_BASE`.

The user needs to ensure that the CPU using this feature
is able to be initialised at least twice while jumping
to the RIOT `reset_handler_default` function, since it
initialises the CPU again (calls cpu_init()).

Co-authored-by: Kaspar Schleiser <kaspar@schleiser.de>
RIOTBOOT_SLOT_LEN is calculated as an hexadecimal value and
handles ROM_LEN defined as kilobytes like '512K'

This enables support for all the cortex-m0+/3/4/7 arch,
so most boards embedding these are potentially supported.
One needs just to ensure that the CPU can be initialised
at least twice.

Co-authored-by: Gaëtan Harter <gaetan.harter@fu-berlin.de>
Currently only tested boards provide the feature riotboot.
Potentially all boards embeding a cortex-m0+/3/4/7 are
able to have riotboot as a feature, but other dependencies
need to be met, e.g. usage of cortexm.ld linker script,
double initialisation of cpu_init(), etc. See doc in
bootloaders/riotboot.
The tests overrides the target all to be tested by the CI.
All the instructions how to use it are in README.md
The test is successful if the image boots and displays
information about the image and running slot.

Co-authored-by: Federico Pellegrin <fede@evolware.org>
Co-authored-by: Federico Pellegrin <fede@evolware.org>
@kaspar030
Copy link
Contributor

I squashed as @kYc0o is currently offline. As I co-authored most of the code, we'll need more than my ACK. Anyhow, I think we should get this in ASAP. We can always iterate.

@emmanuelsearch
Copy link
Member

@kaspar030 we have another ACK from @danpetry so we're good if Murdock is green.

@emmanuelsearch emmanuelsearch merged commit 3a3c23d into RIOT-OS:master Dec 18, 2018
@miri64
Copy link
Member

miri64 commented Dec 18, 2018

🎉

@kYc0o
Copy link
Contributor Author

kYc0o commented Dec 19, 2018

😃 a big thanks to all the contributors and reviewers for making this PR in!

@waehlisch
Copy link
Member

@jcarrano

It's easier to fix it myself in a PR later.

We don't want forget this: Can you create issues or the PR timely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: OTA Area: Over-the-air updates CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.