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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 09-01-2023 07:14 pm, Marc Zyngier wrote:
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).

OK, thanks.

Please give it a go if you have a chance (though the whole branch
might be of interest to you...).

Sure, I will try this branch.

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


Thanks,
Ganapat



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux