[Bug Report] incorrect xs field check under VS

[Phantom1003]

Type of issue: bug report
Impact: unknown
Development Phase: proposal
Other information
In the following test case, we first enable mpv and xs bit in mstatus, and then set up xs field in vsstatus. With the xs field, any custom instructions should not raise illegal signal.
From the co-simulation result, spike successfully executes the rocc instruction, while rocket throws an exception.

[rocket] 3 0x00080000178 (0x30005073)
[spike]  core   0: 0x0000000080000178 (0x30005073) csrwi   mstatus, 0
[rocket] 3 0x0008000017c (0x000182b7) x 5 0x0000000000018000
[spike]  core   0: 0x000000008000017c (0x000182b7) lui     t0, 0x18
[rocket] 3 0x00080000180 (0x3002a073)
[spike]  core   0: 0x0000000080000180 (0x3002a073) csrs    mstatus, t0
[rocket] 3 0x00080000184 (0x0010031b) x 6 0x0000000000000001
[spike]  core   0: 0x0000000080000184 (0x0010031b) addiw   t1, zero, 1
[rocket] 3 0x00080000188 (0x02731313) x 6 0x0000008000000000
[spike]  core   0: 0x0000000080000188 (0x02731313) slli    t1, t1, 39
[rocket] 3 0x0008000018c (0x30032073)
[spike]  core   0: 0x000000008000018c (0x30032073) csrs    mstatus, t1
...
[rocket] 3 0x000800001a4 (0x20029073)
[spike]  core   0: 0x00000000800001a4 (0x20029073) csrw    vsstatus, t0
[rocket] 3 0x000800001a8 (0x20002573) x10 0x8000000200018000
[spike]  core   0: 0x00000000800001a8 (0x20002573) csrr    a0, vsstatus
...
[rocket] 3 0x000800001bc (0x30200073)
[spike]  core   0: 0x00000000800001bc (0x30200073) mret
[rocket] 1 0x000800001c0 (0x00000613) x12 0x0000000000000000
[spike]  core   0: 0x00000000800001c0 (0x00000613) li      a2, 0
[rocket] 3 0x00080000004 (0x34302f73) x30 0x000000000000700b
[spike]  core   0: 0x00000000800001c4 (0x0000700b) custom0.rd.rs1.rs2 (args unknown)
[error] PC SIM 00000000800001c4, DUT 0000000080000004
[error] INSN SIM 0000700b, DUT 34302f73

rocket-7.zip

We believe there is a typo in the rocc_illegal signal, where the .vs should be .xs:

rocket-chip/src/main/scala/rocket/CSR.scala

Line 854 in 850e1d5

io_dec.rocc_illegal := io.status.xs === 0 || reg_mstatus.v && reg_vsstatus.vs === 0 || !reg_misa('x'-'a')

Please tell us about your environment:
- version: edc8d40

[Phantom1003]

@aswaterman Would you please confirm this report?

[aswaterman]

I would have said this differently: RoCC and Hypervisor extension are currently incompatible. This is a request for a new feature, not a report about a bug.

Please work with the folks at UC Berkeley who use RoCC to make sure they are OK with whatever you propose. I don't work with RoCC. (cc @jerryz123 to see who should work with you on this PR.)

[Phantom1003]

I still insist that this is a bug, and this report contains two problems.

1. The xs bits of vsstatus is writable, which is inconsistent with the specification:

Read-only fields SD and XS summarize the extension context status as it is visible to VS-mode
only. For example, the value of the HS-level sstatus.FS does not affect vsstatus.SD.

rocket-chip/src/main/scala/rocket/CSR.scala

Lines 1337 to 1347 in 850e1d5

	when (decoded_addr(CSRs.vsstatus)) {
	val new_vsstatus = new MStatus().fromBits(wdata)
	reg_vsstatus.sie := new_vsstatus.sie
	reg_vsstatus.spie := new_vsstatus.spie
	reg_vsstatus.spp := new_vsstatus.spp
	reg_vsstatus.mxr := new_vsstatus.mxr
	reg_vsstatus.sum := new_vsstatus.sum
	reg_vsstatus.fs := formFS(new_vsstatus.fs)
	reg_vsstatus.vs := formVS(new_vsstatus.vs)
	if (usingRoCC) reg_vsstatus.xs := Fill(2, new_vsstatus.xs.orR)
	}

2. The rocc_illegal signal incorrectly uses vs field, which is used for vector_illegal. And if RoCC and Hypervisor extension are currently incompatible, there should be reg_mstatus.v && usingRoCC instead of reg_vsstatus.vs.
   

   rocket-chip/src/main/scala/rocket/CSR.scala

   Lines 851 to 854 in 850e1d5

   	io_dec.fp_illegal := io.status.fs === 0 || reg_mstatus.v && reg_vsstatus.fs === 0 || !reg_misa('f'-'a')
   	io_dec.vector_illegal := io.status.vs === 0 || reg_mstatus.v && reg_vsstatus.vs === 0 || !reg_misa('v'-'a')
   	io_dec.fp_csr := decodeFast(fp_csrs.keys.toList)
   	io_dec.rocc_illegal := io.status.xs === 0 || reg_mstatus.v && reg_vsstatus.vs === 0 || !reg_misa('x'-'a')

[aswaterman]

I agree XS should not be directly writable, but it’s only a bug that manifests with RoCC enabled, and so the maintainers/users of RoCC (i.e. not me :-)) should deal with it.

I also agree that rocc_illegal should use XS, not VS, but this is really an unrelated topic. Please keep bug reports to one bug at a time.

[jerryz123]

The last time this came up was 2 years ago. This issue gives me deja-vu.

I believe XS should be read-only, but hard wired to dirty when RoCC is present. #2508

[aswaterman]

I agree that’s the lowest-effort-but-seemingly-correct thing to do. It also makes it obvious what to do with vsstatus.

[Phantom1003]

Thanks to Andrew and Jerry, the bugs mentioned have been fixed.