On Thu, Oct 20, 2016 at 02:06:08PM -0600, Alex Williamson wrote:
> [cc +david]
> 
> On Thu, 20 Oct 2016 22:17:18 +0300
> "Aviv B.D." <bd.aviv@gmail.com> wrote:
> 
> > On Mon, Oct 17, 2016 at 7:07 PM, Alex Williamson <alex.williamson@redhat.com
> > > wrote:  
> > 
> > > On Mon, 17 Oct 2016 18:44:21 +0300
> > > "Aviv B.D" <bd.aviv@gmail.com> wrote:
> > >  
> > > > From: "Aviv Ben-David" <bd.aviv@gmail.com>
> > > >
> > > > * Advertize Cache Mode capability in iommu cap register.
> > > >   This capability is controlled by "cache-mode" property of intel-iommu  
> > > device.  
> > > >   To enable this option call QEMU with "-device  
> > > intel-iommu,cache-mode=true".  
> > > >
> > > > * On page cache invalidation in intel vIOMMU, check if the domain belong  
> > > to  
> > > >   registered notifier, and notify accordingly.
> > > >
> > > > Currently this patch still doesn't enabling VFIO devices support with  
> > > vIOMMU  
> > > > present. Current problems:
> > > > * vfio_iommu_map_notify is not aware about memory range belong to  
> > > specific  
> > > >   VFIOGuestIOMMU.  
> > >
> > > Could you elaborate on why this is an issue?
> > >  
> > 
> > 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?

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.

> I am curious why you get two regions separated by one page, can you
> give an example of the ranges for each?  Thanks,
> 
> Alex
> 

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