Skip to content

Commit 4dbc7ec

Browse files
committed
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.
1 parent d3c308f commit 4dbc7ec

File tree

2 files changed

+15
-15
lines changed

2 files changed

+15
-15
lines changed

riscv/csrs.cc

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1075,6 +1075,7 @@ bool virtualized_satp_csr_t::unlogged_write(const reg_t val) noexcept {
10751075
wide_counter_csr_t::wide_counter_csr_t(processor_t* const proc, const reg_t addr, smcntrpmf_csr_t_p config_csr):
10761076
csr_t(proc, addr),
10771077
val(0),
1078+
written(false),
10781079
config_csr(config_csr) {
10791080
}
10801081

@@ -1083,31 +1084,30 @@ reg_t wide_counter_csr_t::read() const noexcept {
10831084
}
10841085

10851086
void wide_counter_csr_t::bump(const reg_t howmuch) noexcept {
1086-
if (is_counting_enabled()) {
1087+
if (written) {
1088+
// Because writing a CSR serializes the simulator, howmuch should
1089+
// reflect exactly one instruction: the explicit CSR write.
1090+
// If counting is disabled, though, howmuch will be zero.
1091+
assert(howmuch <= 1);
1092+
// The ISA mandates that explicit writes to instret take precedence
1093+
// over the instret, so simply skip the increment.
1094+
written = false;
1095+
} else if (is_counting_enabled()) {
10871096
val += howmuch; // to keep log reasonable size, don't log every bump
10881097
}
10891098
// Clear cached value
10901099
config_csr->reset_prev();
10911100
}
10921101

10931102
bool wide_counter_csr_t::unlogged_write(const reg_t val) noexcept {
1103+
// The simulator should be serialized and
1104+
assert(!written);
1105+
written = true;
1106+
10941107
this->val = val;
1095-
// The ISA mandates that if an instruction writes instret, the write
1096-
// takes precedence over the increment to instret. However, Spike
1097-
// unconditionally increments instret after executing an instruction.
1098-
// Correct for this artifact by decrementing instret here.
1099-
// Ensure that Smctrpmf hasn't disabled counting.
1100-
if (is_counting_enabled()) {
1101-
this->val--;
1102-
}
11031108
return true;
11041109
}
11051110

1106-
reg_t wide_counter_csr_t::written_value() const noexcept {
1107-
// Re-adjust for upcoming bump()
1108-
return this->val + 1;
1109-
}
1110-
11111111
// Returns true if counting is not inhibited by Smcntrpmf.
11121112
// Note that minstretcfg / mcyclecfg / mhpmevent* share the same inhibit bits.
11131113
bool wide_counter_csr_t::is_counting_enabled() const noexcept {

riscv/csrs.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -542,10 +542,10 @@ class wide_counter_csr_t: public csr_t {
542542
void bump(const reg_t howmuch) noexcept;
543543
protected:
544544
virtual bool unlogged_write(const reg_t val) noexcept override;
545-
virtual reg_t written_value() const noexcept override;
546545
private:
547546
bool is_counting_enabled() const noexcept;
548547
reg_t val;
548+
bool written;
549549
smcntrpmf_csr_t_p config_csr;
550550
};
551551

0 commit comments

Comments
 (0)