Spike modifications for cv32e40s reference model #2462

torjeikenes · 2024-06-05T12:30:38Z

This PR adds the changes to Spike necessary to support the reference model in #2432.
I have attempted to split it into two main contributions:

Added spike support for CV32E40S
- Configuration of memory regions
- Modifications to CSRs
- Modifications to RVFI
Modified spike to work with the reference model
- Injection of interrupts and debug requests
- Modifications to interrupt handling in spike, adding an interrupt_allowed check and external interrupts
- Store and revert to previous states

(@silabs-robin, I can't add reviewers)

- Inject interrupts and debug requests - Store and revert to previous states - Add a seperate interrupt_allowed bool

vendor/riscv/riscv-isa-sim/riscv/Proc.cc

silabs-robin · 2024-06-06T08:45:33Z

vendor/patches/riscv/riscv-isa-sim/0021-cv32e40s-support.patch

@@ -0,0 +1,415 @@
+diff --git a/vendor/riscv/riscv-isa-sim/riscv/Proc.cc b/vendor/riscv/riscv-isa-sim/riscv/Proc.cc


Were these patch files the source which some vendorization scripts turned into the rest of the file changes? How was it done?

I did not use the script. I just used git diff redirected to a .patch file and applied this.

How do we know that everything new in this PR is compatible with the existing script-based flow?

With my current setup on a server with an older git version, I cannot run the script in this branch or on master. Can you check if it runs for you? I get a git error when running this python3 util/vendor.py vendor/riscv_riscv-isa-sim.vendor.hjson

vendor/riscv/riscv-isa-sim/riscv/Types.h

silabs-robin · 2024-06-06T08:48:55Z

vendor/riscv/riscv-isa-sim/riscv/csrs.cc

-   openhw::reg::unlogged_write( openhw::reg::unlogged_read()-1);
+
+  // disable since we (at the moment) check mcounterinhibit
+  //openhw::reg::unlogged_write( openhw::reg::unlogged_read()-1);


What is this?

I modified this and the +1 below to add support for disabling mcycle counting with mcountinhibit to match cv32e40s. I have not tested this much with mcountinhibit enabled since mcycle would likely mismatch with the core anyway.

Is the old behavior preserved unless the new behavior is explicitly asked for?

I think so, but I have not done much testing with mcountinhibit enabled.
Since Spike "sees" fewer cycles than the core, mcycle would likely have a mismatch to a core anyway unless larger modifications are made.

silabs-robin · 2024-06-06T08:49:28Z

vendor/riscv/riscv-isa-sim/riscv/csrs.cc

  return true;
 }

 reg_t wide_counter_csr_t::written_value() const noexcept {
  // Re-adjust for upcoming bump()
-  return  openhw::reg::unlogged_read() + 1;
+  return  openhw::reg::unlogged_read(); //+ 1; // Disable since we dont always bump


Why didn't the +1 cause problems before? And should the code simply be deleted?

The +1 caused errors in cv32e40s_csr_access_test which reads the mcycle csr. Spike did not support disabling and enabling counting of mcycle with mcountinhibit. In 40S counting is disabled by default, so this caused a mismatch when the mcycle csr was read. Here mcycle counted up in Spike but not in the core, since mcountinhibit was enabled (disabling counting).

silabs-robin · 2024-06-06T08:49:41Z

vendor/riscv/riscv-isa-sim/riscv/csrs.cc

@@ -1239,9 +1242,10 @@ reg_t dcsr_csr_t::read() const noexcept {
  v = set_field(v, DCSR_EBREAKH, ebreakh);
  v = set_field(v, DCSR_EBREAKS, ebreaks);
  v = set_field(v, DCSR_EBREAKU, ebreaku);
-  v = set_field(v, DCSR_STOPCYCLE, 0);
+  v = set_field(v, DCSR_STOPCYCLE, 1); //TODO: Make configurable


This TODO needs resolution.
Plus several others.

Not sure if these changes also affect cva6, or if they need to be configurable to each core. This should also be checked with the script discussed above

DCSR_STOPCYCLE is bit 10, AKA dcsr.STOPCOUNT, and it is set to 1 to reflect https://docs.openhwgroup.org/projects/cv32e40s-user-manual/en/latest/control_status_registers.html#debug-control-and-status-dcsr ?
Is that correct?

If the entire Spike source now gets hardcoded to 40s-specifics, then I agree with the comment that it should be made configurable.

That is correct.
If these changes interfere with cva6, a parameter could be added to configure this. DCSR has changes both to DCSR_STOPCYCLE and DCSR_MPRVEN, so maybe one parameter could contain the full DCSR, or there could be multiple individual parameters to configure stopcycle and mprven .

vendor/riscv/riscv-isa-sim/riscv/riscv_dpi.cc

silabs-robin · 2024-06-06T11:22:50Z

vendor/riscv/riscv-isa-sim/riscv/processor.cc

@@ -643,7 +644,7 @@ void processor_t::set_mmu_capability(int cap)
 void processor_t::take_interrupt(reg_t pending_interrupts)
 {
  // Do nothing if no pending interrupts
-  if (!pending_interrupts) {
+  if (!pending_interrupts || !interrupt_allowed) {


Is the default case for interrupt_allowed handled such that other users won't get surprised by this?

Added a default value now

But is it possible for the surrounding logic to change this signal so that users of other processor cores get their Spike-based simulations tampered with unexpectedly?

Since interrupt_allowed is set to true by default and only modified by the added interrupt() method, this should not influence old behavior.

silabs-robin · 2024-06-06T12:20:48Z

vendor/patches/riscv/riscv-isa-sim/0021-cv32e40s-support.patch

For the other PR, even if I couldn't compile even vanilla Spike I am still about to run ci_check for 40s to test the PR branch.

But here, I don't know how I can try this out myself.

@MikeOpenHWGroup and @MarioOpenHWGroup, the least net use of energy is most likely if one of you could run some check that you are already used to running, which uses some of the files involved in this PR.

@torjeikenes Would one very minimal way to test this simply be to mkdir build; cd build; ../configure; make as in building spike within the correct directory in vendor/?

yes, mkdir build; cd build; ../configure; make in vendor/riscv/riscv-isa-sim should test that it builds ok.

silabs-robin