[PATCH v2] kvm: arm/arm64: Fix emulated physical timer IRQ injection

[PATCH v2] kvm: arm/arm64: Fix emulated physical timer IRQ injection

[Andre Przywara]

When KVM emulates a physical timer, we keep track of the interrupt
condition and try to inject an IRQ to the guest when needed.
This works if the timer expires when either the guest is running or KVM
does work on behalf of it (like handling a trap).
However when the guest's VCPU is not scheduled (for instance because
the guest issued a WFI instruction before), we miss injecting the interrupt
when the VCPU's state gets restored back in kvm_timer_vcpu_load().

Fix this by moving the interrupt injection check into the
phys_timer_emulate() function, so that all possible paths of execution
are covered.

Cc: Stable <stable@vger.kernel.org> # 4.15+
Fixes: bbdd52cfcba29 ("KVM: arm/arm64: Avoid phys timer emulation in vcpu entry/exit")
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
Changelog v1...v2:
- clear IRQ line *before* starting the soft timer

 virt/kvm/arm/arch_timer.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index bd3d57f40f1b..03a4ea776b85 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -294,16 +294,25 @@ static void phys_timer_emulate(struct kvm_vcpu *vcpu)
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
 	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
 
+	/* If the timer cannot fire at all, then we don't need a soft timer. */
+	if (!kvm_timer_irq_can_fire(ptimer)) {
+		soft_timer_cancel(&timer->phys_timer, NULL);
+		kvm_timer_update_irq(vcpu, false, ptimer);
+		return;
+	}
+
 	/*
-	 * If the timer can fire now we have just raised the IRQ line and we
-	 * don't need to have a soft timer scheduled for the future.  If the
-	 * timer cannot fire at all, then we also don't need a soft timer.
+	 * If the timer can fire now, we don't need to have a soft timer
+	 * scheduled for the future, as we also raise the IRQ line.
 	 */
-	if (kvm_timer_should_fire(ptimer) || !kvm_timer_irq_can_fire(ptimer)) {
+	if (kvm_timer_should_fire(ptimer)) {
 		soft_timer_cancel(&timer->phys_timer, NULL);
+		kvm_timer_update_irq(vcpu, true, ptimer);
+
 		return;
 	}
 
+	kvm_timer_update_irq(vcpu, false, ptimer);
 	soft_timer_start(&timer->phys_timer, kvm_timer_compute_delta(ptimer));
 }
 
@@ -316,7 +325,6 @@ static void kvm_timer_update_state(struct kvm_vcpu *vcpu)
 {
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
 	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
-	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
 	bool level;
 
 	if (unlikely(!timer->enabled))
@@ -332,9 +340,6 @@ static void kvm_timer_update_state(struct kvm_vcpu *vcpu)
 	level = kvm_timer_should_fire(vtimer);
 	kvm_timer_update_irq(vcpu, level, vtimer);
 
-	if (kvm_timer_should_fire(ptimer) != ptimer->irq.level)
-		kvm_timer_update_irq(vcpu, !ptimer->irq.level, ptimer);
-
 	phys_timer_emulate(vcpu);
 }
 
-- 
2.14.4

[PATCH v2] kvm: arm/arm64: Fix emulated physical timer IRQ injection

[Christoffer Dall]

On Tue, Jul 17, 2018 at 10:51:37AM +0100, Andre Przywara wrote:
> When KVM emulates a physical timer, we keep track of the interrupt
> condition and try to inject an IRQ to the guest when needed.
> This works if the timer expires when either the guest is running or KVM
> does work on behalf of it (like handling a trap).
> However when the guest's VCPU is not scheduled (for instance because
> the guest issued a WFI instruction before), we miss injecting the interrupt
> when the VCPU's state gets restored back in kvm_timer_vcpu_load().
> 
> Fix this by moving the interrupt injection check into the
> phys_timer_emulate() function, so that all possible paths of execution
> are covered.
> 
> Cc: Stable <stable@vger.kernel.org> # 4.15+
> Fixes: bbdd52cfcba29 ("KVM: arm/arm64: Avoid phys timer emulation in vcpu entry/exit")
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
> Changelog v1...v2:
> - clear IRQ line *before* starting the soft timer
> 
>  virt/kvm/arm/arch_timer.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index bd3d57f40f1b..03a4ea776b85 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -294,16 +294,25 @@ static void phys_timer_emulate(struct kvm_vcpu *vcpu)

