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

Add support for cmse_nonsecure_entry attribute #75810

Merged
merged 3 commits into from
Sep 30, 2020

Conversation

hug-dev
Copy link
Contributor

@hug-dev hug-dev commented Aug 22, 2020

This pull request adds the cmse_nonsecure_entry attribute under an unstable feature.

I was not sure if it was fine for me to send directly the pull-request or if I should submit a RFC first. I was told on Zulip that it was fine to do so but please close it if I need first submit a RFC or follow another process instead.

The cmse_nonsecure_entry attribute is a LLVM attribute that will be available in LLVM 11. I plan to rebase on the upgrade PR once merged to make this one compile.

This attribute modifies code generation of the function as explained here to make it work with the TrustZone-M hardware feature. This feature is only available on thumbv8m targets so I created an error for that if one tries to use this attribute for another target.

I added this attribute in Rust as any other LLVM attribute are added but since this one is target-dependent I am not sure if it was the best thing to do. Please indicate me if you think of other ways, like isolating target-dependent attributes together.


Tracking issue: #75835

@rust-highfive
Copy link
Collaborator

Some changes occurred in diagnostic error codes

cc @GuillaumeGomez

@rust-highfive
Copy link
Collaborator

r? @oli-obk

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 22, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Aug 22, 2020

r? @jonas-schievink can you take this one? (if not, reassign to me)

Copy link
Contributor

@jonas-schievink jonas-schievink left a comment

Choose a reason for hiding this comment

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

Compiler changes look basically good to me. The whole PR is so small that I think we can add this feature without an RFC/MCP/FCP/whatever, but stabilizing the feature will require more care.

I think a few small things need to be done before this can land:

  • The PR needs a rebase to fix the merge conflicts.
  • The new feature needs to be added to and documented in the unstable book (src/doc/unstable-book).
  • A UI test should be added that ensures that the error you added is emitted.
  • A tracking issue needs to be opened.

It would also be good to figure out more about plans for CMSE support in Rust. For example, is this the only thing that needs compiler support to allow writing privileged Rust apps that expose an interface for unprivileged code, or is more needed? If more language features are needed, it would be a good idea to coordinate with the language team to figure out the bigger picture before implementing more things.

src/librustc_middle/middle/codegen_fn_attrs.rs Outdated Show resolved Hide resolved
@hug-dev
Copy link
Contributor Author

hug-dev commented Aug 22, 2020

Great, thanks a lot for the quick and helpful review 👍

The PR needs a rebase to fix the merge conflicts.

As soon as #73526 is merged, I will rebase on top of that as the new LLVM version is needed.

The new feature needs to be added to and documented in the unstable book (src/doc/unstable-book).
A UI test should be added that ensures that the error you added is emitted.

Sure, will add those things!

A tracking issue needs to be opened.

Should I add it now and use the issue number in this PR? With the title: "Tracking issue for the cmse_nonsecure_entry attribute"?

It would also be good to figure out more about plans for CMSE support in Rust. For example, is this the only thing that needs compiler support to allow writing privileged Rust apps that expose an interface for unprivileged code, or is more needed? If more language features are needed, it would be a good idea to coordinate with the language team to figure out the bigger picture before implementing more things.

For the Secure code to expose the Non-Secure Entry functions, only this attribute is needed. The rest of the work is done by the linker. It is worth to note that as far as I know LLD currently does not support CMSE so that one must use arm-none-eabi-ld in their .cargo/config scripts. Adding the --cmse-implib --out-implib=veneers.o options to ld will make it generate the veneers.o file that can be passed to Non-Secure.

There is also another attribute to be implemented, cmse_nonsecure_call which has the specificity to be a call site attribute. You can use it from the Secure code to be able to call a Non-Secure function pointer like:

fn non_secure_callback(callback: fn(i32) -> i32) {
    let val = (#[cmse_nonsecure_call] callback)(42);
}

This would need a new Rust feature to be able to add call site attributes. This is also gated by the stmt_expr_attributes feature which is not stable yet. I have made a local version that works but only for this specific attribute. I can make a RFC/MCP/FCP to propose a design for this feature and have cmse_nonsecure_call be the first example?

The other things needed to build a TrustZone-M application are mostly in cortex-m already: abstraction over the TT* instructions and Secure Attribution Unit (SAU) support. I am planning to make a blog post to summarize all those features showing an example.

@jonas-schievink
Copy link
Contributor

Sounds good!

This would need a new Rust feature to be able to add call site attributes. This is also gated by the stmt_expr_attributes feature which is not stable yet. I have made a local version that works but only for this specific attribute. I can make a RFC/MCP/FCP to propose a design for this feature and have cmse_nonsecure_call be the first example?

This is getting somewhat off-topic, but this sounds like something that belongs in the function's ABI instead of each call site, which would make it way simpler to implement. Maybe we should discuss this on Zulip?

@hug-dev hug-dev marked this pull request as ready for review August 23, 2020 12:33
@hug-dev
Copy link
Contributor Author

hug-dev commented Aug 23, 2020

I should have adressed all of your comments and created #75835 as a tracking issue.

5.4](https://developer.arm.com/documentation/ecm0359818/latest/) for details).
With this attribute, the compiler will do the following:
* add a special symbol on the function which is the `__acle_se_` prefix and the
standard function name
Copy link
Contributor

Choose a reason for hiding this comment

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

Does that mean the function's name will be mangled differently or is this an aliased or additional symbol?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this is an additional symbol. So that if you set this attribute on a function named toto, the compiler will generate the following assembly:

toto:
__acle_se_toto:
    INSTRUCTIONS

and the toto symbol will be set as weak. That way the linker can create the veneer like so:

toto:
    SG
    B.W __acle_se_toto

and the symbol toto and its address is passed to NS.

This is explained section 3.4.4 in here.

I don't think it modifies the mangling. However, as you can see in the examples in this PR, I believe you would always use the no_mangle and extern "C" ABI on the entry functions as they go through FFI.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I think we should definitely forbid using this attribute with non-"C" ABI functions then. Since the Rust ABI is unstable, it can pass arguments either directly in registers, or in memory, and that would be a problem here.

Copy link
Contributor Author

@hug-dev hug-dev Sep 30, 2020

Choose a reason for hiding this comment

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

Yes that makes sense, I am looking if there is a way to check what the ABI of the function compiled is.

[edit]: and add tests with and without the C ABI as well!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was easier than anticipated 😃 Added a new error code for that.

@bors
Copy link
Contributor

bors commented Aug 30, 2020

☔ The latest upstream changes (presumably #74862) made this pull request unmergeable. Please resolve the merge conflicts.

@nihalpasham
Copy link

nihalpasham commented Sep 4, 2020

Thought this may be useful to folks interested in the PR. I published a small medium post, summarizing TZ for ARMv8-M (as a future reference to myself).

https://bit.ly/3jMji9g

@jyn514 jyn514 added A-codegen Area: Code generation requires-nightly This issue requires a nightly compiler in some way. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Sep 15, 2020
Secure entry functions do not support if arguments are passed on the
stack. An "unsupported" diagnostic will be emitted by LLVM if that is
the case.
This commits adds support in Rust for that diagnostic so that an error
will be output if that is the case!

Signed-off-by: Hugues de Valon <hugues.devalon@arm.com>
@hug-dev
Copy link
Contributor Author

hug-dev commented Sep 30, 2020

Hey @jonas-schievink 👋 !

Sorry for the force-push but since when rebasing I realised that everything changed under the feet of this PR, I gathered I might as well 😅

I added a first commit that updates the LLVM submodule to the one that includes the bug fix. The bug fix is there but when creating an entry function which requires arguments being passed on stack, it throws an "unsupported" diagnostic which was not supported in Rust ABI. I added support for it in the first commit as well.

Also added a test for that case, the error message:

error: <unknown>:0:0: in function entry_function i32 (i32, i32, i32, i32, i32): secure entry function requires arguments on stack

weirdly lacks location information but at least you have the function name so I thought it was enough for now.

I don't know how to pass that UI test locally though so I am sure it will fail on the CI 😢

This patch adds support for the LLVM cmse_nonsecure_entry attribute.
This is a target-dependent attribute that only has sense for the
thumbv8m Rust targets.
You can find more information about this attribute here:
https://developer.arm.com/documentation/ecm0359818/latest/

Signed-off-by: Hugues de Valon <hugues.devalon@arm.com>
Copy link
Contributor

@jonas-schievink jonas-schievink left a comment

Choose a reason for hiding this comment

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

Nice! Left some more comments.

src/test/ui/cmse-nonsecure-entry/params-on-stack.rs Outdated Show resolved Hide resolved
src/test/ui/cmse-nonsecure-entry/params-on-stack.rs Outdated Show resolved Hide resolved
src/test/ui/cmse-nonsecure-entry/trustzone-only.rs Outdated Show resolved Hide resolved
Signed-off-by: Hugues de Valon <hugues.devalon@arm.com>
@jonas-schievink
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Sep 30, 2020

📌 Commit 2588287 has been approved by jonas-schievink

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 30, 2020
@bors
Copy link
Contributor

bors commented Sep 30, 2020

⌛ Testing commit 2588287 with merge 867bd42...

@bors
Copy link
Contributor

bors commented Sep 30, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: jonas-schievink
Pushing 867bd42 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 30, 2020
@bors bors merged commit 867bd42 into rust-lang:master Sep 30, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 30, 2020
@hug-dev hug-dev deleted the cmse-nonsecure-entry branch September 30, 2020 21:41
@nihalpasham
Copy link

There is also another attribute to be implemented, cmse_nonsecure_call which has the specificity to be a call site attribute. You can use it from the Secure code to be able to call a Non-Secure function pointer like:

fn non_secure_callback(callback: fn(i32) -> i32) {
    let val = (#[cmse_nonsecure_call] callback)(42);
}

This would need a new Rust feature to be able to add call site attributes. This is also gated by the stmt_expr_attributes feature which is not stable yet. I have made a local version that works but only for this specific attribute. I can make a RFC/MCP/FCP to propose a design for this feature and have cmse_nonsecure_call be the first example?

The other things needed to build a TrustZone-M application are mostly in cortex-m already: abstraction over the TT* instructions and Secure Attribution Unit (SAU) support. I am planning to make a blog post to summarize all those features showing an example.

Hi @hug-dev - so, I've been experimenting with this PR in nightly over the weekend and firstly thank you so much for the work 👍. Just a quick question or rather clarification - From the above comments, I understand we still need to wait for support to be added for call-site attributes in Rust before we can build a fully functioning demo i.e. TrustZone-M hands-off control to non-secure code from the secure world upon initial bootup and we require the call-site attr for the first callback from secure code. - right

If my understanding is correct - would you happen to know if anyone in the Rust community/team is working on call-site attributes support? or if we can put in a PR for this without duplication.

@hug-dev
Copy link
Contributor Author

hug-dev commented Oct 14, 2020

Thank you 😄 I am glad this is useful.

TrustZone-M hands-off control to non-secure code from the secure world upon initial bootup and we require the call-site attr for the first callback from secure code.

You are right that you could use the cmse_nonsecure_call for the first switch Secure -> Non-Secure but you could also do it manually!
In a local example I did the following:

unsafe {
    // The beginning of the flash memory for the Non-Secure application
    let ns_vector_table = 0x00200000u32 as *const u32;
    // Set the last bit to zero to show a transition from Secure to Non-Secure
    let ns_reset_vector = *(0x00200004u32 as *const u32) & !1u32;
    // Switch the stack pointer to the Main Stack Pointer from Non-Secure world
    cortex_m::register::msp::write_ns(*ns_vector_table);
    // Branch to the Non-Secure reset vector
    cortex_m::asm::bx_ns(ns_reset_vector);
}

I added write_ns and bx_ns in cortex-m just for that 🤓!
Note that this is not really secure as it does not clear the Secure registers before jumping so information could be leaked there. Instead, if cmse_nonsecure_call was there, it could be something like:

unsafe {
    // The beginning of the flash memory for the Non-Secure application
    let ns_vector_table = 0x00200000u32 as *const u32;
    cortex_m::register::msp::write_ns(*ns_vector_table);

    let ns_entry: extern "C" fn() = unsafe { std::mem::transmute(*(0x00200004u32 as *const u32)) };
    (#[cmse_nonsecure_call] ns_entry)()
}

I don't think anyone is currently working on call site attributes, and I was meant to start a "design proposal" of how to implement this attribute. There was also a suggestion to encode it as a different ABI. If you are definitely interested in this one, I can get to it sooner than later!

@nihalpasham
Copy link

I don't think anyone is currently working on call site attributes, and I was meant to start a "design proposal" of how to implement this attribute. There was also a suggestion to encode it as a different ABI. If you are definitely interested in this one, I can get to it sooner than later!

That would be great.... definitely very interested in getting this to work (so as to avoid relying on C for TrustZone setup and usage). I'm working on a project of mine where I'm evaluating different kinds of embedded -Hardware Accelerators and Root(s) of Trust (examples below) and one of my requirements is a secure runtime environment for hosting the driver used to communicate with an HRT.

So, yep I'm most interested and would also love to help in any way I can. {although full disclosure - my knowledge on compilers is it magically compiles 😅 }

@hug-dev
Copy link
Contributor Author

hug-dev commented Oct 14, 2020

Nice! Well I can start by making a proposal on Zulip that I will post here when ready. Then you can help reviewing it 😃

@nihalpasham
Copy link

@hug-dev Hi, just checking in to see if there are any new updates.

@hug-dev
Copy link
Contributor Author

hug-dev commented Dec 10, 2020

Hey @nihalpasham!
Arg sorry, I was working on other stuff and it completely slipped my mind. I am soon going to holidays for the Christmas break but I will make a note to myself to resurrect my notes and work on this and start a discussion around this issue in the beginning of Jannaury. I hope that's ok with you!

@nihalpasham
Copy link

Oh, that's alright. I kind of lost track of this too.

PS - Wish you a merry Christmas and happy holidays!

@nihalpasham
Copy link

Hey @hug-dev, Just thought I'd check-in to see if you're back.

@hug-dev
Copy link
Contributor Author

hug-dev commented Jan 13, 2021

Hello! I am 😄 Ok, I will take some time this week or the next to think about this again and start a discussion on Zulip 👍

@hug-dev
Copy link
Contributor Author

hug-dev commented Jan 18, 2021

Discussion about how to add the remaining cmse_nonsecure_call feature is happening here on Zulip.

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 3, 2021
…chievink

Add a new ABI to support cmse_nonsecure_call

This adds support for the `cmse_nonsecure_call` feature to be able to perform non-secure function call.

See the discussion on Zulip [here](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Support.20for.20callsite.20attributes/near/223054928).

This is a followup to rust-lang#75810 which added `cmse_nonsecure_entry`. As for that PR, I assume that the changes are small enough to not have to go through a RFC but I don't mind doing one if needed 😃
I did not yet create a tracking issue, but if most of it is fine, I can create one and update the various files accordingly (they refer to the other tracking issue now).

On the Zulip chat, I believe `@jonas-schievink` volunteered to be a reviewer 💯
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation merged-by-bors This PR was explicitly merged by bors. requires-nightly This issue requires a nightly compiler in some way. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants