[prev in list] [next in list] [prev in thread] [next in thread] 

List:       qemu-devel
Subject:    Re: [PATCH 2/2] target/arm: Implement FEAT WFxT and enable for '-cpu max'
From:       Peter Maydell <peter.maydell () linaro ! org>
Date:       2024-04-30 18:42:31
Message-ID: CAFEAcA-G2YAArmV7Ttg_-iRoh43n5YeXiBafqsjeAyu_9u8saA () mail ! gmail ! com
[Download RAW message or body]

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

[prev in list] [next in list] [prev in thread] [next in thread]