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

[Gleb Natapov <gleb@redhat.com>]

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.