Skip to content

Add mcounteren register#2403

Open
SamuelRiedel wants to merge 10 commits into
lowRISC:masterfrom
SamuelRiedel:mcounteren
Open

Add mcounteren register#2403
SamuelRiedel wants to merge 10 commits into
lowRISC:masterfrom
SamuelRiedel:mcounteren

Conversation

@SamuelRiedel
Copy link
Copy Markdown
Contributor

@SamuelRiedel SamuelRiedel commented May 11, 2026

This PR adds the mcounteren register, which has so far been tied to zero and implements the U-mode performance counter aliases. The mcounteren register can be locked with an external MUBI signal (mcounteren_writable_i).

DV:
This PR adds two directed tests:

  • mcounteren_test goes through multiple configurations of the mcounteren register and validates that u-mode can only access the ones enabled.
  • mcounteren_lock_test verifies that the mcounteren CSR cannot be modified without the mcounteren_writable_i signal being set.

To Dos:

  • For the directed tests and the cosim to pass, we have to update riscv-isa-sim to also support those performance counters: Enable ZIHPM unpriviledged performance counters riscv-isa-sim#29
  • Currently, u-mode counter reads will lead to illegal instruction exceptions. However, we are not gating the value at the CSR module's interface for simplicity. The signal will not leave the core, but we should double-check that this is no security issue.
  • The dv/uvm/core_ibex/directed_tests/directed_testlist.yaml changes a lot everytime I regenerate it after even small modifications. This polluted the diff quite a bit. Is this expected?

@SamuelRiedel SamuelRiedel force-pushed the mcounteren branch 2 times, most recently from 58188e6 to 63fb24f Compare May 11, 2026 12:00
Copy link
Copy Markdown
Contributor

@gautschimi gautschimi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look all good to me! Just a few minor comments. (I did not look at the two WIP commits about formal)

Comment thread rtl/ibex_cs_registers.sv
CSR_HPMCOUNTER20, CSR_HPMCOUNTER21, CSR_HPMCOUNTER22, CSR_HPMCOUNTER23,
CSR_HPMCOUNTER24, CSR_HPMCOUNTER25, CSR_HPMCOUNTER26, CSR_HPMCOUNTER27,
CSR_HPMCOUNTER28, CSR_HPMCOUNTER29, CSR_HPMCOUNTER30, CSR_HPMCOUNTER31: begin
csr_rdata_int = mhpmcounter[mhpmcounter_idx][31:0];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we assign this to zero if the condition below does not match?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering the same. The values currently never make it to the register file because we trap, but we could also gate it to prevent any potential security concerns.

Comment thread rtl/ibex_top.sv
.out_o(fetch_enable_buf)
);

prim_buf #(.Width($bits(ibex_mubi_t))) u_mcounteren_writable_buf (
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this buffer used for?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was following the fetch_enable example here, which says to buffer security critical signals to prevent synthesis optimisation removing them.

@@ -0,0 +1,224 @@
# Copyright lowRISC contributors.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is good! It verifies that the counters are accessible or not depending on mcounteren.

Maybe we can add a short test that also verifies that the read values are correct.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add this, that should not be difficult.

@@ -60,6 +60,14 @@
test_srcs: empty/empty.S
Copy link
Copy Markdown
Contributor

@gautschimi gautschimi May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did something change in this file (except for the new mcounteren_test)? The diff is big. Did the order of the tests change or was something real added?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is what I was also confused about. The order seems to change every time I add a test or even modify the description of the test. This makes it quite hard to check the diff. I was wondering whether someone with more experience with this tool had any insights on this.

@SamuelRiedel
Copy link
Copy Markdown
Contributor Author

I disabled the check of the mcounteren CSR in the formal verification for now. Another reason for the failure of the formal verification was that we ran out of memory on the runner that has twice as many cores but half as much memory available. I'll address this in a separate PR and rebase once merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants