From d3c308fc7963fdafdc479fdfa9cb3e253ca958ce Mon Sep 17 00:00:00 2001 From: Andrew Waterman Date: Mon, 7 Apr 2025 17:48:42 -0700 Subject: [PATCH 1/2] Bump counters by 0 when disabled, rather than not bumping No functional change intended; fixes implementation of a forthcoming bug fix. --- riscv/execute.cc | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/riscv/execute.cc b/riscv/execute.cc index 1fa6111f..8b3a5c83 100644 --- a/riscv/execute.cc +++ b/riscv/execute.cc @@ -365,12 +365,10 @@ void processor_t::step(size_t n) in_wfi = true; } - if (!(state.mcountinhibit->read() & MCOUNTINHIBIT_IR)) - state.minstret->bump(instret); + state.minstret->bump((state.mcountinhibit->read() & MCOUNTINHIBIT_IR) ? 0 : instret); // Model a hart whose CPI is 1. - if (!(state.mcountinhibit->read() & MCOUNTINHIBIT_CY)) - state.mcycle->bump(instret); + state.mcycle->bump((state.mcountinhibit->read() & MCOUNTINHIBIT_CY) ? 0 : instret); n -= instret; } From 51835fd75228842236186bbefad3b868f8ed5257 Mon Sep 17 00:00:00 2001 From: Andrew Waterman Date: Mon, 7 Apr 2025 17:49:49 -0700 Subject: [PATCH 2/2] Fix implementation of instreth writes Decrementing the counter in advance of incrementing it produces incorrect results when there's a carry-out. Fix by getting rid of this scheme altogher: just skip the increment if there was an expicit write. --- riscv/csrs.cc | 29 +++++++++++++++-------------- riscv/csrs.h | 2 +- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/riscv/csrs.cc b/riscv/csrs.cc index 3dbac7b7..e47cdd43 100644 --- a/riscv/csrs.cc +++ b/riscv/csrs.cc @@ -1075,6 +1075,7 @@ bool virtualized_satp_csr_t::unlogged_write(const reg_t val) noexcept { wide_counter_csr_t::wide_counter_csr_t(processor_t* const proc, const reg_t addr, smcntrpmf_csr_t_p config_csr): csr_t(proc, addr), val(0), + written(false), config_csr(config_csr) { } @@ -1083,7 +1084,15 @@ reg_t wide_counter_csr_t::read() const noexcept { } void wide_counter_csr_t::bump(const reg_t howmuch) noexcept { - if (is_counting_enabled()) { + if (written) { + // Because writing a CSR serializes the simulator, howmuch should + // reflect exactly one instruction: the explicit CSR write. + // If counting is disabled, though, howmuch will be zero. + assert(howmuch <= 1); + // The ISA mandates that explicit writes to instret take precedence + // over the instret, so simply skip the increment. + written = false; + } else if (is_counting_enabled()) { val += howmuch; // to keep log reasonable size, don't log every bump } // Clear cached value @@ -1091,23 +1100,15 @@ void wide_counter_csr_t::bump(const reg_t howmuch) noexcept { } bool wide_counter_csr_t::unlogged_write(const reg_t val) noexcept { + // Because writing a CSR serializes the simulator and is followed by a + // bump, back-to-back writes with no intervening bump should never occur. + assert(!written); + written = true; + this->val = val; - // The ISA mandates that if an instruction writes instret, the write - // takes precedence over the increment to instret. However, Spike - // unconditionally increments instret after executing an instruction. - // Correct for this artifact by decrementing instret here. - // Ensure that Smctrpmf hasn't disabled counting. - if (is_counting_enabled()) { - this->val--; - } return true; } -reg_t wide_counter_csr_t::written_value() const noexcept { - // Re-adjust for upcoming bump() - return this->val + 1; -} - // Returns true if counting is not inhibited by Smcntrpmf. // Note that minstretcfg / mcyclecfg / mhpmevent* share the same inhibit bits. bool wide_counter_csr_t::is_counting_enabled() const noexcept { diff --git a/riscv/csrs.h b/riscv/csrs.h index 278bdb37..f8b36215 100644 --- a/riscv/csrs.h +++ b/riscv/csrs.h @@ -542,10 +542,10 @@ class wide_counter_csr_t: public csr_t { void bump(const reg_t howmuch) noexcept; protected: virtual bool unlogged_write(const reg_t val) noexcept override; - virtual reg_t written_value() const noexcept override; private: bool is_counting_enabled() const noexcept; reg_t val; + bool written; smcntrpmf_csr_t_p config_csr; };