On Thu, 16 Jan 2020 at 18:45, Peter Maydell <peter.mayd...@linaro.org> 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
