Add Smstateen/Ssstateen Extension and CSRs

[neverlandiz]

This PR addresses Issue #547 and adds the Smstateen extension, along with the following Smstateen/Ssstateen CSRs

• mstateen0, mstateen1, mstateen2, mstateen3
• mstateen0h, mstateen1h, mstateen2h, mstateen3h
• hstateen0, hstateen1, hstateen2, hstateen3
• hstateen0h, hstateen1h, hstateen2h, hstateen3h
• sstateen0, sstateen1, sstateen2, sstateen3

[ThinkOpenly]

My comments in hstateen0/hstateen0h generally apply to the others as well.

[dhower-qc]

The comments apply to all the CSRs.

[dhower-qc]

We also need an arch/ext/Ssstateen.yaml file.

[neverlandiz]

I think there is already a Ssstateen.yaml file?

[dhower-qc]

I think there is already a Ssstateen.yaml file?

You're right. Ignore my ignorance please ;)

[ThinkOpenly]

Interesting CI failures!

/home/runner/work/riscv-unified-db/riscv-unified-db/lib/idl/ast.rb:298:in `type_error': In file gen/arch/rv32/csr/hstateen0h.yaml (Idl::AstNode::TypeError)
On line 147
In the code:

  146: return **HERE** >> $bits(CSR[hstateen0])[63:32] << **HERE**;

A type error occurred
  Range too large for bits (msb = 63, range size = 32)

'hstateen0his trying to serve as an alias forhstateen0[63:32], but in RV32 mode, hstateen0is only 32 bits, so those "upper" bits don't exist. The simplest way to "fix" this would be to makehstateen0` be 64 bits always, but that seems to violate the text of the spec (4.1 State Enable Extensions):

For RV32, [hstateen0 is] 32-bit, and [...] there is a corresponding ... high-half [CSR] for the upper 32 bits: [...] hstateen0h [...].

Conversely, it would seem to violate the text if hstateen0h existed in RV64 mode (and the high bits of hstateen0 were aliases of it).

I was wondering if we could implement a new "internally" visible 64-bit CSR to which they both could alias, but not sure we really want to diverge from the spec in that way.

The only solution I can think of at the moment is to have hstateen0h (and friends) actually implement (not alias) those "upper 32 bits" of hstateen0.

Any other thoughts, @dhower-qc ?

[dhower-qc]

The spec isn't self-consistent here. It states two things simultaneously:

• hstateen0 is 32-bits in RV32
• hstateen0 has an 'upper' 32-bits in RV32 that can be accessed through hstateen0h (high-half [CSR] for the upper 32 bits)

I think the intent is that hstateen0 is always 64 bits but that 32 aren't visible through Zicsr instructions in RV32. Otherwise, the word "upper" doesn't make any sense.

I recommend making all the stateenN CSRs 64-bit rather than MXLEN, and maybe putting in a clarification request on the manual.

[ThinkOpenly]

The spec isn't self-consistent here. It states two things simultaneously:

• hstateen0 is 32-bits in RV32
• hstateen0 has an 'upper' 32-bits in RV32 that can be accessed through hstateen0h (high-half [CSR] for the upper 32 bits)

I think the intent is that hstateen0 is always 64 bits but that 32 aren't visible through Zicsr instructions in RV32. Otherwise, the word "upper" doesn't make any sense.

That's not how I read it. I read this as literal:

For RV32, the registers listed above are 32-bit

And this as a terse convention:

corresponding set of high-half CSRs for the upper 32 bits of each register...

...indicating that the 32-bit high-half CSRs represent the upper 32 bits of the RV64 representations of the associated registers.

...which I think is pretty much how we all understood it, but that does force us to figure out a good representation for it.

[jordancarlin]

In case you haven't seen it, there is a related discussion about CSR registers vs CSR addresses towards the end of this issue: riscv/riscv-isa-manual#1255.

[ThinkOpenly]

In case you haven't seen it, there is a related discussion about CSR registers vs CSR addresses towards the end of this issue: riscv/riscv-isa-manual#1255.

That's a very interesting read. Thanks for the pointer, @jordancarlin!

To me, that discussion pretty clearly asserts that 64-bit CSRs that, in RV32 mode, have "upper half" *h CSR addresses are architecturally 64-bit registers.

However, that would make the spec incorrect when it says:

For RV32, the registers listed above are 32-bit

"The registers listed above" are mstateen[0-3], stateen[0-3], and hstateen[0-3].

[ThinkOpenly]

In case you haven't seen it, there is a related discussion about CSR registers vs CSR addresses towards the end of this issue: riscv/riscv-isa-manual#1255.

That's a very interesting read. Thanks for the pointer, @jordancarlin!

To me, that discussion pretty clearly asserts that 64-bit CSRs that, in RV32 mode, have "upper half" *h CSR addresses are architecturally 64-bit registers.

However, that would make the spec incorrect when it says:

For RV32, the registers listed above are 32-bit

"The registers listed above" are mstateen[0-3], stateen[0-3], and hstateen[0-3].

riscv/riscv-isa-manual#1255 turned into riscv/riscv-isa-manual#1966, so I added the observation as a comment there (riscv/riscv-isa-manual#1966 (comment)).

I should probably turn the suggested text therein into a PR.

[ThinkOpenly]

I should probably turn the suggested text therein into a PR.

Done: riscv/riscv-isa-manual#2066.

Presuming the discussion in that issue is dispositive, I'm with Derek's suggestion above:

I recommend making all the stateenN CSRs 64-bit rather than MXLEN

Nicely, this simplifies things here and elsewhere.

[ThinkOpenly]

You might have run into a bug in the Ruby code...

/home/runner/work/riscv-unified-db/riscv-unified-db/lib/cfg_arch.rb:433:in `map': undefined method `version' for Extension#A:Extension (NoMethodError)

        extensions.map(&:version).flatten
                  ^^^^
Did you mean?  versions

I'm not familiar with this code, but it looks like it should be "versions". @dhower-qc ?

[ThinkOpenly]

Running your branch with the following change got the regress-gen-isa-manual test to pass:

diff --git a/lib/cfg_arch.rb b/lib/cfg_arch.rb
index f60ed1121..16de8bf4b 100644
--- a/lib/cfg_arch.rb
+++ b/lib/cfg_arch.rb
@@ -433 +433 @@ class ConfiguredArchitecture < Architecture
-        extensions.map(&:version).flatten
+        extensions.map(&:versions).flatten

If you want to include that change with this PR, I would not object.

[neverlandiz]

Sounds good, I'll add that change then.

[ThinkOpenly]

ping @dhower-qc