Re: [Qemu-devel] [PATCH v9 1/9] memory: add section range info for IOMMU notifier

[Peter Xu]

On Tue, Apr 18, 2017 at 09:55:11PM +1000, David Gibson wrote:
> On Tue, Apr 18, 2017 at 05:56:37PM +0800, Peter Xu wrote:
> > On Tue, Apr 11, 2017 at 11:56:54AM +1000, David Gibson wrote:
> > > On Mon, Apr 10, 2017 at 03:09:50PM +0800, Peter Xu wrote:
> > > > On Mon, Apr 10, 2017 at 02:39:22PM +1000, David Gibson wrote:
> > > > > On Fri, Apr 07, 2017 at 06:59:07PM +0800, Peter Xu wrote:
> > > > > > In this patch, IOMMUNotifier.{start|end} are introduced to store 
> > > > > > section
> > > > > > information for a specific notifier. When notification occurs, we 
> > > > > > not
> > > > > > only check the notification type (MAP|UNMAP), but also check 
> > > > > > whether the
> > > > > > notified iova range overlaps with the range of specific IOMMU 
> > > > > > notifier,
> > > > > > and skip those notifiers if not in the listened range.
> > > > > > 
> > > > > > When removing an region, we need to make sure we removed the correct
> > > > > > VFIOGuestIOMMU by checking the IOMMUNotifier.start address as well.
> > > > > > 
> > > > > > This patch is solving the problem that vfio-pci devices receive
> > > > > > duplicated UNMAP notification on x86 platform when vIOMMU is there. 
> > > > > > The
> > > > > > issue is that x86 IOMMU has a (0, 2^64-1) IOMMU region, which is
> > > > > > splitted by the (0xfee00000, 0xfeefffff) IRQ region. AFAIK
> > > > > > this (splitted IOMMU region) is only happening on x86.
> > > > > > 
> > > > > > This patch also helps vhost to leverage the new interface as well, 
> > > > > > so
> > > > > > that vhost won't get duplicated cache flushes. In that sense, it's 
> > > > > > an
> > > > > > slight performance improvement.
> > > > > > 
> > > > > > Suggested-by: David Gibson <address@hidden>
> > > > > > Reviewed-by: Eric Auger <address@hidden>
> > > > > > Reviewed-by: Michael S. Tsirkin <address@hidden>
> > > > > > Acked-by: Alex Williamson <address@hidden>
> > > > > > Signed-off-by: Peter Xu <address@hidden>
> > > > > > ---
> > > > > >  hw/vfio/common.c      | 12 +++++++++---
> > > > > >  hw/virtio/vhost.c     | 10 ++++++++--
> > > > > >  include/exec/memory.h | 19 ++++++++++++++++++-
> > > > > >  memory.c              |  9 +++++++++
> > > > > >  4 files changed, 44 insertions(+), 6 deletions(-)
> > > > > > 
> > > > > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > > > > > index f3ba9b9..6b33b9f 100644
> > > > > > --- a/hw/vfio/common.c
> > > > > > +++ b/hw/vfio/common.c
> > > > > > @@ -478,8 +478,13 @@ static void 
> > > > > > vfio_listener_region_add(MemoryListener *listener,
> > > > > >          giommu->iommu_offset = 
> > > > > > section->offset_within_address_space -
> > > > > >                                 section->offset_within_region;
> > > > > >          giommu->container = container;
> > > > > > -        giommu->n.notify = vfio_iommu_map_notify;
> > > > > > -        giommu->n.notifier_flags = IOMMU_NOTIFIER_ALL;
> > > > > > +        llend = 
> > > > > > int128_add(int128_make64(section->offset_within_region),
> > > > > > +                           section->size);
> > > > > > +        llend = int128_sub(llend, int128_one());
> > > > > > +        iommu_notifier_init(&giommu->n, vfio_iommu_map_notify,
> > > > > > +                            IOMMU_NOTIFIER_ALL,
> > > > > > +                            section->offset_within_region,
> > > > > > +                            int128_get64(llend));
> > > > > 
> > > > > Seems to me it would make sense to put the fiddling around to convert
> > > > > the MemoryRegionSection into the necessary values would make sense to
> > > > > go inside iommu_notifier_init().
> > > > 
> > > > But will we always get one MemoryRegionSection if we are not in any of
> > > > the region_{add|del} callback? E.g., what if we want to init an IOMMU
> > > > notifier that covers just the whole IOMMU region range?
> > > 
> > > I suppose so.  It's just the only likely users of the interface I can
> > > see will be always doing this from region_{add,del}.
> > > 
> > > > Considering above, I would still slightly prefer current interface.
> > > 
> > > Ok, well my opinion on the matter isn't terribly strong.
> > 
> > Hi, David,
> > 
> > Sorry to respond late (so that context might be lost). Just want to
> > make sure that you are okay with current patch and interface, right?
> > 
> > I think Marcel is going to merge it if np, and I would like to have
> > your confirmation on this before the merge. Thanks!
> 
> Yes, that's fine.

I see that Edurado has already queued the patches. Thanks for the
confirmation!

-- 
Peter Xu