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

Add warning about FEATURE_UVISOR being deprecated #7000

Merged
merged 2 commits into from May 25, 2018

Conversation

Projects
None yet
9 participants
@danny4478
Contributor

danny4478 commented May 24, 2018

Description

Add warning about FEATURE_UVISOR being deprecated.
If FEATURE_UVISOR is being used (compiled), then a compilation warning is issued.

Pull request type

[ ] Fix
[ x ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-psa May 24, 2018

#define __UVISOR_DEPRECATION_H__
#if defined(UVISOR_PRESENT) && UVISOR_PRESENT == 1
#warning "---- WARNING: You are using FEATURE_UVISOR which is deprecated!! ----"

This comment has been minimized.

@0xc0170

0xc0170 May 24, 2018

Member

You are using FEATURE_UVISOR which is deprecated since mbed-os-5.9 might be better - follow what we do for API - since what release it is deprecated.

Lets wait for @AnotherButler review

This comment has been minimized.

@adbridge

adbridge May 24, 2018

Contributor

Agree with @0xc0170 Uvisor is NOT yet deprecated. The warning should state that the feature is going to be deprecated in 5.10...

This comment has been minimized.

@iriark01

iriark01 May 24, 2018

Contributor

Can we hold off on warning messages? We're reviewing all of them. Also saying "it's deprecated' without saying what to use instead leaves the user with nothing.

@simonfordarm

This comment has been minimized.

@0xc0170

0xc0170 May 24, 2018

Member

Can we hold off on warning messages? We're reviewing all of them. Also saying "it's deprecated' without saying what to use instead leaves the user with nothing.

@alzix is trying to resolve this to add this here to the message and to the commit message as well.

Can we hold off on warning messages?

What do you suggest?

@0xc0170 0xc0170 requested a review from AnotherButler May 24, 2018

@adbridge adbridge self-requested a review May 24, 2018

@danny4478 danny4478 force-pushed the kfnta:uvisor_depr branch from ebcab5f to aa38778 May 24, 2018

@adbridge

This comment has been minimized.

Contributor

adbridge commented May 24, 2018

@danny4478 I just spoke to Sam Taylor about this deprecation. He is of the opinion that we have to be very careful with this deprecation message and the text needs to be agreed by Nicholas and probably Aaron also before being submitted to the code base.

@danny4478

This comment has been minimized.

Contributor

danny4478 commented May 24, 2018

@ndevillard please advise

#define __UVISOR_DEPRECATION_H__
#if defined(UVISOR_PRESENT) && UVISOR_PRESENT == 1
#warning "---- WARNING: You are using FEATURE_UVISOR which is deprecated since mbed-os-5.9!! ----"

This comment has been minimized.

@AnotherButler

AnotherButler May 24, 2018

Contributor

Perhaps something like:

"Warning: In 5.10, Arm Mbed uVisor is being deprecated in favor of our upcoming security features. We strongly encourage you to use these new security features instead."

I'd like to link to mention and link to PSA or PSA crypto, but I don't know if linking to a private repo makes sense.

@ndevillard @ChiefBureaucraticOfficer Please advise.

This comment has been minimized.

@AnotherButler

AnotherButler May 24, 2018

Contributor

After talking with @ChiefBureaucraticOfficer we suggest "Warning: You are using FEATURE_UVISOR, which is unsupported as of Mbed OS 5.9."

@ndevillard @simonfordarm What are your thoughts on this phrasing?

@cmonr cmonr added needs: CI and removed needs: review labels May 25, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 25, 2018

Due to having not seen a deprecation warning be agreed upon in the last nine hours, I've taken @AnotherButler's most recent suggestion and updated this PR with the message.

Going to put this through CI to make sure no surprises come up with the file addition, but will hold off on merging until key parties agree on the text. Also intend on taking advantage of the low CI utilization at the moment.

Fyi, a change to the text (aka another commit) will require CI again.

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented May 25, 2018

Build : SUCCESS

Build number : 2145
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7000/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 25, 2018

/morph test

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added needs: review and removed needs: CI labels May 25, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 25, 2018

CI completed, this is now almost green to get in.

Reviewers - approvals?

@adbridge

This comment has been minimized.

Contributor

adbridge commented May 25, 2018

Looking at the email from @ndevillard this warning is insufficient.

Also uvisor will still be in 5.9 so won't actually be fully deprecated until 5.10.
I'm wondering if it should say something like:

"uvisor will be deprecated in the 5.10 release and will be replaced in due course by a PSA-compliant Secure Partition Manager"

@adbridge adbridge added the risk: A label May 25, 2018

@adbridge

This comment has been minimized.

Contributor

adbridge commented May 25, 2018

Agreed with @ChiefBureaucraticOfficer that this can go in as is for RC1 and we will improve it for RC2

@cmonr cmonr merged commit a348b45 into ARMmbed:master May 25, 2018

13 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/astyle Passed, 906 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10065 cycles (-158 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/littlefs Passed, code size is 9964B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
@adbridge

This comment has been minimized.

Contributor

adbridge commented May 31, 2018

Looking at the email from @ndevillard this warning is insufficient.
I'm wondering if it should say something like:
"uvisor will be deprecated in the 5.10 release and will be replaced in due course by a PSA-compliant >Secure Partition Manager"
Agreed with @ChiefBureaucraticOfficer that this can go in as is for RC1 and we will improve it for RC2

@danny4478 @ndevillard
Can we please have a PR with the improved warning text asap so that we can get it into RC2 as agreed?

@iriark01

This comment has been minimized.

Contributor

iriark01 commented May 31, 2018

Warning: uVisor is superseded by the Secure Partition Manager (SPM) defined in the ARM Platform Security Architecture (PSA). uVisor is deprecated as of Mbed OS 5.9, and being replaced by a native PSA-compliant implementation of SPM.

I think you can use that one

@alzix

This comment has been minimized.

Contributor

alzix commented May 31, 2018

@orenc17 please handle asap

@alzix alzix deleted the kfnta:uvisor_depr branch May 31, 2018

@orenc17

This comment has been minimized.

Contributor

orenc17 commented May 31, 2018

Created #7072

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment