[rtl] Fix the minstret counter

[SamuelRiedel]

This PR resolves two different RTL issues that lead to inaccurate instret counts.

Fix minstret update after explicit write:

Previously, a write to minstret would set the value in the ID stage and then increment it in the WB stage again. This doesn't match the RISC-V Specification Section 6.1:

Some CSRs, such as the instructions-retired counter, instret, may be modified as side effects of instruction execution. In these cases, if a CSR access instruction reads a CSR, it reads the value prior to the execution of the instruction. If a CSR access instruction writes such a CSR, the explicit write is done instead of the update from the side effect. In particular, a value written to instret by one instruction will be the value read by the following instruction.

Fix speculative minstret increment:

Prevents over-counting by validating the wb_count_q signal. This ensures the counter is only speculatively incremented during back-to-back instruction execution, fixing a bug where stale signals caused the counter to jump ahead after a stall cycle.

The following figures explain the behavior. We are looking at the relevant signals for two back to back CSR reads of minstret after some stall cycles (empty WB stage). The counter is initially on 0x3c retired instructions. The CSRs are read in cycle 2 & 3.

Previous behavior

Both CSR reads return the same value (0x3d). Whenever instr_ret_spec is set, we read the minstret_next value (the counter's increment) because we assume that there is an instruction retiring in the WB stage ahead of us. Otherwise, we read the minstret_raw signal (the current counter value). instr_ret_spec was previously directly connected to wb_count_q and therefore, we would always read the incremented value, because this just stores whether the current/last instruction in the WB stage counted towards the instret count. The counter increment is triggered by instr_ret, which happens in cycle 3 for the first CSR read and the counter stores the increment in cycle 4. Note that if we read the counter again in cycle 5, the counter already reads 0x3f even though there is no instruction in the WB stage retiring and we only retired 0x3e instructions by then.

Fixed behavior

We now validate instr_ret_spec with wb_valid_q. This invalidates it in the first cycles, when the WB stage is empty, and reading minstret returns the actual counter value 0x3c in cycle 2. In cycle 3, the previous read is retiring, but the counter did not have time to update and will do so in the next cycle. Now instr_ret_spec is set and we read the speculative/incremented counter value 0x3d in our second read. Similarly, in cycle 4 we would read the incremented value because the second read is retiring. In cycle 5 we would again see the counters unmodified value since it had a change to update after the last instruction retired.

Verification

The current CSR tests seem to exclude the cosim and focus on making sure that the access behavior/rights is correct but not the counter value. Since we noticed this bug when adding verification for the mcounteren CSR where we access and compare the minstret with the cosim enabled, I extended that test to verify both testcases. This test was able to catch both of those errors together with the cosim. In the future, a fully randomzied test would be ideal.

I suggest we use #2403 for the verification for now instead of waiting with those fixes until we have a full blown DV environment for the minstret.

[marnovandermaas]

I think this implementation makes sense. If you would like to do the DV in the other pull request as well. Does it make sense to implement both minstret and mcounteren in the same pull request?

[gautschimi]

looks good to me!