Re: [PATCH][RFC] pv-ops: fix shared irq device passthrough

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Tue, Nov 10, 2009 at 09:11:23AM -0800, Kay, Allen M wrote:
> > With this patch you can share the PCI devices on the same IRQ, correct? Will
> > this mean that you can assign to a guest a USB controller, while
> > Dom0 has controller of the PCI NIC (and assuming that both of those
> > share the same interrupt line)? If so, won't the Dom0 start throwing
> > a fit b/c there are unhandled IRQs and eventually disable the IRQ line?
> 
> No, interrupts are injected to both domains sharing the same IRQ.
> See arch/x86/irq.c/__do_IRQ_guest() for details.

You are looking at it from the Xen level. The patch was for Linux pv_ops.
> 
> > Or even the guest decide that there are too many IRQs and decided to
> > disable the IRQ line?
> 
> Note that in __do_IRQ_guest(), interrupt handling control flow for PCI passthrough case
> Is the same for PV event channel case.  Guest doesn't disable the IRQ line unless
> there is a malfunction.

Right, and the way Linux detects if there is a malfunction is by seeing if the
interrupt handlers don't handle the interrupt. The logic in Dom0 is that do_IRQ is
called, which then calls (depending on the type of interrupt - edge or level)
the proper top-level interrupt handler:


354 void
355 handle_level_irq(unsigned int irq, struct irq_desc *desc)
356 {

.. snip ..

which goes through all of the IRQ handlers sharing the IRQ level.
If there is one, then it will just call one of them.

377         spin_unlock(&desc->lock);
378 
379         action_ret = handle_IRQ_event(irq, action);
380         if (!noirqdebug)
381                 note_interrupt(irq, desc, action_ret);
382 

The 'handle_IRQ_event' calls the IRQ handler, for example the
ahci one:

2143 static irqreturn_t ahci_interrupt(int irq, void *dev_instance)
2144 {
.. snip ..
2156         /* sigh.  0xffffffff is a valid return from h/w */
2157         irq_stat = readl(mmio + HOST_IRQ_STAT);
2158         if (!irq_stat)
2159                 return IRQ_NONE;

And lets that the interrupt was fired off on the USB controller (which
is owned by the guest, not Dom0) and the AHCI one did not have presently
any data (the guest is booting of a SCSI controller or what not).

The 'note_interrupt' would decide that since this isn't handled:

226 void note_interrupt(unsigned int irq, struct irq_desc *desc,
227                     irqreturn_t action_ret)
228 {
229         if (unlikely(action_ret != IRQ_HANDLED)) {

it would accumulate the count of unhandled interrupts and then
eventually disable the interrupt:

256         if (unlikely(desc->irqs_unhandled > 99900)) {
257                 /*
258                  * The interrupt is stuck
259                  */
260                 __report_bad_irq(irq, desc, action_ret);
261                 /*
262                  * Now kill the IRQ
263                  */
264                 printk(KERN_EMERG "Disabling IRQ #%d\n", irq);
265                 desc->status |= IRQ_DISABLED | IRQ_SPURIOUS_DISABLED;
266                 desc->depth++;
267                 desc->chip->disable(irq);


I think this problem exists with or _without_ the proposed patch.

Except that _without_ the patch you can't even get to share an interrupt
in which case you would never go down this logic path.