On Thu, 6 Apr 2023 at 16:16, Leonid Komarianskyi <leonid_komarians...@epam.com> 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.
This is https://gitlab.com/qemu-project/qemu/-/issues/60 -- thanks for sending a patch. > Suggested-by: Volodymyr Babchuk <volodymyr_babc...@epam.com> > Co-Authored-By: Dmytro Firsov <dmytro_fir...@epam.com> > Signed-off-by: Leonid Komarianskyi <leonid_komarians...@epam.com> > --- > target/arm/helper.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/target/arm/helper.c b/target/arm/helper.c > index 2297626bfb..2fbba15040 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -2618,6 +2618,7 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx) > int istatus = count - offset >= gt->cval; > uint64_t nexttick; > int irqstate; > + bool nexttick_overflow = false; > > gt->ctl = deposit32(gt->ctl, 2, 1, istatus); > > @@ -2630,6 +2631,16 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx) > } else { > /* Next transition is when we hit cval */ > nexttick = gt->cval + offset; > + if (nexttick < offset) { > + /* > + * 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_overflow = true; > + } Rather than adding in a bool, I think I prefer the version of the patch in one of the comments to the bug report: /* Next transition is when we hit cval */ nexttick = gt->cval + offset; + if (nexttick < gt->cval) { + nexttick = UINT64_MAX; + } i.e. we just saturate nexttick, and then let the existing handling of "turns out nexttick is too big" handle things. 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. > } > /* > * Note that the desired next expiry time might be beyond the > @@ -2637,7 +2648,8 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx) > * set the timer for as far in the future as possible. When the > * timer expires we will reset the timer for any remaining period. > */ > - if (nexttick > INT64_MAX / gt_cntfrq_period_ns(cpu)) { > + if ((nexttick > INT64_MAX / gt_cntfrq_period_ns(cpu)) > + || nexttick_overflow) { > timer_mod_ns(cpu->gt_timer[timeridx], INT64_MAX); > } else { > timer_mod(cpu->gt_timer[timeridx], nexttick); > -- > 2.25.1 thanks -- PMM
