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

M2351: Enhance secure/non-secure image build flow for non-PSA target #11288

Merged
merged 6 commits into from Sep 17, 2019

Conversation

@ccli8
Copy link
Contributor

commented Aug 22, 2019

Description

This PR is to enhance secure/non-secure image build flow for non-PSA target. Its update is abstracted from #10959 just for the non-PSA part to improve build UX. It is expected that this PR and #10959 are compatible for the build flow. It has the following modifications:

  1. Rename non-PSA target name to NUMAKER_PFM_M2351_NOPSA_S/NS
  2. Support non-PSA secure/non-secure combined build

The above modifications enhance build flow but at the same time cause incompatibility with current one:

  1. The target name NUMAKER_PFM_M2351 is recycled. User needs to change to NUMAKER_PFM_M2351_NOPSA_S/NS to build non-PSA secure/non-secure targets.
  2. Just flash combined secure/non-secure image altogether instead of flash secure image first and then non-secure image.
  3. User can also drop pre-built secure image saved in TARGET_NU_PREBUILD_SECURE directory and provide its own by adding the line below in mbed_app.json instead of removing it through .mbedignore:
    "target.extra_labels_remove": ["NU_PREBUILD_SECURE"]

Pull request type

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

Release Notes

Changes for NuMaker-PFM-M2351:

  1. Change target name to NU_PFM_M2351_NPSA_S for non-PSA secure target and and NU_PFM_M2351_NPSA_NS for non-PSA non-secure target. Original target name NUMAKER_PFM_M2351 is recycled.
  2. Combine secure image and non-secure image into one in non-secure target post-build. With this, user just needs to flash the combined image instead of flash secure image and then non-secure image separately.
  3. To drop pre-built secure image in mbed-os tree and provide custom one, add the line '"target.extra_labels_remove": ["NU_PREBUILD_SECURE"]' into 'mbed_app.json' instead of via .mbedignnore.
@ciarmcom ciarmcom requested review from Ronny-Liu and ARMmbed/mbed-os-maintainers Aug 22, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

@ccli8, thank you for your changes.
@Ronny-Liu @ARMmbed/mbed-os-maintainers @ARMmbed/mbed-os-tools please review.

@ccli8

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

Any update?

@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

Rename non-PSA target name to NUMAKER_PFM_M2351_NOPSA_S/NS

Can this be non breaking change or this target has not been released ?

Please add release notes section above.

@0xc0170 0xc0170 requested a review from bulislaw Aug 26, 2019
@ccli8

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

Add Release Notes section.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

Thanks, what is the impact of this change ? What are we breaking ? My previous question still stand: "Can this be non breaking change or this target has not been released ?"

We should be aware of the scope of breakages. Are there any migration steps needed?

@ccli8

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

Can this be non breaking change or this target has not been released ?

This target has been released with target name NUMAKER_PFM_M2351. But to support combined build and further TF-M #10959, target name must be changed. So this PR must be breaking change. The related changes user needs to follow are as Release Notes section describes.

@bulislaw

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

I'm not big fan of the breaking changes, even if it's only for 1 target. Why do we add nopsa to the target name? That would make me think there's a PSA version. Also what does nu in NU_PREBUILD_SECURE stands for?

@devran01 @Patater please review

@0xc0170 0xc0170 requested review from Patater and devran01 Aug 27, 2019
@ccli8

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2019

I'm not big fan of the breaking changes, even if it's only for 1 target. Why do we add nopsa to the target name? That would make me think there's a PSA version.

Yes, there are also PSA targets. #10959 is on-going for it. Actually, there will be four target names for the NuMaker-PFM-M2351 board:

  • NUMAKER_PFM_M2351_S: Target name for building M2351 TFM secure code
  • NUMAKER_PFM_M2351_NS: Target name for building M2351 TFM non-secure code
  • NUMAKER_PFM_M2351_NOPSA_S: Target name for building M2351 non-TFM secure code
  • NUMAKER_PFM_M2351_NOPSA_NS: Target name for building M2351 non-TFM non-secure code

#10959 is blocked with TF-M integration. This PR is an excerpt of #10959 to support combined build on non-PSA target in advance.

Also what does nu in NU_PREBUILD_SECURE stands for?

For TZ target, prebuilt secure image will place in mbed-os tree. In normal cases, user just needs to build non-secure code. But if user doesn't want the prebuilt secure image and wants to provide his own for e.g. ROM/RAM re-partition on NuMaker-PFM-M2351, he needs to remove the prebuilt one in mbed-os tree. NU_PREBUILD_SECURE is the means for it.

@mark-edgeworth

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

These target names may be too long for the database that stores them (there is a 20 character limit I believe)

@ccli8

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2019

These target names may be too long for the database that stores them (there is a 20 character limit I believe)

@mark-edgeworth Which database? Can this be released?

@@ -8353,8 +8353,7 @@
"macros_add": ["CMSDK_CM7"],
"device_has_add": ["MPU"]
},
"NUMAKER_PFM_M2351": {
"core": "Cortex-M23-NS",
"NUMAKER_PFM_M2351_CM": {

This comment has been minimized.

Copy link
@madchutney

madchutney Aug 28, 2019

Contributor

This change needs to be replicated into the online database (please ask you TAM representative) and the platform database in https://github.com/ARMmbed/mbed-os-tools

This comment has been minimized.

Copy link
@ccli8

ccli8 Aug 29, 2019

Author Contributor

This change needs to be replicated into the online database (please ask you TAM representative)

OK

the platform database in https://github.com/ARMmbed/mbed-os-tools

The platform name NUMAKER_PFM_M2351 in mbed-os-tools database is kept unchanged for compatibility. Don't want to meet mismatch issue with mbed-os-tools and mbed-os.

This comment has been minimized.

Copy link
@madchutney

madchutney Sep 2, 2019

Contributor

I'm not sure what your comments mean, can you explain? The names need the same in all places or the entire build system, testing and reporting will not work.

mbed-os-tools defines the board type when the product/detect code from the board is detected. This means that some systems will attempts to build based on this board type, which will not be present in mbed-os.

This comment has been minimized.

Copy link
@ccli8

ccli8 Sep 3, 2019

Author Contributor

@madchutney Originally, I want to keep the M2351 platform name in mbed-os-tools unchanged, so users don't need to care about version match between mbed-os and mbed-os-tools. Now that the name must be consistent across different tools, I will send another PR to mbed-os-tools repo to change platform name to NU_PFM_M2351 from NUMAKER_PFM_M2351 after this PR is confirmed. bcf8cc6 is addressing the target name/platform name conversion for M2351 like other PSA targets.

"mbed_ram_size" : "0x10000"
"public": false
},
"NUMAKER_PFM_M2351_NOPSA_NS": {

This comment has been minimized.

Copy link
@madchutney

madchutney Aug 28, 2019

Contributor

This name is too long as it also needs to be in the online database (max 20 characters)

This comment has been minimized.

Copy link
@ccli8

ccli8 Aug 29, 2019

Author Contributor

Fixed

"mbed_ram_start" : "0x30008000",
"mbed_ram_size" : "0x10000"
},
"NUMAKER_PFM_M2351_NOPSA_S": {

This comment has been minimized.

Copy link
@madchutney

madchutney Aug 28, 2019

Contributor

This name is too long as it also needs to be in the online database (max 20 characters)

This comment has been minimized.

Copy link
@ccli8

ccli8 Aug 29, 2019

Author Contributor

Fixed

@mark-edgeworth

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

These target names may be too long for the database that stores them (there is a 20 character limit I believe)

@mark-edgeworth Which database? Can this be released?

I'm sorry, no. See the inline review comments for why (@madchutney is responding now).

assert os.path.isfile(ns_hex), "Non-secure image %s must be regular file" % s_hex

# Keep original non-secure before merge with secure
ns_nosecure_hex = _ + "_no-secure-merge" + ext

This comment has been minimized.

Copy link
@madchutney

madchutney Aug 28, 2019

Contributor

Please don't use _ as variable name, convention says this can be used when a return value is not required but then to actually use it is wrong.

This comment has been minimized.

Copy link
@ccli8

ccli8 Aug 29, 2019

Author Contributor

Fixed

t_self.notify.info("Found secure image %s" % s_hex)

_, ext = os.path.splitext(s_hex)
assert ext == ".hex", "Secure image %s must be in Intel HEX format" % s_hex

This comment has been minimized.

Copy link
@madchutney

madchutney Aug 28, 2019

Contributor

This assert does not seem to be checking for a programming assertion but rather a test for the file retrieved.

This comment has been minimized.

Copy link
@ccli8

ccli8 Aug 29, 2019

Author Contributor

Fixed

@ccli8

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2019

For target name exceeding 20 characters limited by on-line, I will change to:

  • NU_PFM_M2351_P_S: Target name for building M2351 TFM secure code
  • NU_PFM_M2351_P_NS: Target name for building M2351 TFM non-secure code
  • NU_PFM_M2351_NP_S: Target name for building M2351 non-TFM secure code
  • NU_PFM_M2351_NP_NS: Target name for building M2351 non-TFM non-secure code

Their base name NUMAKER_PFM_M2351_CM will rename to NU_PFM_M2351_CM.

@0xc0170 0xc0170 added needs: work and removed needs: review labels Aug 28, 2019
@ccli8 ccli8 force-pushed the OpenNuvoton:nuvoton_m2351_comb-sec-nonsec branch from a2d47e7 to 5a87dbb Aug 29, 2019
@ccli8

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

Make modifications:

  1. Do rebase
  2. Rename target to NU_PFM_M2351+SUFFIX rather than NUMAKER_PFM_M2351+SUFFIX due to max 20 chars limit in online database
  3. Translate target name NU_PFM_M2351+SUFFIX to platform name NUMAKER_PFM_M2351 (tools/test_api.py), which has registered in mbed-os-tools and keeps unchanged for compatibility.
@ccli8

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2019

Any update?

ccli8 added 6 commits Aug 22, 2019
This will add back immediately after target renaming is done.
1.  Create a private target name NU_PFM_M2351_CM which stands for the
    NuMaker-PFM-M2351 board and is to be extended.
2.  NU_PFM_M2351_NPSA_S/NS target names for non-PSA secure/non-secure targets
    respectively.
3.  The original target name NUMAKER_PFM_M2351 is recycled and cannot be used.
    Use NU_PFM_M2351_S/NS for non-PSA secure/non-secure targets instead.

NOTE:   Target name doesn't follow the rule below because online database has
        limit of max 20 chars:

        NUMAKER_PFM_M2351_PSA/NOPSA_S/NS

        Instead, it has the rule:

        NU_PFM_M2351_[NPSA_]S/NS

        NU_PFM_M2351_S/NS for PSA targets. This is to be consistent with current
        PSA target naming. So the resolved target names are:

        NU_PFM_M2351_S          : PSA secure target
        NU_PFM_M2351_NS         : PSA non-secure target
        NU_PFM_M2351_NPSA_S     : Non-PSA secure target
        NU_PFM_M2351_NPSA_NS    : Non-PSA non-secure target
Support secure/non-secure combined build for non-PSA target:
1.  In secure post-build, deliver built secure image to TARGET_NU_PREBUILD_SECURE
    directory which is to combine later.
2.  In non-secure post-build, merge non-secure image with secure image saved in
    TARGET_NU_PREBUILD_SECURE directory.
3.  In non-secure post-build, user can also drop pre-built secure image saved in
    TARGET_NU_PREBUILD_SECURE directory and provide its own by adding the line below
    in mbed_app.json:
    "target.extra_labels_remove": ["NU_PREBUILD_SECURE"]
NOTE1:  USB UART is partitioned for non-secure world. Secure world still can share
        it with limit that its interrupt cannot use in secure world.
NOTE2:  In secure world, USB UART is only for Greentea and STDIO. Developers shouldn't
        use it for other purposes.
This is necessary for exporting M2351 uvision project.
@ccli8 ccli8 force-pushed the OpenNuvoton:nuvoton_m2351_comb-sec-nonsec branch from b1ca3e1 to 0ed126b Sep 16, 2019
@ccli8

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

Make modifications:

  1. Do rebase
  2. Rename targets for #11288 (comment). This change requires #11487 @devran01
NU_PFM_M2351_S: Target name for building M2351 TFM secure code
NU_PFM_M2351_NS: Target name for building M2351 TFM non-secure code
NU_PFM_M2351_NPSA_S: Target name for building M2351 non-TFM secure code
NU_PFM_M2351_NPSA_NS: Target name for building M2351 non-TFM non-secure code
@0xc0170 0xc0170 added needs: review and removed needs: work labels Sep 16, 2019
@devran01

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

@ccli8 Thanks for the modifications.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

Rename targets for #11288 (comment). This change requires #11487 @devran01

As soon as dependency is in, we can restart CI here !

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Sep 17, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

CI started

dependency is in!

@mbed-ci

This comment has been minimized.

Copy link

commented Sep 17, 2019

Test run: FAILED

Summary: 2 of 4 test jobs failed
Build number : 4
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_build-IAR
@0xc0170

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

CI restarted

@mbed-ci

This comment has been minimized.

Copy link

commented Sep 17, 2019

Test run: SUCCESS

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

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Sep 17, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

This is now ready for integration

@0xc0170 0xc0170 merged commit ffbd92c into ARMmbed:master Sep 17, 2019
25 checks passed
25 checks passed
continuous-integration/jenkins/pr-head This commit looks good
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(+0 bytes) RAM(-36 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 8673 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 8464B.
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
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.