[PATCH 0/2] KVM: arm/arm64: Fix two problems with the arch timer introduced in v4.15-rc1

[PATCH 0/2] KVM: arm/arm64: Fix two problems with the arch timer introduced in v4.15-rc1

[Christoffer Dall]

The first patch solves the problem with the spurious IRQ (which is not a
problem at all and just an artifact of what Jia is writing about), and
it solves the issue of accidentally overwriting the in-memory copy of
the guest state with a disabled timer, leaving the VCPU in the weeds.

The second patch addresses addresses an issue identified when booting
with kvmtool  The reason why we didn't see it with QEMU is that QEMU is
so happy to signal its threads in the initial setup phase, that it hides
the bug.

Jia, I'd appreciate your tested-by after having applied both patches on
your platform.

Thanks,
-Christoffer

Christoffer Dall (2):
  KVM: arm/arm64: Properly handle arch-timer IRQs after
    vtimer_save_state
  KVM: arm/arm64: Fix timer enable flow

 virt/kvm/arm/arch_timer.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

-- 
2.14.2

[PATCH 1/2] KVM: arm/arm64: Properly handle arch-timer IRQs after vtimer_save_state

[Christoffer Dall]

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 interrupt, but we should check the actual state of the
timer in the timer control register to understand if the hardware timer
is really firing or not.

We also add an ISB to vtimer_save_state() to ensure the timer is
actually disabled once we enable interrupts, which should clarify the
intention of the implementation, and reduce the risk of unwanted
interrupts.

Fixes: b103cc3f10c0 ("KVM: arm/arm64: Avoid timer save/restore in vcpu entry/exit")
Reported-by: Marc Zyngier <marc.zyngier@arm.com>
Reported-by: Jia He <hejianet@gmail.com>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 virt/kvm/arm/arch_timer.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index aa9adfafe12b..14c018f990a7 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -92,16 +92,23 @@ 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 hardware interrupt
+	 * signal 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);
+		cnt_ctl &= ARCH_TIMER_CTRL_ENABLE | ARCH_TIMER_CTRL_IT_STAT |
+			   ARCH_TIMER_CTRL_IT_MASK;
+		if (cnt_ctl == (ARCH_TIMER_CTRL_ENABLE | ARCH_TIMER_CTRL_IT_STAT))
 			kvm_timer_update_irq(vcpu, true, vtimer);
 	}
 
@@ -355,6 +362,7 @@ static void vtimer_save_state(struct kvm_vcpu *vcpu)
 
 	/* Disable the virtual timer */
 	write_sysreg_el0(0, cntv_ctl);
+	isb();
 
 	vtimer->loaded = false;
 out:
-- 
2.14.2

[PATCH 1/2] KVM: arm/arm64: Properly handle arch-timer IRQs after vtimer_save_state

[Marc Zyngier]

On Fri, 15 Dec 2017 14:16:55 +0000,
Christoffer Dall wrote:
> 
> 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 interrupt, but we should check the actual state of the
> timer in the timer control register to understand if the hardware timer
> is really firing or not.
> 
> We also add an ISB to vtimer_save_state() to ensure the timer is
> actually disabled once we enable interrupts, which should clarify the
> intention of the implementation, and reduce the risk of unwanted
> interrupts.
> 
> Fixes: b103cc3f10c0 ("KVM: arm/arm64: Avoid timer save/restore in vcpu entry/exit")
> Reported-by: Marc Zyngier <marc.zyngier@arm.com>
> Reported-by: Jia He <hejianet@gmail.com>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  virt/kvm/arm/arch_timer.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index aa9adfafe12b..14c018f990a7 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -92,16 +92,23 @@ 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 hardware interrupt
> +	 * signal 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);
> +		cnt_ctl &= ARCH_TIMER_CTRL_ENABLE | ARCH_TIMER_CTRL_IT_STAT |
> +			   ARCH_TIMER_CTRL_IT_MASK;
> +		if (cnt_ctl == (ARCH_TIMER_CTRL_ENABLE | ARCH_TIMER_CTRL_IT_STAT))
>  			kvm_timer_update_irq(vcpu, true, vtimer);
>  	}
>  
> @@ -355,6 +362,7 @@ static void vtimer_save_state(struct kvm_vcpu *vcpu)
>  
>  	/* Disable the virtual timer */
>  	write_sysreg_el0(0, cntv_ctl);
> +	isb();
>  
>  	vtimer->loaded = false;
>  out:
> -- 
> 2.14.2
> 

Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
Tested-by: Marc Zyngier <marc.zyngier@arm.com>

	M.

[PATCH 2/2] KVM: arm/arm64: Fix timer enable flow

[Christoffer Dall]

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.

Fixes: 4a2c4da1250d ("arm/arm64: KVM: Load the timer state when enabling the timer")
Reported-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 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 14c018f990a7..cc29a8148328 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -846,10 +846,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;
-- 
2.14.2

[PATCH 2/2] KVM: arm/arm64: Fix timer enable flow

[Marc Zyngier]

On Fri, 15 Dec 2017 14:16:56 +0000,
Christoffer Dall wrote:
> 
> 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.
> 
> Fixes: 4a2c4da1250d ("arm/arm64: KVM: Load the timer state when enabling the timer")
> Reported-by: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  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 14c018f990a7..cc29a8148328 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -846,10 +846,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;
> -- 
> 2.14.2
> 

Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
Tested-by: Marc Zyngier <marc.zyngier@arm.com>

	M.