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

PSA: TFM import #10829

Merged
merged 9 commits into from Jul 1, 2019

Conversation

@devran01
Copy link
Contributor

commented Jun 13, 2019

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

devran01 and others added 5 commits Jun 6, 2019
- 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)
- Link to bug tracking: https://developer.trustedfirmware.org/T230

(cherry picked from commit 0c23e86)
(cherry picked from commit 9c1e080)
)

- Link to bug tracking: https://developer.trustedfirmware.org/T241

(cherry picked from commit da01e34)
(cherry picked from commit 280715f)
@ciarmcom ciarmcom requested review from alzix, orenc17, Patater and ARMmbed/mbed-os-maintainers Jun 13, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

Copy link
Contributor

left a comment

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.

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

This comment has been minimized.

Copy link
@mark-edgeworth

mark-edgeworth Jun 18, 2019

Contributor

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

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

This comment has been minimized.

Copy link
@mark-edgeworth

mark-edgeworth Jun 18, 2019

Contributor

Why abbreviate so aggressively? tfm_thread_exit(), anyone?

return IPC_SUCCESS;
}

return IPC_ERROR_MEMORY_CHECK;
}

uint32_t tfm_spm_partition_get_privileged_mode(uint32_t partition_idx)

This comment has been minimized.

Copy link
@mark-edgeworth

mark-edgeworth Jun 18, 2019

Contributor

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

struct spm_partition_desc_t,
sp_thrd);

if (p_next_partition->static_data.partition_flags &

This comment has been minimized.

Copy link
@mark-edgeworth

mark-edgeworth Jun 18, 2019

Contributor

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

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

This comment has been minimized.

Copy link
@mark-edgeworth

mark-edgeworth Jun 18, 2019

Contributor

CURRENT_THREAD

}

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

This comment has been minimized.

Copy link
@mark-edgeworth

mark-edgeworth Jun 18, 2019

Contributor

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

}

/* 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)

This comment has been minimized.

Copy link
@mark-edgeworth

mark-edgeworth Jun 18, 2019

Contributor

thrd->thread

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

This comment has been minimized.

Copy link
@mark-edgeworth

mark-edgeworth Jun 18, 2019

Contributor

TFM_SVC_EXIT_THREAD


ctrl.w = __get_CONTROL();

if (privileged == TFM_PARTITION_PRIVILEGED_MODE) {

This comment has been minimized.

Copy link
@mark-edgeworth

mark-edgeworth Jun 18, 2019

Contributor

I'd prefer a ternary operator here.

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

This comment has been minimized.

Copy link
@mark-edgeworth

mark-edgeworth Jun 18, 2019

Contributor

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

This comment has been minimized.

Copy link
@Patater

Patater Jun 20, 2019

Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

@Patater could we have your review please ?

Copy link
Contributor

left a comment

  • 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?
#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)

This comment has been minimized.

Copy link
@Patater

Patater Jun 20, 2019

Contributor

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

This comment has been minimized.

Copy link
@devran01

devran01 Jun 26, 2019

Author Contributor

done

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

#ifndef TFM_PSA_API

This comment has been minimized.

Copy link
@Patater

Patater Jun 20, 2019

Contributor

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

This comment has been minimized.

Copy link
@devran01

devran01 Jun 26, 2019

Author Contributor

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

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

This comment has been minimized.

Copy link
@Patater

Patater Jun 20, 2019

Contributor

Please comment in our patch why this change is needed.

This comment has been minimized.

Copy link
@devran01

devran01 Jun 26, 2019

Author Contributor

done

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

This comment has been minimized.

Copy link
@alzix

alzix Jun 20, 2019

Contributor

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.

This comment has been minimized.

Copy link
@devran01

devran01 Jun 26, 2019

Author Contributor

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

@devran01

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

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

devran01 added 3 commits Jun 13, 2019
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 <devaraj.ranganna@arm.com>
when TFM_PSA_API is not set

Signed-off-by: Devaraj Ranganna <devaraj.ranganna@arm.com>
- 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 <devaraj.ranganna@arm.com>
@0xc0170

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

@devran01 Can you answer @Patater review?

@devran01

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

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

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 <devaraj.ranganna@arm.com>
@devran01 devran01 force-pushed the devran01:feature_trusted-firmware-m_e7efdc6 branch from 61e9f71 to 98fa63a Jun 26, 2019
@mbed-ci

This comment has been minimized.

Copy link

commented Jun 28, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

@0xc0170 CI failed. Can you please check?

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

@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
Patater approved these changes Jul 1, 2019
Copy link
Contributor

left a comment

LGTM

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

Set to 5.14 based on the changes and PR type.

@0xc0170 0xc0170 merged commit 1aad06b into ARMmbed:master Jul 1, 2019
26 checks passed
26 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-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(-96 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 Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8627 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8448B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
@0xc0170 0xc0170 removed the ready for merge label Jul 1, 2019
@devran01 devran01 deleted the devran01:feature_trusted-firmware-m_e7efdc6 branch Jul 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.