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] [PATCH] x86/IRQ: prevent vector sharing within IO-APICs

>>> On 14.11.11 at 15:20, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>>>> On 14.11.11 at 14:59, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 14/11/11 08:57, Jan Beulich wrote:
>>>>>> On 11.11.11 at 18:13, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> On 11/11/11 16:00, Jan Beulich wrote:
>>>>> Following the prevention of vector sharing for MSIs, this change
>>>>> enforces the same within IO-APICs: Pin based interrupts use the IO-APIC
>>>>> as their identifying device under the AMD IOMMU (and just like for
>>>>> MSIs, only the identifying device is used to remap interrupts here,
>>>>> with no regard to an interrupt's destination).
>>>>>
>>>>> Additionally, LAPIC initiated EOIs (for level triggered interrupts) too
>>>>> use only the vector for identifying which interrupts to end. While this
>>>>> generally causes no significant problem (at worst an interrupt would be
>>>>> re-raised without a new interrupt event actually having occurred)
>>>> At worst, hardware asserts a line interrupt, deasserts it later, and an
>>>> EOI broadcast gets rid of any record that the IRQ was ever raised. 
>>>> While I would classify this as buggy behavior, I believe I have seen
>>>> some hardware doing this when investigating the line level IRQ migration
>>>> bug, as clearing the IRR did not immediately cause another interrupt to
>>>> be generated.
>>>>
>>>>> , it
>>>>> still seems better to avoid the situation.
>>>>>
>>>>> For this second aspect, a distinction is being made between the
>>>>> traditional and the directed-EOI cases: In the former, vectors should
>>>>> not be shared throughout all IO-APICs in the system, while in the
>>>>> latter case only individual IO-APICs need to be contrained (or, if the
>>>>> firmware indicates so, sub- groups of them having the same GSI appear
>>>>> at multiple pins).
>>>>>
>>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>> Provisional nack because it is my understanding that under all
>>>> circumstances, you must maintain a vector exclusivity map across all
>>>> IO-APICs because of the broadcast problem.  Or have I made a mistake in
>>>> my reasoning?
>>> With directed EOI there's no broadcasting involved, which is why
>>> global sharing prevention is not necessary.
>>>
>>> However, after some more thinking over the weekend I think we need
>>> to also/first adjust end_level_ioapic_irq()'s call to io_apic_eoi_vector():
>>> It shouldn't really iterate over all IO-APICs, but instead call
>>> eoi_IO_APIC_irq(). Thoughts? (That would also eliminate the need to
>>> look up pin or vector in __io_apic_eoi(), as all remaining call sites pass
>>> both.)
>>>
>>> Jan
>>>
>> 
>> I believe that should work.  By the point end_level_ioapic_irq() is
>> called, all the irq_desc information should point to the new vector, so
>> eoi_IO_APIC_irq() should get it correct.  At the time I made that patch,
>> I was not so familiar with the IO-APIC code so decided that calling
>> io_apic_eoi was the safer bet.
> 
> I'm having some more fundamental problem with this original change
> of yours: The assignment of the new vector happens in the context
> of move_native_irq(), which gets called *after* doing the manual
> EOI (and hence also *after* the vector != desc->arch.vector check).
> How does that do what it is supposed to?
> 
> (Below the [untested] patch that I would propose to do what I described
> above, pending your clarification regarding the original change.)

Here is one that actually compiles (and does even more cleanup, as
pointed out by the compiler).

Jan

--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -69,10 +69,6 @@ int __read_mostly nr_ioapics;
 
 #define ioapic_has_eoi_reg(apic) (mp_ioapics[(apic)].mpc_apicver >= 0x20)
 
-#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)
  *
@@ -213,21 +209,18 @@ static void ioapic_write_entry(
     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
+/* EOI an IO-APIC entry.  Vector may be -1, indicating that it should be
+ * worked out using the pin.  This function expects that the ioapic_lock is
+ * being held, 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 )
+        if ( vector == IRQ_VECTOR_UNASSIGNED )
             vector = __ioapic_read_entry(apic, pin, TRUE).vector;
 
         *(IO_APIC_BASE(apic)+16) = vector;
@@ -239,42 +232,6 @@ static void __io_apic_eoi(unsigned int a
         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_entries[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. */
-                    __io_apic_eoi(apic, vector, p);
-                }
-            }
-            
-            /* 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 )
@@ -301,17 +258,6 @@ static void __io_apic_eoi(unsigned int a
     }
 }
 
-/* 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);
-}
-
 /*
  * Saves all the IO-APIC RTE's
  */
@@ -1693,11 +1639,7 @@ static void end_level_ioapic_irq(struct 
 
     /* Manually EOI the old vector if we are moving to the new */
     if ( vector && i != vector )
-    {
-        int ioapic;
-        for (ioapic = 0; ioapic < nr_ioapics; ioapic++)
-            io_apic_eoi_vector(ioapic, i);
-    }
+        eoi_IO_APIC_irq(desc);
 
     v = apic_read(APIC_TMR + ((i & ~0x1f) >> 1));
 


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