4.0/4.1 requests

4.0/4.1 requests

[Jan Beulich]

Hi Keir,

without the old IO-APIC part addressed, I wonder whether we should
really have the backport of 23805:7048810180de ("IRQ: manually EOI
migrating line interrupts") in both trees.

I'd also like you to add 23820:ba75234a6f56 ("bitmap_scnlistprintf()
should always zero-terminate its output buffer") to 4.0 (I see it's
already in 4.1), as this not being fixed confuses 'e' debug key output.

Jan

Re: 4.0/4.1 requests

[Jan Beulich]

>>> On 08.09.11 at 11:19, "Jan Beulich" <JBeulich@suse.com> wrote:
> Hi Keir,
> 
> without the old IO-APIC part addressed, I wonder whether we should
> really have the backport of 23805:7048810180de ("IRQ: manually EOI
> migrating line interrupts") in both trees.
> 
> I'd also like you to add 23820:ba75234a6f56 ("bitmap_scnlistprintf()
> should always zero-terminate its output buffer") to 4.0 (I see it's
> already in 4.1), as this not being fixed confuses 'e' debug key output.

Also shouldn't we have a backport of 23112:84e3706df07a ("Passthrough:
disable bus-mastering on any card that causes an IOMMU fault.") in the
4.0 tree (even if it doesn't fully address the problem, yet - that's the
case in the other trees too)?

Jan

Re: 4.0/4.1 requests

[Keir Fraser]

On 08/09/2011 11:12, "Jan Beulich" <JBeulich@suse.com> wrote:

>>>> On 08.09.11 at 11:19, "Jan Beulich" <JBeulich@suse.com> wrote:
>> Hi Keir,
>> 
>> without the old IO-APIC part addressed, I wonder whether we should
>> really have the backport of 23805:7048810180de ("IRQ: manually EOI
>> migrating line interrupts") in both trees.
>> 
>> I'd also like you to add 23820:ba75234a6f56 ("bitmap_scnlistprintf()
>> should always zero-terminate its output buffer") to 4.0 (I see it's
>> already in 4.1), as this not being fixed confuses 'e' debug key output.
> 
> Also shouldn't we have a backport of 23112:84e3706df07a ("Passthrough:
> disable bus-mastering on any card that causes an IOMMU fault.") in the
> 4.0 tree (even if it doesn't fully address the problem, yet - that's the
> case in the other trees too)?

Now done.

 -- Keir

> Jan
> 

Re: 4.0/4.1 requests

[Keir Fraser]

On 08/09/2011 10:19, "Jan Beulich" <JBeulich@suse.com> wrote:

> Hi Keir,
> 
> without the old IO-APIC part addressed, I wonder whether we should
> really have the backport of 23805:7048810180de ("IRQ: manually EOI
> migrating line interrupts") in both trees.

It does fix a real bug. Perhaps Andrew could hack up a patch to make it
dependent on IO-APIC version?

> I'd also like you to add 23820:ba75234a6f56 ("bitmap_scnlistprintf()
> should always zero-terminate its output buffer") to 4.0 (I see it's
> already in 4.1), as this not being fixed confuses 'e' debug key output.

Ok.

 -- Keir

> Jan
> 

Re: 4.0/4.1 requests

[Andrew Cooper]

On 08/09/11 11:17, Keir Fraser wrote:
> On 08/09/2011 10:19, "Jan Beulich" <JBeulich@suse.com> wrote:
>
>> Hi Keir,
>>
>> without the old IO-APIC part addressed, I wonder whether we should
>> really have the backport of 23805:7048810180de ("IRQ: manually EOI
>> migrating line interrupts") in both trees.
> It does fix a real bug. Perhaps Andrew could hack up a patch to make it
> dependent on IO-APIC version?
Now that I am not racing against a release deadline, this is a
possibility.  It probably means falling through to the "fake an EOI for
line level interrupt code", as the Status bit is RO in the IO-APIC (The
IRR bit is RW but both need updating)

Does anyone know which revision of the IO-APIC was the first with an EOI
register?

If not, I think I have some document trawling to do.

