On Mon, 20 Nov 2023 at 17:52, Richard Henderson <[email protected]> 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: [email protected] > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/60 > > Signed-off-by: Alex Bennée <[email protected]> > > Signed-off-by: Peter Maydell <[email protected]> > > --- > > 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
