On Tue, Apr 30, 2024 at 5:29 AM Atish Patra <ati...@rivosinc.com> wrote:
>
> Currently, if a counter monitoring cycle/instret is stopped via
> mcountinhibit we just update the state while the value is saved
> during the next read. This is not accurate as the read may happen
> many cycles after the counter is stopped. Ideally, the read should
> return the value saved when the counter is stopped.
>
> Thus, save the value of the counter during the inhibit update
> operation and return that value during the read if corresponding bit
> in mcountihibit is set.
>
> Signed-off-by: Atish Patra <ati...@rivosinc.com>
Reviewed-by: Alistair Francis <alistair.fran...@wdc.com>
Alistair
> ---
> target/riscv/cpu.h | 1 -
> target/riscv/csr.c | 32 ++++++++++++++++++++------------
> target/riscv/machine.c | 1 -
> 3 files changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 3b1a02b9449a..09bbf7ce9880 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -153,7 +153,6 @@ typedef struct PMUCTRState {
> target_ulong mhpmcounter_prev;
> /* Snapshort value of a counter in RV32 */
> target_ulong mhpmcounterh_prev;
> - bool started;
> /* Value beyond UINT32_MAX/UINT64_MAX before overflow interrupt trigger
> */
> target_ulong irq_overflow_left;
> } PMUCTRState;
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 726096444fae..68ca31aff47d 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -929,17 +929,11 @@ static RISCVException riscv_pmu_read_ctr(CPURISCVState
> *env, target_ulong *val,
>
> if (get_field(env->mcountinhibit, BIT(ctr_idx))) {
> /*
> - * Counter should not increment if inhibit bit is set. We can't
> really
> - * stop the icount counting. Just return the counter value written by
> - * the supervisor to indicate that counter was not incremented.
> + * Counter should not increment if inhibit bit is set. Just return
> the
> + * current counter value.
> */
> - if (!counter->started) {
> - *val = ctr_val;
> - return RISCV_EXCP_NONE;
> - } else {
> - /* Mark that the counter has been stopped */
> - counter->started = false;
> - }
> + *val = ctr_val;
> + return RISCV_EXCP_NONE;
> }
>
> /*
> @@ -1973,9 +1967,23 @@ static RISCVException
> write_mcountinhibit(CPURISCVState *env, int csrno,
>
> /* Check if any other counter is also monitoring cycles/instructions */
> for (cidx = 0; cidx < RV_MAX_MHPMCOUNTERS; cidx++) {
> - if (!get_field(env->mcountinhibit, BIT(cidx))) {
> counter = &env->pmu_ctrs[cidx];
> - counter->started = true;
> + if (get_field(env->mcountinhibit, BIT(cidx)) && (val & BIT(cidx))) {
> + /*
> + * Update the counter value for cycle/instret as we can't stop
> the
> + * host ticks. But we should show the current value at this
> moment.
> + */
> + if (riscv_pmu_ctr_monitor_cycles(env, cidx) ||
> + riscv_pmu_ctr_monitor_instructions(env, cidx)) {
> + counter->mhpmcounter_val = get_ticks(false) -
> + counter->mhpmcounter_prev +
> + counter->mhpmcounter_val;
> + if (riscv_cpu_mxl(env) == MXL_RV32) {
> + counter->mhpmcounterh_val = get_ticks(false) -
> + counter->mhpmcounterh_prev +
> + counter->mhpmcounterh_val;
> + }
> + }
> }
> }
>
> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> index 76f2150f78b5..3e0f2dd2ce2a 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -328,7 +328,6 @@ static const VMStateDescription vmstate_pmu_ctr_state = {
> VMSTATE_UINTTL(mhpmcounterh_val, PMUCTRState),
> VMSTATE_UINTTL(mhpmcounter_prev, PMUCTRState),
> VMSTATE_UINTTL(mhpmcounterh_prev, PMUCTRState),
> - VMSTATE_BOOL(started, PMUCTRState),
> VMSTATE_END_OF_LIST()
> }
> };
>
> --
> 2.34.1
>
>
