[Bug 1859021] [NEW] qemu-system-aarch64 (tcg): cval + voff overflow not handled, causes qemu to hang

[Bug 1859021] [NEW] qemu-system-aarch64 (tcg): cval + voff overflow not handled, causes qemu to hang

[Alex Longwall]

Public bug reported:

The Armv8 architecture reference manual states that for any timer set
(e.g. CNTP* and CNTV*), the condition for such timer to generate an
interrupt (if enabled & unmasked) is:

CVAL <= CNT(P/V)CT

Although this is arguably sloppy coding, I have seen code that is
therefore assuming it can set CVAL to a very high value (e.g.
UINT64_MAX) and leave the interrupt enabled in CTL, and never get the
interrupt.

On latest master commit as the time of writing, there is an integer
overflow in target/arm/helper.c gt_recalc_timer affecting the virtual
timer when the interrupt is enabled in CTL:

    /* Next transition is when we hit cval */
    nexttick = gt->cval + offset;

When this overflow happens, I notice that qemu is no longer responsive and that I have to SIGKILL the process:
    - qemu takes nearly all the cpu time of the cores it is running on (e.g. 50% cpu usage if running on half the cores) and is completely unresponsive
    - no guest interrupt (reported via -d int) is generated

Here the minimal code example to reproduce the issue:

    mov     x0, #1
    msr     cntvoff_el2, x0
    mov     x0, #-1
    msr     cntv_cval_el0, x0
    mov     x0, #1
    msr     cntv_ctl_el0, x0 // interrupt generation enabled, not masked; qemu will start to hang here

Options used:
-nographic -machine virt,virtualization=on,gic-version=2,accel=tcg -cpu cortex-a57
-smp 4 -m 1024 -kernel whatever.elf -d unimp,guest_errors,int -semihosting-config enable,target=native
-serial mon:stdio

Version used: 4.2

** Affects: qemu
     Importance: Undecided
         Status: New

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1859021

Title:
  qemu-system-aarch64 (tcg):  cval + voff overflow not handled, causes
  qemu to hang

Status in QEMU:
  New

Bug description:
  The Armv8 architecture reference manual states that for any timer set
  (e.g. CNTP* and CNTV*), the condition for such timer to generate an
  interrupt (if enabled & unmasked) is:

  CVAL <= CNT(P/V)CT

  Although this is arguably sloppy coding, I have seen code that is
  therefore assuming it can set CVAL to a very high value (e.g.
  UINT64_MAX) and leave the interrupt enabled in CTL, and never get the
  interrupt.

  On latest master commit as the time of writing, there is an integer
  overflow in target/arm/helper.c gt_recalc_timer affecting the virtual
  timer when the interrupt is enabled in CTL:

      /* Next transition is when we hit cval */
      nexttick = gt->cval + offset;

  When this overflow happens, I notice that qemu is no longer responsive and that I have to SIGKILL the process:
      - qemu takes nearly all the cpu time of the cores it is running on (e.g. 50% cpu usage if running on half the cores) and is completely unresponsive
      - no guest interrupt (reported via -d int) is generated

  Here the minimal code example to reproduce the issue:

      mov     x0, #1
      msr     cntvoff_el2, x0
      mov     x0, #-1
      msr     cntv_cval_el0, x0
      mov     x0, #1
      msr     cntv_ctl_el0, x0 // interrupt generation enabled, not masked; qemu will start to hang here

  Options used:
  -nographic -machine virt,virtualization=on,gic-version=2,accel=tcg -cpu cortex-a57
  -smp 4 -m 1024 -kernel whatever.elf -d unimp,guest_errors,int -semihosting-config enable,target=native
  -serial mon:stdio

  Version used: 4.2

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1859021/+subscriptions

[Bug 1859021] Re: qemu-system-aarch64 (tcg): cval + voff overflow not handled, causes qemu to hang

[Alex Bennée]

** Tags added: arm tcg

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1859021

Title:
  qemu-system-aarch64 (tcg):  cval + voff overflow not handled, causes
  qemu to hang

Status in QEMU:
  New

Bug description:
  The Armv8 architecture reference manual states that for any timer set
  (e.g. CNTP* and CNTV*), the condition for such timer to generate an
  interrupt (if enabled & unmasked) is:

  CVAL <= CNT(P/V)CT

  Although this is arguably sloppy coding, I have seen code that is
  therefore assuming it can set CVAL to a very high value (e.g.
  UINT64_MAX) and leave the interrupt enabled in CTL, and never get the
  interrupt.

  On latest master commit as the time of writing, there is an integer
  overflow in target/arm/helper.c gt_recalc_timer affecting the virtual
  timer when the interrupt is enabled in CTL:

      /* Next transition is when we hit cval */
      nexttick = gt->cval + offset;

  When this overflow happens, I notice that qemu is no longer responsive and that I have to SIGKILL the process:
      - qemu takes nearly all the cpu time of the cores it is running on (e.g. 50% cpu usage if running on half the cores) and is completely unresponsive
      - no guest interrupt (reported via -d int) is generated

  Here the minimal code example to reproduce the issue:

      mov     x0, #1
      msr     cntvoff_el2, x0
      mov     x0, #-1
      msr     cntv_cval_el0, x0
      mov     x0, #1
      msr     cntv_ctl_el0, x0 // interrupt generation enabled, not masked; qemu will start to hang here

  Options used:
  -nographic -machine virt,virtualization=on,gic-version=2,accel=tcg -cpu cortex-a57
  -smp 4 -m 1024 -kernel whatever.elf -d unimp,guest_errors,int -semihosting-config enable,target=native
  -serial mon:stdio

  Version used: 4.2

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1859021/+subscriptions

[RFC PATCH] tests/tcg: add a vtimer test for aarch64

[Alex Bennée]

Bug: https://bugs.launchpad.net/bugs/1859021

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/tcg/aarch64/system/vtimer.c         | 48 +++++++++++++++++++++++
 tests/tcg/aarch64/Makefile.softmmu-target |  4 ++
 2 files changed, 52 insertions(+)
 create mode 100644 tests/tcg/aarch64/system/vtimer.c

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 7b4eede3f07..62cdddbb215 100644
--- a/tests/tcg/aarch64/Makefile.softmmu-target
+++ b/tests/tcg/aarch64/Makefile.softmmu-target
@@ -62,3 +62,7 @@ run-memory-replay: memory-replay run-memory-record
 	  "$< on $(TARGET_NAME)")
 
 EXTRA_TESTS+=memory-record memory-replay
+
+# vtimer test
+QEMU_EL2_MACHINE=-machine virt,virtualization=on,gic-version=2 -cpu cortex-a57 -smp 4
+run-vtimer: QEMU_OPTS=$(QEMU_EL2_MACHINE) $(QEMU_SEMIHOST)  -kernel
-- 
2.20.1

[Bug 1859021] Re: qemu-system-aarch64 (tcg): cval + voff overflow not handled, causes qemu to hang

[Alex Bennée]

Bug: https://bugs.launchpad.net/bugs/1859021

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/tcg/aarch64/system/vtimer.c         | 48 +++++++++++++++++++++++
 tests/tcg/aarch64/Makefile.softmmu-target |  4 ++
 2 files changed, 52 insertions(+)
 create mode 100644 tests/tcg/aarch64/system/vtimer.c

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 7b4eede3f07..62cdddbb215 100644
--- a/tests/tcg/aarch64/Makefile.softmmu-target
+++ b/tests/tcg/aarch64/Makefile.softmmu-target
@@ -62,3 +62,7 @@ run-memory-replay: memory-replay run-memory-record
 	  "$< on $(TARGET_NAME)")
 
 EXTRA_TESTS+=memory-record memory-replay
+
+# vtimer test
+QEMU_EL2_MACHINE=-machine virt,virtualization=on,gic-version=2 -cpu cortex-a57 -smp 4
+run-vtimer: QEMU_OPTS=$(QEMU_EL2_MACHINE) $(QEMU_SEMIHOST)  -kernel
-- 
2.20.1


** Changed in: qemu
       Status: New => Confirmed

** Changed in: qemu
     Assignee: (unassigned) => Alex Bennée (ajbennee)

** Tags added: testcase

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1859021

Title:
  qemu-system-aarch64 (tcg):  cval + voff overflow not handled, causes
  qemu to hang

Status in QEMU:
  Confirmed

Bug description:
  The Armv8 architecture reference manual states that for any timer set
  (e.g. CNTP* and CNTV*), the condition for such timer to generate an
  interrupt (if enabled & unmasked) is:

  CVAL <= CNT(P/V)CT

  Although this is arguably sloppy coding, I have seen code that is
  therefore assuming it can set CVAL to a very high value (e.g.
  UINT64_MAX) and leave the interrupt enabled in CTL, and never get the
  interrupt.

  On latest master commit as the time of writing, there is an integer
  overflow in target/arm/helper.c gt_recalc_timer affecting the virtual
  timer when the interrupt is enabled in CTL:

      /* Next transition is when we hit cval */
      nexttick = gt->cval + offset;

  When this overflow happens, I notice that qemu is no longer responsive and that I have to SIGKILL the process:
      - qemu takes nearly all the cpu time of the cores it is running on (e.g. 50% cpu usage if running on half the cores) and is completely unresponsive
      - no guest interrupt (reported via -d int) is generated

  Here the minimal code example to reproduce the issue:

      mov     x0, #1
      msr     cntvoff_el2, x0
      mov     x0, #-1
      msr     cntv_cval_el0, x0
      mov     x0, #1
      msr     cntv_ctl_el0, x0 // interrupt generation enabled, not masked; qemu will start to hang here

  Options used:
  -nographic -machine virt,virtualization=on,gic-version=2,accel=tcg -cpu cortex-a57
  -smp 4 -m 1024 -kernel whatever.elf -d unimp,guest_errors,int -semihosting-config enable,target=native
  -serial mon:stdio

  Version used: 4.2

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1859021/+subscriptions

[PATCH v1 0/2] fix for bug 1859021

[Alex Bennée]

Hi,

This is a fairly trivial fix for a uint64_t overflow however I spent
some time making sure I understood why we got stuck in a busy loop and
adding tests to both tcg and kvm-unit-tests.

Alex Bennée (2):
  target/arm: detect 64 bit overflow caused by high cval + voff
  tests/tcg: add a vtimer test for aarch64

 target/arm/helper.c                       |  3 +
 tests/tcg/aarch64/system/vtimer.c         | 80 +++++++++++++++++++++++
 tests/tcg/aarch64/Makefile.softmmu-target |  4 ++
 3 files changed, 87 insertions(+)
 create mode 100644 tests/tcg/aarch64/system/vtimer.c

-- 
2.20.1

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

[Alex Bennée]

If we don't detect this we will be stuck in a busy loop as we schedule
a timer for before now which will continually trigger gt_recalc_timer
even though we haven't reached the state required to trigger the IRQ.

Bug: https://bugs.launchpad.net/bugs/1859021
Cc: 1859021@bugs.launchpad.net
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/arm/helper.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 19a57a17da5..eb17106f7bd 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2481,6 +2481,9 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
         } else {
             /* Next transition is when we hit cval */
             nexttick = gt->cval + offset;
+            if (nexttick < gt->cval) {
+                nexttick = UINT64_MAX;
+            }
         }
         /* Note that the desired next expiry time might be beyond the
          * signed-64-bit range of a QEMUTimer -- in this case we just
-- 
2.20.1

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

[Alex Bennée]

If we don't detect this we will be stuck in a busy loop as we schedule
a timer for before now which will continually trigger gt_recalc_timer
even though we haven't reached the state required to trigger the IRQ.

Bug: https://bugs.launchpad.net/bugs/1859021
Cc: 1859021@bugs.launchpad.net
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/arm/helper.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 19a57a17da5..eb17106f7bd 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2481,6 +2481,9 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
         } else {
             /* Next transition is when we hit cval */
             nexttick = gt->cval + offset;
+            if (nexttick < gt->cval) {
+                nexttick = UINT64_MAX;
+            }
         }
         /* Note that the desired next expiry time might be beyond the
          * signed-64-bit range of a QEMUTimer -- in this case we just
-- 
2.20.1

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1859021

Title:
  qemu-system-aarch64 (tcg):  cval + voff overflow not handled, causes
  qemu to hang

Status in QEMU:
  Confirmed

Bug description:
  The Armv8 architecture reference manual states that for any timer set
  (e.g. CNTP* and CNTV*), the condition for such timer to generate an
  interrupt (if enabled & unmasked) is:

  CVAL <= CNT(P/V)CT

  Although this is arguably sloppy coding, I have seen code that is
  therefore assuming it can set CVAL to a very high value (e.g.
  UINT64_MAX) and leave the interrupt enabled in CTL, and never get the
  interrupt.

  On latest master commit as the time of writing, there is an integer
  overflow in target/arm/helper.c gt_recalc_timer affecting the virtual
  timer when the interrupt is enabled in CTL:

      /* Next transition is when we hit cval */
      nexttick = gt->cval + offset;

  When this overflow happens, I notice that qemu is no longer responsive and that I have to SIGKILL the process:
      - qemu takes nearly all the cpu time of the cores it is running on (e.g. 50% cpu usage if running on half the cores) and is completely unresponsive
      - no guest interrupt (reported via -d int) is generated

  Here the minimal code example to reproduce the issue:

      mov     x0, #1
      msr     cntvoff_el2, x0
      mov     x0, #-1
      msr     cntv_cval_el0, x0
      mov     x0, #1
      msr     cntv_ctl_el0, x0 // interrupt generation enabled, not masked; qemu will start to hang here

  Options used:
  -nographic -machine virt,virtualization=on,gic-version=2,accel=tcg -cpu cortex-a57
  -smp 4 -m 1024 -kernel whatever.elf -d unimp,guest_errors,int -semihosting-config enable,target=native
  -serial mon:stdio

  Version used: 4.2

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1859021/+subscriptions

[PATCH v1 2/2] tests/tcg: add a vtimer test for aarch64

[Alex Bennée]

Bug: https://bugs.launchpad.net/bugs/1859021

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/tcg/aarch64/system/vtimer.c         | 80 +++++++++++++++++++++++
 tests/tcg/aarch64/Makefile.softmmu-target |  4 ++
 2 files changed, 84 insertions(+)
 create mode 100644 tests/tcg/aarch64/system/vtimer.c

diff --git a/tests/tcg/aarch64/system/vtimer.c b/tests/tcg/aarch64/system/vtimer.c
new file mode 100644
index 00000000000..2f6299b5d2c
--- /dev/null
+++ b/tests/tcg/aarch64/system/vtimer.c
@@ -0,0 +1,80 @@
+/*
+ * Simple Virtual Timer Tests
+ *
+ * Note: kvm-unit-tests has a much more comprehensive exercising of
+ * the timer sub-system. However this test case can tweak _EL2 values
+ * to trigger bugs which can't be done with that.
+ *
+ * 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)
+
+/* Physical Counter */
+static uint64_t last_pct;
+/* Timer Values */
+static uint32_t last_phys_tval;
+static uint32_t last_virt_tval;
+
+static void dump_status(void)
+{
+    uint64_t pct = read_sysreg(cntpct_el0);
+    uint32_t phys_tval = read_sysreg(cntp_tval_el0);
+    uint32_t virt_tval = read_sysreg(cntv_tval_el0);
+
+    ml_printf("timer values:\n");
+    /* the physical timer monotonically increments */
+    ml_printf("cntpct_el0=%ld (+%ld)\n", pct, pct - last_pct);
+    /* the various tvals decrement based on cval */
+    ml_printf("cntp_tval_el0=%ld (-%ld)\n", phys_tval,
+              last_phys_tval - phys_tval);
+    ml_printf("cntv_tval_el0=%ld (-%ld)\n", virt_tval,
+              last_virt_tval - virt_tval);
+
+    last_pct = pct;
+    last_phys_tval = phys_tval;
+    last_virt_tval = virt_tval;
+}
+
+int main(void)
+{
+    int i;
+
+    ml_printf("VTimer Tests\n");
+
+    dump_status();
+
+    ml_printf("Tweaking voff_el2 and cval\n");
+    write_sysreg(cntvoff_el2, 1);
+    write_sysreg(cntv_cval_el0, -1);
+
+    dump_status();
+
+    ml_printf("Enabling timer IRQs\n");
+    write_sysreg(cntv_ctl_el0, 1);
+    /* for bug 1859021 we hang here */
+
+    dump_status();
+
+    ml_printf("End of Vtimer test\n");
+    return 0;
+}
diff --git a/tests/tcg/aarch64/Makefile.softmmu-target b/tests/tcg/aarch64/Makefile.softmmu-target
index 7b4eede3f07..62cdddbb215 100644
--- a/tests/tcg/aarch64/Makefile.softmmu-target
+++ b/tests/tcg/aarch64/Makefile.softmmu-target
@@ -62,3 +62,7 @@ run-memory-replay: memory-replay run-memory-record
 	  "$< on $(TARGET_NAME)")
 
 EXTRA_TESTS+=memory-record memory-replay
+
+# vtimer test
+QEMU_EL2_MACHINE=-machine virt,virtualization=on,gic-version=2 -cpu cortex-a57 -smp 4
+run-vtimer: QEMU_OPTS=$(QEMU_EL2_MACHINE) $(QEMU_SEMIHOST)  -kernel
-- 
2.20.1

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

[Peter Maydell]

On Fri, 10 Jan 2020 at 16:16, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> If we don't detect this we will be stuck in a busy loop as we schedule
> a timer for before now which will continually trigger gt_recalc_timer
> even though we haven't reached the state required to trigger the IRQ.
>
> Bug: https://bugs.launchpad.net/bugs/1859021
> Cc: 1859021@bugs.launchpad.net
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  target/arm/helper.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 19a57a17da5..eb17106f7bd 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -2481,6 +2481,9 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
>          } else {
>              /* Next transition is when we hit cval */
>              nexttick = gt->cval + offset;
> +            if (nexttick < gt->cval) {
> +                nexttick = UINT64_MAX;
> +            }
>          }

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...

thanks
-- PMM

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

[Peter Maydell]

On Fri, 10 Jan 2020 at 16:16, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> If we don't detect this we will be stuck in a busy loop as we schedule
> a timer for before now which will continually trigger gt_recalc_timer
> even though we haven't reached the state required to trigger the IRQ.
>
> Bug: https://bugs.launchpad.net/bugs/1859021
> Cc: 1859021@bugs.launchpad.net
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  target/arm/helper.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 19a57a17da5..eb17106f7bd 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -2481,6 +2481,9 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
>          } else {
>              /* Next transition is when we hit cval */
>              nexttick = gt->cval + offset;
> +            if (nexttick < gt->cval) {
> +                nexttick = UINT64_MAX;
> +            }
>          }

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...

thanks
-- PMM

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1859021

Title:
  qemu-system-aarch64 (tcg):  cval + voff overflow not handled, causes
  qemu to hang

Status in QEMU:
  Confirmed

Bug description:
  The Armv8 architecture reference manual states that for any timer set
  (e.g. CNTP* and CNTV*), the condition for such timer to generate an
  interrupt (if enabled & unmasked) is:

  CVAL <= CNT(P/V)CT

  Although this is arguably sloppy coding, I have seen code that is
  therefore assuming it can set CVAL to a very high value (e.g.
  UINT64_MAX) and leave the interrupt enabled in CTL, and never get the
  interrupt.

  On latest master commit as the time of writing, there is an integer
  overflow in target/arm/helper.c gt_recalc_timer affecting the virtual
  timer when the interrupt is enabled in CTL:

      /* Next transition is when we hit cval */
      nexttick = gt->cval + offset;

  When this overflow happens, I notice that qemu is no longer responsive and that I have to SIGKILL the process:
      - qemu takes nearly all the cpu time of the cores it is running on (e.g. 50% cpu usage if running on half the cores) and is completely unresponsive
      - no guest interrupt (reported via -d int) is generated

  Here the minimal code example to reproduce the issue:

      mov     x0, #1
      msr     cntvoff_el2, x0
      mov     x0, #-1
      msr     cntv_cval_el0, x0
      mov     x0, #1
      msr     cntv_ctl_el0, x0 // interrupt generation enabled, not masked; qemu will start to hang here

  Options used:
  -nographic -machine virt,virtualization=on,gic-version=2,accel=tcg -cpu cortex-a57
  -smp 4 -m 1024 -kernel whatever.elf -d unimp,guest_errors,int -semihosting-config enable,target=native
  -serial mon:stdio

  Version used: 4.2

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1859021/+subscriptions

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 <peter.maydell@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

[Bug 1859021] 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 <peter.maydell@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

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1859021

Title:
  qemu-system-aarch64 (tcg):  cval + voff overflow not handled, causes
  qemu to hang

Status in QEMU:
  Confirmed

Bug description:
  The Armv8 architecture reference manual states that for any timer set
  (e.g. CNTP* and CNTV*), the condition for such timer to generate an
  interrupt (if enabled & unmasked) is:

  CVAL <= CNT(P/V)CT

  Although this is arguably sloppy coding, I have seen code that is
  therefore assuming it can set CVAL to a very high value (e.g.
  UINT64_MAX) and leave the interrupt enabled in CTL, and never get the
  interrupt.

  On latest master commit as the time of writing, there is an integer
  overflow in target/arm/helper.c gt_recalc_timer affecting the virtual
  timer when the interrupt is enabled in CTL:

      /* Next transition is when we hit cval */
      nexttick = gt->cval + offset;

  When this overflow happens, I notice that qemu is no longer responsive and that I have to SIGKILL the process:
      - qemu takes nearly all the cpu time of the cores it is running on (e.g. 50% cpu usage if running on half the cores) and is completely unresponsive
      - no guest interrupt (reported via -d int) is generated

  Here the minimal code example to reproduce the issue:

      mov     x0, #1
      msr     cntvoff_el2, x0
      mov     x0, #-1
      msr     cntv_cval_el0, x0
      mov     x0, #1
      msr     cntv_ctl_el0, x0 // interrupt generation enabled, not masked; qemu will start to hang here

  Options used:
  -nographic -machine virt,virtualization=on,gic-version=2,accel=tcg -cpu cortex-a57
  -smp 4 -m 1024 -kernel whatever.elf -d unimp,guest_errors,int -semihosting-config enable,target=native
  -serial mon:stdio

  Version used: 4.2

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1859021/+subscriptions

Re: [PATCH v1 2/2] tests/tcg: add a vtimer test for aarch64

[Peter Maydell]

On Fri, 10 Jan 2020 at 16:16, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Bug: https://bugs.launchpad.net/bugs/1859021
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  tests/tcg/aarch64/system/vtimer.c         | 80 +++++++++++++++++++++++
>  tests/tcg/aarch64/Makefile.softmmu-target |  4 ++
>  2 files changed, 84 insertions(+)
>  create mode 100644 tests/tcg/aarch64/system/vtimer.c
>
> diff --git a/tests/tcg/aarch64/system/vtimer.c b/tests/tcg/aarch64/system/vtimer.c
> new file mode 100644
> index 00000000000..2f6299b5d2c
> --- /dev/null
> +++ b/tests/tcg/aarch64/system/vtimer.c
> @@ -0,0 +1,80 @@
> +/*
> + * Simple Virtual Timer Tests
> + *
> + * Note: kvm-unit-tests has a much more comprehensive exercising of
> + * the timer sub-system. However this test case can tweak _EL2 values
> + * to trigger bugs which can't be done with that.
> + *
> + * 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)

Code 'grabbed from Linux' is unlikely to be GPL-2-or-later...

QEMU already has a stringify() macro in compiler.h.

thanks
-- PMM

Re: [PATCH v1 2/2] tests/tcg: add a vtimer test for aarch64

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Fri, 10 Jan 2020 at 16:16, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> Bug: https://bugs.launchpad.net/bugs/1859021
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  tests/tcg/aarch64/system/vtimer.c         | 80 +++++++++++++++++++++++
>>  tests/tcg/aarch64/Makefile.softmmu-target |  4 ++
>>  2 files changed, 84 insertions(+)
>>  create mode 100644 tests/tcg/aarch64/system/vtimer.c
>>
>> diff --git a/tests/tcg/aarch64/system/vtimer.c b/tests/tcg/aarch64/system/vtimer.c
>> new file mode 100644
>> index 00000000000..2f6299b5d2c
>> --- /dev/null
>> +++ b/tests/tcg/aarch64/system/vtimer.c
>> @@ -0,0 +1,80 @@
>> +/*
>> + * Simple Virtual Timer Tests
>> + *
>> + * Note: kvm-unit-tests has a much more comprehensive exercising of
>> + * the timer sub-system. However this test case can tweak _EL2 values
>> + * to trigger bugs which can't be done with that.
>> + *
>> + * 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)
>
> Code 'grabbed from Linux' is unlikely to be GPL-2-or-later...
>
> QEMU already has a stringify() macro in compiler.h.

Hmm I don't think I can include compiler.h in a system mode test. I can
certainly copy and paste our local version though ;-)

>
> thanks
> -- PMM


-- 
Alex Bennée

[Bug 1859021] Re: qemu-system-aarch64 (tcg): cval + voff overflow not handled, causes qemu to hang

[Alex Bennée]

A different approach was posted that basically elides the overflow case
by not scheduling timers for IRQ events which have already happened:

  https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg07915.html

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1859021

Title:
  qemu-system-aarch64 (tcg):  cval + voff overflow not handled, causes
  qemu to hang

Status in QEMU:
  Confirmed

Bug description:
  The Armv8 architecture reference manual states that for any timer set
  (e.g. CNTP* and CNTV*), the condition for such timer to generate an
  interrupt (if enabled & unmasked) is:

  CVAL <= CNT(P/V)CT

  Although this is arguably sloppy coding, I have seen code that is
  therefore assuming it can set CVAL to a very high value (e.g.
  UINT64_MAX) and leave the interrupt enabled in CTL, and never get the
  interrupt.

  On latest master commit as the time of writing, there is an integer
  overflow in target/arm/helper.c gt_recalc_timer affecting the virtual
  timer when the interrupt is enabled in CTL:

      /* Next transition is when we hit cval */
      nexttick = gt->cval + offset;

  When this overflow happens, I notice that qemu is no longer responsive and that I have to SIGKILL the process:
      - qemu takes nearly all the cpu time of the cores it is running on (e.g. 50% cpu usage if running on half the cores) and is completely unresponsive
      - no guest interrupt (reported via -d int) is generated

  Here the minimal code example to reproduce the issue:

      mov     x0, #1
      msr     cntvoff_el2, x0
      mov     x0, #-1
      msr     cntv_cval_el0, x0
      mov     x0, #1
      msr     cntv_ctl_el0, x0 // interrupt generation enabled, not masked; qemu will start to hang here

  Options used:
  -nographic -machine virt,virtualization=on,gic-version=2,accel=tcg -cpu cortex-a57
  -smp 4 -m 1024 -kernel whatever.elf -d unimp,guest_errors,int -semihosting-config enable,target=native
  -serial mon:stdio

  Version used: 4.2

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1859021/+subscriptions

[Bug 1859021] Re: qemu-system-aarch64 (tcg): cval + voff overflow not handled, causes qemu to hang

[Thomas Huth]

This is an automated cleanup. This bug report has been moved
to QEMU's new bug tracker on gitlab.com and thus gets marked
as 'expired' now. Please continue with the discussion here:

 https://gitlab.com/qemu-project/qemu/-/issues/60


** Changed in: qemu
       Status: Confirmed => Expired

** Changed in: qemu
     Assignee: Alex Bennée (ajbennee) => (unassigned)

** Bug watch added: gitlab.com/qemu-project/qemu/-/issues #60
   https://gitlab.com/qemu-project/qemu/-/issues/60

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1859021

Title:
  qemu-system-aarch64 (tcg):  cval + voff overflow not handled, causes
  qemu to hang

Status in QEMU:
  Expired

Bug description:
  The Armv8 architecture reference manual states that for any timer set
  (e.g. CNTP* and CNTV*), the condition for such timer to generate an
  interrupt (if enabled & unmasked) is:

  CVAL <= CNT(P/V)CT

  Although this is arguably sloppy coding, I have seen code that is
  therefore assuming it can set CVAL to a very high value (e.g.
  UINT64_MAX) and leave the interrupt enabled in CTL, and never get the
  interrupt.

  On latest master commit as the time of writing, there is an integer
  overflow in target/arm/helper.c gt_recalc_timer affecting the virtual
  timer when the interrupt is enabled in CTL:

      /* Next transition is when we hit cval */
      nexttick = gt->cval + offset;

  When this overflow happens, I notice that qemu is no longer responsive and that I have to SIGKILL the process:
      - qemu takes nearly all the cpu time of the cores it is running on (e.g. 50% cpu usage if running on half the cores) and is completely unresponsive
      - no guest interrupt (reported via -d int) is generated

  Here the minimal code example to reproduce the issue:

      mov     x0, #1
      msr     cntvoff_el2, x0
      mov     x0, #-1
      msr     cntv_cval_el0, x0
      mov     x0, #1
      msr     cntv_ctl_el0, x0 // interrupt generation enabled, not masked; qemu will start to hang here

  Options used:
  -nographic -machine virt,virtualization=on,gic-version=2,accel=tcg -cpu cortex-a57
  -smp 4 -m 1024 -kernel whatever.elf -d unimp,guest_errors,int -semihosting-config enable,target=native
  -serial mon:stdio

  Version used: 4.2

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1859021/+subscriptions