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

bootutil: Fix compatible sector check #1883

Merged
merged 3 commits into from
Jan 8, 2024

Conversation

nordicjm
Copy link
Collaborator

@nordicjm nordicjm commented Jan 3, 2024

bootutil: Add debug logging for boot status write

Adds debug level logging which shows the offset of where a
sector swap status write is occurring at


bootutil: Fix compatible sector checking

Fixes an issue whereby slot sizes were checked but the check was
not done properly. This also adds debug log messages to show the
sector configuration including if slot sizes are not optimal


docs: release: Add release notes on changes

Adds release notes on bootutil changes

Adds debug level logging which shows the offset of where a
sector swap status write is occurring at

Signed-off-by: Jamie McCrae <[email protected]>
@nordicjm nordicjm added bug enhancement area: core Affects core functionality labels Jan 3, 2024
@nordicjm nordicjm requested a review from d3zd3z January 3, 2024 07:44
@nordicjm nordicjm force-pushed the fixswapmovesectorcheck branch 2 times, most recently from a55d426 to 4e45668 Compare January 3, 2024 08:51
if ((num_sectors_pri != num_sectors_sec) &&
(num_sectors_pri != (num_sectors_sec + 1))) {
(num_sectors_pri != (num_sectors_sec + 1)) &&
(num_usable_sectors_pri != num_sectors_sec) &&

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is true that we are trying to have the code check compatibility here, I think there's a "miss" of a case. The first two logic entries are basically the 'old' way - pri/sec size are equal or pri is one sector larger than sec. The new logic added allows for the idea that we're now calculating the app sectors by subtracting out the size of the trailer.
I think the trouble here is that if trailer info is really stored outside the app image itself, then the first two original logic entries aren't really valid. Just adding a single sector to the flash map for the primary would pass this test but would not give any allowance of space for the trailer info even if it only took a single sector additional on top of the move sector.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the trouble here is that if trailer info is really stored outside the app image itself, then the first two original logic entries aren't really valid

That is true but that is how most boards are setup in zephyr today and existing projects probably, so would break basically everything. If a user enables debug logging then they will see that their configuration is wrong, but the main thing is to maintain backwards compatibility

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I think maybe I missed your point, do you mean having this:

    if ((num_sectors_pri != num_sectors_sec) &&
            (num_sectors_pri != (num_sectors_sec + 1)) &&
            (num_usable_sectors_pri != (num_sectors_sec + 1))) {

instead of this:

    if ((num_sectors_pri != num_sectors_sec) &&
            (num_sectors_pri != (num_sectors_sec + 1)) &&
            (num_usable_sectors_pri != num_sectors_sec) &&
            (num_usable_sectors_pri != (num_sectors_sec + 1))) {

?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok - so I think we're in agreement on two points?

  • Trailer info get stored /inside/ the flash area where the application lives (inside the confines of what slot1 size would dictate). Hmm - or is is more accurate to say that trailer info is stored at the end of the flash area minus the move sector. If it's really 'at the end minus the move sector', then I guess I can better understand your idea that adding MORE than a single sector to the primary slot0 is how to accomodate larger trailer sizes without impinging on the app space itself.
  • The flash map only gets a single extra sector as this is how it's been done for a long time and there's no desire to break configs out in the wild. Yeah?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slot0 encompasses everything: image, swap status, mcuboot flags. The swap status and mcuboot flags end in the final sector of the slot and will go backwards from there (if your swap status takes 2.5 sectors then the combined swap status and mcuboot flags would start at (n-3) and continue until the end of flash. In this code, num_sectors_pri and num_sectors_sec are the number of sectors in the whole flash area, so if you had the above example and had 129 sectors assigned to slot0 and 128 assigned to slot1, 3 sectors of slot0 are needed for the swap status and 1 for the move sector, so your image can occupy (129 - 3 - 1) = 125 sectors, thus slot1 has 3 extra sectors that can never be used, so this addresses that specific issue by allowing the slot sizes to differ by the number of sectors that the swap status takes up plus one instead of just differing by plus one as it allowed previously.

Yes, unless there is a good reason to break things, keeping them work even if not optimal is desired.

Fixes an issue whereby slot sizes were checked but the check was
not done properly. This also adds debug log messages to show the
sector configuration including if slot sizes are not optimal

Signed-off-by: Jamie McCrae <[email protected]>
Adds release notes on bootutil changes

Signed-off-by: Jamie McCrae <[email protected]>
@nordicjm nordicjm force-pushed the fixswapmovesectorcheck branch from 4e45668 to 67a1f40 Compare January 8, 2024 11:47
Comment on lines +246 to +254
first_trailer_idx = boot_img_num_sectors(state, BOOT_PRIMARY_SLOT) - 1;

while (1) {
sz += sector_sz;
if (sz >= trailer_sz) {
break;
}
first_trailer_idx--;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is size or division optimization for

first_trailer_idx = boot_img_num_sectors(state, BOOT_PRIMARY_SLOT) -  (trailer_sz + sector_sz - 1) / sector_sz;

?
I know that this was here previously, just wondering why.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was added 5 years ago like that apparently, could be changed to the summarised form if wanted

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah. Lets fix it first, than we can go through these things and do some refactoring.

@nordicjm nordicjm merged commit b994ba2 into mcu-tools:main Jan 8, 2024
55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: core Affects core functionality bug enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants