-
Notifications
You must be signed in to change notification settings - Fork 743
Add mcounteren register
#2403
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Add mcounteren register
#2403
Changes from all commits
6355f79
72eaebf
216c77c
bacacea
497d1ec
ed45547
791beb6
6c1539e
c27c013
e0d9935
37ddcd4
49bffac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -128,6 +128,17 @@ class Cosim { | |
| // A full 64-bit value is provided setting both the mcycle and mcycleh CSRs. | ||
| virtual void set_mcycle(uint64_t mcycle) = 0; | ||
|
|
||
| // Set the value of minstret. | ||
| // | ||
| // The co-simulation model doesn't alter the value of minstret itself (other | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How come this is true? Spike should have all the information it needs to calculate minstret right?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right. And it does, the Spike counter is just lagging one instruction behind Ibex. It should match though, so I'll investigate a bit more... |
||
| // than instructions that do a direct CSR write). minstret should be set to | ||
| // the correct value before any `step` call that may execute an instruction | ||
| // that observes the value of minstret. | ||
| // | ||
| // A full 64-bit value is provided setting both the minstret and minstreth | ||
| // CSRs. | ||
| virtual void set_minstret(uint64_t minstret) = 0; | ||
|
|
||
| // Set the value of a CSR. This is used when it is needed to have direct | ||
| // communication between DUT and Spike (e.g. Performance counters). | ||
| virtual void set_csr(const int csr_num, const uint32_t new_val) = 0; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -103,6 +103,7 @@ module top import ibex_pkg::*; #( | |
|
|
||
| // CPU Control Signals | ||
| input ibex_mubi_t fetch_enable_i, | ||
| input ibex_mubi_t mcounteren_writable_i, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. This allows locking the |
||
| output logic core_sleep_o, | ||
| output logic alert_minor_o, | ||
| output logic alert_major_internal_o, | ||
|
|
@@ -162,7 +163,9 @@ NotDebug: assume property (!ibex_top_i.u_ibex_core.debug_mode & !debug_req_i); | |
| ConstantBoot: assume property (boot_addr_i == $past(boot_addr_i)); | ||
| // 3. Always fetch enable | ||
| FetchEnable: assume property (fetch_enable_i == IbexMuBiOn); | ||
| // 4. Never try to sleep if we couldn't ever wake up | ||
| // 4. Always have mcounteren writable | ||
| McounterenWritable: assume property (mcounteren_writable_i == IbexMuBiOn); | ||
| // 5. Never try to sleep if we couldn't ever wake up | ||
| WFIStart: assume property (`IDC.ctrl_fsm_cs == SLEEP |-> ( | ||
| `CSR.mie_q.irq_software | | ||
| `CSR.mie_q.irq_timer | | ||
|
|
@@ -441,18 +444,25 @@ logic ex_is_checkable_csr; | |
| assign ex_is_checkable_csr = ~( | ||
| ((CSR_MHPMCOUNTER3H <= `CSR_ADDR) && (`CSR_ADDR <= CSR_MHPMCOUNTER31H)) | | ||
| ((CSR_MHPMCOUNTER3 <= `CSR_ADDR) && (`CSR_ADDR <= CSR_MHPMCOUNTER31)) | | ||
| ((CSR_HPMCOUNTER3H <= `CSR_ADDR) && (`CSR_ADDR <= CSR_HPMCOUNTER31H)) | | ||
| ((CSR_HPMCOUNTER3 <= `CSR_ADDR) && (`CSR_ADDR <= CSR_HPMCOUNTER31)) | | ||
| ((CSR_MHPMEVENT3 <= `CSR_ADDR) && (`CSR_ADDR <= CSR_MHPMEVENT31)) | | ||
| (`CSR_ADDR == CSR_CPUCTRLSTS) | (`CSR_ADDR == CSR_SECURESEED) | | ||
| (`CSR_ADDR == CSR_MIE) | | ||
| (`CSR_ADDR == CSR_MCYCLE) | (`CSR_ADDR == CSR_MCYCLEH) | | ||
| (`CSR_ADDR == CSR_CYCLE) | (`CSR_ADDR == CSR_CYCLEH) | | ||
|
|
||
| // TODO: | ||
| (`CSR_ADDR == CSR_MINSTRET) | (`CSR_ADDR == CSR_MINSTRETH) | | ||
| (`CSR_ADDR == CSR_INSTRET) | (`CSR_ADDR == CSR_INSTRETH) | | ||
| (`CSR_ADDR == CSR_MCOUNTINHIBIT) | ||
| ); | ||
|
|
||
| `undef INSTR | ||
|
|
||
| // Force mcounteren to always be zero to match the current Sail model. | ||
| McounterenStubbedZero: assume property (`CSR.mcounteren_q == 32'h0); | ||
|
|
||
| ////////////////////// Decompression Invariant Defs ////////////////////// | ||
| // These will be used to show that the decompressed instruction stored is in fact the decompressed version of the compressed instruction. | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -80,6 +80,23 @@ def add_configs_and_handwritten_directed_tests(): | |
| test_srcs: empty/empty.S | ||
| config: riscv-tests | ||
|
|
||
| - test: mcounteren_test | ||
| desc: > | ||
| Tests the mcounteren CSR: reset value, hardwired-zero bit 1 (time), | ||
| and U-mode counter access gating. | ||
| iterations: 1 | ||
| test_srcs: mcounteren_test/mcounteren_test.S | ||
| config: riscv-tests | ||
|
|
||
| - test: mcounteren_lock_test | ||
| desc: > | ||
| Tests that mcounteren retains its value after mcounteren_writable_i is | ||
| de-asserted mid-simulation (write-lock). | ||
| iterations: 1 | ||
| rtl_test: core_ibex_mcounteren_lock_test | ||
| test_srcs: mcounteren_test/mcounteren_lock_test.S | ||
| config: riscv-tests | ||
|
|
||
| - test: pmp_mseccfg_test_rlb1_l0_0_u0 | ||
| desc: > | ||
| mseccfg test | ||
|
|
@@ -469,11 +486,11 @@ def _main() -> int: | |
| add_configs_and_handwritten_directed_tests() | ||
|
|
||
| if 'riscv-tests' in test_suite_list or test_suite == 'all': | ||
| isa_tests = {'rv32mi', 'rv32uc', 'rv32ui', 'rv32um'} | ||
| isa_tests = ['rv32mi', 'rv32uc', 'rv32um', 'rv32ui'] | ||
| append_directed_testlist(isa_tests, '../../../../vendor/riscv-tests/isa/', 'riscv-tests', 1) | ||
|
|
||
| if 'riscv-arch-tests' in test_suite_list or test_suite == 'all': | ||
| arch_tests = {'rv32i_m/B/src', 'rv32i_m/C/src', 'rv32i_m/I/src', 'rv32i_m/M/src', 'rv32i_m/Zifencei/src'} | ||
| arch_tests = ['rv32i_m/M/src', 'rv32i_m/C/src', 'rv32i_m/Zifencei/src', 'rv32i_m/I/src', 'rv32i_m/B/src'] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit extra space here. |
||
| append_directed_testlist(arch_tests, '../../../../vendor/riscv-arch-tests/riscv-test-suite/', 'riscv-arch-tests', 1) | ||
|
|
||
| if 'epmp-tests' in test_suite_list or test_suite == 'all': | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| # Copyright lowRISC contributors. | ||
| # Licensed under the Apache License, Version 2.0, see LICENSE for details. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| # This test verifies the mcounteren write-lock mechanism by dynamically setting | ||
| # the mcounteren_writable_i hardware input. It validates that register updates | ||
| # are silently ignored while locked and succeed when unlocked, using immediate | ||
| # software readbacks to confirm the state. Inter-process signaling with the UVM | ||
| # testbench is achieved by monitoring writes to mcycle (lock command) and | ||
| # mcycleh (unlock command). | ||
|
|
||
| #include "riscv_test.h" | ||
| #include "test_macros.h" | ||
|
|
||
| RVTEST_RV32M | ||
| RVTEST_CODE_BEGIN | ||
|
|
||
| # Initial Write (Unlocked) | ||
| li s0, 0x5 | ||
| csrw mcounteren, s0 | ||
| csrr t1, mcounteren | ||
| li t2, 0x5 | ||
| # If readback != 0x5, fail immediately | ||
| bne t1, t2, fail | ||
|
|
||
| # Tell UVM to lock by writing to mcycle, which is monitored by the UVM | ||
| # testbench to set `mcounteren_writable` | ||
| csrw mcycle, x0 | ||
|
|
||
| # Small delay loop to let the hardware pin force propagate | ||
| .rept 5 | ||
| nop | ||
| .endr | ||
|
|
||
| # Try to overwrite mcounteren with 0x0 while locked | ||
| li s1, 0x0 | ||
| csrw mcounteren, s1 | ||
| csrr t1, mcounteren | ||
| bne t1, s0, fail | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be s1?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, because in this instance, the register should be locked. So the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also shouldn't this one fail if it is equal?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wait, they are both the same lines you commented. So here we either fail if:
I decided to do the first to also catch behaviour where the overwrite had some sort of effect, like changing just a few bits to zero. |
||
|
|
||
| # Tell UVM to unlock by writing to mcycleh | ||
| csrw mcycleh, x0 | ||
|
|
||
| .rept 5 | ||
| nop | ||
| .endr | ||
|
|
||
| # Try to overwrite it with 0x0 again. This time while unlocked | ||
| li s3, 0x0 | ||
| csrw mcounteren, s3 | ||
| csrr t1, mcounteren | ||
| bne t1, s3, fail | ||
|
|
||
| # Success Exit | ||
| j pass | ||
| TEST_PASSFAIL | ||
|
|
||
| RVTEST_CODE_END | ||
|
|
||
| RVTEST_DATA_BEGIN | ||
| TEST_DATA | ||
| RVTEST_DATA_END | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bump adds ZIHPM extension to Spike but it also pulls in the Zc* extensions. Is there any risk in that inclusion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. But that will be fine, because we anyway already have the Zc* extension in the RTL. Only the DV is not fully merged yet, but already having Spike support those extensions will not hurt.