-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
boot_info: A subsystem to share bootloader info #59375
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate of #59025
f89ac09
to
b9a46cc
Compare
11cc261
to
35f12dd
Compare
Thanks. Alternatives are nice when evaluating the best solution for introducing new functionality. To ensure I am understanding the two proposals correctly - this PR adds a generic way of storing/retrieving a byte array between bootloader and application, but it doesn't define any semantics for the contents of this byte array? Whereas #59025 provides a similar functionality, but also adds semantics for the stored data and allows storing/retrieving using the settings API? |
Correct, this PR adds a shared memory area that does not signify anything about the data stored there and does not do anything with mcuboot or other bootloaders out of the box, the other PR adds mcuboot code to store the bootloader information and zephyr code to read that information, and allows it to be accessed via settings keys (the infrastructure is designed so that other bootloader support could also be added, it would have to work in a similar fashion of having the data accessible via settings with the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking for now as I cannot see this adding any functionality not already present in the existing retention system implementation.
Hi @henrikbrixandersen, thank you for the feedback. Regarding the added functionality: the PR also provides using bbram as the backend for the boot_info system. The way it is designed also allows to add a flash partition or a general purpose register as backend. Regarding a comparison with existing solutions: the PR provides a solution without any drivers that need to be created, saving scarce resources. The proposed solution is also simple to setup in a dts file and very simple to use. The The PR does not try to define what the data format is that should be used, and it does so for several reasons: To summarize: the PR adds functionality by extending the backends that can be used to share information and it reduces the resources used. At the same time it is also easy to expand to other storage solutions. Finally, since there is some concern about duplicate functionality you could consider it to replace the |
boot_info is a simple subsystem to share information between a bootloader and an application. It can store the info in a bbram device or a zephyr,memory-region (RAM). Signed-off-by: Laczen JMS <laczenjms@gmail.com>
Added the possibility to store the boot information on a flash partition or on an eeprom. Signed-off-by: Laczen JMS <laczenjms@gmail.com>
Architecture WG:
|
@carlescufi @henrikbrixandersen @de-nordic @nordicjm mcuboot can easily be modified to share information trough a shared memory (ram) method using:
The zephyr application can then read the area using mcuboot specific routines to convert the tlv stored data. This method could be used by any bootloader. |
So your solution to what you brought up at the architecture working group is not to use the system you brought up at the architecture working group at all. @carlescufi can we move forward with this case now being resolved in favour of #59025 ? |
No, this is about the bootloader side, the zephyr app side does need some interface to work with. But as you have noticed, it is indeed possible to use mcuboot without any zephyr specific subsystem. This illustrates how bad the proposal of #59025 really is, it adds lots of things without being required. If this was not known from the start it clearly justifies reworking #59025 to a simpler solution, if it was known there was misleading of developers. |
@carlescufi, during the architecture meeting it was requested to put some figures on the size difference between the retention system and the boot_info system. I have added a test to allow an easy comparison (https://github.com/Laczen/zephyr/tree/retention_test). The result of this test: The retention subsystem adds 2.3kB of code compared to the boot_info subsystem. To put this in perspective the mcuboot bootloader is considered to be getting obese at around 22kB. |
Probably because you have forgotten something (e.g. the retained_mem driver ?), the reference to the test is given and this is a comparison between both systems. Nothing to hide, easy to replicate. As I said, you forgot to include a part in the comparison (the retained_mem driver): on the nr52840dk_nrf52840 vanilla hello_world: Memory region Used Size Region Size %age Used
FLASH: 20184 B 1 MB 1.92%
RAM: 5568 B 63 KB 8.63%
RetainedMem: 0 GB 1 KB 0.00%
IDT_LIST: 0 GB 2 KB 0.00% on the nrf52840dk_nrf52840 modified hello_world (your version): Memory region Used Size Region Size %age Used
FLASH: 22428 B 1 MB 2.14%
RAM: 5632 B 63 KB 8.73%
RetainedMem: 0 GB 1 KB 0.00%
IDT_LIST: 0 GB 2 KB 0.00% So the conclusion remains. |
@Laczen how come you have ram of 63k on nr52840dk_nrf52840? This board has 256 of RAM as shown here as well: #59375 (comment) |
Because in the overlay to define the |
So your size post can be entirely then discarded then, since neither the "virgin state" nor the "modified state" are valid (note: I provided the overlay, so use of your own overlay doesn't even make sense). |
Man oh man, any of my code size comparison between I really feel sorry for you that you have so much problems accepting that the |
The astute observer would also notice that your RAM usage makes no sense, with the configuration file I posted (which would be a configuration used with a bootloader such as MCUboot), one can see that the retained memory driver has no global objects that would contribute to RAM usage: https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/retained_mem/retained_mem_zephyr_ram.c Given that yours shows a 64 byte difference in RAM usage, seems like you aren't being honest with your testing. |
The astute observer would just ask: "what is the reason for this difference ?" and the answer would be: |
I don’t understand this last comment. |
Mcuboot has everything to use a part of RAM to store information for the application. It does not need boot_info for this. So the size of boot_info for mcuboot would be 0. Of course for an application the boot_info subsystem would be required. Comparing the size of If wanted I can also add the difference in size for a bootloader. |
No it does not. You're actually talking nonsense and pulling things from the void. If it supports it right now, this entire PR is superfluous to requirements rights? |
@thedjnK It was shown above (#59375 (comment)) how to configure mcuboot to share a part of ram and how data can be added to this. This does not make the entire PR superfluous at all: there are two parts when sharing information between two applications: the first one is adding the data in a bootloader, the second one is providing secure access to this information for the application. |
@henrikbrixandersen, maybe the explanation was not clear enough. What This is what makes When it comes to making a comparison in size between |
You specifically brought this to the architecture working group with the guise that it would write "to flash or BBRAM" from the bootloader (these were your words, as captured in Carle's notes from the meeting above, therefore you need to add your code to do this to MCUboot, it does not magic itself onto the device out of thin air with no additional code additions, and so far you have failed to demonstrate what the additional flash usage for this would be.
What's secure about it? How are applications prevented from changing the data on it? |
This is exactly the comparison that was made, when mcuboot uses the
If you would like a comparison for flash or bbram, no problem. Which one would you like to be compared ?
Secure like providing multithreads/userthreads support. If a option read-only would be requested (this should be part of a review) it can be added easily (it would just remove the |
RAM, using your boot_info code which this whole PR is about
But it's not? 2 threads can call the same functions at the same time, there is no security/safety in this code. And not only that but this code uses the whole parent device be it a block of RAM or the whole BBRAM device, there is no segmenting of data/access and no way to ensure different systems making use of it do not clash or even to know which system should use which addresses of the system. This is as far from secure as you can get |
This is exactly the comparison that is made. If you would like to try and reduce the size of
Two threads cannot access the same function at the same time, this is prevented by using a semaphore. Tests are carried out in userspace if supported to test any unauthorized access from threads. If segmenting of the data is required it can also be added, but first this needs to be required. Regarding clashing of data: the dts needs to correctly describe the |
a. maximize reuse of existing zephyr systems (replaced ram backend with flash backend + flash simulator) b. treat all backends equal c. provide possibility for multiple segments Signed-off-by: Laczen JMS <laczenjms@gmail.com>
I have updated the boot_info subsystem to further simplify. It now no longer has a direct RAM backend but instead uses the flash simulator to provide this functionality. Boot_info can now share info trough: bbram, eeprom, flash and ram (trough the flash simulator). To compare the The comparison has the following result: west build -p -b qemu_cortex_m3 tests/subsys/boot_info
Memory region Used Size Region Size %age Used
FLASH: 19857 B 256 KB 7.57%
RAM: 12432 B 63 KB 19.27%
FlashSim: 1 KB 1 KB 100.00%
IDT_LIST: 0 GB 2 KB 0.00%
west build -p -b qemu_cortex_m3 tests/subsys/retention
Memory region Used Size Region Size %age Used
FLASH: 22771 B 256 KB 8.69%
RAM: 12480 B 63 KB 19.35%
RetainedMem: 0 GB 1 KB 0.00%
IDT_LIST: 0 GB 2 KB 0.00% Or about 2.8kB in favor of boot_info. |
I have updated my PRs and posted patches for new tests which test my full mcuboot -> application data sharing system, the commit from #61583 is also required. mcuboot build with retention and additional features disabled for nrf52840dk_nrf52840:
with it enabled:
total size increase: +1202 bytes flash Application without changes (note: includes building with mcuboot support but no retention/shared data support):
Application with changes and including some printf output to show it is working:
total size increase: +984 bytes flash Output from running:
I therefore find the above post by laczen to be, as per the previous information posts he has provided, deceitful. |
For use of boot_info in user threads it is required to allow access to the device from this thread. To achieve this access to the backend device is needed. Signed-off-by: Laczen JMS <laczenjms@gmail.com>
Added retained memory as backend, at the moment userspace testing is not used. For userspace access from flash zephyrproject-rtos#61730 is required, for userspace access from retained_mem zephyrproject-rtos#61850 is required. Signed-off-by: Laczen JMS <laczenjms@gmail.com>
updated test to include a header and a crc for boot_info, this allows easy comparison with retention. Signed-off-by: Laczen JMS <laczenjms@gmail.com>
Proposal #62175 is more versatile and provides the same possibilities. |
boot_info is a simple subsystem to share information between a bootloader and an application. It can store the info in a bbram device or a zephyr,memory-region (RAM).
The proposed subsystem can easily be extended to support using a flash partition as backend.Flash partition backend has been added.
2023/07/20: Eeprom backend has been added.