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

[Alex Williamson]

On Fri, 21 Oct 2016 13:17:08 +0800
Peter Xu <address@hidden> 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.

That's effectively what I'm suggesting except I was trying to add some
efficiency by tracking both the MemoryRegion and the
MemoryRegionSection separately so we don't need to traverse a list to
see if a MemoryRegion is already registered.

> > 
> > 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?

The contain different offsets since this is based on the section rather
than the MemoryRegion.  Thanks,

Alex