From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gleb Natapov Subject: Re: [PATCH][RFC] Use return value from kvm_set_irq() to re-inject PIT interrupts. Date: Wed, 26 Aug 2009 16:19:17 +0300 Message-ID: <20090826131917.GE30093@redhat.com> References: <20090824120623.GC30093@redhat.com> <20090824163256.GA8653@amt.cnet> <20090824171646.GA11762@redhat.com> <20090824174427.GA10311@amt.cnet> <20090824181905.GB11762@redhat.com> <20090826124348.GA32235@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, avi@redhat.com To: Marcelo Tosatti Return-path: Received: from mx1.redhat.com ([209.132.183.28]:21560 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756078AbZHZNTR (ORCPT ); Wed, 26 Aug 2009 09:19:17 -0400 Received: from int-mx08.intmail.prod.int.phx2.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.21]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id n7QDJJpW000881 for ; Wed, 26 Aug 2009 09:19:19 -0400 Content-Disposition: inline In-Reply-To: <20090826124348.GA32235@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: On Wed, Aug 26, 2009 at 09:43:48AM -0300, Marcelo Tosatti wrote: > On Mon, Aug 24, 2009 at 09:19:05PM +0300, Gleb Natapov wrote: > > > > Current code very fragile and relies on hacks to work. Lets take calling > > > > of ack notifiers on pic reset as an example. Why is it needed? > > > > > > To signal the ack notifiers users that, in case of reset with pending > > > IRR, the given interrupt has been "acked" (its an artificial ack event). > > > > > But IRR was not acked. The reason it is done is that otherwise the > > current logic will prevent further interrupt injection. > > Or will keep the host irq disabled, for the assigned device case (in > case you drop the hackish ack notification from pic_reset). > > I don't think it exists there because of PIT reinjection only, it seems > a generic problem for users of ack notifiers (a reset notifier as you > mentioned would also do it, and be cleaner). > Yes, I agree pic reset should be propagated to assigned devices somehow. > > > Is there a need to differentiate between actual interrupt ack and reset > > > with pending IRR? At the time this code was written, there was no > > > indication that differentation would be necessary. > > This is two different things. Ack notifiers should be called when guest > > acks interrupt. Calling it on reset is wrong (see below). We can add reset > > notifiers, but we just build yet another infrastructure to support > > current reinjection scheme. > > Its not specific to PIT reinjection. > > Anything that relies on ack notification to perform some action (either > reinjection or host irq line enablement or some other use) suffers from > the same thing. > > You might argue that a separate reset notification is more appropriate. > > > > > It is obviously wrong thing to do from assigned devices POV. > > > > > > Thats not entirely clear to me. So what happens if a guest with PIC > > > assigned device resets with a pending IRR? The host interrupt line will > > > be kept disabled, even though the guest is able to process further > > > interrupts? > > The host interrupt line will be enabled (assigned device ack notifier > > does this) without clearing interrupt condition in assigned device > > (guest hasn't acked irq so how can we be sure it ran device's irq > > handler?). Host will hang. > > > > > > Why ioapic calls mask notifiers but pic doesn't? > > > > > > Because it is not implemented. > > I see that. Why? Why it was important to implement for ioapic but not > > for pic? > > 4780c65904f0fc4e312ee2da9383eacbe04e61ea > This commit and previous one adds infrastructure to fix a bug that is there only because how we choose to do pit reinjection. Do it differently and you can revert both of them. > > Do we know what doesn't work now? > > What you mean? I mean that pit doesn't call mask notifier so similar bug to 4780c65 hides somewhere out there. How can we test it? > > > > > Besides diffstat for the patch shows: > > > > 2 files changed, 16 insertions(+), 59 deletions(-) > > > > > > > > 43 lines less for the same functionality. Looks like clear win to me. > > > > > > > > > Ack notifiers are asynchronous notifications. Using the return value > > > > > from kvm_set_irq implies that timer emulation is based on a "tick > > > > > generating device" on the host side. > > > > No notification is needed in the first place. You know immediately > > > > if injection fails or not. I don't see why "using return value from > > > > kvm_set_irq implies that timer emulation is based on a "tick generating > > > > device" on the host side"? What can you do with ack notifiers that can't > > > > be done without? > > > > > > If you don't have a host timer emulating the guest PIT, to periodically > > > bang on kvm_set_irq, how do you know when to attempt reinjection? > > > > > > You keep calling kvm_set_irq on every guest entry to figure out when > > > reinjection is possible? > > If we have timer to inject then yes. It is relatively cheap. Most of the > > time pending count will be zero. > > Won't work with non-tick-based emulation on the host. Why? This is the most important point, can you elaborate? -- Gleb.