Re: [PATCH] x86/IRQ: prevent vector sharing within IO-APICs

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 14/11/11 08:57, Jan Beulich wrote:
>>>> On 11.11.11 at 18:13, Andrew Cooper <andrew.cooper3@citrix.com> 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@suse.com>
>> 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.

Having had the weekend to consider the logic in your patch, I now think
I was wrong, and your reasoning is correct.  Therefore, Ack.

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