[PATCH V2 0/5] Fix UNMAP notifier for intel-iommu

[PATCH V2 0/5] Fix UNMAP notifier for intel-iommu

[Jason Wang]

Hi All:

According to ATS, device should work if ATS is disabled. This is not
correctly implemented in the current intel-iommu since it doesn't
handle the UNMAP notifier correctly. This breaks the vhost-net +
vIOMMU without dt.

The root casue is that the when there's a device IOTLB miss (note that
it's not specific to PCI so it can work without ATS), Qemu doesn't
build the IOVA tree, so when guest start an IOTLB invalidation, Qemu
won't trigger the UNMAP notifier.

Fixing this by triggering UNMAP notifier in those cases.

Thanks

Changes since V1:

- Do not depend on the iova tree for such kind of invalidation but
  simply tries to do UNMAP for all attached IOMMU notifiers

Jason Wang (4):
  intel-iommu: fail MAP notifier without caching mode
  intel-iommu: fail DEVIOTLB_UNMAP without dt mode
  memory: introduce memory_region_unmap_iommu_notifier_range()
  smmu: switch to use memory_region_unmap_iommu_notifier_range()

Peter Xu (1):
  intel-iommu: send UNMAP notifications for domain or global inv desc

 hw/arm/smmu-common.c  | 16 +---------------
 hw/i386/intel_iommu.c | 29 ++++++++++++++++++++++++-----
 include/exec/memory.h | 10 ++++++++++
 softmmu/memory.c      | 13 +++++++++++++
 4 files changed, 48 insertions(+), 20 deletions(-)

-- 
2.25.1

[PATCH V2 1/5] intel-iommu: fail MAP notifier without caching mode

[Jason Wang]

Without caching mode, MAP notifier won't work correctly since guest
won't send IOTLB update event when it establishes new mappings in the
I/O page tables. Let's fail the IOMMU notifiers early instead of
misbehaving silently.

Reviewed-by: Eric Auger <eric.auger@redhat.com>
Tested-by: Viktor Prutyanov <viktor@daynix.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/i386/intel_iommu.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 98a5c304a7..0de3e31577 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3186,6 +3186,13 @@ static int vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
                          "Snoop Control with vhost or VFIO is not supported");
         return -ENOTSUP;
     }
+    if (!s->caching_mode && (new & IOMMU_NOTIFIER_MAP)) {
+        error_setg_errno(errp, ENOTSUP,
+                         "device %02x.%02x.%x requires caching mode",
+                         pci_bus_num(vtd_as->bus), PCI_SLOT(vtd_as->devfn),
+                         PCI_FUNC(vtd_as->devfn));
+        return -ENOTSUP;
+    }
 
     /* Update per-address-space notifier flags */
     vtd_as->notifier_flags = new;
-- 
2.25.1

[PATCH V2 2/5] intel-iommu: fail DEVIOTLB_UNMAP without dt mode

[Jason Wang]

Without dt mode, device IOTLB notifier won't work since guest won't
send device IOTLB invalidation descriptor in this case. Let's fail
early instead of misbehaving silently.

Reviewed-by: Laurent Vivier <lvivier@redhat.com>
Tested-by: Laurent Vivier <lvivier@redhat.com>
Tested-by: Viktor Prutyanov <viktor@daynix.com>
Buglink: https://bugzilla.redhat.com/2156876
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/i386/intel_iommu.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 0de3e31577..f006fa6031 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3179,6 +3179,7 @@ static int vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
 {
     VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu);
     IntelIOMMUState *s = vtd_as->iommu_state;
+    X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
 
     /* TODO: add support for VFIO and vhost users */
     if (s->snoop_control) {
@@ -3193,6 +3194,13 @@ static int vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
                          PCI_FUNC(vtd_as->devfn));
         return -ENOTSUP;
     }
+    if (!x86_iommu->dt_supported && (new & IOMMU_NOTIFIER_DEVIOTLB_UNMAP)) {
+        error_setg_errno(errp, ENOTSUP,
+                         "device %02x.%02x.%x requires device IOTLB mode",
+                         pci_bus_num(vtd_as->bus), PCI_SLOT(vtd_as->devfn),
+                         PCI_FUNC(vtd_as->devfn));
+        return -ENOTSUP;
+    }
 
     /* Update per-address-space notifier flags */
     vtd_as->notifier_flags = new;
-- 
2.25.1

[PATCH V2 3/5] memory: introduce memory_region_unmap_iommu_notifier_range()

[Jason Wang]

This patch introduces a new helper to unmap the range of a specific
IOMMU notifier.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 include/exec/memory.h | 10 ++++++++++
 softmmu/memory.c      | 13 +++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 2e602a2fad..6fa0b071f0 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1731,6 +1731,16 @@ void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr,
 void memory_region_notify_iommu_one(IOMMUNotifier *notifier,
                                     IOMMUTLBEvent *event);
 
