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

[Richard Henderson <richard.henderson@linaro.org>]

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~