Repository navigation
[rtl] Trap on reserved Zcmp encodings without starting an expansion - #2498
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
2ab4b21 to
bdc47c0
Compare
SamuelRiedel
left a comment
SamuelRiedel
left a comment
There was a problem hiding this comment.
Thank you for fixing those and providing a test. It looks good to me, I just have two quick questions.
Also, once we merge this, let's create an issue to clean up the test and the cosim flag once the compiler and the DV update land.
| test_srcs: zcmp_reserved_test/zcmp_reserved_test.S | ||
| config: riscv-tests | ||
| rtl_params: | ||
| PMPEnable: 1 |
There was a problem hiding this comment.
PMP is not strictly necessary for this test, is it?
There was a problem hiding this comment.
The Zcmp checks don't need PMP, but the riscv-tests startup code does on Ibex. Without PMP its pmpaddr0 write traps, and because Ibex aligns the mtvec base to 256 bytes, the trap lands in the test's handler, which then fails. I confirmed this on maxperf. The test has to repeat PMPEnable: 1 because its own rtl_params replace the riscv-tests defaults instead of merging with them.
| CHECK_RESERVED_MV 9, 0xac06 | ||
| CHECK_RESERVED_MV 26, 0xac46 |
There was a problem hiding this comment.
Is there a specific reason for jumping to 26 here for the test number?
There was a problem hiding this comment.
No, that was an oversight on my side. The two cm.mv* checks (0xac06 and 0xac46) were added together in a later revision, to cover both reserved funct2 values of that group. The first was slotted in as 9, but the second was numbered after the existing checks instead of next to it. I've renumbered the checks, and the controls after them, so they now count up from 1 to 30 in order. Each check also folds its number into the register values it plants and re-checks, so those values moved too. Every check still tests the same encoding in the same way, and the test still passes.
bdc47c0 to
9b5dfb6
Compare
|
Thanks! Agreed, I'll open an issue for that once this is merged. |
Zcmp reserves cm.mvsa01 with r1s' == r2s', since both fields name destinations. The decoder only checked funct2 and expanded it into two writes to the same register. Raise an illegal-instruction exception instead, without starting the expansion, as the decoder already does for the reserved rlists of cm.push and cm.pop. Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
cm.push, cm.pop, cm.popret and cm.popretz with rlist 0-3 raise illegal_instr_o but stay marked as expanded. So the RVFI trap row shows the 32-bit uop instead of the fetched 16-bit encoding, and the controller treats the trap as part of an atomic Zcmp sequence, holding off debug requests, single step and triggers around it. mcause and mtval were already right. Mark these paths INSTR_NOT_EXPANDED. IbexIllegalInstrNotExpanded checks that no illegal instruction from the compressed decoder is marked as expanded. Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
9b5dfb6 to
0db3069
Compare
hcallahan-lowrisc
left a comment
hcallahan-lowrisc
left a comment
There was a problem hiding this comment.
Thanks a lot @kulan-pal. The DV changes look good to me, and I'm happy if Sam has ack'd on the RTL changes.
| TEST_DATA | ||
| .balign 16 | ||
| stack_bot: | ||
| .fill 16, 4, 0 |
There was a problem hiding this comment.
I think the .data stack only needs to reserve 16 bytes (4 words) the checks touch
| .fill 16, 4, 0 |
There was a problem hiding this comment.
Agreed, the checks only use four words. They sit below stack_top (-4(sp) to -16(sp)), so I kept four words under the label and dropped the unused ones above it instead.
Nothing in core_ibex runs a reserved Zcmp encoding: riscv-dv cannot generate Zcmp and the vendored suites predate it. zcmp_reserved_test runs the eight cm.mvsa01 encodings with equal registers, the two reserved funct2 values of cm.mv*, and cm.push, cm.pop, cm.popretz and cm.popret with rlist 0-3. Each must trap once with mcause 2 and mtval equal to the encoding, and must not touch the registers or stack words a real expansion would. Legal neighbours run as controls. The test runs with +disable_cosim=1 (mismatches are not fatal) because the cosim does not enable Zcmp in Spike, which traps on every cm.* instruction. The self-checks decide the result. Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
0db3069 to
8982fc7
Compare
3 participants
Two reserved Zcmp cases are handled wrongly:
cm.mvsa01with both register fields equal is reserved, but the decoder only checkedfunct2and expanded itinto two moves to the same register. The eight encodings (
0xac22,0xaca6, ...,0xafbe) execute instead oftrapping.
cm.push/cm.pop/cm.popret/cm.popretzwith rlist 0-3 do trap, but stay marked as expanded. The RVFI traprow then shows a uop instead of the fetched halfword, and a debug request arriving with the trap is held off.
The first commit makes the equal-register encodings illegal. The second marks the rlist traps as not expanded and
adds the assertion
IbexIllegalInstrNotExpanded. The third addszcmp_reserved_test, which runs 26 reservedencodings (the eight equal-register
cm.mvsa01, one encoding for each reservedfunct2, and rlist 0-3 of eachpush/pop with
spimm = 0) plus legal neighbours, and checksmcause,mtvaland that no register or stack wordchanges.
zcmp_reserved_teston e1a6be2opentitanandmaxperf-pmp-bmbalancedcm.mvsa01(0xac22writess0twice)0xb802)The test runs with
+disable_cosim=1because the cosim does not enable Zcmp in Spike yet (#2324). The lowRISCSpike fork has the same
cm.mvsa01bug; I will send a fix there separately.AI disclosure (CLA §9): written with help from Claude Code and reviewed with OpenAI Codex; I have reviewed and understood every change and take full responsibility for it.