TF-M: directory structure update proposal to import twincpu branch to mbed-os #11640

urutva · 2019-10-07T12:32:52Z

Description

In mbed-os, we currently support v8-M targets using TF-M Secure Partition Manager (SPM) and mbed-os services and twin v7-M targets using MBED SPM and mbed-os services. This combination adds significant maintenance overhead. Therefore we are planning use both PSA SPM and services from TF-M to support v8-M and twin v7-M targets.

This PR proposes a new directory structure which is required to import TF-M twin cpu implementation into mbed-os.

At the time of creating this PR, TF-M master only supports v8-M targets and twin-cpu targets are supported on a feature branch. Therefore our plan is,

Continue supporting v8-M targets on mbed-os master (TF-M SPM and mbed-os services)
Continue supporting twin v7-M targets on mbed-os master (MBED SPM and mbed-os services)
Continue supporting single v7-M targets on mbed-os master (emulation and mbed-os services)
On feature-twincpu branch, add support for twincpu (TF-M SPM and TF-M PSA services)
On feature-twincpu branch, maintain support for single v7-M targets on mbed-os master (emulation and mbed-os services)
Merge feature-twincpu to master once it is stable and TF-M feature-twincpu branch gets merged with TF-M master branch

Pull request type

[ ] Fix
[x] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[x] Breaking change

Reviewers

@Patater @jainvikas8

Release Notes

Switch from MBED Secure Partition Manager to TF-M Secure Partition Manager to support twincpu v7-M targets
Switch from Mbed secure services to TF-M secure services to support twincpu v7-M targets

Impact Analysis

Non-secure side application:
There is no impact on non-secure side application as the PSA APIs will remain the same.
Secure side application:
Breaking change if someone wants to add their own secure service, but we don't provide any migration for them. They need to use TF-M (https://git.trustedfirmware.org/trusted-firmware-m.git/tree/) instead.

ciarmcom · 2019-10-07T13:01:12Z

@Devran01, thank you for your changes.
@Patater @jainvikas8 @ARMmbed/mbed-os-maintainers please review.

jainvikas8 · 2019-10-07T13:07:38Z

@Devran01 Please could check the astyle issue by Travis.

urutva · 2019-10-07T14:12:47Z

@jainvikas8 Updated .astyleignore to match new directory structure

urutva · 2019-10-07T15:12:34Z

psa-autogen check fails because Mbed PSA services for v8-M have been removed from feature branch.

bulislaw · 2019-10-09T14:01:16Z

It's hard to judge what's going on with 500 files moving, can you describe the new model and what has changed? Also Maybe it's time to remove PSA from components/ directory to features/psa. Components was meant for supporting external hw modules, not sure how PSA ended up there in the first place.

0Grit · 2019-10-10T06:43:24Z

@bulislaw I still can't get a handle on the true design intent behind "components"
See:
#9479
#7644 (comment)
#11253 (comment)
#9285 (comment)

https://github.com/ARMmbed/mbed-os-5-docs/blob/development/docs/reference/build_rules.md#component-directories

urutva · 2019-10-11T09:49:28Z

