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

From: Peter Xu <peterx@redhat.com>
To: David Gibson <david@gibson.dropbear.id.au>
Cc: Alex Williamson <alex.williamson@redhat.com>,
	"Aviv B.D." <bd.aviv@gmail.com>,
	qemu-devel@nongnu.org, "Michael S. Tsirkin" <mst@redhat.com>,
	Jan Kiszka <jan.kiszka@siemens.com>
Subject: Re: [Qemu-devel] [PATCH v4 RESEND 0/3] IOMMU: intel_iommu support map and unmap notifications
Date: Fri, 21 Oct 2016 13:17:08 +0800	[thread overview]
Message-ID: <20161021051708.GP15168@pxdev.xzpeter.org> (raw)
In-Reply-To: <20161021005005.GV11140@umbus.fritz.box>

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.

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

Thanks,

-- peterx