Re: [Qemu-devel] [PATCH v2 04/10] intel-iommu: only do page walk for MAP notifiers

[Peter Xu <peterx@redhat.com>]

On Thu, May 17, 2018 at 03:39:50PM +0200, Auger Eric wrote:
> Hi Peter,
> 
> On 05/04/2018 05:08 AM, Peter Xu wrote:
> > For UNMAP-only IOMMU notifiers, we don't really need to walk the page
> s/really// ;-)

Ok.

> > tables.  Fasten that procedure by skipping the page table walk.  That
> > should boost performance for UNMAP-only notifiers like vhost.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  include/hw/i386/intel_iommu.h |  2 ++
> >  hw/i386/intel_iommu.c         | 43 +++++++++++++++++++++++++++++++----
> >  2 files changed, 40 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> > index ee517704e7..9e0a6c1c6a 100644
> > --- a/include/hw/i386/intel_iommu.h
> > +++ b/include/hw/i386/intel_iommu.h
> > @@ -93,6 +93,8 @@ struct VTDAddressSpace {
> >      IntelIOMMUState *iommu_state;
> >      VTDContextCacheEntry context_cache_entry;
> >      QLIST_ENTRY(VTDAddressSpace) next;
> > +    /* Superset of notifier flags that this address space has */
> > +    IOMMUNotifierFlag notifier_flags;
> >  };
> >  
> >  struct VTDBus {
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index 112971638d..9a418abfb6 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -138,6 +138,12 @@ static inline void vtd_iommu_unlock(IntelIOMMUState *s)
> >      qemu_mutex_unlock(&s->iommu_lock);
> >  }
> >  
> > +/* Whether the address space needs to notify new mappings */
> > +static inline gboolean vtd_as_notify_mappings(VTDAddressSpace *as)
> would suggest vtd_as_has_map_notifier()? But tastes & colours ;-)

Yeah it is.  But okay, I can switch to that especially it's only used
in this patch and it's new.

> > +{
> > +    return as->notifier_flags & IOMMU_NOTIFIER_MAP;
> > +}
> > +
> >  /* GHashTable functions */
> >  static gboolean vtd_uint64_equal(gconstpointer v1, gconstpointer v2)
> >  {
> > @@ -1433,14 +1439,35 @@ static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s,
> >      VTDAddressSpace *vtd_as;
> >      VTDContextEntry ce;
> >      int ret;
> > +    hwaddr size = (1 << am) * VTD_PAGE_SIZE;
> >  
> >      QLIST_FOREACH(vtd_as, &(s->notifiers_list), next) {
> >          ret = vtd_dev_to_context_entry(s, pci_bus_num(vtd_as->bus),
> >                                         vtd_as->devfn, &ce);
> >          if (!ret && domain_id == VTD_CONTEXT_ENTRY_DID(ce.hi)) {
> > -            vtd_page_walk(&ce, addr, addr + (1 << am) * VTD_PAGE_SIZE,
> > -                          vtd_page_invalidate_notify_hook,
> > -                          (void *)&vtd_as->iommu, true, s->aw_bits);
> > +            if (vtd_as_notify_mappings(vtd_as)) {
> > +                /*
> > +                 * For MAP-inclusive notifiers, we need to walk the
> > +                 * page table to sync the shadow page table.
> > +                 */
> Potentially we may have several notifiers attached to the IOMMU MR ~
> vtd_as, each of them having different flags. Those flags are OR'ed in
> memory_region_update_iommu_notify_flags and this is the one you now
> store in the vtd_as. So maybe your comment may rather state:
> as soon as we have at least one MAP notifier, we need to do the PTW?

Actually this is not 100% clear too, since all the "MAP notifiers" are
actually both MAP+UNMAP notifiers...  Maybe:

  As long as we have MAP notifications registered in any of our IOMMU
  notifiers, we need to sync the shadow page table.

> 
> nit: not related to this patch: vtd_page_walk kerneldoc comments misses
> @notify_unmap param comment
> side note: the name of the hook is a bit misleading as it suggests we
> invalidate the entry, whereas we update any valid entry and invalidate
> stale ones (if notify_unmap=true)?
> > +                vtd_page_walk(&ce, addr, addr + size,
> > +                              vtd_page_invalidate_notify_hook,
> > +                              (void *)&vtd_as->iommu, true, s->aw_bits);
> > +            } else {
> > +                /*
> > +                 * For UNMAP-only notifiers, we don't need to walk the
> > +                 * page tables.  We just deliver the PSI down to
> > +                 * invalidate caches.
> 
> We just unmap the range?

Isn't it the same thing? :)

If to be explicit, here we know we only registered UNMAP
notifications, it's not really "unmap", it's really cache
invalidations only.

> > +                 */
> > +                IOMMUTLBEntry entry = {
> > +                    .target_as = &address_space_memory,
> > +                    .iova = addr,
> > +                    .translated_addr = 0,
> > +                    .addr_mask = size - 1,
> > +                    .perm = IOMMU_NONE,
> > +                };
> > +                memory_region_notify_iommu(&vtd_as->iommu, entry);
> > +            }
> >          }
> >      }
> >  }
> > @@ -2380,6 +2407,9 @@ static void vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
> >          exit(1);
> >      }
> >  
> > +    /* Update per-address-space notifier flags */
> > +    vtd_as->notifier_flags = new;
> > +
> >      if (old == IOMMU_NOTIFIER_NONE) {
> >          /* Insert new ones */
> >          QLIST_INSERT_HEAD(&s->notifiers_list, vtd_as, next);
> > @@ -2890,8 +2920,11 @@ static void vtd_iommu_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n)
> >                                    PCI_FUNC(vtd_as->devfn),
> >                                    VTD_CONTEXT_ENTRY_DID(ce.hi),
> >                                    ce.hi, ce.lo);
> > -        vtd_page_walk(&ce, 0, ~0ULL, vtd_replay_hook, (void *)n, false,
> > -                      s->aw_bits);
> > +        if (vtd_as_notify_mappings(vtd_as)) {
> > +            /* This is required only for MAP typed notifiers */
> > +            vtd_page_walk(&ce, 0, ~0ULL, vtd_replay_hook, (void *)n, false,
> > +                          s->aw_bits);
> > +        }
> >      } else {
> >          trace_vtd_replay_ce_invalid(bus_n, PCI_SLOT(vtd_as->devfn),
> >                                      PCI_FUNC(vtd_as->devfn));
> > 
> A worthwhile improvement indeed!

I hope so. :) Thanks,

-- 
Peter Xu