Re: [PATCH][RFC] Use return value from kvm_set_irq() to re-inject PIT interrupts.

[Marcelo Tosatti <mtosatti@redhat.com>]

On Wed, Aug 26, 2009 at 04:19:17PM +0300, Gleb Natapov wrote:
> 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?

Program periodic PIT, mask irq0, wait a while, unmask irq0 ?