Re: [PATCH for-8.2] target/arm: Handle overflow in calculation of next timer tick

[Peter Maydell]

On Mon, 20 Nov 2023 at 17:52, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 11/20/23 09:35, Peter Maydell wrote:
> > In commit edac4d8a168 back in 2015 when we added support for
> > the virtual timer offset CNTVOFF_EL2, we didn't correctly update
> > the timer-recalculation code that figures out when the timer
> > interrupt is next going to change state. We got it wrong in
> > two ways:
> >   * for the 0->1 transition, we didn't notice that gt->cval + offset
> >     can overflow a uint64_t
> >   * for the 1->0 transition, we didn't notice that the transition
> >     might now happen before the count rolls over, if offset > count
> >
> > In the former case, we end up trying to set the next interrupt
> > for a time in the past, which results in QEMU hanging as the
> > timer fires continuously.
> >
> > In the latter case, we would fail to update the interrupt
> > status when we are supposed to.
> >
> > Fix the calculations in both cases.
> >
> > The test case is Alex Bennée's from the bug report, and tests
> > the 0->1 transition overflow case.
> >
> > Fixes: edac4d8a168 ("target-arm: Add CNTVOFF_EL2")
> > Cc: qemu-stable@nongnu.org
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/60
> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> > Thanks to Leonid for his recent patch which prodded me
> > into looking at this again. I preferred to fix both halves
> > of the if(), rather than just one, and I have thrown in
> > Alex's test case since it was conveniently to hand.
> > ---
> >   target/arm/helper.c                       | 25 ++++++++++--
> >   tests/tcg/aarch64/system/vtimer.c         | 48 +++++++++++++++++++++++
> >   tests/tcg/aarch64/Makefile.softmmu-target |  7 +++-
> >   3 files changed, 75 insertions(+), 5 deletions(-)
> >   create mode 100644 tests/tcg/aarch64/system/vtimer.c
> >
> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index ff1970981ee..0430ae55edf 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -2646,11 +2646,28 @@ static void gt_recalc_timer(ARMCPU *cpu, int 
> > timeridx)
> >           gt->ctl = deposit32(gt->ctl, 2, 1, istatus);
> >
> >           if (istatus) {
> > -            /* Next transition is when count rolls back over to zero */
> > -            nexttick = UINT64_MAX;
> > +            /*
> > +             * Next transition is when (count - offset) rolls back over to 
> > 0.
> > +             * If offset > count then this is when count == offset;
> > +             * if offset <= count then this is when count == offset + 
> > UINT64_MAX
>
> Is it really UINT64_MAX or 2**64, i.e. UINT64_MAX + 1?

Yes, it should be the latter, though as you say it doesn't
affect the code.

thanks
-- PMM