zephyr: arm: Update reading the flash image reset vector #1661

mmahadevan108 · 2023-03-24T17:48:50Z

This change uses the flash functions to read the applications reset vector. This allows flexibility on where the application image is programmed.
The current implementation in Zephyr for ARM does not allow the use-case where the application is programmed to a different flash device than where mcuboot is programmed and running from.

This was tested on NXP ARM platforms for the case where the Zephyr and mcuboot are in the same flash device and for the case where Zephyr and mcuboot are on different flash devices.

mmahadevan108 · 2023-03-24T17:50:01Z

cc @d3zd3z

nordicjm · 2023-03-27T12:33:08Z

Seems like a duplicate of #1569

dleach02 · 2023-03-27T16:09:49Z

@nordicjm, it looks like the #1569 is addressing a larger issue and has been in draft mode since January. This particular PR is fairly focused on a single problem we encountered as described in the initial text above. Do you think it would be easier to get this one approved and merged before your referenced PR?

nordicjm · 2023-03-28T06:40:56Z

@de-nordic

nordicjm · 2023-03-30T11:18:12Z

boot/zephyr/main.c


    /* Jump to flash image */
-    rc = flash_device_base(rsp->br_flash_dev_id, &flash_base);
+    area_id = flash_area_id_from_image_slot(0);


Looks like this breaks XIP

Yes it does, because it hardcodes vt to primary slot.

I am unclear how this breaks XIP.

XIP can boot from either slot, you are hardcoding this to slot 0.

Is'nt that what we currently do i.e boot with respect to flash_base.

That is not related to rsp->br_image_off

@nordicjm I am not understanding the above comment.

The current implementation will always read the Zephyr binary with address CONFIG_FLASH_BASE_ADDRESS as the starting point.
This PR adds more flexibility on the starting address where to look for Zephyr binary.

This PR adds more flexibility on the starting address where to look for Zephyr binary.

No, it completely breaks XIP, that is not more flexible. I've pointed this out many times now, you need to follow the code. Old code:

rsp->br_image_off = boot_img_slot_off(state, active_slot); rc = flash_device_base(rsp->br_flash_dev_id, &flash_base); vt = (struct arm_vector_table *)(flash_base + rsp->br_image_off + rsp->br_hdr->ih_hdr_size);

So yes, the base flash address which I never ever mentioned is the same, the image offset which would point to the base of the selected image is now completely absent from this code:

area_id = flash_area_id_from_image_slot(0); rc = flash_area_read(fap, rsp->br_hdr->ih_hdr_size, dst, 8); vt = (struct arm_vector_table *)dst;

@nordicjm @de-nordic @d3zd3z I have pushed an update to the implementation after understanding the XIP issues that were highlighted by the prior implementation.

@nordicjm @de-nordic @d3zd3z can you help re-review.

nordicjm

Breaks XIP

de-nordic

#1661 (review)

d3zd3z · 2023-12-14T15:20:02Z

What is the status here? Is this still needed?

mmahadevan108 · 2024-02-09T14:24:20Z

@nordicjm @de-nordic @d3zd3z , can you help review this PR.

mmahadevan108 · 2024-02-09T14:24:36Z

cc @danieldegrasse

d3zd3z · 2024-03-15T19:30:13Z

It is still unclear what problem this is solving. The end of this function jumps to an address in flash, so it only makes sense when there is XIP in flash. Is it just finding the wrong address? Could that be fixed a little easier?

danieldegrasse · 2024-03-18T19:30:10Z

It is still unclear what problem this is solving. The end of this function jumps to an address in flash, so it only makes sense when there is XIP in flash. Is it just finding the wrong address? Could that be fixed a little easier?

My understanding of the issue is as follows: say you have a system where MCUBoot will boot from internal flash (memory mapped to 0x1000_0000), and then you flash your application image to external flash (memory mapped to 0x1800_0000). The system can XIP from this external flash, but MCUBoot still needs to jump to the program running there.

In the current implementation of MCUBoot for ARM, the function flash_device_base will simply return CONFIG_FLASH_BASE_ADDRESS, which will be incorrect for the case above. We need MCUBoot to instead read the flash vector table from the external flash device at 0x1800_0000. The idea of the code here is to read the flash vector table from the offset it will be at within the image slot we intend to boot from. By using the flash_area interfaces here, we can generically read the flash vector table from the image slot, and we will jump to an offset from 0x1800_000 (in the external flash memory) instead of booting from internal flash memory.

nordicjm · 2024-03-19T06:57:01Z

It is still unclear what problem this is solving. The end of this function jumps to an address in flash, so it only makes sense when there is XIP in flash. Is it just finding the wrong address? Could that be fixed a little easier?

My understanding of the issue is as follows: say you have a system where MCUBoot will boot from internal flash (memory mapped to 0x1000_0000), and then you flash your application image to external flash (memory mapped to 0x1800_0000). The system can XIP from this external flash, but MCUBoot still needs to jump to the program running there.

In the current implementation of MCUBoot for ARM, the function flash_device_base will simply return CONFIG_FLASH_BASE_ADDRESS, which will be incorrect for the case above. We need MCUBoot to instead read the flash vector table from the external flash device at 0x1800_0000. The idea of the code here is to read the flash vector table from the offset it will be at within the image slot we intend to boot from. By using the flash_area interfaces here, we can generically read the flash vector table from the image slot, and we will jump to an offset from 0x1800_000 (in the external flash memory) instead of booting from internal flash memory.

So then how about fixing flash_device_base in boot/zephyr/flash_map_extended.c so it returns the correct base address for a given flash area?

mmahadevan108 · 2024-03-19T22:20:36Z

It is still unclear what problem this is solving. The end of this function jumps to an address in flash, so it only makes sense when there is XIP in flash. Is it just finding the wrong address? Could that be fixed a little easier?

My understanding of the issue is as follows: say you have a system where MCUBoot will boot from internal flash (memory mapped to 0x1000_0000), and then you flash your application image to external flash (memory mapped to 0x1800_0000). The system can XIP from this external flash, but MCUBoot still needs to jump to the program running there.
In the current implementation of MCUBoot for ARM, the function flash_device_base will simply return CONFIG_FLASH_BASE_ADDRESS, which will be incorrect for the case above. We need MCUBoot to instead read the flash vector table from the external flash device at 0x1800_0000. The idea of the code here is to read the flash vector table from the offset it will be at within the image slot we intend to boot from. By using the flash_area interfaces here, we can generically read the flash vector table from the image slot, and we will jump to an offset from 0x1800_000 (in the external flash memory) instead of booting from internal flash memory.

So then how about fixing flash_device_base in boot/zephyr/flash_map_extended.c so it returns the correct base address for a given flash area?

I don't see an API in Zephyr where we could get this information from a device. Do you suggest I propose a change to Zephyr flash API to get this information?

nordicjm · 2024-03-20T07:34:01Z

It is still unclear what problem this is solving. The end of this function jumps to an address in flash, so it only makes sense when there is XIP in flash. Is it just finding the wrong address? Could that be fixed a little easier?

My understanding of the issue is as follows: say you have a system where MCUBoot will boot from internal flash (memory mapped to 0x1000_0000), and then you flash your application image to external flash (memory mapped to 0x1800_0000). The system can XIP from this external flash, but MCUBoot still needs to jump to the program running there.
In the current implementation of MCUBoot for ARM, the function flash_device_base will simply return CONFIG_FLASH_BASE_ADDRESS, which will be incorrect for the case above. We need MCUBoot to instead read the flash vector table from the external flash device at 0x1800_0000. The idea of the code here is to read the flash vector table from the offset it will be at within the image slot we intend to boot from. By using the flash_area interfaces here, we can generically read the flash vector table from the image slot, and we will jump to an offset from 0x1800_000 (in the external flash memory) instead of booting from internal flash memory.

So then how about fixing flash_device_base in boot/zephyr/flash_map_extended.c so it returns the correct base address for a given flash area?

I don't see an API in Zephyr where we could get this information from a device. Do you suggest I propose a change to Zephyr flash API to get this information?

const struct flash_area *fa;
rc = flash_area_open(area_id, &fa);
fa->fa_off ??

danieldegrasse · 2024-03-20T14:51:17Z

So then how about fixing flash_device_base in boot/zephyr/flash_map_extended.c so it returns the correct base address for a given flash area?

@mmahadevan108 Given that we do have an API to do this, I think this fix makes the most sense

mmahadevan108 · 2024-03-20T15:19:31Z

It is still unclear what problem this is solving. The end of this function jumps to an address in flash, so it only makes sense when there is XIP in flash. Is it just finding the wrong address? Could that be fixed a little easier?

My understanding of the issue is as follows: say you have a system where MCUBoot will boot from internal flash (memory mapped to 0x1000_0000), and then you flash your application image to external flash (memory mapped to 0x1800_0000). The system can XIP from this external flash, but MCUBoot still needs to jump to the program running there.
In the current implementation of MCUBoot for ARM, the function flash_device_base will simply return CONFIG_FLASH_BASE_ADDRESS, which will be incorrect for the case above. We need MCUBoot to instead read the flash vector table from the external flash device at 0x1800_0000. The idea of the code here is to read the flash vector table from the offset it will be at within the image slot we intend to boot from. By using the flash_area interfaces here, we can generically read the flash vector table from the image slot, and we will jump to an offset from 0x1800_000 (in the external flash memory) instead of booting from internal flash memory.

So then how about fixing flash_device_base in boot/zephyr/flash_map_extended.c so it returns the correct base address for a given flash area?

I don't see an API in Zephyr where we could get this information from a device. Do you suggest I propose a change to Zephyr flash API to get this information?
const struct flash_area *fa;
rc = flash_area_open(area_id, &fa);
fa->fa_off ??

flash_area_open is what this PR is using. However fa->fa_off will give us the offset in a given partition, this does not get us the base address of the flash device.
In this PR, once the area is open we read from the appropriate location using flash_area_read

danieldegrasse · 2024-03-20T16:39:41Z

flash_area_open is what this PR is using. However fa->fa_off will give us the offset in a given partition, this does not get us the base address of the flash device.
In this PR, cnce the area is open we read from the appropriate location using flash_area_read

Sure- I understand the reasoning behind the current approach. However couldn't we also simply update the way flash_device_base works to return the base address of the flash device the application image is present on? Then flash_base + rsp->br_image_off + rsp->br_hdr->ih_hdr_size would get the correct pointer into flash memory.

mmahadevan108 · 2024-03-20T20:57:36Z

flash_area_open is what this PR is using. However fa->fa_off will give us the offset in a given partition, this does not get us the base address of the flash device.
In this PR, cnce the area is open we read from the appropriate location using flash_area_read

Sure- I understand the reasoning behind the current approach. However couldn't we also simply update the way flash_device_base works to return the base address of the flash device the application image is present on? Then flash_base + rsp->br_image_off + rsp->br_hdr->ih_hdr_size would get the correct pointer into flash memory.

The issue is we do not have a flash_base we can get for the Flash device if image partition is on a different flash compared to MCUboot.

nordicjm · 2024-03-21T07:38:11Z

flash_area_open is what this PR is using. However fa->fa_off will give us the offset in a given partition, this does not get us the base address of the flash device.
In this PR, cnce the area is open we read from the appropriate location using flash_area_read

Sure- I understand the reasoning behind the current approach. However couldn't we also simply update the way flash_device_base works to return the base address of the flash device the application image is present on? Then flash_base + rsp->br_image_off + rsp->br_hdr->ih_hdr_size would get the correct pointer into flash memory.

The issue is we do not have a flash_base we can get for the Flash device if image partition is on a different flash compared to MCUboot.

The dts has the reg address, use that?

danieldegrasse · 2024-03-21T14:24:03Z

The dts has the reg address, use that?

This is a good solution for internal flash devices, but it is not always that easy for external ones. To give a concrete example, NXP's FlexSPI peripheral supports memory mapped access to external flash parts. Multiple flash chips can be present on one FlexSPI, and their contents will be placed sequentially in the memory map (based on the chip select index for each flash chip). However, the "reg" property of these flash parts will be set to their chip select value, since the FlexSPI is considered a SPI device, and in that context the "reg" property means SPI chip select index. In order to really effectively support reading the flash base address for these parts, I think we'd need to add to the flash API.

Personally, I'm convinced @mmahadevan108's solution makes sense here. Reading the stack pointer and the reset vector via the flash API should be reliable for XIP from the same flash chip or another device, based on my understanding of how the flash area API works. Do you have a specific concern with implementing the boot process this way, versus extracting the flash base address?

mmahadevan108 · 2024-04-01T15:38:57Z

@d3zd3z @nordicjm @de-nordic, can you help revisit this PR.

mmahadevan108 · 2024-04-10T15:24:39Z

@d3zd3z @nordicjm @de-nordic, any comments?

nordicjm · 2024-04-11T06:34:56Z

The flash partition's reg address should point to the correct memory mapped address if it's being used in memory mapped mode, and that address should be absolute

mmahadevan108 · 2024-04-11T20:35:27Z

We cannot use the zephyr.flash node as the Zephyr application we are loading is on a different flash device. The other way of getting the reg address is to add NXP/Flash specific defines in the boot/zephyr/flash_map_extended.c file which is not ideal and something we would like to avoid.

nordicjm · 2024-04-12T06:39:44Z

You have a partitions part, right? That should contain the memory mapped address of the device if it's being used in memory mapped mode.

mmahadevan108 · 2024-04-12T15:14:03Z

Here is the partitions I am using in my overlay file. As shared earlier, we cannot get the address of the Flash device from this. The only thing available is the offset within the flash device. What is also available is the Flash Device node, we are using this to access the Flash device via the Flash API's

/delete-node/ &slot0_partition;
/delete-node/ &slot1_partition;
/delete-node/ &scratch_partition;

&w25q64jvssiq {
        partitions {
                compatible = "fixed-partitions";
                #address-cells = <1>;
                #size-cells = <1>;

                slot0_partition: partition@0 {
                        label = "image-0";
                        reg = <0x0000000 DT_SIZE_K(960)>;
                };
                slot1_partition: partition@f0000 {
                        label = "image-1";
                        reg = <0x000f0000 DT_SIZE_K(960)>;
                };
                scratch_partition: partition@1e0000 {
                        label = "image-scratch";
                        reg = <0x001e0000 DT_SIZE_K(128)>;
                };
        };
};

nordicjm · 2024-04-15T06:51:47Z

So you are using the external flash from MCUboot in SPI mode then somehow switching to memory mapped mode to execute the slot itself? Where is that code?

de-nordic · 2024-04-16T13:14:08Z

boot/zephyr/main.c

-    vt = (struct arm_vector_table *)(flash_base +
-                                     rsp->br_image_off +
-                                     rsp->br_hdr->ih_hdr_size);
+    rc = flash_area_read(fap, rsp->br_hdr->ih_hdr_size, dst, 8);


Th read size should not be hardcoded here, should be picked from the dst.

I have made this change.

boot/zephyr/main.c

This change uses the flash functions to read the applications reset vector. This allow flexibility on which flash device the application is programmed. For e.g: MCUBoot can be programmed and running from Internal Flash while Zephyr can be loaded from a different Flash device. This change is made for ARM platform, it can be extended to non-ARM platforms as well. Signed-off-by: Mahesh Mahadevan <mahesh.mahadevan@nxp.com>

nordicjm

Tested swap using move and direct XIP on nrf52840dk, both working

mmahadevan108 · 2024-04-19T16:03:08Z

@de-nordic can you help re-review.

mmahadevan108 · 2024-04-22T14:51:36Z

@d3zd3z, can this PR be merged?

d3zd3z · 2024-04-23T16:31:48Z

I think this looks good to make it into the next release. Please remember to add release notes. I'll go ahead and merge this, and put the release notes in an update for the release.

de-nordic requested review from de-nordic, d3zd3z, nvlsianpu and nordicjm March 29, 2023 14:19

nordicjm reviewed Mar 30, 2023

View reviewed changes

nordicjm requested changes Apr 6, 2023

View reviewed changes

de-nordic requested changes Aug 7, 2023

View reviewed changes

mmahadevan108 force-pushed the Fix_Flash_Boot branch from 9c42d03 to 715bc06 Compare February 7, 2024 03:11

mmahadevan108 requested review from nordicjm and de-nordic February 7, 2024 15:28

de-nordic reviewed Apr 16, 2024

View reviewed changes

nordicjm reviewed Apr 16, 2024

View reviewed changes

boot/zephyr/main.c Show resolved Hide resolved

mmahadevan108 force-pushed the Fix_Flash_Boot branch from 715bc06 to 92fceb4 Compare April 17, 2024 16:25

nordicjm requested review from nordicjm and de-nordic April 18, 2024 07:07

nordicjm reviewed Apr 18, 2024

View reviewed changes

boot/zephyr/main.c Outdated Show resolved Hide resolved

mmahadevan108 force-pushed the Fix_Flash_Boot branch from 92fceb4 to ea306df Compare April 18, 2024 14:52

nordicjm approved these changes Apr 19, 2024

View reviewed changes

de-nordic approved these changes Apr 19, 2024

View reviewed changes

d3zd3z merged commit 453096b into mcu-tools:main Apr 23, 2024
55 checks passed

mmahadevan108 deleted the Fix_Flash_Boot branch April 23, 2024 16:49

zephyr: arm: Update reading the flash image reset vector #1661

zephyr: arm: Update reading the flash image reset vector #1661

Conversation

mmahadevan108 commented Mar 24, 2023 • edited Loading

mmahadevan108 commented Mar 24, 2023

nordicjm commented Mar 27, 2023

dleach02 commented Mar 27, 2023

nordicjm commented Mar 28, 2023

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 left a comment

Choose a reason for hiding this comment

de-nordic left a comment

Choose a reason for hiding this comment

d3zd3z commented Dec 14, 2023

mmahadevan108 commented Feb 9, 2024

mmahadevan108 commented Feb 9, 2024

d3zd3z commented Mar 15, 2024

danieldegrasse commented Mar 18, 2024

nordicjm commented Mar 19, 2024

mmahadevan108 commented Mar 19, 2024

nordicjm commented Mar 20, 2024

danieldegrasse commented Mar 20, 2024

mmahadevan108 commented Mar 20, 2024 • edited Loading

danieldegrasse commented Mar 20, 2024

mmahadevan108 commented Mar 20, 2024

nordicjm commented Mar 21, 2024

danieldegrasse commented Mar 21, 2024

mmahadevan108 commented Apr 1, 2024

mmahadevan108 commented Apr 10, 2024

nordicjm commented Apr 11, 2024

mmahadevan108 commented Apr 11, 2024 • edited Loading

nordicjm commented Apr 12, 2024

mmahadevan108 commented Apr 12, 2024 • edited Loading

nordicjm commented Apr 15, 2024

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

mmahadevan108 commented Apr 19, 2024

mmahadevan108 commented Apr 22, 2024

d3zd3z commented Apr 23, 2024

mmahadevan108 commented Mar 24, 2023 •

edited

Loading

mmahadevan108 commented Mar 20, 2024 •

edited

Loading

mmahadevan108 commented Apr 11, 2024 •

edited

Loading

mmahadevan108 commented Apr 12, 2024 •

edited

Loading