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

platform reset API rename #9997

Merged
merged 1 commit into from Mar 11, 2019

Conversation

@alzix
Copy link
Contributor

commented Mar 7, 2019

Description

Renamed newly added psa_system_reset() API to mbed_system_reset()
We should not use PSA prefix for APIs not defined in PSA specs.

Must be merged after #9910

Targeting RC2

Pull request type

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

Reviewers

@mikisch81 @orenc17 @mmahadevan108

Release Notes

@ciarmcom ciarmcom requested review from maclobdell, MarceloSalazar, mikisch81, orenc17 and ARMmbed/mbed-os-maintainers Mar 7, 2019

components/TARGET_PSA/inc/psa/lifecycle.h Outdated
@@ -63,9 +64,11 @@ psa_status_t mbed_psa_reboot_and_request_new_security_state(uint32_t new_state);


/** \brief Resets the system
*
*
* PSA targets do not allow NPS to access system power domain.

This comment has been minimized.

Copy link
@mikisch81

mikisch81 Mar 7, 2019

Contributor

NPS ->NSPE

components/TARGET_PSA/inc/psa/lifecycle.h Outdated
*
*
* PSA targets do not allow NPS to access system power domain.
* This API requests system reset to be caried out by SPE once all critical secure tasks are finished.

This comment has been minimized.

Copy link
@mikisch81

mikisch81 Mar 7, 2019

Contributor

carried

@orenc17
Copy link
Contributor

left a comment

please align with psa/errors.h

@alzix

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

@orenc17, I agree that service should be revisited and adapted to use psa/errors.h. it was written before psa/errors.h was introduced. But as the errors are all internal (the API returns void or actually do not returns at all) it can be post 5.12

@alzix

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

@orenc17 @mikisch81 please review latest changes.
updated error.h usage
fixed compilation warnings

@0xc0170 0xc0170 added needs: work and removed needs: review labels Mar 8, 2019

@alzix alzix force-pushed the kfnta:alzix/platform_rename branch Mar 8, 2019

@alzix

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

rebased on top #9910

@alzix

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

A bit context for this PR: this PR renames an API added couple of days ago. It was not part of any release yet and is not expected to effect any external developer.
This API used to implement NVIC Reset on ARMv8-M targets on nonsrcure side.
It is currently used only by NXP port, thus this PR should come after NXP PR is merged as it tweaks the NXP sources. For this very reason @mmahadevan108 is on the CC list

@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

It is currently used only by NXP port, thus this PR should come after NXP PR is merged as it tweaks the NXP sources. For this very reason @mmahadevan108 is on the CC list

This needs rebase now, dependency was merged.

@alzix alzix force-pushed the kfnta:alzix/platform_rename branch Mar 9, 2019

@alzix

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2019

rebased and squashed

@alzix alzix force-pushed the kfnta:alzix/platform_rename branch Mar 10, 2019

@alzix

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2019

fixed astyle

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 10, 2019

Test run: SUCCESS

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

@alzix

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2019

all green - can we merge it?
please note - if this PR merged before #9996, a minor change in #9996 will be required.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 10, 2019

please note - if this PR merged before #9996, a minor change in #9996 will be required.

It's merged, all good here?

@0xc0170
Copy link
Member

left a comment

why mbed_system_reset is not in platform, and accessible to all ? The name leads me to believe this is common API.

@orenc17

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2019

@alzix please rebase and edit TESTS/psa/attestation/main.cpp accordingly

@alzix alzix force-pushed the kfnta:alzix/platform_rename branch Mar 11, 2019

@alzix

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

@0xc0170

why mbed_system_reset is not in platform, and accessible to all ? The name leads me to believe this is common AP

on ARMv8-M architecture system reset can only be performed from TZ. This API provides an entry point to TZ. We cannot move this API to platform as it is a PSA service and better reside with rest of PSA sources, for the sake of not adding further compilation tweaks

@alzix

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

Rebased and fixed attestation test.

@NirSonnenschein, can we start CI? We need make sure it will be merged before 5.12 official?

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

CI started on latest fixes

@0xc0170
Copy link
Member

left a comment

Line 57 - todo in lifecycle needs to be removed

I've checked lifecycle - 3 functions, 3 different prefixes

psa_...
mbed_psa...
mbed_...

does each one has special meaning ? why not all mbed_psa or just psa?

@0xc0170 0xc0170 added needs: work and removed needs: review labels Mar 11, 2019

Rename psa_system_reset to mbed_psa_system_reset
add noreturn attributes
update lifecycle service to use psa/error.h
fix doxygen

@alzix alzix force-pushed the kfnta:alzix/platform_rename branch to 661613c Mar 11, 2019

@alzix

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

@0xc0170, solid points on the API names and doxygen

API names - psa_ prefix are for APIs defined in PSA specs. We need these APIs as is for compliance.
mbed_psa APIs are required but are not standardised ant left IMPDEF. We must not use psa_ prefix for these.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

Restarting CI (earlier run will be aborted)

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Mar 11, 2019

@0xc0170 0xc0170 removed request for ARMmbed/mbed-os-tools Mar 11, 2019

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 11, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test
  • jenkins-ci/mbed-os-ci_exporter
@mbed-ci

This comment has been minimized.

Copy link

commented Mar 11, 2019

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 3
Build artifacts

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Mar 11, 2019

@0xc0170 0xc0170 removed request for ARMmbed/mbed-os-core Mar 11, 2019

@0xc0170 0xc0170 merged commit 525d463 into ARMmbed:master Mar 11, 2019

28 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-ARMC5 Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR8 Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARMC5 Success
Details
jenkins-ci/mbed2-build-ARMC6 Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR8 Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9135 cycles (-32 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
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.