search

lowRISC / lowrisc-chip

The root repo for lowRISC project and FPGA demos.
http://www.lowrisc.org/
Other
596 stars 148 forks source link

Adding shadow CSRs for tagctrl #41

Closed wsong83 closed 7 years ago

asb commented 7 years ago

Hi Wei, my only review comment is for the changes to riscv-isa-sim. I think some necessary defines were accidentally deleted (DEFAULT_RSTVEC, DRAM_BASE amongst others). I'd just leave that block of defines (line 71-76 in encoding.h prior to the patch) as-is.

As we discussed, there's a question for the future about whether we want to add independent tagctrl CSRs for different privilege levels in order to aid context switching (or maybe we can just get away with having an additional scratch CSR?).

wsong83 commented 7 years ago

I had removed the DRAM_BASE macro deliberately. DRAM_BASE depends on the memory space configuration come from Chisel. A constant macro in encoding.h causing problems when the actual DRAM_BASE differs from the pre-set value. What I see is any program can either read the up-to-date value from the automatically generated dev_map.h or reading the configstring, either way the value is always in sync with hardware.

No special opinion against RSTVEC but just cannot find where it is used why the value is defined to 0x00001000. Why do you need it?

wsong83 commented 7 years ago

Need more check, do not merge!

asb commented 7 years ago

These defines are used in the spike implementation - i.e. it won't compile without them. encoding.h in other projects probably shouldn't have these defines, but for now spike needs them. Given that we hope to update all these projects to the latest upstream version soon anyway, I suggest it's probably easiest having a slightly different encoding.h in spike for now.

wsong83 commented 7 years ago

It looks like spike need these macros to build its own configstring. So the macro values are specific to the spike emulated machine. Can you add these macros in a spike header file instead of the shared encoding.h? In this way, there will be no difference in the encoding.h between repos (all encoding.h files are in the control of riscv-opcodes). Defining DRAM_BASE in encoding.h is a bad move to me and I am not really willing to add it back (the upstream code still have it).

asb commented 7 years ago

Looks good to me.