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.

Sign up for GitHub

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

Jump to bottom

Update Mbed OS CMake build file for Mbed CE #2152

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Update Mbed OS CMake build file for Mbed CE #2152

wants to merge 1 commit into from

Conversation

multiplemonomials
Copy link

Hi, I'm Jamie, maintainer of Mbed CE, the community fork of Mbed. As Mbed OS 6 has been end-of-lifed by ARM, we hope to keep support for the OS alive in our fork for a long time coming!

Several members of the project have expressed interest in getting mcuboot running with Mbed CE, so I took some time and put together a test project based off of George Beckstein's original work. I was able to get it working more or less out of the box, but I did have to make some build system changes to get things running.

In particular, I wasn't happy with how the original design required running manual commands after compiling the code. That was an unfortunate necessity with the original Mbed 5/6 build system, but with Mbed CE, we use standard CMake, so we can improve this situation! I added a CMake script which automates calling imgtool to generate initial images, update images, and the public key file needed to build the bootloader.

With these changes, I am able to get a rudimentary demo application booting and updating. I and the others on the mbed-ce project (including @lefebvresam and @zhiyong-ft) hope to test out (and document) more features in time!

On another note, we heard from this issue that there is discussion about removing the Mbed port of mcuboot. We at mbed-ce are happy to help maintain and test the port going forward, and really want it to stay in the main branch of mcuboot!

Compatibility note: This PR will maintain compatibility with Mbed OS 6 using the Mbed CLI build system and/or Mbed Studio. However, it will break compatibility with the Mbed CLI 2 CMake-based build system. This build system never really reached maturity before ARM Mbed shut down, so I doubt that too many people will be using it who do not use Mbed CE.

multiplemonomials
multiplemonomials commented Dec 18, 2024
View reviewed changes
boot/bootutil/src/swap_scratch.c Outdated
@@ -174,6 +174,13 @@ boot_slots_compatible(struct boot_loader_state *state)
j = sz1 = secondary_slot_sz = 0;
smaller = 0;
while (i < num_sectors_primary || j < num_sectors_secondary) {
// Make sure that the boot_img_sector_size() calls below don't access undefined memory.
Copy link
Author

Choose a reason for hiding this comment

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

I ran into a rather nasty issue when testing where I didn't set BOOT_MAX_IMG_SECTORS large enough, so this for loop exhibited undefined behavior. Throwing in a check against this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

this doesn't even make sense, this is already checked on line 156 above

Copy link
Author

@multiplemonomials multiplemonomials Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, I checked in the debugger, and if I set BOOT_MAX_IMG_SECTORS to 1000, on a device that uses 1536, I see:
image.

Looking into it a bit more, it seems this may be an issue due to the Mbed version of flash_area_get_sectors() never returning a count value greater than MCUBOOT_MAX_IMG_SECTORS. That causes the existing warning (which I totally didn't notice X_X) to never trigger.

Copy link
Author

Choose a reason for hiding this comment

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

ok pushed a fix!

@lefebvresam lefebvresam mentioned this pull request Dec 18, 2024
Open
nordicjm
nordicjm requested changes Dec 18, 2024
View reviewed changes
boot/bootutil/src/swap_scratch.c Outdated
@@ -174,6 +174,13 @@ boot_slots_compatible(struct boot_loader_state *state)
j = sz1 = secondary_slot_sz = 0;
smaller = 0;
while (i < num_sectors_primary || j < num_sectors_secondary) {
// Make sure that the boot_img_sector_size() calls below don't access undefined memory.
Copy link
Collaborator

Choose a reason for hiding this comment

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

this doesn't even make sense, this is already checked on line 156 above

boot/bootutil/src/swap_scratch.c Outdated
// Make sure that the boot_img_sector_size() calls below don't access undefined memory.
if(i >= BOOT_MAX_IMG_SECTORS || j >= BOOT_MAX_IMG_SECTORS)
{
BOOT_LOG_WRN("Cannot upgrade: BOOT_MAX_IMG_SECTORS too small");
Copy link
Collaborator

Choose a reason for hiding this comment

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

as above

nordicjm
nordicjm reviewed Dec 18, 2024
View reviewed changes
Copy link
Collaborator

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

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

changes need to be squashed

boot/mbed/CMakeLists.txt Show resolved Hide resolved
boot/mbed/mcuboot_imgtool.cmake Outdated
# Find or install imgtool ------------------------------------------------------------------------

if(NOT MCUBOOT_IMGTOOL_FOUND)

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

boot/mbed/mcuboot_imgtool.cmake Outdated

get_filename_component(IMGTOOL_SCRIPTS_DIR ${CMAKE_CURRENT_LIST_DIR}/../../scripts REALPATH)

# Find or install imgtool ------------------------------------------------------------------------
Copy link
Collaborator

Choose a reason for hiding this comment

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

dashes not needed on comment

Copy link
Author

Choose a reason for hiding this comment

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

removed

boot/mbed/mcuboot_imgtool.cmake Outdated

if(NOT MCUBOOT_IMGTOOL_FOUND)

# If we are using the Mbed venv, we can install asn1tools automatically
Copy link
Collaborator

Choose a reason for hiding this comment

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

cmake indent is 2 spaces not 4

Copy link
Author

Choose a reason for hiding this comment

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

converted to 2

boot/mbed/mcuboot_imgtool.cmake Outdated
# this function. Otherwise, it will default to ${CMAKE_CURRENT_BINARY_DIR}/<target name>-initial-image.hex
#
function(mcuboot_generate_initial_image TARGET) # optional 2nd arg: file name

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

boot/mbed/mcuboot_imgtool.cmake Outdated
gen_upload_target(${TARGET}-${IMAGE_TYPE}-image ${IMAGE_BASE_PATH}.bin)
add_dependencies(flash-${TARGET}-${IMAGE_TYPE}-image ${TARGET})

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

boot/mbed/mcuboot_imgtool.cmake Show resolved Hide resolved
boot/mbed/mcuboot_imgtool.cmake Outdated Show resolved Hide resolved
boot/mbed/mcuboot_imgtool.cmake Show resolved Hide resolved
boot/mbed/src/flash_map_backend.cpp Outdated
bd_size_t erase_size = bd->get_erase_size(offset);
sectors[*count].fs_size = erase_size;

if(*count < MCUBOOT_MAX_IMG_SECTORS)
Copy link
Collaborator

Choose a reason for hiding this comment

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

spaces needed in c code i.e. if (

Copy link
Author

Choose a reason for hiding this comment

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

done

nordicjm
nordicjm reviewed Dec 18, 2024
View reviewed changes
boot/mbed/src/flash_map_backend.cpp Outdated
sectors[*count].fs_size = erase_size;

if(*count < MCUBOOT_MAX_IMG_SECTORS)
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

also as code above, bracket goes on same line as if

Copy link
Author

Choose a reason for hiding this comment

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

done

@multiplemonomials multiplemonomials force-pushed the mbed-ce-update-cmakelists branch from 0fa393a to 57c1b31 Compare December 18, 2024 17:21
@zhiyong-ft
Copy link

@multiplemonomials I tested encrypted image with 0fa393a as bootloader, and mcuboot v1.8.0 included in primary application on DISCO_F746NG. This feature seems working now. I guess there was a bug in v1.8.0 to upgrade from encrypted image. I will wait until you are done with this batch of revision to update documentation.

A minor issue: is target.restrict_size no longer honored? I have this in mbed_app.json5:

 "target.restrict_size": "0x1C000",

The build bootloader hex file has content up till 0x1D034.

Other than this I don't see other issues as far as bootloader is concerned.

@lefebvresam
Copy link

lefebvresam commented Dec 18, 2024 •
edited
Loading

@zhiyong-ft If you use the new methodology with flash bank settings, then:

            // This replaces "target.restrict_size":
            "target.memory_bank_config": {
                "FLASH": {
                    "size": 0xBE000,
                    "start": 0x8021000
                }
            }

Size without Application header (0x1000) and Trailer TLV's (0x1000), total slot size 0xC0000 in my case.
Start at the end of Application Header, which is at 0x8020000.

@zhiyong-ft
Copy link

@zhiyong-ft If you use the new methodology with flash bank settings, then:

            // This replaces "target.restrict_size":
            "target.memory_bank_config": {
                "FLASH": {
                    "size": 0xBE000,
                    "start": 0x8021000
                }
            }

Size without Application header (0x1000) and Trailer TLV's (0x1000), total slot size 0xC0000 in my case. Start at the end of Application Header, which is at 0x8020000.

Could you point me the documentation on this? I tried to build the bootloader for NUCLEO_F767ZI, starting address in hex file is: 0x200000, while the correct starting address should be 0x8000000. I suspect this has something to do with this setting not being set.

@lefebvresam
Copy link

lefebvresam commented Dec 18, 2024 •
edited
Loading

@zhiyong-ft mbed-ce/mbed-os#401 (comment)

Flash.PDF

@nordicjm
Copy link
Collaborator

Just to point out that v1.8.0 is 3 years old at this point, it should not be being used

@lefebvresam
Copy link

Just to point out that v1.8.0 is 3 years old at this point, it should not be being used

v1.8.0 from what?

@nordicjm
Copy link
Collaborator

Just to point out that v1.8.0 is 3 years old at this point, it should not be being used

v1.8.0 from what?

MCUboot: https://github.com/mcu-tools/mcuboot/releases/tag/v1.8.0

@lefebvresam
Copy link

Just to point out that v1.8.0 is 3 years old at this point, it should not be being used

v1.8.0 from what?

MCUboot: https://github.com/mcu-tools/mcuboot/releases/tag/v1.8.0

I am using the fork:

[submodule "mcuboot"]
	path = mcuboot
	url = https://github.com/multiplemonomials/mcuboot.git
	branch = dev/get-version-number

@zhiyong-ft
Copy link

Just to point out that v1.8.0 is 3 years old at this point, it should not be being used

The unfortunate fact is that before @multiplemonomials started to work on it, v1.8.0 was the last working version that can build in Mbed Studio and upgrade unencrypted image from secondary slot, yet crashes when trying to upgrade from encrypted image. But this is behind us now.

@lefebvresam
Copy link

Just to point out that v1.8.0 is 3 years old at this point, it should not be being used

The unfortunate fact is that before @multiplemonomials started to work on it, v1.8.0 was the last working version that can build in Mbed Studio and upgrade unencrypted image from secondary slot, yet crashes when trying to upgrade from encrypted image. But this is behind us now.

Using encrypted images will be my next step. And returning from using mbed-ce is no option anymore. My firmware is almost finished and I spent two years on it.

@zhiyong-ft
Copy link

Just to point out that v1.8.0 is 3 years old at this point, it should not be being used

The unfortunate fact is that before @multiplemonomials started to work on it, v1.8.0 was the last working version that can build in Mbed Studio and upgrade unencrypted image from secondary slot, yet crashes when trying to upgrade from encrypted image. But this is behind us now.

Using encrypted images will be my next step. And returning from using mbed-ce is no option anymore. My firmware is almost finished and I spent two years on it.

I outlined steps needed to get encrypted image to work in another thread. Give it a try. It is rather straightforward once we know it is working. It took me a while to map out the kinks because it was not mentioned at all in Mr. Beckstein's original port. He too seems have had some trouble.

@lefebvresam
Copy link

Just to point out that v1.8.0 is 3 years old at this point, it should not be being used

The unfortunate fact is that before @multiplemonomials started to work on it, v1.8.0 was the last working version that can build in Mbed Studio and upgrade unencrypted image from secondary slot, yet crashes when trying to upgrade from encrypted image. But this is behind us now.

Using encrypted images will be my next step. And returning from using mbed-ce is no option anymore. My firmware is almost finished and I spent two years on it.

I outlined steps needed to get encrypted image to work in another thread. Give it a try. It is rather straightforward once we know it is working. It took me a while to map out the kinks because it was not mentioned at all in Mr. Beckstein's original port. He too seems have had some trouble.

Can you paste a link to that?

@multiplemonomials multiplemonomials force-pushed the mbed-ce-update-cmakelists branch 3 times, most recently from e8017b7 to ce340d5 Compare December 19, 2024 17:30
@multiplemonomials
Copy link
Author

@zhiyong-ft @nordicjm I have updated this PR in response to comments and to add a way to generate encrypted images. Should be ready for another look!

@zhiyong-ft
Copy link

@multiplemonomials I can confirm that mcuboot as of ce340d5 can boot primary application and upgrade encrypted images from secondary slot.

nordicjm
nordicjm approved these changes Dec 20, 2024
View reviewed changes
Copy link
Collaborator

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

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

minor nit. looks ok

boot/mbed/CMakeLists.txt Outdated Show resolved Hide resolved
@multiplemonomials multiplemonomials force-pushed the mbed-ce-update-cmakelists branch 2 times, most recently from 6b84b5a to a4da87d Compare December 20, 2024 08:49
@multiplemonomials
Copy link
Author

@nordicjm updated to add signed-off-by, could you trigger CI again?

@multiplemonomials
Copy link
Author

Any idea why the Zephyr check is failing? I don't believe it's related to anything I did...

@nordicjm
Copy link
Collaborator

Any idea why the Zephyr check is failing? I don't believe it's related to anything I did...

You need to rebase your branch, that issue has been fixed

@multiplemonomials
Update Mbed CMakeLists.txt for Mbed CE
de8c4b0 
Signed-off-by: Jamie Smith <jsmith@crackofdawn.onmicrosoft.com>
@multiplemonomials multiplemonomials force-pushed the mbed-ce-update-cmakelists branch from a4da87d to de8c4b0 Compare December 24, 2024 06:58
@multiplemonomials
Copy link
Author

Oh, gotcha. Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhiyong-ft zhiyong-ft zhiyong-ft left review comments

@nordicjm nordicjm nordicjm approved these changes

@davidvincze davidvincze Awaiting requested review from davidvincze

@d3zd3z d3zd3z Awaiting requested review from d3zd3z d3zd3z is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@multiplemonomials @zhiyong-ft @lefebvresam @nordicjm