Skip to content
Open
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ci/vars.env
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
# Quote values to ensure they are parsed as string (version numbers might
# end up as float otherwise).
VERILATOR_VERSION=v4.210
IBEX_COSIM_VERSION=6d5b660
IBEX_COSIM_VERSION=aadf648

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 bump adds ZIHPM extension to Spike but it also pulls in the Zc* extensions. Is there any risk in that inclusion?

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.

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.

RISCV_TOOLCHAIN_TAR_VERSION=20220210-1
RISCV_TOOLCHAIN_TAR_VARIANT=lowrisc-toolchain-gcc-rv32imcb
RISCV_COMPLIANCE_GIT_VERSION=844c6660ef3f0d9b96957991109dfd80cc4938e2
Expand Down
11 changes: 11 additions & 0 deletions dv/cosim/cosim.h
Original file line number Diff line number Diff line change
Expand Up @@ -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

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.

How come this is true? Spike should have all the information it needs to calculate minstret right?

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.

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;
Expand Down
7 changes: 7 additions & 0 deletions dv/cosim/cosim_dpi.cc
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,13 @@ void riscv_cosim_set_mcycle(Cosim *cosim, svBitVecVal *mcycle) {
cosim->set_mcycle(mcycle_full);
}

void riscv_cosim_set_minstret(Cosim *cosim, svBitVecVal *minstret) {
assert(cosim);

uint64_t minstret_full = minstret[0] | (uint64_t)minstret[1] << 32;
cosim->set_minstret(minstret_full);
}

void riscv_cosim_set_csr(Cosim *cosim, const int csr_id,
const svBitVecVal *csr_val) {
assert(cosim);
Expand Down
1 change: 1 addition & 0 deletions dv/cosim/cosim_dpi.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ void riscv_cosim_set_nmi(Cosim *cosim, svBit nmi);
void riscv_cosim_set_nmi_int(Cosim *cosim, svBit nmi_int);
void riscv_cosim_set_debug_req(Cosim *cosim, svBit debug_req);
void riscv_cosim_set_mcycle(Cosim *cosim, svBitVecVal *mcycle);
void riscv_cosim_set_minstret(Cosim *cosim, svBitVecVal *minstret);
void riscv_cosim_set_csr(Cosim *cosim, const int csr_id,
const svBitVecVal *csr_val);
void riscv_cosim_set_ic_scr_key_valid(Cosim *cosim, svBit valid);
Expand Down
1 change: 1 addition & 0 deletions dv/cosim/cosim_dpi.svh
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import "DPI-C" function void riscv_cosim_set_nmi(chandle cosim_handle, bit nmi);
import "DPI-C" function void riscv_cosim_set_nmi_int(chandle cosim_handle, bit nmi_int);
import "DPI-C" function void riscv_cosim_set_debug_req(chandle cosim_handle, bit debug_req);
import "DPI-C" function void riscv_cosim_set_mcycle(chandle cosim_handle, bit [63:0] mcycle);
import "DPI-C" function void riscv_cosim_set_minstret(chandle cosim_handle, bit [63:0] minstret);
import "DPI-C" function void riscv_cosim_set_csr(chandle cosim_handle, int csr_id,
bit [31:0] csr_val);
import "DPI-C" function void riscv_cosim_set_ic_scr_key_valid(chandle cosim_handle, bit valid);
Expand Down
39 changes: 38 additions & 1 deletion dv/cosim/spike_cosim.cc
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,10 @@ SpikeCosim::SpikeCosim(const std::string &isa_string, uint32_t start_pc,
uint32_t pmp_num_regions, uint32_t pmp_granularity,
uint32_t mhpm_counter_num, uint32_t dm_start_addr,
uint32_t dm_end_addr)
: nmi_mode(false), pending_iside_error(false), insn_cnt(0) {
: nmi_mode(false),
pending_iside_error(false),
insn_cnt(0),
mhpm_counter_num(mhpm_counter_num) {
FILE *log_file = nullptr;
if (trace_log_path.length() != 0) {
log = std::make_unique<log_file_t>(trace_log_path.c_str());
Expand Down Expand Up @@ -747,6 +750,27 @@ void SpikeCosim::set_mcycle(uint64_t mcycle) {
// to write all 64 bits at once at least.
}

void SpikeCosim::set_minstret(uint64_t minstret) {
uint32_t upper_minstret = minstret >> 32;
uint32_t lower_minstret = minstret & 0xffffffff;

// Spike decrements the MINSTRET CSR when you write to it. This is the same
// issue as with MCYCLE. See `set_mcycle` for more details.

// Write the lower half first, incremented twice due to the double decrement
processor->get_state()->csrmap[CSR_MINSTRET]->write(lower_minstret + 2);

if ((processor->get_state()->csrmap[CSR_MINSTRET]->read() & 0xffffffff) ==
0) {
// If the lower half is 0 at this point then the upper half will get
// decremented, so increment it first.
upper_minstret++;
}

// Set the upper half
processor->get_state()->csrmap[CSR_MINSTRETH]->write(upper_minstret);
}

void SpikeCosim::set_csr(const int csr_num, const uint32_t new_val) {
// Note that this is tested with ibex-cosim-v0.3 version of Spike. 'set_csr'
// method might have a hardwired zero for mhpmcounterX registers.
Expand Down Expand Up @@ -832,6 +856,19 @@ void SpikeCosim::fixup_csr(int csr_num, uint32_t csr_val) {
processor->set_csr(csr_num, new_val);
#else
processor->put_csr(csr_num, new_val);
#endif
break;
}
case CSR_MCOUNTEREN: {
// Bits 3..3+mhpm_counter_num-1 correspond to implemented HPM counters
reg_t hpm_mask = ((1 << mhpm_counter_num) - 1) << 3;
// Bit 0 and 2 are for mcycle and minstret which are always implemented
// Bit 1 is for time which is not implemented, hence the mask 0x5
reg_t new_val = csr_val & (0x5 | hpm_mask);
#ifdef OLD_SPIKE
processor->set_csr(csr_num, new_val);
#else
processor->put_csr(csr_num, new_val);
#endif
break;
}
Expand Down
2 changes: 2 additions & 0 deletions dv/cosim/spike_cosim.h
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ class SpikeCosim : public simif_t, public Cosim {
void misaligned_pmp_fixup();

unsigned int insn_cnt;
uint32_t mhpm_counter_num;

public:
SpikeCosim(const std::string &isa_string, uint32_t start_pc,
Expand Down Expand Up @@ -131,6 +132,7 @@ class SpikeCosim : public simif_t, public Cosim {
void set_nmi_int(bool nmi_int) override;
void set_debug_req(bool debug_req) override;
void set_mcycle(uint64_t mcycle) override;
void set_minstret(uint64_t minstret) override;
void set_csr(const int csr_num, const uint32_t new_val) override;
void set_ic_scr_key_valid(bool valid) override;
void notify_dside_access(const DSideAccessInfo &access_info) override;
Expand Down
1 change: 1 addition & 0 deletions dv/formal/check/top.sv
Original file line number Diff line number Diff line change
Expand Up @@ -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,

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'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?

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.

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.

output logic core_sleep_o,
output logic alert_minor_o,
output logic alert_major_internal_o,
Expand Down
1 change: 1 addition & 0 deletions dv/riscv_compliance/rtl/ibex_riscv_compliance.sv
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,7 @@ module ibex_riscv_compliance (
.double_fault_seen_o ( ),

.fetch_enable_i (ibex_pkg::IbexMuBiOn ),
.mcounteren_writable_i (ibex_pkg::IbexMuBiOn ),
.alert_minor_o ( ),
.alert_major_internal_o ( ),
.alert_major_bus_o ( ),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ class ibex_cosim_scoreboard extends uvm_scoreboard;
riscv_cosim_set_nmi_int(cosim_handle, rvfi_instr.nmi_int);
riscv_cosim_set_mip(cosim_handle, rvfi_instr.pre_mip, rvfi_instr.post_mip);
riscv_cosim_set_mcycle(cosim_handle, rvfi_instr.mcycle);
riscv_cosim_set_minstret(cosim_handle, rvfi_instr.minstret);

// Set performance counters through a pseudo-backdoor write
for (int i=0; i < 10; i++) begin
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ class ibex_rvfi_monitor extends uvm_monitor;
trans_collected.debug_req = vif.monitor_cb.ext_debug_req;
trans_collected.rf_wr_suppress = vif.monitor_cb.ext_rf_wr_suppress;
trans_collected.mcycle = vif.monitor_cb.ext_mcycle;
trans_collected.minstret = vif.monitor_cb.ext_minstret;
trans_collected.ic_scr_key_valid = vif.monitor_cb.ext_ic_scr_key_valid;

for (int i=0; i < 10; i++) begin
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ class ibex_rvfi_seq_item extends uvm_sequence_item;
bit debug_req;
bit rf_wr_suppress;
bit [63:0] mcycle;
bit [63:0] minstret;

bit [31:0] mhpmcounters [10];
bit [31:0] mhpmcountersh [10];
Expand All @@ -34,6 +35,7 @@ class ibex_rvfi_seq_item extends uvm_sequence_item;
`uvm_field_int (debug_req, UVM_DEFAULT)
`uvm_field_int (rf_wr_suppress, UVM_DEFAULT)
`uvm_field_int (mcycle, UVM_DEFAULT)
`uvm_field_int (minstret, UVM_DEFAULT)
`uvm_field_sarray_int (mhpmcounters, UVM_DEFAULT)
`uvm_field_sarray_int (mhpmcountersh, UVM_DEFAULT)
`uvm_field_int (ic_scr_key_valid, UVM_DEFAULT)
Expand Down
8 changes: 8 additions & 0 deletions dv/uvm/core_ibex/directed_tests/directed_testlist.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,14 @@
test_srcs: empty/empty.S
Comment thread
gautschimi marked this conversation as resolved.
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: pmp_mseccfg_test_rlb1_l0_0_u0
desc: >
mseccfg test
Expand Down
12 changes: 10 additions & 2 deletions dv/uvm/core_ibex/directed_tests/gen_testlist.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,14 @@ 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: pmp_mseccfg_test_rlb1_l0_0_u0
desc: >
mseccfg test
Expand Down Expand Up @@ -469,11 +477,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']

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.

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':
Expand Down
Loading