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

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

>>> On 09.09.11 at 17:06, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>@@ -397,18 +397,7 @@ static void clear_IO_APIC_pin(unsigned i
>             entry.trigger = 1;
>             __ioapic_write_entry(apic, pin, TRUE, entry);
>         }
>-        if (mp_ioapics[apic].mpc_apicver >= 0x20)
>-            io_apic_eoi(apic, entry.vector);
>-        else {
>-            /*
>-             * Mechanism by which we clear remoteIRR in this case is by
>-             * changing the trigger mode to edge and back to level.
>-             */
>-            entry.trigger = 0;
>-            __ioapic_write_entry(apic, pin, TRUE, entry);
>-            entry.trigger = 1;
>-            __ioapic_write_entry(apic, pin, TRUE, entry);
>-        }
>+        io_apic_eoi(apic, entry.vector, pin);

This should be __io_apic_eoi() - all other functions called here are the
non-locking ones, too.

>     }
> 
>     /*
>...
>@@ -2622,3 +2611,86 @@ 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 */
>+void io_apic_eoi(unsigned int apic, unsigned int vector, unsigned int pin)

static?

>+{
>+    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 */
>+void __io_apic_eoi(unsigned int apic, unsigned int vector, unsigned int pin)

static!

>+    {
>+        /* Else fake an EOI by switching to edge triggered mode
>+         * and back */
>+        struct IO_APIC_route_entry entry;
>+        bool_t need_to_unmask = 0;
>+

You may want to assert that at least one of vector and pin is not -1.

>+        /* If pin is unknown, search for it */
>+        if ( pin == -1 )
>+        {
>+            unsigned int p;
>+            for ( p = 0; p < nr_ioapic_registers[apic]; ++p )
>+                if ( __ioapic_read_entry(apic, p, TRUE).vector == vector )
>+                {
>+                    pin = p;
>+                    break;

While we seem to agree that sharing of vectors within an IO-APIC must
be prevented, I don't think this is currently being enforced, and hence
you can't just "break" here - you need to handle all matching pins.

>+                }
>+            
>+            /* If search fails, nothing to do */
>+            if ( pin == -1 )
>+                return;
>+        }
>+
>+        /* If vector is unknown, read it from the IO-APIC */
>+        if ( vector == -1 )
>+            vector = __ioapic_read_entry(apic, pin, TRUE).vector;

You don't seem to use vector further down.

>+
>+        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);
>+        }
>+    }
>+}
>diff -r 0268e7380953 xen/include/asm-x86/io_apic.h
>--- a/xen/include/asm-x86/io_apic.h	Mon Sep 05 15:10:28 2011 +0100
>+++ b/xen/include/asm-x86/io_apic.h	Fri Sep 09 15:58:59 2011 +0100
>@@ -157,10 +157,13 @@ static inline void io_apic_write(unsigne
> 	__io_apic_write(apic, reg, value);
> }
> 
>-static inline void io_apic_eoi(unsigned int apic, unsigned int vector)
>-{
>-	*(IO_APIC_BASE(apic)+16) = vector;
>-}
>+#define ioapic_has_eoi_reg(apic) (mp_ioapics[(apic)].mpc_apicver >= 0x20)

Is this used outside of io_apic.c?

>+
>+void __io_apic_eoi(unsigned int apic, unsigned int vector, unsigned int pin);
>+void io_apic_eoi(unsigned int apic, unsigned int vector, unsigned int pin);
>+
>+#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))

None of these should be either (see also above).

Jan

> 
> /*
>  * Re-write a value: to be used for read-modify-write