Re: [Qemu-devel] [PATCH] vfio: avoid adding same iommu mr for notify

[Peter Xu]

On Thu, Nov 17, 2016 at 02:24:54PM +1100, David Gibson wrote:
> On Mon, Nov 14, 2016 at 07:59:28PM -0500, Peter Xu wrote:
> > When one IOMMU memory region is splitted into multiple memory sections,
> > vfio will register multiple same notifiers to a vIOMMU for the same
> > region. That's not sensible. What we need is to register one IOMMU
> > notifier for each IOMMU region, not per section.
> > 
> > Solution is simple - we traverse the container->giommu_list, and skip
> > the registration if memory region is already registered.
> > 
> > To make vfio's region_add() short, vfio_listener_region_add_iommu() is
> > introduced.
> > 
> > Signed-off-by: Peter Xu <address@hidden>
> 
> This is wrong.  It will work on the register side, but when the first
> section attached to the IOMMU is removed, the IOMMU will be removed
> from the list (triggering an unmap of the whole vfio space), rather
> than when the *last* section attached to the MR is removed.
> 
> You'll get away with it in the simple x86 case, because both sections
> will be removed at basically the same time, but it's not correct in
> general.

Hmm, I got your point. Instead of keeping multiple VFIOGuestIOMMU as
you have mentioned previous (and below), IMHO another choice is to
have a reference count for each VFIOGuestIOMMU.

> 
> I really think a better approach is to add the section boundary
> information to the IOMMUNotifier structure within VFIOGuestIOMMU, and
> check that as well as the MR on remove.  That additionally means the
> IOMMU notifier won't get called for portions of the MR outside the
> mapped sections, which the notifier handler probably isn't going to
> expect.

Here, the problem is: what we want to listen to here. IMHO, here what
we are trying to listen to is memory region, not memory region
section. In that case, keeping multiple VFIOGuestIOMMU object is
awkward. I would prefer to have a VFIOGuestIOMMU.refcount field as
mentioned above, then:

- inc() every time the IOMMU memory region is added,
- dec() every time the IOMMU memory region is removed,
- free() when dec() == 0

Thanks,

-- peterx