PSA: TFM import #10829

urutva · 2019-06-13T17:02:23Z

Description

Import TF-M sources from master branch at hash e7efdc6e8d89a22c4cac5b6ccc8e7d0d332ff034

Pull request type

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

Reviewers

@alzix @orenc17 @Patater

Release Notes

- Remove un-needed files - Disable printf and uart - Modify include paths - Guard macros from mbed_lib with ifndef (cherry picked from commit 1f30b52) (cherry picked from commit 71cd34d) (cherry picked from commit 185d286) (cherry picked from commit fb068d2)

- Link to bug tracking: https://developer.trustedfirmware.org/T239 (cherry picked from commit 5f2e4b3) (cherry picked from commit 5d41a2a)

…#230) - Link to bug tracking: https://developer.trustedfirmware.org/T230 (cherry picked from commit 0c23e86) (cherry picked from commit 9c1e080)

…RMmbed#241) - Link to bug tracking: https://developer.trustedfirmware.org/T241 (cherry picked from commit da01e34) (cherry picked from commit 280715f)

ciarmcom · 2019-06-13T19:00:40Z

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

mark-edgeworth

Ok, comments added as I see it. Apologies in advance if I'm out of step with the whole of Mbed OS so far, and feel free to disregard if you want. Your comments are welcome though.
Most general point - abbreviating names to this extent makes the code less readable for me.

mark-edgeworth · 2019-06-18T13:21:20Z

components/TARGET_PSA/TARGET_TFM/COMPONENT_SPE/secure_fw/core/ipc/include/tfm_spm.h

+ *  ctxb        -    State context storage pointer
+ *
+ * Notes:
+ *  This is a staging API. Scheduler should be called in SPM finally and


In light of this comment should this be marked as deprecated in some way?

mark-edgeworth · 2019-06-18T13:24:44Z

components/TARGET_PSA/TARGET_TFM/COMPONENT_SPE/secure_fw/core/ipc/include/tfm_thread.h

 */
-void tfm_pendsv_do_schedule(struct tfm_state_context_ext *ctxb);
+void tfm_thrd_exit(void);


Why abbreviate so aggressively? tfm_thread_exit(), anyone?

mark-edgeworth · 2019-06-18T13:29:39Z

components/TARGET_PSA/TARGET_TFM/COMPONENT_SPE/secure_fw/core/ipc/tfm_spm.c

        return IPC_SUCCESS;
    }

    return IPC_ERROR_MEMORY_CHECK;
 }

+uint32_t tfm_spm_partition_get_privileged_mode(uint32_t partition_idx)


Personally I'd prefer something along the lines of '..._is_priviledged_mode()' that returns TRUE or FALSE, but that might be too many years of Java. The function as it stands will still need its (typeless) return value checking against one of these constants; an opportunity for a bug later...

mark-edgeworth · 2019-06-18T13:30:56Z

components/TARGET_PSA/TARGET_TFM/COMPONENT_SPE/secure_fw/core/ipc/tfm_spm.c

+                                                 struct spm_partition_desc_t,
+                                                 sp_thrd);
+
+        if (p_next_partition->static_data.partition_flags &


Looks like a duplicate of that 'tfm_spm_partition_get_privileged_mode()' function above.

mark-edgeworth · 2019-06-18T13:32:02Z

components/TARGET_PSA/TARGET_TFM/COMPONENT_SPE/secure_fw/core/ipc/tfm_thread.c

-        tfm_thrd_context_switch(ctxb, CURR_THRD, pth);
-    }
+    /* Update current thread indicator */
+    CURR_THRD = next;


CURRENT_THREAD

mark-edgeworth · 2019-06-18T13:34:13Z

components/TARGET_PSA/TARGET_TFM/COMPONENT_SPE/secure_fw/core/ipc/tfm_thread.c

 }

 /* Remove current thread out of the schedulable list */
-void tfm_thrd_do_exit(void)
+void tfm_svcall_thrd_exit(void)


thrd->thread. This is currently inconsistent with other functions defined, eg 'tfm_nspm_thread_entry'

mark-edgeworth · 2019-06-18T13:34:41Z

components/TARGET_PSA/TARGET_TFM/COMPONENT_SPE/secure_fw/core/ipc/tfm_thread.c

 }

-/* Scheduling won't happen immediately but after the exception returns */
-void tfm_thrd_activate_schedule(void)
+void tfm_thrd_start_scheduler(struct tfm_thrd_ctx *pth)


thrd->thread

mark-edgeworth · 2019-06-18T13:35:35Z

components/TARGET_PSA/TARGET_TFM/COMPONENT_SPE/secure_fw/core/tfm_svc.h

@@ -23,6 +23,7 @@ typedef enum {
 #ifdef TFM_PSA_API
    TFM_SVC_IPC_REQUEST,
    TFM_SVC_SCHEDULE,
+    TFM_SVC_EXIT_THRD,


TFM_SVC_EXIT_THREAD

mark-edgeworth · 2019-06-18T13:36:45Z

components/TARGET_PSA/TARGET_TFM/COMPONENT_SPE/secure_fw/spm/spm_api.c

+
+    ctrl.w = __get_CONTROL();
+
+    if (privileged == TFM_PARTITION_PRIVILEGED_MODE) {


I'd prefer a ternary operator here.

mark-edgeworth · 2019-06-18T13:38:10Z

tools/importer/tfm_importer.json

-    "280715f9b74ab29459d81edaf02b39e7a6acb13c",
-    "ea81bf91c90ae23dd9de012bfd7498613be00601",
-    "5342015bb12a486a1c563175a8a7129f0737c925"
+    "11bff3f3cbfbd3e2c284e884d0066531e6b47d7e",


Don't know what these are - should they be committed?

These are commit hashes of patches present in the Mbed OS repo (in this very PR, in fact!), necessary to complete the import and integration of TF-M into Mbed OS. Yes, they are needed and should be committed.

orenc17 · 2019-06-18T14:43:49Z

@mark-edgeworth

all of your comments are on TF-M sources, these sources are imported as is from trusted-firmware.org
the change in tools/importer/tfm_importer.json is due to bug fixes getting merged into TF-M.

@ARMmbed/mbed-os-maintainers i think that the changes require tests results of the PSA tests on Arm v8-m PSA targets (e.g. LPC55s69)

mark-edgeworth · 2019-06-18T14:49:12Z

@mark-edgeworth

* all of your comments are on TF-M sources, these sources are imported as is from trusted-firmware.org

Ok, thanks for clarifying.

adbridge · 2019-06-20T15:40:54Z

@Patater could we have your review please ?

Patater

Do we need the patch "TF-M patch/workaround related to (TF-M issue #T240)"? TF-M has merged the fix.
There seems to be a patch open for https://developer.trustedfirmware.org/T396, with a request for your comment. Could we use that patchset instead of our own?

Patater · 2019-06-20T17:14:21Z

components/TARGET_PSA/TARGET_TFM/COMPONENT_SPE/secure_fw/spm/spm_db.h

 #endif
 };

 /* Macros to pick linker symbols and allow to form the partition data base */
 #define REGION(a, b, c) a##b##c
 #define REGION_NAME(a, b, c) REGION(a, b, c)
-#if (TFM_LVL == 1) && !defined(TFM_PSA_API)
+#if (TFM_LVL == 1)


Please comment in our patch why this check is changed from the original.

Patater · 2019-06-20T17:15:00Z

components/TARGET_PSA/TARGET_TFM/COMPONENT_SPE/secure_fw/spm/spm_api.c

@@ -286,7 +290,6 @@ uint32_t tfm_spm_partition_get_flags(uint32_t partition_idx)
            partition_flags;
 }

-#ifndef TFM_PSA_API


Please comment in our patch (in the code, not the commit description) why this check is removed.

This change is not relevant anymore. Retained the code from TF-M.

Patater · 2019-06-20T17:15:28Z

components/TARGET_PSA/TARGET_TFM/COMPONENT_SPE/secure_fw/spm/spm_db_setup.h

@@ -38,7 +38,7 @@ struct spm_partition_db_t {
        data.partition_priority = TFM_PRIORITY(priority);                     \
    } while (0)

-#if (TFM_LVL == 1) && !defined(TFM_PSA_API)
+#if (TFM_LVL == 1)


Please comment in our patch why this change is needed.

alzix · 2019-06-20T18:06:08Z

tools/importer/tfm_importer.json

+    "910a402ce6c96b654cb6ae1a5b679e4f856c5419",
+    "d3c610e090282893ba1820c30fc8be3c03195091",
+    "fb35880cecff7205fd19aaaed02b9958b9bfce5a",
+    "be5cfdf18e6e70c9360dea7456c85189a56f277e"


a while ago i've added an option to specify a help message/description for each hash. Take a look here https://github.com/ARMmbed/mbed-os/blob/master/tools/importer/importer.py#L239

Consider specifying a commit message for each hash - after couple of months when you will be back for new re-import - it will help you a lot.

@alzix Ah cool. Thanks. I'll update the json file.

urutva · 2019-06-26T08:35:16Z

@mark-edgeworth

all of your comments are on TF-M sources, these sources are imported as is from trusted-firmware.org

the change in tools/importer/tfm_importer.json is due to bug fixes getting merged into TF-M.

@ARMmbed/mbed-os-maintainers i think that the changes require tests results of the PSA tests on Arm v8-m PSA targets (e.g. LPC55s69)

@orenc17 @ARMmbed/mbed-os-maintainers I ran PSA relevant test cases on LPC55S69 and all of them pass.

compiler errors as mbed-cli only generates "-D" macros only for "macros" defined in targets.json TF-M task link: https://developer.trustedfirmware.org/T396 Signed-off-by: Devaraj Ranganna <[email protected]>

when TFM_PSA_API is not set Signed-off-by: Devaraj Ranganna <[email protected]>

- Link to bug tracking: https://developer.trustedfirmware.org/T240 The issue is fixed by TF-M team. However they autogenerate region details (code, ro, rw, zi and stack ) using linker scripts and in mbed-os we also autogenerate region details but using mix of service definition in json file and other template files. Signed-off-by: Devaraj Ranganna <[email protected]>

0xc0170 · 2019-06-26T14:17:52Z

@Devran01 Can you answer @Patater review?

urutva · 2019-06-26T14:56:39Z

Do we need the patch "TF-M patch/workaround related to (TF-M issue #T240)"? TF-M has merged the fix.

We can't avoid this patch even though the issue is fixed by TF-M team. They autogenerate region details (code, ro, rw, zi and stack ) using linker scripts and in mbed-os we also autogenerate region details but using mix of service definition in json file and other template files.
However, I adapted our solution to work with TF-M changes except using linker scripts. I'll push the changes soon.

There seems to be a patch open for https://developer.trustedfirmware.org/T396, with a request for your comment. Could we use that patchset instead of our own?

The patchset is for code quality improvement and is a single commit and has lot of other changes. This patch will be removed while updating to next release of TM-M (assuming that the patchset would land in master by that time).

Signed-off-by: Devaraj Ranganna <[email protected]>

mbed-ci · 2019-06-28T15:01:51Z

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_cloud-client-test

urutva · 2019-07-01T11:23:18Z

@0xc0170 CI failed. Can you please check?

0xc0170 · 2019-07-01T12:19:29Z

@Devran01 CI is all good (was restarted and now green). We should now get approvals as there were earlier requests above.

@Patater Is this needs work or ready to progress?

Patater

LGTM

0xc0170 · 2019-07-01T13:35:47Z

Set to 5.14 based on the changes and PR type.

urutva and others added 5 commits June 6, 2019 14:47

[trusted-firmware-m]: Updated to e7efdc6

b0fe59d

TF-M patch: Fix tfm_ns_lock_init issue (TF-M issue ARMmbed#239)

795e641

- Link to bug tracking: https://developer.trustedfirmware.org/T239 (cherry picked from commit 5f2e4b3) (cherry picked from commit 5d41a2a)

TF-M patch: Fix service handles not cleared issue (TF-M issue ARMmbed…

35938a4

…#230) - Link to bug tracking: https://developer.trustedfirmware.org/T230 (cherry picked from commit 0c23e86) (cherry picked from commit 9c1e080)

TF-M patch: Fix tfm_psa_call_venner wrong argument type (TF-M issue A…

910a402

…RMmbed#241) - Link to bug tracking: https://developer.trustedfirmware.org/T241 (cherry picked from commit da01e34) (cherry picked from commit 280715f)

ciarmcom requested review from alzix, orenc17, Patater and a team June 13, 2019 19:00

ciarmcom added the needs: review label Jun 13, 2019

mark-edgeworth reviewed Jun 18, 2019

View reviewed changes

Patater suggested changes Jun 20, 2019

View reviewed changes

alzix reviewed Jun 20, 2019

View reviewed changes

urutva added 3 commits June 26, 2019 14:23

TF-M patch: Change #if TFM_PSA_API to #ifdef TFM_PSA_API to avoid

cb748c5

compiler errors as mbed-cli only generates "-D" macros only for "macros" defined in targets.json TF-M task link: https://developer.trustedfirmware.org/T396 Signed-off-by: Devaraj Ranganna <[email protected]>

TF-M patch: Remove secure_fw/core/tfm_func_api.c which is required only

9a51105

when TFM_PSA_API is not set Signed-off-by: Devaraj Ranganna <[email protected]>

0xc0170 approved these changes Jun 26, 2019

View reviewed changes

Update commit sha's

98fa63a

Signed-off-by: Devaraj Ranganna <[email protected]>

urutva force-pushed the feature_trusted-firmware-m_e7efdc6 branch from 61e9f71 to 98fa63a Compare June 26, 2019 16:02

Patater approved these changes Jul 1, 2019

View reviewed changes

0xc0170 added ready for merge release-version: 5.14.0-rc1 and removed needs: review labels Jul 1, 2019

0xc0170 merged commit 1aad06b into ARMmbed:master Jul 1, 2019

0xc0170 removed the ready for merge label Jul 1, 2019

urutva deleted the feature_trusted-firmware-m_e7efdc6 branch July 2, 2019 08:14


		ctrl.w = __get_CONTROL();

		if (privileged == TFM_PARTITION_PRIVILEGED_MODE) {

PSA: TFM import #10829

PSA: TFM import #10829

Uh oh!

Conversation

urutva commented Jun 13, 2019

Description

Pull request type

Reviewers

Release Notes

Uh oh!

ciarmcom commented Jun 13, 2019

Uh oh!

mark-edgeworth 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!

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!

orenc17 commented Jun 18, 2019

Uh oh!

mark-edgeworth commented Jun 18, 2019

Uh oh!

adbridge commented Jun 20, 2019

Uh oh!

Patater 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!

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!

urutva commented Jun 26, 2019

Uh oh!

0xc0170 commented Jun 26, 2019

Uh oh!

urutva commented Jun 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mbed-ci commented Jun 28, 2019

Uh oh!

urutva commented Jul 1, 2019

Uh oh!

0xc0170 commented Jul 1, 2019

Uh oh!

Patater left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Jul 1, 2019

Uh oh!

urutva commented Jun 26, 2019 •

edited

Loading