+/**
+ * memory_region_unmap_iommu_notifier_range: notify a unmap for an IOMMU
+ *                                           translation that covers the
+ *                                           range of a notifier
+ *
+ * @notifier: the notifier to be notified
+ */
+void memory_region_unmap_iommu_notifier_range(IOMMUNotifier *n);
+
+
 /**
  * memory_region_register_iommu_notifier: register a notifier for changes to
  * IOMMU translation entries.
diff --git a/softmmu/memory.c b/softmmu/memory.c
index 9d64efca26..ba43b4474e 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1996,6 +1996,19 @@ void memory_region_notify_iommu_one(IOMMUNotifier *notifier,
     }
 }
 
+void memory_region_unmap_iommu_notifier_range(IOMMUNotifier *n)
+{
+    IOMMUTLBEvent event;
+
+    event.type = IOMMU_NOTIFIER_UNMAP;
+    event.entry.target_as = &address_space_memory;
+    event.entry.iova = n->start;
+    event.entry.perm = IOMMU_NONE;
+    event.entry.addr_mask = n->end - n->start;
+
+    memory_region_notify_iommu_one(n, &event);
+}
+
 void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr,
                                 int iommu_idx,
                                 IOMMUTLBEvent event)
-- 
2.25.1

[PATCH V2 4/5] smmu: switch to use memory_region_unmap_iommu_notifier_range()

[Jason Wang]

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/arm/smmu-common.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 733c964778..5e2847d511 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -467,20 +467,6 @@ IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid)
     return NULL;
 }
 
-/* Unmap the whole notifier's range */
-static void smmu_unmap_notifier_range(IOMMUNotifier *n)
-{
-    IOMMUTLBEvent event;
-
-    event.type = IOMMU_NOTIFIER_UNMAP;
-    event.entry.target_as = &address_space_memory;
-    event.entry.iova = n->start;
-    event.entry.perm = IOMMU_NONE;
-    event.entry.addr_mask = n->end - n->start;
-
-    memory_region_notify_iommu_one(n, &event);
-}
-
 /* Unmap all notifiers attached to @mr */
 static void smmu_inv_notifiers_mr(IOMMUMemoryRegion *mr)
 {
@@ -488,7 +474,7 @@ static void smmu_inv_notifiers_mr(IOMMUMemoryRegion *mr)
 
     trace_smmu_inv_notifiers_mr(mr->parent_obj.name);
     IOMMU_NOTIFIER_FOREACH(n, mr) {
-        smmu_unmap_notifier_range(n);
+        memory_region_unmap_iommu_notifier_range(n);
     }
 }
 
-- 
2.25.1

[PATCH V2 5/5] intel-iommu: send UNMAP notifications for domain or global inv desc

[Jason Wang]

From: Peter Xu <peterx@redhat.com>

We don't send UNMAP notification upon domain or global invalidation
which will lead the notifier can't work correctly. One example is to
use vhost remote IOTLB without enabling device IOTLB.

Fixing this by sending UNMAP notification.

Signed-off-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/i386/intel_iommu.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index f006fa6031..a62896759c 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -1530,13 +1530,17 @@ static int vtd_sync_shadow_page_table_range(VTDAddressSpace *vtd_as,
     return vtd_page_walk(s, ce, addr, addr + size, &info, vtd_as->pasid);
 }
 
-static int vtd_sync_shadow_page_table(VTDAddressSpace *vtd_as)
+static int vtd_address_space_sync(VTDAddressSpace *vtd_as)
 {
     int ret;
     VTDContextEntry ce;
     IOMMUNotifier *n;
 
-    if (!(vtd_as->iommu.iommu_notify_flags & IOMMU_NOTIFIER_IOTLB_EVENTS)) {
+    /* If no MAP notifier registered, we simply invalidate all the cache */
+    if (!vtd_as_has_map_notifier(vtd_as)) {
+        IOMMU_NOTIFIER_FOREACH(n, &vtd_as->iommu) {
+            memory_region_unmap_iommu_notifier_range(n);
+        }
         return 0;
     }
 
@@ -2000,7 +2004,7 @@ static void vtd_iommu_replay_all(IntelIOMMUState *s)
     VTDAddressSpace *vtd_as;
 
     QLIST_FOREACH(vtd_as, &s->vtd_as_with_notifiers, next) {
-        vtd_sync_shadow_page_table(vtd_as);
+        vtd_address_space_sync(vtd_as);
     }
 }
 
@@ -2082,7 +2086,7 @@ static void vtd_context_device_invalidate(IntelIOMMUState *s,
              * framework will skip MAP notifications if that
              * happened.
              */
-            vtd_sync_shadow_page_table(vtd_as);
+            vtd_address_space_sync(vtd_as);
         }
     }
 }
