Design doc fixups #104
Merged
Design doc fixups #104
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 9 commits
August 4, 2017 14:45
Adjust alignment and add missing leading zero to IMAGE_F_PKCS1_PSS_RSA2048_SHA256. This needed some comment changes to keep things 80 column clean. Signed-off-by: Marti Bolivar <marti.bolivar@linaro.org>
The comment for BOOT_SWAP_TYPE_NONE says "Just boot whatever is in slot 0". That's not correct: if configured to do so (and this the strongly recommended configuration), mcuboot will first cryptographically validate the contents of slot 0 before booting it. Fix the comment to be more accurate. Signed-off-by: Marti Bolivar <marti.bolivar@linaro.org>
Signed-off-by: Marti Bolivar <marti.bolivar@linaro.org>
Signed-off-by: Marti Bolivar <marti.bolivar@linaro.org>
Copy up to date image flags from image.h. Fixes for: - Wrong comment for IMAGE_F_ECDSA224_SHA256 - Missing definition for IMAGE_F_PKCS1_PSS_RSA2048_SHA256 Signed-off-by: Marti Bolivar <marti.bolivar@linaro.org>
Distinguish between flash areas and flash area IDs. Say what the bootloader area is, since that's not discussed anywhere else. Signed-off-by: Marti Bolivar <marti.bolivar@linaro.org>
The discussion about image slots assumes that the bootloader swaps, but that is not all it can do. Clear this up. Signed-off-by: Marti Bolivar <marti.bolivar@linaro.org>
The design document says that image trailers can be located in scratch. That's not true: the contents in scratch are just a subset of the complete image trailer. Fix this up by making it clear that image trailers are at the end of image areas, and sometimes some of their data is copied into and out of scratch. Signed-off-by: Marti Bolivar <marti.bolivar@linaro.org>
The swap status field definition claims image data are swapped one sector at a time, but that's not true: image data are actually swapped around in increments equal to the size of the scratch area, which can be multiple sectors in length. Its contents are also misleading in claiming that it's a series of single byte records, which ignores padding when min-write-size > 1. It also assumes the reader knows how images are swapped, but that's not explained until later on in the document. Finally, the paragraph is in a list of "definitions" of the image trailer's fields, but it doesn't actually define the contents; it's just a high level description of what the field is for. Fix all of these issues by re-working this paragraph. Signed-off-by: Marti Bolivar <marti.bolivar@linaro.org>
The current "boot states" description doesn't make sense and shouldn't be used. For one thing, with three possible pending states, two possible confirmed states, and two image slots each with a combined (pending, confirmed) state, the total number of boot states is 36, but the document says there are "four possible states". For another, the actual bits on flash map to the "boot states" in a way that is carefully designed to ensure that only those 4 are the "outcome" of a boot. The fact that this map does not cover the entire space of what is being presented as the "logical" states of the device is a strong indication that the pending/confirmed state space is a bad choice, not connected with the actual operation of the bootloader. A state space that is better for describing how the bootloader behaves is given the by the enumeration of "swap types" which appear under each of the tables in the "IMAGE TRAILERS" section, as well as the bootloader code itself. To help fix the description of the bootloader' operation, rewrite the "boot states" portions of the design document, deleting the pending/confirmed "states" and replacing them with swap types. There is still more work to do here: - There is still an "important caveat" to describing things in terms of swap types, which means it's not quite right. - It's strange to say that "none" is a swap type. - This doesn't provide a clean explanation for how mcuboot handles an interrupted swap. But it's another step. Signed-off-by: Marti Bolivar <marti.bolivar@linaro.org>
mbolivar
force-pushed
the
design-doc-fixups
branch
from
162c596
to
048d8d8
Compare
|
I fixed some issues with the commit log in the last patch in v2. No changes to the patches themselves. |
d3zd3z
reviewed
Aug 5, 2017
| @@ -222,21 +218,31 @@ An image trailer has the following structure: | |||
| | MAGIC (16 octets) | | |||
| +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | |||
|
|
|||
| These records are at the end of each image slot. The offset immediately | |||
| following such a record represents the start of the next flash area. | |||
| The offset immediately following such a record represents the start of the next | |||
There was a problem hiding this comment.
Hmm. This sentence is still somewhat awkward, and I think you've removed the part that is easier to understand. The current wording sounds like a good way to describe it for a standard document, but not really a design document. How about something like:
"The image trailer is placed such that it butts up against the end of the partition. For example, if the following slot begins at offset 'n', the MAGIC field will be at offset 'n-16'."
There was a problem hiding this comment.
Good idea. The next thing might not be a 'slot', so I'll use 'flash area'.
| trailers" what is meant is the aggregate information provided by both image | ||
| slot's trailers. | ||
| At startup, the boot loader determines the boot swap type by inspecting the | ||
| image trailers. When using the term "image trailers" what is meant is the |
There was a problem hiding this comment.
I think this sentence really just indicates that the following description was awkward. If we take it out, do things still make sense? If not, I think we should fix the descriptions below, rather than try to define what plural means.
There was a problem hiding this comment.
I've tried to improve this paragraph.
|
v3 posted accounting for review comments from @d3zd3z. Diff from v2: |
utzig
approved these changes
Aug 8, 2017
| confirmed | X | | | ||
| -----------------+--------+--------' | ||
| swap: test | | ||
| result: BOOT_SWAP_TYPE_TEST | |
There was a problem hiding this comment.
I changed the same thing on my last doc updates, but decided to rollback before finishing the commit! :-)
|
@utzig are you saying I should squash all of the design doc changes into one commit? I made the commits separate logical changes on purpose so they'd be easier to review. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This continues the work in MCUB-67. The substantial change is deleting the 'pending' / 'confirmed' state space in favor of one defined in terms of 'swap types'.