From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [patch 3/3] KVM: PIT: fix injection logic and count Date: Tue, 29 Jul 2008 15:55:26 +0300 Message-ID: <488F133E.7010702@qumranet.com> References: <20080726200058.559700262@localhost.localdomain> <20080726200349.277527367@localhost.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Sheng Yang , kvm To: Marcelo Tosatti Return-path: Received: from il.qumranet.com ([212.179.150.194]:11413 "EHLO il.qumranet.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752655AbYG2Mz2 (ORCPT ); Tue, 29 Jul 2008 08:55:28 -0400 In-Reply-To: <20080726200349.277527367@localhost.localdomain> Sender: kvm-owner@vger.kernel.org List-ID: Marcelo Tosatti wrote: > The PIT injection logic is problematic under the following cases: > > 1) If there is a higher priority vector to be delivered by the time > kvm_pit_timer_intr_post is invoked ps->inject_pending won't be set. > This opens the possibility for missing many PIT event injections (say if > guest executes hlt at this point). > > 2) ps->inject_pending is racy with more than two vcpus. Since there's no locking > around read/dec of pt->pending, two vcpu's can inject two interrupts for a single > pt->pending count. > > Fix 1 by using an irq ack notifier: only reinject when the previous irq > has been acked. Fix 2 with appropriate locking around manipulation of > pending count and irq_ack by the injection / ack paths. > > Also, count_load_time should be set at the time the count is reloaded, > not when the interrupt is injected (BTW, LAPIC uses the same apparently > broken scheme, could someone explain what was the reasoning behind > that? kvm_apic_timer_intr_post). > > Signed-off-by: Marcelo Tosatti > > Index: kvm/arch/x86/kvm/i8254.c > =================================================================== > --- kvm.orig/arch/x86/kvm/i8254.c > +++ kvm/arch/x86/kvm/i8254.c > @@ -207,6 +207,8 @@ static int __pit_timer_fn(struct kvm_kpi > > pt->timer.expires = ktime_add_ns(pt->timer.expires, pt->period); > pt->scheduled = ktime_to_ns(pt->timer.expires); > + if (pt->period) > + ps->channels[0].count_load_time = pt->timer.expires; > > return (pt->period == 0 ? 0 : 1); > } > Sometimes the guest leaves the timer enabled but the output pin masked, (e.g. it doesn't use the timer bug doesn't bother to turn it off properly). This results in extraneous interrupts, causing unnecessary vmexits and increased power usage. But if we detect that the guest isn't processing the interrupts, we can turn the timer off, and after the next injection, calculate the number of missing interrupts, and turn the timer on again. This will have to be done carefully (taking care of the guest adjusting the frequency during the period where we missed injections, for example). Comments? Questions? Patches? -- error compiling committee.c: too many arguments to function