Re: RE: [PATCH] [RFC] VT-d: always clean up dpci timers.

[Tim Deegan <Tim.Deegan@citrix.com>]

At 10:01 +0100 on 21 Jul (1311242513), Keir Fraser wrote:
> On 21/07/2011 09:50, "Tim Deegan" <Tim.Deegan@citrix.com> 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 <Tim.Deegan@citrix.com>

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;
 }