[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 04/10] intel-iommu: only do page walk for MAP
From: |
Peter Xu |
Subject: |
Re: [Qemu-devel] [PATCH v2 04/10] intel-iommu: only do page walk for MAP notifiers |
Date: |
Fri, 18 May 2018 13:53:07 +0800 |
User-agent: |
Mutt/1.9.5 (2018-04-13) |
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 <address@hidden>
> > ---
> > 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
- Re: [Qemu-devel] [PATCH v2 01/10] intel-iommu: send PSI always even if across PDEs, (continued)
[Qemu-devel] [PATCH v2 02/10] intel-iommu: remove IntelIOMMUNotifierNode, Peter Xu, 2018/05/03
[Qemu-devel] [PATCH v2 04/10] intel-iommu: only do page walk for MAP notifiers, Peter Xu, 2018/05/03
[Qemu-devel] [PATCH v2 03/10] intel-iommu: add iommu lock, Peter Xu, 2018/05/03
[Qemu-devel] [PATCH v2 05/10] intel-iommu: introduce vtd_page_walk_info, Peter Xu, 2018/05/03
[Qemu-devel] [PATCH v2 06/10] intel-iommu: pass in address space when page walk, Peter Xu, 2018/05/03