Re: [PATCH v7 07/11] target/riscv: Save counter values during countinhibit update

[Daniel Henrique Barboza]

On 6/26/24 8:57 PM, Atish Patra 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: Daniel Henrique Barboza <dbarboza@ventanamicro.com>


>   target/riscv/cpu.h     |  1 -
>   target/riscv/csr.c     | 34 ++++++++++++++++++++++------------
>   target/riscv/machine.c |  5 ++---
>   3 files changed, 24 insertions(+), 16 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index d56d640b06be..91fe2a46ba35 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 c292d036bcb2..e4adfa324efe 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1062,17 +1062,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;
>       }
>   
>       /*
> @@ -2114,9 +2108,25 @@ 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 =
> +                    riscv_pmu_ctr_get_fixed_counters_val(env, cidx, false) -
> +                                           counter->mhpmcounter_prev +
> +                                           counter->mhpmcounter_val;
> +                if (riscv_cpu_mxl(env) == MXL_RV32) {
> +                    counter->mhpmcounterh_val =
> +                        riscv_pmu_ctr_get_fixed_counters_val(env, cidx, true) -
> +                                                counter->mhpmcounterh_prev +
> +                                                counter->mhpmcounterh_val;
> +                }
> +            }
>           }
>       }
>   
> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> index 76f2150f78b5..492c2c6d9d79 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -320,15 +320,14 @@ static bool pmu_needed(void *opaque)
>   
>   static const VMStateDescription vmstate_pmu_ctr_state = {
>       .name = "cpu/pmu",
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> +    .version_id = 2,
> +    .minimum_version_id = 2,
>       .needed = pmu_needed,
>       .fields = (const VMStateField[]) {
>           VMSTATE_UINTTL(mhpmcounter_val, PMUCTRState),
>           VMSTATE_UINTTL(mhpmcounterh_val, PMUCTRState),
>           VMSTATE_UINTTL(mhpmcounter_prev, PMUCTRState),
>           VMSTATE_UINTTL(mhpmcounterh_prev, PMUCTRState),
> -        VMSTATE_BOOL(started, PMUCTRState),
>           VMSTATE_END_OF_LIST()
>       }
>   };