boot: imgtool: Extend align to 128B

[URNHere]

I wanted to use the latest mcuboot upstream version together building it with Zephyr.
We are using a Renesas RA6M3 MCU, which has a controller which can write the flash only 128Bytes at a time.
I extended the aligment and max-alignment in the imgtool signer to allow the app to be signed by west, and the image gets correctly signed.

For what concerns bootloader support, I managed to make the boot work on a custom board we have by setting CONFIG_BOOT_SERIAL_UNALIGNED_BUFFER_SIZE=128.
CONFIG_BOOT_MAX_IMG_SECTORS_AUTO=n
CONFIG_BOOT_MAX_IMG_SECTORS=14
As the renesas drivers do not support the soc-nv-flash compatible yet and the mapped-partitions but AFAIK there is some work in progress here:
zephyrproject-rtos/zephyr#110365
zephyrproject-rtos/zephyr#109125

[d3zd3z]

This doesn't appear to test the larger alignment in the simulator.

Also, the docs/design.md needs to be updated to include the new alignments possible.

[URNHere]

Thanks for the review.
I followed what has been done in #2564.
Didn't edit the sim.yaml yet (maybe you have some advice before I do).
When I run cargo test --features "max-align-128" or --features "max-align-64", the only test that fails is oversized_secondary_slot and only for Stm32f4SpiFlash (if I comment it out from ALL_DEVICES all tests pass).
Do you have any hints?
First time contributing, and not so proficient with rust, let me know if you have some advice.

[d3zd3z]

Please ensure the simulator support is also then tested. Also, the docs still need to be updated to include the new alignments.

[jeremydick]

#2801 will handle setting CONFIG_BOOT_SERIAL_UNALIGNED_BUFFER_SIZE to 128 for all renesas RA family of MCUs

[nordicjm]

Would suggest reading comments on #2440 if your write size is 128 bytes then really it doesn't make much sense to use a swap mode since your overhead is so large it destroys any real swap use case

[URNHere]

Thanks @nordicjm for the review. Still new to the project, and I missed that closed PR.
So, no swap modes due to the overhead it brings regarding flash size (I'll remove that assert line change).
However, I am missing one point to fully understand: #2440 was closed, but if I want to sign an image with Imgtool using --overwrite-only for the Renesas RA family I still need the modifications to allow 128B as max-align (since it is used by Imgtool even in that case), am I correct or am I missing something?

[jeremydick]

I'm not understanding the reason for the closure of #2440 either. I believe swap is still usable. The write block size is 128 bytes, but the erase block or sector size is larger (a combination of 8-8KiB and 30-32KiB blocks on the RA6M3, other RA6 family have similar). If you treat it all as 32KiB sectors, only 6KiB is required for the trailer, rather than 1.5MiB. Treating it all as 8KiB sectors requires a 24KiB trailer. Not great, but not unusable.

[URNHere]

Yes, also looking back at the CMakeLists.txt we have:

 if(CONFIG_BOOT_MAX_IMG_SECTORS_AUTO)
    if(DEFINED slot0_size AND DEFINED erase_size_slot0)
      math(EXPR slot_min_sectors "${slot0_size} / ${erase_size_slot0}")

      if(${slot_min_sectors} GREATER ${auto_min_sectors})
        set(auto_min_sectors ${slot_min_sectors})

        if(${image} EQUAL 0)
          set(image_0_min_sectors ${slot_min_sectors})
        endif()
      endif()

In my case we have:
slot0_size = 448KB, erase_size_slot0 = 32KB (.dts of the RA6M3 here ), as I reserved the small 8K sectors to MCUBoot, and all the big 32K to the app) , which yields to slot_min_sectors= 14.

Then we have:
 if(CONFIG_BOOT_SWAP_USING_SCRATCH OR CONFIG_BOOT_SWAP_USING_MOVE)
      if(CONFIG_BOOT_MAX_IMG_SECTORS_AUTO AND DEFINED image_0_min_sectors AND "${image_0_min_sectors}" GREATER "0")
        math(EXPR boot_status_data_size "${image_0_min_sectors} * (3 * ${write_size})")

And:
math(EXPR boot_swap_data_size "${max_align_size} * 4")

boot_status_data_size = 14 * 3 * 128 = 5376 B
boot_swap_data_size = 128*4 = 512 B

In the end:
math(EXPR trailer_size "${key_size} + ${boot_magic_size} + ${boot_swap_data_size} + ${boot_status_data_size}")
trailer_size = 256 (considering RSA) + 128 (magic is aligned to 128, which is the write size) + 5376 + 512 = 6272 B, which needs a whole 32KB sector, because it has to be aligned to the erase size. Not so great, but definitely far from the 1.5MB depicted in #2440.

@nordicjm You're far more expert than me, can you tell me please if we miss something?

P.S. I found out how it manages to sign in overwrite only mode, it sets align to 1 here

[nordicjm]

Thanks @nordicjm for the review. Still new to the project, and I missed that closed PR. So, no swap modes due to the overhead it brings regarding flash size (I'll remove that assert line change). However, I am missing one point to fully understand: #2440 was closed, but if I want to sign an image with Imgtool using --overwrite-only for the Renesas RA family I still need the modifications to allow 128B as max-align (since it is used by Imgtool even in that case), am I correct or am I missing something?

With overwrite only, the max align is ignored since you have nothing extra to write, so you don't need any changes

[nordicjm]

I'm not understanding the reason for the closure of #2440 either. I believe swap is still usable. The write block size is 128 bytes, but the erase block or sector size is larger (a combination of 8-8KiB and 30-32KiB blocks on the RA6M3, other RA6 family have similar). If you treat it all as 32KiB sectors, only 6KiB is required for the trailer, rather than 1.5MiB. Treating it all as 8KiB sectors requires a 24KiB trailer. Not great, but not unusable.

The comment gives the figures it uses which was based on a random image size of 512KiB and assuming that the erase and write block sizes were the same (the user did not specify an erase size)

[URNHere]

Ok thanks for all the clarifications.
Updated the sim.yaml to include some testing for the new aligments and the docs, in case you want to go ahead and give it a test.
I had to disable the oversized_secondary_slot because it was failing only on one device (stm32f4ExternalSpi).
@nordicjm and @d3zd3z if you consider it something useful to add we can go ahead, otherwise we can close the PR, up to you.

[URNHere]

Should be signed-off now, sorry about that

[d3zd3z]

I think this is in reasonably good shape. I think, aside from this, we probably have an issue with the CI test for each PR getting too long, but that is something to address separately. There is a formatting issue in the docs, which should be fixable by pasting in the actual output of --help instead of editing that output.

[d3zd3z]

I have confirmed that #2808 fixes the issue with the Stm32f4SpiFlash, and the combined result passes CI without needing to disable at specific part.

[URNHere]

Great, I'll remove the suppression, thanks for your help!
I'll let the CI finish a full round of tests to assess if there is anything else to do before pushing another time.

[d3zd3z]

We'll need to get 2808 merged before I'd expect CI to pass. You can also pull 2808 into your tree, rebase on top of that, and set the merge base of this PR to my branch, but it might be easier to just get the 2 reviews on 2808 and get it merged, and you can then just rebase on main.

[URNHere]

Ok, I'll keep an eye out for #2808 being merged, I'll rebase and remove that suppression before pushing again.
Thanks for your help! Highly appreciated!

[URNHere]

Rebased on top of main which now features the commits contained in #2808 and edited the second commit to remove the suppression tests/core.rs not in the modified files list anymore.