[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] x86/IRQ: prevent vector sharing within IO-APICs
>>> 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.) 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) * @@ -220,14 +216,11 @@ static void ioapic_write_entry( * 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 ) @@ -1645,7 +1602,6 @@ static void mask_and_ack_level_ioapic_ir static void end_level_ioapic_irq(struct irq_desc *desc, u8 vector) { unsigned long v; - int i; if ( !ioapic_ack_new ) { @@ -1689,15 +1645,9 @@ static void end_level_ioapic_irq(struct * operation to prevent an edge-triggered interrupt escaping meanwhile. * The idea is from Manfred Spraul. --macro */ - i = desc->arch.vector; - /* 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); - } + if ( vector && desc->arch.vector != vector ) + 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |