Re: [PATCH 1/3] target/riscv: Save counter values during countinhibit update

[Alistair Francis <alistair23@gmail.com>]

On Tue, Apr 30, 2024 at 5:29 AM Atish Patra <atishp@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 <atishp@rivosinc.com>

Reviewed-by: Alistair Francis <alistair.francis@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
>
>