Re: [PATCH v2 4/5] KVM: Kick resamplefd for split kernel irqchip

[Alex Williamson]

On Mon, 9 Mar 2020 20:38:08 -0400
Peter Xu <address@hidden> wrote:

> On Mon, Mar 09, 2020 at 04:33:59PM -0600, Alex Williamson wrote:
> 
> [...]
> 
> > > > diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
> > > > index 15747fe2c2..13921b333d 100644
> > > > --- a/hw/intc/ioapic.c
> > > > +++ b/hw/intc/ioapic.c
> > > > @@ -236,8 +236,27 @@ void ioapic_eoi_broadcast(int vector)
> > > >          for (n = 0; n < IOAPIC_NUM_PINS; n++) {
> > > >              entry = s->ioredtbl[n];
> > > >  
> > > > -            if ((entry & IOAPIC_VECTOR_MASK) != vector ||
> > > > -                ((entry >> IOAPIC_LVT_TRIGGER_MODE_SHIFT) & 1) != 
> > > > IOAPIC_TRIGGER_LEVEL) {
> > > > +            if ((entry & IOAPIC_VECTOR_MASK) != vector) {
> > > > +                continue;
> > > > +            }
> > > > +
> > > > +            /*
> > > > +             * When IOAPIC is in the userspace while APIC is still in
> > > > +             * the kernel (i.e., split irqchip), we have a trick to
> > > > +             * kick the resamplefd logic for registered irqfds from
> > > > +             * userspace to deactivate the IRQ.  When that happens, it
> > > > +             * means the irq bypassed userspace IOAPIC (so the irr and
> > > > +             * remote-irr of the table entry should be bypassed too
> > > > +             * even if interrupt come), then we don't need to clear
> > > > +             * the remote-IRR and check irr again because they'll
> > > > +             * always be zeros.
> > > > +             */
> > > > +            if (kvm_resample_fd_notify(n)) {
> > > > +                continue;
> > > > +            }    
> > > 
> > > It seems the problem I reported is here.  In my configuration virtio-blk
> > > and an assigned e1000e share an interrupt.  virtio-blk is initializing
> > > and apparently triggers an interrupt.  The vfio-pci device is
> > > configured for INTx though not active yet, but kvm_resample_fd_notify()
> > > kicks the fd here, so we continue.  If I remove the continue here both
> > > devices seem to work, but I don't claim to understand the condition
> > > we're trying to continue for here yet.  This series needs more testing
> > > with shared interrupts.  Thanks,  
> > 
> > I'm also curious how this ended up between testing whether the vector
> > is masked and testing that it's level triggered.  We shouldn't have any
> > edge triggered resamplers.  
> 
> We had a similar discussion in V1 (with Paolo):
> 
> https://patchwork.kernel.org/patch/11407441/#23190891
> 
> So my understanding is that VFIO will unmask the intx IRQ when it
> comes, and register the resamplefd too, no matter whether it's level
> triggered (at least from what the code does).  Am I right?

As Paolo replied in your previous discussion, INTx is always level
triggered.
 
> > I find however that if I move the resampler
> > notify to after the remote IRR test, my NIC gets starved of interrupts.
> > So empirically, it seems kvm_resample_fd_notify() should be a void
> > function called unconditionally between the original mask+level check
> > removed above and the IRR check below.  Thanks,  
> 
> Yes IMHO we can't move that notify() to be after the remote IRR check
> because the IRR and remote IRR will be completely bypassed for the
> assigned device.  In other words, even if the interrupt has arrived
> for the assigned device, both IRR and remote IRR should always be zero
> (assuming the virtio-blk device doesn't generate any IRQ).  So we
> probably still need to do the notify even if remote-irr is not set.

Yep.  Thanks,

Alex