Re: [PATCH v2 09/14] target/riscv: Rebuild fixed-event PMU overflow deadlines

[Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com>]

On 9/10/2026 11:39 AM, TANG Tiancheng wrote:
> timer_mod_anticipate_ns() cannot postpone the shared overflow timer when
> a counter is stopped or reconfigured. Recompute the earliest deadline
> from all eligible counters after PMU changes and timer expiry. Check for
> an actual counter wrap before setting OF or LCOFIP.
> 
> Keep counter arithmetic in source units. For timer scheduling, convert
> only raw icount instruction counts to nanoseconds. Check how many fit
> within INT64_MAX - now nanoseconds before conversion, using INT64_MAX as
> the deadline if the count is larger. Recompute the remaining count at
> expiry instead of keeping irq_overflow_left.
> 
> Icount time warp can expire the timer without executing instructions.
> If the instruction source has not advanced, defer its next deadline
> until execution resumes to avoid an endless warp/rearm loop.
> 
> Extend the cycle-control test to cover selector writes while CY is set
> and resuming mcycle after CY is cleared.
> 
> Signed-off-by: TANG Tiancheng <lyndra@linux.alibaba.com>
> ---

Reviewed-by: Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com>

>   target/riscv/cpu.h                     |   4 +-
>   target/riscv/tcg/cpu_helper.c          |   2 +
>   target/riscv/tcg/pmu.c                 | 239 +++++++++++++--------------------
>   target/riscv/tcg/pmu.h                 |   3 +-
>   target/riscv/tcg/tcg-cpu.c             |  10 ++
>   tests/tcg/riscv64/pmu-cycle-controls.S |  27 ++++
>   6 files changed, 139 insertions(+), 146 deletions(-)
> 
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index f7b1bfc9cf5069125bc22dc2674d8e67431c5970..17b9929785f668487d2ec851b736d588e025fd91 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -236,8 +236,6 @@ typedef struct PMUCTRState {
>       uint64_t mhpmcounter_val;
>       /* Snapshot value of a counter */
>       uint64_t mhpmcounter_prev;
> -    /* Value beyond INT64_MAX before overflow interrupt trigger */
> -    uint64_t irq_overflow_left;
>   } PMUCTRState;
>   
>   typedef enum {
> @@ -583,6 +581,8 @@ struct ArchCPU {
>       RISCVSATPModes satp_modes;
>   
>       QEMUTimer *pmu_timer;
> +    uint64_t pmu_timer_instret_snapshot;
> +    bool pmu_timer_stalled;
>       /* A bitmask of Available programmable counters */
>       uint32_t pmu_avail_ctrs;
>       /* Mapping of events to counters */
> diff --git a/target/riscv/tcg/cpu_helper.c b/target/riscv/tcg/cpu_helper.c
> index 07d92226527d85da93b8810e526d044e64fa4e3d..89751cdbf29f5bbd46d0d6b8c9d23eac5e3accac 100644
> --- a/target/riscv/tcg/cpu_helper.c
> +++ b/target/riscv/tcg/cpu_helper.c
> @@ -889,6 +889,8 @@ void riscv_cpu_set_mode(CPURISCVState *env, privilege_mode_t newpriv,
>               riscv_cpu_update_mip(env, 0, 0);
>           }
>       }
> +
> +    riscv_pmu_rebuild_timer(env);
>   }
>   
>   /*
> diff --git a/target/riscv/tcg/pmu.c b/target/riscv/tcg/pmu.c
> index 2f600c5a0fc2d7ba380ef34f89af6366e3e27884..f88f6ae671d877ed874d22c9d4b840edb6f433a5 100644
> --- a/target/riscv/tcg/pmu.c
> +++ b/target/riscv/tcg/pmu.c
> @@ -26,13 +26,6 @@
>   #include "system/device_tree.h"
>   #include "system/cpu-timers.h"
>   
> -/*
> - * cpu_get_ticks() does not expose the host tick frequency.  Use a 1 GHz
> - * approximation only when scheduling non-icount overflow checks; fixed
> - * counter values remain in host-tick units.
> - */
> -#define RISCV_PMU_HOST_TICK_HZ_ASSUMED 1000000000
> -
>   static bool riscv_pmu_counter_valid(RISCVCPU *cpu, uint32_t ctr_idx)
>   {
>       if (ctr_idx < 3 || ctr_idx >= RV_MAX_MHPMCOUNTERS ||
> @@ -324,7 +317,6 @@ void riscv_pmu_write_event(CPURISCVState *env, uint32_t ctr_idx,
>                              uint64_t value, uint64_t wr_mask)
>   {
>       RISCVPMUFixedSnapshot snapshot;
> -    PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
>   
>       riscv_pmu_take_fixed_snapshot(env, &snapshot);
>       if (riscv_pmu_fixed_ctr_running(env, ctr_idx)) {
> @@ -337,10 +329,7 @@ void riscv_pmu_write_event(CPURISCVState *env, uint32_t ctr_idx,
>       if (riscv_pmu_fixed_ctr_enabled(env, ctr_idx)) {
>           riscv_pmu_set_fixed_baseline(env, ctr_idx, &snapshot);
>       }
> -
> -    if (riscv_pmu_fixed_ctr_running(env, ctr_idx)) {
> -        riscv_pmu_setup_timer(env, counter->mhpmcounter_val, ctr_idx);
> -    }
> +    riscv_pmu_rebuild_timer(env);
>   }
>   
>   void riscv_pmu_write_counter(CPURISCVState *env, uint32_t ctr_idx,
> @@ -349,23 +338,19 @@ void riscv_pmu_write_counter(CPURISCVState *env, uint32_t ctr_idx,
>       RISCVPMUFixedSnapshot snapshot;
>       PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
>       bool rv32 = xl == MXL_RV32;
> -    bool running;
>       int start = upper_half ? 32 : 0;
>       int length = rv32 ? 32 : 64;
>   
>       g_assert(rv32 || !upper_half);
>   
>       riscv_pmu_take_fixed_snapshot(env, &snapshot);
> -    running = riscv_pmu_fixed_ctr_running(env, ctr_idx);
> -    if (running) {
> +    if (riscv_pmu_fixed_ctr_running(env, ctr_idx)) {
>           riscv_pmu_accumulate_fixed_delta(env, ctr_idx, &snapshot);
>       }
>       counter->mhpmcounter_val = deposit64(counter->mhpmcounter_val,
>                                            start, length, value);
>       /* mhpmcounter_prev tracks the source, not the written counter value. */
> -    if (running && ctr_idx > 2) {
> -        riscv_pmu_setup_timer(env, counter->mhpmcounter_val, ctr_idx);
> -    }
> +    riscv_pmu_rebuild_timer(env);
>   }
>   
>   void riscv_pmu_write_inhibit(CPURISCVState *env, uint32_t value)
> @@ -396,11 +381,8 @@ void riscv_pmu_write_inhibit(CPURISCVState *env, uint32_t value)
>           if (riscv_pmu_fixed_ctr_enabled(env, ctr_idx)) {
>               riscv_pmu_set_fixed_baseline(env, ctr_idx, &snapshot);
>           }
> -        if (ctr_idx > 2 && riscv_pmu_fixed_ctr_running(env, ctr_idx)) {
> -            riscv_pmu_setup_timer(env, env->pmu_ctrs[ctr_idx].mhpmcounter_val,
> -                                  ctr_idx);
> -        }
>       }
> +    riscv_pmu_rebuild_timer(env);
>   }
>   
>   void riscv_pmu_decr_instret(CPURISCVState *env)
> @@ -512,17 +494,6 @@ static bool riscv_pmu_event_supported(uint32_t event_idx)
>       }
>   }
>   
> -static int64_t pmu_ticks_to_ns(CPURISCVState *env, uint32_t ctr_idx,
> -                               int64_t value)
> -{
> -    if (icount_enabled() &&
> -        riscv_pmu_ctr_monitor_instructions(env, ctr_idx)) {
> -        return icount_to_ns(value);
> -    }
> -
> -    return (NANOSECONDS_PER_SECOND / RISCV_PMU_HOST_TICK_HZ_ASSUMED) * value;
> -}
> -
>   void riscv_pmu_rebuild_event_map(CPURISCVState *env)
>   {
>       uint32_t ctr_idx, ctr_mask, event_idx;
> @@ -551,148 +522,132 @@ void riscv_pmu_rebuild_event_map(CPURISCVState *env)
>       }
>   }
>   
> -static bool pmu_hpmevent_set_of_if_clear(CPURISCVState *env, uint32_t ctr_idx)
> -{
> -    if (!get_field(env->mhpmevent_val[ctr_idx], MHPMEVENT_BIT_OF)) {
> -        env->mhpmevent_val[ctr_idx] |= MHPMEVENT_BIT_OF;
> -        return true;
> -    } else {
> -        return false;
> -    }
> -}
> -
> -static void pmu_timer_trigger_irq_counter(RISCVCPU *cpu, uint32_t ctr_idx)
> +static int64_t riscv_pmu_overflow_delay_ns(CPURISCVState *env,
> +                                           uint32_t ctr_idx,
> +                                           uint64_t value, int64_t now)
>   {
> -    CPURISCVState *env = &cpu->env;
> -    PMUCTRState *counter;
> -    int64_t irq_trigger_at;
> -    uint64_t curr_ctr_val, curr_ctrh_val;
> -    uint64_t ctr_val;
> +    uint64_t remaining;
> +    uint64_t max_delay = INT64_MAX - now;
>   
> -    if (!riscv_pmu_counter_enabled(cpu, ctr_idx)) {
> -        return;
> +    if (!value) {
> +        /* A complete 64-bit wrap is beyond the signed timer horizon. */
> +        return max_delay;
>       }
> +    remaining = -value;
>   
> -    /* Generate interrupt only if OF bit is clear */
> -    if (get_field(env->mhpmevent_val[ctr_idx], MHPMEVENT_BIT_OF)) {
> -        return;
> -    }
> -
> -    counter = &env->pmu_ctrs[ctr_idx];
> -    if (counter->irq_overflow_left > 0) {
> -        irq_trigger_at = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> -                        counter->irq_overflow_left;
> -        timer_mod_anticipate_ns(cpu->pmu_timer, irq_trigger_at);
> -        counter->irq_overflow_left = 0;
> -        return;
> -    }
> +    if (icount_enabled() &&
> +        riscv_pmu_ctr_monitor_instructions(env, ctr_idx)) {
> +        /* Use one adaptive-shift sample for both bounds and conversion. */
> +        uint64_t ns_per_tick = icount_to_ns(1);
> +        uint64_t max_ticks = max_delay / ns_per_tick;
>   
> -    riscv_pmu_read_ctr(env, (target_ulong *)&curr_ctr_val, false, ctr_idx,
> -                        riscv_cpu_mxl(env));
> -    ctr_val = counter->mhpmcounter_val;
> -    if (riscv_cpu_mxl(env) == MXL_RV32) {
> -        riscv_pmu_read_ctr(env, (target_ulong *)&curr_ctrh_val, true, ctr_idx,
> -                            riscv_cpu_mxl(env));
> -        curr_ctr_val = curr_ctr_val | (curr_ctrh_val << 32);
> +        if (remaining > max_ticks) {
> +            return max_delay;
> +        }
> +        return remaining * ns_per_tick;
>       }
>   
>       /*
> -     * We can not accommodate for inhibited modes when setting up timer. Check
> -     * if the counter has actually overflowed or not by comparing current
> -     * counter value (accommodated for inhibited modes) with software written
> -     * counter value.
> +     * Cycle under icount is already virtual ns.  Non-icount fixed events
> +     * retain QEMU's existing one-host-tick-per-ns deadline approximation.
>        */
> -    if (curr_ctr_val >= ctr_val) {
> -        riscv_pmu_setup_timer(env, curr_ctr_val, ctr_idx);
> -        return;
> -    }
> -
> -    if (cpu->pmu_avail_ctrs & BIT(ctr_idx)) {
> -        if (pmu_hpmevent_set_of_if_clear(env, ctr_idx)) {
> -            riscv_cpu_update_mip(env, MIP_LCOFIP, BOOL_TO_MASK(1));
> -        }
> -    }
> +    return MIN(remaining, max_delay);
>   }
>   
> -static void pmu_timer_trigger_irq(RISCVCPU *cpu,
> -                                  enum riscv_pmu_event_idx evt_idx)
> +static void riscv_pmu_rebuild_timer_internal(CPURISCVState *env,
> +                                             bool timer_expired)
>   {
> +    RISCVCPU *cpu = env_archcpu(env);
> +    RISCVPMUFixedSnapshot snapshot;
>       uint32_t ctr_idx;
>       uint32_t ctr_mask;
> +    int64_t deadline = INT64_MAX;
> +    int64_t now;
> +    bool have_deadline = false;
> +    bool timer_horizon_exhausted;
> +    bool stalled = false;
>   
> -    if (evt_idx != RISCV_PMU_EVENT_HW_CPU_CYCLES &&
> -        evt_idx != RISCV_PMU_EVENT_HW_INSTRUCTIONS) {
> +    if (!cpu->pmu_timer) {
>           return;
>       }
>   
> -    ctr_mask = riscv_pmu_event_counter_mask(cpu, evt_idx);
> +    riscv_pmu_take_fixed_snapshot(env, &snapshot);
> +    now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +    /* No future absolute timer deadline is representable at this point. */
> +    timer_horizon_exhausted = now == INT64_MAX;
> +
> +    ctr_mask = riscv_pmu_event_counter_mask(
> +        cpu, RISCV_PMU_EVENT_HW_CPU_CYCLES);
> +    ctr_mask |= riscv_pmu_event_counter_mask(
> +        cpu, RISCV_PMU_EVENT_HW_INSTRUCTIONS);
>   
>       while (ctr_mask) {
> +        PMUCTRState *counter;
> +        int64_t candidate;
> +
>           ctr_idx = ctz32(ctr_mask);
>           ctr_mask &= ~BIT(ctr_idx);
> -        pmu_timer_trigger_irq_counter(cpu, ctr_idx);
> -    }
> -}
> +        counter = &env->pmu_ctrs[ctr_idx];
>   
> -/* Timer callback for instret and cycle counter overflow */
> -void riscv_pmu_timer_cb(void *priv)
> -{
> -    RISCVCPU *cpu = priv;
> +        if (!riscv_pmu_fixed_ctr_running(env, ctr_idx)) {
> +            continue;
> +        }
> +        riscv_pmu_accumulate_fixed_delta(env, ctr_idx, &snapshot);
> +        if ((env->mhpmevent_val[ctr_idx] & MHPMEVENT_BIT_OF) ||
> +            riscv_pmu_counter_filtered(env,
> +                                       env->mhpmevent_val[ctr_idx])) {
> +            continue;
> +        }
>   
> -    /* Timer event was triggered only for these events */
> -    pmu_timer_trigger_irq(cpu, RISCV_PMU_EVENT_HW_CPU_CYCLES);
> -    pmu_timer_trigger_irq(cpu, RISCV_PMU_EVENT_HW_INSTRUCTIONS);
> -}
> +        /*
> +         * Settle current deltas and overflows even when no future deadline is
> +         * representable.
> +         */
> +        if (timer_horizon_exhausted) {
> +            continue;
> +        }
>   
> -int riscv_pmu_setup_timer(CPURISCVState *env, uint64_t value, uint32_t ctr_idx)
> -{
> -    uint64_t overflow_delta, overflow_at, curr_ns;
> -    int64_t overflow_ns, overflow_left = 0;
> -    RISCVCPU *cpu = env_archcpu(env);
> -    PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
> +        if (timer_expired && icount_enabled() &&
> +            riscv_pmu_ctr_monitor_instructions(env, ctr_idx) &&
> +            snapshot.instret == cpu->pmu_timer_instret_snapshot) {
> +            /*
> +             * Icount can warp QEMU_CLOCK_VIRTUAL to this deadline without
> +             * executing an instruction. Re-arming the unchanged instruction
> +             * distance would create a warp/rearm loop; defer it until this
> +             * CPU enters execution again.
> +             */
> +            stalled = true;
> +            continue;
> +        }
>   
> -    /* No need to setup a timer if LCOFI is disabled when OF is set */
> -    if (!riscv_pmu_counter_valid(cpu, ctr_idx) || !cpu->cfg.ext_sscofpmf ||
> -        get_field(env->mhpmevent_val[ctr_idx], MHPMEVENT_BIT_OF)) {
> -        return -1;
> +        candidate = now + riscv_pmu_overflow_delay_ns(
> +                              env, ctr_idx, counter->mhpmcounter_val, now);
> +        if (!have_deadline || candidate < deadline) {
> +            deadline = candidate;
> +            have_deadline = true;
> +        }
>       }
>   
> -    if (value) {
> -        overflow_delta = UINT64_MAX - value + 1;
> +    cpu->pmu_timer_instret_snapshot = snapshot.instret;
> +    cpu->pmu_timer_stalled = stalled;
> +    if (have_deadline) {
> +        timer_mod_ns(cpu->pmu_timer, deadline);
>       } else {
> -        overflow_delta = UINT64_MAX;
> -    }
> -
> -    /*
> -     * QEMU supports only int64_t timers while RISC-V counters are uint64_t.
> -     * Compute the leftover and save it so that it can be reprogrammed again
> -     * when timer expires.
> -     */
> -    if (overflow_delta > INT64_MAX) {
> -        overflow_left = overflow_delta - INT64_MAX;
> +        timer_del(cpu->pmu_timer);
>       }
> +}
>   
> -    if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) ||
> -        riscv_pmu_ctr_monitor_instructions(env, ctr_idx)) {
> -        overflow_ns = pmu_ticks_to_ns(env, ctr_idx,
> -                                      (int64_t)overflow_delta);
> -        overflow_left = pmu_ticks_to_ns(env, ctr_idx, overflow_left);
> -    } else {
> -        return -1;
> -    }
> -    curr_ns = (uint64_t)qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> -    overflow_at =  curr_ns + overflow_ns;
> -    if (overflow_at <= curr_ns)
> -        overflow_at = UINT64_MAX;
> +void riscv_pmu_rebuild_timer(CPURISCVState *env)
> +{
> +    riscv_pmu_rebuild_timer_internal(env, false);
> +}
>   
> -    if (overflow_at > INT64_MAX) {
> -        overflow_left += overflow_at - INT64_MAX;
> -        counter->irq_overflow_left = overflow_left;
> -        overflow_at = INT64_MAX;
> -    }
> -    timer_mod_anticipate_ns(cpu->pmu_timer, overflow_at);
> +/* Timer callback for instret and cycle counter overflow */
> +void riscv_pmu_timer_cb(void *priv)
> +{
> +    RISCVCPU *cpu = priv;
>   
> -    return 0;
> +    riscv_pmu_rebuild_timer_internal(&cpu->env, true);
>   }
>   
>   
> diff --git a/target/riscv/tcg/pmu.h b/target/riscv/tcg/pmu.h
> index 1494fbc21f53137a90c1338f6ca8e3c3e750276a..d9238ae680f5e67031511db4f9afc2884212c5c2 100644
> --- a/target/riscv/tcg/pmu.h
> +++ b/target/riscv/tcg/pmu.h
> @@ -44,12 +44,11 @@ void riscv_pmu_write_counter(CPURISCVState *env, uint32_t ctr_idx,
>                                target_ulong value, bool upper_half, RISCVMXL xl);
>   void riscv_pmu_write_inhibit(CPURISCVState *env, uint32_t value);
>   void riscv_pmu_timer_cb(void *priv);
> +void riscv_pmu_rebuild_timer(CPURISCVState *env);
>   void riscv_pmu_init(RISCVCPU *cpu, Error **errp);
>   void riscv_pmu_rebuild_event_map(CPURISCVState *env);
>   int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum riscv_pmu_event_idx event_idx);
>   void riscv_pmu_generate_fdt_node(void *fdt, uint32_t cmask, char *pmu_name);
> -int riscv_pmu_setup_timer(CPURISCVState *env, uint64_t value,
> -                          uint32_t ctr_idx);
>   void riscv_pmu_update_fixed_ctrs(CPURISCVState *env, privilege_mode_t newpriv,
>                                    bool new_virt);
>   void riscv_pmu_decr_instret(CPURISCVState *env);
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index b68160af8307c1e46d3f200c5313823d240eb7b3..cdcb94f3bcaf0526512f1994e9f7127209e1adcb 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -247,6 +247,15 @@ static void riscv_restore_state_to_opc(CPUState *cs,
>   }
>   
>   #ifndef CONFIG_USER_ONLY
> +static void riscv_cpu_exec_enter(CPUState *cs)
> +{
> +    RISCVCPU *cpu = RISCV_CPU(cs);
> +
> +    if (cpu->pmu_timer_stalled) {
> +        riscv_pmu_rebuild_timer(&cpu->env);
> +    }
> +}
> +
>   static vaddr riscv_pointer_wrap(CPUState *cs, int mmu_idx,
>                                   vaddr result, vaddr base)
>   {
> @@ -283,6 +292,7 @@ const TCGCPUOps riscv_tcg_ops = {
>       .mmu_index = riscv_cpu_mmu_index,
>   
>   #ifndef CONFIG_USER_ONLY
> +    .cpu_exec_enter = riscv_cpu_exec_enter,
>       .tlb_fill = riscv_cpu_tlb_fill,
>       .pointer_wrap = riscv_pointer_wrap,
>       .cpu_exec_interrupt = riscv_cpu_exec_interrupt,
> diff --git a/tests/tcg/riscv64/pmu-cycle-controls.S b/tests/tcg/riscv64/pmu-cycle-controls.S
> index fdd14755f4d196dc2a3ec6c8b8540adc40b3a253..474d46d61baa2c3ec55a5a40d7bdb7ddd3dd7f2f 100644
> --- a/tests/tcg/riscv64/pmu-cycle-controls.S
> +++ b/tests/tcg/riscv64/pmu-cycle-controls.S
> @@ -17,6 +17,8 @@ _start:
>   	 * 1: mcycle changes while M-mode is filtered
>   	 * 2: disabling MINH adds the filtered interval
>   	 * 3: mcycle does not resume after disabling MINH
> +	 * 4: writing mhpmevent3 advances mcycle while CY is set
> +	 * 5: mcycle does not resume after clearing CY
>   	 */
>   	li	t4, 0
>   	csrw	mcountinhibit, zero
> @@ -61,6 +63,31 @@ _start:
>   	slli	t1, t1, 3
>   	or	t4, t4, t1
>   
> +	/* Changing HPM3's event must leave mcycle stopped while CY is set. */
> +	li	t0, 1			/* mcountinhibit.CY */
> +	csrw	mcountinhibit, t0
> +	csrr	s4, mcycle
> +	.rept	128
> +	nop
> +	.endr
> +	li	t0, 1			/* HW_CPU_CYCLES */
> +	csrw	mhpmevent3, t0		/* mhpmevent3; rebuilds PMU timer */
> +	csrr	s5, mcycle
> +	xor	t1, s4, s5
> +	sltu	t1, zero, t1
> +	slli	t1, t1, 4
> +	or	t4, t4, t1
> +	csrw	mcountinhibit, zero
> +	csrr	s6, mcycle
> +	.rept	128
> +	nop
> +	.endr
> +	csrr	t1, mcycle
> +	sltu	t1, s6, t1
> +	xori	t1, t1, 1
> +	slli	t1, t1, 5
> +	or	t4, t4, t1
> +	csrw	mhpmevent3, zero
>   	lla	a1, semiargs
>   	li	t0, 0x20026		/* ADP_Stopped_ApplicationExit */
>   	sd	t0, 0(a1)
>