Re: [PATCH v1 1/2] target/arm: detect 64 bit overflow caused by high cval + voff

[Peter Maydell]

On Thu, 16 Jan 2020 at 18:45, Peter Maydell <address@hidden> wrote:
> There's something odd going on with this code. Adding a bit of context:
>
>         uint64_t offset = timeridx == GTIMER_VIRT ?
>                                       cpu->env.cp15.cntvoff_el2 : 0;
>         uint64_t count = gt_get_countervalue(&cpu->env);
>         /* Note that this must be unsigned 64 bit arithmetic: */
>         int istatus = count - offset >= gt->cval;
>         [...]
>         if (istatus) {
>             /* Next transition is when count rolls back over to zero */
>             nexttick = UINT64_MAX;
>         } else {
>             /* Next transition is when we hit cval */
>             nexttick = gt->cval + offset;
>         }
>
> I think this patch is correct, in that the 'nexttick' values
> are all absolute and this cval/offset combination implies
> that the next timer interrupt is going to be in a future
> so distant we can't even fit the duration in a uint64_t.
>
> But the other half of the 'if' also looks wrong: that's
> for the case of "timer has fired, how long until the
> wraparound causes the interrupt line to go low again?".
> UINT64_MAX is right for the EL1 case where offset is 0,
> but the offset might actually be set such that the wrap
> around happens fairly soon. We want to calculate the
> tick when (count - offset) hits 0, saturated to
> UINT64_MAX. It's getting late here and I couldn't figure
> out what that expression should be with 15 minutes of
> fiddling around with pen and paper diagrams. I'll have another
> go tomorrow if nobody else gets there first...

With a fresher brain:

For the if (istatus) branch we want the absolute tick
when (count - offset) wraps round to 0, saturated to UINT64_MAX.
I think this is:
    if (offset <= count) {
        nexttick = UINT64_MAX;
    } else {
        nexttick = offset;
    }

Should we consider this a separate bugfix to go in its own patch?

thanks
-- PMM