Re: [PATCH 1/3] KVM: arm64: nv: only emulate timers that have not yet fired

[Marc Zyngier <maz@kernel.org>]

On Mon, 09 Jan 2023 12:25:13 +0000,
Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> 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@os.amperecomputing.com> wrote:
> >> 
> >> From: D Scott Phillips <scott@os.amperecomputing.com>
> >> 
> >> 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@os.amperecomputing.com>
> >> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
> >> ---
> >>   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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel