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

boot: bootutil: Add boot information #1692

Merged
merged 5 commits into from
Aug 8, 2023
Merged

boot: bootutil: Add boot information #1692

merged 5 commits into from
Aug 8, 2023

Conversation

nordicjm
Copy link
Collaborator

@nordicjm nordicjm commented Apr 27, 2023 •
edited
Loading

Adds TLV defines and optional function for use with the bootloader shared data feature.

@zephyrbot zephyrbot mentioned this pull request Apr 27, 2023
Closed
@nordicjm nordicjm marked this pull request as ready for review April 28, 2023 12:52
@nordicjm
Copy link
Collaborator Author

nordicjm commented May 5, 2023

@utzig @de-nordic @nvlsianpu @d3zd3z can you review this PR please?

@de-nordic de-nordic self-assigned this May 9, 2023
@de-nordic de-nordic added the area: zephyr Affects the Zephyr port label May 9, 2023
de-nordic
de-nordic reviewed May 16, 2023
View reviewed changes
boot/bootutil/src/loader.c Outdated
Comment on lines 149 to 150
BOOT_IMG_AREA(state, active_slot),
active_slot);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure whether this is correct. I mean sharing active_slot may be correct because this is MCUboot/MCUmgr established slot numbering, but the BOOT_IMG_AREA(state, active_slot) pointer (struct flash_area) may differ between MCUboot and app builds, so sharing it does not help.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

active_slot is what I added for XIP, the boot image area is part of the original API which presumably things like mynewt use so don't want to change/break it.

@nordicjm nordicjm requested a review from de-nordic June 6, 2023 09:26
@nordicjm nordicjm force-pushed the zephyrsharedata branch 2 times, most recently from 7180d75 to 05192ef Compare June 6, 2023 14:34
@zephyrbot zephyrbot mentioned this pull request Jun 7, 2023
Merged
@nordicjm nordicjm mentioned this pull request Jun 21, 2023
Closed
@Laczen
Copy link

Laczen commented Aug 2, 2023

@nordicjm, I am surprized that you have not taken any action to remove this PR.

The solution provided is not beneficial for mcuboot. The introduction of a zephyr dependent config #ifndef __zephyr___ in bootutil code will start a deviation from common code to os specific code. The basics of the proposed PR: using the retention system to share data is wrong and unnecessary. A simple change to the mcuboot config allows to define the shared data memory as a part of a zephyr dts file.

@d3zd3z
Copy link
Member

d3zd3z commented Aug 3, 2023

My suggestions would be to perhaps split this into two PRs, as we are kind of doing two different things here. One is using the retention system in Zephyr as a mechanism to share data. The other is a new type of boot record. I think both of these are fine, but the new type of boot record should probably just be in the main boot code, and not Zephyr specific, and just the use of retention would be in the Zephyr code. These could be selected by configs.

@nordicjm
bootutil: Add active slot number and max app size to shared data
0218d02 
This allows the currently executing slot number to be checked by
the external function, which can be used by XIP images to know
which slot is currently being executed from to allow for correct
uploading/positioning of firmware files, and also provides the
maximum size of an upgrade that can be loaded so that applications
can reject images that are too large.

Signed-off-by: Jamie McCrae <jamie.mccrae@nordicsemi.no>
@nordicjm
bootutil: Add bootloader info TLV entries
d43030f 
Adds TLV defines for use with the bootloader shared data
feature.

Signed-off-by: Jamie McCrae <jamie.mccrae@nordicsemi.no>
@nordicjm nordicjm mentioned this pull request Aug 3, 2023
Merged
@nordicjm nordicjm changed the title Zephyr: Add shared boot information support boot: bootutil: Add boot information Aug 3, 2023
@nordicjm nordicjm added [DNM] Do Not Merge area: core Affects core functionality and removed area: zephyr Affects the Zephyr port [DNM] Do Not Merge labels Aug 3, 2023
d3zd3z
d3zd3z requested changes Aug 4, 2023
View reviewed changes
Copy link
Member

@d3zd3z d3zd3z left a comment

Choose a reason for hiding this comment

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

Small issue with a weird include that I think is redundant anyway.

boot/bootutil/src/boot_record.c Outdated Show resolved Hide resolved
@nordicjm nordicjm requested a review from d3zd3z August 7, 2023 06:35
@Laczen
Copy link

Laczen commented Aug 7, 2023

IMO this PR does not follow the intended use of mcuboot shared data:

  1. boot_add_data_to_shared_area() allows to add a tlv to the shared data area. This should be used directly at the start to save any configuration specific info to the shared data area.
  2. boot_save_shared_data() allows image specific data to be added and has to be implemented by the os.

If the boot_save_shared_data() is to be default included in the library this might break existing os implementations.

@nordicjm
boot: bootutil: Add optional boot info shared data saving
883d287 
Adds the ability to share MCUboot configuration with
applications via shared memory.

Signed-off-by: Jamie McCrae <jamie.mccrae@nordicsemi.no>
@nordicjm
docs: Add note on addition of new shared boot info
933e30f 
Adds a note that there is now additional information that can
(optionally) be provided via shared boot information.

Signed-off-by: Jamie McCrae <jamie.mccrae@nordicsemi.no>
@nordicjm
doc: design: Add updated information on data sharing
0d72961 
Adds additional information on the new BOOTINFO data sharing
functionality

Signed-off-by: Jamie McCrae <jamie.mccrae@nordicsemi.no>
@nordicjm nordicjm merged commit 88b2865 into mcu-tools:main Aug 8, 2023
54 checks passed
@Laczen Laczen mentioned this pull request Aug 9, 2023
Closed
@nordicjm nordicjm deleted the zephyrsharedata branch December 12, 2023 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Laczen Laczen Laczen left review comments

@thedjnK thedjnK thedjnK left review comments

@nvlsianpu nvlsianpu nvlsianpu approved these changes

@d3zd3z d3zd3z d3zd3z approved these changes

@utzig utzig Awaiting requested review from utzig

@de-nordic de-nordic Awaiting requested review from de-nordic

Assignees

@de-nordic de-nordic

Labels
area: core Affects core functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nordicjm @Laczen @d3zd3z @thedjnK @nvlsianpu @de-nordic