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

[Eduardo Habkost]

On Tue, Apr 18, 2017 at 06:20:11PM +0300, Marcel Apfelbaum wrote:
> On 04/10/2017 10:09 AM, 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?
> > 
> > Considering above, I would still slightly prefer current interface.
> > 
> > > 
> > > >          QLIST_INSERT_HEAD(&container->giommu_list, giommu, 
> > > > giommu_next);
> > > > 
> > > >          memory_region_register_iommu_notifier(giommu->iommu, 
> > > > &giommu->n);
> > > > @@ -550,7 +555,8 @@ static void vfio_listener_region_del(MemoryListener 
> > > > *listener,
> > > >          VFIOGuestIOMMU *giommu;
> > > > 
> > > >          QLIST_FOREACH(giommu, &container->giommu_list, giommu_next) {
> > > > -            if (giommu->iommu == section->mr) {
> > > > +            if (giommu->iommu == section->mr &&
> > > > +                giommu->n.start == section->offset_within_region) {
> > > 
> > > This test should be sufficient, but I'd be a bit more comfortable if
> > > there was a test and assert() that the end matches as well.  I also
> > > wonder if remove-matching-notifier helper would be useful here.
> > > Although vhost doesn't appear to ever remove, so maybe it's premature.
> > 
> > Oh... vhost does remove it, but I just forgot to touch it up :( ...
> > Thanks for pointing out.
> > 
> > Marcel, if this is the only comment, would you mind squash below
> > change into current patch? Thanks,
> 
> Hi Peter,
> I asked Eduardo to merge the series through this machine tree
> since I don't have a tree yet.
> He already squashed the change, thanks Eduardo!
> Marcel

The series is queued at:

git://github.com/ehabkost/qemu-hacks.git machine-next

(Including the vhost_iommu_region_del() fixup below).

v2.9.0-rc5 was tagged today, v2.9.0 is expected to be tagged in 2
days. I plan to submit a pull request as soon as it is tagged.

-- 
Eduardo