Re: [PATCH] KVM: arm/arm64: don't set vtimer->cnt_ctl in kvm_arch_timer_handler

[Christoffer Dall]

On Thu, Dec 21, 2017 at 05:16:48PM +0800, Jia He wrote:
> 
> Sorry for the late, I ever thought you would send out v2 with isb().
> It seems not.
> 

I did:

https://lists.cs.columbia.edu/pipermail/kvmarm/2017-December/029078.html

> 
> On 12/15/2017 6:04 PM, Christoffer Dall Wrote:
> >On Fri, Dec 15, 2017 at 10:27:02AM +0800, Jia He wrote:
> >
> >[...]
> [...]
> >
> >Meanwhile, I think I thought of a cleaner way to do this.  Could you
> >test the following two patches on your platform as well?
> >
> >>From 3a594a3aa222bd64a86f6c6afcb209c9be20d5c5 Mon Sep 17 00:00:00 2001
> >From: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
> >Date: Thu, 14 Dec 2017 19:54:50 +0100
> >Subject: [PATCH 1/2] KVM: arm/arm64: Properly handle arch-timer IRQs after
> >  vtimer_save_state
> >
> >The recent timer rework was assuming that once the timer was disabled,
> >we should no longer see any interrupts from the timer.  This assumption
> >turns out to not be true, and instead we have to handle the case when
> >the timer ISR runs even after the timer has been disabled.
> >
> >This requires a couple of changes:
> >
> >First, we should never overwrite the cached guest state of the timer
> >control register when the ISR runs, because KVM may have disabled its
> >timers when doing vcpu_put(), even though the guest still had the timer
> >enabled.
> >
> >Second, we shouldn't assume that the timer is actually firing just
> >because we see an ISR, but we should check the ISTATUS field of the
> >timer control register to understand if the hardware timer is really
> >firing or not.
> >
> >Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
> 
> Signed-off-by: Jia He <jia.he@xxxxxxxxxxxxxxxx>
> 

Did you write parts of this patch and thus I should have had your
signed-off-by ?

Or did you mean to provide another tag.

Anyway, these patches have been pulled already, so I hope we can live
with the way they are.

> >---
> >  virt/kvm/arm/arch_timer.c | 19 ++++++++++++-------
> >  1 file changed, 12 insertions(+), 7 deletions(-)
> >
> >diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> >index aa9adfafe12b..792bcf6277b6 100644
> >--- a/virt/kvm/arm/arch_timer.c
> >+++ b/virt/kvm/arm/arch_timer.c
> >@@ -92,16 +92,21 @@ static irqreturn_t kvm_arch_timer_handler(int irq, void *dev_id)
> >  {
> >  	struct kvm_vcpu *vcpu = *(struct kvm_vcpu **)dev_id;
> >  	struct arch_timer_context *vtimer;
> >+	u32 cnt_ctl;
> >-	if (!vcpu) {
> >-		pr_warn_once("Spurious arch timer IRQ on non-VCPU thread\n");
> >-		return IRQ_NONE;
> >-	}
> >-	vtimer = vcpu_vtimer(vcpu);
> >+	/*
> >+	 * We may see a timer interrupt after vcpu_put() has been called which
> >+	 * sets the CPU's vcpu pointer to NULL, because even though the timer
> >+	 * has been disabled in vtimer_save_state(), the singal may not have
> >+	 * been retired from the interrupt controller yet.
> >+	 */
> >+	if (!vcpu)
> >+		return IRQ_HANDLED;
> >+	vtimer = vcpu_vtimer(vcpu);
> >  	if (!vtimer->irq.level) {
> >-		vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl);
> >-		if (kvm_timer_irq_can_fire(vtimer))
> >+		cnt_ctl = read_sysreg_el0(cntv_ctl);
> >+		if (cnt_ctl & ARCH_TIMER_CTRL_IT_STAT)
> >  			kvm_timer_update_irq(vcpu, true, vtimer);
> >  	}
> >
> >
> >>From ed96302b47d209024814df116994f64dc8f07c96 Mon Sep 17 00:00:00 2001
> >From: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
> >Date: Fri, 15 Dec 2017 00:30:12 +0100
> >Subject: [PATCH 2/2] KVM: arm/arm64: Fix timer enable flow
> >
> >When enabling the timer on the first run, we fail to ever restore the
> >state and mark it as loaded.  That means, that in the initial entry to
> >the VCPU ioctl, unless we exit to userspace for some reason such as a
> >pending signal, if the guest programs a timer and blocks, we will wait
> >forever, because we never read back the hardware state (the loaded flag
> >is not set), and so we think the timer is disabled, and we never
> >schedule a background soft timer.
> >
> >The end result?  The VCPU blocks forever, and the only solution is to
> >kill the thread.
> >
> >Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
> >---
> >  virt/kvm/arm/arch_timer.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> >
> >diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> >index 792bcf6277b6..8869658e6983 100644
> >--- a/virt/kvm/arm/arch_timer.c
> >+++ b/virt/kvm/arm/arch_timer.c
> >@@ -843,10 +843,7 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
> >  no_vgic:
> >  	preempt_disable();
> >  	timer->enabled = 1;
> >-	if (!irqchip_in_kernel(vcpu->kvm))
> >-		kvm_timer_vcpu_load_user(vcpu);
> >-	else
> >-		kvm_timer_vcpu_load_vgic(vcpu);
> >+	kvm_timer_vcpu_load(vcpu);
> >  	preempt_enable();
> >  	return 0;
> >
> >
> >Thanks,
> >-Christoffer
> >
> I have tested your 2 patches in my QDF2400 server for 10 times, the
> guest can be boot up without any issues.
> Without this patch, the guest will always hang in booting stages.
> 
Thanks for this, it's comforting to know.

Thanks,
-Christoffer