Enforce sector alignmnent for managed bootloader builds #5818

sarahmarshy · 2018-01-09T16:59:48Z

These changes will align application start and end addresses to sector boundaries when building a managed bootloader application.

Changes have been made to arm_pack_manager to store sector information in index.json. This required regenerating the index. The list of tuples for sector start, and size are stored in the sectors key for each target in index.json. With the release of PDSC version 1.4.0, changes were made to PDSC tags for memory, so this required additional modifications to arm_pack_manager to find ROM/RAM size from PDSC files. See XML format for memory in PDSC files here.

@theotherjimmy

theotherjimmy · 2018-01-09T17:18:39Z

tools/arm_pack_manager/__init__.py

+            algo_bin = algo_itr.next()
+            flm_file = algo_bin.read()
+            return PackFlashAlgo(flm_file).sector_sizes
+        except:


Can this exception be more specific? even except Exception would be better (this catches OOM and Control-C right now)

excepts Exception in new commit.

theotherjimmy · 2018-01-11T17:37:05Z

tools/config/__init__.py

+            target_size = size
+        if (target_size < 0):
+            raise ConfigException("No valid sector found")
+        sector_num = (address - target_start)//target_size


Could you add spaces around the // operator? I prefer that style.

theotherjimmy · 2018-01-11T17:37:21Z

tools/config/__init__.py

+        if (target_size < 0):
+            raise ConfigException("No valid sector found")
+        sector_num = (address - target_start)//target_size
+        return target_start + (sector_num*target_size)


I think this logic will not work well when the next sector after the current one is differently sized compared to current sector. This happens in ST parts for sure.

Could you add spaces around the * operator?

Could you give an example? I tried this on Odin, and it worked.

That statement assumed that the sector list was a list of all of the sectors.

theotherjimmy · 2018-01-11T17:37:57Z

tools/config/__init__.py

@@ -540,7 +556,7 @@ def regions(self):
            raise ConfigException(
                "Bootloader build requested but no bootlader configuration")

-    def _generate_booloader_build(self, target_overrides, rom_start, rom_size):
+    def _generate_bootloader_build(self, target_overrides, rom_start, rom_size, sectors):


👍 For the typo correction. Do we have to pass sectors in? Could this function also do self.sectors?

theotherjimmy · 2018-01-11T17:39:47Z

tools/config/__init__.py

            yield Region("application", rom_start + start, rom_size - start,
                         True, None)
        if start > rom_size:
            raise ConfigException("Not enough memory on device to fit all "
                                  "application regions")
+    @staticmethod
+    def _align_on_sector(address, sectors):


Could this be named _sector_round_up or _sector_ceiling? This way I know which direction it rounds the address passed in.

Looks like this is actually floor/round_down?

Yes. It is.

Then I don't think the code using it is correct. You cannot round down the end of the bootloader, that must be rounded up.

theotherjimmy · 2018-01-11T17:44:26Z

tools/config/__init__.py

+        target_start = -1
+        for (start, size) in sectors:
+            if address < start:
+                break


If this just rounds up, could it return start here?

No. The list of sectors is not a list of all possible sectors. It is a list of tuples with (start of block, size of sectors in that block). Maybe I should have made that more clear.

theotherjimmy · 2018-01-11T18:07:41Z

tools/config/__init__.py

            yield Region("application", rom_start + start, rom_size - start,
                         True, None)
        if start > rom_size:
            raise ConfigException("Not enough memory on device to fit all "
                                  "application regions")
+    @staticmethod
+    def _align_on_sector(address, sectors):


I found an odd corner case where this function reports an incorrect address:

_align_on_sector(address = 1, sectors = [(0, 1)]) == 1

it should raise ConfigException

No, it shouldn't. Sectors = (0, 1) implies the start address for this block is 0 and there are size 1 sectors from 0 to the end of ROM. So an address of 1 is already sector aligned.

Ah, so the sectors list is formatted a little differently then.

theotherjimmy · 2018-01-11T19:48:08Z

tools/config/__init__.py

-            yield Region("bootloader", rom_start + start, part_size, False,
+            start = Config._align_on_sector(rom_start + start, sectors) - rom_start
+            offset = start + rom_start
+            part_size = Config._align_on_sector(offset + part_size, sectors) - offset


This rounds down the part size, correct? This needs to round up.

Rounding down might truncate the bootloader.

theotherjimmy · 2018-01-11T19:49:01Z

tools/config/__init__.py

                         filename)
            start += part_size
        if 'target.restrict_size' in target_overrides:
            new_size = int(target_overrides['target.restrict_size'], 0)
-            yield Region("application", rom_start + start, new_size, True, None)
+            start = Config._align_on_sector(rom_start + start, sectors) - rom_start


This also rounds down, correct? I think it should round up.

theotherjimmy · 2018-01-11T19:49:51Z

tools/config/__init__.py

            start += new_size
-            yield Region("post_application", rom_start +start, rom_size - start,
+            start = Config._align_on_sector(rom_start + start, sectors) - rom_start


I don't think this round up/down is needed. start should already be sector aligned (call 4 lines up)

Well, it has size added to it now, which might put the new address in a different block with different sector sizes.

theotherjimmy · 2018-01-11T19:50:21Z

tools/config/__init__.py

                         False, None)
        else:
+            start = Config._align_on_sector(rom_start + start, sectors) - rom_start


This should probably round up.

sarahmarshy · 2018-01-19T15:41:35Z

@theotherjimmy, can you review again?

tools/config/__init__.py

theotherjimmy · 2018-01-22T17:11:56Z

tools/config/__init__.py

@@ -540,7 +558,7 @@ def regions(self):
            raise ConfigException(
                "Bootloader build requested but no bootlader configuration")

-    def _generate_booloader_build(self, target_overrides, rom_start, rom_size):
+    def _generate_bootloader_build(self, target_overrides, rom_start, rom_size):
        start = 0


If you plan on changing this to be the address of the start of ROM, could you instead assign that here? Doing otherwise can be confusing.

theotherjimmy · 2018-01-22T17:13:33Z

tools/config/__init__.py

        if 'target.restrict_size' in target_overrides:
            new_size = int(target_overrides['target.restrict_size'], 0)
-            yield Region("application", rom_start + start, new_size, True, None)
+            new_size = Config._align_floor(start + new_size, self.sectors) - start


If bootloader_img is not present, then start will be 0, and incorrect if rom_start is not 0.

Alternatively, You are mixing types for start. It begins life as an offset, and changes to an address in the if 'target.bootloader_img' in target_overrides: branch. This leads to start being either an offset or address in this block, which assumes that start is an address.

@theotherjimmy, the latest commit should fix this. Good catch.

theotherjimmy · 2018-01-22T21:53:33Z

tools/arm_pack_manager/__init__.py

+            algo_bin = algo_itr.next()
+            flm_file = algo_bin.read()
+            return PackFlashAlgo(flm_file).sector_sizes
+        except:


Can this exception be more specific? even except Exception would be better (this catches OOM and Control-C right now)

0xc0170 · 2018-01-24T13:40:50Z

/morph build

mbed-ci · 2018-01-24T14:02:19Z

Build : SUCCESS

Build number : 938
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5818/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

mbed-ci · 2018-01-24T17:13:16Z

Exporter Build : FAILURE

Build number : 626
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/5818/

theotherjimmy · 2018-01-24T17:44:11Z

That export build looks like something went horribly wrong. @cmonr, @studavekar Are we currently experiencing a hiccup in the export CI?

cmonr · 2018-01-24T17:53:24Z

Well, we weren't until now... This failure mode is known and we've been monitoring it all week.

cmonr · 2018-01-24T17:57:35Z

Fortunately, it looks like this instance was isolated. Holding off on restarting the build until morph test completes.

cmonr · 2018-01-25T16:47:24Z

/morph build

mbed-ci · 2018-01-25T17:07:48Z

Build : SUCCESS

Build number : 956
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5818/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

mbed-ci · 2018-01-25T22:36:07Z

Exporter Build : FAILURE

Build number : 641
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/5818/

0xc0170 · 2018-01-26T10:35:39Z

/morph export-build

0xc0170 · 2018-01-26T10:39:06Z

@sarahmarshy Can you review the failure, looks related, and only for iar/uvision

mbed-ci · 2018-01-26T11:38:51Z

Exporter Build : FAILURE

Build number : 652
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/5818/

studavekar · 2018-01-26T14:36:46Z

The deployed tools on build node are fixed. This PR adds in a additional python module and will make changes to install requirements.txt into virtualenv.

cmonr · 2018-01-26T17:11:44Z

@sarahmarshy Could you rebase this, so we can rerun tests?

Modify arm_pack_manager to look for new keys in pdsc files, because some vendors have changed their format

Aligns application start address to sector boundary Aligns application end address to sector boundary

Use ceiling for bootloader end address Use floor for application size

Correct spacing.

sarahmarshy · 2018-01-26T19:24:56Z

@cmonr rebased.

cmonr · 2018-01-26T19:56:22Z

/morph build

cmonr · 2018-01-26T20:05:04Z

Holding off on build until CI machines are updated with needed python module.

cmonr · 2018-01-26T23:20:02Z

/morph build

mbed-ci · 2018-01-26T23:56:31Z

Build : SUCCESS

Build number : 980
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5818/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

mbed-ci · 2018-01-27T02:21:24Z

Exporter Build : SUCCESS

Build number : 674
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/5818/

0xc0170 · 2018-01-29T08:26:01Z

/morph test

mbed-ci · 2018-01-30T01:59:16Z

Test : SUCCESS

Build number : 815
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/5818/815

sarahmarshy mentioned this pull request Jan 9, 2018

Add new config option for SOTP #5732

Closed

cmonr added the needs: review label Jan 9, 2018

0xc0170 requested a review from theotherjimmy January 10, 2018 11:18

theotherjimmy suggested changes Jan 11, 2018

View reviewed changes

theotherjimmy reviewed Jan 11, 2018

View reviewed changes

sarahmarshy force-pushed the bootloader-align-sectors branch 3 times, most recently from 1386549 to ab137d8 Compare January 12, 2018 15:24

theotherjimmy suggested changes Jan 22, 2018

View reviewed changes

theotherjimmy approved these changes Jan 22, 2018

View reviewed changes

0xc0170 added needs: CI and removed needs: review labels Jan 24, 2018

0xc0170 added needs: work and removed needs: CI labels Jan 26, 2018

sarahmarshy and others added 10 commits January 26, 2018 13:24

Move FLM parsing to arm_pack_manager and regenerate index

5009fdb

Modify arm_pack_manager to look for new keys in pdsc files, because some vendors have changed their format

Add functions for getting sectors in Config class

a219508

Fix typo in _generate_bootloader_build

c74de99

Align managed bootloader addresses on sector boundaries

86d4ce1

Aligns application start address to sector boundary Aligns application end address to sector boundary

Add pyelftools dependency

41dac7a

Error check existence of sectors

6f39d5a

Use Config class sector variable

785dd10

Introduce sector ceiling/floor rounding

623b171

Use ceiling for bootloader end address Use floor for application size

Treat start variable as addrss in generate_bootloader_build

58330fb

Correct spacing.

Change exception in ArmPackManager FLM reader

695dc00

sarahmarshy force-pushed the bootloader-align-sectors branch from 5e2a321 to 695dc00 Compare January 26, 2018 19:24

cmonr added needs: CI and removed needs: work labels Jan 26, 2018

0xc0170 added ready for merge and removed needs: CI labels Jan 30, 2018

cmonr merged commit 78b65d0 into ARMmbed:master Jan 30, 2018

cmonr added release-version: 5.7.5 and removed ready for merge labels Jan 30, 2018

theotherjimmy mentioned this pull request Feb 2, 2018

[feature-nrf528xx] Switch to managed bootloader for SoftDevice merge on NRF52 #6006

Merged

Enforce sector alignmnent for managed bootloader builds #5818

Enforce sector alignmnent for managed bootloader builds #5818

Uh oh!

Conversation

sarahmarshy commented Jan 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarahmarshy commented Jan 19, 2018

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theotherjimmy Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Jan 24, 2018

Uh oh!

mbed-ci commented Jan 24, 2018

Triggering tests

Uh oh!

mbed-ci commented Jan 24, 2018

Uh oh!

theotherjimmy commented Jan 24, 2018

Uh oh!

cmonr commented Jan 24, 2018

Uh oh!

cmonr commented Jan 24, 2018

theotherjimmy Jan 22, 2018 •

edited

Loading

studavekar commented Jan 26, 2018 •

edited by cmonr

Loading