>> I'd also like you to add 23820:ba75234a6f56 ("bitmap_scnlistprintf()
>> should always zero-terminate its output buffer") to 4.0 (I see it's
>> already in 4.1), as this not being fixed confuses 'e' debug key output.
> Ok.
>
>  -- Keir
>
>> Jan
>>
>

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

Re: 4.0/4.1 requests

[Keir Fraser]

On 08/09/2011 11:48, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:

>>> Hi Keir,
>>> 
>>> without the old IO-APIC part addressed, I wonder whether we should
>>> really have the backport of 23805:7048810180de ("IRQ: manually EOI
>>> migrating line interrupts") in both trees.
>> It does fix a real bug. Perhaps Andrew could hack up a patch to make it
>> dependent on IO-APIC version?
> Now that I am not racing against a release deadline, this is a
> possibility.  It probably means falling through to the "fake an EOI for
> line level interrupt code", as the Status bit is RO in the IO-APIC (The
> IRR bit is RW but both need updating)

Falling back to the old behaviour would be acceptable. I think we're talking
only very old systems here.

> Does anyone know which revision of the IO-APIC was the first with an EOI
> register?

Jan's patches which use the IOAPIC EOI register have a version check. You
can copy that.

 -- Keir

> If not, I think I have some document trawling to do.

Re: 4.0/4.1 requests

[Jan Beulich]

>>> On 08.09.11 at 12:48, Andrew Cooper <andrew.cooper3@citrix.com> wrote:

> 
> On 08/09/11 11:17, Keir Fraser wrote:
>> On 08/09/2011 10:19, "Jan Beulich" <JBeulich@suse.com> wrote:
>>
>>> Hi Keir,
>>>
>>> without the old IO-APIC part addressed, I wonder whether we should
>>> really have the backport of 23805:7048810180de ("IRQ: manually EOI
>>> migrating line interrupts") in both trees.
>> It does fix a real bug. Perhaps Andrew could hack up a patch to make it
>> dependent on IO-APIC version?
> Now that I am not racing against a release deadline, this is a
> possibility.  It probably means falling through to the "fake an EOI for
> line level interrupt code", as the Status bit is RO in the IO-APIC (The
> IRR bit is RW but both need updating)
> 
> Does anyone know which revision of the IO-APIC was the first with an EOI
> register?

As Keir said, code already exists which does this version check. But
to answer explicitly: It's version 0x20.

Jan

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

[Andrew Cooper]

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

--- SNIP ---

Attached is my first attempt at expanding the EOI to work on older
IO-APICs.  It has not undergone any significant testing yet.

Does this look suitable?

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


[-- Attachment #2: IO-APIC-eoi.patch --]
[-- Type: text/x-patch, Size: 4212 bytes --]

diff -r 0268e7380953 xen/arch/x86/io_apic.c
--- a/xen/arch/x86/io_apic.c	Mon Sep 05 15:10:28 2011 +0100
+++ b/xen/arch/x86/io_apic.c	Thu Sep 08 14:17:31 2011 +0100
@@ -357,7 +357,7 @@ static void __eoi_IO_APIC_irq(unsigned i
         pin = entry->pin;
         if (pin == -1)
             break;
-        io_apic_eoi(entry->apic, vector);
+        io_apic_eoi_vector(entry->apic, vector);
         if (!entry->next)
             break;
         entry = irq_2_pin + entry->next;
@@ -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_pin(apic, pin);
     }
 
     /*
@@ -1750,7 +1739,7 @@ static void end_level_ioapic_irq (unsign
     {
         int ioapic;
         for (ioapic = 0; ioapic < nr_ioapics; ioapic++)
-            io_apic_eoi(ioapic, i);
+            io_apic_eoi_vector(ioapic, i);
     }
 
     v = apic_read(APIC_TMR + ((i & ~0x1f) >> 1));
@@ -2622,3 +2611,67 @@ 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 given the vector it points to */
+void io_apic_eoi_vector(unsigned int apic, unsigned int vector)
+{
+    unsigned int flags;
+    
+    spin_lock_irqsave(&ioapic_lock, flags);
+
+    if ( ioapic_has_eoi_reg(apic) )
+        /* Prefer the use of the EOI register if available */
+        *(IO_APIC_BASE(apic)+16) = vector;
+    else
+    {
+        /* Else search through the IO-APIC entries to find the
+         * correct pin */
+        struct IO_APIC_route_entry entry;
+        unsigned int pin = 0;
+
+        do
+        {
+            entry = __ioapic_read_entry(apic, pin, TRUE);
+
+            if ( entry.vector == vector )
+            {
+                /* And if found, fake an EOI by flipping 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);
+                break;
+            }
+
+        } while ( pin++ < nr_ioapic_registers[apic] );
+    }
+
+    spin_unlock_irqrestore(&ioapic_lock, flags);
+}
+
+/* EOI an IO-APIC entry given its pin */
+void io_apic_eoi_pin(unsigned int apic, unsigned int pin)
+{
+    struct IO_APIC_route_entry entry;
+    unsigned int flags;
+
+    entry = __ioapic_read_entry(apic, pin, TRUE);
+
+    spin_lock_irqsave(&ioapic_lock, flags);
+
+    if ( ioapic_has_eoi_reg(apic) )
+        /* Prefer the use of the EOI register if available */
+        *(IO_APIC_BASE(apic)+16) = entry.vector;
+    else
+    {
+        /* Else fake an EOI by switching to edge triggered mode
+         * and back */
+        entry.trigger = 0;
+        __ioapic_write_entry(apic, pin, TRUE, entry);
+        entry.trigger = 1;
+        __ioapic_write_entry(apic, pin, TRUE, entry);
+    }
+
+    spin_unlock_irqrestore(&ioapic_lock, flags);
+}
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	Thu Sep 08 14:17:31 2011 +0100
@@ -157,10 +157,9 @@ 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)
+void io_apic_eoi_vector(unsigned int apic, unsigned int vector);
+void io_apic_eoi_pin(unsigned int apic, unsigned int pin);
 
 /*
  * Re-write a value: to be used for read-modify-write

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

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

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

[Jan Beulich]

>>> On 08.09.11 at 15:18, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> --- SNIP ---
> 
> Attached is my first attempt at expanding the EOI to work on older
> IO-APICs.  It has not undergone any significant testing yet.
> 
> Does this look suitable?

It looks correct, but I'd prefer not having two io_apic_eoi_*() functions:
Namely in the "normal" __eoi_IO_APIC_irq() case you have pin *and*
vector readily available, and hence there is no need to look up anything.
So perhaps a better option would be to pass both pin and vector to
io_apic_eoi() (using e.g. -1 to identify the "unknown-needs-lookup"
case).

Jan

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

[Andrew Cooper]

On 08/09/11 14:40, Jan Beulich wrote:
>>>> On 08.09.11 at 15:18, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> --- SNIP ---
>>
>> Attached is my first attempt at expanding the EOI to work on older
>> IO-APICs.  It has not undergone any significant testing yet.
>>
>> Does this look suitable?
> It looks correct, but I'd prefer not having two io_apic_eoi_*() functions:
> Namely in the "normal" __eoi_IO_APIC_irq() case you have pin *and*
> vector readily available, and hence there is no need to look up anything.
> So perhaps a better option would be to pass both pin and vector to
> io_apic_eoi() (using e.g. -1 to identify the "unknown-needs-lookup"
> case).
>
> Jan

Ok - I will refactor with that suggestion.

Also, the code in end_level_ioapic_irq() suggests masking the entry
while playing with it, so I will include that as well.

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

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

[Andrew Cooper]

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

---SNIP---

v2 of the EOI patch.

This provides one function to perform EOI'ing, taking a vector or a pin
(or both).

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


[-- Attachment #2: IO-APIC-eoi.patch --]
[-- Type: text/x-patch, Size: 5238 bytes --]

diff -r 0268e7380953 xen/arch/x86/io_apic.c
--- 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 15:58:59 2011 +0100
@@ -357,7 +357,7 @@ static void __eoi_IO_APIC_irq(unsigned i
         pin = entry->pin;
         if (pin == -1)
             break;
-        io_apic_eoi(entry->apic, vector);
+        __io_apic_eoi(entry->apic, vector, pin);
         if (!entry->next)
             break;
         entry = irq_2_pin + entry->next;
@@ -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);
     }
 
     /*
@@ -1750,7 +1739,7 @@ static void end_level_ioapic_irq (unsign
     {
         int ioapic;
         for (ioapic = 0; ioapic < nr_ioapics; ioapic++)
-            io_apic_eoi(ioapic, i);
+            io_apic_eoi_vector(ioapic, i);
     }
 
     v = apic_read(APIC_TMR + ((i & ~0x1f) >> 1));
@@ -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)
+{
+    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)
+{
+    ASSERT( spin_is_locked(&ioapic_lock) );
+    ASSERT( !local_irq_is_enabled() );
+
+    /* 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 )
+                if ( __ioapic_read_entry(apic, p, TRUE).vector == vector )
+                {
+                    pin = p;
+                    break;
+                }
+            
+            /* 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;
+
+        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)
+
+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))
 
 /*
  * Re-write a value: to be used for read-modify-write

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

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

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

[Jan Beulich]

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

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

[Andrew Cooper]

On 09/09/11 16:39, Jan Beulich wrote:
>>>> 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.

Questionable - I traced the calls and at this point and cant see the
ioapic lock being taken.  I guess it might be safer overall to use
non-locking and leave the problem to whoever cleans up the irq code...

>>     }
>>
>>     /*
>> ...
>> @@ -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.

There is a BUG_ON at the top of the function if both vector and pin are -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.

Good point - I will leave a comment to remove it when fixed.

>> +                }
>> +            
>> +            /* 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.

So I dont.

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

Not according to cscope - I will adjust them appropriately.

>> +
>> +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
-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com

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

[Jan Beulich]

>>> On 09.09.11 at 17:55, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 09/09/11 16:39, Jan Beulich wrote:
>>>>> 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.
> 
> Questionable - I traced the calls and at this point and cant see the
> ioapic lock being taken.  I guess it might be safer overall to use
> non-locking and leave the problem to whoever cleans up the irq code...

It indeed is not being taken, and as you say this is deliberate (we may be
cleaning up in a crashed environment here).

>>> +    {
>>> +        /* 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.
> 
> There is a BUG_ON at the top of the function if both vector and pin are -1.

Sorry, I must have missed that.

Jan

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

[Andrew Cooper]

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

---SNIP---

Version 3.  Applied Jan's suggestions, and extended some of the comments.

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


[-- Attachment #2: IO-APIC-eoi.patch --]
[-- Type: text/x-patch, Size: 7217 bytes --]

diff -r 0268e7380953 xen/arch/x86/io_apic.c
--- 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);
+
+#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)
  *
@@ -357,7 +367,7 @@ static void __eoi_IO_APIC_irq(unsigned i
         pin = entry->pin;
         if (pin == -1)
             break;
-        io_apic_eoi(entry->apic, vector);
+        __io_apic_eoi(entry->apic, vector, pin);
         if (!entry->next)
             break;
         entry = irq_2_pin + entry->next;
@@ -397,18 +407,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);
     }
 
     /*
@@ -1750,7 +1749,7 @@ static void end_level_ioapic_irq (unsign
     {
         int ioapic;
         for (ioapic = 0; ioapic < nr_ioapics; ioapic++)
-            io_apic_eoi(ioapic, i);
+            io_apic_eoi_vector(ioapic, i);
     }
 
     v = apic_read(APIC_TMR + ((i & ~0x1f) >> 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 
+                     * 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. */
+
+                    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);
+        }
+    }
+}
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 17:20:49 2011 +0100
@@ -157,11 +157,6 @@ 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;
-}
-
 /*
  * Re-write a value: to be used for read-modify-write
  * cycles where the read already set up the index register.

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

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

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

[Andrew Cooper]

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

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?

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


[-- Attachment #2: IO-APIC-eoi.patch --]
[-- Type: text/x-patch, Size: 7217 bytes --]

diff -r 0268e7380953 xen/arch/x86/io_apic.c
--- 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);
+
+#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)
  *
@@ -357,7 +367,7 @@ static void __eoi_IO_APIC_irq(unsigned i
         pin = entry->pin;
         if (pin == -1)
             break;
-        io_apic_eoi(entry->apic, vector);
+        __io_apic_eoi(entry->apic, vector, pin);
         if (!entry->next)
             break;
         entry = irq_2_pin + entry->next;
@@ -397,18 +407,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);
     }
 
     /*
@@ -1750,7 +1749,7 @@ static void end_level_ioapic_irq (unsign
     {
         int ioapic;
         for (ioapic = 0; ioapic < nr_ioapics; ioapic++)
-            io_apic_eoi(ioapic, i);
+            io_apic_eoi_vector(ioapic, i);
     }
 
     v = apic_read(APIC_TMR + ((i & ~0x1f) >> 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 
+                     * 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. */
+
+                    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);
+        }
+    }
+}
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 17:20:49 2011 +0100
@@ -157,11 +157,6 @@ 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;
-}
-
 /*
  * Re-write a value: to be used for read-modify-write
  * cycles where the read already set up the index register.

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

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

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

[Andrew Cooper]

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

On 09/09/11 17:47, Andrew Cooper 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?
>
Remember to qrefresh this time.

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


[-- Attachment #2: IO-APIC-eoi.patch --]
[-- Type: text/x-patch, Size: 7216 bytes --]

diff -r 0268e7380953 xen/arch/x86/io_apic.c
--- 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:48:33 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);
+
+#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)
  *
@@ -357,7 +367,7 @@ static void __eoi_IO_APIC_irq(unsigned i
         pin = entry->pin;
         if (pin == -1)
             break;
-        io_apic_eoi(entry->apic, vector);
+        __io_apic_eoi(entry->apic, vector, pin);
         if (!entry->next)
             break;
         entry = irq_2_pin + entry->next;
@@ -397,18 +407,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);
     }
 
     /*
@@ -1750,7 +1749,7 @@ static void end_level_ioapic_irq (unsign
     {
         int ioapic;
         for (ioapic = 0; ioapic < nr_ioapics; ioapic++)
-            io_apic_eoi(ioapic, i);
+            io_apic_eoi_vector(ioapic, i);
     }
 
     v = apic_read(APIC_TMR + ((i & ~0x1f) >> 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 
+                     * 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. */
+
+                    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);
+        }
+    }
+}
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 17:48:33 2011 +0100
@@ -157,11 +157,6 @@ 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;
-}
-
 /*
  * Re-write a value: to be used for read-modify-write
  * cycles where the read already set up the index register.

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

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

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

[Jan Beulich]

>>> 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);
>+        }
>+    }
>+}
>...

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

[Andrew Cooper]

On 12/09/11 07:50, Jan Beulich wrote:
>>>> 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()).

Ok

>> +
>> +#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 I call the function recursively, it will loop forever.  Anyway, the
need to clear multiple pins is only temorary until George finishes his
per-device AMD interrupt remap patch which will enforce vector
uniqueness in each IO-APIC.  My expectation is that this issue will be
fixed in the next few weeks.

>> +                    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);
>> +        }
>> +    }
>> +}
>> ...

I will formally submit the patch for inclusion now.

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

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

[Jan Beulich]

>>> On 12.09.11 at 12:15, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 12/09/11 07:50, Jan Beulich wrote:
>>>>> On 09.09.11 at 18:47, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>> +                     * 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 I call the function recursively, it will loop forever.  Anyway, the

Why would it loop forever? You get in here only with pin == -1, and
for the recursive call you'd pass the pin number you determined.

> need to clear multiple pins is only temorary until George finishes his
> per-device AMD interrupt remap patch which will enforce vector
> uniqueness in each IO-APIC.  My expectation is that this issue will be
> fixed in the next few weeks.

Sure.

Jan

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

[Keir Fraser]

On 12/09/2011 11:23, "Jan Beulich" <JBeulich@suse.com> wrote:

>>>> On 12.09.11 at 12:15, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 12/09/11 07:50, Jan Beulich wrote:
>>>>>> On 09.09.11 at 18:47, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>>> +                     * 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 I call the function recursively, it will loop forever.  Anyway, the
> 
> Why would it loop forever? You get in here only with pin == -1, and
> for the recursive call you'd pass the pin number you determined.

Exactly. Please re-send the patch with the recursive call. It makes the
function quite a bit shorter.

 -- Keir

>> need to clear multiple pins is only temorary until George finishes his
>> per-device AMD interrupt remap patch which will enforce vector
>> uniqueness in each IO-APIC.  My expectation is that this issue will be
>> fixed in the next few weeks.
> 
> Sure.
> 
> Jan
>