Zephyr: Latest Zephyr HEAD results in a crash in mcuboot tree

[rgundi]

The latest changes to zephyr around the flash_map has broken mcubootloader (at least for intel_s1000_crb). Below are some of the commits where it has been changed.

69c15f0ed17e1c74e1288b5bf7f99aeef829afeb
04bf2e1bd1cd5deb706572083cdf9c5e8b249628
6199e978368e7a82e4feb8b830c50a2267926460

It builds fine but there's a run-time error in the function "flash_area_get_sectors".

[rgundi]

These changes were done by Findlay Feng. I couldn't find him to tag him here. @galak , can you please tag him if you know him?

[nvlsianpu]

rgundi - pleas elaborate more on the flash_area_get_sectors bug.

[nvlsianpu]

@rgundi ^^

[rgundi]

I haven't debugged it a great deal. When the latest mcubootloader tree is built with latest Zephyr and run, it results in an exception. I could figure out the exception is happening somewhere inside the function flash_area_layout which is called from flash_area_get_sectors. I believe it might be happening right after the call to flash_page_foreach function.

[rgundi]

Could figure out what the issue is. I am able to get mcuboot booting properly by modifying subsys/storage/flash_map/flash_map_default.c. I changed DT_FLASH_AREA_##i##_DEV to DT_JEDEC_SPI_NOR_0_LABEL and it is working for intel_s1000.

DT_FLASH_AREA_##i##_DEV is actually referring to SPI_0 but it should instead refer to MX25UM512 which is the SPI flash chip in intel_s1000. I think either scripts/dts/extract/flash.py or the DTS for intel_s1000 needs to be modified to achieve this.

@galak can you please pitch in?

[nashif]

@galak comments on above?

[SebastianBoe]

Me and @hakonfam have independently been affected by / reproduced this.

MCUBoot does not boot apps with the latest Zephyr.

Reproduced with nrf52840_pca10056.

[hakonfam]

Some more detail: The issue is that the order in which DT_FLASH_AREA_x_y is written to generated_dts_board_unfixed.h does not match what is expected by mcuboot.

E.g. mcuboot expects the second item to be correspond to IMAGE-0, but it seems to be rather random what order the different areas are placed.

[galak]

Some more detail: The issue is that the order in which DT_FLASH_AREA_x_y is written to generated_dts_board_unfixed.h does not match what is expected by mcuboot.

E.g. mcuboot expects the second item to be correspond to IMAGE-0, but it seems to be rather random what order the different areas are placed.

Why does the order matter to mcuboot, isn't that the whole point of the defines and flash_map?

[galak]

It feels like there are multiple issues/bug's here. on intel_s1000 the issue (or first issue) is related to how DT_FLASH_AREA_##i##_DEV gets set. That I'm working on. Not sure what's going on for nrf52840_pca10056.

[nashif]

I actually was able to boot on nrf52840_pca10056 with mcuboot, I guess this is a different issue indeed.

[nashif]

i confirm the issue on intel_s1000_crb

[galak]

i confirm the issue on intel_s1000_crb

Can you confirm the fix? in PR #13367.

[hakonfam]

My guess is that the bug is hard to reproduce since the use of an on-ordered python dict makes the order it occurs in random.

MCUboot uses FLASH_AREA_IMAGE_0 to point in to the flash_map list, this has the value 1.
So MCUboot needs flash_map[1] to point to the image_0 partition.

```
51 /*
52 * This depends on the mappings defined in sysflash.h, and assumes
53 * that slot 0, slot 1, and the scratch areas are contiguous.
54 */
55 int flash_area_id_from_image_slot(int slot)
56 {
57 return slot + FLASH_AREA_IMAGE_0;
58 }
````

When you look at flash_map_default.c you see that the order in which the partitions is placed is dictated by their placement in generated_dts_board_unfixed.h.

If IMAGE_0 is not stored in DT_FLASH_AREA_1_... in generated_dts_board_unfixed.h, MCUboot will load the wrong data from flash_map.

[galak]

My guess is that the bug is hard to reproduce since the use of an on-ordered python dict makes the order it occurs in random.

MCUboot uses FLASH_AREA_IMAGE_0 to point in to the flash_map list, this has the value 1.
So MCUboot needs flash_map[1] to point to the image_0 partition.

51 /*
52  * This depends on the mappings defined in sysflash.h, and assumes
53  * that slot 0, slot 1, and the scratch areas are contiguous.
54  */
55 int flash_area_id_from_image_slot(int slot)
56 {
57     return slot + FLASH_AREA_IMAGE_0;
58 }

When you look at flash_map_default.c you see that the order in which the partitions is placed is dictated by their placement in generated_dts_board_unfixed.h.

If IMAGE_0 is not stored in DT_FLASH_AREA_1_... in generated_dts_board_unfixed.h, MCUboot will load the wrong data from flash_map.

Sounds like a bug in mcuboot.

[hakonfam]

The issue is in flash_map_default.c IMO, if one just points to the DT_ symbols directly there is no issue.
I guess this file belongs to zephyr but is maintained by mcuboot?

Edit: But what is the use of a define which changes value depending on the order python decides to place them in? I'm thinking of the DT_FLASH_AREA_x_... variables.

[galak]

The DT_FLASH_AREA_ defines are purely to populate the FLASH_MAP, so one should be using the flash_map API not the defines directly.

[hakonfam]

Note that it is the flash_map which is built incorrectly, so the current API towards it becomes useless.

This because the order in which the items occurs in flash_map is random.

Edit: I am talking about the default_flash_map struct in flash_map_default.c

[SebastianBoe]

This might clarify things.

Have a look at this extract from _unfixed.h:

#define DT_FLASH_AREA_0_LABEL           IMAGE_1
#define DT_FLASH_AREA_0_OFFSET_0        479232
#define DT_FLASH_AREA_1_LABEL           MCUBOOT
#define DT_FLASH_AREA_1_OFFSET_0        0
#define DT_FLASH_AREA_2_LABEL           IMAGE_0
#define DT_FLASH_AREA_2_OFFSET_0        49152

from this extract one can see that IMAGE_1, MCUBOOT, and IMAGE_0, have flash area id 0, 1, and 2 respectively.

This is a problem because of this function from mcuboot:

/*

• This depends on the mappings defined in sysflash.h, that slot 0,
• slot 1, and the scratch areas are contiguous.
  */
  int flash_area_id_from_image_slot(int slot)
  {
  return slot + FLASH_AREA_IMAGE_0;
  }

this function believes that slot 1 has an id 1 higher than slot 0, but this is not so, as can be seen in the above _unfixed.h.

The question is, how does one implement flash_area_id_from_image_slot, when image 1 does not necessarily come after image 0 in the flash_map.

Presumably, the id's assigned are random, which would explain why I only see the problem intermittenly, and why Anas was not able to reproduce.

[galak]

Note that it is the flash_map which is built incorrectly, so the current API towards it becomes useless.

This because the order in which the items occurs in flash_map is random.

Edit: I am talking about the default_flash_map struct in flash_map_default.c

While the order is random, there are a set of defines that give you the symbolic flash partition names which is what should be used.

[galak]

This might clarify things.

Have a look at this extract from _unfixed.h:

#define DT_FLASH_AREA_0_LABEL         IMAGE_1
#define DT_FLASH_AREA_0_OFFSET_0      479232
#define DT_FLASH_AREA_1_LABEL         MCUBOOT
#define DT_FLASH_AREA_1_OFFSET_0      0
#define DT_FLASH_AREA_2_LABEL         IMAGE_0
#define DT_FLASH_AREA_2_OFFSET_0      49152

from this extract one can see that IMAGE_1, MCUBOOT, and IMAGE_0, have flash area id 0, 1, and 2 respectively.

This is a problem because of this function from mcuboot:

/*

• This depends on the mappings defined in sysflash.h, that slot 0,
• slot 1, and the scratch areas are contiguous.
  */
  int flash_area_id_from_image_slot(int slot)
  {
  return slot + FLASH_AREA_IMAGE_0;
  }

this function believes that slot 1 has an id 1 higher than slot 0, but this is not so, as can be seen in the above _unfixed.h.

The question is, how does one implement flash_area_id_from_image_slot, when image 1 does not necessarily come after image 0 in the flash_map.

Presumably, the id's assigned are random, which would explain why I only see the problem intermittenly, and why Anas was not able to reproduce.

Can it be:

int flash_area_id_from_image_slot(int slot) {

switch(slot):
    case 0:
        return DT_FLASH_AREA_IMAGE_0_ID;
    case 1:
        return DT_FLASH_AREA_IMAGE_1_ID;

[hakonfam]

No, because you don't know what slot the partitions are placed in.

Edit: My bad, if you use the ID to point in and get the correct defines, then you are correct.

[galak]

So I think it best to not depend on the order and use symbolic names. Since using strings and compares to partition labels seems heavy, I think the proposed solution is the direction we should head.

From a DTS point of view the order of the partition entries in the DTS should not be assumed.

[SebastianBoe]

Jesus, I don't know how this happened, but I was accidentally using an old revision of mcuboot.

The latest revision of MCUBoot has the same fix as we have found.

https://github.com/JuulLabs-OSS/mcuboot/commit/419a47531bcd940c8a34437129443e9828fb2b01

[galak]

Jesus, I don't know how this happened, but I was accidentally using an old revision of mcuboot.

The latest revision of MCUBoot has the same fix as we have found.

JuulLabs-OSS/mcuboot@419a475

Just to be clear, using latest mcuboot works on nrf52840_pca10056 for you reliable?

[SebastianBoe]

Correct.

[galak]

Closing this as I assume with the merging of #13367 and using the recent/up to date mcuboot things are working on all platforms. Please re-open if not.

[chtavares]

Hello, I use the last mcuboot and I can't change my flash using frdm k64f board. When I rebooted happens that error bellow:

[00:00:00.004,948] <inf> mcuboot: Starting bootloader
[00:00:00.010,967] <inf> mcuboot: Image 0: magic=good, copy_done=0x3, image_ok=0x1
[00:00:00.018,793] <inf> mcuboot: Scratch: magic=unset, copy_done=0x5b, image_ok=0x3
[00:00:00.016,952] <inf> mcuboot: Boot source: slot 0
[00:00:00.014,445] <inf> mcuboot: Swap type: test
***** BUS FAULT *****
  Imprecise data bus error
***** Hardware exception *****
Current thread ID = 0x00000000
Faulting instruction address = 0x3ebe

I don't know why it happens.
So, I used the git bisect for to find where started the problem, and the commit is:

commit dc0713a706e7e2898c172c6c0965b1b784859841
Author: Andy Ross <[email protected]>
Date:   Thu Jul 5 12:50:41 2018 -0700

    kernel: Cleanup. Remove redundant test when calling _Swap()

    _Swap() must already handle the case where _get_next_ready_thread() is
    the same as _current.

    Signed-off-by: Andy Ross <[email protected]>

Unfortunately, I don't know how to solve it.

[galak]

@chtavares592 do you mind opening a new issue for this.

[chtavares]

Sorry, I will open now. Thanks