In the absence of any objections, I'm putting this into
target-arm.next.
thanks
-- PMM
On Mon, 24 Feb 2025 at 14:52, Peter Maydell <peter.mayd...@linaro.org> wrote:
>
> This has been reviewed, but nobody's picked it up. Paolo,
> MAINTAINERS says qemu-timer.c is part of your "Main loop"
> responsibilities. Did you want to pick it up? Or I could
> throw it in to target-arm.next if you prefer.
>
> thanks
> -- PMM
>
> On Mon, 10 Feb 2025 at 13:58, Peter Maydell <peter.mayd...@linaro.org> wrote:
> >
> > Currently we call icount_start_warp_timer() from timerlist_rearm().
> > This produces incorrect behaviour, because timerlist_rearm() is
> > called, for instance, when a timer callback modifies its timer. We
> > cannot decide here to warp the timer forwards to the next timer
> > deadline merely because all_cpu_threads_idle() is true, because the
> > timer callback we were called from (or some other callback later in
> > the list of callbacks being invoked) may be about to raise a CPU
> > interrupt and move a CPU from idle to ready.5A
> >
> > The only valid place to choose to warp the timer forward is from the
> > main loop, when we know we have no outstanding IO or timer callbacks
> > that might be about to wake up a CPU.
> >
> > For Arm guests, this bug was mostly latent until the refactoring
> > commit f6fc36deef6abc ("target/arm/helper: Implement
> > CNTHCTL_EL2.CNT[VP]MASK"), which exposed it because it refactored a
> > timer callback so that it happened to call timer_mod() first and
> > raise the interrupt second, when it had previously raised the
> > interrupt first and called timer_mod() afterwards.
> >
> > This call seems to have originally derived from the
> > pre-record-and-replay icount code, which (as of e.g. commit
> > db1a49726c3c in 2010) in this location did a call to
> > qemu_notify_event(), necessary to get the icount code in the vCPU
> > round-robin thread to stop and recalculate the icount deadline when a
> > timer was reprogrammed from the IO thread. In current QEMU,
> > everything is done on the vCPU thread when we are in icount mode, so
> > there's no need to try to notify another thread here.
> >
> > I suspect that the other reason why this call was doing icount timer
> > warping is that it pre-dates commit efab87cf79077a from 2015, which
> > added a call to icount_start_warp_timer() to main_loop_wait(). Once
> > the call in timerlist_rearm() has been removed, if the timer
> > callbacks don't cause any CPU to be woken up then we will end up
> > calling icount_start_warp_timer() from main_loop_wait() when the rr
> > main loop code calls rr_wait_io_event().
> >
> > Remove the incorrect call from timerlist_rearm().
> >
> > Cc: qemu-sta...@nongnu.org
> > Signed-off-by: Peter Maydell <peter.mayd...@linaro.org>
> > ---
> > As far as I can tell, this is the right thing, and it fixes the
> > "icount warps the timer when it should not" bug I'm trying to
> > address, but I'm not super familiar with the icount subsystem or its
> > evolution, so it's possible I've accidentally broken some other setup
> > here. This does pass the tcg, functional and avocado tests,
> > including the record-and-replay ones. I've cc'd it to stable as a
> > bugfix, but it definitely merits careful review first.
> > ---
> > util/qemu-timer.c | 4 ----
> > 1 file changed, 4 deletions(-)
> >
> > diff --git a/util/qemu-timer.c b/util/qemu-timer.c
> > index 0e8a453eaa1..af6e1787e57 100644
> > --- a/util/qemu-timer.c
> > +++ b/util/qemu-timer.c
> > @@ -409,10 +409,6 @@ static bool timer_mod_ns_locked(QEMUTimerList
> > *timer_list,
> >
> > static void timerlist_rearm(QEMUTimerList *timer_list)
> > {
> > - /* Interrupt execution to force deadline recalculation. */
> > - if (icount_enabled() && timer_list->clock->type == QEMU_CLOCK_VIRTUAL)
> > {
> > - icount_start_warp_timer();
> > - }
> > timerlist_notify(timer_list);
> > }
> >
> > --
> > 2.34.1
