From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radim =?utf-8?B?S3LEjW3DocWZ?= Subject: Re: [PATCH 5/5] KVM: x86: Using TSC deadline may cause multiple interrupts by user writes Date: Wed, 8 Oct 2014 12:06:21 +0200 Message-ID: <20141008100619.GA20422@potion.brq.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> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Nadav Amit , pbonzini@redhat.com, joro@8bytes.org, kvm@vger.kernel.org To: Nadav Amit Return-path: Received: from mx1.redhat.com ([209.132.183.28]:38377 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754555AbaJHKGg (ORCPT ); Wed, 8 Oct 2014 06:06:36 -0400 Content-Disposition: inline In-Reply-To: <4E3FA8A7-6CEF-4077-AD91-9AAE1AF86FEF@gmail.com> Sender: kvm-owner@vger.kernel.org List-ID: 2014-10-07 12:35+0300, Nadav Amit: > Thanks for reviewing this patch and the rest of the gang. Happy to do so, I've learned a lot. > On Oct 6, 2014, at 11:57 PM, Radim Kr=C4=8Dm=C3=A1=C5=99 wrote: > > 2014-10-03 01:10+0300, Nadav Amit: > >> Setting the TSC deadline MSR that are initiated by the host (using= ioctl's) may > >> cause superfluous interrupt. This occurs in the following case: > >>=20 > >> 1. A TSC deadline timer interrupt is pending. > >> 2. TSC deadline was still not cleared (which happens during vcpu_r= un). > >> 3. Userspace uses KVM_GET_MSRS/KVM_SET_MSRS to load the same deadl= ine msr. > >>=20 > >> To solve this situation, ignore host initiated TSC deadline writes= that do not > >> change the deadline value. > >=20 > > I find this change slightly dubious =E2=80=A6 > Why? I see similar handling of MSR_TSC_ADJUST. In other modes, we don't inject pending timer when writing to APIC_TMICT. (Which, sadly, is inconsistent with APIC_LVTT.) Adding a workaround is usually worse than removing the reason ... > > - why does the userspace do that? > It seems qemu=E2=80=99s kvm_cpu_exec does so when it calls kvm_arch_p= ut_registers. > It is pretty much done after every exit to userspace. Thanks, it really doesn't do much checking of what is needed. > > - why is host_initiated required? > Since if the guest writes to the MSR, it means it wants to rearm the = TSC deadline. Even if the deadline passed, interrupt should be triggere= d. MSR isn't 0, so the deadline hasn't passed for the guest yet. > If the guest writes the same value on the deadline MSR twice, it migh= t expect two interrupts. When guest writes to it without getting an interrupt first, it might expect just one. (Which it better IMO.) > >=20 > > Thanks. > >=20 > > It seems like an performance improvement, so why shouldn't return w= hen > > 'data <=3D tscdeadline && pending()' > > or even > > 'data <=3D now() && pending()' > >=20 > > (Sorry, I ran out of time to search for answers today.) > The bug I encountered is not a performance issue, but a superfluous i= nterrupt (functional bug). True. > As I said, the guest may write a new deadline MSR value which is in t= he past and expect an interrupt. And it would get one from the currently pending timer. What about the following patch? (The introduced else branch could use some abstractions.) --8<--- KVM: x86: fix deadline tsc interrupt injection 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 fired= , so we should be able to cancel or postpone it. Another problem comes from interaction with QEMU, or other userspace that can set deadline MSR without a good reason, when timer is already 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. The solution is to remove the injection when replacing a pending timer and to improve the usual QEMU path, we inject without a hrtimer when th= e deadline has already passed. 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(-) 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 *a= pic) 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_vcp= u *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); }