search

lowRISC / ibex

Ibex is a small 32 bit RISC-V CPU core, previously known as zero-riscy.
https://www.lowrisc.org
Apache License 2.0
1.37k stars 543 forks source link

Illegal Instruction Retires #1132

Open shetalani opened 4 years ago

shetalani commented 4 years ago

RISC-V Specification

3.1.11: "The minstret CSR counts the number of instructions the hart has retired. The mcycle and minstret registers have 64-bit precision on all RV32 and RV64 systems.

The counter registers have an arbitrary value after system reset, and can be written with a given value. Any CSR write takes effect after the writing instruction has otherwise completed."

The counter-inhibit register mcountinhibit is a 32-bit WARL register that controls which of the hardware performance-monitoring counters increment. The settings in this register only control whether the counters increment; their accessibility is not affected by the setting of this register.

When the CY, IR, or HPMn bit in the mcountinhibit register is clear, the cycle, instret, or hpmcountern register increments as usual. When the CY, IR, or HPMn bit is set, the corresponding counter does not increment.

Issue Description

An illegal instruction increments the minstret counter as if it was a legal one.

Steps to Reproduce

As shown below, the minstret counter is incremented for the illegal instruction csrrw x8, 0x010, x3, knowing that IR bit of mcountinhibit is set to '0 and thus allowing for the minstret to be incremented.

minstret_ill

Product: OneSpin 360 DV-Verify App: Processor Verification App Tool's version: 2020.2.0

shetalani commented 4 years ago

Same happens for ecall and ebreak instructions, as well as load/ store access faults. For your reference, please check https://github.com/openhwgroup/cv32e40p/issues/533

imphil commented 3 years ago

@tomroberts-lowrisc is this issue fixed by https://github.com/lowRISC/ibex/pull/1141, or are there outstanding issues?

tomeroberts commented 3 years ago

AFAIK @shetalani are you able to rerun and confirm this is now fixed?

shetalani commented 3 years ago

Hi @tomroberts-lowrisc, not really. It's not for the illegal cases of executing dret outside debug mode, or mret in U mode.

tomeroberts commented 3 years ago

Ok - thanks for confirming @shetalani I'll have a look at adding the missing things you mentioned

GregAC commented 3 years ago

From a quick look I think the issue is using the illegal_insn_dec signal as an indication of an illegal instruction, instead we want the illegal_insn_d signal from inside the controller (this may allows us to drop some other terms from the current instr_perf_count_id_o expression.

Indeed I think the illegal_insn_o signal in ibex_id_stage.sv is a little broken. It's only for RVFI (so no functional issues from it being broken) but should be fixed. Maybe illegal_insn_d gets output as illegal_insn_o from the controller and fed out to top-level? May need some renaming as we'll have an illegal_insn_i and illegal_insn_o with subtly different meanings on the controller.