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? > > > > > > > > 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? >From http://www.mail-archive.com/kvm@xxxxxxxxxxxxxxx/msg18644.html. An injectable timer interrupt is defined by: - time(now) >= time(next_expiration) - Previous timer interrupt has been acked (thus we can inject). The thing is, sure you can drop ack notifiers and check IRR on every guest entry, but why bother if you can receive an asynchronous notification? Would you prefer to replace + if (!ktimer->can_inject) With kvm_set_irq() ? Not relatively cheap. -- 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
