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

Override ROM/RAM start/size for TrustZone targets #7133

Merged
merged 7 commits into from Jun 21, 2018

Conversation

Projects
None yet
5 participants
@ccli8
Contributor

ccli8 commented Jun 6, 2018

Description

On TrustZone targets, ROM/RAM start/size are not so fixed and can be changed by external tool or other means. This PR introduces new target configuration options for user to override ROM/RAM start/size dependent on its real case.

  1. Introduce new target configuration options (mbed_rom_start/mbed_rom_size)
    1. Override rom_start/rom_size pulled from CMSIS pack or no CMSIS pack
    2. Pass overridden flash information (MBED_ROM_START/MBED_ROM_SIZE) to compiler
    3. Pass overridden flash information (MBED_ROM_START/MBED_ROM_SIZE) to linker
  2. Introduce new target configuration options (mbed_ram_start/mbed_ram_size)
    1. Override ram_start/ram_size pulled from CMSIS pack or no CMSIS pack
    2. Pass overridden SRAM information (MBED_RAM_START/MBED_RAM_SIZE) to compiler
    3. Pass overridden SRAM information (MBED_RAM_START/MBED_RAM_SIZE) to linker

Take our on-going NUMAKER_PFM_M2351 as an example. We have partitioned 512 KB flash/96KB SRAM to:

Secure flash: 64KB
Non-secure flash: 448KB
Secure SRAM: 8KB
Non-secure SRAM: 88KB

For build for NUMAKER_PFM_M2351 secure target, we must have mbed_app.json:

"target_overrides": {

    "NUMAKER_PFM_M2351_S": {
        
        "target.mbed_rom_start": "0x0",
        "target.mbed_rom_size": "0x10000",
        "target.mbed_ram_start": "0x20000000",
        "target.mbed_ram_size": "0x2000",
        
    }
},

For build for NUMAKER_PFM_M2351 non-secure target, we must have mbed_app.json:

"target_overrides": {

    "NUMAKER_PFM_M2351_NS": {
        
        "target.mbed_rom_start": "0x10010000",
        "target.mbed_rom_size": "0x70000",
        "target.mbed_ram_start": "0x30002000",
        "target.mbed_ram_size": "0x16000",
        
    }
},

Related PR

#6890

Pull request type

[ ] Fix
[ ] Refactor
[ ] New target
[X] Feature
[ ] Breaking change

ccli8 added some commits Jun 5, 2018

Fix flash start/size in bootloader related build
This fix relies on target configuration options (mbed_rom_start/mbed_rom_size) defined
to override CMSIS pack or no CMSIS pack.

This is useful for a target which:
1. Doesn't support CMSIS pack, or
2. Supports TrustZone and user needs to change its flash partition
Pass fixed flash information (MBED_ROM_START/MBED_ROM_SIZE) to compil…
…er/linker

This fix relies on target configuration options (mbed_rom_start/mbed_rom_size) defined.
Pass fixed SRAM information (MBED_RAM_START/MBED_RAM_SIZE) to compile…
…r/linker

This fix relies on target configuration options (mbed_ram_start/mbed_ram_size) defined.

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-tools Jun 6, 2018

@theotherjimmy

Please don't modify macros in the toolchains. If you need the MBED_ROM_START and other macros outside of boot loader mode, we should do that in config, is it's a configuration decision.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 6, 2018

@ccli8 I like this proposal. I take it that the macros are needed even when you're not building a bootloader?

@ccli8

This comment has been minimized.

Contributor

ccli8 commented Jun 7, 2018

the macros are needed even when you're not building a bootloader?

Yes. The MBED_ROM_START and like macros are not bound to bootloader.
Per my experience, they have the following uses:

  1. Override flash start/size for flash algorithm (flash_api.c)
    Still take NUMAKER_PFM_M2351 as an example:

    /* Non-secure flash */
    static const flash_target_config_t flash_target_config_ns = {
        .page_size  = 4,
    
    #if defined(MBED_ROM_START)
        .flash_start = MBED_ROM_START,
    #else
        .flash_start = 0x10040000,
    #endif
    
    #if defined(MBED_ROM_SIZE)
        .flash_size = MBED_ROM_SIZE,
    #else
        .flash_size = 0x40000,
    #endif
    
        .sectors = sectors_info_ns,
        .sector_info_count = sizeof(sectors_info_ns) / sizeof(sector_info_t)
    };
    
  2. Override flash start/size and SRAM start/size for linker script (*.sct, *.ld, *.icf)

Then about how to define the MBED_ROM_START and like macros. If we define them in config, we can just pass them to compiler and cannot pass to linker (need make_ld_define?). Actually, target.mbed_rom_start and MBED_ROM_START are for the same purpose. User would feel confused if it needs to define them multiple places. So I generate MBED_ROM_START from target.mbed_rom_start to centralize the configuration.

@theotherjimmy

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 7, 2018

@ccli8 My suggestion is to make this behave like a bootloader build. That way a user also gets visual verification. I can make that modification if you would like.

@ccli8

This comment has been minimized.

Contributor

ccli8 commented Jun 8, 2018

@theotherjimmy OK. Please modify it.

@ccli8

This comment has been minimized.

Contributor

ccli8 commented Jun 8, 2018

@theotherjimmy I fix the MBED_ROM_START macros don't pass to linker in ae16aa2.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 8, 2018

@ccli8 I upgraded the implementation to treat the ROM + RAM specifiers as part of the bootloader mode.

This should let you override them as you would any other configuration variable, and starts a generic "RAM reservation" framework using regions, much like the bootloader builds. @marcuschangarm RAM reservation.

@ccli8

This comment has been minimized.

Contributor

ccli8 commented Jun 11, 2018

@theotherjimmy The modification doesn't meet my requirement. Compiling mbed-cloud-clent-example on NUMAKER_PFM_M2351_NS target with mbed_app.json:

"NUMAKER_PFM_M2351_NS": {
        "target.mbed_rom_start":    "0x10010000",
        "target.mbed_rom_size":     "0x70000",
        "target.mbed_ram_start":    "0x30002000",
        "target.mbed_ram_size":     "0x16000",
   
        "target.mbed_app_start"             : "0x10024000",
        "target.mbed_app_size"              : "0x5C000",

Checking .profile-c/.profile-cxx, ROM start/size are overridden by mbed_app_start/mbed_app_size and don't correctly pass to compiler.

'-DAPPLICATION_ADDR=0x10024000', '-DAPPLICATION_SIZE=0x5c000', '-DAPPLICATION_RAM_ADDR=0x30002000', '-DAPPLICATION_RAM_SIZE=0x16000'

Checking .profile-ld, same as above, ROM start/size don't correctly pass to linker.

'--predefine="-DMBED_APP_START=0x10024000"', '--predefine="-DMBED_APP_SIZE=0x5c000"', '--predefine="-DMBED_RAM_START=0x30002000"', '--predefine="-DMBED_RAM_SIZE=0x16000"'

mbed_rom_start/mbed_rom_size and mbed_ram_start/mbed_ram_size mean physical ROM/RAM start/size (even though configurable here) and shouldn't change by application (mbed_app_start/mbed_app_size).

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 11, 2018

@ccli8 Cloud-client-example uses an unmanaged bootloader project. The defines in here will not take priority over them. The numbers are correct.

@ccli8

This comment has been minimized.

Contributor

ccli8 commented Jun 12, 2018

@theotherjimmy The modification meets the initial goal: use mbed_rom_start/mbed_rom_size to override rom_start/rom_size pulled from CMSIS pack for correct bootloader/application build. But besides passed to build tool, mbed_rom_start/mbed_rom_size must also pass to compiler (for flash algorithm to know actual flash start/size) and linker (for .sct/.ld/.icf to know actual flash start/size and SRAM start/size) unchangeable.

As I know, mbed_app_start defines the start where application is located and mbed_rom_start added here defines the start of physical flash. They shouldn't be mixed. Hopefully, I would like to get ROM start/size = 0x10010000/0x70000 (defined in mbed_app.json) irrespective of bootloader/application build.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 12, 2018

They shouldn't be mixed.

This is where we disagree. If this patch is to be useful to those without cmsis-packs, then we need them to be mixable.

@ccli8

This comment has been minimized.

Contributor

ccli8 commented Jun 13, 2018

@theotherjimmy It seems the disagree lies in that you use mbed_rom_start/mbed_rom_size only for bootloader/application build, but I expect more: they can also pass to compiler/linker irrespective of bootloader/application build. At the start, this PR has three goals with mbed_rom_start/mbed_rom_size added:

  1. Override rom_start/rom_size pulled from CMSIS pack or no CMSIS pack
  2. Pass overridden flash information (MBED_ROM_START/MBED_ROM_SIZE) to compiler
  3. Pass overridden flash information (MBED_ROM_START/MBED_ROM_SIZE) to linker

Currently, the modification meets G1 and doesn't meet G2/G3. In my opinion, now that mbed_rom_start/mbed_rom_size have been passed to build tool, why not also pass along to compiler/linker to meet G2/G3? I'm also focusing on G2/G3 because all G1/G2/G3 are necessary for development of my on-going TrustZone target. Is it possible to add support for G2/G3 based on the modification?

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 13, 2018

@ccli8 That's where this patch gets clever: The presence of mbed_rom_start, etc. enable bootloader mode. Therefore G2/G3 is the same as with BL mode: MBED_APP_START and MBED_APP_SIZE. This should simplify thing for the linker script.

Why do you need the direct ROM information?

for memory in memories:
try:
size = cmsis_part['memory']['IRAM1']['size']
start = cmsis_part['memory']['IRAM1']['start']

This comment has been minimized.

@theotherjimmy

theotherjimmy Jun 13, 2018

Contributor

Oh, that's wrong. Just a sec.

@ccli8

This comment has been minimized.

Contributor

ccli8 commented Jun 14, 2018

@theotherjimmy

Why do you need the direct ROM information?

Like G2, flash algorithm needs ROM information to be passed in. Take flash_api.c on NUMAKER_PFM_M2351 as an example: FMC_SECURE_ROM_SIZE currently is hard-coded to 0x40000 (256Kib), but actually it must be configurable dependent on actual secure/non-secure flash partition.

/* Secure flash */
static const flash_target_config_t flash_target_config = {
    .page_size  = 4,
    .flash_start = 0x0,
#if defined(FMC_SECURE_ROM_SIZE)
    .flash_size = FMC_SECURE_ROM_SIZE,
#else
    .flash_size = 0x80000,
#endif
    .sectors = sectors_info,
    .sector_info_count = sizeof(sectors_info) / sizeof(sector_info_t)
};

/* Non-secure flash */
static const flash_target_config_t flash_target_config_ns = {
    .page_size  = 4,
    .flash_start = NS_OFFSET + FMC_SECURE_ROM_SIZE,
#if defined(FMC_SECURE_ROM_SIZE)
    .flash_size = 0x80000 - FMC_SECURE_ROM_SIZE,
#else
    .flash_size = 0,
#endif
    .sectors = sectors_info_ns,
    .sector_info_count = sizeof(sectors_info_ns) / sizeof(sector_info_t)
};

@ccli8

This comment has been minimized.

Contributor

ccli8 commented Jun 14, 2018

@theotherjimmy Continuing my above comment. For G3 (linker script), MBED_APP_START/MBED_APP_SIZE have been enough after checking NUMAKER_PFM_M2351's current linker script. But for G2 (flash algorithm), MBED_APP_START/MBED_APP_SIZE cannot replace MBED_ROM_START/MBED_ROM_SIZE. Physical ROM information is still necessary for flash algorithm as exemplified above. Another approach to G2 is to manually add macros MBED_ROM_START/MBED_ROM_SIZE into target configuration option macros, but it may be seen as duplicate to target.mbed_rom_stat/taget.mbed_rom_size and get confusing.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 14, 2018

@ccli8 Thanks for the detail. I'll make MBED_ROM_START + MBED_ROM_SIZE available to C, C++.

@0xc0170 0xc0170 added needs: work and removed needs: review labels Jun 20, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 20, 2018

@ccli8 Thanks for the detail. I'll make MBED_ROM_START + MBED_ROM_SIZE available to C, C++.

Are we waiting for another commit here to resolve this? I added a label needs: work

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 20, 2018

@0xc0170 Yes. We're waiting on me. Just a moment.

Add Rom config info getter and rom defines
 * MBED_ROM_START = start of current rom (independent of BL modes)
 * MBED_ROM_SIZE = size of current rom (independent of BL modes)
@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 20, 2018

@ccli8 That should do it. I'm liking the way the configuration is getting a "richer" set of features and the toolchains are adding the appropriate stuff as defines.

@ccli8

This comment has been minimized.

Contributor

ccli8 commented Jun 21, 2018

@theotherjimmy This modification can meet my requirement of configuring TrustZone target. Many thanks for your support.

@cmonr

cmonr approved these changes Jun 21, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 21, 2018

/morph build

@cmonr cmonr added needs: CI and removed needs: work labels Jun 21, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Jun 21, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr cmonr merged commit 95d2b3d into ARMmbed:master Jun 21, 2018

14 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build 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/astyle Passed, 919 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10129 cycles (+1233 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 9964B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@0xc0170 0xc0170 removed the needs: CI label Jun 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment