[RFC] Spurious PIC interrupts

[Andrew Cooper <andrew.cooper3@citrix.com>]

[-- Attachment #1: Type: text/plain, Size: 876 bytes --]

Since Jan's patch to print and mask bogus PIC vectors, we have found
some issues on older hardware were supurious PIC vectors are being
repeatedly logged, as spurious vectors will ignore the relevant mask bit.

The log message is deceptive in the case of a spurious vector.  I have
attached an RFC patch which changes the bogus_8259A_irq logic to be able
to detect spurious vectors and be rather less verbose about them.

The new bogus_8259A_irq() function is basically a copy of
_mask_and_ack_8259A_irq(), but returning a boolean indicating whether it
was a genuine interrupt or not, which controls whether the "No irq
handler" message in do_IRQ gets printed or not.

Jan: are you happy with the style of the adjustment, or could you
suggest a better way of doing it?

-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com


[-- Attachment #2: pic-bogus-spurious.patch --]
[-- Type: text/x-patch, Size: 4841 bytes --]

# HG changeset patch
# Parent 1126b3079bef37e1bb5a97b90c14a51d4e1c91c3

diff -r 1126b3079bef xen/arch/x86/i8259.c
--- a/xen/arch/x86/i8259.c
+++ b/xen/arch/x86/i8259.c
@@ -87,8 +87,82 @@ static DEFINE_SPINLOCK(i8259A_lock);
 
 static void _mask_and_ack_8259A_irq(unsigned int irq);
 
-void (*__read_mostly bogus_8259A_irq)(unsigned int irq) =
-    _mask_and_ack_8259A_irq;
+/*
+ * Deal with a bogus 8259A interrupt.  Return True if
+ * it is a genuine interrupt, and False if it is spurious.
+ */
+bool_t __read_mostly bogus_8259A_irq(unsigned int irq)
+{
+    unsigned int irqmask = 1 << irq;
+    unsigned long flags;
+    bool_t real_irq = True;
+    bool_t need_eoi = i8259A_irq_type.ack != disable_8259A_irq;
+
+    spin_lock_irqsave(&i8259A_lock, flags);
+    /*
+     * Lightweight spurious IRQ detection. We do not want
+     * to overdo spurious IRQ handling - it's usually a sign
+     * of hardware problems, so we only do the checks we can
+     * do without slowing down good hardware unnecesserily.
+     *
+     * Note that IRQ7 and IRQ15 (the two spurious IRQs
+     * usually resulting from the 8259A-1|2 PICs) occur
+     * even if the IRQ is masked in the 8259A. Thus we
+     * can check spurious 8259A IRQs without doing the
+     * quite slow i8259A_irq_real() call for every IRQ.
+     * This does not cover 100% of spurious interrupts,
+     * but should be enough to warn the user that there
+     * is something bad going on ...
+     */
+    if (cached_irq_mask & irqmask)
+        goto spurious_8259A_irq;
+    cached_irq_mask |= irqmask;
+
+ handle_real_irq:
+    if (irq & 8) {
+        inb(0xA1);              /* DUMMY - (do we need this?) */
+        outb(cached_A1,0xA1);
+        if ( need_eoi ) {
+            outb(0x60+(irq&7),0xA0);/* 'Specific EOI' to slave */
+            outb(0x62,0x20);        /* 'Specific EOI' to master-IRQ2 */
+        }
+    } else {
+        inb(0x21);              /* DUMMY - (do we need this?) */
+        outb(cached_21,0x21);
+        if ( need_eoi )
+            outb(0x60+irq,0x20);    /* 'Specific EOI' to master */
+    }
+
+    goto out;
+
+ spurious_8259A_irq:
+    /*
+     * this is the slow path - should happen rarely.
+     */
+    if (i8259A_irq_real(irq))
+        /*
+         * oops, the IRQ _is_ in service according to the
+         * 8259A - not spurious, go handle it.
+         */
+        goto handle_real_irq;
+
+    {
+        static int spurious_irq_mask;
+        /*
+         * At this point we can be sure the IRQ is spurious,
+         * lets ACK and report it. [once per IRQ]
+         */
+        if (!(spurious_irq_mask & irqmask)) {
+            printk("spurious 8259A interrupt: IRQ%d.\n", irq);
+            spurious_irq_mask |= irqmask;
+        }
+        real_irq = False;
+    }
+
+out:
+    spin_unlock_irqrestore(&i8259A_lock, flags);
+    return real_irq;
+}
 
 static void mask_and_ack_8259A_irq(struct irq_desc *desc)
 {
@@ -367,19 +441,13 @@ void __devinit init_8259A(int auto_eoi)
                                is to be investigated) */
 
     if (auto_eoi)
-    {
         /*
          * in AEOI mode we just have to mask the interrupt
          * when acking.
          */
         i8259A_irq_type.ack = disable_8259A_irq;
-        bogus_8259A_irq = _disable_8259A_irq;
-    }
     else
-    {
         i8259A_irq_type.ack = mask_and_ack_8259A_irq;
-        bogus_8259A_irq = _mask_and_ack_8259A_irq;
-    }
 
     udelay(100);            /* wait for 8259A to initialize */
 
diff -r 1126b3079bef xen/arch/x86/irq.c
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -817,11 +817,11 @@ void do_IRQ(struct cpu_user_regs *regs)
                 ack_APIC_irq();
             else
                 kind = "";
-            if ( vector >= FIRST_LEGACY_VECTOR &&
-                 vector <= LAST_LEGACY_VECTOR )
-                bogus_8259A_irq(vector - FIRST_LEGACY_VECTOR);
-            printk("CPU%u: No irq handler for vector %02x (IRQ %d%s)\n",
-                   smp_processor_id(), vector, irq, kind);
+            if ( ! ( vector >= FIRST_LEGACY_VECTOR &&
+                     vector <= LAST_LEGACY_VECTOR &&
+                     bogus_8259A_irq(vector - FIRST_LEGACY_VECTOR) ) )
+                printk("CPU%u: No irq handler for vector %02x (IRQ %d%s)\n",
+                       smp_processor_id(), vector, irq, kind);
             TRACE_1D(TRC_HW_IRQ_UNMAPPED_VECTOR, vector);
         }
         goto out_no_unlock;
diff -r 1126b3079bef xen/include/asm-x86/irq.h
--- a/xen/include/asm-x86/irq.h
+++ b/xen/include/asm-x86/irq.h
@@ -104,7 +104,7 @@ void mask_8259A(void);
 void unmask_8259A(void);
 void init_8259A(int aeoi);
 void make_8259A_irq(unsigned int irq);
-extern void (*bogus_8259A_irq)(unsigned int irq);
+bool_t bogus_8259A_irq(unsigned int irq);
 int i8259A_suspend(void);
 int i8259A_resume(void);
 

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel