Add mcounteren register#2403
Conversation
SamuelRiedel
force-pushed
the
mcounteren
branch
4 times, most recently
from
fcb3067 to
bb40866
Compare
SamuelRiedel
requested review from
gautschimi,
marnovandermaas,
nasahlpa and
rswarbrick
gautschimi
left a comment
gautschimi
left a comment
There was a problem hiding this comment.
The changes look all good to me! Just a few minor comments. (I did not look at the two WIP commits about formal)
|
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. |
gautschimi
left a comment
gautschimi
left a comment
There was a problem hiding this comment.
Looks good to me!
I mostly looked at RTL, but the changes in python, C, yaml, etc. look also good to me!
SamuelRiedel
force-pushed
the
mcounteren
branch
2 times, most recently
from
c799da7 to
49bffac
Compare
|
|
||
| // CPU Control Signals | ||
| input ibex_mubi_t fetch_enable_i, | ||
| input ibex_mubi_t mcounteren_writable_i, |
There was a problem hiding this comment.
What's the point of making this an input as opposed to a parameter? Is there a use-case to be able to switch this dynamically during runtime?
There was a problem hiding this comment.
Yes. This allows locking the mcounteren in a certain configuration. For example, the boot code could enable only the instret counter to be accessible to the user mode and then lock it to prevent the software from switching it later and use other counters to profile the core. This option, gives us the most flexibility and security.
marnovandermaas
left a comment
marnovandermaas
left a comment
There was a problem hiding this comment.
I realised there are some more commits so here are some more comments. Very good work on this and please disregard my previous mention that the docs needed updating.
The `wb_count_q` signal can become stale. It tracks whether the current or previous retired instruction increments the `instret` count. If there is a bubble before an `minstret` read, the counter will be updated, but `wb_count_q` and therefore `perf_instr_ret_wb_spec_o` will incorrectly suggest speculatively incrementing the counter further. This led to the `instret` counter being one instruction ahead every time we read it after a stall cycle. We fix this by validating `wb_count_q` and only incrementing speculatively if we actually have to. That is, when we are executing instructions back-to-back and the counter hasn't updated yet.
According to the Specification Section 6.1 CSR Instructions: 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.
The set created a random order of the tests. So small modifications or additions to the testlist polluted the diff. Fixing the order in a list makes the output deterministic. I swapped the order of the tests to match the current output order of the testlist to avoid a diff.
|
@marnovandermaas thank you for your review. Now that the CI works again, this PR would be ready to merge from my side. I included a check for the |
marnovandermaas
left a comment
marnovandermaas
left a comment
There was a problem hiding this comment.
These changes look good to me now. Thanks for including the instret fixes as well. And for checking everything in CI.
3 participants
This PR adds the
mcounterenregister, which has so far been tied to zero and implements the U-mode performance counter aliases. Themcounterenregister can be locked with an external MUBI signal (mcounteren_writable_i).DV:
This PR adds two directed tests:
mcounteren_testgoes through multiple configurations of the mcounteren register and validates that u-mode can only access the ones enabled.mcounteren_lock_testverifies that the mcounteren CSR cannot be modified without themcounteren_writable_isignal being set.To Dos:
dv/uvm/core_ibex/directed_tests/directed_testlist.yamlchanges a lot everytime I regenerate it after even small modifications. This polluted the diff quite a bit. Is this expected?resolves #1973
fixes #2312