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

Support secure/non-secure flash IAP for Cortex-M23/M33 #6630

Merged
merged 6 commits into from Apr 19, 2018

Conversation

Projects
None yet
6 participants
@ccli8
Contributor

ccli8 commented Apr 13, 2018

Description

This PR tries to support secure/non-secure flash IAP for Cortex-M23/M33 targets. It has the following behavior:

  1. Flash IAP H/W is required to be secure-only. It can erase/program both secure/non-secure flash.
  2. Target flash IAP port reports two flash_target_config_t, target_config and target_config_ns for secure/non-secure flash respectively.
  3. Non-secure application can erase/program its non-secure flash through secure flash IAP functions, flash_erase_sector/flash_program_page, but cannot erase/program secure flash, which is guarded in secure flash IAP functions.

Pull request type

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

@cyliangtw @deepikabhavnani

@0xc0170 0xc0170 requested review from deepikabhavnani and c1728p9 Apr 13, 2018

@0xc0170

Would split be more readable ? to have cmsis flash algo nonsecure ? Because looking at the diff it's lot of ifdef in the implementation here now

#if defined (__ARM_FEATURE_CMSE) && (__ARM_FEATURE_CMSE == 3U)
__attribute__((cmse_nonsecure_entry))
#endif

This comment has been minimized.

@0xc0170

0xc0170 Apr 13, 2018

Member

Isn't this somehow in CMSIS as one line macro that could say (NON SECURE FUNCTION or similar) ? This is in the entire file.

is attribute valid for all 3 toolchains?

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Apr 13, 2018

Contributor

Attribute is valid for all three toolchains, and it to specify which function is entry point to secure world.

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Apr 13, 2018

Contributor

Its not a macro in CMSIS, but its good idea to have this in macro like below:

#if defined (__ARM_FEATURE_CMSE) && (__ARM_FEATURE_CMSE == 3U)
#define SECURE_ENTRY_FNC __attribute__((cmse_nonsecure_entry))
#else
#define SECURE_ENTRY_FNC
#endif

Will look neat if code below is changed

#if defined (__ARM_FEATURE_CMSE) && (__ARM_FEATURE_CMSE == 3U)
__attribute__((cmse_nonsecure_entry))
#endif
int32_t flash_init(flash_t *obj)
{
    flash_set_target_config(obj);
    return 0;
}

to

SECURE_ENTRY_FNC int32_t flash_init(flash_t *obj)
{
    flash_set_target_config(obj);
    return 0;
}

This comment has been minimized.

@ccli8

ccli8 Apr 16, 2018

Contributor

@0xc0170 @deepikabhavnani I add MBED_NONSECURE_ENTRY for defining all-toolchain secure gateway functions in 1b7b94e and 92d937d.

This comment has been minimized.

@ccli8

ccli8 Apr 16, 2018

Contributor

Would split be more readable ?

@0xc0170 To split into multiple files, what's the suggested folder structure?
mbed-os/hal/TARGET_FLASH_CMSIS_ALGO/TARGET_CORTEX_M4/flash_common_algo.c
mbed-os/hal/TARGET_FLASH_CMSIS_ALGO/TARGET_CORTEX_M23/flash_common_algo.c
mbed-os/hal/TARGET_FLASH_CMSIS_ALGO/TARGET_CORTEX_M33/flash_common_algo.c
Or other approach?
@deepikabhavnani

This comment has been minimized.

@0xc0170

0xc0170 Apr 16, 2018

Member

If you can create a commit that we could review? I would like to compare _S version to regular one

This comment has been minimized.

@ccli8

ccli8 Apr 16, 2018

Contributor

@0xc0170 The idea is implemented in 1c51eee.

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Apr 16, 2018

Contributor

@0xc0170 - We are avoiding directory creation for S/NS as it will be difficult to remove later. With these folders in place, it might give false impression that Mbed OS supports both secure / non-secure world.. Current secure library generation is not fully secure and will change with PSA in place. I would suggest to go back to old version with defines, as it will be easier to change them.

@ccli8 Sorry for delayed response here.

This comment has been minimized.

@0xc0170

0xc0170 Apr 16, 2018

Member

OK, sounds good

This comment has been minimized.

@ccli8

ccli8 Apr 17, 2018

Contributor

@0xc0170 @deepikabhavnani OK. I cancel the commit of creating new flash algorithm folder.

@0xc0170 0xc0170 added needs: work and removed needs: review labels Apr 13, 2018

int32_t flash_erase_sector(flash_t *obj, uint32_t address)
{
#if defined (__ARM_FEATURE_CMSE) && (__ARM_FEATURE_CMSE == 3U)
if (cmse_nonsecure_caller()) {
// Confine non-secure access to non-secure flash

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Apr 13, 2018

Contributor

Can we have a common static function/macro to check range of non-secure flash, which can be called ftom all API's?

This comment has been minimized.

@ccli8

ccli8 Apr 16, 2018

Contributor

@deepikabhavnani I refactored the code in 00147b5.

@@ -50,6 +50,9 @@ typedef struct {
*/
struct flash_s {
const flash_target_config_t *target_config;
#if defined(__CORTEX_M23) || defined(__CORTEX_M33)
const flash_target_config_t *target_config_ns;

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Apr 13, 2018

Contributor

Condition everywhere for access of target_config_ns is (__ARM_FEATURE_CMSE) && (__ARM_FEATURE_CMSE == 3U),
But here we always have pointer irrespective of secure/non-secure build.. Shouldn't it be confined to secure build where we have complete flash info.. But non-secure world does not know anything about secure flash and it will access flash using target_config?

This comment has been minimized.

@ccli8

ccli8 Apr 16, 2018

Contributor

@deepikabhavnani I expect the following behavior:

  1. flash_t is initialized through secure gateway function flash_init for both secure/non-secure worlds, so all target_config/target_config_ns/flash_algo structs are initialized in secure-only flash_set_target_config (flash_api.c).
    struct flash_s {
        const flash_target_config_t *target_config;
    #if defined(__CORTEX_M23) || defined(__CORTEX_M33)
        const flash_target_config_t *target_config_ns;
    #endif
        const flash_algo_t *flash_algo;
    };
    
  2. For non-secure world, flash_t should be seen as an opaque struct. It can be accessed only through secure gateway functions flash_init/flash_free/flash_erase_sector etc. flashIAP.h/flashIAP.cpp follows this rule.

@0xc0170 0xc0170 added needs: review and removed needs: work labels Apr 16, 2018

MBED_NONSECURE_ENTRY

This comment has been minimized.

@0xc0170

0xc0170 Apr 16, 2018

Member

I would place this on the same line, any attribute it is like that MBED_WEAK function(), etc

This comment has been minimized.

@ccli8

ccli8 Apr 16, 2018

Contributor

@0xc0170 I refine the code in 7a7b634.

@0xc0170 0xc0170 added needs: work and removed needs: review labels Apr 16, 2018

@cmonr cmonr added needs: review and removed needs: work labels Apr 16, 2018

@ccli8 ccli8 force-pushed the OpenNuvoton:nuvoton_add_nonsecure_flash branch from 1c51eee to 7a7b634 Apr 17, 2018

@0xc0170

0xc0170 requested changes Apr 17, 2018 edited

Can you describe what config_ns brings ? as a comment there? With this update, we have two target configs (how they differ, when to use which one).

The rest looks good to me

@ccli8

This comment has been minimized.

Contributor

ccli8 commented Apr 18, 2018

@0xc0170 I add comment for target_config_ns in 076a160.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 18, 2018

/morph build

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 18, 2018

@deepikabhavnani Approve/request changes?

@mbed-ci

This comment has been minimized.

mbed-ci commented Apr 18, 2018

Build : SUCCESS

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

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.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 19, 2018

@c1728p9 @deepikabhavnani Please approve/request changes.

@0xc0170 0xc0170 merged commit c6b6bab into ARMmbed:master Apr 19, 2018

12 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/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9556 cycles (+28 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/littlefs Passed, code size is 10092B (+0.00%)
Details
travis-ci/tools Local tools testing has passed
Details

@ccli8 ccli8 deleted the OpenNuvoton:nuvoton_add_nonsecure_flash branch Apr 20, 2018

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