WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

>>> On 09.09.11 at 18:47, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 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);
>+        }
>+    }
>+}
>...

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel