From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] x86/IRQ: prevent vector sharing within IO-APICs Date: Mon, 14 Nov 2011 13:59:07 +0000 Message-ID: <4EC11EAB.80400@citrix.com> References: <4EBD54B80200007800060776@nat28.tlf.novell.com> <4EBD579F.70901@citrix.com> <4EC0E6010200007800060B29@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <4EC0E6010200007800060B29@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Jan Beulich Cc: "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org On 14/11/11 08:57, Jan Beulich wrote: >>>> On 11.11.11 at 18:13, Andrew Cooper 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 >> 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