If gt_timer is enabled before cval initialization on a virtualized
setup on QEMU, cval equals (UINT64_MAX - 1). Adding an offset value
to this causes an overflow that sets timer into the past, which leads
to infinite loop, because this timer fires immediately and calls
gt_recalc_timer() once more, which in turn sets the timer into the
past again and as a result, QEMU hangs. This patch adds check for
overflowing of the nexttick variable.

Suggested-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
Co-Authored-By: Dmytro Firsov <dmytro_firsov@epam.com>
Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@epam.com>
---
 target/arm/helper.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 3b22596eab..b4aaa2965b 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2665,6 +2665,16 @@ 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) {
+                /*
+                 * If gt->cval value is close to UINT64_MAX then adding
+                 * to it offset can lead to overflow of nexttick variable.
+                 * So, this check tests that arguments sum is less than any
+                 * addend, and in case it is overflowed we have to mod timer
+                 * to INT64_MAX.
+                 */
+                nexttick = UINT64_MAX;
+            }
         }
         /*
          * Note that the desired next expiry time might be beyond the
-- 
2.25.1

Peter Maydell, thank you for your comments.
I apologize for so late response - returned to this issue and now I will 
answer faster. I fixed the commit according to your recommendations, 
please take a look at the new version.

 > There is also a comment or two from me in the bug report pointing
 > out that the handling of wraparound is also wrong in the other
 > half of this if(); we should look at that too.

I read this topic and as I understand changing the other half of "if" is 
not related to the reported issue. Since it affects running virtualized 
setups on arm64 QEMU, e.g. Zephyr 
(https://github.com/zephyrproject-rtos/zephyr/blob/main//boards/arm64/xenvm/doc/index.rst) 
maybe is it worth merging at least this change?

Best regards,
Leonid Komarianskyi.

On 09.11.23 15:55, Leonid Komarianskyi wrote:
> If gt_timer is enabled before cval initialization on a virtualized
> setup on QEMU, cval equals (UINT64_MAX - 1). Adding an offset value
> to this causes an overflow that sets timer into the past, which leads
> to infinite loop, because this timer fires immediately and calls
> gt_recalc_timer() once more, which in turn sets the timer into the
> past again and as a result, QEMU hangs. This patch adds check for
> overflowing of the nexttick variable.
>
> Suggested-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> Co-Authored-By: Dmytro Firsov <dmytro_firsov@epam.com>
> Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@epam.com>
> ---
>   target/arm/helper.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 3b22596eab..b4aaa2965b 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -2665,6 +2665,16 @@ 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) {
> +                /*
> +                 * If gt->cval value is close to UINT64_MAX then adding
> +                 * to it offset can lead to overflow of nexttick variable.
> +                 * So, this check tests that arguments sum is less than any
> +                 * addend, and in case it is overflowed we have to mod timer
> +                 * to INT64_MAX.
> +                 */
> +                nexttick = UINT64_MAX;
> +            }
>           }
>           /*
>            * Note that the desired next expiry time might be beyond the

On Thu, 9 Nov 2023 at 15:14, Leonid Komarianskyi
<Leonid_Komarianskyi@epam.com> wrote:
>
> Peter Maydell, thank you for your comments.
> I apologize for so late response - returned to this issue and now I will
> answer faster. I fixed the commit according to your recommendations,
> please take a look at the new version.
>
>  > There is also a comment or two from me in the bug report pointing
>  > out that the handling of wraparound is also wrong in the other
>  > half of this if(); we should look at that too.
>
> I read this topic and as I understand changing the other half of "if" is
> not related to the reported issue. Since it affects running virtualized
> setups on arm64 QEMU, e.g. Zephyr
> (https://github.com/zephyrproject-rtos/zephyr/blob/main//boards/arm64/xenvm/doc/index.rst)
> maybe is it worth merging at least this change?

I feel they're really pretty much the same thing -- when we
added support for the timer offset registers we didn't correctly
update the arithmetic that calculates when the next interrupt
line transition happens.

I've just posted my version of a patch that I think should
fix both halves of the if():

https://patchew.org/QEMU/20231120173506.3729884-1-peter.maydell@linaro.org/

Thanks for prodding me into looking at this issue again -- I had
somehow got the mistaken impression that it only happened in
some weird icount sleep=off situations.

thanks
-- PMM