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 <da...@gibson.dropbear.id.au>
> > > > > > Reviewed-by: Eric Auger <eric.au...@redhat.com>
> > > > > > Reviewed-by: Michael S. Tsirkin <m...@redhat.com>
> > > > > > Acked-by: Alex Williamson <alex.william...@redhat.com>
> > > > > > Signed-off-by: Peter Xu <pet...@redhat.com>
> > > > > > ---
> > > > > > 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