@bulislaw Good point. I guess COMPONENT was chosen for PSA to handle complex combination of supporting single v7-M, dual-v7-M and v8-M in both Secure (_S) and non-secure (_NS) modes in mbed-os (https://github.com/ARMmbed/mbed-os-5-docs/blob/development/docs/porting/psa/spm.md). Our plan is to run TF-M on the secure side and mbed-os on the non-secure side, but TF-M will be built outside of mbed-os build system. This simplifies the combination that needs to be supported with mbed-os build system which will be supporting single v7-M, dual-v7-M and v8-M for non-secure mode (_NS).

I'll look into moving PSA related files to features/psa.

The plan:

Continue supporting v8-M targets on mbed-os master (TF-M SPM and mbed-os services)
Continue supporting twin v7-M targets on mbed-os master (MBED SPM and mbed-os services)
Continue supporting single v7-M targets on mbed-os master (emulation and mbed-os services)
On feature-twincpu branch, add support for twincpu (TF-M SPM and TF-M PSA services)
On feature-twincpu branch, maintain support for single v7-M targets on mbed-os master (emulation and mbed-os services)

Merge feature-twincpu to master once it is stable and TF-M feature-twincpu branch gets merged with TF-M master branch.

Patater · 2019-10-11T11:19:12Z

@Devran01 Do we need generate_partition_code.py at all anymore? Probably it and its test can be removed. It wouldn't be used for single v7 targets.

AssertionError: The source file /home/travis/build/ARMmbed/mbed-os/components/TARGET_PSA/COMPONENT_MBED_PSA_SRV/services/attestation/COMPONENT_SPE/psa_attestation_partition.c mentioned in /home/travis/build/ARMmbed/mbed-os/components/TARGET_PSA/COMPONENT_MBED_PSA_SRV/services/attestation/attestation_partition_psa.json doesn't exist.

The command "python tools/psa/generate_partition_code.py" exited with 1.

urutva · 2019-10-14T16:14:48Z

@Devran01 Do we need generate_partition_code.py at all anymore? Probably it and its test can be removed. It wouldn't be used for single v7 targets.

AssertionError: The source file /home/travis/build/ARMmbed/mbed-os/components/TARGET_PSA/COMPONENT_MBED_PSA_SRV/services/attestation/COMPONENT_SPE/psa_attestation_partition.c mentioned in /home/travis/build/ARMmbed/mbed-os/components/TARGET_PSA/COMPONENT_MBED_PSA_SRV/services/attestation/attestation_partition_psa.json doesn't exist.

The command "python tools/psa/generate_partition_code.py" exited with 1.

@Patater No we don't for twincpu.
New changes:

Move PSA codebase from COMPONENTS to features folder. COMPONENTS should be used for supporting external hw modules.
Cleanup tools folder. PSA related hooks are removed from the build system.
Replace TARGET_PSA with FEATURE_PSA and TARGET_TFM with FEATURE_TFM.

Patater · 2019-10-17T12:19:40Z

platform/mbed_retarget.cpp

@@ -913,7 +913,7 @@ extern "C" long PREFIX(_flen)(FILEHANDLE fh)
 }

 // Do not compile this code for TFM secure target
-#if !defined(COMPONENT_SPE) || !defined(TARGET_TFM)


Why remove COMPONENT_SPE here?

@Patater We do not have COMPONENT_SPE anymore or FEATURE_SPE as we'll not build secure side using mbed-cli. But in this case we need to identify non-secure and the current #if i snot doing that. I'll change the code to #if defined(FEATURE_NSPE) which is only defined for NSPE.

Patater · 2019-10-17T12:20:12Z

tools/test/psa/__init__.py

@@ -1,17 +0,0 @@
-"""


Commit "Remvoe PSA related scripts from tools/test folder" should say "Remove PSA related scripts from tools/test folder"

I'll squash the commits and update the comment.

Patater · 2019-10-17T12:20:58Z

.travis.yml

+        - python -m pip install --upgrade pip==18.1
+        - python -m pip install --upgrade setuptools==40.4.3
+        - pip install -r requirements.txt
+        - pip list --verbose


How was the events test working previously without these changes?

They were included as part of psa-autogen test. The events test failed when I removed them.

jainvikas8 · 2019-10-17T15:11:39Z

.travis.yml

@@ -1,4 +1,4 @@
-# Copyright (c) 2013-2018 Arm Limited. All rights reserved.


Please fix up the commit message.

@jainvikas8 what's missing from the commit message?

jainvikas8

Heading less than 50 characters, please.

urutva · 2019-10-17T18:04:19Z

Heading less than 50 characters, please.

Fixed.

0xc0170 · 2019-10-18T13:05:19Z

[x] Breaking change

If this is breaking change, it needs to have Release notes section: https://os.mbed.com/docs/mbed-os/v5.14/contributing/workflow.html#pull-request-template-breaking-changes

0xc0170 · 2019-10-18T13:06:22Z

Ah wait, this is for feature branch, was not labeled. Fixing now.

Still, this should include how is this breaking.

0xc0170 · 2019-10-18T13:09:06Z

I would like to understand this one : +59 −23,788 - almost 24k lines removed - what is being removed and why. Can you provide short description (it shall be part of the release notes section). What is being removed, are we losing any functionality.

Keep this up to date during the reviews here so we can read the current status.

urutva · 2019-10-21T10:58:32Z

@0xc0170 I've updated PR Description and Release Notes

urutva · 2019-10-21T11:15:45Z

@jainvikas8 As suggested, removed psa_manifest/sid.h as service id definitions are not relevant for single-v7-M.

0xc0170 · 2019-10-21T11:21:31Z

In the release notes there is no impact - so what is breaking here? Is it the switch from SPM to TFM? It's not clear to me.

urutva · 2019-11-13T11:08:05Z

@Patater Can you please review last two commits 58f7534 and 49dce36?

0xc0170 · 2019-11-13T14:37:38Z

58f7534

This commit should be on master as fix, I'll start CI now as this is for feature branch (rebase will clean it up anyway)

mbed-ci · 2019-11-13T15:28:39Z

Test run: FAILED

Summary: 3 of 4 test jobs failed
Build number : 3
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_build-IAR
jenkins-ci/mbed-os-ci_build-ARM
jenkins-ci/mbed-os-ci_build-GCC_ARM

0xc0170 · 2019-11-13T16:45:48Z

[ERROR] name 'find_secure_image' is not defined error for blinky example, please review

urutva · 2019-11-14T10:08:39Z

[ERROR] name 'find_secure_image' is not defined error for blinky example, please review

@0xc0170 I've fixed the issues. Can you please restart the CI?

Patater · 2019-11-14T10:24:17Z

targets/targets.json

@@ -2042,113 +2042,6 @@
        "device_has_remove": ["QSPI"],
        "release_versions": ["2", "5"]
    },
-    "LPC55S69": {


Commit "Remove v8-M and dual v7-M targets" seems only to remove v8-M targets. Consider changing commit message.

Yeah, rebasing on top master had removed dual v7-M targets. Updated the commit message.

Patater · 2019-11-14T10:25:50Z

tools/test/psa/test_find_secure_image.py

@@ -1,57 +0,0 @@
-# Copyright (c) 2019 ARM Limited


If you are keeping the find_secure_image() function, shouldn't we also keep the test?

Yes, missed that. I'll bring back the test.

Patater · 2019-11-14T10:27:09Z

targets/targets.json

@@ -9183,31 +9132,6 @@
        },
        "program_cycle_s": 10
    },
-    "CY8CKIT_062_WIFI_BT": {


Why remove this target in commit "Refactor Mbed PSA service code base "?

The target shouldn't have been removed. Fixed.

The Mbed OS PSA services is moved to features/FEATURE_PSA/TARGET_MBED_PSA_SRV. Removed PSA_SRV_IMPL, PSA_SRV_EMUL and NSPE. Added new feature `PSA` to support PSA in Mbed OS. Created following generic PSA targets: * `PSA_Target` (Root level PSA target) * `PSA_V7_M_NSPE` (Single v7-M NSPE generic target) * `PSA_V7_M_SPE` (Single v7-M SPE generic target) * `PSA_DUAL_V7_M_NSPE` (Dual v7-M NSPE generic target) * `PSA_DUAL_V7_M_SPE` (Dual v7-M SPE generic target) * `PSA_V8_M_NSPE` (v8-M NSPE generic target) * `PSA_V8_M_SPE` (v8-M SPE generic target) Added document features/FEATURE_PSA/supporting_psa_in_mbed-os.md which describes PSA support in Mbed OS. Signed-off-by: Devaraj Ranganna <devaraj.ranganna@arm.com>

Signed-off-by: Devaraj Ranganna <devaraj.ranganna@arm.com>

1. Remove psa related hooks from the build system. 2. Remove PSA related scripts from tools/test folder 3. Remove psa-autogen job from travis which was running generate_partition_code.py and is not needed anymore 4. Install python modules needed for events and littlefs tests Signed-off-by: Devaraj Ranganna <devaraj.ranganna@arm.com>

Removed MUSCA A1 and LPC55S69 targets as they are not supported on feature branch. The targets will be re-introduced before merging feature branch to master. Signed-off-by: Devaraj Ranganna <devaraj.ranganna@arm.com>

The example `mbed-os-example-atecc608a` needs the secure element driver which is removed from feature branch. The secure element support should be added to TF-M and when we import TF-M to mbed-os, secure element will be supported in mbed-os. Because of this compilation of the example `mbed-os-example-atecc608a` is disabled. Note: DO NOT merge this to `master` without secure element support in mbed-os. Signed-off-by: Devaraj Ranganna <devaraj.ranganna@arm.com>

Current algorithm doesn't support multiple inheritance when inheritance depth is more than 1 on two parents. It'll not parse the "_add" or "_remove" attributes on a second parent which is at the same level as that of a parent that defines the attribute. This change ensures that second parent which is at the same level as that of a parent that defines the attribute is parsed for "_add" or "_remove" attributes. However, this change will parse the parent which defines the attributes twice, once to evaluate the attribute and then to evaluate the "_add" or "_remove" attribute. But, no target is allowed to define an attribute and also "_add" or "_remove" for the same attribute. Signed-off-by: Devaraj Ranganna <devaraj.ranganna@arm.com>

0xc0170 · 2019-11-14T11:29:29Z

I restarted CI.

Note, there is a failure on current master that might show up here as well.

mbed-ci · 2019-11-14T13:41:09Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 4
Build artifacts

0xc0170 · 2019-11-14T13:58:10Z

@Patater All requests resolved?

There's pr merge CI issue, will be fixed and this shall be ready to integrate?

Patater · 2019-11-14T16:39:06Z

@Patater All requests resolved?

Not yet.

Patater · 2019-11-15T15:58:11Z

@Patater All requests resolved?

Not yet.

@Devran01 showed me where the test_find_secure_image() function went. I hadn't expected it to move, but it has indeed been re-added. All OK.

urutva · 2019-11-19T14:29:15Z

@0xc0170 @Patater Can this PR be merged?

ciarmcom requested review from jainvikas8, Patater and a team October 7, 2019 13:01

ciarmcom added the needs: review label Oct 7, 2019

Patater suggested changes Oct 17, 2019

View reviewed changes

urutva force-pushed the feature-twincpu-pr branch from 85c602e to 825d727 Compare October 17, 2019 15:04

jainvikas8 reviewed Oct 17, 2019

View reviewed changes

urutva force-pushed the feature-twincpu-pr branch 2 times, most recently from aac9784 to 0c73fa7 Compare October 17, 2019 18:02

0xc0170 added the BREAKING-CHANGE label Oct 18, 2019

0xc0170 added the release-version: feature branch label Oct 18, 2019

urutva force-pushed the feature-twincpu-pr branch from 0c73fa7 to ddf6e89 Compare October 21, 2019 11:14

0xc0170 requested a review from Patater October 21, 2019 11:20

Patater approved these changes Nov 13, 2019

View reviewed changes

urutva force-pushed the feature-twincpu-pr branch from 58f7534 to 134e2bd Compare November 13, 2019 18:11

Patater suggested changes Nov 14, 2019

View reviewed changes

urutva force-pushed the feature-twincpu-pr branch 2 times, most recently from 2a9ec19 to 0174e69 Compare November 14, 2019 11:13

urutva added 6 commits November 14, 2019 11:15

Update .astyleignore to match new directory structure

cd04e9e

Signed-off-by: Devaraj Ranganna <devaraj.ranganna@arm.com>

Remove v8-M targets

d2de5d6

Removed MUSCA A1 and LPC55S69 targets as they are not supported on feature branch. The targets will be re-introduced before merging feature branch to master. Signed-off-by: Devaraj Ranganna <devaraj.ranganna@arm.com>

urutva force-pushed the feature-twincpu-pr branch from 0174e69 to 0423668 Compare November 14, 2019 11:15

0xc0170 requested a review from Patater November 14, 2019 11:28

0xc0170 added needs: review and removed needs: work labels Nov 14, 2019

0xc0170 approved these changes Nov 14, 2019

View reviewed changes

Patater approved these changes Nov 15, 2019

View reviewed changes

0xc0170 merged commit 7ea6419 into ARMmbed:feature_twincpu Nov 19, 2019

0xc0170 removed the needs: review label Nov 19, 2019

		@@ -1,4 +1,4 @@
		# Copyright (c) 2013-2018 Arm Limited. All rights reserved.

TF-M: directory structure update proposal to import twincpu branch to mbed-os #11640

TF-M: directory structure update proposal to import twincpu branch to mbed-os #11640

Conversation

urutva commented Oct 7, 2019 • edited

Description

Pull request type

Reviewers

Release Notes

Impact Analysis

ciarmcom commented Oct 7, 2019

jainvikas8 commented Oct 7, 2019

urutva commented Oct 7, 2019

urutva commented Oct 7, 2019

bulislaw commented Oct 9, 2019

0Grit commented Oct 10, 2019

urutva commented Oct 11, 2019

Patater commented Oct 11, 2019

urutva commented Oct 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jainvikas8 left a comment

Choose a reason for hiding this comment

urutva commented Oct 17, 2019

0xc0170 commented Oct 18, 2019

0xc0170 commented Oct 18, 2019 • edited

0xc0170 commented Oct 18, 2019 • edited

urutva commented Oct 21, 2019

urutva commented Oct 21, 2019

0xc0170 commented Oct 21, 2019

urutva commented Nov 13, 2019

0xc0170 commented Nov 13, 2019

mbed-ci commented Nov 13, 2019

0xc0170 commented Nov 13, 2019

urutva commented Nov 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xc0170 commented Nov 14, 2019

mbed-ci commented Nov 14, 2019

0xc0170 commented Nov 14, 2019

Patater commented Nov 14, 2019

Patater commented Nov 15, 2019

urutva commented Nov 19, 2019

urutva commented Oct 7, 2019 •

edited

0xc0170 commented Oct 18, 2019 •

edited

0xc0170 commented Oct 18, 2019 •

edited