For UNMAP-only IOMMU notifiers, we don't really need to walk the page
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)
+{
+ 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.
+ */
+ 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.
+ */
+ 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));
--
2.17.0
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// ;-)
> 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 ;-)
> +{
> + 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?
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?
> + */
> + 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!
Thanks
Eric
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
Hi Peter,
On 05/18/2018 07:53 AM, Peter Xu wrote:
> 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:
Can't IOMMU_NOTIFIER_MAP flag value be used without
IOMMU_NOTIFIER_UNMAP? I don't see such restriction in the
memory_region_register_iommu_notifier API.
>
> 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.
yes you're right I meant We just invalidate the range in cache. The
sentence "We just deliver the PSI down to invalidate caches." was not
crystal clear to me at first reading.
Thanks
Eric
>
>>> + */
>>> + 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,
>
On Fri, May 18, 2018 at 09:38:07AM +0200, Auger Eric wrote:
> Hi Peter,
>
> On 05/18/2018 07:53 AM, Peter Xu wrote:
> > 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:
>
> Can't IOMMU_NOTIFIER_MAP flag value be used without
> IOMMU_NOTIFIER_UNMAP? I don't see such restriction in the
> memory_region_register_iommu_notifier API.
Yes from the API it can, but I can hardly think of a use case of
that.
> >
> > 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.
> yes you're right I meant We just invalidate the range in cache. The
> sentence "We just deliver the PSI down to invalidate caches." was not
> crystal clear to me at first reading.
Okay. I just posted a new version, please feel free to comment again
if you have better suggestions. Otherwise I'll just keep the comment
for now. Thanks,
--
Peter Xu
© 2016 - 2025 Red Hat, Inc.