@@ -2140,7 +2144,7 @@ static void vtd_iotlb_domain_invalidate(IntelIOMMUState *s, uint16_t domain_id)
         if (!vtd_dev_to_context_entry(s, pci_bus_num(vtd_as->bus),
                                       vtd_as->devfn, &ce) &&
             domain_id == vtd_get_domain_id(s, &ce, vtd_as->pasid)) {
-            vtd_sync_shadow_page_table(vtd_as);
+            vtd_address_space_sync(vtd_as);
         }
     }
 }
-- 
2.25.1

Re: [PATCH V2 0/5] Fix UNMAP notifier for intel-iommu

[Peter Xu]

On Thu, Feb 23, 2023 at 02:59:19PM +0800, Jason Wang wrote:
> Hi All:
> 
> According to ATS, device should work if ATS is disabled. This is not
> correctly implemented in the current intel-iommu since it doesn't
> handle the UNMAP notifier correctly. This breaks the vhost-net +
> vIOMMU without dt.
> 
> The root casue is that the when there's a device IOTLB miss (note that
> it's not specific to PCI so it can work without ATS), Qemu doesn't
> build the IOVA tree, so when guest start an IOTLB invalidation, Qemu
> won't trigger the UNMAP notifier.
> 
> Fixing this by triggering UNMAP notifier in those cases.

Maybe someday we should start merging different places where we used the
same IOMMU_NOTIFIER_UNMAP event and also taking care of truncation of
notifier ranges within memory.c, but that can definitely be done later.

Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks, Jason!

-- 
Peter Xu

Re: [PATCH V2 0/5] Fix UNMAP notifier for intel-iommu

[Jason Wang]

On Thu, Feb 23, 2023 at 11:47 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Thu, Feb 23, 2023 at 02:59:19PM +0800, Jason Wang wrote:
> > Hi All:
> >
> > According to ATS, device should work if ATS is disabled. This is not
> > correctly implemented in the current intel-iommu since it doesn't
> > handle the UNMAP notifier correctly. This breaks the vhost-net +
> > vIOMMU without dt.
> >
> > The root casue is that the when there's a device IOTLB miss (note that
> > it's not specific to PCI so it can work without ATS), Qemu doesn't
> > build the IOVA tree, so when guest start an IOTLB invalidation, Qemu
> > won't trigger the UNMAP notifier.
> >
> > Fixing this by triggering UNMAP notifier in those cases.
>
> Maybe someday we should start merging different places where we used the
> same IOMMU_NOTIFIER_UNMAP event and also taking care of truncation of
> notifier ranges within memory.c, but that can definitely be done later.

Yes, probably.

>
> Reviewed-by: Peter Xu <peterx@redhat.com>
>
> Thanks, Jason!

You're welcome :)

Thanks

>
> --
> Peter Xu
>

Re: [PATCH V2 3/5] memory: introduce memory_region_unmap_iommu_notifier_range()

[Thomas Huth]

On 23/02/2023 07.59, Jason Wang wrote:
> This patch introduces a new helper to unmap the range of a specific
> IOMMU notifier.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>   include/exec/memory.h | 10 ++++++++++
>   softmmu/memory.c      | 13 +++++++++++++
>   2 files changed, 23 insertions(+)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 2e602a2fad..6fa0b071f0 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1731,6 +1731,16 @@ void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr,
>   void memory_region_notify_iommu_one(IOMMUNotifier *notifier,
>                                       IOMMUTLBEvent *event);
>   
> +/**
> + * memory_region_unmap_iommu_notifier_range: notify a unmap for an IOMMU
> + *                                           translation that covers the
> + *                                           range of a notifier
> + *
> + * @notifier: the notifier to be notified
> + */
> +void memory_region_unmap_iommu_notifier_range(IOMMUNotifier *n);

FWIW, this causes warning during the build process for me now:

../include/exec/memory.h:1741: warning: Function parameter or member 'n' not described in 'memory_region_unmap_iommu_notifier_range'
../include/exec/memory.h:1741: warning: Excess function parameter 'notifier' description in 'memory_region_unmap_iommu_notifier_range'

  Thomas

Re: [PATCH V2 3/5] memory: introduce memory_region_unmap_iommu_notifier_range()

[Michael S. Tsirkin]

On Thu, Feb 23, 2023 at 02:59:22PM +0800, Jason Wang wrote:
> This patch introduces a new helper to unmap the range of a specific
> IOMMU notifier.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  include/exec/memory.h | 10 ++++++++++
>  softmmu/memory.c      | 13 +++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 2e602a2fad..6fa0b071f0 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1731,6 +1731,16 @@ void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr,
>  void memory_region_notify_iommu_one(IOMMUNotifier *notifier,
>                                      IOMMUTLBEvent *event);
>  
> +/**
> + * memory_region_unmap_iommu_notifier_range: notify a unmap for an IOMMU
> + *                                           translation that covers the
> + *                                           range of a notifier
> + *
> + * @notifier: the notifier to be notified
> + */
> +void memory_region_unmap_iommu_notifier_range(IOMMUNotifier *n);
> +
> +
>  /**
>   * memory_region_register_iommu_notifier: register a notifier for changes to
>   * IOMMU translation entries.

This causes doc warnings:

/scm/qemu/docs/../include/exec/memory.h:1741: warning: Function parameter or member 'n' not described in 'memory_region_unmap_iommu_notifier_range'
/scm/qemu/docs/../include/exec/memory.h:1741: warning: Excess function parameter 'notifier' description in 'memory_region_unmap_iommu_notifier_range'


please fix.


> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 9d64efca26..ba43b4474e 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -1996,6 +1996,19 @@ void memory_region_notify_iommu_one(IOMMUNotifier *notifier,
>      }
>  }
>  
> +void memory_region_unmap_iommu_notifier_range(IOMMUNotifier *n)
> +{
> +    IOMMUTLBEvent event;
> +
> +    event.type = IOMMU_NOTIFIER_UNMAP;
> +    event.entry.target_as = &address_space_memory;
> +    event.entry.iova = n->start;
> +    event.entry.perm = IOMMU_NONE;
> +    event.entry.addr_mask = n->end - n->start;
> +
> +    memory_region_notify_iommu_one(n, &event);
> +}
> +
>  void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr,
>                                  int iommu_idx,
>                                  IOMMUTLBEvent event)
> -- 
> 2.25.1

Re: [PATCH V2 3/5] memory: introduce memory_region_unmap_iommu_notifier_range()

[Jason Wang]

On Wed, Mar 8, 2023 at 9:02 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Feb 23, 2023 at 02:59:22PM +0800, Jason Wang wrote:
> > This patch introduces a new helper to unmap the range of a specific
> > IOMMU notifier.
> >
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> >  include/exec/memory.h | 10 ++++++++++
> >  softmmu/memory.c      | 13 +++++++++++++
> >  2 files changed, 23 insertions(+)
> >
> > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > index 2e602a2fad..6fa0b071f0 100644
> > --- a/include/exec/memory.h
> > +++ b/include/exec/memory.h
> > @@ -1731,6 +1731,16 @@ void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr,
> >  void memory_region_notify_iommu_one(IOMMUNotifier *notifier,
> >                                      IOMMUTLBEvent *event);
> >
> > +/**
> > + * memory_region_unmap_iommu_notifier_range: notify a unmap for an IOMMU
> > + *                                           translation that covers the
> > + *                                           range of a notifier
> > + *
> > + * @notifier: the notifier to be notified
> > + */
> > +void memory_region_unmap_iommu_notifier_range(IOMMUNotifier *n);
> > +
> > +
> >  /**
> >   * memory_region_register_iommu_notifier: register a notifier for changes to
> >   * IOMMU translation entries.
>
> This causes doc warnings:
>
> /scm/qemu/docs/../include/exec/memory.h:1741: warning: Function parameter or member 'n' not described in 'memory_region_unmap_iommu_notifier_range'
> /scm/qemu/docs/../include/exec/memory.h:1741: warning: Excess function parameter 'notifier' description in 'memory_region_unmap_iommu_notifier_range'
>
>
> please fix.

Will do.

Thanks

>
>
> > diff --git a/softmmu/memory.c b/softmmu/memory.c
> > index 9d64efca26..ba43b4474e 100644
> > --- a/softmmu/memory.c
> > +++ b/softmmu/memory.c
> > @@ -1996,6 +1996,19 @@ void memory_region_notify_iommu_one(IOMMUNotifier *notifier,
> >      }
> >  }
> >
> > +void memory_region_unmap_iommu_notifier_range(IOMMUNotifier *n)
> > +{
> > +    IOMMUTLBEvent event;
> > +
> > +    event.type = IOMMU_NOTIFIER_UNMAP;
> > +    event.entry.target_as = &address_space_memory;
> > +    event.entry.iova = n->start;
> > +    event.entry.perm = IOMMU_NONE;
> > +    event.entry.addr_mask = n->end - n->start;
> > +
> > +    memory_region_notify_iommu_one(n, &event);
> > +}
> > +
> >  void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr,
> >                                  int iommu_idx,
> >                                  IOMMUTLBEvent event)
> > --
> > 2.25.1
>