Skip to content

Call mbed_tfm_init before mbed_toolchain_init #13400

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 10, 2020
Merged

Call mbed_tfm_init before mbed_toolchain_init #13400

merged 1 commit into from
Sep 10, 2020

Conversation

gbrtth
Copy link
Contributor

@gbrtth gbrtth commented Aug 7, 2020

Summary of changes

This commit changes the boot sequence in mbed_start(), calling mbed_tfm_init()
before mbed_toolchain_init().

The reason for this changes is that there are platforms (Musca-B1, Musca-S1), which use
certain components (eg. GPIO) through TF-M ioctl services, and if mbed_toolchain_init() tries
to initialize these components through constructors before mbed_tfm_init() finishes, fault will
occur on the platforms.

Impact of changes

Migration actions required

Documentation

None

Pull request type 

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results 

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[X] Tests / results supplied as part of this PR

Reviewers

@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Aug 7, 2020
@ciarmcom ciarmcom requested review from a team August 7, 2020 19:00
@ciarmcom
Copy link
Member

ciarmcom commented Aug 7, 2020

@gbrtth, thank you for your changes.
@ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 10, 2020

@urutva I recall this was touched in the recent PRs, can you review this change please?

@0xc0170 0xc0170 requested review from jainvikas8 and urutva August 10, 2020 09:26
jainvikas8
jainvikas8 reviewed Aug 10, 2020
View reviewed changes
Copy link
Contributor

@jainvikas8 jainvikas8 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a common code:

  • Is this been tested with TF-M Dual V7 targets and non-TF-M targets?

The reason for this changes is that there are platforms (Musca-B1, Musca-S1), which use
certain components (eg. GPIO) through TF-M ioctl services, and if mbed_toolchain_init() tries
to initialize these components through constructors before mbed_tfm_init() finishes, fault will
occur on the platforms.

  • Is this issue reproducible on ARM_MUSCA_B1 with TF-M v1.1?

@gbrtth
Copy link
Contributor Author

gbrtth commented Aug 18, 2020 •
edited
Loading

This is a common code:

* Is this been tested with TF-M Dual V7 targets and non-TF-M targets?

The reason for this changes is that there are platforms (Musca-B1, Musca-S1), which use
certain components (eg. GPIO) through TF-M ioctl services, and if mbed_toolchain_init() tries
to initialize these components through constructors before mbed_tfm_init() finishes, fault will
occur on the platforms.

I cannot test them with TF-M Dual V7 or non-TF-M targets, because I do not have such platforms.

* Is this issue reproducible on `ARM_MUSCA_B1` with TF-M v1.1?

I need to cross-check it again. Let me get back to you once it is done.

@gbrtth
Copy link
Contributor Author

gbrtth commented Aug 25, 2020

This is a common code:

* Is this been tested with TF-M Dual V7 targets and non-TF-M targets?

The reason for this changes is that there are platforms (Musca-B1, Musca-S1), which use
certain components (eg. GPIO) through TF-M ioctl services, and if mbed_toolchain_init() tries
to initialize these components through constructors before mbed_tfm_init() finishes, fault will
occur on the platforms.

I cannot test them with TF-M Dual V7 or non-TF-M targets, because I do not have such platforms.

* Is this issue reproducible on `ARM_MUSCA_B1` with TF-M v1.1?

I need to cross-check it again. Let me get back to you once it is done.

We have managed to reproduce this issue on Musca B1. Please see the attached logs.
Musca_B1_mbed_run.zip

@jainvikas8
Copy link
Contributor

jainvikas8 commented Aug 25, 2020 •
edited
Loading

We have managed to reproduce this issue on Musca B1. Please see the attached logs.
Musca_B1_mbed_run.zip

Ok, please you could try - https://github.com/ARMmbed/mbed-os-tf-m-regression-tests/blob/master/README.md#execute-all-tests-for-arm_musca_b1 on the current PR fix applied to the Mbed OS. This is to make sure we are not breaking other tests with this fix.

Also please update the description, which test is having the issue, it would have made it clear that there was a test failing.

@mergify
Copy link

mergify bot commented Sep 2, 2020

This PR cannot be merged due to conflicts. Please rebase to resolve them.

@jainvikas8
Copy link
Contributor

@gbrtth

@mark-horvath-arm @gbrtth
Call mbed_tfm_init earlier
656e046 
Change-Id: I0f45425058bf5cabbda877463ff1d8f4d713be8f
Signed-off-by: Mark Horvath <mark.horvath@arm.com>
@gbrtth
Copy link
Contributor Author

gbrtth commented Sep 8, 2020

@gbrtth

* Cherry-picked this PR changes onto Mbed OS ([9073a48](https://github.com/ARMmbed/mbed-os/commit/9073a486675f474dfa3d08555b192f987c3b8683)) and tested TF-M and PSA compliance tests on `ARM_MUSCA_B1` using https://github.com/ARMmbed/mbed-os-tf-m-regression-tests repo.

* Please make documentation changes on this file - https://github.com/ARMmbed/mbed-os/blob/master/cmsis/device/rtos/include/mbed_boot.h to proceed with this PR.

Documentation is changed.

Patater
Patater approved these changes Sep 9, 2020
View reviewed changes
Copy link
Contributor

@Patater Patater left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mergify mergify bot added needs: CI and removed needs: work labels Sep 9, 2020
@Patater
Copy link
Contributor

Patater commented Sep 9, 2020

CI Started

@mbed-ci
Copy link

mbed-ci commented Sep 9, 2020

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_build-ARM ✔️
jenkins-ci/mbed-os-ci_build-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_dynamic-memory-usage ✔️
jenkins-ci/mbed-os-ci_cloud-client-pytest ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️

@0xc0170 0xc0170 merged commit 5940b16 into ARMmbed:master Sep 10, 2020
@mergify mergify bot removed the ready for merge label Sep 10, 2020
@mbedmain mbedmain removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jainvikas8 jainvikas8 jainvikas8 left review comments

@Patater Patater Patater approved these changes

@urutva urutva Awaiting requested review from urutva

Assignees
No one assigned
Labels
release-version: 6.3.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@gbrtth @ciarmcom @0xc0170 @jainvikas8 @Patater @mbed-ci @mbedmain @mark-horvath-arm