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

riscv-rt: Support rv32e #178

Closed
wants to merge 5 commits into from
Closed

riscv-rt: Support rv32e #178

wants to merge 5 commits into from

Conversation

hegza
Copy link

@hegza hegza commented Feb 9, 2024

BLUF

riscv-rt fails to link on riscv32emc. GCC complains about "mis-matched ISA string to merge 'i' and 'e'". This PR changes riscv-rt such that it does link on riscv32emc, by removing accesses to registers that don't exist on RVE.

Contents

As defined in RISC-V Unprivileged Spec. 20191213, the only change in RVE is to reduce the number of integer registers to 16 (x0-x15). This requires also a separate calling convention that seems a little underspecified.

Somebody on the internet said these are also implied by the respective ilp32e calling convention:

  • 2 callee saved registers instead of 12 (but this is not relevant to us I think)
  • 32-bit / 4-byte stack alignment instead of 128 bits / 16 bytes (this is relevant)

This PR does the following:

  • Disable start of program load-to-zero for x16--x31 for RVE, since these do not exist on RVE
  • Force & assert the stack pointer is 4-byte aligned on RVE instead of 16-byte align as on RVI
  • Configure the trap handler to only caller-save a0--a5 (x10--x15) and t0--t2 (x5--x7) instead of a0--a7 (x10--x17) and t0--t6 (x5--x7 + x28--x31) on RVE
  • Disable a6--a7 (x16--x17) and t3--t6 (x28--x31) for RVE in the TrapFrame struct, since these do not exist on RVE

WIP

  1. The latest riscv-rt master branch fails to compile when codegen-units > 1 in Cargo.toml' [profile.*].

    .../riscv32-unknown-elf/bin/ld:link.x:77: undefined symbol `default_start_trap' referenced in expression

    But I think that is unrelated to the issue at hand. Perhaps worthy of its own issue.

  2. I think this patch might not be all that is required for RVE support, but this is at least the minimal amount of changes that makes riscv-rt link on RVE, allowing further testing.

  3. FPGA testing pending. I won't be able to test the interrupts yet, but I should be able to confirm the runtime works otherwise. Done.

Extra context

We're building an RV32EMC in the lab. It's not supported by latest Rust, so we're rolling our own toolchain https://github.com/soc-hub-fi/rust-rv32emc-docker 😃

@hegza hegza force-pushed the rve branch 2 times, most recently from 959971b to 9991925 Compare February 9, 2024 14:16
@hegza hegza marked this pull request as ready for review February 9, 2024 14:19
@hegza hegza requested a review from a team as a code owner February 9, 2024 14:19
@hegza hegza changed the title Support rv32e riscv-rt: Support rv32e Feb 9, 2024
@hegza
Copy link
Author

hegza commented Feb 9, 2024

Eh, curiously enough, something like the original problem re-surfaces if built with --release. I'm doing the testing on a virtual machine without any extra package caching, and I've done a clean build between debug & release.

/opt/riscv/riscv32-unknown-elf/bin/ld: error: /root/riscv-rt-test/target/riscv32emc-unknown-none-elf/release/deps/libriscv_rt-88803cdbe53aa305.rlib(riscv_rt-88803cdbe53aa305.riscv_rt.2debb9bc8c5d66ff-cgu.0.rcgu.o): mis-matched ISA string to merge 'i' and 'e'
/opt/riscv/riscv32-unknown-elf/bin/ld: failed to merge target specific data of file /root/riscv-rt-test/target/riscv32emc-unknown-none-elf/release/deps/libriscv_rt-88803cdbe53aa305.rlib(riscv_rt-88803cdbe53aa305.riscv_rt.2debb9bc8c5d66ff-cgu.0.rcgu.o)

@hegza
Copy link
Author

hegza commented Feb 9, 2024

Latest commit fixes the issue with --release. It seems the LLVM spurious error affects us too 🙂

@hegza
Copy link
Author

hegza commented Feb 9, 2024

By disabling -Fsingle-hart I figured also the M-instructions broke similarly to what happened before with E. I added the .attributes hack for rv32em and that fixed the issue. I presume same would happen for C-type instructions so I pre-emptively added also "emc" and "ec".

@hegza
Copy link
Author

hegza commented Feb 9, 2024

FPGA testing done. riscv-rt is capable of running a led blinker on an Ibex-type RV32EMC 🥳

almindor
almindor previously approved these changes Feb 9, 2024
Copy link
Contributor

@almindor almindor left a comment

Choose a reason for hiding this comment

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

LGTM, I have absolutely no way to test this however

@romancardenas
Copy link
Contributor

Looks good to me!! @hegza Should we leave it as WIP until you can test it further?

@hegza
Copy link
Author

hegza commented Feb 13, 2024

I think it's up to you whether you can accept the incremental support for emc without guarantees that the interrupt behavior is sound. If that's fine, please merge. It's easier for me to progress incrementally from here.

In time, I am likely to test some emc interrupts based on my fork at http://github.com/soc-hub-fi/pulpissimo-riscv/ which is a port of riscv-rt for Ibex. Ibex does not support direct-mode interrups so vectored must be the default.

If there's anything minor regarding code style I'd like to address it.

@romancardenas
Copy link
Contributor

The main issue I see is that the official Rust toolchain does not support these targets yet, so it is not possible to keep a simple CI to check that we don't break anything in the future.

@hegza
Copy link
Author

hegza commented Feb 13, 2024

Yeah I'd agree with that. I think it's a valid idea to wait for the official toolchain to catch up, then add the CI checks, then merge.

hegza added 5 commits March 15, 2024 11:37
As defined in RISC-V Unprivileged Spec. 20191213, the only change in RVE
is to reduce the number of integer registers to 16 (x0-x15). This requires
also a separate calling convention that seems a little underspecified.

Somebody on the internet said these are also implied:

- 2 callee saved registers instead of 12
- 32-bit / 4-byte stack alignment instead of 128 bits / 16 bytes

This patch does the following:

- Disable start of program load-to-zero for x16--x31 for RVE, since these do
  not exist on RVE
- Force & assert the stack pointer is 4-byte aligned on RVE
- Configure the trap handler to only caller-save a0--a5 and t0--t2 instead of
  a0--a7 and t0--t6 on RVE
- Disable a6--a7 (x16--x17) and t3--t6 (x28--x31) for RVE in the TrapFrame
  struct, since these do not exist on RVE

This may not be all that is required for RVE support, but this is at
least the minimal amount of changes that makes riscv-rt link on RVE,
allowing further testing.
The LLVM spurious error affects us too :)
I presume these are required if there happens to be any C or M
instructions in the binary.

At least the "rv32em" variant here is required for our rv32emc if
-Fsingle-hart is disabled.
RVE does not support registers x16--x31, including t3 (x28). I replaced
the use of t3 in the flash algorithm with a5 (x15) to make it compile.
@hegza hegza marked this pull request as draft March 15, 2024 11:08
@hegza hegza closed this Apr 26, 2024
@hegza hegza deleted the rve branch April 26, 2024 11:10
@hegza hegza restored the rve branch May 17, 2024 14:51
@hegza hegza deleted the rve branch May 17, 2024 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants