Re: [PATCH v2 4/4] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

[Alex Williamson <alex.williamson@redhat.com>]

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