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

Permit non-TrustZone ARMv8 build #10520

Merged
merged 1 commit into from May 17, 2019

Conversation

Projects
None yet
7 participants
@kjbracey-arm
Copy link
Contributor

commented May 2, 2019

Description

Change the heuristic for selection of CMSE in the tools python, so that a non-TrustZone ARMv8 build can happen.

Ideally we would have more direct flagging in the targets, but this refines the heuristic so the necessary behaviour can be easily achieved.

  • DOMAIN_NS=1 is based purely on the -NS suffix on the core name.

  • Enabling CMSE in the compiler and outputting a secure import library is now enabled when the core doesn't have an -NS suffix by either the target label TFM being present or the flag trustzone being set.

This covers the existing ARMv8-M behaviour - TF-M builds have the TFM label, as per its documentation; M2351 secure builds have no explicit flagging, so we ensure that the M2351_NS target has the trustzone flag set, and the out-of-tree secure target inherits that.

Replacement for #10514, hopefully fixes #9460

Marked it as "fix", as I think it's quite reasonable to regard not being able to make non-TrustZone builds for current ARMs as a bug, although arguably it's a functionality change.

Pull request type

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

Reviewers

@jeromecoutant, @alzix, @bridadan, @bulislaw

Release Notes

  • Non-TrustZone ARMv8 targets now supported (core must be set to Cortex-M33 rather than Cortex-M33-NS).

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:build_tz_heuristic branch 2 times, most recently from 1219492 to 4b856de May 2, 2019


@property
def is_TrustZone_non_secure_target(self):
return self.core.endswith('-NS')

This comment has been minimized.

Copy link
@alzix

alzix May 2, 2019

Contributor
Suggested change
return self.core.endswith('-NS')
return not self.is_TrustZone_secure_target

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm May 2, 2019

Author Contributor

No, because it's not the opposite of secure - there are 3 cases:

  • TrustZone secure target
  • TrustZone non-secure target
  • non-TrustZone target (neither of the above)

(I believe that's the same as for PSA)

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

I've (finally) realised, after sufficient prodding by @jeromecoutant, this still doesn't cover the M2351 properly, as we do need to support a secure build for it - it's out of-tree, so I missed it.

https://github.com/OpenNuvoton/NuMaker-mbed-TZ-secure-example

So we just need to do something to get is_TrustZone_secure_target true for that. One of Jerome's suggestions was to look for MBED_TZ_DEFAULT_ACCESS=1 in the macros, and I can't see anything better, if our assumption is "change Python only, no change to targets JSON, in-tree or out".

@ciarmcom ciarmcom requested review from alzix, bridadan, bulislaw and ARMmbed/mbed-os-maintainers May 2, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented May 2, 2019

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:build_tz_heuristic branch 2 times, most recently from 49b4551 to 579b9a2 May 2, 2019

kwargs.get('build_dir', False)):
# Create Secure library
if target.is_TrustZone_secure_target:
if kwargs.get('build_dir', False):

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm May 2, 2019

Author Contributor

Do we really need this "if"? It isn't there in GCC and IAR versions.

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

Revised to now cover M2351, which did mean adding a specific flag in targets.json.

Adjusted TF-M documentation to not say "ARMv8-M" so much - the behaviour it's documenting is now activated by the presence of the "TFM" flag it says to add.

I don't believe we have any specific documentation about making non-TF-M TrustZone images like M2351 is doing. If there is, that would now need to state that it's the trustzone flag that needs to be turned on, rather than it being automatic because it's an ARMv8-M chip.

# having trustzone = false (default), no TFM flag, and no -NS suffix.
@property
def is_TrustZone_secure_target(self):
return (self.trustzone or 'TFM' in self.labels) and not self.core.endswith('-NS')

This comment has been minimized.

Copy link
@jeromecoutant

jeromecoutant May 2, 2019

Contributor

You could add
"trustzone": true,
in SPE_Target, and remove this TFM check here ?

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm May 2, 2019

Author Contributor

Sadly no, because some SPE_Targets are not using TrustZone. See CY8CKIT_062_WIFI_BT_M0_PSA.

Dual-CPU set-up with the secure and non-secure images running on different cores with message-passing.

I could add trustzone to all the ARMv8-M SPE_Targets individually, but that would need to be added to the documentation of creating a TF-M target, and I'm wary about breaking some out-of-tree target I haven't seen. This ensures that the existing instructions work.

And I'm not going to make it "SPE_Target and core arch >= 8", because there's no reason you can't run the same dual-cpu PSA setup with an M23 and M33 - if those guys want a slight CPU performance upgrade from M0+M4 without a complete system rearchitecture.

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm May 2, 2019

Author Contributor

I guess I'm also wary of documenting the trustzone flag as part of the TF-M instructions, if we think it might be subject to change. It's part of this being the "quick" fix - avoiding discussions about what the targets.json should look like.

If we're willing to commit to it, I could add it, so that the "detect TFM label" is then clearly legacy support - TFM targets are supposed to also have the trustzone flag set properly.

This comment has been minimized.

Copy link
@jeromecoutant

jeromecoutant May 3, 2019

Contributor

If you don't add trustzone to true in SPE_Target,
maybe we got a strange situation where trustzone value is kept to the default value, i.e. false?

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm May 6, 2019

Author Contributor

Yes, and that's strange if it is a TFM build. (Not if it isn't).

But the trustzone value is not directly exposed anywhere. The Target.is_TrustZone_xxx properties will give the right answer - looking at both that flag and the TFM label.

I can't add the flag to SPE_Target because not all SPE Targets are trustzone.

Although maybe I could and set it back to false in the exception cases, as it's TFM which is the "normal" case? So PSOC6 inherits from SPE_Target and flips trustzone back to false?

(I'm kind of playing games here trying to avoid changing any documentation or existing publicly visible behaviour so this can slip into 5.12.y as a "target update".)

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

With respect to @bridadan's core attribute proposal, I don't think this necessarily conflicts - the "trustzone" flag here can be viewed as a property of the build ("used"), rather than a property of the core ("present").

In discussions with the CMSIS guys, I was pointing out that for FPU and other things they do distinguish between "present" and "used", but they don't for CMSE/TrustZone. They're not currently planning to change that, but if they did, then any core attribute would be the "present", and this would be the "used".

We don't necessarily need to lock this form of the trustzone flag in yet - it really only exists for that M2351 build and its out-of-tree secure image, as there's no other reasonable way to make that keep working, and I believe we generally only intend to support TF-M anyway?

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:build_tz_heuristic branch from 579b9a2 to f9362d9 May 3, 2019

Permit non-TrustZone ARMv8 build
Change the heuristic for selection of CMSE in the tools python, so that
a non-TrustZone ARMv8 build can happen.

Ideally we would have more direct flagging in the targets, but this
refines the heuristic so the necessary behaviour can be easily
achieved.

* DOMAIN_NS=1 is based purely on the `-NS` suffix on the core name.

* Enabling CMSE in the compiler and outputting a secure import library
  is now enabled when the core doesn't have an `-NS` suffix by either
  the target label `TFM` being present or the flag `trustzone` being set.

This covers the existing ARMv8-M behaviour - TF-M builds have the TFM
label, as per its documentation; M2351 secure builds have no explicit
flagging, so we ensure that the M2351_NS target has the trustzone flag
set, and the out-of-tree secure target inherits that.

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:build_tz_heuristic branch from f9362d9 to 65e0887 May 3, 2019

@bulislaw
Copy link
Member

left a comment

Would be good to review the portign guide and document the different ways (non-)TrustZone targets can be added.

@adbridge adbridge added needs: CI and removed needs: review labels May 16, 2019

@adbridge

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

CI started

@adbridge

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

@kjbracey-arm Could you take a look at the test failures ?

@adbridge adbridge added needs: work and removed needs: CI labels May 16, 2019

@adbridge

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

Seems there have been some CI issues so I am going to restart the ci on this one to see if that fixes things.

@mbed-ci

This comment has been minimized.

Copy link

commented May 16, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_cloud-client-test
  • jenkins-ci/mbed-os-ci_dynamic-memory-usage
  • jenkins-ci/mbed-os-ci_greentea-test
@mbed-ci

This comment has been minimized.

Copy link

commented May 16, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_exporter
@adbridge

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

Re-running exporters

@adbridge adbridge merged commit 9fb4429 into ARMmbed:master May 17, 2019

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 Success
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 8561 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

@kjbracey-arm kjbracey-arm deleted the kjbracey-arm:build_tz_heuristic branch May 17, 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.