Re: Re: 4.0/4.1 requests - IO-APIC EOI v4 [RFC]

["Jan Beulich" <JBeulich@suse.com>]

>>> On 09.09.11 at 18:47, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 09/09/11 17:22, Andrew Cooper wrote:
>> ---SNIP---
>>
>> Version 3.  Applied Jan's suggestions, and extended some of the comments.
>>
> 
> V4 - get the BUG_ON logic correct (oops).
> 
> I have now done a few reboot tests and a kexec test with this patch.
> 
> Are there any other tests you would suggest, or shall I formally submit
> the patch for unstable and backporting?


Looks technically good now, but there are a few cosmetic comments still:

>--- a/xen/arch/x86/io_apic.c	Mon Sep 05 15:10:28 2011 +0100
>+++ b/xen/arch/x86/io_apic.c	Fri Sep 09 17:20:49 2011 +0100
>@@ -68,6 +68,16 @@ int __read_mostly nr_ioapics;
> #define MAX_PLUS_SHARED_IRQS nr_irqs_gsi
> #define PIN_MAP_SIZE (MAX_PLUS_SHARED_IRQS + nr_irqs_gsi)
> 
>+
>+#define ioapic_has_eoi_reg(apic) (mp_ioapics[(apic)].mpc_apicver >= 0x20)
>+
>+static void __io_apic_eoi(unsigned int apic, unsigned int vector, unsigned int pin);
>+static void io_apic_eoi(unsigned int apic, unsigned int vector, unsigned int pin);

No need to declare these here if you move the definitions up before
the first use (would fit well after ioapic_write_entry()).

>+
>+#define io_apic_eoi_vector(apic, vector) io_apic_eoi((apic), (vector), -1)
>+#define io_apic_eoi_pin(apic, pin) io_apic_eoi((apic), -1, (pin))
>+
>+
> /*
>  * This is performance-critical, we want to do it O(1)
>  *
>...
>@@ -2622,3 +2621,124 @@ void __init init_ioapic_mappings(void)
>     printk(XENLOG_INFO "IRQ limits: %u GSI, %u MSI/MSI-X\n",
>            nr_irqs_gsi, nr_irqs - nr_irqs_gsi);
> }
>+
>+/* EOI an IO-APIC entry.  One of vector or pin may be -1, indicating that
>+ * it should be worked out using the other.  This function disables interrupts
>+ * and takes the ioapic_lock */
>+static void io_apic_eoi(unsigned int apic, unsigned int vector, unsigned int pin)
>+{
>+    unsigned int flags;
>+    spin_lock_irqsave(&ioapic_lock, flags);
>+    __io_apic_eoi(apic, vector, pin);
>+    spin_unlock_irqrestore(&ioapic_lock, flags);
>+}
>+
>+/* EOI an IO-APIC entry.  One of vector or pin may be -1, indicating that
>+ * it should be worked out using the other.  This function expect that the
>+ * ioapic_lock is taken, and interrupts are disabled (or there is a good reason
>+ * not to), and that if both pin and vector are passed, that they refer to the
>+ * same redirection entry in the IO-APIC. */
>+static void __io_apic_eoi(unsigned int apic, unsigned int vector, unsigned int pin)
>+{
>+    /* Ensure some useful information is passed in */
>+    BUG_ON( !(vector == -1 && pin != -1) );
>+    
>+    /* Prefer the use of the EOI register if available */
>+    if ( ioapic_has_eoi_reg(apic) )
>+    {
>+        /* If vector is unknown, read it from the IO-APIC */
>+        if ( vector == -1 )
>+            vector = __ioapic_read_entry(apic, pin, TRUE).vector;
>+
>+        *(IO_APIC_BASE(apic)+16) = vector;
>+    }
>+    else
>+    {
>+        /* Else fake an EOI by switching to edge triggered mode
>+         * and back */
>+        struct IO_APIC_route_entry entry;
>+        bool_t need_to_unmask = 0;
>+
>+        /* If pin is unknown, search for it */
>+        if ( pin == -1 )
>+        {
>+            unsigned int p;
>+            for ( p = 0; p < nr_ioapic_registers[apic]; ++p )
>+            {
>+                entry = __ioapic_read_entry(apic, p, TRUE);
>+                if ( entry.vector == vector )
>+                {
>+                    pin = p;
>+                    /* break; */
>+
>+                    /* Here should be a break out of the loop, but at the 

... moment ...

>+                     * Xen code doesn't actually prevent multiple IO-APIC
>+                     * entries being assigned the same vector, so EOI all
>+                     * pins which have the correct vector.
>+                     *
>+                     * Remove the following code when the above assertion
>+                     * is fulfilled. */
>+

Why don't you just call __io_apic_eoi() recursively here?

Jan

>+                    if ( ! entry.mask )
>+                    {
>+                        /* If entry is not currently masked, mask it and make
>+                         * a note to unmask it later */
>+                        entry.mask = 1;
>+                        __ioapic_write_entry(apic, pin, TRUE, entry);
>+                        need_to_unmask = 1;
>+                    }
>+
>+                    /* Flip the trigger mode to edge and back */
>+                    entry.trigger = 0;
>+                    __ioapic_write_entry(apic, pin, TRUE, entry);
>+                    entry.trigger = 1;
>+                    __ioapic_write_entry(apic, pin, TRUE, entry);
>+
>+                    if ( need_to_unmask )
>+                    {
>+                        /* Unmask if neccesary */
>+                        entry.mask = 0;
>+                        __ioapic_write_entry(apic, pin, TRUE, entry);
>+                    }
>+
>+                }
>+            }
>+            
>+            /* If search fails, nothing to do */
>+
>+            /* if ( pin == -1 ) */
>+
>+            /* Because the loop wasn't broken out of (see comment above),
>+             * all relevant pins have been EOI, so we can always return.
>+             * 
>+             * Re-instate the if statement above when the Xen logic has been
>+             * fixed.*/
>+
>+            return;
>+        }
>+
>+        entry = __ioapic_read_entry(apic, pin, TRUE);
>+
>+        if ( ! entry.mask )
>+        {
>+            /* If entry is not currently masked, mask it and make
>+             * a note to unmask it later */
>+            entry.mask = 1;
>+            __ioapic_write_entry(apic, pin, TRUE, entry);
>+            need_to_unmask = 1;
>+        }
>+
>+        /* Flip the trigger mode to edge and back */
>+        entry.trigger = 0;
>+        __ioapic_write_entry(apic, pin, TRUE, entry);
>+        entry.trigger = 1;
>+        __ioapic_write_entry(apic, pin, TRUE, entry);
>+
>+        if ( need_to_unmask )
>+        {
>+            /* Unmask if neccesary */
>+            entry.mask = 0;
>+            __ioapic_write_entry(apic, pin, TRUE, entry);
>+        }
>+    }
>+}
>...