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

ccli8 · 2018-04-13T08:55:56Z

Description

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

Flash IAP H/W is required to be secure-only. It can erase/program both secure/non-secure flash.
Target flash IAP port reports two flash_target_config_t, target_config and target_config_ns for secure/non-secure flash respectively.
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

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

0xc0170 · 2018-04-13T09:04:58Z

hal/TARGET_FLASH_CMSIS_ALGO/flash_common_algo.c

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


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?

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

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; }

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

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

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

@0xc0170 The idea is implemented in 1c51eee.

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

OK, sounds good

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

deepikabhavnani · 2018-04-13T17:19:04Z

hal/TARGET_FLASH_CMSIS_ALGO/flash_common_algo.c

 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


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

@deepikabhavnani I refactored the code in 00147b5.

deepikabhavnani · 2018-04-13T17:24:57Z

hal/TARGET_FLASH_CMSIS_ALGO/flash_data.h

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


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?

@deepikabhavnani I expect the following behavior:

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; };

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.

…nctions in flash IAP

0xc0170 · 2018-04-16T07:47:45Z

hal/TARGET_FLASH_CMSIS_ALGO/flash_common_algo.c


+MBED_NONSECURE_ENTRY


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

@0xc0170 I refine the code in 7a7b634.

0xc0170

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 · 2018-04-18T03:34:43Z

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

0xc0170 · 2018-04-18T10:48:25Z

/morph build

0xc0170 · 2018-04-18T11:17:36Z

@deepikabhavnani Approve/request changes?

mbed-ci · 2018-04-18T11:33:33Z

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 · 2018-04-18T20:35:41Z

Test : SUCCESS

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

mbed-ci · 2018-04-18T21:50:06Z

Exporter Build : SUCCESS

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

0xc0170 · 2018-04-19T07:42:32Z

@c1728p9 @deepikabhavnani Please approve/request changes.

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

a62c079

0xc0170 added the needs: review label Apr 13, 2018

0xc0170 requested review from deepikabhavnani and c1728p9 April 13, 2018 09:03

0xc0170 requested changes Apr 13, 2018

View reviewed changes

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

deepikabhavnani reviewed Apr 13, 2018

View reviewed changes

ccli8 added 3 commits April 16, 2018 09:38

Add MBED_NONSECURE_ENTRY for all-toolchain secure gateway functions

1b7b94e

Replace with MBED_NONSECURE_ENTRY for all-toolchain secure gateway fu…

92d937d

…nctions in flash IAP

Refactor code for non-secure flash check in flash IAP

00147b5

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

0xc0170 requested changes Apr 16, 2018

View reviewed changes

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

Refine code with MBED_NONSECURE_ENTRY in flash IAP

7a7b634

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

ccli8 force-pushed the nuvoton_add_nonsecure_flash branch from 1c51eee to 7a7b634 Compare April 17, 2018 01:33

0xc0170 requested changes Apr 17, 2018

View reviewed changes

Add comment for target_config/target_config_ns in flash IAP

076a160

0xc0170 approved these changes Apr 18, 2018

View reviewed changes

0xc0170 added the release-version: 5.9.0-rc1 label Apr 19, 2018

c1728p9 approved these changes Apr 19, 2018

View reviewed changes

0xc0170 added ready for merge and removed needs: review labels Apr 19, 2018

deepikabhavnani approved these changes Apr 19, 2018

View reviewed changes

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

0xc0170 removed the ready for merge label Apr 19, 2018

ccli8 deleted the nuvoton_add_nonsecure_flash branch April 20, 2018 01:16

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

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

Uh oh!

Conversation

ccli8 commented Apr 13, 2018

Description

Pull request type

Uh oh!

0xc0170 left a comment

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!

0xc0170 Apr 16, 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!

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!

0xc0170 left a comment • 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!

ccli8 commented Apr 18, 2018

Uh oh!

0xc0170 commented Apr 18, 2018

Uh oh!

0xc0170 commented Apr 18, 2018

Uh oh!

mbed-ci commented Apr 18, 2018

Triggering tests

Uh oh!

mbed-ci commented Apr 18, 2018

Uh oh!

mbed-ci commented Apr 18, 2018

Uh oh!

0xc0170 commented Apr 19, 2018

Uh oh!

Uh oh!

0xc0170 Apr 16, 2018 •

edited

Loading

0xc0170 left a comment •

edited

Loading