Re: [Qemu-devel] [PATCH v4 RESEND 0/3] IOMMU: intel_iommu support map and unmap notifications

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2815 bytes --]

On Fri, Oct 21, 2016 at 01:17:08PM +0800, Peter Xu wrote:
> On Fri, Oct 21, 2016 at 11:50:05AM +1100, David Gibson wrote:
> 
> [...]
> 
> > > > In my setup the VFIO registered two memory areas with one page of
> > > > unregistered memory
> > > > between them.
> > > > 
> > > > When I'm calling memory_region_notify_iommu it calls the notifier function
> > > > of VFIO twice
> > > > when the second time is failing with warning to console as the new mapping
> > > > is already present.
> > > > 
> > > > The notifier function of VFIO should ignore IOMMUTLBEntry that is not in
> > > > the correct
> > > > range.
> > > 
> > > Hmm, right vfio_listener_region_add() is called for a
> > > MemoryRegionSection, but then we add an iommu notifier to the
> > > MemoryRegion, so we end up with a notifier per MemoryRegionSection
> > > regardless of whether they're backed by the same MemoryRegion.  Seems
> > > like we need a MemoryRegion based list of VFIOGuestIOMMUs so we only
> > > register once per MemoryRegion and then sort though the list of
> > > VFIOGuestIOMMUs for a given MemoryRegion to find the one affected.
> > > David, does that sound right to you?
> 
> I think we already have such a list (VFIOContainer.giommu_list)?  Can
> we use that to do it? When we try to add a new IOMMU notifier for
> specific MemoryRegion, we can first traverse VFIOContainer.giommu_list
> and see whether there are existing MemoryRegion registered, and we
> only register if it's the first one.

Yes, I think that would work, but I still prefer the alternate
approach I describe below.

> > Well, I think that would work.  But I think it would be better to fix
> > it from the other side:
> > 
> > We add the range to be notified into the IOMMUNotifier structure and
> > filter based on that range in memory_region_notify_iommu.
> > 
> > It means a little more list searching and filtering on notify, but it
> > avoids having to have another list and search on the VFIO side.  I
> > think it will also better deal with cases where part of an IOMMU
> > mapped region is inaccessible due to an intermediate bridge.
> 
> IIUC, this will still need to keep several VFIOGuestIOMMUs which
> contains exactly the same content?

Well, it would no longer be exactly the same content since the
different ranges would be stored within the IOMMUNotifier
substructure.  But, yes, it would be very similar.

I think it's still worth it for improved clarity, and the possibility
to handle in future cases where part of the IOMMU region simply isn't
accessible at all in the CPU address space.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]