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

Enforce sector alignmnent for managed bootloader builds #5818

Merged
merged 10 commits into from Jan 30, 2018

Conversation

Projects
None yet
6 participants
@sarahmarshy
Contributor

sarahmarshy commented Jan 9, 2018

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

@cmonr cmonr added the needs: review label Jan 9, 2018

@0xc0170 0xc0170 requested a review from theotherjimmy Jan 10, 2018

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 11, 2018

Contributor

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 22, 2018

Contributor

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

This comment has been minimized.

@sarahmarshy

sarahmarshy Jan 22, 2018

Contributor

excepts Exception in new commit.

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 11, 2018

Contributor

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

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 11, 2018

Contributor

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?

This comment has been minimized.

@sarahmarshy

sarahmarshy Jan 11, 2018

Contributor

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 11, 2018

Contributor

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

@@ -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):

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 11, 2018

Contributor

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

This comment has been minimized.

@sarahmarshy

sarahmarshy Jan 11, 2018

Contributor

Good call.

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):

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 11, 2018

Contributor

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 11, 2018

Contributor

Looks like this is actually floor/round_down?

This comment has been minimized.

@sarahmarshy

sarahmarshy Jan 11, 2018

Contributor

Yes. It is.

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 11, 2018

Contributor

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.

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 11, 2018

Contributor

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

This comment has been minimized.

@sarahmarshy

sarahmarshy Jan 11, 2018

Contributor

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.

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):

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 11, 2018

Contributor

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

This comment has been minimized.

@sarahmarshy

sarahmarshy Jan 11, 2018

Contributor

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.

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 11, 2018

Contributor

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

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 comment has been minimized.

@theotherjimmy

theotherjimmy Jan 11, 2018

Contributor

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 11, 2018

Contributor

Rounding down might truncate the bootloader.

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 comment has been minimized.

@theotherjimmy

theotherjimmy Jan 11, 2018

Contributor

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

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 11, 2018

Contributor

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

This comment has been minimized.

@sarahmarshy

sarahmarshy Jan 11, 2018

Contributor

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

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 11, 2018

Contributor

This should probably round up.

@sarahmarshy sarahmarshy force-pushed the sarahmarshy:bootloader-align-sectors branch 3 times, most recently from 1386549 to ab137d8 Jan 11, 2018

@sarahmarshy

This comment has been minimized.

Contributor

sarahmarshy commented Jan 19, 2018

@theotherjimmy, can you review again?

rom_size = int(cmsis_part['memory']['PROGRAM_FLASH']['size'], 0)
rom_start = int(cmsis_part['memory']['PROGRAM_FLASH']['start'], 0)
except KeyError:
raise ConfigException("Not enough information in CMSIS packs to "
"build a bootloader project")

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 22, 2018

Contributor

Could you re-align this line?

"build a bootloader project")
if ('target.bootloader_img' in target_overrides or
'target.restrict_size' in target_overrides):
return self._generate_booloader_build(target_overrides,
return self._generate_bootloader_build(target_overrides,
rom_start, rom_size)

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 22, 2018

Contributor

Could you re-align this line?

@@ -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

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 22, 2018

Contributor

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.

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 22, 2018

Contributor

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.

This comment has been minimized.

@sarahmarshy

sarahmarshy Jan 22, 2018

Contributor

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

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 22, 2018

Contributor

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

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 24, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jan 24, 2018

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

This comment has been minimized.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jan 24, 2018

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 24, 2018

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 24, 2018

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 25, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jan 25, 2018

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

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 26, 2018

/morph export-build

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 26, 2018

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

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

@mbed-ci

This comment has been minimized.

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Jan 26, 2018

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

This comment has been minimized.

Contributor

cmonr commented Jan 26, 2018

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

sarahmarshy added some commits Jan 9, 2018

Move FLM parsing to arm_pack_manager and regenerate index
Modify arm_pack_manager to look for new keys in pdsc files, because some
vendors have changed their format
Align managed bootloader addresses on sector boundaries
Aligns application start address to sector boundary
Aligns application end address to sector boundary
Introduce sector ceiling/floor rounding
Use ceiling for bootloader end address
Use floor for application size

@sarahmarshy sarahmarshy force-pushed the sarahmarshy:bootloader-align-sectors branch from 5e2a321 to 695dc00 Jan 26, 2018

@sarahmarshy

This comment has been minimized.

Contributor

sarahmarshy commented Jan 26, 2018

@cmonr rebased.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 26, 2018

/morph build

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 26, 2018

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 26, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jan 26, 2018

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

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 29, 2018

/morph test

@mbed-ci

This comment has been minimized.

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

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

19 checks passed

ARM mbed CI Verification build successful.
Details
AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Local events testing has passed
Details
travis-ci/littlefs Local littlefs testing has passed
Details
travis-ci/mbed2-ATMEL Local mbed2-ATMEL testing has passed
Details
travis-ci/mbed2-MAXIM Local mbed2-MAXIM testing has passed
Details
travis-ci/mbed2-NORDIC Local mbed2-NORDIC testing has passed
Details
travis-ci/mbed2-NUVOTON Local mbed2-NUVOTON testing has passed
Details
travis-ci/mbed2-NXP Local mbed2-NXP testing has passed
Details
travis-ci/mbed2-RENESAS Local mbed2-RENESAS testing has passed
Details
travis-ci/mbed2-SILICON_LABS Local mbed2-SILICON_LABS testing has passed
Details
travis-ci/mbed2-STM Local mbed2-STM testing has passed
Details
travis-ci/tools Local tools testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment