From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH 5/5] KVM: x86: Using TSC deadline may cause multiple interrupts by user writes Date: Fri, 10 Oct 2014 16:02:05 +0200 Message-ID: <5437E6DD.6070103@redhat.com> References: <1412287806-16016-1-git-send-email-namit@cs.technion.ac.il> <1412287806-16016-6-git-send-email-namit@cs.technion.ac.il> <20141006205737.GC2722@potion.brq.redhat.com> <4E3FA8A7-6CEF-4077-AD91-9AAE1AF86FEF@gmail.com> <20141008100619.GA20422@potion.brq.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Nadav Amit , joro@8bytes.org, kvm@vger.kernel.org To: =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , Nadav Amit Return-path: Received: from mail-wi0-f179.google.com ([209.85.212.179]:35799 "EHLO mail-wi0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754062AbaJJOCM (ORCPT ); Fri, 10 Oct 2014 10:02:12 -0400 Received: by mail-wi0-f179.google.com with SMTP id d1so2131314wiv.12 for ; Fri, 10 Oct 2014 07:02:09 -0700 (PDT) In-Reply-To: <20141008100619.GA20422@potion.brq.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: Il 08/10/2014 12:06, Radim Kr=C4=8Dm=C3=A1=C5=99 ha scritto: > KVM: x86: fix deadline tsc interrupt injection >=20 > The check in kvm_set_lapic_tscdeadline_msr() was trying to prevent a > situation where we lose a pending deadline timer in a MSR write. > Losing it is fine, because it effectively occurs before the timer fir= ed, > so we should be able to cancel or postpone it. >=20 > Another problem comes from interaction with QEMU, or other userspace > that can set deadline MSR without a good reason, when timer is alread= y > pending: one guest's deadline request results in more than one > interrupt because one is injected immediately on MSR write from > userspace and one through hrtimer later. >=20 > The solution is to remove the injection when replacing a pending time= r > and to improve the usual QEMU path, we inject without a hrtimer when = the > deadline has already passed. >=20 > Signed-off-by: Radim Kr=C4=8Dm=C3=A1=C5=99 > Reported-by: Nadav Amit > --- > arch/x86/kvm/lapic.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) >=20 > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index b8345dd..51428dd 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -1096,9 +1096,12 @@ static void start_apic_timer(struct kvm_lapic = *apic) > if (likely(tscdeadline > guest_tsc)) { > ns =3D (tscdeadline - guest_tsc) * 1000000ULL; > do_div(ns, this_tsc_khz); > + hrtimer_start(&apic->lapic_timer.timer, > + ktime_add_ns(now, ns), HRTIMER_MODE_ABS); > + } else { > + atomic_inc(&ktimer->pending); > + kvm_make_request(KVM_REQ_PENDING_TIMER, vcpu); > } > - hrtimer_start(&apic->lapic_timer.timer, > - ktime_add_ns(now, ns), HRTIMER_MODE_ABS); > =20 > local_irq_restore(flags); > } > @@ -1355,9 +1358,6 @@ void kvm_set_lapic_tscdeadline_msr(struct kvm_v= cpu *vcpu, u64 data) > return; > =20 > hrtimer_cancel(&apic->lapic_timer.timer); > - /* Inject here so clearing tscdeadline won't override new value */ > - if (apic_has_pending_timer(vcpu)) > - kvm_inject_apic_timer_irqs(vcpu); > apic->lapic_timer.tscdeadline =3D data; > start_apic_timer(apic); > } Radim, the patch looks good but please extract this code: /* * There is a race window between reading and incrementing, but= we do * not care about potentially losing timer events in the !reinj= ect * case anyway. Note: KVM_REQ_PENDING_TIMER is implicitly check= ed * in vcpu_enter_guest. */ if (!atomic_read(&ktimer->pending)) { atomic_inc(&ktimer->pending); /* FIXME: this code should not know anything about vcpu= s */ kvm_make_request(KVM_REQ_PENDING_TIMER, vcpu); } if (waitqueue_active(q)) wake_up_interruptible(q); to a new "static void apic_timer_expired(struct kvm_lapic *apic)" funct= ion, and call it from both apic_timer_fn and start_apic_timer. Also, we should not need to do wake_up_interruptible() unless we have changed ktimer->pending from zero to non-zero. Paolo