From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tim Deegan Subject: Re: RE: [PATCH] [RFC] VT-d: always clean up dpci timers. Date: Mon, 25 Jul 2011 15:21:20 +0100 Message-ID: <20110725142120.GD8970@whitby.uk.xensource.com> References: <20110721085047.GB2688@whitby.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Keir Fraser Cc: "xen-devel@lists.xensource.com" , "Kay, Allen M" List-Id: xen-devel@lists.xenproject.org At 10:01 +0100 on 21 Jul (1311242513), Keir Fraser wrote: > On 21/07/2011 09:50, "Tim Deegan" wrote: > > > At 18:08 -0700 on 20 Jul (1311185311), Kay, Allen M wrote: > >> Hi Tim, > >> > >> Can you provide the code flow that can cause this failure? > >> > >> In pci_release_devices(), pci_clean_dpci_irqs() is called before > >> "d->need_iommu = 0" in deassign_device(). If this path is taken, then > >> it should not return from !need_iommu(d) in pci_clean_dpci_irqs(). > > > > The problem is that the xl toolstack has already deassigned the domain's > > devices, using a hypercall to invoke deassign_device(), so by the time > > the domain is destroyed, pci_release_devices() can't tell that it once > > had a PCI device passed through. > > > > It seems like the Right Thing[tm] would be for deassign_device() to find > > and undo the relevant IRQ plumbing but I couldn't see how. Is there a > > mapping from bdf to irq in the iommu code or are they handled entirely > > separately? > > Could we make need_iommu(d) sticky? Being able to clear it doesn't seem an > important case (such a domain is probably being torn down anyway) and > clearly it can lead to fragility. The fact that presumably we'd end up doing > unnecessary IOMMU PT work for the remaining lifetime of the domain doesn't > seem a major downside to me. If you prefer it that way. TBH I think I prefer the other way though: things that gate on need_iommu() should be cleaned up by iommu->teardown(). --8<---- PCI passthrough: don't tear down IOMMU when the last device is deassigned. Otherwise there's a risk that not all iommu-related state will get torn down during domain destruction. Signed-off-by: Tim Deegan diff -r 9dbbf1631193 xen/drivers/passthrough/iommu.c --- a/xen/drivers/passthrough/iommu.c Mon Jul 25 14:21:13 2011 +0100 +++ b/xen/drivers/passthrough/iommu.c Mon Jul 25 15:14:31 2011 +0100 @@ -296,12 +296,6 @@ int deassign_device(struct domain *d, u8 return ret; } - if ( !has_arch_pdevs(d) && need_iommu(d) ) - { - d->need_iommu = 0; - hd->platform_ops->teardown(d); - } - return ret; }