[RFC] Spurious PIC interrupts

[RFC] Spurious PIC interrupts

[Andrew Cooper]

[-- 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

Re: [RFC] Spurious PIC interrupts

[Keir Fraser]

On 28/08/2012 20:59, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:

> 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?

No, you should make the change to _mask_and_ack_8259A_irq() itself, and
callers which do not care about the return code can simply discard it.

I hardly even want one instance of that appalling function in the tree, let
alone two! Terrible abuse of goto, and I'm pretty laid back about goto use.

And we don't use True/False anywhere else in Xen. We don't even have any
centralised definition of true/false of any kind, so you should really just
use 1/0 directly. Alternatively you could propose a patch to define
true/false in our types.h, and use those. I don't know where the capitalised
True/False came from!

 -- Keir

Re: [RFC] Spurious PIC interrupts

[Andrew Cooper]

On 28/08/12 22:32, Keir Fraser wrote:
> On 28/08/2012 20:59, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:
>
>> 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?
> No, you should make the change to _mask_and_ack_8259A_irq() itself, and
> callers which do not care about the return code can simply discard it.

Ok - I initially avoided that because _mask_and_ack_8259A_irq() is used
to fill a function pointer structure, and preferred less change to the core.

I will re-design somewhat with these points in mind.

~Andrew

>
> I hardly even want one instance of that appalling function in the tree, let
> alone two! Terrible abuse of goto, and I'm pretty laid back about goto use.
>
> And we don't use True/False anywhere else in Xen. We don't even have any
> centralised definition of true/false of any kind, so you should really just
> use 1/0 directly. Alternatively you could propose a patch to define
> true/false in our types.h, and use those. I don't know where the capitalised
> True/False came from!
>
>  -- Keir
>
>

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

Re: [RFC] Spurious PIC interrupts

[Keir Fraser]

On 29/08/2012 10:08, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:

>> No, you should make the change to _mask_and_ack_8259A_irq() itself, and
>> callers which do not care about the return code can simply discard it.
> 
> Ok - I initially avoided that because _mask_and_ack_8259A_irq() is used
> to fill a function pointer structure, and preferred less change to the core.
> 
> I will re-design somewhat with these points in mind.

Worst case rename to __mask_and_ack_8259A_irq() and implement new
_mask_and_ack_8259A_irq() which simply calls it and discards the return
code. Still much better than duplicating the function.

 -- Keir