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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
