On Tue, 2010-11-02 at 19:40 +0100, Jan Kiszka wrote: > Am 02.11.2010 19:24, Michael S. Tsirkin wrote: > > On Tue, Nov 02, 2010 at 06:56:14PM +0100, Jan Kiszka wrote: > >>>> dev->host_irq_disabled = false; > >>>> } > >>>> - spin_unlock(&dev->intx_lock); > >>>> +out: > >>>> + spin_unlock_irq(&dev->intx_lock); > >>>> + > >>>> + if (reassert) > >>>> + kvm_set_irq(dev->kvm, dev->irq_source_id, dev->guest_irq, 1); > >>> > >>> Hmm, I think this still has more overhead than it needs to have. > >>> Instead of setting level to 0 and then back to 1, can't we just > >>> avoid set to 1 in the first place? This would need a different > >>> interface than pci_2_3_irq_check_and_unmask to avoid a race > >>> where interrupt is received while we are acking another one: > >>> > >>> block userspace access > >>> check pending bit > >>> if (!pending) > >>> set irq (0) > >>> clear pending > >>> block userspace access > >>> > >>> Would be worth it for high volume devices. > >> > >> The problem is that we can't reorder guest IRQ line clearing and host > >> IRQ line enabling without taking a lock across host IRQ disable + guest > >> IRQ raise - and that is now distributed across hard and threaded IRQ > >> handlers and we don't want to hold and IRQ-safe lock during kvm_set_irq. > > > > Oh I think I confused you. > > What I mean is: > > > > block userspace access > > check interrupt status bit > > if (!interrupt status bit set) > > set irq (0) > > clear interrupt disable bit > > block userspace access > > > > This way we enable interrupt after set irq so not need for > > extra locks I think. > > OK. Would require some serious refactoring again. > > But what about edge IRQs? Don't we need to toggle the bit for them? And > as we do not differentiate between level and edge, we currently have to > do this unconditionally. > > > > > Hmm one thing I noticed is that pci_block_user_cfg_access > > will BUG_ON if it was already blocked. So I think we have > > a bug here when interrupt handler kicks in right after > > we unmask interrupts. > > > > Probably need some kind of lock to protect against this. > > > > Or an atomic counter. Will have a look. > > Alex, does VFIO take care of this already? Yes, VFIO has a lock used by the interrupt handler and the EOI handler that prevents them from both blocking user cfg access at the same time. Alex -- 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
