Re: [PATCH] On unmap, flush IOMMU TLB and return correct size

[Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

On Wed, 2013-09-25 at 23:15 +0100, David Woodhouse wrote:
> On Wed, 2013-09-25 at 15:46 -0600, Alex Williamson wrote:
> > On Wed, 2013-09-25 at 21:11 +0100, David Woodhouse wrote:
> > > I wouldn't bother to go looking for opportunities to use super pages if
> > > we remove the last non-SP-capable IOMMU from the domain.
> > 
> > I predict bugs getting filed if a guest sees a performance hit after
> > adding a device that is not restored when the device is removed.  If
> > only we could assume a similar feature set among IOMMUs in a system.
> 
> Ok, fine. As long as you don't have *too* much mapped, that shouldn't
> suck too much.

Of course if we knew this was going to happen in advance of attaching
the new device, I wonder if we might opt to manage it with a separate
IOMMU domain.

> > > > > FWIW we currently screw up the handling of cache-coherent vs.
> > > > > non-coherent page tables too. That one wants a wbinvd somewhere when we
> > > > > add a non-coherent IOMMU to the domain.
> > > > 
> > > > You're not selling the "trust the IOMMU driver" story very well here.
> > > > Can we assume that the IOMMU_CACHE flag (SNP) is ignored appropriately
> > > > by non-coherent IOMMUs?  Is there any downside to ignoring it and always
> > > > setting SNP in the IOMMU page tables?  AMD IOMMU ignores it, but it's
> > > > also always cache coherent.  Thanks,
> > > 
> > > SNP is a separate issue. I'm speaking of cache coherency of the hardware
> > > page table walk — the feature bit that all the horrid clflush calls are
> > > predicated on.
> > > 
> > > Again, this is just a bug. We *should* be getting this right, but don't
> > > yet.
> > 
> > And for DMA_PTE_SNP?  intel_iommu_map() won't let us set this bit if the
> > domain contains a hardware unit that doesn't support ecap.SC, but it
> > also doesn't update existing mappings.  Barring hardware bugs, it seems
> > much easier to unconditionally set DMA_PTE_SNP but still advertise
> > IOMMU_CAP_CACHE_COHERENCY based on the composition of the domain.
> > Otherwise we have to reject adding devices to a domain that change the
> > coherency once DMA mappings are in play or never set IOMMU_CACHE and
> > advertise to KVM that the domain is always non-coherent.  Thanks,
> 
> It's not that the domain is non-coherent, is it? The SNP bit allows you
> to *force* a PCIe DMA request to snoop the CPU cache, even if it didn't
> *request* that.
> 
> If your device is actually *trying* to do cache-coherent (i.e. snooping)
> DMA, surely that works regardless of the DMA_PTE_SNP setting?
> 
> Or am I missing something?

It's the opposite, devices are trying to do No-Snoop transactions and
KVM would really rather prefer wbinvd was a no-op since emulated devices
don't need it.  We therefore need to toggle KVM's behavior when we have
an assigned device attached to an IOMMU domain where the hardware isn't
stripping the No-Snoop attribute.  Some graphics cards seem to make use
of this and will get error codes loading their driver if it doesn't
work.  Thanks,

Alex

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu