Re: [PATCH 2/2] target/arm: Implement FEAT WFxT and enable for '-cpu max'

[Peter Maydell]

On Tue, 30 Apr 2024 at 18:31, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 4/30/24 07:00, Peter Maydell wrote:
> > +    if (uadd64_overflow(timeout, offset, &nexttick)) {
> > +        nexttick = UINT64_MAX;
> > +    }
> > +    if (nexttick > INT64_MAX / gt_cntfrq_period_ns(cpu)) {
> > +        /*
> > +         * If the timeout is too long for the signed 64-bit range
> > +         * of a QEMUTimer, let it expire early.
> > +         */
> > +        timer_mod_ns(cpu->wfxt_timer, INT64_MAX);
> > +    } else {
> > +        timer_mod(cpu->wfxt_timer, nexttick);
> > +    }
>
> The use of both UINT64_MAX and INT64_MAX is confusing.  Perhaps
>
>      if (uadd64_overflow(timeout, offset, &nexttick) ||
>          nexttick > INT64_MAX / gt_cntfrq_period_ns(cpu)) {
>          nexttick = INT64_MAX;
>      }
>      timer_mod(cpu->wfxt_timer, nexttick);

I'm following here the pattern of the logic in gt_recalc_timer()
(which could admittedly also be considered confusing...).

Also note that timer_mod_ns() and timer_mod() aren't the
same thing. The latter calls timer_mod_ns() on its argument
multiplied by ts->scale, so if you pass it INT64_MAX
the multiply is liable to overflow.

thanks
-- PMM