Skip to content

Avoid building mbed_tz_context.c for TF-M targets #9324

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 2 commits into from
Jan 14, 2019
Merged

Avoid building mbed_tz_context.c for TF-M targets #9324

merged 2 commits into from
Jan 14, 2019

Conversation

mikisch81
Copy link
Contributor

Description

TF-M v8m secure-side implements their own TZ context APIs so need to avoid building the Mbed implementation.

TARGET_TFM is a macro which will be set when #9221 is merged (Only for Secure targets).

Pull request type

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

Reviewers

@deepikabhavnani

@mikisch81 mikisch81 changed the title Avoid mbed tz context Avoid building mbed_tz_context.c for TF-M targets Jan 10, 2019
@mikisch81
Copy link
Contributor Author

@ARMmbed/mbed-os-core

@ciarmcom
Copy link
Member

@mikisch81, thank you for your changes.
@deepikabhavnani @ARMmbed/mbed-os-tools @ARMmbed/mbed-os-maintainers please review.

@ciarmcom ciarmcom requested review from deepikabhavnani and a team January 10, 2019 13:07
@mikisch81 mikisch81 mentioned this pull request Jan 10, 2019
Merged
deepikabhavnani
deepikabhavnani approved these changes Jan 10, 2019
View reviewed changes
Copy link

@deepikabhavnani deepikabhavnani left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

bridadan
bridadan approved these changes Jan 10, 2019
View reviewed changes
cmsis/TARGET_CORTEX_M/mbed_tz_context.c Outdated
@@ -22,6 +22,8 @@
* limitations under the License.
*/

#if !defined(TARGET_TFM)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think using the below would be more flexible (it also allows the builder to set the macro to 0/1 in addition to just being set).

Suggested change
#if !defined(TARGET_TFM)
#if !TARGET_TFM

@deepikabhavnani there was a PR about this recently, can you double check me on this?

Copy link

Choose a reason for hiding this comment

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

Thanks for catching this @bridadan. Yes we had a PR about this recently

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bridadan TFM will be added to the RAM_MUSCA_A1_S target inside extra_labels.
So it will generate #define TARGET_TFM 1 inside mbed_config.h? and for any other target #define TARGET_TFM 0?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mikisch81 yup, everything you said is correct and the way you have written it will work just fine! I was commenting because there was a previous PR that helped standardize this kind of #if check in the code base (#9163). If you accept my suggested change, it would bring your changes inline with that styling.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, and also this PR: https://github.com/ARMmbed/mbed-os/pull/9327/files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bridadan done

Michael Schwarcz added 2 commits January 10, 2019 18:06
CMSIS/CORTEX-M: Don't build mbed_tz_context.c in TF-M targets
d3f7abd 
- TF-M v8m secure-side implements their own TZ context APIs so need to avoid building the Mbed implementation.
Update cmsis_importer.json SHAs
1e8169d 
- Missing: CMSIS/RTX: Patch to conditionally compile (fb35475)
- New: CMSIS/CORTEX-M: Don't build mbed_tz_context.c in TF-M targets (d3f7abd)
@mikisch81
Copy link
Contributor Author

@cmonr as everyone approved, can this PR move to needs:ci?

@mikisch81
Copy link
Contributor Author

@cmonr as everyone approved, can this PR move to needs:ci?

@0xc0170

@NirSonnenschein
Copy link
Contributor

CI started

@mbed-ci
Copy link

mbed-ci commented Jan 13, 2019

Test run: SUCCESS

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

@0xc0170 0xc0170 merged commit c5014f4 into ARMmbed:master Jan 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deepikabhavnani deepikabhavnani deepikabhavnani approved these changes

@0xc0170 0xc0170 0xc0170 approved these changes

@bridadan bridadan bridadan approved these changes

@NirSonnenschein NirSonnenschein NirSonnenschein approved these changes

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

Successfully merging this pull request may close these issues.
7 participants
@mikisch81 @ciarmcom @NirSonnenschein @mbed-ci @0xc0170 @bridadan @deepikabhavnani