Skip to content
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

TF-M sources integration to Mbed-OS #9653

Merged
merged 25 commits into from Feb 19, 2019

Conversation

@mikisch81
Copy link
Contributor

commented Feb 11, 2019

Description

This PR introduces TF-M sources into Mbed-OS according to ISGDEVPREQ-1027.

TF-M sources were imported into mbed-os tree using the same Importer script as CMSIS, afterwards a series of patches were applied which fix some issues and align the code to work with Mbed-OS.

The TF-M sources were imported from the feature-ipc branch of the TF-M repository, hash 45e5276bc04036654693cbf335829aa15a64ed5f.

Commit 34433bc added a manifest json file which lists the files/folder needed to import and a list of SHAs to apply afterwards.

We did not import TF-M partitions (Crypto, Attestation, etc..) as we are re-using the existing partitions in Mbed-OS (Internal-storage, PSA-Crypto and Platform), commit 42d8f2b adds an auto-generation tool which generated in
commit c855263 all the partitions' header files needed by TF-M core.

Pull request type

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

Release notes

The PSA program offers a reference implementation (TF-M) aimed at facilitating the deployment of PSA among RTOS vendors. TF-M is an OS-agnostic project, it presents generic methods to achieve PSA secure partitioning and offer PSA secure services like attestation, secure storage, or crypto. Integrating TF-M allows Mbed OS to achieve PSA compatibility.

Mbed OS has imported the TF-M core to be used by its' own secure services.

Currently only v8m targets can run TF-M, for porting TF-M to be used by a target please refer TF-M integration guide.

Reviewers

@gaborkertesz @ashok-rao @deepikabhavnani @ARMmbed/mbed-os-psa @ARMmbed/mbed-os-tools

@mikisch81 mikisch81 force-pushed the kfnta:tfm_src_integ branch Feb 11, 2019

@ciarmcom ciarmcom requested review from ashok-rao, deepikabhavnani and ARMmbed/mbed-os-maintainers Feb 11, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

@mikisch81 mikisch81 force-pushed the kfnta:tfm_src_integ branch Feb 11, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

1 similar comment
@ciarmcom

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

@deepikabhavnani
Copy link
Contributor

left a comment

Thanks for splitting. Looks good to me 👍

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

[X] Functionality change

Please add release notes section, following https://os.mbed.com/docs/mbed-os/v5.11/contributing/workflow.html#functionality-change

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

@mikisch81 Good news!

Last 7 commits are from pending PRs to Mbed-OS and should not be reviewed here:

Those two PRs are now merged in to Mbed OS, so rebasing this PR will make them disappear from this PR!

Please do so to make review of this PR a bit cleaner.

@cmonr cmonr added needs: work and removed needs: review labels Feb 11, 2019

@mikisch81 mikisch81 force-pushed the kfnta:tfm_src_integ branch to 0996c2e Feb 12, 2019

@mikisch81

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2019

Rebased this PR on top of master to get all the dependent PRs in.

@mikisch81 mikisch81 force-pushed the kfnta:tfm_src_integ branch from 0996c2e Feb 12, 2019

@mikisch81

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2019

Latest update removes the workaround we did in crypto partition and adds another patch to TF-M sources which fix an internal TF-M issue.

@mikisch81

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2019

@0xc0170 Added release notes

@bridadan
Copy link
Contributor

left a comment

The tools/tfm folder looks very similar to the existing tools/spm code in terms of the json and py files. Are there plans to merge these in a coming PR? Lots of the code appears to be duplicated.

@@ -3,8 +3,8 @@
"type": "APPLICATION-ROT",
"priority": "NORMAL",
"id": "0x0000000A",
"entry_point": "pits_entry",
"stack_size": "0x400",
"entry_point": "its_entry",

This comment has been minimized.

Copy link
@bridadan

bridadan Feb 12, 2019

Contributor

Was this rename intentional?

This comment has been minimized.

Copy link
@orenc17

orenc17 Feb 12, 2019

Contributor

yes, ITS is the correct name

tools/tfm/partition_description_schema.json Outdated
@@ -0,0 +1,196 @@
{

This comment has been minimized.

Copy link
@bridadan

bridadan Feb 12, 2019

Contributor

This file is identical to the file found here: https://github.com/ARMmbed/mbed-os/tree/master/tools/spm

This comment has been minimized.

Copy link
@orenc17

orenc17 Feb 12, 2019

Contributor

it's basically is a duplicate with minor changes, we'll remove the unneeded duplicates

tools/tfm/__init__.py Outdated
@@ -0,0 +1,23 @@
# Copyright (c) 2017-2018 ARM Limited

This comment has been minimized.

Copy link
@bridadan
tools/tfm/generate_partition_code.py Outdated
@@ -0,0 +1,689 @@
#!/usr/bin/python

This comment has been minimized.

Copy link
@bridadan

bridadan Feb 12, 2019

Contributor

This file shares most of the same codes as this one: https://github.com/ARMmbed/mbed-os/blob/master/tools/spm/generate_partition_code.py

There are a decent amount of changes though.

This comment has been minimized.

Copy link
@mikisch81

mikisch81 Feb 12, 2019

Author Contributor

Eventually everything under tools/spm will be removed when TFM will support dual-core, it will be easier to remove if it does not share stuff with tools/tfm.

components/TARGET_PSA/TARGET_TFM/COMPONENT_SPE/autogen/tfm_partition_defs.inc Outdated
@@ -0,0 +1,37 @@
/* Copyright (c) 2017-2019 ARM Limited

This comment has been minimized.

Copy link
@bridadan

bridadan Feb 12, 2019

Contributor

General question: any reason for going with .inc instead of .h? Mbed OS generally sticks with .h I believe.

This comment has been minimized.

Copy link
@cmonr

cmonr Feb 12, 2019

Contributor

+1
The only other file that uses a .inc suffix in Mbed OS is features/FEATURE_BLE/targets/TARGET_CORDIO_LL/thirdparty/uecc/asm_arm.inc, and even that's third party.

This comment has been minimized.

Copy link
@mikisch81

mikisch81 Feb 12, 2019

Author Contributor

These are the partition header files used by TF-M.
TF-M actually generated these files according to their secure partitions manifests.
But because we use our own partitions we need to generate those headers ourselves.
So in order not to change TF-M imported code we keep the same suffix.

This comment has been minimized.

Copy link
@bridadan

bridadan Feb 12, 2019

Contributor

So in order not to change TF-M imported code we keep the same suffix.

Good enough for me!

This comment has been minimized.

Copy link
@bridadan

bridadan Feb 12, 2019

Contributor

Actually, I'm not even sure if our build system picks up .inc files. Looking here: https://github.com/ARMmbed/mbed-os/blob/master/tools/resources/__init__.py#L413-L432

Seems like it won't be brought in as an include directory, does that sound right @theotherjimmy ?

This comment has been minimized.

Copy link
@bridadan

bridadan Feb 12, 2019

Contributor

@mikisch81 I believe that's working because there are other .h files present in the directory, so it is being picked up as an include path. If you removed all of the other .h files and just had .inc files, I don't believe the build system would keep that directory as an include path.

This comment has been minimized.

Copy link
@mikisch81

mikisch81 Feb 12, 2019

Author Contributor

One of the generated files is an .h file, so you are probably right, but all these files are generated together and shouldn't move or change afterwards.
We can ask TFM why the inc extension instead of regular .h.

This comment has been minimized.

Copy link
@mikisch81

mikisch81 Feb 13, 2019

Author Contributor

@bridadan We really need to avoid changing imported TF-M code, the only exceptions should be blocking issues (like bugs or porting issues).

This comment has been minimized.

Copy link
@bridadan

bridadan Feb 13, 2019

Contributor

If this is a TF-M imported thing then I understand. We should get an issue filed to add .inc to our list of header file types in our build system.

This comment has been minimized.

Copy link
@bridadan

bridadan Feb 14, 2019

Contributor

PR up for this issue: #9715

@@ -22,4 +22,5 @@ TESTS/mbed_hal/trng/pithy
targets
components/802.15.4_RF
components/wifi
components/TARGET_PSA/TARGET_TFM

This comment has been minimized.

Copy link
@cmonr

cmonr Feb 12, 2019

Contributor

@ARMmbed/mbed-os-maintainers Heads up, another large block of code that's about to be ignored by astyle

Might need to talk with techleads about why components seems to be special in regards to code styling.

This comment has been minimized.

Copy link
@mikisch81

mikisch81 Feb 12, 2019

Author Contributor

Everything under TARGET_TFM is imported from an external source, so it should be ignored by astyle as I see it.

This comment has been minimized.

Copy link
@0xc0170

@cmonr cmonr requested review from bulislaw and ARMmbed/mbed-os-maintainers Feb 12, 2019

@mikisch81

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2019

The tools/tfm folder looks very similar to the existing tools/spm code in terms of the json and py files. Are there plans to merge these in a coming PR? Lots of the code appears to be duplicated.

@bridadan tools/spm contains code generation tools for the 5.11 spm dual-core implementation, in 5.12 we integrate TFM v8m spm implementation which generate different files. Because we still need to support dual-core spm (we are currently in the process of contributing dual-core spm to TFM) we need to keep both generation tools.

@mikisch81 mikisch81 force-pushed the kfnta:tfm_src_integ branch from eaea561 to 9e33f0d Feb 19, 2019

@mikisch81

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2019

Rebased PR on top of master

@mikisch81

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2019

This needs to be added here : https://github.com/ARMmbed/mbed-os/blob/master/LICENSE . Also the 3rd party code needs license files in its folder (see https://os.mbed.com/docs/mbed-os/v5.11/contributing/license.html - Using different license). Briefly, add bsd 3 clause license file and LICENSE file to describe the license as in the license guide.

@0xc0170 Done

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

I'll run CI. @SenRamakri / @donatieng final review call

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Feb 19, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

CI started meanwhile

@ashok-rao
Copy link
Contributor

left a comment

Approved based on email discussions. Musca related items will be coming in #9221

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 19, 2019

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 3
Build artifacts

@SenRamakri
Copy link
Collaborator

left a comment

I have minor comments/questions and would be good if you can answer them, but they are not blockers, so approved.

#include "secure_fw/core/tfm_secure_api.h"

#define SPM_INVALID_PARTITION_IDX (~0U)

This comment has been minimized.

Copy link
@SenRamakri

SenRamakri Feb 19, 2019

Collaborator

Does these enums require Doxygen comments?

This comment has been minimized.

Copy link
@mikisch81

mikisch81 Feb 19, 2019

Author Contributor

This file is also imported.

#define TFM_PARTITION_TYPE_APP "APPLICATION-ROT"
#define TFM_PARTITION_TYPE_PSA "PSA-ROT"

#define TFM_STACK_SIZE 1024

This comment has been minimized.

Copy link
@SenRamakri

SenRamakri Feb 19, 2019

Collaborator

Should TFM_STACK_SIZE be configurable, driven from config files? On what basis we are hardcoding this to 1024?

This comment has been minimized.

Copy link
@mikisch81

mikisch81 Feb 19, 2019

Author Contributor

Actually each partitions' stack size is configured in their manifest, this is one of our patches to TFM code, see commit fc78640, so TFM_STACK_SIZE is not used anymore.

tfm_spm_partition_set_state(idx, SPM_PARTITION_STATE_IDLE);
} else {
tfm_spm_partition_err_handler(part, TFM_INIT_FAILURE, res);
fail_cnt++;

This comment has been minimized.

Copy link
@SenRamakri

SenRamakri Feb 19, 2019

Collaborator

Minor thing, but can't we just break here(right after fail_cnt++) instead of looping again? Or is it really required to call err_handler for each error?

This comment has been minimized.

Copy link
@mikisch81

mikisch81 Feb 19, 2019

Author Contributor

This file is imported from TFM repository (same as cmsis code), we added patches but they are mainly fixes for issues which were raised during the Musca port.

*/
#define TFM_CLIENT_ID_IS_NS(client_id) ((client_id)<0)

/* FixMe: sort out DEBUG compile option and limit return value options

This comment has been minimized.

Copy link
@SenRamakri

SenRamakri Feb 19, 2019

Collaborator

FixMe?? Should this be fixed?

This comment has been minimized.

Copy link
@mikisch81

mikisch81 Feb 19, 2019

Author Contributor

This file is also imported.

@cmonr cmonr added ready for merge and removed needs: CI labels Feb 19, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2019

Final review responses look alright. Merging.

@cmonr cmonr merged commit 4c04771 into ARMmbed:master Feb 19, 2019

27 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9974 cycles
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Feb 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.