Re: [PATCH 06/10] target/arm: Detect overflow when calculating next PMU interrupt

[Richard Henderson]

On 8/11/22 10:16, Peter Maydell wrote:
> In pmccntr_op_finish() and pmevcntr_op_finish() we calculate the next
> point at which we will get an overflow and need to fire the PMU
> interrupt or set the overflow flag.  We do this by calculating the
> number of nanoseconds to the overflow event and then adding it to
> qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL).  However, we don't check
> whether that signed addition overflows, which can happen if the next
> PMU interrupt would happen massively far in the future (250 years or
> more).
>
> Since QEMU assumes that "when the QEMU_CLOCK_VIRTUAL rolls over" is
> "never", the sensible behaviour in this situation is simply to not
> try to set the timer if it would be beyond that point.  Detect the
> overflow, and skip setting the timer in that case.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/helper.c | 22 ++++++++++++++--------
>   1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 434885d024a..b7a420981f8 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -1227,10 +1227,13 @@ static void pmccntr_op_finish(CPUARMState *env)
>           int64_t overflow_in = cycles_ns_per(remaining_cycles);
>   
>           if (overflow_in > 0) {
> -            int64_t overflow_at = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> -                overflow_in;
> -            ARMCPU *cpu = env_archcpu(env);
> -            timer_mod_anticipate_ns(cpu->pmu_timer, overflow_at);
> +            int64_t overflow_at;
> +
> +            if (!sadd64_overflow(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> +                                overflow_in, &overflow_at)) {

Maybe my mailer, but it looks like alignment is off on the continuation line 
here...

> +            int64_t overflow_at;
> +
> +            if (!sadd64_overflow(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> +                                 overflow_in, &overflow_at)) {

but not here.

Anyway,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~