[RFC] make XLENs changable #1167

liweiwei90 · 2022-12-08T08:40:20Z

This patchset add the support for changable XLENs:

Instruction functions use different XLENs for different privilege modes
Make *XL field of *status CSRs writable
The length of CSRs is changed to the XLEN of its related privilege mode
Limit the *XL to legal value(1 or 2 or unchanged)
Ensure MXLEN >= SXLEN >=VSXLEN/VUXLEN/UXLEN, and VSXLEN >= VUXLEN

Tests for changable XLENs are lacked currently.

aswaterman · 2022-12-08T08:45:56Z

I think we should take a step back and determine whether Spike really needs to support this feature. It adds a lot of complexity, but it isn’t clearly of high value.

More generally, we should look at the roadmap of RISC-V features and decide what Spike needs to support and what can be omitted.

liweiwei90 · 2022-12-08T09:25:54Z

This seems to be a very interesting feature supported by RISC-V. As spike is the golden model, I think adding some complexity to support this feature seems acceptable. It introduces minor change to the main stream of simulation. Most of the additional complexity is for CSRs access.

scottj97

Commit a4f6efe does way more than what its commit message claims.

Don't make XLEN fields writable before everything else is fixed. We don't want Spike to be broken at any commit point. You should be able to change all the CSRs to support dynamic XLEN first (one at a time!), and everything should still work with fixed XLEN. The last step is to enable writable XLEN fields.

No tests == doesn't work.

I concur with @aswaterman that this might not be desirable to have. Is there any demand for this support? I'm not aware of any RISC-V implementation that supports this capability.

scottj97 · 2022-12-08T12:48:54Z

riscv/csrs.cc

-  basic_csr_t(proc, addr, 0) {
+cause_csr_t::cause_csr_t(processor_t* const proc, const reg_t addr, uint8_t prv_index):
+  basic_csr_t(proc, addr, 0),
+  prv_index(prv_index) {
 }

 reg_t cause_csr_t::read() const noexcept {
  reg_t val = basic_csr_t::read();
  // When reading, the interrupt bit needs to adjust to xlen. Spike does
  // not generally support dynamic xlen, but this code was (partly)


Comment is now out of date

OK. I'll modify it.

scottj97 · 2022-12-08T12:49:50Z

riscv/csrs.cc

 }

 reg_t cause_csr_t::read() const noexcept {
  reg_t val = basic_csr_t::read();
  // When reading, the interrupt bit needs to adjust to xlen. Spike does
  // not generally support dynamic xlen, but this code was (partly)
  // there since at least 2015 (ea58df8 and c4350ef).
-  if (proc->get_isa().get_max_xlen() > proc->get_xlen()) // Move interrupt bit to top of xlen
-    return val | ((val >> (proc->get_isa().get_max_xlen()-1)) << (proc->get_xlen()-1));
+  unsigned xlen = proc->get_xlen(prv_index & 3, prv_index & 4);


Don't use bit fields when C++ has much better data structures. Make a new struct and pass that to get_xlen().

OK. I'll adjust it.

scottj97 · 2022-12-08T12:55:27Z

riscv/csrs.cc

@@ -500,19 +545,51 @@ bool mstatus_csr_t::unlogged_write(const reg_t val) noexcept {

  const reg_t requested_mpp = proc->legalize_privilege(get_field(val, MSTATUS_MPP));
  const reg_t adjusted_val = set_field(val, MSTATUS_MPP, requested_mpp);
-  const reg_t new_mstatus = (read() & ~mask) | (adjusted_val & mask);
+  reg_t new_mstatus = (this->val & ~mask) | (adjusted_val & mask);


Why can't this still be const? I don't see it being modified below...

Yeah. It's not necessary currently. I'll restore it.

scottj97 · 2022-12-08T12:58:22Z

riscv/csrs.cc

+    int orig_uxl = get_field(new_mstatus, MSTATUS_UXL);
+    int new_uxl = legalize_xl(get_field(val, MSTATUS_UXL), orig_uxl);
+    int orig_sxl = get_field(new_mstatus, MSTATUS_SXL);
+    int new_sxl = legalize_xl(get_field(val, MSTATUS_SXL), orig_sxl);


Why can't you do these adjustments up above where all the other fields (e.g. MPP) are legalized? Then write this->val only once with the final value in the end.

In current implementation, update_sxl may modify uxl field of mstatus. Maybe I can add this to the legalize_xl function.

riscv/processor.h

scottj97 · 2022-12-08T13:04:39Z

riscv/csrs.cc

@@ -1239,7 +1239,7 @@ bool hgatp_csr_t::unlogged_write(const reg_t val) noexcept {
  proc->get_mmu()->flush_tlb();

  reg_t mask;
-  if (proc->get_const_xlen() == 32) {
+  if (proc->get_xlen(PRV_S) == 32) {


Not a safe change, since this CSR's implementation still assumes a const xlen. (Because it doesn't dynamically adjust its shape when xlen changes.)

Why the CSR's implementation is const xlen? Hgatp is stated as HSXLEN bits in spec . So I think its length is changable.

If I do CSRW when HSXLEN=64 and then read it back when HSXLEN=32 what happens?

I'm not even sure what's supposed to happen in that case, since the MODE field changes interpretation. But at least the VMID field needs to move. And there's no code to do that in this implementation.

The fields for HSXLEN=64 cannot totally map to fields for HSXLEN=32, even though they have the same field names. I think this may be the work of software.

Yes, what's supposed to happen here when XLEN changes doesn't seem to be well-specified. But at least we could make some effort not to leave total garbage in there -- move VMID, reset Mode, etc?

I think you're pioneering a lot of architectural work here, and there will be many more questions like this that we don't have good answers for.

“Complete garbage” might be what’s required by the CSR width modulation section in chapter 2. I think it requires a bitwise copy of the whole register, even if bits end up in the wrong fields.

The current implementation is to use bitwise copy for most CSRs, by leaving them alone, and truncating their length to their related XLEN when read and write.
However, the CSR value may be unchanged if their XLEN is back-to-back changed(64->32->64). This seems a little different from the description from the CSR width modulation section in chapter 2：
“If the new width is wider than the previous width, the temporary register is zero-extended to the wider width”

“Complete garbage” might be what’s required by the CSR width modulation section in chapter 2. I think it requires a bitwise copy of the whole register, even if bits end up in the wrong fields.

The opening paragraph there says: "fields and bits of the new-width CSR are, unless specified otherwise, determined..."

I think the two different figures for hgatp for HSXLEN of 32/64 count as "specified otherwise." VMID and MODE move and it wouldn't be correct to jam the upper bits of PPN into the VMID field when switching from 64 to 32.

This kind of per-field relocation might not be well explained in the spec, but is necessary in order for changing misa.MXL back and forth between 2 (xlen=64) and 1 (xlen=32) to work.

But it's because of architectural questions like this that I am very leery of this being the right time & place to pioneer this feature.

scottj97 · 2022-12-08T13:18:39Z

riscv/csrs.cc

 }

 bool vsstatus_csr_t::unlogged_write(const reg_t val) noexcept {
-  const reg_t newval = (this->val & ~sstatus_write_mask) | (val & sstatus_write_mask);
+  unsigned vslen = proc->get_xlen(PRV_S, true);


Use consistent names. In the previous method this was vsxlen

OK. I'll modify it.

scottj97 · 2022-12-08T15:56:20Z

f8e1509 does not compile

aswaterman · 2022-12-08T20:08:34Z

Historically, other architectures had the feature to change XLEN to facilitate an ISA transition. When x86-64 processors were deployed, IA-32 software was still the norm, and so the processors wouldn’t have been commercially successful without the ability to run IA-32 code. RISC-V is not in the same boat: RV32 and RV64 were created simultaneously, and so there was no such transition. RV64 is already the norm for applications-class processors. For this reason, I suspect few vendors will choose to implement this feature, and so there probably won’t be much demand to support it in Spike.

If I’m proven wrong about the demand, then obviously I’d change my opinion and would support adding this feature to Spike.

I imagine RV128 processors might need this feature (but RV128 support will be a major feature for other reasons).

liweiwei90 · 2022-12-09T06:26:13Z

Thanks for your comments.

Commit a4f6efe does way more than what its commit message claims.

Don't make XLEN fields writable before everything else is fixed. We don't want Spike to be broken at any commit point. You should be able to change all the CSRs to support dynamic XLEN first (one at a time!), and everything should still work with fixed XLEN. The last step is to enable writable XLEN fields.

OK. I'll reorganized the commits.

No tests == doesn't work.

Yeah. I'll try to adjust the riscv-tests to test it(any other seggestion?). The main reason I sent this RFC is to discuss whether there are problems in my understanding of how XLENs work.

I concur with @aswaterman that this might not be desirable to have. Is there any demand for this support? I'm not aware of any RISC-V implementation that supports this capability.

I search the RISC-V implementations, and find Xuantie C908 seems to support partial of this feature:
"Support for RV32 COMPAT mode which allows for 64-bit RISC-V CPUs to run 32-bit binary code" (https://www.cnx-software.com/2022/11/04/t-head-xuantie-c908-risc-v-core-targets-aiot-applications/)

liweiwei90 · 2022-12-09T06:31:22Z

Historically, other architectures had the feature to change XLEN to facilitate an ISA transition. When x86-64 processors were deployed, IA-32 software was still the norm, and so the processors wouldn’t have been commercially successful without the ability to run IA-32 code. RISC-V is not in the same boat: RV32 and RV64 were created simultaneously, and so there was no such transition. RV64 is already the norm for applications-class processors. For this reason, I suspect few vendors will choose to implement this feature, and so there probably won’t be much demand to support it in Spike.

Yeah. I didn't find an implementation to support all of this feature. Xuantie C908 seems to support partial of it.

If I’m proven wrong about the demand, then obviously I’d change my opinion and would support adding this feature to Spike.

I imagine RV128 processors might need this feature (but RV128 support will be a major feature for other reasons).

I have a question of RV128: there is description about the xlen changing from 32 to wider, how about 64 to 128?

aswaterman · 2022-12-09T06:38:45Z

I have a question of RV128: there is description about the xlen changing from 32 to wider, how about 64 to 128?

RV128 isn’t defined yet, so this isn’t fully defined, either. The presumption is that this feature will support RV128 in the obvious way.

aswaterman · 2022-12-09T06:40:10Z

Yeah. I didn't find an implementation to support all of this feature. Xuantie C908 seems to support partial of it.

Yeah, this isn’t compelling evidence of demand (especially since it sounds like a nonconforming implementation).

liweiwei90 · 2022-12-09T07:27:22Z

Yeah, this isn’t compelling evidence of demand (especially since it sounds like a nonconforming implementation).

Sorry, I don't know how much it supports currently. Why I said 'partial' is that it doesn't support 'H' extension which means VSXLEN/VUXLEN will not work.

liweiwei90 · 2022-12-09T07:31:01Z

f8e1509 does not compile

rv64_rv32_low_csr_t is used in the next commit.

scottj97 · 2022-12-09T19:07:50Z

f8e1509 does not compile

rv64_rv32_low_csr_t is used in the next commit.

If it doesn't compile, that's a problem. Here's the beginning of the giant list of errors that I get when doing a clean compile from f8e1509:

g++ -MMD -MP  -DPREFIX=\"/usr/local\" -Wall -Wno-unused -Wno-nonportable-include-path -g -O2 -fPIC -g -O2 -DPREFIX=\"/usr/local\" -Wall -Wno-unused -Wno-nonportable-include-path -g -O2 -fPIC -std=c++17 -g -O2   -I. -I/local_home/sjohnson/spike-regress/riscv-isa-sim -I/local_home/sjohnson/spike-regress/riscv-isa-sim/fesvr -I/local_home/sjohnson/spike-regress/riscv-isa-sim/riscv -I/local_home/sjohnson/spike-regress/riscv-isa-sim/disasm -I/local_home/sjohnson/spike-regress/riscv-isa-sim/customext -I/local_home/sjohnson/spike-regress/riscv-isa-sim/fdt -I/local_home/sjohnson/spike-regress/riscv-isa-sim/softfloat -I/local_home/sjohnson/spike-regress/riscv-isa-sim/spike_main -I/local_home/sjohnson/spike-regress/riscv-isa-sim/spike_dasm -I/include/boost-0 -x c++-header /local_home/sjohnson/spike-regress/riscv-isa-sim/riscv/insn_template.h -c -o insn_template.h.gch
In file included from /local_home/sjohnson/spike-regress/riscv-isa-sim/riscv/processor.h:18,
                 from /local_home/sjohnson/spike-regress/riscv-isa-sim/riscv/mmu.h:11,
                 from /local_home/sjohnson/spike-regress/riscv-isa-sim/riscv/insn_template.h:4:
/local_home/sjohnson/spike-regress/riscv-isa-sim/riscv/triggers.h:9:1: error: expected unqualified-id before ‘namespace’
 namespace triggers {
 ^~~~~~~~~
In file included from /local_home/sjohnson/spike-regress/riscv-isa-sim/fesvr/memif.h:9,
                 from /local_home/sjohnson/spike-regress/riscv-isa-sim/riscv/processor.h:19,
                 from /local_home/sjohnson/spike-regress/riscv-isa-sim/riscv/mmu.h:11,
                 from /local_home/sjohnson/spike-regress/riscv-isa-sim/riscv/insn_template.h:4:
/local_home/sjohnson/spike-regress/riscv-isa-sim/fesvr/byteorder.h:68:88: error: invalid use of qualified-name ‘rv32_high_csr_t::target_endian<T>::zero’
 template<typename T> const target_endian<T> target_endian<T>::zero = target_endian(T(0));
                                                                                        ^
/local_home/sjohnson/spike-regress/riscv-isa-sim/fesvr/byteorder.h:69:93: error: invalid use of qualified-name ‘rv32_high_csr_t::target_endian<T>::all_ones’
 template<typename T> const target_endian<T> target_endian<T>::all_ones = target_endian(~T(0));
                                                                                             ^
/local_home/sjohnson/spike-regress/riscv-isa-sim/fesvr/byteorder.h:74:10: error: explicit specialization in non-namespace scope ‘class rv32_high_csr_t’
 template<> class target_endian<uint8_t> : public base_endian<uint8_t> {
          ^

liweiwei90 · 2022-12-10T02:53:35Z

OK, there is bug in the code(Maybe not the unused rv64_rv32_low_csr_t). I'll fix it later.

scottj97

ee8c63f is way too big and does way too much at once. It also changes behavior of csrw minstret, changing the value written to the CSR by 1. It also changes logging of mstatush sometimes, though that behavior is then reverted back by ffe656a.

dbfbc9f somehow changes the values in mstatush bits MBE and SBE.

Though I still don't think this is the right time for this feature.

liweiwei90 · 2023-01-06T07:53:57Z

ee8c63f is way too big and does way too much at once. It also changes behavior of csrw minstret, changing the value written to the CSR by 1. It also changes logging of mstatush sometimes, though that behavior is then reverted back by ffe656a.

OK. I'll reorganize them later.

dbfbc9f somehow changes the values in mstatush bits MBE and SBE.

Sorry. I didn't find the way to change MBE and SBE in this commit. Can you tell me how it works?

Though I still don't think this is the right time for this feature.

OK. Maybe we can keep it in development here.

scottj97 · 2023-01-06T19:41:06Z

dbfbc9f somehow changes the values in mstatush bits MBE and SBE.

Sorry. I didn't find the way to change MBE and SBE in this commit. Can you tell me how it works?

My test is proprietary (sorry) but here's all it does with mstatus or mstatush:

core   0: 0x80000160 (0x30005073) csrwi   mstatus, 0
core   0: 3 0x80000160 (0x30005073) c768_mstatus 0x00000000
core   0: 0x80000164 (0x00002537) lui     a0, 0x2
core   0: 3 0x80000164 (0x00002537) x10 0x00002000
core   0: 0x80000168 (0x80050513) addi    a0, a0, -2048
core   0: 3 0x80000168 (0x80050513) x10 0x00001800
core   0: 0x8000016c (0x30052073) csrs    mstatus, a0
core   0: 3 0x8000016c (0x30052073) c768_mstatus 0x00001800
core   0: 0x80000170 (0x00000297) auipc   t0, 0x0
core   0: 3 0x80000170 (0x00000297) x5  0x80000170
core   0: 0x80000174 (0x01428293) addi    t0, t0, 20
core   0: 3 0x80000174 (0x01428293) x5  0x80000184
core   0: 0x80000178 (0x34129073) csrw    mepc, t0
core   0: 3 0x80000178 (0x34129073) c833_mepc 0x80000184
core   0: 0x8000017c (0xf1402573) csrr    a0, mhartid
core   0: 3 0x8000017c (0xf1402573) x10 0x00000000
core   0: 0x80000180 (0x30200073) mret
core   0: 3 0x80000180 (0x30200073) c768_mstatus 0x00000080

... some time later ...

core   0: 0x80000218 (0x31002273) csrr    tp, mstatush
core   0: 3 0x80000218 (0x31002273) x4  0x00000005

Before dbfbc9f, that last csrr wrote 0 to x4.

It would help if you broke up dbfbc9f into several steps: first keep sxl/uxl fixed, but move the computation of those fields into mstatus_csr_t::read(). Then make them writable in a second commit.

riscv/csrs.cc

liweiwei90 · 2023-01-09T02:16:15Z

... some time later ...
core   0: 0x80000218 (0x31002273) csrr    tp, mstatush
core   0: 3 0x80000218 (0x31002273) x4  0x00000005
Before dbfbc9f, that last csrr wrote 0 to x4.

It would help if you broke up dbfbc9f into several steps: first keep sxl/uxl fixed, but move the computation of those fields into mstatus_csr_t::read(). Then make them writable in a second commit.

Yeah. This is the UXL and SXL fields problem. I didn't clear it when MXLEN = 32. I'll fix it later.

scottj97 · 2023-01-09T14:54:04Z

Yeah. This is the UXL and SXL fields problem. I didn't clear it when MXLEN = 32. I'll fix it later.

Right. My mistake, that wasn't MBE/SBE bits.

…ll be checked by their verify_permissions) Add rv64_rv32_low_csr_t class for 64bits CSRs in RV64 that are also low half CSRs in RV32

…lated xlen

Remove unsed xlen_to_uxl function

Remove unused get_const_xlen function

liweiwei90 force-pushed the plct-rv32u-dev branch from f186dea to 81639ec Compare December 8, 2022 11:34

scottj97 suggested changes Dec 8, 2022

View reviewed changes

liweiwei90 force-pushed the plct-rv32u-dev branch 2 times, most recently from aa47842 to 3ecb188 Compare January 5, 2023 12:17

scottj97 suggested changes Jan 5, 2023

View reviewed changes

scottj97 suggested changes Jan 6, 2023

View reviewed changes

riscv/csrs.cc Show resolved Hide resolved

Weiwei Li added 8 commits January 10, 2023 22:17

Use different xlen(mxlen,sxlen,uxlen,vsxlen,vuxlen) for different mode

04b31d2

Make PMPCFG1/3/... only work when mxlen==32

6eabeec

Bind *cause CSRs with related privilege mode

e4015a1

Bind *satp CSRs with related privilege mode

cb6927c

Bind Satp CSR length with xlen for related privilege mode

08118c7

Bind rv32_high_csr_t CSRs with related privilege mode

3c2101c

Add rv32_counter_proxy_csr_t for counter related high-half CSRs

49dca07

Adjust mstatush write based on mxlen

c2241bf

Weiwei Li added 9 commits January 10, 2023 22:17

Add all RV32 only CSRs to csrmap array (whether related xlen is 32 wi…

3ef13fe

…ll be checked by their verify_permissions) Add rv64_rv32_low_csr_t class for 64bits CSRs in RV64 that are also low half CSRs in RV32

Adjust interrupt bit of *cause CSRs based on mxlen

16e318b

Adjust the read-out and write-in value for CSRs based on the their re…

859ac56

…lated xlen

Adjust xlen for triggers

c5e5aac

Make UXL field in vsstatus writable

bc2c175

Make VSXL field in hstatus writable

39360e7

Remove unsed xlen_to_uxl function

Make UXL/SXL fields in mstatus writable

35accd9

Remove unused get_const_xlen function

Make MXL fields in misa writable

b68cde0

Remove unsed paddr_bits function

977381d

liweiwei90 force-pushed the plct-rv32u-dev branch from 3ecb188 to 977381d Compare January 10, 2023 15:10

[RFC] make XLENs changable #1167

Are you sure you want to change the base?

[RFC] make XLENs changable #1167

Conversation

liweiwei90 commented Dec 8, 2022

aswaterman commented Dec 8, 2022

liweiwei90 commented Dec 8, 2022

scottj97 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liweiwei90 Dec 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottj97 commented Dec 8, 2022

aswaterman commented Dec 8, 2022 • edited Loading

liweiwei90 commented Dec 9, 2022 • edited Loading

liweiwei90 commented Dec 9, 2022

aswaterman commented Dec 9, 2022

aswaterman commented Dec 9, 2022

liweiwei90 commented Dec 9, 2022 • edited Loading

liweiwei90 commented Dec 9, 2022

scottj97 commented Dec 9, 2022

liweiwei90 commented Dec 10, 2022 • edited Loading

scottj97 left a comment

Choose a reason for hiding this comment

liweiwei90 commented Jan 6, 2023

scottj97 commented Jan 6, 2023

liweiwei90 commented Jan 9, 2023

scottj97 commented Jan 9, 2023

liweiwei90 Dec 10, 2022 •

edited

Loading

aswaterman commented Dec 8, 2022 •

edited

Loading

liweiwei90 commented Dec 9, 2022 •

edited

Loading

liweiwei90 commented Dec 9, 2022 •

edited

Loading

liweiwei90 commented Dec 10, 2022 •

edited

Loading