On Wed, Apr 06, 2016 at 09:37:22AM +0100, Marc Zyngier wrote:
> On a host that runs NTP, corrections can have a direct impact on
> the background timer that we program on the behalf of a vcpu.
>
> In particular, NTP performing a forward correction will result in
> a timer expiring sooner than expected from a guest point of view.
> Not a big deal, we kick the vcpu anyway.
>
> But on wake-up, the vcpu thread is going to perform a check to
> find out whether or not it should block. And at that point, the
> timer check is going to say "timer has not expired yet, go back
> to sleep". This results in the timer event being lost forever.
>
> There are multiple ways to handle this. One would be record that
> the timer has expired and let kvm_cpu_has_pending_timer return
> true in that case, but that would be fairly invasive. Another is
> to check for the "short sleep" condition in the hrtimer callback,
> and restart the timer for the remaining time when the condition
> is detected.
>
> This patch implements the latter, with a bit of refactoring in
> order to avoid too much code duplication.
>
> Reported-by: Alexander Graf <[email protected]>
> Signed-off-by: Marc Zyngier <[email protected]>
> ---
> virt/kvm/arm/arch_timer.c | 47
> +++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 37 insertions(+), 10 deletions(-)
>
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index a9ad4fe..4d0e77a 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -98,10 +98,46 @@ static void kvm_timer_inject_irq_work(struct work_struct
> *work)
> kvm_vcpu_kick(vcpu);
> }
>
> +static u64 kvm_timer_compute_delta(struct kvm_vcpu *vcpu)
> +{
> + cycle_t cval, now;
> +
> + cval = vcpu->arch.timer_cpu.cntv_cval;
> + now = kvm_phys_timer_read() - vcpu->kvm->arch.timer.cntvoff;
> +
> + if (now < cval) {
> + u64 ns;
> +
> + ns = cyclecounter_cyc2ns(timecounter->cc,
> + cval - now,
> + timecounter->mask,
> + &timecounter->frac);
> + return ns;
> + }
> +
> + return 0;
> +}
> +
> static enum hrtimer_restart kvm_timer_expire(struct hrtimer *hrt)
> {
> struct arch_timer_cpu *timer;
> + struct kvm_vcpu *vcpu;
> + u64 ns;
> +
> timer = container_of(hrt, struct arch_timer_cpu, timer);
> + vcpu = container_of(timer, struct kvm_vcpu, arch.timer_cpu);
> +
> + /*
> + * Check that the timer has really expired from the guest's
> + * PoV (NTP on the host may have forced it to expire
> + * early). If we should have slept longer, restart it.
> + */
> + ns = kvm_timer_compute_delta(vcpu);
> + if (unlikely(ns)) {
> + hrtimer_forward_now(hrt, ns_to_ktime(ns));
> + return HRTIMER_RESTART;
> + }
> +
> queue_work(wqueue, &timer->expired);
> return HRTIMER_NORESTART;
> }
> @@ -176,8 +212,6 @@ static int kvm_timer_update_state(struct kvm_vcpu *vcpu)
> void kvm_timer_schedule(struct kvm_vcpu *vcpu)
> {
> struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> - u64 ns;
> - cycle_t cval, now;
>
> BUG_ON(timer_is_armed(timer));
>
> @@ -197,14 +231,7 @@ void kvm_timer_schedule(struct kvm_vcpu *vcpu)
> return;
>
> /* The timer has not yet expired, schedule a background timer */
> - cval = timer->cntv_cval;
> - now = kvm_phys_timer_read() - vcpu->kvm->arch.timer.cntvoff;
> -
> - ns = cyclecounter_cyc2ns(timecounter->cc,
> - cval - now,
> - timecounter->mask,
> - &timecounter->frac);
> - timer_arm(timer, ns);
> + timer_arm(timer, kvm_timer_compute_delta(vcpu));
> }
>
> void kvm_timer_unschedule(struct kvm_vcpu *vcpu)
> --
> 2.1.4
>
How do you guys feel about adding this to the patch for improved sleep
at night (pun intended):
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index a9ad4fe..230f720 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -91,6 +91,8 @@ static void kvm_timer_inject_irq_work(struct work_struct
*work)
vcpu = container_of(work, struct kvm_vcpu, arch.timer_cpu.expired);
vcpu->arch.timer_cpu.armed = false;
+ BUG_ON(!kvm_timer_should_fire(vcpu));
+
/*
* If the vcpu is blocked we want to wake it up so that it will see
* the timer has expired when entering the guest.
Otherwise:
Reviewed-by: Christoffer Dall <[email protected]>
And I can queue this with CC to stable, but I would like Alex's
tested-by if possible.
Thanks,
-Christoffer
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
