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

Increase minimum flash write #949

Closed
wants to merge 16 commits into from

Conversation

kjassmann-renesas
Copy link
Contributor

An admittedly incomplete attempt at #713, but basic functionality is working for us on our flash with 128 byte minimum write size and 32K erase size (no ECC). Based on comments in #713, the following may still need investigation/updates:

  • mcumgr and newt
  • simulator
  • other OS integration (though default settings are backward compatible)
  • ECC considerations
  • other areas, perhaps?

de-nordic and others added 4 commits January 13, 2021 13:00
The commit adds support for IMAGE_F_ROM_FIXED flag that allows setting
information on image base address into image_header.

Signed-off-by: Dominik Ermel <[email protected]>
Signed-off-by: David Brown <[email protected]>
Run "bundle update" and upgrade most ruby gems. This should fix a
warning from GH because of a vulnerable nokogiri version.

GHSA-vr8q-g5c7-m54m

Signed-off-by: Fabio Utzig <[email protected]>
Signed-off-by: David Brown <[email protected]>
Describe changes included in this patch release.

Signed-off-by: David Brown <[email protected]>
Update the various version indicators for 1.7.1.

Signed-off-by: David Brown <[email protected]>
@d3zd3z
Copy link
Member

d3zd3z commented Feb 23, 2021

How large does this make the trailer area with your flash devices? One of the reason we've kept the write size somewhat small is because making it large can greatly increase the size of this area.

@kjassmann-renesas
Copy link
Contributor Author

How large does this make the trailer area with your flash devices? One of the reason we've kept the write size somewhat small is because making it large can greatly increase the size of this area.

We removed the limit on BOOT_MAX_IMG_SECTORS. With the 32K erase size, it's common for our applications to have very few sectors. The trailer size is (5 * BOOT_MAX_ALIGN) + (BOOT_MAX_IMG_SECTORS * min-write-size * 3). For a 128K image, BOOT_MAX_IMG_SECTORS is 4, so the trailer is 17 * 128 = 2176 bytes. Yes, that's a lot of mostly unused space, but it leaves over 98% of the flash for the image, so I thought it might be an acceptable trade off for some applications that want to be able to confirm or revert upgrades.

…before it is passed to flash_area_get_sectors. The flash driver should use count to ensure an overrun does not occur.

Signed-off-by: Kristine Jassmann <[email protected]>
Adding required sign-off.

Signed-off-by: Kristine Jassmann <[email protected]>
@kjassmann-renesas
Copy link
Contributor Author

Using your branch, we can get only 32 aligned trailer if we set DEFAULT_MAX_ALIGN = 32 in the file image.py on line 44

max_align is set in the init to
def __init__(..., max_align=DEFAULT_MAX_ALIGN)

You have added
self.max_align = DEFAULT_MAX_ALIGN if max_align is None else int(max_align)

What is the correct way to configure this setting?

We set max_align using a new --max_align argument to imgtool.py. For example:

> python3 scripts/imgtool.py sign -k root-rsa-2048.pem --header-size 0x80 --slot-size 0x8000 --max-sectors 1 --version 1.0.1 --align 128 --max-align 128 --confirm --pad-header input.bin signed.bin

@kjassmann-renesas
Copy link
Contributor Author

where is the correct place to configure MCUBOOT_BOOT_MAX_ALIGN in the project?

We are setting MCUBOOT_BOOT_MAX_ALIGN in mcuboot_config.h. Should I update mcuboot_config.template.h?

@d3zd3z
Copy link
Member

d3zd3z commented Mar 31, 2021

where is the correct place to configure MCUBOOT_BOOT_MAX_ALIGN in the project?

Ideally, it would be configured appropriately for the target platform. For Zephyr, that would be through a Kconfig option. For Mynewt, I think it would be boot/mynewt/syscfg.yml.

@utzig
Copy link
Member

utzig commented Apr 1, 2021

I don't think d0378c8 is OK. While 32 really is arbitrary there is a minimum amount which is OK; it surely won't work with 1 or 2 sectors (depending on the swap mode). Maybe changing it to 4, or better 8, would be more reasonable.

I am starting to consider if we should not just get rid to this MAX_BOOT_ALIGN concept and use whatever align is provided by the flash driver and from imgtool parameters. newt already gets it from the flash driver in Mynewt, so does mcumgr/newtmgr in both Zephyr and Mynewt. I have no idea if some flash drivers might have a run-time only align size? For Mynewt I am quite sure none does, but for Zephyr I wouldn't know.

Regarding mcumgr, newt, etc I can take care of later. At a minimum we need the changes here, and that they are running on the simulator. I will take a look at the simulator changes in the weekend but I assume it is a pretty straight-forward change.

Also since ECC was mentioned, some time ago someone asked if MCUboot writes to non-erased sectors. I answered "no" at the time, but there was a mention that it indeed does. I think the reason is that they way MCUboot checks that a sector is erased is by reading it, which might not work for ECC flashes. So we have to look into that, but it is not in the scope of this PR, and it is OS specific...

@kjassmann-renesas
Copy link
Contributor Author

I don't think d0378c8 is OK. While 32 really is arbitrary there is a minimum amount which is OK; it surely won't work with 1 or 2 sectors (depending on the swap mode). Maybe changing it to 4, or better 8, would be more reasonable.

Could you please explain? We are testing 1 sector using a confirmed upgrade, and it's working for swap, overwrite, and overwrite fast modes. We haven't tested XIP or RAM loading.

I am starting to consider if we should not just get rid to this MAX_BOOT_ALIGN concept and use whatever align is provided by the flash driver and from imgtool parameters. newt already gets it from the flash driver in Mynewt, so does mcumgr/newtmgr in both Zephyr and Mynewt. I have no idea if some flash drivers might have a run-time only align size? For Mynewt I am quite sure none does, but for Zephyr I wouldn't know.

I am also not aware of any flash drivers with a run-time alignment. I think the build time option may be smaller, and optimal size is very important to us.

Regarding mcumgr, newt, etc I can take care of later. At a minimum we need the changes here, and that they are running on the simulator. I will take a look at the simulator changes in the weekend but I assume it is a pretty straight-forward change.

Thanks! I will be interested to see the simulator update.

Also since ECC was mentioned, some time ago someone asked if MCUboot writes to non-erased sectors. I answered "no" at the time, but there was a mention that it indeed does. I think the reason is that they way MCUboot checks that a sector is erased is by reading it, which might not work for ECC flashes. So we have to look into that, but it is not in the scope of this PR, and it is OS specific...

It definitely writes to non-erased sectors after this change (in swap and overwrite fast mode at least). We are buffering writes at the driver layer and flushing them when we see a write to a new sector. We would prefer to update MCUboot to only write to erased sectors, but buffered writes are acceptable for now. Our flash does not have ECC.

@SwissKnife64
Copy link

Using MCUboot with a STM32H7 we have had do do some fixes to get the boot loader running, but the writing of "confirmed" from the application still fails.

We had to correct the writing of the magic 16 byte pattern. The function for writing the magic pattern has to use a bigger buffer and write the padding and the magic pattern to the flash. The current implementation tries to write only 16 byte and fails silently on STM32H743 due to an error in the flash driver.

The check for alignment in the file drivers/flash/flash_stm32h7x.c has an error on line 103, detected by @SpooniSpoon:
with error if ((offset % FLASH_NB_32BITWORD_IN_FLASHWORD * 4) != 0)
corrected if ((offset % (FLASH_NB_32BITWORD_IN_FLASHWORD * 4)) != 0) with additional braces for correct calculation precedence.
(Sorry, but not yet used to do github pull requests.)

Then there is a conceptual question to solve:
MCUBoot uses { byte padding[n]; byte magic[16] },
and the Zephyr implementation in the application { byte magic[16]; byte padding[n] },
resulting in different position of magic pattern.
At MCUboot the 16 magic pattern is always at the end of the slot, at Zephyr the position of the magic pattern moves with paddings > 16 bytes.

@d3zd3z
Copy link
Member

d3zd3z commented Apr 1, 2021

Also since ECC was mentioned, some time ago someone asked if MCUboot writes to non-erased sectors. I answered "no" at the time, but there was a mention that it indeed does. I think the reason is that they way MCUboot checks that a sector is erased is by reading it, which might not work for ECC flashes. So we have to look into that, but it is not in the scope of this PR, and it is OS specific...

For the LPC55S69, originally, reads from unwritten data would result in a fault. We ended up working around this by using a flash command to read that would return an error status, and in that case, we would put bogus data into the buffer. That flash device has commands to distinguish between corrupt data and erased flash, but it isn't currently being used.

@github-actions
Copy link

github-actions bot commented Jun 1, 2021

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the stale label Jun 1, 2021
@github-actions github-actions bot closed this Jun 16, 2021
@kjassmann-renesas
Copy link
Contributor Author

Please reopen this pull request.

@utzig utzig reopened this Jun 16, 2021
@github-actions github-actions bot removed the stale label Jun 17, 2021
Copy link
Contributor

@mnkp mnkp left a comment

Choose a reason for hiding this comment

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

Grepping for flash_area_write reveals a few more places where a change is required

boot/bootutil/src/bootutil_misc.c:    rc = flash_area_write(fap, off, bs->enckey[slot], BOOT_ENC_KEY_SIZE);
boot/bootutil/src/bootutil_public.c:    rc = flash_area_write(fap, off, boot_img_magic, BOOT_MAGIC_SZ);

That's in the functions writing the magic and encryption keys. They need to be adapted to use boot_write_trailer or similar approach.

required=True)
@click.option('--max-align', type=click.Choice(['1', '2', '4', '8', '16', '32',
Copy link
Contributor

Choose a reason for hiding this comment

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

This extra option seems unnecessary. Makes imgtool less intuitive for the end user. We can calculate the value of max_align. It should be equal to align if the value of align is greater than 8 or otherwise it should be 8. Also, without the option it will be easier to refactor mcuboot code, if the need arises.

@mnkp
Copy link
Contributor

mnkp commented Aug 4, 2021

I am starting to consider if we should not just get rid to this MAX_BOOT_ALIGN concept and use whatever align is provided by the flash driver and from imgtool parameters. newt already gets it from the flash driver in Mynewt, so does mcumgr/newtmgr in both Zephyr and Mynewt. I have no idea if some flash drivers might have a run-time only align size? For Mynewt I am quite sure none does, but for Zephyr I wouldn't know.

I am also not aware of any flash drivers with a run-time alignment. I think the build time option may be smaller, and optimal size is very important to us.

Since MCUboot supports storing images in an external flash what should happen if the flash alignment of the external and internal flash are different? Should BOOT_MAX_ALIGN be always the larger of the two, should it be specific to the flash the image is written to?

@d3zd3z
Copy link
Member

d3zd3z commented Aug 10, 2021

Since MCUboot supports storing images in an external flash what should happen if the flash alignment of the external and internal flash are different? Should BOOT_MAX_ALIGN be always the larger of the two, should it be specific to the flash the image is written to?

I think it works if the max align is set to the larger of the two devices.

@utzig
Copy link
Member

utzig commented Oct 25, 2021

@michaelthomasj @kjassmann-renesas Please fix the sign-off issues.

The sign off issues persist...

@d3zd3z
Copy link
Member

d3zd3z commented Oct 25, 2021

I think you'll need to rebase this, and make sure that what you are pushing here only has the commits you want in it. Some of the ones I see don't have a Signed-off-by. Another, is missing the blank line before the signed-off-by. Also, there should be at least one line of description in the body of the commit (so, a summary line, a blank line, the body, which is one or more lines, a blank line, and the Signed-off-by line).

…/mcuboot into inc-min-flash-write

Signed-off-by: Michael Thomas <[email protected]>
Signed-off-by: Michael Thomas <[email protected]>
Signed-off-by: Michael Thomas <[email protected]>
Signed-off-by: Michael Thomas <[email protected]>
#define BOOT_MAX_ALIGN 8
#define BOOT_MAGIC_ALIGN_SIZE BOOT_MAGIC_SZ
#endif

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change

Double empty lines.

@@ -38,6 +38,10 @@ extern "C" {
#define BOOT_ENC_KEY_SIZE 16
#endif

#define BOOT_ENC_KEY_ALIGN_SIZE \
((((BOOT_ENC_KEY_SIZE - 1) / BOOT_MAX_ALIGN) + 1) * BOOT_MAX_ALIGN)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change

Double empty lines.

@@ -71,7 +71,17 @@ extern "C" {
/** Swapping encountered an unrecoverable error */
#define BOOT_SWAP_TYPE_PANIC 0xff

#define BOOT_MAGIC_SZ 16

Choose a reason for hiding this comment

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

Remove #define BOOT_MAGIC_SZ (sizeof boot_img_magic)? (at line 90)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#define BOOT_MAGIC_SZ 16
#define BOOT_MAGIC_SZ (sizeof boot_img_magic)

Actually the most correct would be to bring line 90 upwards.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes it would be

Copy link
Collaborator

Choose a reason for hiding this comment

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

BOOT_MAGIC_SZ needs to be resolved at preprocessing time due to a new change introduced in this PR and sizeof is only resolved at compile time.
So I had to leave it as 16

@d3zd3z
Copy link
Member

d3zd3z commented Nov 5, 2021

This does need to be rebased. It is pulling in older version of numerous commits, and includes a merge commit.

Also, please look for lines with whitespace at the end. At least the last of the commits introduces a bunch.

Comment on lines +78 to +79
#define BOOT_MAGIC_ALIGN_SIZE \
((((BOOT_MAGIC_SZ - 1) / BOOT_MAX_ALIGN) + 1) * BOOT_MAX_ALIGN)
Copy link
Collaborator

@gustavonihei gustavonihei Nov 8, 2021

Choose a reason for hiding this comment

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

Comment on lines +325 to +330
/* image_trailer structure was modified with additional padding such that
* the pad+magic ends up in a flash minimum write region. The address
* returned by boot_magic_off() is the start of magic which is not the
* start of the flash write boundary and thus writes to the magic will fail.
* To account for this change, write to magic is first padded with 0xFF
* before writing to the trailer. */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/* image_trailer structure was modified with additional padding such that
* the pad+magic ends up in a flash minimum write region. The address
* returned by boot_magic_off() is the start of magic which is not the
* start of the flash write boundary and thus writes to the magic will fail.
* To account for this change, write to magic is first padded with 0xFF
* before writing to the trailer. */
/* image_trailer structure was modified with additional padding such that
* the pad+magic ends up in a flash minimum write region. The address
* returned by boot_magic_off() is the start of magic which is not the
* start of the flash write boundary and thus writes to the magic will fail.
* To account for this change, write to magic is first padded with 0xFF
* before writing to the trailer.
*/
  1. Missing blank line before the beginning of the block comment.
  2. Dangling whitespaces at the end of each line.
  3. For consistency, the terminator for the block comment should be placed in a new line.

int rc;
uint8_t magic[BOOT_MAGIC_ALIGN_SIZE];
uint8_t erased_val;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
uint8_t erased_val;
uint8_t erased_val;

Dangling whitespaces

alexdolzhenkovbdl added a commit to alexdolzhenkovbdl/mcuboot that referenced this pull request Nov 11, 2021
This fix is based on mcuboot fix: mcu-tools#949
It was tested using stm32h733vghx micro on zephyr2.7

The main idea of the fix to use 32 byte align for mcu magic which is supported by stm32h7xx
alexdolzhenkovbdl added a commit to alexdolzhenkovbdl/zephyr that referenced this pull request Nov 11, 2021
This fix is based on mcuboot fix: mcu-tools/mcuboot#949
It was tested using stm32h733vghx micro on zephyr2.7

The main idea of the fix to use 32 byte align for mcu magic which is supported by stm32h7xx
@d3zd3z
Copy link
Member

d3zd3z commented Nov 15, 2021

Be careful, the latest patch adds a lot of extraneous trailing whitespace. This will definitely need to be rebased, with the changes squashed into a cleaner history.

@@ -157,7 +157,7 @@ boot_magic_off(const struct flash_area *fap)
static inline uint32_t
boot_image_ok_off(const struct flash_area *fap)
{
return boot_magic_off(fap) - BOOT_MAX_ALIGN;
return (boot_magic_off(fap) - BOOT_MAX_ALIGN) & ~(BOOT_MAX_ALIGN - 1);
Copy link
Collaborator

@gustavonihei gustavonihei Nov 19, 2021

Choose a reason for hiding this comment

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

Comment on lines +339 to +340
fap->fa_id, (unsigned long)off,
(unsigned long)(fap->fa_off + off));
Copy link
Collaborator

@gustavonihei gustavonihei Nov 19, 2021

Choose a reason for hiding this comment

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

This change should be reverted. Flash Area members must now be accessed through getters.

@gustavonihei
Copy link
Collaborator

@kjassmann-renesas @utzig Do you mind if I take over this PR? It has been open for too long, and this is holding some other PRs from our side.
To avoid losing the PR comment history, it might be interesting to continue the work on the same branch. Then I'll need permission to force push to it in order to properly rebase and squash the commits.

@kjassmann-renesas
Copy link
Contributor Author

@kjassmann-renesas @utzig Do you mind if I take over this PR? It has been open for too long, and this is holding some other PRs from our side. To avoid losing the PR comment history, it might be interesting to continue the work on the same branch. Then I'll need permission to force push to it in order to properly rebase and squash the commits.

@gustavonihei, we have already handed this off to @utzig. No issues on our end if he wants to transfer it to you.

@@ -71,7 +71,17 @@ extern "C" {
/** Swapping encountered an unrecoverable error */
#define BOOT_SWAP_TYPE_PANIC 0xff

#define BOOT_MAGIC_SZ 16
Copy link
Collaborator

Choose a reason for hiding this comment

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

yes it would be

@@ -109,16 +109,24 @@ struct boot_status {
* | Encryption key 0 (16 octets) [*] |
* | |
* +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
* | 0xff padding as needed (BOOT_MAX_ALIGN - 16 EK0 octets) [*] |
Copy link
Collaborator

Choose a reason for hiding this comment

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

EK0?

Copy link
Collaborator

@gustavonihei gustavonihei Nov 22, 2021

Choose a reason for hiding this comment

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

I believe it stands for Encryption Key 0. In #1217 I've proposed a different layout in an attempt to making it easier to understand.

* | Encryption key 1 (16 octets) [*] |
* | |
* +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
* | 0xff padding as needed (BOOT_MAX_ALIGN - 16 EK1 octets) [*] |
Copy link
Collaborator

Choose a reason for hiding this comment

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

EK1?


erased_val = flash_area_erased_val(fap);

memset(&magic[0], erased_val, sizeof(magic));
Copy link
Collaborator

Choose a reason for hiding this comment

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

set size should be (BOOT_MAGIC_ALIGN_SIZE - BOOT_MAGIC_SZ)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Clang static analyzer emits a warning due to the possibility of (BOOT_MAGIC_ALIGN_SIZE - BOOT_MAGIC_SZ) resulting in a call to memset with 0 size.
Since filling the magic buffer with the erased_val pattern is not wrong either, I believe there is no problem leaving it like this. Do you agree?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Both C99 and C11 allow to have 0 length here for size_t type parameter of <string.h> library functions.
But if Clang complains I agree that we can just do what is done here.

@gustavonihei
Copy link
Collaborator

@kjassmann-renesas @utzig Do you mind if I take over this PR? It has been open for too long, and this is holding some other PRs from our side. To avoid losing the PR comment history, it might be interesting to continue the work on the same branch. Then I'll need permission to force push to it in order to properly rebase and squash the commits.

@gustavonihei, we have already handed this off to @utzig. No issues on our end if he wants to transfer it to you.

I have no write permission to renesas/mcuboot. I created another PR at #1217.

rmelch added a commit to rmelch/zephyr that referenced this pull request Dec 14, 2021
This fix is based on mcuboot fix: mcu-tools/mcuboot#949
It was tested using stm32h733vghx micro on zephyr2.7

The main idea of the fix to use 32 byte align for mcu magic which is supported by stm32h7xx

Signed-off-by: Robert Melchers <[email protected]>
@alexdolzhenkovbdl
Copy link

Should this PR be closed since #1217 has fixed this issue and has been merged?

@kjassmann-renesas
Copy link
Contributor Author

Should this PR be closed since #1217 has fixed this issue and has been merged?

@alexdolzhenkovbdl, thanks for bringing this up!

I'll close this since it's merged with #1217

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: core Affects core functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.