bootutil: Add support for devices without erase #2114

de-nordic · 2024-11-11T15:38:13Z

Commits:

bootutil: add support for not calling erase when device does not require it and MCUBOOT_DEV_WITHOUT_ERASE_THEN_NO_ERASE option that controls this; add MCUBOOT_MINIMAL_SCRAMBLE that allows to reduce amount of removed data, where applicable;
Zephyr: Kconfig options and supporting code
Zephyr: switch to flash_area_flatten in bs_custom_storage_erase
~~- Zephyr: switch to boot_set_next, from flash_area_erase, for Zephyr builds of bootutil_public.~~

The first MCUboot option allows to improve life time of devices for which erase is emulated by write, and is not really required by the device design and the second MCUboot option allows to just remove enough of data, where needed, by either reduced range of erase or reduced write; for example by scrambling only image header, rather than removing the entire image.

Tested on nrf52840dk and nrf54l15. In case of nrf54l it reduces swap time by half, in comparison to code using erase.

Note: Even though, finally, swap_erase_trailer_sectors is no longer used, I have left it there because it may be used in the future.
I have left a Note around swap_status_init function, that partially corresponds to this; it seems that in many places call to swap_scramble_trailer_sectors, whee followed by swap_status_init, should in reality be swap_erase_trailer_sectors fulfilling hardware requirement for preparing device for write; the problem is that swap_status_init initializes only part of swap information, which means that if the region of entire swap info is not scrambled first, leftovers may lead to improper behaviour, for example test will not revert.
There is re-design of swap_status_init required to mitigate this, as in some paths the whole "init" may just mean removal of everything except information that swap_status_init would put there anyway, or combined init operation that would first remove no longer needed information than overwrite what should be re-initialized - but this requires further analysis.

So far this PR works fine on devices that do not require erase as well as these that require erase before write.

nordicjm · 2024-11-18T08:13:48Z

boot/bootutil/src/bootutil_priv.h

 int boot_erase_region(const struct flash_area *fap, uint32_t off, uint32_t sz);
+/* SImilar to boot_erase_region but will alwasy remove data */


-I+i, *always

nordicjm · 2024-11-18T08:16:00Z

boot/bootutil/src/loader.c

+        uint8_t buf[BOOT_MAX_ALIGN];
+        size_t size_done = 0;
+
+        memset(buf, flash_area_erased_val(fa), sizeof(buf));
+
+        while (size_done < size) {
+            ret = flash_area_write(fa, size_done + off, buf, sizeof(buf));
+            if (ret != 0) {
+                break;
+            }
+            size_done += sizeof(buf);
+        }


this just assumes max align will not overflow the size but makes no checks to ensure that, it should reduces buffer on final write to ensure it does not overflow

Yes, that was intentional as I have assumes that MCUboot should write at BOOT_MAX_ALIGN pace anyway, but maybe that should be fixed.

nordicjm · 2024-11-18T08:17:57Z

boot/bootutil/src/loader.c

+    } else {
+        size = MAX(sizeof(((struct image_header *)0)->ih_magic),
+                   BOOT_MAX_ALIGN);
+        size = (size + BOOT_MAX_ALIGN - 1) & ~(BOOT_MAX_ALIGN - 1);


This should do the same for footer data because it might contain swap status or mcuboot flags for swapping images and those should always be cleared

Ah right, header and slot trailer are verified separately.

nordicjm · 2024-11-18T08:19:09Z

boot/zephyr/Kconfig

+	  The same can be achieved with just removal of header, leaving the
+	  rest of image untouched, as without header MCUboot will not be able
+	  to recognize image in slot as bootable.


include footer

nordicjm · 2024-11-18T08:20:36Z

boot/bootutil/src/bootutil_public.c

+#if !defined(CONFIG_MCUBOOT) && defined(__ZEPHYR__)
+            flash_area_flatten(fa, 0, flash_area_get_size(fa));
+#else


think it was mentioned in a previous PR that OS specific code should not be present in here

I can fix that although MCUboot has significant problem where we are coupled with Flash Map API which means that this has to be replaced with a function that is implemented for every supported system. I will figure way around.

de-nordic · 2024-12-10T19:23:53Z

Converting to draft; after significant changes I need to re-test this on devices.

nordicjm

Changes look OK

nordicjm · 2025-01-03T11:56:16Z

boot/bootutil/src/loader.c

+    int ret = 0;
+
+    /* Whether device with erase or not, without minimal scramble
+     * removing deata in entire slot.


nordicjm · 2025-01-03T11:56:29Z

boot/bootutil/src/loader.c

+
+            /* Corrected offset and size of current sector to erase */
+            off = flash_sector_get_off(&sector);
+            cs = flash_sector_get_size(&sector);


would rename from cs as I keep thinking chip select

nordicjm · 2025-01-03T11:57:21Z

boot/bootutil/src/swap_misc.c

-        slot = BOOT_PRIMARY_SLOT;
-    } else if (flash_area_get_id(fap) == fa_id_secondary) {
-        slot = BOOT_SECONDARY_SLOT;
+        /* delete starting from last sector and moving to beginning */


*Delete (capital)

nordicjm · 2025-01-03T11:57:56Z

boot/bootutil/src/swap_misc.c

+        int fa_id_secondary;
+        uint8_t image_index;
+
+        BOOT_LOG_DBG("erasing trailer; fa_id=%d", flash_area_get_id(fap));


Suggested change

BOOT_LOG_DBG("erasing trailer; fa_id=%d", flash_area_get_id(fap));

BOOT_LOG_DBG("Erasing trailer; fa_id=%d", flash_area_get_id(fap));

Would be nice to fix the text whilst we're here

nordicjm · 2025-01-03T11:58:07Z

boot/bootutil/src/swap_misc.c

    } else {
-        return BOOT_EFLASH;
+        BOOT_LOG_DBG("erasing trailer not required; fa_id=%d", flash_area_get_id(fap));


Suggested change

BOOT_LOG_DBG("erasing trailer not required; fa_id=%d", flash_area_get_id(fap));

BOOT_LOG_DBG("Erasing trailer not required; fa_id=%d", flash_area_get_id(fap));

nordicjm · 2025-01-03T11:58:19Z

boot/bootutil/src/swap_misc.c

-        sector--;
-        total_sz += sz;
-    } while (total_sz < trailer_sz);
+    BOOT_LOG_DBG("scrambling trailer; fa_id=%d", flash_area_get_id(fap));


nordicjm · 2025-01-03T11:58:24Z

boot/bootutil/src/swap_misc.c


-    return rc;
+    /* delete starting from last sector and moving to beginning */


nordicjm · 2025-01-03T11:59:46Z

boot/zephyr/Kconfig

+	  When this option is enabled, MCUboot will check for type of device
+	  and will avoid erase where not needed.
+
+# The depends on FLASH_HAS_EXPLICIT_ERASE is intentionlally missing for now


*intentionally. Also not sure on this, this option should not be deselectable if the device requires an erase

You are not able to tell whether external device, that zephyr has configured driver for, is actually used by MCUboot

it really should have been a dts property, making it kconfig was a bad choice

No. It was right choice.
Making it DTS property would require to make it Kconfig at some point anyway, otherwise you would not be able to tune your subsystems according to what hardware has to offer, or rather what drivers are supporting.
Also, you can have, as I do, drivers that have no DTS entries but can still set Kconfigs to tune subsystems that use them.

dts means it can be used from inside of Kconfig and cmake and it is known per driver, having it at Kconfig means you have no clue about per driver. It is a hardware feature, not a software feature.

# Workaround for not being able to have commas in macro arguments DT_CHOSEN_Z_CODE_PARTITION := zephyr,code-partition config FLASH_LOAD_SIZE default $(dt_chosen_reg_size_hex,$(DT_CHOSEN_Z_CODE_PARTITION)) depends on BOARD_LPCXPRESSO55S28 && TRUSTED_EXECUTION_SECURE

Like above, you can always get per instance in kconfig from dts also

I have a clue, because I can look at the Kconfig symbol and see what is selecting it, except things picked by ${dt_} macros, which just evaluate to value and you can not tell where they come from, until you open Kconfig, identify the DT, and then search for generated DTS file to figure out. That happens per driver.

Symbols currently y-selecting this symbol: - NORDIC_QSPI_NOR - SOC_FLASH_NRF

It is true that erase is hardware feature (or rather problem), but Kconfig exposes here that certain feature exists, is exposed by software/driver, and should be potentially addressed.
Following the example you have provided we can always do this:

config FLASH_HAS_HAS_SOME_FEATURE default $(dt_compat_any_has_prop, some-prop)

as well as we can just avoid it and go with directly setting the Kconfig by driver, because driver itself will be filtered out if there is no DTS option for it (usually, if driver has proper depends).

Problem with selecting things via ${dt_} macros is that you either have to add a lot of different compat, one per feature, or introduce device classes, because properties have common name space and "some-prop" existing in flash device can not be easily distinguished from "some-prop" in, for example, modem.

And still after having all the information you only know whether there is any device that has a feature but it is up to you to figure out if anything uses it and disable path that don't.

nordicjm

noticed swap using offset file update is missing

nordicjm · 2025-01-29T10:51:53Z

boot/bootutil/src/bootutil_misc.c

+        *size = flash_sector_get_size(&sector);
+    } else {
+        /* For device not requiring erase align to write block */
+        *size = ALIGN_UP(sizeof(((struct image_header *)0)->ih_magic), write_block);


this does not work for swap using offset

nordicjm · 2025-01-29T10:52:20Z

boot/bootutil/src/loader.c

+    return ret;
+}
+
+


Suggested change

nordicjm · 2025-01-29T10:53:13Z

boot/bootutil/src/loader.c

+    size = flash_area_get_size(fa);
+    ret = boot_scramble_region(fa, 0, size);
+#else
+


Suggested change

Getter for sector size stored in flash_sector object. Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no>

The commit adds two MCUboot configuration options: - MCUBOOT_SUPPORT_DEV_WITHOUT_ERASE - MCUBOOT_SUPPORT_DEV_WITH_ERASE - MCUBOOT_MINIMAL_SCRAMBLE The first one should be enabled to support devices that do not require erase. When such devices are used in system then MCUboot will avoid erasing such device, which is not needed by hardware, and will just write data to it. This allows to both improve device lifetime and reduce time of operations like swap. The second option is just bringing a configuration option for already existing support for deviceses with erase. The third option allows to reduce amount of removed data. When enabled, MCUboot will remove enough of data, depending on the purpose of the removal, to just fulfill the purpose; for example if removal of data is done to make image unrecognizable for MCUboot, with this option, it will only remove header. Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no>

Add Kconfig options: - CONFIG_MCUBOOT_STORAGE_WITHOUT_ERASE that enables MCUboot configuration MCUBOOT_SUPPORT_DEV_WITHOUT_ERASE - CONFIG_MCUBOOT_STORAGE_WITH_ERASE that enables MCUboot configuration MCUBOOT_SUPPORT_DEV_WITH_ERASE - CONFIG_MCUBOOT_STORAGE_MINIMAL_SCRAMBLE that enables MCUboot configuration MCUBOOT_MINIMAL_SCRAMBLE Adds implementation of flash_area_erase_required, which is required when MCUBOOT_STORAGE_DEV_WITHOUT_ERASE is enabled. Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no>

The intention of bs_custom_storage_erase is to remove data from device; to support devices that do not require erase, without calling erase, so that devices that do not implement such functions could work, the flash_area_erase has been replaced with flash_area_flatten. Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no>

By default enable all other systems to work with devices that require erase prior to write. Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no>

de-nordic force-pushed the improve-no-erase-device-time branch 15 times, most recently from cf8def0 to 03ac085 Compare November 15, 2024 12:22

de-nordic marked this pull request as ready for review November 15, 2024 13:48

de-nordic requested review from nordicjm and davidvincze as code owners November 15, 2024 13:48

de-nordic force-pushed the improve-no-erase-device-time branch from 03ac085 to b96fe4a Compare November 15, 2024 14:42

nordicjm reviewed Nov 18, 2024

View reviewed changes

de-nordic force-pushed the improve-no-erase-device-time branch from b96fe4a to 269f9e2 Compare December 10, 2024 19:23

de-nordic requested review from almir-okato, romanjoe and d3zd3z as code owners December 10, 2024 19:23

de-nordic marked this pull request as draft December 10, 2024 19:23

de-nordic force-pushed the improve-no-erase-device-time branch 4 times, most recently from 75494c5 to 5a405e7 Compare December 18, 2024 13:47

de-nordic force-pushed the improve-no-erase-device-time branch 3 times, most recently from 693327f to cec59f0 Compare December 19, 2024 14:56

