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@xxxxxxxxxxxxxxx> # 4.15+
> Fixes: bbdd52cfcba29 ("KVM: arm/arm64: Avoid phys timer emulation in vcpu entry/exit")
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> ---
> 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
