On Mon, 09 Jan 2023 12:25:13 +0000,
Ganapatrao Kulkarni <gankulkarni@xxxxxxxxxxxxxxxxxxxxxx> wrote:
>
>
> Hi Marc,
>
> On 29-12-2022 06:30 pm, Marc Zyngier wrote:
> > On Wed, 24 Aug 2022 07:03:02 +0100,
> > Ganapatrao Kulkarni <gankulkarni@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> >>
> >> From: D Scott Phillips <scott@xxxxxxxxxxxxxxxxxxxxxx>
> >>
> >> The timer emulation logic goes into an infinite loop when the NestedVM(L2)
> >> timer is being emulated.
> >>
> >> While the CPU is executing in L1 context, the L2 timers are emulated using
> >> host hrtimer. When the delta of cval and current time reaches zero, the
> >> vtimer interrupt is fired/forwarded to L2, however the emulation function
> >> in Host-Hypervisor(L0) is still restarting the hrtimer with an expiry time
> >> set to now, triggering hrtimer to fire immediately and resulting in a
> >> continuous trigger of hrtimer and endless looping in the timer emulation.
> >>
> >> Adding a fix to avoid restarting of the hrtimer if the interrupt is
> >> already fired.
> >>
> >> Signed-off-by: D Scott Phillips <scott@xxxxxxxxxxxxxxxxxxxxxx>
> >> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@xxxxxxxxxxxxxxxxxxxxxx>
> >> ---
> >> arch/arm64/kvm/arch_timer.c | 3 ++-
> >> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> >> index 2371796b1ab5..27a6ec46803a 100644
> >> --- a/arch/arm64/kvm/arch_timer.c
> >> +++ b/arch/arm64/kvm/arch_timer.c
> >> @@ -472,7 +472,8 @@ static void timer_emulate(struct arch_timer_context *ctx)
> >> return;
> >> }
> >> - soft_timer_start(&ctx->hrtimer, kvm_timer_compute_delta(ctx));
> >> + if (!ctx->irq.level)
> >> + soft_timer_start(&ctx->hrtimer, kvm_timer_compute_delta(ctx));
> >> }
> >> static void timer_save_state(struct arch_timer_context *ctx)
> >
> > I think this is a regression introduced by bee038a67487 ("KVM:
> > arm/arm64: Rework the timer code to use a timer_map"), and you can see
> > it because the comment in this function doesn't make much sense
> > anymore.
>
> OK, check was removed while rework in bee038a67487.
> >
> > Does the following work for you, mostly restoring the original code?
>
> Below diff too works and avoids the unnecessary soft timer restarts
> with zero delta.
> >
> > Thanks,
> >
> > M.
> >
> > diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> > index ad2a5df88810..4945c5b96f05 100644
> > --- a/arch/arm64/kvm/arch_timer.c
> > +++ b/arch/arm64/kvm/arch_timer.c
> > @@ -480,7 +480,7 @@ static void timer_emulate(struct arch_timer_context *ctx)
> > * scheduled for the future. If the timer cannot fire at all,
> > * then we also don't need a soft timer.
> > */
> > - if (!kvm_timer_irq_can_fire(ctx)) {
> > + if (should_fire || !kvm_timer_irq_can_fire(ctx)) {
>
> Now, aligns to comment.
> > soft_timer_cancel(&ctx->hrtimer);
> > return;
> > }
> >
>
> Shall I resend this patch as regression fix of bee038a67487?
I already have a patch written for this at [1], also getting rid of
the soft_timer_cancel() call in the process (as it doesn't make much
sense either).
Please give it a go if you have a chance (though the whole branch
might be of interest to you...).
Thanks,
M.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/commit/?h=kvm-arm64/nv-6.2-WIP&id=effdcfa175c374a1740f60642d221ad2e930c978
--
Without deviation from the norm, progress is not possible.
