riscv-rt: compatibility with RV32E and RV64E

[romancardenas]

Avoids using x16+ registers in assembly code. This is necessary to provide compatibility with RV32E.
Note that there is still work to do. Namely, the trap frame must adapt to the target base instruction set.

Solves #189

Related: #239

[rmsyn]

Would it be a good idea to order these fields by there x* name (similar to the spec)?

Also, maybe we use coniditional compilation to not include the x16-x31 registers for RV32E builds?

Mostly thinking about downstream users that may use a pointer to the TrapFrame, which they may or may not try to map to memory layout.

It may not be necessary, but could be a helpful reminder that those registers don't exist on RV32E platforms.

[romancardenas]

I'd try to avoid changing the order of the attributes in case users use C or assembly code that relies on the current order.

I'm working on feature-gating registers for RVE32. It's a bit tricky, as we need to add "trash" padding to ensure that the stack is 16-byte aligned. I'll open a PR shortly with all this.

[rmsyn]

I'd try to avoid changing the order of the attributes in case users use C or assembly code that relies on the current order.

I guess that's also part of what I'm asking, "should we intentionally break downstream users compiling RV32E?" Especially regarding not providing them in-memory registers that don't exist on the platform.

For non-RV32E users, I agree with you, we should avoid breaking them without a good reason.

I'm working on feature-gating registers for RVE32. It's a bit tricky, as we need to add "trash" padding to ensure that the stack is 16-byte aligned. I'll open a PR shortly with all this.

Sounds good, I'll keep an eye out for it.

Maybe for the missing registers on RV32E targets:

pub struct TrapFrame {
    #[cfg(not(rv32e))]
    pub a6: usize,
    #[cfg(rv32e)]
    _a6: usize,
    ...
}

[hegza]

I'm working on feature-gating registers for RVE32. It's a bit tricky, as we need to add "trash" padding to ensure that the stack is 16-byte aligned. I'll open a PR shortly with all this.

Wait, 16-byte aligned? You mean for RV32E? Stack on RV32E is specified to be 32-bit / 4-byte aligned. It's a specific feature of RV32E and in contrast to RV32I. References in LLVM & Clang:

• https://github.com/llvm/llvm-project/blob/528bcf3a55ca520c31c77ed5fbacf09bff8f39ec/clang/lib/Basic/Targets/RISCV.h#L145-L150.
• https://github.com/llvm/llvm-project/blob/5b17f85a7d722439a39f1ac1c554aed7858adab4/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp#L145-L146
• https://github.com/llvm/llvm-project/blob/5b17f85a7d722439a39f1ac1c554aed7858adab4/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp#L30-L36

[romancardenas]

I've been working on a complete riscv-target-parser crate to help us in the build scripts. Also, I think it is time to stop using the riscv, riscv32, and riscv64 cfg flags and just use target_arch.

My main challenge now is configuring riscv-rt-macros according to flags triggered in riscv-rt. I don't know how to do this without features, but I will read elsewhere and see if there is any other approach. I don't love letting users activate a riscve feature regardless of the target.

[rmsyn]

My main challenge now is configuring riscv-rt-macros according to flags triggered in riscv-rt. I don't know how to do this without features

If this specifically about RV32E stuff, maybe a combination of target_arch and target_feature flags in a #[cfg(...)]? Although, it looks like there are some restrictions on using target_feature on riscv* targets.

I would be happy to help out, if you have some work-in-progress code you would like extra eyes on.

[romancardenas]

This is the best way I could think of. I tried forwarding environment variables from build scripts to no avail, as procedural macros are compiled separately from regular crates. The macro code can be squeezed a bit, but more or less this is it.

[rmsyn]

This is the best way I could think of. I tried forwarding environment variables from build scripts to no avail, as procedural macros are compiled separately from regular crates. The macro code can be squeezed a bit, but more or less this is it.

Yeah, I tried using conditional compilation with target_feature = "i" + target_feature = "e", but it doesn't look like base ISA are available.

If target_feature = "i" and target_feature = "e" were available, we could use conditional compilation to define a core_interrupt_inner the four variants we care about, i.e. riscv32e, riscv32i, riscv64e, riscv64i. Then we could have the single core_interrupt macro function wrapping the unsafe inner functions.

I can look at the rustc/cargo source, and see what it takes to add base ISA as conditional compilation attribute. It may make more sense for the base ISA stuff to go under the instruction_set attribute (currently only has ARM in the docs).

The changes here LGTM.

Edit: something like this is what I was thinking: rv32e-compat-asm...rmsyn:riscv:rv32e-compat-asm

[romancardenas]

I tried something like that, but I found out that the procedural macros are compiled for the host, not the target. Thus, the cfg(target_arch = "riscv32") does not work unless your host machine is a RV32

[rmsyn]

I tried something like that, but I found out that the procedural macros are compiled for the host, not the target. Thus, the cfg(target_arch = "riscv32") does not work unless your host machine is a RV32

What about replacing them with the cfg! macro, which is evaluated at run-time instead of build-time?

Something like: rmsyn/riscv@rust-embedded:riscv:rv32e-compat-asm...rv32e-compat-asm

[romancardenas]

I'm currently with a considerable workload at work. Will come back to this this weekend, probably

[rmsyn]

I'm currently with a considerable workload at work. Will come back to this this weekend, probably

No worries, I hope all goes well.

[romancardenas]

@rmsyn take a look to my latest commit. Using cfg!(target =...) didn't work, but I managed to do something very similar with environment variables.

[rmsyn]

Are these reserved fields added for alignment?

[romancardenas]

Yes, the trap frame must be 16-byte aligned. Thus, for RV32E, we need a couple of usize reserved fields

[rmsyn]

Changes LGTM, congrats on the env solution!

[romancardenas]

I added a few minor improvements and wrote all the important changes in the CHANGELOG.md file

[romancardenas]

@hegza can you confirm that this version fits your RV32E projects?

[hegza]

I maintain a short RVE patch-set that works for us, here: master...hegza:riscv:feat/rve

I can smoke test our platform possibly today already.

[romancardenas]

Thanks for your review! I fixed the alignment issue. Now, it should be basically identical to your fork.

[hegza]

Tested RT by swapping it in place of our fork for a minimal smoke test and indeed there was no smoke. So, works on my machine (rv32emc), I guess 😅 Compiles alright and prints over serial without putting the core into crazy states.

Notably, no traps were taken in the test case. It takes slightly more involved setup.

[hegza]

RV32E requires 4-byte trap alignment instead of 16-byte, compare to my implementation:

master...hegza:riscv:feat/rve#diff-dd85e4f259537e87b681ea17211538f2f6280a0b186b8e3fe7d333a6f937a7c2R414-R427

[hegza]

For the record, if the RT manages to create any invalid stores to registers x16.. on an RV32E Rust backend, it would fail to compile. So, the fact that it compiles is already a good sign. TrapFrame looked alright as well.

[romancardenas]

So I guess it is ready to merge, then!

[romancardenas]

I just noticed that we were breaking the 16 byte alignment in our runtime. I guess we could add a bunch of feature gates to relax these constraints in RVxE targets, but I'd rather leave this work for a future PR

[rmsyn]

I guess we could add a bunch of feature gates to relax these constraints in RVxE targets, but I'd rather leave this work for a future PR

As far as I can tell, there is no difference for the stack pointer alignment in RVxE implementations, the only difference is the register count (based on Ch. 3 of the Unprivileged Spec). Is there somewhere else that specifies a difference in stack pointer alignment?

[hegza]

I haven't found this specified either but that's what clang and LLVM are doing, as linked in earlier discussion already:

• https://github.com/llvm/llvm-project/blob/528bcf3a55ca520c31c77ed5fbacf09bff8f39ec/clang/lib/Basic/Targets/RISCV.h#L145-L150
• https://github.com/llvm/llvm-project/blob/5b17f85a7d722439a39f1ac1c554aed7858adab4/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp#L145-L146
• https://github.com/llvm/llvm-project/blob/5b17f85a7d722439a39f1ac1c554aed7858adab4/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp#L30-L36

We also discussed this earlier over at rust-lang/rust: rust-lang/rust#130555 (comment)

[rmsyn]

I haven't found this specified either but that's what clang and LLVM are doing, as linked in earlier discussion already:

* https://github.com/llvm/llvm-project/blob/528bcf3a55ca520c31c77ed5fbacf09bff8f39ec/clang/lib/Basic/Targets/RISCV.h#L145-L150

* https://github.com/llvm/llvm-project/blob/5b17f85a7d722439a39f1ac1c554aed7858adab4/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp#L145-L146

* https://github.com/llvm/llvm-project/blob/5b17f85a7d722439a39f1ac1c554aed7858adab4/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp#L30-L36

We also discussed this earlier over at rust-lang/rust: rust-lang/rust#130555 (comment)

Looks like GCC does the same thing: https://github.com/gcc-mirror/gcc/blob/d136fa00f0d5faff8397edcd7e4ebb3445ab21b0/gcc/config/riscv/riscv.h#L202-L205

I wonder where they get this information from? Since they both do it, they must be basing it on something. I'm looking into some soft-core implementations like CVW and PicoRV32, since those look like the closest thing to an actual hardware implementation at the moment.