On Wed, Aug 12, 2009 at 12:57:07PM +0300, Avi Kivity wrote: > On 08/12/2009 12:47 PM, Gleb Natapov wrote: >>> Ah, the real motivation is msi. Pushing locks down doesn't help if we >>> keep locking them. But for msi we avoid the lock entirely. >>> >>> >> Yes. MSI is one. Multiple IOAPICs may inject interrupt in parallel too >> (if we will choose to implement multiple IOAPICs sometime). >> > > Right. Given msi, I don't think we'll have multiple ioapics though. > >>>>> Why a spinlock and not a mutex? >>>>> >>>>> >>>>> >>>> Protected sections are small and we do not sleep there. >>>> >>>> >>> So what? A mutex is better since it allows preemption (and still has >>> spinlock performance if it isn't preempted). >>> >>> >> This lock will be taken during irq injection from irqfd, so may be leave >> it as spinlock and take it _irqsave()? Do we want to allow irq injection >> from interrupt context? Otherwise if you say that performance is the >> same I don't care one way or the other. >> > > Let's leave _irqsave() until later since that has other implications. > So change it to mutex then? >>>>> Need to explain why this is safe. I'm not sure it is, because we touch >>>>> state afterwards in pic_intack(). We need to do all vcpu-synchronous >>>>> operations before dropping the lock. >>>>> >>>>> >>>> Forst pic_intack() calls pic_clear_isr() only in auto eoi mode and this mode >>>> is already broken for assigned devices. Second for level triggered >>>> interrupts pic_intack() does nothing after calling pic_clear_isr() and >>>> third I can move pic_clear_isr() call to the end of pic_intack(). >>>> >>>> >>> I meant, in a comment. >>> >> I you agree with above I'll add it as a comment. >> > > Sure. > >>> It should. What if there's a reset with an assigned device? We need to >>> release the device interrupt (after doing FLR?). >>> >> Doing this will just re-enable host interrupt while irq condition is not >> cleared in the device. The host will hang. So I think we really shouldn't. >> > > Ok. What about timer acks? > Interrupt wasn't processed by a guest. Why should it be accounted as such? > > > >>>> I don't see why we clear remote_irr before dropping the lock. If, while >>>> lock was dropped, interrupt was delivered to this entry it will be >>>> injected when ack notifier returns. >>>> >>>> >>> But we'll clear remote_irr afterward the redelivery, and we should to >>> that only after the new interrupt is acked. >>> >> It depend on whether you consider calling ack notifiers a part of >> interrupt acknowledgement process. > > I don't really care, but the ack process has to be atomic. Since we > need to drop the lock, it means the notifier is not part of the process. > >> If you do then remote_irr should not >> be cleared before ack notifiers since ack process is not completed yet. >> With current users functionally it shouldn't matter when we clear >> remote_irr. I prefer doing it like we do it now since this how it was >> before my patches and since code is simpler this way. >> > > No, I think it introduces a race if an interrupt is raised while the ack > notifier is running. > > > >>> No, I mean keep the for loop in kvm_ioapic_update_eoi(). >>> >>> >> Can't do that. __kvm_ioapic_update_eoi() is called from other place with >> lock held already. >> > > Ok. > > -- > error compiling committee.c: too many arguments to function -- 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
