by Jia He

[permalink] [raw]

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

Hi Christoffer

Sorry for the late, I ever thought you would send out v2 with isb(). It
seems not.

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 <[email protected]>
> 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 <[email protected]>

Signed-off-by: Jia He <[email protected]>

> ---
> 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 <[email protected]>
> 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 <[email protected]>
> ---
> 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.

--
Cheers,
Jia

2017-12-21 11:35:19

by Christoffer Dall

[permalink] [raw]

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

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 <[email protected]>
> >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 <[email protected]>
>
> Signed-off-by: Jia He <[email protected]>
>

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 <[email protected]>
> >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 <[email protected]>
> >---
> > 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