From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] x86: fix ordering of operations in destroy_irq() Date: Thu, 30 May 2013 18:22:19 +0100 Message-ID: <51A78ACB.7050105@citrix.com> References: <51A5C33A02000078000D974A@nat28.tlf.novell.com> <51A77CEB.6030409@eu.citrix.com> <51A78F9D020000780009B71B@nat28.tlf.novell.com> <51A783A3.6080703@eu.citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <51A783A3.6080703@eu.citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: George Dunlap Cc: "Keir (Xen.org)" , Jan Beulich , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 30/05/2013 17:51, George Dunlap wrote: > On 05/30/2013 05:42 PM, Jan Beulich wrote: >>>>> George Dunlap 05/30/13 6:23 PM >>> >>> On 05/29/2013 07:58 AM, Jan Beulich wrote: >>>> The fix for XSA-36, switching the default of vector map management to >>>> be per-device, exposed more readily a problem with the cleanup of these >>>> vector maps: dynamic_irq_cleanup() clearing desc->arch.used_vectors >>>> keeps the subsequently invoked clear_irq_vector() from clearing the >>>> bits for both the in-use and a possibly still outstanding old vector. >>>> >>>> Fix this by folding dynamic_irq_cleanup() into destroy_irq(), which was >>>> its only caller, deferring the clearing of the vector map pointer until >>>> after clear_irq_vector(). >>>> >>>> Once at it, also defer resetting of desc->handler until after the loop >>>> around smp_mb() checking for IRQ_INPROGRESS to be clear, fixing a >>>> (mostly theoretical) issue with the intercation with do_IRQ(): If we >>>> don't defer the pointer reset, do_IRQ() could, for non-guest IRQs, call >>>> ->ack() and ->end() with different ->handler pointers, potentially >>>> leading to an IRQ remaining un-acked. The issue is mostly theoretical >>>> because non-guest IRQs are subject to destroy_irq() only on (boot time) >>>> error paths. >>>> >>>> As to the changed locking: Invoking clear_irq_vector() with desc->lock >>>> held is okay because vector_lock already nests inside desc->lock (proven >>>> by set_desc_affinity(), which takes vector_lock and gets called from >>>> various desc->handler->ack implementations, getting invoked with >>>> desc->lock held). >>>> >>>> Reported-by: Andrew Cooper >>>> Signed-off-by: Jan Beulich >>> How big of an impact is this bug? How many people are actually affected >>> by it? >> Andrew will likely be able to give you more precise info on this, but this >> fixes a problem observed in practice. Any AMD system with IOMMU would >> be affected. >> >>> It's a bit hard for me to tell from the description, but it looks like >>> it's code motion, then some "theoretical" issues. >> No, the description is pretty precise here: It fixes an actual issue and, >> along the way, also a theoretical one. >> >>> Is the improvement this patch represents worth the potential risk of >>> bugs at this point? >> I think so - otherwise it would need to be backported right away after the >> release. > Right -- then if you could also commit this tomorrow, it will get the > best testing we can give it. :-) > > Acked-by: George Dunlap > > -George As for the impact without this patch. Following XSA-36, any AMD boxes with IOMMUs will either fail with an ASSERT() or incorrectly program their interrupt remapping tables after you map/unmap MSI/MSI-X irqs a few times. Basically, the desc->arch.used_vectors steadily accumulated history until an ASSERT(!test_bit ...) failed. This is because the per-device vector table code was broken right from the go, but wasn't observed as global was the default. ~Andrew