Implement swap without using scratch #589
Conversation
utzig
force-pushed
the
swap-move
branch
6 times, most recently
from
4267878
to
e60214a
Compare
|
Ah, also currently this allows for both |
|
Ok, now it's only missing design doc update. I will wait for review issues before I write it down. |
|
Btw, encrypted image swap is currently broken as was found out after merging e84f0ef! |
There was a problem hiding this comment.
The code largely looks good, other than the one test occasionally causing segfaults.
Maybe we should at least document that the code assumes the header is smaller than the chunk size (which is 1024), and that if someone had a larger vector table, this would have to be increased, or the code changed.
utzig
force-pushed
the
swap-move
branch
2 times, most recently
from
fbc4a97
to
5fe73ad
Compare
|
cc @hakonfam |
| * a permanent upgrade in case a reset occurs. | ||
| */ | ||
| void | ||
| fixup_revert(const struct boot_loader_state *state, struct boot_status *bs, |
There was a problem hiding this comment.
Can you add more explanation for this? E.g. what is the "right" time?
There was a problem hiding this comment.
It might be a bad wording; maybe it should be called "wrong" instead of "right". The idea is that there is a short period of time during the beginning of a revert where metadata might be lost because status in slot0 must be erased and status in slot1 was already previously erased. If a reset occurs at this time the swap breaks. The fixup... routine just saves temporarily the data in slot1 (and it will be erased again moments later), but it is saved long enough to walk a few extra steps without loosing information.
There was a problem hiding this comment.
Thanks, I got what you meant by "right" ;). Your comment gives good context which I think should be added to the function doc comment.
There was a problem hiding this comment.
I did not copy the previous answer, but hopefully I improved the text with: 29ec1ab
boot/bootutil/src/bootutil_priv.h
Outdated
| int boot_write_copy_done(const struct flash_area *fap); | ||
| int boot_write_image_ok(const struct flash_area *fap); | ||
| int boot_write_swap_info(const struct flash_area *fap, uint8_t swap_type, | ||
| uint8_t image_num); | ||
| int boot_write_swap_size(const struct flash_area *fap, uint32_t swap_size); | ||
| int boot_read_swap_size(int image_index, uint32_t *swap_size); | ||
| int boot_erase_trailer_sectors(const struct boot_loader_state *state, |
There was a problem hiding this comment.
Personally, I'd like the new swap implementations to have their own (shared) header file. To make it even more decoupled.
There was a problem hiding this comment.
Done with 5887060. I did not go too far, simply moved new stuff from the PR, but it looks like it would be possible to move more non overwrite-only stuff in there... would rather do later...
|
@oyvindronningstad I doubt I would be the best person to document this, all function names look self-explanatory to me! 😂 |
There was a problem hiding this comment.
Some guiding questions for writing documentation. They are questions I get from just looking at the header file with no real knowledge of the implementation, hopefully they will make it easier if you want to write docs :).
Also, consider changing the names. E.g. changing boot_ to swap_ would IMO be more descriptive.
|
|
||
| #if defined(MCUBOOT_SWAP_USING_SCRATCH) || defined(MCUBOOT_SWAP_USING_MOVE) | ||
|
|
||
| int boot_erase_trailer_sectors(const struct boot_loader_state *state, |
There was a problem hiding this comment.
Why does this need a separate function?
|
|
||
| int boot_erase_trailer_sectors(const struct boot_loader_state *state, | ||
| const struct flash_area *fap); | ||
| int boot_copy_region(struct boot_loader_state *state, |
There was a problem hiding this comment.
Is this for copying between different slots or within the same slot?
Does it erase?
Does it have alignment restrictions?
| const struct flash_area *fap_src, | ||
| const struct flash_area *fap_dst, | ||
| uint32_t off_src, uint32_t off_dst, uint32_t sz); | ||
| int boot_erase_region(const struct flash_area *fap, uint32_t off, uint32_t sz); |
There was a problem hiding this comment.
Does it have alignment restrictions?
| const struct flash_area *fap_dst, | ||
| uint32_t off_src, uint32_t off_dst, uint32_t sz); | ||
| int boot_erase_region(const struct flash_area *fap, uint32_t off, uint32_t sz); | ||
| int boot_status_init(const struct boot_loader_state *state, |
There was a problem hiding this comment.
Does this initialize the structure in flash, or does it read from flash and initialize a RAM copy, or neither?
Which are input and output parameters?
| int boot_status_init(const struct boot_loader_state *state, | ||
| const struct flash_area *fap, | ||
| const struct boot_status *bs); | ||
| void boot_perform_swap(struct boot_loader_state *state, |
There was a problem hiding this comment.
What are input and output parameters?
When is this meant to be called? Many times or once only?
Does it resume where it left off if the device resets?
| void boot_perform_swap(struct boot_loader_state *state, | ||
| struct boot_status *bs, | ||
| uint32_t copy_size); | ||
| int boot_status_source(struct boot_loader_state *state); |
There was a problem hiding this comment.
What is a 'source' in this context? What are input and output parameters?
| struct boot_status *bs, | ||
| uint32_t copy_size); | ||
| int boot_status_source(struct boot_loader_state *state); | ||
| int boot_read_status_bytes(const struct flash_area *fap, |
There was a problem hiding this comment.
What are 'status bytes' and where are they read from? Which parameter (or return value?) contains the status bytes after I've called this?
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
There was a problem hiding this comment.
Top level docs: e.g. Describe the end result of a swap, and maybe a sentence about each method (SCRATCH and MOVE)
|
@oyvindronningstad I applied a few changes loosely based on your suggestions: renamed functions that are only required by swap upgrade functionality to |
|
@oyvindronningstad Oops, forgot to point the commit, here it is: ba05ddc |
There was a problem hiding this comment.
@utzig Thanks a lot!
One final request: Add top level docs that just point to the relevant design docs.
Signed-off-by: Fabio Utzig <utzig@apache.org>
This moves the functionality that is unique to a scratch based swap upgrade into a separate file. Later other upgrade strategies can be added by reimplementing those functions. Signed-off-by: Fabio Utzig <utzig@apache.org>
|
@utzig, nice that you are copying my idea for upgrade without scratch area from ZEPboot. The copy is not so good, you should first move up the image in the primary partition, this distributes the flash erases evenly. And it would be nice to mention the idea contributor. |
This implements a swap upgrade that does not use a scratch area. It works by first moving all sectors in the primary slot up one position, and then looping on moving sector of index X of the secondary slot to index X of the primary slot, followed by moving sector X+1 of the primary slot to X on the secondary slot, for each sector X. The idea behind this implementation was initially suggested by Jehudi Maes (@Laczen) and implemented on his own bootloader (ZEPboot). Signed-off-by: Fabio Utzig <utzig@apache.org>
Signed-off-by: Fabio Utzig <utzig@apache.org>
Add cap for swap using move and rename old swap upgrade cap to swap using scratch. Update sim to allow swapping tests to also run using move. Signed-off-by: Fabio Utzig <utzig@apache.org>
Update `make_device` to return a slice of unsupported caps for a test. This allows skipping tests in devices that are known to be non working under some build configuration. The device constructor was updated to return a `Result`, so that the specific reason for skipping can be returned as a `String`. Signed-off-by: Fabio Utzig <utzig@apache.org>
Add Mynewt option to enable building a bootloader that uses an alternative swap algorithm, that first moves up all sectors in slot1 and then directly swaps between slot0 and slot1. Signed-off-by: Fabio Utzig <utzig@apache.org>
Signed-off-by: Fabio Utzig <utzig@apache.org>
Add Zephyr option to enable building a bootloader that uses an alternative swap algorithm, that first moves up all sectors in slot1 and then directly swaps between slot0 and slot1. Signed-off-by: Fabio Utzig <utzig@apache.org>
|
@Laczen Thanks, I've swapped the indexes and now it moves up the primary. |
|
@nvlsianpu @d3zd3z I added "[EXPERIMENTAL]" to the Zephyr Kconfig (9d94fd3#diff-56e9ade09681f44d80863bfce76a0f2bR130) and would like to leave it there at least until this looks as reliable as the scratch method. Is this OK? |
This implements a swap upgrade that does not use a scratch area. It works by first moving all sectors in the secondary slot up one position, and then looping on moving sector of index X of the primary slot to index X of the secondary slot, followed by moving sector X+1 of the secondary slot to X on the primary slot, for each sector X.