Please update the comment on this function as well.

>  	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
>  	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
>  
> +	/* If the timer cannot fire at all, then we don't need a soft timer. */
> +	if (!kvm_timer_irq_can_fire(ptimer)) {
> +		soft_timer_cancel(&timer->phys_timer, NULL);
> +		kvm_timer_update_irq(vcpu, false, ptimer);
> +		return;
> +	}
> +
>  	/*
> -	 * If the timer can fire now we have just raised the IRQ line and we
> -	 * don't need to have a soft timer scheduled for the future.  If the
> -	 * timer cannot fire at all, then we also don't need a soft timer.
> +	 * If the timer can fire now, we don't need to have a soft timer
> +	 * scheduled for the future, as we also raise the IRQ line.
>  	 */
> -	if (kvm_timer_should_fire(ptimer) || !kvm_timer_irq_can_fire(ptimer)) {
> +	if (kvm_timer_should_fire(ptimer)) {
>  		soft_timer_cancel(&timer->phys_timer, NULL);
> +		kvm_timer_update_irq(vcpu, true, ptimer);
> +
>  		return;
>  	}
>  
> +	kvm_timer_update_irq(vcpu, false, ptimer);
>  	soft_timer_start(&timer->phys_timer, kvm_timer_compute_delta(ptimer));

I find this overly complex.

How about:

diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index bd3d57f..b5dcfca 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -288,7 +288,7 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
 	}
 }
 
-/* Schedule the background timer for the emulated timer. */
+/* Emulate the physical timer in software and update IRQ signal if needed */
 static void phys_timer_emulate(struct kvm_vcpu *vcpu)
 {
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
@@ -299,12 +299,13 @@ static void phys_timer_emulate(struct kvm_vcpu *vcpu)
 	 * don't need to have a soft timer scheduled for the future.  If the
 	 * timer cannot fire at all, then we also don't need a soft timer.
 	 */
-	if (kvm_timer_should_fire(ptimer) || !kvm_timer_irq_can_fire(ptimer)) {
+	if (kvm_timer_should_fire(ptimer) || !kvm_timer_irq_can_fire(ptimer))
 		soft_timer_cancel(&timer->phys_timer, NULL);
-		return;
-	}
+	else
+		soft_timer_start(&timer->phys_timer, kvm_timer_compute_delta(ptimer));
 
-	soft_timer_start(&timer->phys_timer, kvm_timer_compute_delta(ptimer));
+	if (kvm_timer_should_fire(ptimer) != ptimer->irq.level)
+		kvm_timer_update_irq(vcpu, !ptimer->irq.level, ptimer);
 }
 
 /*
 

>  }
>  
> @@ -316,7 +325,6 @@ static void kvm_timer_update_state(struct kvm_vcpu *vcpu)
>  {
>  	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
>  	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
> -	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
>  	bool level;
>  
>  	if (unlikely(!timer->enabled))
> @@ -332,9 +340,6 @@ static void kvm_timer_update_state(struct kvm_vcpu *vcpu)
>  	level = kvm_timer_should_fire(vtimer);
>  	kvm_timer_update_irq(vcpu, level, vtimer);
>  
> -	if (kvm_timer_should_fire(ptimer) != ptimer->irq.level)
> -		kvm_timer_update_irq(vcpu, !ptimer->irq.level, ptimer);
> -
>  	phys_timer_emulate(vcpu);
>  }
>  
Please also remove the comment in kvm_timer_vcpu_load() which indicates
that the call to the function will only ever schedule a background
timer.

Thanks,
-Christoffer