Add a new ABI to support cmse_nonsecure_call #81346

hug-dev · 2021-01-24T17:25:52Z

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

See the discussion on Zulip here.

This is a followup to #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 💯

rust-highfive · 2021-01-24T17:25:55Z

r? @GuillaumeGomez

(rust-highfive has picked a reviewer for you, use r? to override)

rust-log-analyzer · 2021-01-24T17:39:06Z

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
   Compiling regex v1.4.3
   Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
    Finished release [optimized] target(s) in 10.11s
tidy check
tidy error: following path contains more than 1458 entries, you should move the test to some relevant subdirectory (current: 1459): /checkout/src/test/ui
Found 435 error codes
Found 0 error codes with no tests
Done!
some tidy checks failed

jonas-schievink

Yeah, please create a new tracking issue for this ABI

jonas-schievink · 2021-01-24T18:14:04Z

src/test/ui/cmse-nonsecure-call/params-on-registers.rs

+#[no_mangle]
+pub fn test(a: u32, b: u32, c: u32, d: u32) -> u32 {
+    let non_secure_function = unsafe {
+        core::mem::transmute::<usize, extern "cmse-nonsecure-call" fn(u32, u32, u32, u32) -> u32>(


Can you also add a test that defines a extern "cmse-nonsecure-call" fn in Rust instead of transmuting?

I tried the following (if that is what you meant):

#![no_std] #![feature(abi_cmse_nonsecure_call)] #[no_mangle] pub fn test() -> u32 { toto() } extern "cmse-nonsecure-call" fn toto() -> u32 { 45 }

and I have the following LLVM error:

[hugdev01@machine tests]$ rustc +stage1 --target thumbv8m.main-none-eabi --crate-type lib --emit asm toto.rs rustc: /home/hugdev01/dev/rust-work/rust/src/llvm-project/llvm/lib/Target/ARM/ARMISelLowering.cpp:2556: virtual llvm::SDValue llvm::ARMTargetLowering::LowerCall(llvm::TargetLowering::CallLoweringInfo&, llvm::SmallVectorImpl<llvm::SDValue>&) const: Assertion `!isARMFunc && !isDirect && "Cannot handle call to ARM function or direct call"' failed. Aborted (core dumped)

I believe this is because it is forbidden, in LLVM this callsite attribute can really only be used on function pointers but not directly on function declaration.
The specification document says that section 5.5:

A non-secure function type must only be used as a base type of a pointer. This disallows function definitions with this attribute and ensures a secure executable file only contains secure function definitions.

I guess ideally this kind of error should be returned by Rust instead of LLVM.

Ah, I see. So the intention is that secure and non-secure worlds are always entirely separate executables, and cmse-nonsecure-call is used by the secure side only to invoke callback functions passed by the non-secure side?

Where can I find documentation of the LLVM implementation? https://llvm.org/docs/LangRef.html doesn't mention it.

(I agree that this should be checked by rustc instead of LLVM, especially since this is not a diagnostic but an assertion failure)

So the intention is that secure and non-secure worlds are always entirely separate executables

Yes true!

cmse-nonsecure-call is used by the secure side only to invoke callback functions passed by the non-secure side?

Yes I believe that is the main use-case! It's also possible for the Secure side to call functions that it knows are at particular addresses, for example the non-secure reset handler.

Where can I find documentation of the LLVM implementation? https://llvm.org/docs/LangRef.html doesn't mention it.

There isn't much documentation of how those features are implemented in LLVM 😅. The changes were introduced by this mail on the mailing list a long time ago. Apart from that there are the specifications and the various commits on LLVM to get an idea of how it's implemented. Not ideal, I know.

(I agree that this should be checked by rustc instead of LLVM, especially since this is not a diagnostic but an assertion failure)

I am going to look where it is possible to check the position of the extern keywork to differentiate between function declaration/definition and function pointer type.

I found a way to do this in the AST visitor it seems, by checking if the ABI is applied on a bare function or not. Might be a bit hacky to modify function prototype just for this feature. But it works!

jonas-schievink · 2021-01-24T18:17:59Z

compiler/rustc_codegen_llvm/src/abi.rs

+
+        if self.conv == Conv::CmseNonSecureCall {
+            // This will probably get ignored on all targets but those supporting the TrustZone-M
+            // extension (thumbv8m targets).


We should reject the ABI when using a --target that doesn't support it (hmm, do we not do that for #[cmse_non_secure_entry]?)

True! Initially I was looking how to check the target from this file but I am not sure how to get the target information from there. It looks like it would be possible to do it in the change file compiler/rustc_ast_passes/src/feature_gate.rs as there is access to the rustc_session::session::Session structure which contains the target.

If I do, can I maybe re-use and modify the same error code used for cmse_nonsecure_entry?

Yeah that should be fine

feature_gate.rs is not the right place to check this though. We already have per-target lists of supported ABIs, this should just plug in there.

Ah I see, sorry I mixed up the two things and thought we were talking about the target check. Cool I can move the bare_function check there.

Just wondering how I would do it 😅 In the feature_gate.rs I can have the information that the compiler is currently checking a bare function type but I don't see how I can check that in the check.rs file. Or did you mean that I should call the check_abi function of check.rs (with the bare_function info) from the feature_gate.rs file?

No, feature_gate.rs should not be involved in checking ABI-specific restrictions. I'm not sure where the best way to put this is, perhaps you have to write your own visitor, similar to what inline asm uses?

Ah ok, got it.

I noticed that check_abi on check.rs is only called to check the ABI of function declarations and external blocks which are the exact two cases where the CCmseNonSecureCall ABI is forbidden. I believe the other only use of extern is for function pointer and that one is allowed.

So one easy way would be to add a check for CCmseNonSecureCall inside check_abi and emit an error if that's equal. I tested it and it works as expected.

Maybe it's a bit risky if check_abi is ever used for function pointers but we can add tests covering all the cases?

Yeah, seems fine if there are tests

Ok done, there should be:

one test checking that the ABI on a function pointer compiles successfully

one test checking that the ABI on on function definition fails

one test checking that the ABI on an external block fails

camelid · 2021-01-24T23:06:23Z

r? @jonas-schievink

rust-log-analyzer · 2021-01-25T20:32:25Z

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
   Compiling regex v1.4.3
   Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
    Finished release [optimized] target(s) in 9.62s
tidy check
tidy error: following path contains more than 1458 entries, you should move the test to some relevant subdirectory (current: 1459): /checkout/src/test/ui
Found 436 error codes
Found 0 error codes with no tests
Done!
some tidy checks failed

hug-dev · 2021-01-26T12:21:16Z

tidy error: following path contains more than 1458 entries, you should move the test to some relevant subdirectory (current: 1459): /checkout/src/test/ui

Shall I then group both CMSE features UI tests in the same subdirectory (for the attribute and the ABI)?

jonas-schievink · 2021-01-26T12:21:57Z

Shall I then group both CMSE features UI tests in the same subdirectory (for the attribute and the ABI)?

Yes, that seems good

hug-dev · 2021-01-27T18:13:35Z

I put a C in front of everything as mentionned in the tracking issue as well.

jonas-schievink

Great! Just one minor nit, then this should be good to go.

compiler/rustc_typeck/src/check/check.rs

jonas-schievink · 2021-02-01T14:45:12Z

src/test/ui/cmse-nonsecure/cmse-nonsecure-call/wrong-abi-location-1.stderr

@@ -0,0 +1,9 @@
+error[E0781]: The `"cmse-nonsecure-call"` ABI is only allowed on function pointers.


Why did this not fail in CI? The message should now be lowercased.

I have the feeling that the tests with:

// compile-flags: --target thumbv8m.main-none-eabi --crate-type lib // only-thumbv8m.main-none-eabi

are ignored on CI 😕

edit: updated the stderr files

Oh, right. They are skipped by the PR builder, but the "real" CI should check them.

jonas-schievink · 2021-02-01T14:55:04Z

Let's give this a shot

@bors r+ rollup=iffy

bors · 2021-02-01T14:55:06Z

📌 Commit 4aae28da042dcc0053f36b83c65061798f4d497e has been approved by jonas-schievink

bors · 2021-02-01T19:22:39Z

⌛ Testing commit 4aae28da042dcc0053f36b83c65061798f4d497e with merge 489d779e45bf34f3daea611ef1f14518444b9daf...

rust-log-analyzer · 2021-02-01T19:29:20Z

The job x86_64-gnu-debug failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

    Checking cranelift-frontend v0.69.0 (https://github.com/bytecodealliance/wasmtime/?branch=main#986b5768)
    Checking cranelift-jit v0.69.0 (https://github.com/bytecodealliance/wasmtime/?branch=main#986b5768)
    Checking cranelift-object v0.69.0 (https://github.com/bytecodealliance/wasmtime/?branch=main#986b5768)
    Checking rustc_codegen_cranelift v0.1.0 (/checkout/compiler/rustc_codegen_cranelift)
error[E0004]: non-exhaustive patterns: `CCmseNonSecureCall` not covered
   --> src/abi/mod.rs:26:27
    |
26  |     let call_conv = match fn_abi.conv {
    |                           ^^^^^^^^^^^ pattern `CCmseNonSecureCall` not covered
   ::: /checkout/compiler/rustc_target/src/abi/call/mod.rs:554:5
    |
    |
554 |     CCmseNonSecureCall,
    |
    = help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms
    = note: the matched value is of type `Conv`

bors · 2021-02-01T19:30:56Z

💔 Test failed - checks-actions

This commit adds a new ABI to be selected via `extern "C-cmse-nonsecure-call"` on function pointers in order for the compiler to apply the corresponding cmse_nonsecure_call callsite attribute. For Armv8-M targets supporting TrustZone-M, this will perform a non-secure function call by saving, clearing and calling a non-secure function pointer using the BLXNS instruction. See the page on the unstable book for details. Signed-off-by: Hugues de Valon <hugues.devalon@arm.com>

hug-dev · 2021-02-02T13:08:21Z

compiler/rustc_codegen_cranelift/src/abi/mod.rs

@@ -28,6 +28,7 @@ fn clif_sig_from_fn_abi<'tcx>(
        Conv::X86_64SysV => CallConv::SystemV,
        Conv::X86_64Win64 => CallConv::WindowsFastcall,
        Conv::ArmAapcs
+        | Conv::CCmseNonSecureCall


Rebased and added this to fix the error

jonas-schievink · 2021-02-02T14:25:36Z

@bors r+

bors · 2021-02-02T14:25:37Z

📌 Commit ce9818f has been approved by jonas-schievink

bors · 2021-02-03T06:00:48Z

⌛ Testing commit ce9818f with merge b593389...

bors · 2021-02-03T08:46:38Z

☀️ Test successful - checks-actions
Approved by: jonas-schievink
Pushing b593389 to master...

rust-highfive assigned GuillaumeGomez Jan 24, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 24, 2021

jonas-schievink reviewed Jan 24, 2021

View reviewed changes

rust-highfive assigned jonas-schievink and unassigned GuillaumeGomez Jan 24, 2021

hug-dev mentioned this pull request Jan 25, 2021

Tracking Issue for the C-cmse-nonsecure-call ABI #81391

Open

2 tasks

hug-dev force-pushed the nonsecure-call-abi branch from ac2bb2e to ab06dbc Compare January 25, 2021 20:19

hug-dev force-pushed the nonsecure-call-abi branch from ab06dbc to e18357d Compare January 27, 2021 18:12

hug-dev force-pushed the nonsecure-call-abi branch from e18357d to b56857f Compare January 30, 2021 14:48

jonas-schievink approved these changes Jan 30, 2021

View reviewed changes

compiler/rustc_typeck/src/check/check.rs Outdated Show resolved Hide resolved

jonas-schievink reviewed Feb 1, 2021

View reviewed changes

hug-dev force-pushed the nonsecure-call-abi branch from ad68223 to 4aae28d Compare February 1, 2021 14:52

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 Feb 1, 2021

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 1, 2021

hug-dev force-pushed the nonsecure-call-abi branch from 4aae28d to ce9818f Compare February 2, 2021 13:07

hug-dev commented Feb 2, 2021

View reviewed changes

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 Feb 2, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 3, 2021

bors merged commit b593389 into rust-lang:master Feb 3, 2021

rustbot added this to the 1.51.0 milestone Feb 3, 2021

bors mentioned this pull request Feb 3, 2021

Accept additional user-defined classes in fenced code blocks #79454

Closed

7 tasks

		@@ -0,0 +1,9 @@
		error[E0781]: The `"cmse-nonsecure-call"` ABI is only allowed on function pointers.

Add a new ABI to support cmse_nonsecure_call #81346

Add a new ABI to support cmse_nonsecure_call #81346

Conversation

hug-dev commented Jan 24, 2021

rust-highfive commented Jan 24, 2021

rust-log-analyzer commented Jan 24, 2021

jonas-schievink left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camelid commented Jan 24, 2021

rust-log-analyzer commented Jan 25, 2021

hug-dev commented Jan 26, 2021

jonas-schievink commented Jan 26, 2021

hug-dev commented Jan 27, 2021

jonas-schievink left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hug-dev Feb 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonas-schievink commented Feb 1, 2021

bors commented Feb 1, 2021

bors commented Feb 1, 2021

rust-log-analyzer commented Feb 1, 2021

bors commented Feb 1, 2021

Choose a reason for hiding this comment

jonas-schievink commented Feb 2, 2021

bors commented Feb 2, 2021

bors commented Feb 3, 2021

bors commented Feb 3, 2021

hug-dev Feb 1, 2021 •

edited

Loading