b0 bootloader: new optional mcuboot trailer to support upgradable mcuboot #1
Conversation
This feature is always enabled but NOP if magic header not deteted. The new optional image trailer in mcuboot s0 and s1 partitions are located in the last 16 bytes of each partition. b0 validates and selects which mcuboot slot to boot from (supports fallback, if unconfirmed): 1) mcuboot erase flash sector for mcuboot_image_trailer 2) mcuboot writes new image to flash (e.g. "mcumgr image upload") 3) mcuboot mcuboot_image_trailer with mcuboot_image_trailer_init 4) device is rebooted via "mcumgr reboot" or external reset 5) b0 checks if both mcuboot slots have a valid mcuboot_image_trailer.magic, if not normal boot proceeds 6) b0 changes mcuboot_image_trailer.status from TESTING to BOOTING (if any TESTING) 7) b0 gives slot 1st priority if its trailer.status=TESTING/ACTIVE 8) mcuboot changes trailer.status from BOOTING to ACTIVE if confirmed by "mcumgr image confirm" mcuboot also mark passive slot INVALID, to avoid booting in that (except fallback)
|
Nitpick for commit:
It's actually the last 16 bytes of the last sector/page of the partition. As we've discussed, currently this means that the image trailer is actually located 512 bytes (plus the size of the trailer, so 528 bytes) from the partition end address. |
| if (s0_status == MCUBOOT_IMAGE_TRAILER_STATUS_INACTIVE || s1_status == MCUBOOT_IMAGE_TRAILER_STATUS_TESTING) { | ||
| if (s1_status != MCUBOOT_IMAGE_TRAILER_STATUS_INACTIVE) |
There was a problem hiding this comment.
| if (s0_status == MCUBOOT_IMAGE_TRAILER_STATUS_INACTIVE || s1_status == MCUBOOT_IMAGE_TRAILER_STATUS_TESTING) { | |
| if (s1_status != MCUBOOT_IMAGE_TRAILER_STATUS_INACTIVE) | |
| if (s1_status == MCUBOOT_IMAGE_TRAILER_STATUS_ACTIVE || s1_status == MCUBOOT_IMAGE_TRAILER_STATUS_TESTING) { |
I think you could end up looping (if we didn't also change BOOTING -> INACTIVE a test partition. Either way I believe the logic is equivalent to what we need and maybe easier to read.
Only prioritize S1 if ACTIVE or TESTING
Let me know, if you disagree. I don't doubt that the current logic works out!
There was a problem hiding this comment.
I puzzled with this part but I think S1 could be active while S0=TESTING ?
But I agree if we can simplify it it would be easier to read ...
There was a problem hiding this comment.
Ah, that's true. That case needs to be handled as well!
There was a problem hiding this comment.
Will try test this:
switch (s1_status) {
case MCUBOOT_IMAGE_TRAILER_STATUS_TESTING:
case MCUBOOT_IMAGE_TRAILER_STATUS_BOOTING:
case MCUBOOT_IMAGE_TRAILER_STATUS_ACTIVE:
switch (s0_status) {
case MCUBOOT_IMAGE_TRAILER_STATUS_TESTING:
case MCUBOOT_IMAGE_TRAILER_STATUS_BOOTING:
break; /* s0 has priority */
default:
result = SLOT_PRIORITY_S1;
break;
}
There was a problem hiding this comment.
Should the BOOTING state also be included here? Won't that mean, that we essentially get two attempts when switching bank? (Although that could be practical in some cases to e.g. test that also the FEP application starts OK with the new mcuboot)
There was a problem hiding this comment.
we will keep this - at least for now
- fixed potential nullptr dereference - printk compile warnings - added MCUBOOT_IMAGE_TRAILER_STATUS_UNKNOWN for logging - improved logic in slot_priority_from_image_trailers - logs slot states during boot in check_and_fix_image_trailer - comments updated from review
| break; /* SLOT_PRIORITY_S0 */ | ||
| default: | ||
| result = SLOT_PRIORITY_S1; | ||
| } |
There was a problem hiding this comment.
Does the compiler not cry about missing default case here?
There was a problem hiding this comment.
no - I did not see any warnings related to that, I can add a break but it must be minor
There was a problem hiding this comment.
Just usually switch statements require all cases to be covered (or have a default) so I figured if we ever change compiler flags to have more warnings or something, this is a low hanging fruit to fix preemptively.
But in practice it's minor, yeah
There was a problem hiding this comment.
we will keep this - at least for now
| * 6) b0 updates mcuboot_image_trailer.status appropriately: | ||
| * From TESTING to BOOTING (if any TESTING) | ||
| * From BOOTING to INACTIVE (if any BOOTING) | ||
| * 7) b0 gives a slot 1st priority if its trailer.status=TESTING | ||
| otherwise it prioritizes trailer.status=ACTIVE | ||
| * 8) mcuboot changes trailer.status of booted image to ACTIVE if confirmed by "mcumgr image confirm" | ||
| * and simultaneously marks the not-booted image as INACTIVE | ||
| * NOTE: if any image fails to validate, the other bank will still be attempted as well. | ||
| * Hence b0 can "fallback" to an image otherwise marked as INACTIVE. |
There was a problem hiding this comment.
I think github and indentation was not my friends today ;) Maybe you wanna fix indentation and then we're good to go basically?
There was a problem hiding this comment.
I hand picked all your suggestions this time. I tried to make it look good but will look again now
There was a problem hiding this comment.
we will keep this - at least for now
b0 bootloader: new optional image trailer to support upgradable mcuboot
This feature is always enabled but NOP if magic header not detected.
The new optional image trailer in mcuboot s0 and s1 partitions are located in the
last 16 bytes of each partition.
This feature also require a similar extension of mcuboot.
b0 validates and selects which mcuboot slot to boot from (supports fallback, if unconfirmed):
From TESTING to BOOTING (if any TESTING)
From BOOTING to INACTIVE (if any BOOTING)
otherwise it prioritizes trailer.status=ACTIVE
and simultaneously marks the not-booted image as INACTIVE
NOTE: if any image fails to validate, the other bank will still be attempted as well.
Hence b0 can "fallback" to an image otherwise marked as INACTIVE.