Message-ID: <867c89tc4q.wl-maz@kernel.org>
Date: Mon, 09 Dec 2024 09:54:29 +0000
From: Marc Zyngier <maz@...nel.org>
To: Ganapatrao Kulkarni <gankulkarni@...amperecomputing.com>
Cc: kvmarm <kvmarm@...ts.linux.dev>,
	linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org,
	oliver.upton@...ux.dev,
	christoffer.dall@....com,
	suzuki.poulose@....com,
	will@...nel.org,
	catalin.marinas@....com,
	coltonlewis@...gle.com,
	joey.gouly@....com,
	yuzenghui@...wei.com,
	darren@...amperecomputing.com,
	vishnu@...amperecomputing.com
Subject: Re: [PATCH] KVM: arm64: nv: Set ISTATUS for emulated timers, If timer expired

Ganapatrao,

Did you notice that the Columbia list was killed over two years ago,
as per ac107abef1976 ("KVM: arm64: Advertise new kvmarm mailing
list")? Consider that script/get_maintainer.pl is your friend.

Cc'ing the correct list instead,

On Mon, 09 Dec 2024 05:32:01 +0000,
Ganapatrao Kulkarni <gankulkarni@...amperecomputing.com> wrote:
> 
> During automated testing of Nested Virtualization using avocado-vt,

Which is not merged upstream. So what branch are you using? Based on
what kernel version? On what HW? With which virtualisation features?

> it has been observed that during some boot test iterations,
> the Guest-Hypervisor boot was getting crashed with a
> synchronous exception while it is still booting EDK2.
> 
> The test is launching Multiple instances of Guest-Hypervisor boot

Is the multiple instance aspect really relevant to the reproduction of
the problem?

> and while booting, QEMU monitor issued the command "info register"
> at regular intervals to take a register dump. To execute this
> command, QEMU stops the run and does the register read of various
> registers. While resuming the run, the function kvm_arm_timer_write()
> writes back the saved CNTV_CTL_EL0 register with ISTATUS cleared always

It is userspace that causes this write-back, right? AFAICT, KVM never
does that on its own.

> and resulting in the loss of pending interrupt for emulated timers.

How does a missing interrupt result in a synchronous exception in
EDK2? In my experience, EDK2 panics if it sees spurious interrupts,
not when it is missing interrupts (it just locks up, which is
expected).

> In hardware based timers, ISTATUS is a RO/WI bit and gets set by the
> h/w, if the condition is still met. However, in Nested-Virtualization
> case, the Guest-Hypervisor's EDK2 is using an emulated virtual timer
> and losing ISTATUS state and the interrupt forever.

Why is this specific to NV? Can't the same thing happen to the
physical timer in a non-VHE configuration?

> 
> Adding fix in kvm_arm_timer_write to set ISTATUS for emulated
> timers, if the timer expired already.
> 
> Fixes: 81dc9504a700 ("KVM: arm64: nv: timers: Support hyp timer emulation")

Where is this coming from? This patch doesn't touch the code you are
changing, so how can it be the source of your problems? As far as I
can tell, this has been the case since 5c5196da4e966 ("KVM: arm/arm64:
Support EL1 phys timer register access in set/get reg").

> Co-developed-by: Vishnu Pajjuri <vishnu@...amperecomputing.com>
> Signed-off-by: Vishnu Pajjuri <vishnu@...amperecomputing.com>
> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@...amperecomputing.com>
> ---
>  arch/arm64/kvm/arch_timer.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index 1215df590418..aca58113d790 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -1199,7 +1199,16 @@ static void kvm_arm_timer_write(struct kvm_vcpu *vcpu,
>  		break;
>  
>  	case TIMER_REG_CTL:
> -		timer_set_ctl(timer, val & ~ARCH_TIMER_CTRL_IT_STAT);
> +		struct timer_map map;
> +
> +		val &= ~ARCH_TIMER_CTRL_IT_STAT;
> +		get_timer_map(vcpu, &map);
> +		/* Set ISTATUS bit for emulated timers, if timer expired. */
> +		if (timer == map.emul_vtimer || timer == map.emul_ptimer) {
> +			if (!kvm_timer_compute_delta(timer))
> +				val |= ARCH_TIMER_CTRL_IT_STAT;
> +		}
> +		timer_set_ctl(timer, val);
>  		break;

This really looks awfully complicated, when it is only a matter of
recomputing the interrupt state, something that is at the core of the
timer emulation. Why can't the following work:

diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
index 895f09658ef83..bd6efafbe7109 100644
--- a/arch/arm64/kvm/arch_timer.c
+++ b/arch/arm64/kvm/arch_timer.c
@@ -1298,13 +1298,17 @@ static void kvm_arm_timer_write(struct kvm_vcpu *vcpu,
 				enum kvm_arch_timer_regs treg,
 				u64 val)
 {
+	unsigned long tmp = val;
+
 	switch (treg) {
 	case TIMER_REG_TVAL:
 		timer_set_cval(timer, kvm_phys_timer_read() - timer_get_offset(timer) + (s32)val);
 		break;
 
 	case TIMER_REG_CTL:
-		timer_set_ctl(timer, val & ~ARCH_TIMER_CTRL_IT_STAT);
+		__assign_bit(__ffs(ARCH_TIMER_CTRL_IT_STAT), &tmp,
+			     kvm_timer_should_fire(timer));
+		timer_set_ctl(timer, tmp);
 		break;
 
 	case TIMER_REG_CVAL:

But overall, this very much looks like it is only papering over the
real issue, which is that the *emulation* should regenerate the
pending bit, and not rely on the userspace interface.

As far as I can tell, we already correctly compute the status bit on
read (read_timer_ctl()), so the guest should always observe something
consistent when it traps. We also *never* use the status bit as an
input to the emulation, and always recompute it from scratch (it is
only there for the benefit of the guest or userspace).

So I can't see how upstream is broken in at the moment, and you need
to explain how this actually triggers. Ideally, with a standalone
reproducer or a selftest.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Powered by blists - more mailing lists