[Tracking] Convert from llvm_asm!() to new asm!() #1923

bradjc · 2020-06-09T16:20:39Z

gendx · 2020-06-09T16:55:37Z

This is really great given that inline assembly is likely the main reason to use Rust nightly at the moment. And the new macro seems to be getting quite a bit of traction so we can expect it to stabilize in the medium term.

From a programming point of view this is IMO much cleaner than having to link external .S files, and if I understood correctly inline asm! also allows the compiler to apply more optimizations than with raw .S files being linked blindly at the linker step.

alistair23 · 2020-06-12T21:22:48Z

#1926 is a step in this direction and I think completes the swap for `libraries/riscv-csr``

2092: arch/cortex-m: change a few `llvm_asm!`s to `asm!`s r=ppannuto a=JOE1994 ### Pull Request Overview This PR changes 3 invocations of `llvm_asm!` to `asm!`. This PR intends to make a small step towards the direction of #1923 . I made the changes after reading through the [inline-asm RFC](https://github.com/Amanieu/rfcs/blob/inline-asm/text/0000-inline-asm.md#summary) . ##### Brief overview of changes made 1. `llvm_asm!("nop" :::: "volatile");` => `asm!("nop");` 2. `llvm_asm!("wfi" :::: "volatile");` => `asm!("wfi");` * The only notable change in the first two cases is that the `volatile` keyword is gone. In the new `asm!` macro, `volatile` is assumed by default and there is an option called `pure` which can be used to get rid of the `volatile` semantics. * reference: [Link to Q&A on `volatile` from Rust Zulip chat](https://rust-lang.zulipchat.com/#narrow/stream/216763-project-inline-asm/topic/volatile.20question/near/208320361) (requires sign-in) 3. `llvm_asm!("mrs r0, ipsr": "={r0}"(interrupt_number) : :"r0" :);` => `asm!("mrs r0, ipsr", out("r0") interrupt_number);` * The current `llvm_asm!` invocation specifies `r0` both as output and clobbered register. [It is specified in the LLVM docs that "clobbering named registers that are also present in output constraints is not legal."](http://llvm.org/docs/LangRef.html#clobber-constraints), and it is not possible to do that under the new `asm!` macro syntax. Note the `asm!` macro invocation doesn't have any clobber constraints. ### TODO or Help Wanted Although the changes made in this PR are quite simple, I'm not sure how to properly test the changes. I'd appreciate any advice on testing strategy for this PR 😺 ### Documentation Updated - [x] Updated the relevant files in `/docs`, or no updates are required. ### Formatting - [x] Ran `make prepush`. Co-authored-by: JOE1994 <joseph942010@gmail.com>

2225: arch/cortex-m: change more llvm_asm! to asm! r=bradjc a=JOE1994 ### Pull Request Overview This PR changes 7 invocations of `llvm_asm!` to `asm!`. Like #2092, this PR intends to make progress towards finishing #1923 . I made the changes after reading through the [inline-asm RFC](https://github.com/Amanieu/rfcs/blob/inline-asm/text/0000-inline-asm.md#rules) . #### A couple of notes on the changes * The first two `llvm_asm!` calls with `cc` clobbers are translated to `asm!` calls that don't explicitly state `cc` clobbers. In `asm!`, `cc` clobbers are assumed by default. (Option `preserve_flags` can be used when `cc` clobbers don't happen) * reference: [Rust Zulip chat stream 'project-inline-asm'](https://rust-lang.zulipchat.com/#narrow/stream/216763-project-inline-asm/topic/Moving.20forward.20with.20the.20RFC/near/185306638) * `volatile` is assumed by default in `asm!`, so explicit `volatile` keywords were removed. (Option `pure` can be used to get rid of the `volatile` semantics) * reference: [Link to Q&A on `volatile` from Rust Zulip chat](https://rust-lang.zulipchat.com/#narrow/stream/216763-project-inline-asm/topic/volatile.20question/near/208320361) * `asm!` disallows use of `bool` variables as outputs. I replaced the output `bool` variables with `u32` variables. ```shell error: cannot use value of type `bool` for inline assembly --> arch/cortex-m/src/lib.rs:409:19 | 409 | out("r1") kernel_stack, | ^^^^^^^^^^^^ | = note: only integers, floats, SIMD vectors, pointers and function pointers can be used as arguments for inline assembly ``` ### Testing Strategy I tested this PR by building the kernel and flashing to a `Hail` board with a blinky app. Didn't see any erroneous behavior when pressing the reset button... ### TODO or Help Wanted I somewhat naively replaced `bool` variables with `u32` variables. Would it be better to replace the `bool` variables with `usize` instead? ### Documentation Updated - [x] Updated the relevant files in `/docs`, or no updates are required. ### Formatting - [x] Ran `make prepush`. ### Thank you for reviewing this PR :crab: Co-authored-by: JOE1994 <joseph942010@gmail.com>

hudson-ayers · 2021-01-02T14:23:50Z

With the outstanding PRs merged, there are only 4 uses of llvm_asm remaining (ignoring all the uses in cortex-m0)

2306: chips: stm: llvm_asm -> asm r=hudson-ayers a=bradjc ### Pull Request Overview #1923 ### Testing Strategy This pull request was tested by... ### TODO or Help Wanted This pull request still needs... ### Documentation Updated - [x] Updated the relevant files in `/docs`, or no updates are required. ### Formatting - [x] Ran `make prepush`. Co-authored-by: Brad Campbell <bradjc5@gmail.com>

lschuermann · 2021-01-06T11:04:21Z

As indicated in #2308, once that change is in, I can vouch to do the migration of the rest of the rv32i crate.

2303: llvm_asm -> asm: arty-e21 r=hudson-ayers a=bradjc ### Pull Request Overview Convert to asm macro. #1923 ### Testing Strategy Running apps on arty-e21. ### TODO or Help Wanted n/a ### Documentation Updated - [x] Updated the relevant files in `/docs`, or no updates are required. ### Formatting - [x] Ran `make prepush`. Co-authored-by: Brad Campbell <bradjc5@gmail.com>

2339: Apollo3 llvm_asm -> asm r=hudson-ayers a=bradjc ### Pull Request Overview #1923 Remove earlgray feature, update apollo3. ### Testing Strategy travis ### TODO or Help Wanted n/a ### Documentation Updated - [x] Updated the relevant files in `/docs`, or no updates are required. ### Formatting - [x] Ran `make prepush`. Co-authored-by: Brad Campbell <bradjc5@gmail.com>

2363: rv32i: transition llvm_asm! to asm! r=bradjc a=lschuermann ### Pull Request Overview This pull request transitions the remaining `llvm_asm!` snippet of the `rv32i` towards the new Rust `asm!` macro (see #1923 for the respective tracking issue). It's split up into commits that represent atomic changes respectively. In addition to the raw transition, this changes two things - it removes the unused `abort()` / `_abort` fuction. It was previously public & safe, such that untrusted code could simply call to it. - in `switch_to_process`, it forces the `Riscv32iStoredState` pointer to be passed in a specific register. This is important since, while we don't clobber any registers in the assembly block there, we do write to `t0` after we've saved it to the stack. Later, we access the stored state pointer (from some unspecified register). If the compiler were to put this information in `t0`, we would access the overwritten contents instead. By fixing the state pointer to always be passed in `a0` (which the compiler has done up to now), we can rely on being able to safely overwrite `t0` after it's been saved on the stack. Notably, knowing the register where the stored state is passed in potentially enables an optimization where we don't need to save it to the stack twice (and hence save a memory write & read). However, implementing that made the ASM quite a bit more confusing, so I might implement that as a follow up PR, once I find the time to write it in a clean fashion. All in all, the abort function assembly is removed ```diff - 00015210 <abort>: - 15210: df1ea06f j 0 <_start> - 15214: 00008067 ret ``` and the `switch_to_process` function seems to generate the exact same assembly. One weird thing about `asm!()`: I tried to replace the `lui` + `addi` based immediate load by a `li` pseudo-instruction, however the compiler complained that `_return_to_kernel` was not a valid immediate value there, whereas it continues to work with the `%hi` / `%lo` accessors. The solution™ is to insert the immediate value as a constant, however we can't refer to an assembly label there. Hence leaving it the way it is. ### Testing Strategy This pull request was tested by running a userspace app in the LiteX simulator and on the LiteX Arty board. ### TODO or Help Wanted This pull request still needs careful review. I can recommend stepping through the commits individually such that the changes can be reviewed separately. ### Documentation Updated - [ ] Updated the relevant files in `/docs`, or no updates are required. ### Formatting - [x] Ran `make prepush`. Co-authored-by: Leon Schuermann <leon@is.currently.online> Co-authored-by: Brad Campbell <bradjc5@gmail.com>

2440: RFC: Add `install` make target for boards r=ppannuto a=bradjc Tock has had a long standing convention of two makefile targets for boards to load a kernel onto a board. 1. flash: Uses JTAG to flash the kernel. 1. program: Uses UART and a bootloader to flash the kernel. This is helpful because some boards support both but require different hardware, and it gives the user some idea of what tools will be needed to load the kernel. However, new users must figure out which is appropriate if a board supports both. Often there is one target that experienced users just know to use, and we should make this more clear to new users. Also, having different targets on different boards complicates writing a common tutorial guide for multiple platforms. This patch adds a new target `make install` that just uses the default for the platform. The expectation is that most users can install the kernel using `make install`. The other targets are of course there for users who need something more specific. ### Testing Strategy This pull request was tested by using `make install` to flash the kernel on hail. ### TODO or Help Wanted Thoughts? ### Documentation Updated - [x] Updated the relevant files in `/docs`, or no updates are required. ### Formatting - [x] Ran `make prepush`. 2449: arch: cortex-m: update to asm! r=ppannuto a=bradjc ### Pull Request Overview This pull request switches the last llvm_asm in cortex-m to asm. This was somewhat straightforward, except that Rust will not let me mark r6 as clobbered (`out("r6") _`) like the other registers. I get: ``` error: invalid register `r6`: r6 is used internally by LLVM and cannot be used as an operand for inline asm --> arch/cortex-m/src/lib.rs:254:31 | 254 | out("r4") _, out("r5") _, out("r6") _, out("r8") _, out("r9") _, | ^^^^^^^^^^^ ``` I found that if I manually saved r6 in r3 the kernel still works. #1923 ### Testing Strategy Running hail app on hail. ### TODO or Help Wanted n/a ### Documentation Updated - [x] Updated the relevant files in `/docs`, or no updates are required. ### Formatting - [x] Ran `make prepush`. Co-authored-by: Brad Campbell <bradjc5@gmail.com>

2450: arch: cortex-m0: switch to asm!() r=phil-levis a=bradjc ### Pull Request Overview This pull request implements #1923 for cortex-m0. ### Testing Strategy None? We don't have any cortex-m0 boards. ### TODO or Help Wanted Help trying to verify my changes are OK. ### Documentation Updated - [x] Updated the relevant files in `/docs`, or no updates are required. ### Formatting - [x] Ran `make prepush`. Co-authored-by: Brad Campbell <bradjc5@gmail.com>

bradjc · 2021-03-12T15:47:38Z

Woo!

$ git grep llvm_asm | wc -l
       0

2440: RFC: Add `install` make target for boards r=ppannuto a=bradjc Tock has had a long standing convention of two makefile targets for boards to load a kernel onto a board. 1. flash: Uses JTAG to flash the kernel. 1. program: Uses UART and a bootloader to flash the kernel. This is helpful because some boards support both but require different hardware, and it gives the user some idea of what tools will be needed to load the kernel. However, new users must figure out which is appropriate if a board supports both. Often there is one target that experienced users just know to use, and we should make this more clear to new users. Also, having different targets on different boards complicates writing a common tutorial guide for multiple platforms. This patch adds a new target `make install` that just uses the default for the platform. The expectation is that most users can install the kernel using `make install`. The other targets are of course there for users who need something more specific. ### Testing Strategy This pull request was tested by using `make install` to flash the kernel on hail. ### TODO or Help Wanted Thoughts? ### Documentation Updated - [x] Updated the relevant files in `/docs`, or no updates are required. ### Formatting - [x] Ran `make prepush`. 2449: arch: cortex-m: update to asm! r=ppannuto a=bradjc ### Pull Request Overview This pull request switches the last llvm_asm in cortex-m to asm. This was somewhat straightforward, except that Rust will not let me mark r6 as clobbered (`out("r6") _`) like the other registers. I get: ``` error: invalid register `r6`: r6 is used internally by LLVM and cannot be used as an operand for inline asm --> arch/cortex-m/src/lib.rs:254:31 | 254 | out("r4") _, out("r5") _, out("r6") _, out("r8") _, out("r9") _, | ^^^^^^^^^^^ ``` I found that if I manually saved r6 in r3 the kernel still works. tock#1923 ### Testing Strategy Running hail app on hail. ### TODO or Help Wanted n/a ### Documentation Updated - [x] Updated the relevant files in `/docs`, or no updates are required. ### Formatting - [x] Ran `make prepush`. Co-authored-by: Brad Campbell <bradjc5@gmail.com>

2450: arch: cortex-m0: switch to asm!() r=phil-levis a=bradjc ### Pull Request Overview This pull request implements tock#1923 for cortex-m0. ### Testing Strategy None? We don't have any cortex-m0 boards. ### TODO or Help Wanted Help trying to verify my changes are OK. ### Documentation Updated - [x] Updated the relevant files in `/docs`, or no updates are required. ### Formatting - [x] Ran `make prepush`. Co-authored-by: Brad Campbell <bradjc5@gmail.com>

bradjc added the tracking label Jun 9, 2020

mcy mentioned this issue Jun 17, 2020

Building Tock with stable rustc #1654

Open

28 tasks

JOE1994 mentioned this issue Sep 2, 2020

arch/cortex-m: change a few llvm_asm!s to asm!s #2092

Merged

2 tasks

JOE1994 mentioned this issue Nov 30, 2020

arch/cortex-m: change more llvm_asm! to asm! #2225

Merged

2 tasks

This was referenced Dec 23, 2020

llvm_asm -> asm: arty-e21 #2303

Merged

chips: earlgrey: llvm_asm -> asm #2305

Closed

chips: stm: llvm_asm -> asm #2306

Merged

lschuermann mentioned this issue Dec 24, 2020

rv32i: fix race condition of interrupt in switch_to_process #2308

Merged

2 tasks

bradjc mentioned this issue Jan 11, 2021

Apollo3 llvm_asm -> asm #2339

Merged

2 tasks

lschuermann mentioned this issue Jan 18, 2021

rv32i: transition llvm_asm! to asm! #2363

Merged

2 tasks

bradjc added the release-blocker Issue or PR that must be resolved before the next release label Feb 5, 2021

This was referenced Feb 22, 2021

arch: cortex-m: update to asm! #2449

Merged

arch: cortex-m0: switch to asm!() #2450

Merged

bradjc closed this as completed Mar 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Tracking] Convert from llvm_asm!() to new asm!() #1923

[Tracking] Convert from llvm_asm!() to new asm!() #1923

bradjc commented Jun 9, 2020 •

edited

gendx commented Jun 9, 2020

alistair23 commented Jun 12, 2020

hudson-ayers commented Jan 2, 2021

lschuermann commented Jan 6, 2021 •

edited

bradjc commented Mar 12, 2021

[Tracking] Convert from llvm_asm!() to new asm!() #1923

[Tracking] Convert from llvm_asm!() to new asm!() #1923

Comments

bradjc commented Jun 9, 2020 • edited

gendx commented Jun 9, 2020

alistair23 commented Jun 12, 2020

hudson-ayers commented Jan 2, 2021

lschuermann commented Jan 6, 2021 • edited

bradjc commented Mar 12, 2021

bradjc commented Jun 9, 2020 •

edited

lschuermann commented Jan 6, 2021 •

edited