de-nordic marked this pull request as ready for review December 19, 2024 15:05

de-nordic force-pushed the improve-no-erase-device-time branch from cec59f0 to 8b23727 Compare December 19, 2024 15:20

de-nordic self-assigned this Dec 19, 2024

de-nordic added area: zephyr Affects the Zephyr port area: core Affects core functionality labels Dec 19, 2024

nvlsianpu self-requested a review December 20, 2024 13:31

de-nordic requested a review from nordicjm January 2, 2025 14:48

nordicjm reviewed Jan 3, 2025

View reviewed changes

de-nordic force-pushed the improve-no-erase-device-time branch 2 times, most recently from 4e2073d to c8bc87f Compare January 7, 2025 13:09

de-nordic force-pushed the improve-no-erase-device-time branch from c8bc87f to 4916f7f Compare January 28, 2025 15:15

de-nordic requested a review from nordicjm January 28, 2025 15:18

nordicjm reviewed Jan 29, 2025

View reviewed changes

de-nordic force-pushed the improve-no-erase-device-time branch 4 times, most recently from c8cbd2e to eea1ddf Compare February 8, 2025 13:52

de-nordic force-pushed the improve-no-erase-device-time branch 2 times, most recently from 0b688f9 to 047fc91 Compare February 11, 2025 15:36

zephyrbot mentioned this pull request Feb 11, 2025

mcuboot: Alignments needed for devices without erase zephyrproject-rtos/zephyr#85620

Draft

de-nordic force-pushed the improve-no-erase-device-time branch from 047fc91 to d3300c7 Compare February 11, 2025 15:50

de-nordic added 5 commits February 12, 2025 13:12

mynewt: Add flash_sector_get_size

3c42cfc

Getter for sector size stored in flash_sector object. Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no>

boot: Enable MCUBOOT_DEV_WITH_ERASE in other systems

31b267e

By default enable all other systems to work with devices that require erase prior to write. Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no>

de-nordic force-pushed the improve-no-erase-device-time branch from d3300c7 to 31b267e Compare February 12, 2025 13:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bootutil: Add support for devices without erase #2114

bootutil: Add support for devices without erase #2114

de-nordic commented Nov 11, 2024 •

edited

Loading

nordicjm Nov 18, 2024

nordicjm Nov 18, 2024

de-nordic Nov 18, 2024

nordicjm Nov 18, 2024

de-nordic Nov 18, 2024

nordicjm Nov 18, 2024

nordicjm Nov 18, 2024

de-nordic Nov 18, 2024

de-nordic commented Dec 10, 2024

nordicjm left a comment

nordicjm Jan 3, 2025

nordicjm Jan 3, 2025

nordicjm Jan 3, 2025

nordicjm Jan 3, 2025

nordicjm Jan 3, 2025

nordicjm Jan 3, 2025

nordicjm Jan 3, 2025

nordicjm Jan 3, 2025

de-nordic Jan 28, 2025

nordicjm Jan 29, 2025

de-nordic Jan 29, 2025

nordicjm Jan 29, 2025 •

edited

Loading

de-nordic Jan 29, 2025

nordicjm left a comment

nordicjm Jan 29, 2025

nordicjm Jan 29, 2025

nordicjm Jan 29, 2025

		int boot_erase_region(const struct flash_area *fap, uint32_t off, uint32_t sz);
		/* SImilar to boot_erase_region but will alwasy remove data */

	BOOT_LOG_DBG("erasing trailer; fa_id=%d", flash_area_get_id(fap));
	BOOT_LOG_DBG("Erasing trailer; fa_id=%d", flash_area_get_id(fap));

	BOOT_LOG_DBG("erasing trailer not required; fa_id=%d", flash_area_get_id(fap));
	BOOT_LOG_DBG("Erasing trailer not required; fa_id=%d", flash_area_get_id(fap));


		return rc;
		/* delete starting from last sector and moving to beginning */

bootutil: Add support for devices without erase #2114

Are you sure you want to change the base?

bootutil: Add support for devices without erase #2114

Conversation

de-nordic commented Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

de-nordic commented Dec 10, 2024

nordicjm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nordicjm Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nordicjm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

de-nordic commented Nov 11, 2024 •

edited

Loading

nordicjm Jan 29, 2025 •

edited

Loading