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
+             * For the latter case we set nexttick to an "as far in future
+             * as possible" value and let the code below handle it.
+             */
+            if (offset > count) {
+                nexttick = offset;
+            } else {
+                nexttick = UINT64_MAX;
+            }
         } else {
-            /* Next transition is when we hit cval */
-            nexttick = gt->cval + offset;
+            /*
+             * Next transition is when (count - offset) == cval, i.e.
+             * when count == (cval + offset).
+             * If that would overflow, then again we set up the next interrupt
+             * for "as far in the future as possible" for the code below.
+             */
+            if (uadd64_overflow(gt->cval, offset, &nexttick)) {
+                nexttick = UINT64_MAX;
+            }
         }
         /*
          * Note that the desired next expiry time might be beyond the
diff --git a/tests/tcg/aarch64/system/vtimer.c b/tests/tcg/aarch64/system/vtimer.c
new file mode 100644
index 00000000000..42f2f7796c7
--- /dev/null
+++ b/tests/tcg/aarch64/system/vtimer.c
@@ -0,0 +1,48 @@
+/*
+ * Simple Virtual Timer Test
+ *
+ * Copyright (c) 2020 Linaro Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include <inttypes.h>
+#include <minilib.h>
+
+/* grabbed from Linux */
+#define __stringify_1(x...) #x
+#define __stringify(x...)   __stringify_1(x)
+
+#define read_sysreg(r) ({                                           \
+            uint64_t __val;                                         \
+            asm volatile("mrs %0, " __stringify(r) : "=r" (__val)); \
+            __val;                                                  \
+})
+
+#define write_sysreg(r, v) do {                     \
+        uint64_t __val = (uint64_t)(v);             \
+        asm volatile("msr " __stringify(r) ", %x0"  \
+                 : : "rZ" (__val));                 \
+} while (0)
+
+int main(void)
+{
+    int i;
+
+    ml_printf("VTimer Test\n");
+
+    write_sysreg(cntvoff_el2, 1);
+    write_sysreg(cntv_cval_el0, -1);
+    write_sysreg(cntv_ctl_el0, 1);
+
+    ml_printf("cntvoff_el2=%lx\n", read_sysreg(cntvoff_el2));
+    ml_printf("cntv_cval_el0=%lx\n", read_sysreg(cntv_cval_el0));
+    ml_printf("cntv_ctl_el0=%lx\n", read_sysreg(cntv_ctl_el0));
+
+    /* Now read cval a few times */
+    for (i = 0; i < 10; i++) {
+        ml_printf("%d: cntv_cval_el0=%lx\n", i, read_sysreg(cntv_cval_el0));
+    }
+
+    return 0;
+}
diff --git a/tests/tcg/aarch64/Makefile.softmmu-target b/tests/tcg/aarch64/Makefile.softmmu-target
index b74a2534e38..d71659cc226 100644
--- a/tests/tcg/aarch64/Makefile.softmmu-target
+++ b/tests/tcg/aarch64/Makefile.softmmu-target
@@ -45,7 +45,8 @@ TESTS+=memory-sve
 
 # Running
 QEMU_BASE_MACHINE=-M virt -cpu max -display none
-QEMU_OPTS+=$(QEMU_BASE_MACHINE) -semihosting-config enable=on,target=native,chardev=output -kernel
+QEMU_BASE_ARGS=-semihosting-config enable=on,target=native,chardev=output
+QEMU_OPTS+=$(QEMU_BASE_MACHINE) $(QEMU_BASE_ARGS) -kernel
 
 # console test is manual only
 QEMU_SEMIHOST=-chardev stdio,mux=on,id=stdio0 -semihosting-config enable=on,chardev=stdio0 -mon chardev=stdio0,mode=readline
@@ -55,6 +56,10 @@ run-semiconsole: semiconsole
 run-plugin-semiconsole-with-%: semiconsole
 	$(call skip-test, $<, "MANUAL ONLY")
 
+# vtimer test needs EL2
+QEMU_EL2_MACHINE=-machine virt,virtualization=on,gic-version=2 -cpu cortex-a57 -smp 4
+run-vtimer: QEMU_OPTS=$(QEMU_EL2_MACHINE) $(QEMU_BASE_ARGS) -kernel
+
 # Simple Record/Replay Test
 .PHONY: memory-record
 run-memory-record: memory-record memory
-- 
2.34.1

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?

Beyond this comment nit, the action "set nexttick as far in the future as possible" is 
correct.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

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