intel-iommu: nested vIOMMU, cleanups, bug fixes

During IOVA page table walking, there is a special case when the PSI covers one whole PDE (Page Directory Entry, which contains 512 Page Table Entries) or more. In the past, we skip that entry and we don't notify the IOMMU notifiers. This is not correct. We should send UNMAP notification to registered UNMAP notifiers in this case.

For UNMAP only notifiers, this might cause IOTLBs cached in the devices even if they were already invalid. For MAP/UNMAP notifiers like vfio-pci, this will cause stale page mappings.

This special case doesn't trigger often, but it is very easy to be triggered by nested device assignments, since in that case we'll possibly map the whole L2 guest RAM region into the device's IOVA address space (several GBs at least), which is far bigger than normal kernel driver usages of the device (tens of MBs normally).

Without this patch applied to L1 QEMU, nested device assignment to L2 guests will dump some errors like:

qemu-system-x86_64: VFIO_MAP_DMA: -17
qemu-system-x86_64: vfio_dma_map(0x557305420c30, 0xad000, 0x1000,

                    0x7f89a920d000) = -17 (File exists)

Acked-by Jason Wang <[email protected]>

[peterx: rewrite the commit message]

Signed-off-by Peter Xu <[email protected]>

---
 hw/i386/intel_iommu.c | 42 ++++++++++++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 12 deletions(-)

hw/i386/intel_iommu.c +0 -0

index fb31de9416..b359efd6f9 100644

hw/i386/intel_iommu.c +15 -11

+{
+    assert(hook_fn);
+    trace_vtd_page_walk_one(level, entry->iova, entry->translated_addr,
+                            entry->addr_mask, entry->perm);
+    return hook_fn(entry, private);
+}
+
 /**
  * vtd_page_walk_level - walk over specific level for IOVA range
  *

@@ -781,28 +790,37 @@ static int vtd_page_walk_level(dma_addr_t addr, uint64_t start,
          */
         entry_valid = read_cur | write_cur;
 
+        entry.target_as = &address_space_memory;
+        entry.iova = iova & subpage_mask;
+        entry.perm = IOMMU_ACCESS_FLAG(read_cur, write_cur);
+        entry.addr_mask = ~subpage_mask;
+
         if (vtd_is_last_slpte(slpte, level)) {
-            entry.target_as = &address_space_memory;
-            entry.iova = iova & subpage_mask;
             /* NOTE: this is only meaningful if entry_valid == true */
             entry.translated_addr = vtd_get_slpte_addr(slpte, aw);
-            entry.addr_mask = ~subpage_mask;
-            entry.perm = IOMMU_ACCESS_FLAG(read_cur, write_cur);
             if (!entry_valid && !notify_unmap) {
                 trace_vtd_page_walk_skip_perm(iova, iova_next);
                 goto next;
             }
-            trace_vtd_page_walk_one(level, entry.iova, entry.translated_addr,
-                                    entry.addr_mask, entry.perm);
-            if (hook_fn) {
-                ret = hook_fn(&entry, private);
-                if (ret < 0) {
-                    return ret;
-                }
+            ret = vtd_page_walk_one(&entry, level, hook_fn, private);
+            if (ret < 0) {
+                return ret;

Hi Peter,

Quoted 36 lines · Peter Xu

During IOVA page table walking, there is a special case when the PSI covers one whole PDE (Page Directory Entry, which contains 512 Page Table Entries) or more. In the past, we skip that entry and we don't notify the IOMMU notifiers. This is not correct. We should send UNMAP notification to registered UNMAP notifiers in this case.

For UNMAP only notifiers, this might cause IOTLBs cached in the devices even if they were already invalid. For MAP/UNMAP notifiers like vfio-pci, this will cause stale page mappings.

This special case doesn't trigger often, but it is very easy to be triggered by nested device assignments, since in that case we'll possibly map the whole L2 guest RAM region into the device's IOVA address space (several GBs at least), which is far bigger than normal kernel driver usages of the device (tens of MBs normally).

Without this patch applied to L1 QEMU, nested device assignment to L2 guests will dump some errors like:

qemu-system-x86_64: VFIO_MAP_DMA: -17
qemu-system-x86_64: vfio_dma_map(0x557305420c30, 0xad000, 0x1000,

                    0x7f89a920d000) = -17 (File exists)

Acked-by Jason Wang <[email protected]>

[peterx: rewrite the commit message]

Signed-off-by Peter Xu <[email protected]>

---
 hw/i386/intel_iommu.c | 42 ++++++++++++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 12 deletions(-)

hw/i386/intel_iommu.c +0 -0

index fb31de9416..b359efd6f9 100644
@@ -722,6 +722,15 @@ static int vtd_iova_to_slpte(VTDContextEntry *ce, uint64_t iova, bool is_write,
 

hw/i386/intel_iommu.c static int vtd_iova_to_slpte(VTDContextEntry *ce, uint64_t iova, bool is_write, · line 726

Quoted 4 lines

hw/i386/intel_iommu.c +2 -0

 typedef int (*vtd_page_walk_hook)(IOMMUTLBEntry *entry, void *private);
 
+static int vtd_page_walk_one(IOMMUTLBEntry *entry, int level,
+                             vtd_page_walk_hook hook_fn, void *private)

I find the function name a bit weird as it does not does a ptw but rather call a callback on an entry. vtd_callback_wrapper?

Quoted 44 lines

hw/i386/intel_iommu.c +15 -12

+{
+    assert(hook_fn);
+    trace_vtd_page_walk_one(level, entry->iova, entry->translated_addr,
+                            entry->addr_mask, entry->perm);
+    return hook_fn(entry, private);
+}
+
 /**
  * vtd_page_walk_level - walk over specific level for IOVA range
  *

@@ -781,28 +790,37 @@ static int vtd_page_walk_level(dma_addr_t addr, uint64_t start,
          */
         entry_valid = read_cur | write_cur;
 
+        entry.target_as = &address_space_memory;
+        entry.iova = iova & subpage_mask;
+        entry.perm = IOMMU_ACCESS_FLAG(read_cur, write_cur);
+        entry.addr_mask = ~subpage_mask;
+
         if (vtd_is_last_slpte(slpte, level)) {
-            entry.target_as = &address_space_memory;
-            entry.iova = iova & subpage_mask;
             /* NOTE: this is only meaningful if entry_valid == true */
             entry.translated_addr = vtd_get_slpte_addr(slpte, aw);
-            entry.addr_mask = ~subpage_mask;
-            entry.perm = IOMMU_ACCESS_FLAG(read_cur, write_cur);
             if (!entry_valid && !notify_unmap) {
                 trace_vtd_page_walk_skip_perm(iova, iova_next);
                 goto next;
             }
-            trace_vtd_page_walk_one(level, entry.iova, entry.translated_addr,
-                                    entry.addr_mask, entry.perm);
-            if (hook_fn) {
-                ret = hook_fn(&entry, private);
-                if (ret < 0) {
-                    return ret;
-                }
+            ret = vtd_page_walk_one(&entry, level, hook_fn, private);
+            if (ret < 0) {
+                return ret;
             }
         } else {
             if (!entry_valid) {
-                trace_vtd_page_walk_skip_perm(iova, iova_next);

hw/i386/intel_iommu.c static int vtd_page_walk_level(dma_addr_t addr, uint64_t start, · line 816

Quoted 6 lines

hw/i386/intel_iommu.c +6 -0

+                if (notify_unmap) {
+                    /*
+                     * The whole entry is invalid; unmap it all.
+                     * Translated address is meaningless, zero it.
+                     */
+                    entry.translated_addr = 0x0;

do you really need to zero the translated_addr and the related comment. As soon as perm is NONE this should not be used?

hw/i386/intel_iommu.c static int vtd_page_walk_level(dma_addr_t addr, uint64_t start, · line 826

Quoted 11 lines

hw/i386/intel_iommu.c +7 -0

+                    ret = vtd_page_walk_one(&entry, level, hook_fn, private);
+                    if (ret < 0) {
+                        return ret;
+                    }
+                } else {
+                    trace_vtd_page_walk_skip_perm(iova, iova_next);
+                }
                 goto next;
             }
             ret = vtd_page_walk_level(vtd_get_slpte_addr(slpte, aw), iova,

Besides

Reviewed-by Eric Auger <[email protected]>

Quoted 39 lines · Auger Eric

Hi Peter,

Quoted 36 lines · Peter Xu

During IOVA page table walking, there is a special case when the PSI covers one whole PDE (Page Directory Entry, which contains 512 Page Table Entries) or more. In the past, we skip that entry and we don't notify the IOMMU notifiers. This is not correct. We should send UNMAP notification to registered UNMAP notifiers in this case.

For UNMAP only notifiers, this might cause IOTLBs cached in the devices even if they were already invalid. For MAP/UNMAP notifiers like vfio-pci, this will cause stale page mappings.

This special case doesn't trigger often, but it is very easy to be triggered by nested device assignments, since in that case we'll possibly map the whole L2 guest RAM region into the device's IOVA address space (several GBs at least), which is far bigger than normal kernel driver usages of the device (tens of MBs normally).

Without this patch applied to L1 QEMU, nested device assignment to L2 guests will dump some errors like:

qemu-system-x86_64: VFIO_MAP_DMA: -17
qemu-system-x86_64: vfio_dma_map(0x557305420c30, 0xad000, 0x1000,

                    0x7f89a920d000) = -17 (File exists)

Acked-by Jason Wang <[email protected]>

[peterx: rewrite the commit message]

Signed-off-by Peter Xu <[email protected]>

---
 hw/i386/intel_iommu.c | 42 ++++++++++++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 12 deletions(-)

hw/i386/intel_iommu.c +0 -0

index fb31de9416..b359efd6f9 100644
@@ -722,6 +722,15 @@ static int vtd_iova_to_slpte(VTDContextEntry *ce, uint64_t iova, bool is_write,
 

Quoted 6 lines

hw/i386/intel_iommu.c static int vtd_iova_to_slpte(VTDContextEntry *ce, uint64_t iova, bool is_write, · line 726

Quoted 4 lines

hw/i386/intel_iommu.c +2 -0

 typedef int (*vtd_page_walk_hook)(IOMMUTLBEntry *entry, void *private);
 
+static int vtd_page_walk_one(IOMMUTLBEntry *entry, int level,
+                             vtd_page_walk_hook hook_fn, void *private)

I find the function name a bit weird as it does not does a ptw but rather call a callback on an entry. vtd_callback_wrapper?

It's a hook for the page walk process, and IMHO vtd_callback_wrapper does not really provide any hint for the page walking. So even if you prefer the "callback_wrapper" naming I would still more prefer:

vtd_page_walk_callback[_wrapper]

though if so I'd say I don't see much benefit comparing to use the old vtd_page_walk_hook, which seems fine to me too...

Quoted 45 lines

Quoted 45 lines

hw/i386/intel_iommu.c +15 -12

+{
+    assert(hook_fn);
+    trace_vtd_page_walk_one(level, entry->iova, entry->translated_addr,
+                            entry->addr_mask, entry->perm);
+    return hook_fn(entry, private);
+}
+
 /**
  * vtd_page_walk_level - walk over specific level for IOVA range
  *

@@ -781,28 +790,37 @@ static int vtd_page_walk_level(dma_addr_t addr, uint64_t start,
          */
         entry_valid = read_cur | write_cur;
 
+        entry.target_as = &address_space_memory;
+        entry.iova = iova & subpage_mask;
+        entry.perm = IOMMU_ACCESS_FLAG(read_cur, write_cur);
+        entry.addr_mask = ~subpage_mask;
+
         if (vtd_is_last_slpte(slpte, level)) {
-            entry.target_as = &address_space_memory;
-            entry.iova = iova & subpage_mask;
             /* NOTE: this is only meaningful if entry_valid == true */
             entry.translated_addr = vtd_get_slpte_addr(slpte, aw);
-            entry.addr_mask = ~subpage_mask;
-            entry.perm = IOMMU_ACCESS_FLAG(read_cur, write_cur);
             if (!entry_valid && !notify_unmap) {
                 trace_vtd_page_walk_skip_perm(iova, iova_next);
                 goto next;
             }
-            trace_vtd_page_walk_one(level, entry.iova, entry.translated_addr,
-                                    entry.addr_mask, entry.perm);
-            if (hook_fn) {
-                ret = hook_fn(&entry, private);
-                if (ret < 0) {
-                    return ret;
-                }
+            ret = vtd_page_walk_one(&entry, level, hook_fn, private);
+            if (ret < 0) {
+                return ret;
             }
         } else {
             if (!entry_valid) {
-                trace_vtd_page_walk_skip_perm(iova, iova_next);

Quoted 8 lines

hw/i386/intel_iommu.c static int vtd_page_walk_level(dma_addr_t addr, uint64_t start, · line 816

Quoted 6 lines

hw/i386/intel_iommu.c +6 -0

+                if (notify_unmap) {
+                    /*
+                     * The whole entry is invalid; unmap it all.
+                     * Translated address is meaningless, zero it.
+                     */
+                    entry.translated_addr = 0x0;

do you really need to zero the translated_addr and the related comment. As soon as perm is NONE this should not be used?

Yes here we can avoid setting it. However that'll make sure we don't leak strange numbers to the below notifiers, so I would still slightly prefer to zero it here.

Quoted 12 lines

hw/i386/intel_iommu.c static int vtd_page_walk_level(dma_addr_t addr, uint64_t start, · line 826

Quoted 11 lines

hw/i386/intel_iommu.c +7 -0

+                    ret = vtd_page_walk_one(&entry, level, hook_fn, private);
+                    if (ret < 0) {
+                        return ret;
+                    }
+                } else {
+                    trace_vtd_page_walk_skip_perm(iova, iova_next);
+                }
                 goto next;
             }
             ret = vtd_page_walk_level(vtd_get_slpte_addr(slpte, aw), iova,

Quoted 2 lines

Besides

Reviewed-by Eric Auger <[email protected]>

Thanks for reviewing the series.

Note that this v2 is obsolete, please feel free to read version 3 of this series, which contains quite a lot of functional changes.

Regards,

That is not really necessary. Removing that node struct and put the list entry directly into VTDAddressSpace. It simplfies the code a lot.

Signed-off-by Peter Xu <[email protected]>

---
 include/hw/i386/intel_iommu.h |  9 ++------
 hw/i386/intel_iommu.c         | 41 ++++++++++-------------------------
 2 files changed, 14 insertions(+), 36 deletions(-)

include/hw/i386/intel_iommu.h +1 -6

index 45ec8919b6..220697253f 100644
@@ -67,7 +67,6 @@ typedef union VTD_IR_TableEntry VTD_IR_TableEntry;
 typedef union VTD_IR_MSIAddress VTD_IR_MSIAddress;
 typedef struct VTDIrq VTDIrq;
 typedef struct VTD_MSIMessage VTD_MSIMessage;
-typedef struct IntelIOMMUNotifierNode IntelIOMMUNotifierNode;
 
 /* Context-Entry */
 struct VTDContextEntry {

@@ -93,6 +92,7 @@ struct VTDAddressSpace {
     MemoryRegion iommu_ir;      /* Interrupt region: 0xfeeXXXXX */
     IntelIOMMUState *iommu_state;
     VTDContextCacheEntry context_cache_entry;
+    QLIST_ENTRY(VTDAddressSpace) next;
 };
 
 struct VTDBus {

@@ -253,11 +253,6 @@ struct VTD_MSIMessage {
 /* When IR is enabled, all MSI/MSI-X data bits should be zero */
 #define VTD_IR_MSI_DATA          (0)
 
-struct IntelIOMMUNotifierNode {
-    VTDAddressSpace *vtd_as;
-    QLIST_ENTRY(IntelIOMMUNotifierNode) next;
-};
-
 /* The iommu (DMAR) device state struct */
 struct IntelIOMMUState {
     X86IOMMUState x86_iommu;

include/hw/i386/intel_iommu.h +0 -0

     /* interrupt remapping */
     bool intr_enabled;              /* Whether guest enabled IR */

hw/i386/intel_iommu.c +6 -17

index b359efd6f9..5987b48d43 100644
@@ -1248,10 +1248,10 @@ static void vtd_interrupt_remap_table_setup(IntelIOMMUState *s)
 
 static void vtd_iommu_replay_all(IntelIOMMUState *s)
 {
-    IntelIOMMUNotifierNode *node;
+    VTDAddressSpace *vtd_as;
 
-    QLIST_FOREACH(node, &s->notifiers_list, next) {
-        memory_region_iommu_replay_all(&node->vtd_as->iommu);
+    QLIST_FOREACH(vtd_as, &s->notifiers_list, next) {
+        memory_region_iommu_replay_all(&vtd_as->iommu);
     }
 }
 

@@ -1372,7 +1372,6 @@ static void vtd_iotlb_global_invalidate(IntelIOMMUState *s)
 
 static void vtd_iotlb_domain_invalidate(IntelIOMMUState *s, uint16_t domain_id)
 {
-    IntelIOMMUNotifierNode *node;
     VTDContextEntry ce;
     VTDAddressSpace *vtd_as;
 

@@ -1381,8 +1380,7 @@ static void vtd_iotlb_domain_invalidate(IntelIOMMUState *s, uint16_t domain_id)
     g_hash_table_foreach_remove(s->iotlb, vtd_hash_remove_by_domain,
                                 &domain_id);
 
-    QLIST_FOREACH(node, &s->notifiers_list, next) {
-        vtd_as = node->vtd_as;
+    QLIST_FOREACH(vtd_as, &s->notifiers_list, next) {
         if (!vtd_dev_to_context_entry(s, pci_bus_num(vtd_as->bus),
                                       vtd_as->devfn, &ce) &&
             domain_id == VTD_CONTEXT_ENTRY_DID(ce.hi)) {

@@ -1402,12 +1400,11 @@ static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s,
                                            uint16_t domain_id, hwaddr addr,
                                            uint8_t am)
 {
-    IntelIOMMUNotifierNode *node;
+    VTDAddressSpace *vtd_as;
     VTDContextEntry ce;
     int ret;
 
-    QLIST_FOREACH(node, &(s->notifiers_list), next) {
-        VTDAddressSpace *vtd_as = node->vtd_as;
+    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)) {

@@ -2344,8 +2341,6 @@ static void vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
 {
     VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu);
     IntelIOMMUState *s = vtd_as->iommu_state;
-    IntelIOMMUNotifierNode *node = NULL;
-    IntelIOMMUNotifierNode *next_node = NULL;
 
     if (!s->caching_mode && new & IOMMU_NOTIFIER_MAP) {
         error_report("We need to set caching-mode=1 for intel-iommu to enable "

@@ -2354,21 +2349,11 @@ static void vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
     }
 
     if (old == IOMMU_NOTIFIER_NONE) {
-        node = g_malloc0(sizeof(*node));
-        node->vtd_as = vtd_as;
-        QLIST_INSERT_HEAD(&s->notifiers_list, node, next);
-        return;
-    }
-

hw/i386/intel_iommu.c +1 -0

+        QLIST_REMOVE(vtd_as, next);
     }
 }
 

@@ -2838,12 +2823,10 @@ static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
 
 static void vtd_address_space_unmap_all(IntelIOMMUState *s)
 {

Hi Peter,

Quoted 33 lines · Peter Xu

That is not really necessary. Removing that node struct and put the list entry directly into VTDAddressSpace. It simplfies the code a lot.

Signed-off-by Peter Xu <[email protected]>

---
 include/hw/i386/intel_iommu.h |  9 ++------
 hw/i386/intel_iommu.c         | 41 ++++++++++-------------------------
 2 files changed, 14 insertions(+), 36 deletions(-)

include/hw/i386/intel_iommu.h +1 -1

index 45ec8919b6..220697253f 100644
@@ -67,7 +67,6 @@ typedef union VTD_IR_TableEntry VTD_IR_TableEntry;
 typedef union VTD_IR_MSIAddress VTD_IR_MSIAddress;
 typedef struct VTDIrq VTDIrq;
 typedef struct VTD_MSIMessage VTD_MSIMessage;
-typedef struct IntelIOMMUNotifierNode IntelIOMMUNotifierNode;
 
 /* Context-Entry */
 struct VTDContextEntry {

@@ -93,6 +92,7 @@ struct VTDAddressSpace {
     MemoryRegion iommu_ir;      /* Interrupt region: 0xfeeXXXXX */
     IntelIOMMUState *iommu_state;
     VTDContextCacheEntry context_cache_entry;
+    QLIST_ENTRY(VTDAddressSpace) next;
 };
 
 struct VTDBus {

@@ -253,11 +253,6 @@ struct VTD_MSIMessage {
 /* When IR is enabled, all MSI/MSI-X data bits should be zero */
 #define VTD_IR_MSI_DATA          (0)
 

Quoted 9 lines

include/hw/i386/intel_iommu.h +0 -5

-struct IntelIOMMUNotifierNode {
-    VTDAddressSpace *vtd_as;
-    QLIST_ENTRY(IntelIOMMUNotifierNode) next;
-};
-
 /* The iommu (DMAR) device state struct */
 struct IntelIOMMUState {
     X86IOMMUState x86_iommu;

include/hw/i386/intel_iommu.h struct IntelIOMMUState { · line 293

Quoted 5 lines

include/hw/i386/intel_iommu.h +1 -1

@@ -295,7 +290,7 @@ struct IntelIOMMUState {
     GHashTable *vtd_as_by_busptr;   /* VTDBus objects indexed by PCIBus* reference */
     VTDBus *vtd_as_by_bus_num[VTD_PCI_BUS_MAX]; /* VTDBus objects indexed by bus number */
     /* list of registered notifiers */
-    QLIST_HEAD(, IntelIOMMUNotifierNode) notifiers_list;
+    QLIST_HEAD(, VTDAddressSpace) notifiers_list;

Wouldn't it make sense to rename notifiers_list into something more understandable like address_spaces?

include/hw/i386/intel_iommu.h struct IntelIOMMUState {

Quoted 10 lines

include/hw/i386/intel_iommu.h +1 -9

-    /* update notifier node with new flags */
-    QLIST_FOREACH_SAFE(node, &s->notifiers_list, next, next_node) {
-        if (node->vtd_as == vtd_as) {
-            if (new == IOMMU_NOTIFIER_NONE) {
-                QLIST_REMOVE(node, next);
-                g_free(node);
-            }
-            return;
-        }
+        /* Insert new ones */

s/ones/one

include/hw/i386/intel_iommu.h

Quoted 3 lines

include/hw/i386/intel_iommu.h +3 -0

+        QLIST_INSERT_HEAD(&s->notifiers_list, vtd_as, next);
+    } else if (new == IOMMU_NOTIFIER_NONE) {
+        /* Remove old ones */

same. Not sure the comments are worth.

Quoted 36 lines · Auger Eric

Hi Peter,

Quoted 33 lines · Peter Xu

That is not really necessary. Removing that node struct and put the list entry directly into VTDAddressSpace. It simplfies the code a lot.

Signed-off-by Peter Xu <[email protected]>

---
 include/hw/i386/intel_iommu.h |  9 ++------
 hw/i386/intel_iommu.c         | 41 ++++++++++-------------------------
 2 files changed, 14 insertions(+), 36 deletions(-)

include/hw/i386/intel_iommu.h +1 -1

index 45ec8919b6..220697253f 100644
@@ -67,7 +67,6 @@ typedef union VTD_IR_TableEntry VTD_IR_TableEntry;
 typedef union VTD_IR_MSIAddress VTD_IR_MSIAddress;
 typedef struct VTDIrq VTDIrq;
 typedef struct VTD_MSIMessage VTD_MSIMessage;
-typedef struct IntelIOMMUNotifierNode IntelIOMMUNotifierNode;
 
 /* Context-Entry */
 struct VTDContextEntry {

@@ -93,6 +92,7 @@ struct VTDAddressSpace {
     MemoryRegion iommu_ir;      /* Interrupt region: 0xfeeXXXXX */
     IntelIOMMUState *iommu_state;
     VTDContextCacheEntry context_cache_entry;
+    QLIST_ENTRY(VTDAddressSpace) next;
 };
 
 struct VTDBus {

@@ -253,11 +253,6 @@ struct VTD_MSIMessage {
 /* When IR is enabled, all MSI/MSI-X data bits should be zero */
 #define VTD_IR_MSI_DATA          (0)
 

Quoted 16 lines

Quoted 9 lines

include/hw/i386/intel_iommu.h +0 -5

-struct IntelIOMMUNotifierNode {
-    VTDAddressSpace *vtd_as;
-    QLIST_ENTRY(IntelIOMMUNotifierNode) next;
-};
-
 /* The iommu (DMAR) device state struct */
 struct IntelIOMMUState {
     X86IOMMUState x86_iommu;

include/hw/i386/intel_iommu.h struct IntelIOMMUState { · line 293

Quoted 5 lines

include/hw/i386/intel_iommu.h +1 -1

@@ -295,7 +290,7 @@ struct IntelIOMMUState {
     GHashTable *vtd_as_by_busptr;   /* VTDBus objects indexed by PCIBus* reference */
     VTDBus *vtd_as_by_bus_num[VTD_PCI_BUS_MAX]; /* VTDBus objects indexed by bus number */
     /* list of registered notifiers */
-    QLIST_HEAD(, IntelIOMMUNotifierNode) notifiers_list;
+    QLIST_HEAD(, VTDAddressSpace) notifiers_list;

Wouldn't it make sense to rename notifiers_list into something more understandable like address_spaces?

But address_spaces might be a bit confusing too on the other side as "a list of all VT-d address spaces". How about something like:

address_spaces_with_notifiers

?

[...]

Quoted 10 lines

include/hw/i386/intel_iommu.h struct IntelIOMMUState {

Quoted 10 lines

include/hw/i386/intel_iommu.h +1 -9

-    /* update notifier node with new flags */
-    QLIST_FOREACH_SAFE(node, &s->notifiers_list, next, next_node) {
-        if (node->vtd_as == vtd_as) {
-            if (new == IOMMU_NOTIFIER_NONE) {
-                QLIST_REMOVE(node, next);
-                g_free(node);
-            }
-            return;
-        }
+        /* Insert new ones */

Quoted 6 lines

s/ones/one

include/hw/i386/intel_iommu.h

Quoted 3 lines

include/hw/i386/intel_iommu.h +3 -0

+        QLIST_INSERT_HEAD(&s->notifiers_list, vtd_as, next);
+    } else if (new == IOMMU_NOTIFIER_NONE) {
+        /* Remove old ones */

same. Not sure the comments are worth.

Will remove two "s"s there. Thanks,

Hi Peter,

Quoted 54 lines · Peter Xu

Quoted 52 lines · Auger Eric

Hi Peter,

Quoted 42 lines · Peter Xu

That is not really necessary. Removing that node struct and put the list entry directly into VTDAddressSpace. It simplfies the code a lot.

Signed-off-by Peter Xu <[email protected]>

---
 include/hw/i386/intel_iommu.h |  9 ++------
 hw/i386/intel_iommu.c         | 41 ++++++++++-------------------------
 2 files changed, 14 insertions(+), 36 deletions(-)

include/hw/i386/intel_iommu.h +1 -6

index 45ec8919b6..220697253f 100644
@@ -67,7 +67,6 @@ typedef union VTD_IR_TableEntry VTD_IR_TableEntry;
 typedef union VTD_IR_MSIAddress VTD_IR_MSIAddress;
 typedef struct VTDIrq VTDIrq;
 typedef struct VTD_MSIMessage VTD_MSIMessage;
-typedef struct IntelIOMMUNotifierNode IntelIOMMUNotifierNode;
 
 /* Context-Entry */
 struct VTDContextEntry {

@@ -93,6 +92,7 @@ struct VTDAddressSpace {
     MemoryRegion iommu_ir;      /* Interrupt region: 0xfeeXXXXX */
     IntelIOMMUState *iommu_state;
     VTDContextCacheEntry context_cache_entry;
+    QLIST_ENTRY(VTDAddressSpace) next;
 };
 
 struct VTDBus {

@@ -253,11 +253,6 @@ struct VTD_MSIMessage {
 /* When IR is enabled, all MSI/MSI-X data bits should be zero */
 #define VTD_IR_MSI_DATA          (0)
 
-struct IntelIOMMUNotifierNode {
-    VTDAddressSpace *vtd_as;
-    QLIST_ENTRY(IntelIOMMUNotifierNode) next;
-};
-
 /* The iommu (DMAR) device state struct */
 struct IntelIOMMUState {
     X86IOMMUState x86_iommu;

include/hw/i386/intel_iommu.h struct IntelIOMMUState { · line 293

Quoted 5 lines

include/hw/i386/intel_iommu.h +1 -1

@@ -295,7 +290,7 @@ struct IntelIOMMUState {
     GHashTable *vtd_as_by_busptr;   /* VTDBus objects indexed by PCIBus* reference */
     VTDBus *vtd_as_by_bus_num[VTD_PCI_BUS_MAX]; /* VTDBus objects indexed by bus number */
     /* list of registered notifiers */
-    QLIST_HEAD(, IntelIOMMUNotifierNode) notifiers_list;
+    QLIST_HEAD(, VTDAddressSpace) notifiers_list;

Wouldn't it make sense to rename notifiers_list into something more understandable like address_spaces?

Quoted 4 lines

But address_spaces might be a bit confusing too on the other side as "a list of all VT-d address spaces". How about something like:

address_spaces_with_notifiers

Hum I missed not all of them belonged to that list. a bit long now? vtd_as_with_notifiers?

Add a per-iommu big lock to protect IOMMU status. Currently the only thing to be protected is the IOTLB/context cache, since that can be accessed even without BQL, e.g., in IO dataplane.

Note that we don't need to protect device page tables since that's fully controlled by the guest kernel. However there is still possibility that malicious drivers will program the device to not obey the rule. In that case QEMU can't really do anything useful, instead the guest itself will be responsible for all uncertainties.

Reported-by Fam Zheng <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 include/hw/i386/intel_iommu.h |  6 +++++
 hw/i386/intel_iommu.c         | 43 +++++++++++++++++++++++++++++++----
 2 files changed, 44 insertions(+), 5 deletions(-)

include/hw/i386/intel_iommu.h +6 -0

index 220697253f..ee517704e7 100644
@@ -300,6 +300,12 @@ struct IntelIOMMUState {
     OnOffAuto intr_eim;             /* Toggle for EIM cabability */
     bool buggy_eim;                 /* Force buggy EIM unless eim=off */
     uint8_t aw_bits;                /* Host/IOVA address width (in bits) */
+
+    /*
+     * Protects IOMMU states in general.  Currently it protects the
+     * per-IOMMU IOTLB cache, and context entry cache in VTDAddressSpace.
+     */
+    QemuMutex iommu_lock;
 };
 
 /* Find the VTD Address space associated with the given bus pointer,

hw/i386/intel_iommu.c +10 -0

index 5987b48d43..112971638d 100644
@@ -128,6 +128,16 @@ static uint64_t vtd_set_clear_mask_quad(IntelIOMMUState *s, hwaddr addr,
     return new_val;
 }
 
+static inline void vtd_iommu_lock(IntelIOMMUState *s)
+{
+    qemu_mutex_lock(&s->iommu_lock);
+}
+
+static inline void vtd_iommu_unlock(IntelIOMMUState *s)
+{
+    qemu_mutex_unlock(&s->iommu_lock);
+}
+
 /* GHashTable functions */
 static gboolean vtd_uint64_equal(gconstpointer v1, gconstpointer v2)
 {

hw/i386/intel_iommu.c +25 -3

 {
     assert(s->iotlb);
     g_hash_table_remove_all(s->iotlb);
 }
 
+static void vtd_reset_iotlb(IntelIOMMUState *s)
+{
+    vtd_iommu_lock(s);
+    vtd_reset_iotlb_locked(s);
+    vtd_iommu_unlock(s);
+}
+
 static uint64_t vtd_get_iotlb_key(uint64_t gfn, uint16_t source_id,
                                   uint32_t level)
 {

@@ -215,6 +232,7 @@ static uint64_t vtd_get_iotlb_gfn(hwaddr addr, uint32_t level)
     return (addr & vtd_slpt_level_page_mask(level)) >> VTD_PAGE_SHIFT_4K;
 }
 
+/* Must be with IOMMU lock held */
 static VTDIOTLBEntry *vtd_lookup_iotlb(IntelIOMMUState *s, uint16_t source_id,
                                        hwaddr addr)
 {

@@ -235,6 +253,7 @@ out:
     return entry;
 }
 
+/* Must be with IOMMU lock held */
 static void vtd_update_iotlb(IntelIOMMUState *s, uint16_t source_id,
                              uint16_t domain_id, hwaddr addr, uint64_t slpte,
                              uint8_t access_flags, uint32_t level)

@@ -246,7 +265,7 @@ static void vtd_update_iotlb(IntelIOMMUState *s, uint16_t source_id,
     trace_vtd_iotlb_page_update(source_id, addr, slpte, domain_id);
     if (g_hash_table_size(s->iotlb) >= VTD_IOTLB_MAX_SIZE) {
         trace_vtd_iotlb_reset("iotlb exceeds size limit");
-        vtd_reset_iotlb(s);
+        vtd_reset_iotlb_locked(s);
     }
 
     entry->gfn = gfn;

@@ -1106,7 +1125,7 @@ static bool vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,
     IntelIOMMUState *s = vtd_as->iommu_state;
     VTDContextEntry ce;
     uint8_t bus_num = pci_bus_num(bus);
-    VTDContextCacheEntry *cc_entry = &vtd_as->context_cache_entry;
+    VTDContextCacheEntry *cc_entry;
     uint64_t slpte, page_mask;
     uint32_t level;
     uint16_t source_id = vtd_make_source_id(bus_num, devfn);

@@ -1123,6 +1142,10 @@ static bool vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,
      */
     assert(!vtd_is_interrupt_addr(addr));
 
+    vtd_iommu_lock(s);
+
+    cc_entry = &vtd_as->context_cache_entry;
+
     /* Try to fetch slpte form IOTLB */
     iotlb_entry = vtd_lookup_iotlb(s, source_id, addr);
     if (iotlb_entry) {

@@ -1182,7 +1205,7 @@ static bool vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,
          * IOMMU region can be swapped back.
          */
         vtd_pt_enable_fast_path(s, source_id);
-
+        vtd_iommu_unlock(s);
         return true;
     }
 

@@ -1203,6 +1226,7 @@ static bool vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,
     vtd_update_iotlb(s, source_id, VTD_CONTEXT_ENTRY_DID(ce.hi), addr, slpte,
                      access_flags, level);
 out:
+    vtd_iommu_unlock(s);
     entry->iova = addr & page_mask;
     entry->translated_addr = vtd_get_slpte_addr(slpte, s->aw_bits) & page_mask;
     entry->addr_mask = ~page_mask;

@@ -1210,6 +1234,7 @@ out:
     return true;
 
 error:
+    vtd_iommu_unlock(s);
     entry->iova = 0;
     entry->translated_addr = 0;
     entry->addr_mask = 0;

@@ -1258,10 +1283,13 @@ static void vtd_iommu_replay_all(IntelIOMMUState *s)
 static void vtd_context_global_invalidate(IntelIOMMUState *s)
 {
     trace_vtd_inv_desc_cc_global();
+    /* Protects context cache */
+    vtd_iommu_lock(s);
     s->context_cache_gen++;
     if (s->context_cache_gen == VTD_CONTEXT_CACHE_GEN_MAX) {
         vtd_reset_context_cache(s);
     }
+    vtd_iommu_unlock(s);
     vtd_switch_address_space_all(s);
     /*
      * From VT-d spec 6.5.2.1, a global context entry invalidation

@@ -1377,8 +1405,10 @@ static void vtd_iotlb_domain_invalidate(IntelIOMMUState *s, uint16_t domain_id)
 
     trace_vtd_inv_desc_iotlb_domain(domain_id);
 
+    vtd_iommu_lock(s);
     g_hash_table_foreach_remove(s->iotlb, vtd_hash_remove_by_domain,
                                 &domain_id);
+    vtd_iommu_unlock(s);
 
     QLIST_FOREACH(vtd_as, &s->notifiers_list, next) {
         if (!vtd_dev_to_context_entry(s, pci_bus_num(vtd_as->bus),

@@ -1426,7 +1456,9 @@ static void vtd_iotlb_page_invalidate(IntelIOMMUState *s, uint16_t domain_id,
     info.domain_id = domain_id;
     info.addr = addr;
     info.mask = ~((1 << am) - 1);
+    vtd_iommu_lock(s);
     g_hash_table_foreach_remove(s->iotlb, vtd_hash_remove_by_page, &info);
+    vtd_iommu_unlock(s);
     vtd_iotlb_page_invalidate_notify(s, domain_id, addr, am);
 }
 

Hi Peter,

Quoted 3 lines · Peter Xu

Add a per-iommu big lock to protect IOMMU status. Currently the only thing to be protected is the IOTLB/context cache, since that can be accessed even without BQL, e.g., in IO dataplane.

As discussed together, Peter challenged per device mutex in "Re: [PATCH v11 15/17] hw/arm/smmuv3: Cache/invalidate config data"
https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg02403.html
but at that time I fail to justify the use :-(

Quoted 55 lines

Note that we don't need to protect device page tables since that's fully controlled by the guest kernel. However there is still possibility that malicious drivers will program the device to not obey the rule. In that case QEMU can't really do anything useful, instead the guest itself will be responsible for all uncertainties.

Reported-by Fam Zheng <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 include/hw/i386/intel_iommu.h |  6 +++++
 hw/i386/intel_iommu.c         | 43 +++++++++++++++++++++++++++++++----
 2 files changed, 44 insertions(+), 5 deletions(-)

include/hw/i386/intel_iommu.h +6 -0

index 220697253f..ee517704e7 100644
@@ -300,6 +300,12 @@ struct IntelIOMMUState {
     OnOffAuto intr_eim;             /* Toggle for EIM cabability */
     bool buggy_eim;                 /* Force buggy EIM unless eim=off */
     uint8_t aw_bits;                /* Host/IOVA address width (in bits) */
+
+    /*
+     * Protects IOMMU states in general.  Currently it protects the
+     * per-IOMMU IOTLB cache, and context entry cache in VTDAddressSpace.
+     */
+    QemuMutex iommu_lock;
 };
 
 /* Find the VTD Address space associated with the given bus pointer,

hw/i386/intel_iommu.c +10 -0

index 5987b48d43..112971638d 100644
@@ -128,6 +128,16 @@ static uint64_t vtd_set_clear_mask_quad(IntelIOMMUState *s, hwaddr addr,
     return new_val;
 }
 
+static inline void vtd_iommu_lock(IntelIOMMUState *s)
+{
+    qemu_mutex_lock(&s->iommu_lock);
+}
+
+static inline void vtd_iommu_unlock(IntelIOMMUState *s)
+{
+    qemu_mutex_unlock(&s->iommu_lock);
+}
+
 /* GHashTable functions */
 static gboolean vtd_uint64_equal(gconstpointer v1, gconstpointer v2)
 {

@@ -172,7 +182,7 @@ static gboolean vtd_hash_remove_by_page(gpointer key, gpointer value,
 }
 

hw/i386/intel_iommu.c static gboolean vtd_hash_remove_by_page(gpointer key, gpointer value, · line 185

Quoted 3 lines

hw/i386/intel_iommu.c +1 -1

 /* Reset all the gen of VTDAddressSpace to zero and set the gen of
- * IntelIOMMUState to 1.
+ * IntelIOMMUState to 1.  Must be with IOMMU lock held.

s/Must be/ Must be called ?

not done in vtd_init()

hw/i386/intel_iommu.c static void vtd_reset_context_cache(IntelIOMMUState *s) · line 210

Quoted 9 lines

hw/i386/intel_iommu.c +1 -1

  */
 static void vtd_reset_context_cache(IntelIOMMUState *s)
 {

@@ -197,12 +207,19 @@ static void vtd_reset_context_cache(IntelIOMMUState *s)
     s->context_cache_gen = 1;
 }
 
-static void vtd_reset_iotlb(IntelIOMMUState *s)
+static void vtd_reset_iotlb_locked(IntelIOMMUState *s)

add the above comment and keep the original name?

hw/i386/intel_iommu.c static void vtd_reset_context_cache(IntelIOMMUState *s) · line 216

Quoted 6 lines

hw/i386/intel_iommu.c +1 -0

 {
     assert(s->iotlb);
     g_hash_table_remove_all(s->iotlb);
 }
 
+static void vtd_reset_iotlb(IntelIOMMUState *s)

Quoted 117 lines

hw/i386/intel_iommu.c +24 -3

+{
+    vtd_iommu_lock(s);
+    vtd_reset_iotlb_locked(s);
+    vtd_iommu_unlock(s);
+}
+
 static uint64_t vtd_get_iotlb_key(uint64_t gfn, uint16_t source_id,
                                   uint32_t level)
 {

@@ -215,6 +232,7 @@ static uint64_t vtd_get_iotlb_gfn(hwaddr addr, uint32_t level)
     return (addr & vtd_slpt_level_page_mask(level)) >> VTD_PAGE_SHIFT_4K;
 }
 
+/* Must be with IOMMU lock held */
 static VTDIOTLBEntry *vtd_lookup_iotlb(IntelIOMMUState *s, uint16_t source_id,
                                        hwaddr addr)
 {

@@ -235,6 +253,7 @@ out:
     return entry;
 }
 
+/* Must be with IOMMU lock held */
 static void vtd_update_iotlb(IntelIOMMUState *s, uint16_t source_id,
                              uint16_t domain_id, hwaddr addr, uint64_t slpte,
                              uint8_t access_flags, uint32_t level)

@@ -246,7 +265,7 @@ static void vtd_update_iotlb(IntelIOMMUState *s, uint16_t source_id,
     trace_vtd_iotlb_page_update(source_id, addr, slpte, domain_id);
     if (g_hash_table_size(s->iotlb) >= VTD_IOTLB_MAX_SIZE) {
         trace_vtd_iotlb_reset("iotlb exceeds size limit");
-        vtd_reset_iotlb(s);
+        vtd_reset_iotlb_locked(s);
     }
 
     entry->gfn = gfn;

@@ -1106,7 +1125,7 @@ static bool vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,
     IntelIOMMUState *s = vtd_as->iommu_state;
     VTDContextEntry ce;
     uint8_t bus_num = pci_bus_num(bus);
-    VTDContextCacheEntry *cc_entry = &vtd_as->context_cache_entry;
+    VTDContextCacheEntry *cc_entry;
     uint64_t slpte, page_mask;
     uint32_t level;
     uint16_t source_id = vtd_make_source_id(bus_num, devfn);

@@ -1123,6 +1142,10 @@ static bool vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,
      */
     assert(!vtd_is_interrupt_addr(addr));
 
+    vtd_iommu_lock(s);
+
+    cc_entry = &vtd_as->context_cache_entry;
+
     /* Try to fetch slpte form IOTLB */
     iotlb_entry = vtd_lookup_iotlb(s, source_id, addr);
     if (iotlb_entry) {

@@ -1182,7 +1205,7 @@ static bool vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,
          * IOMMU region can be swapped back.
          */
         vtd_pt_enable_fast_path(s, source_id);
-
+        vtd_iommu_unlock(s);
         return true;
     }
 

@@ -1203,6 +1226,7 @@ static bool vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,
     vtd_update_iotlb(s, source_id, VTD_CONTEXT_ENTRY_DID(ce.hi), addr, slpte,
                      access_flags, level);
 out:
+    vtd_iommu_unlock(s);
     entry->iova = addr & page_mask;
     entry->translated_addr = vtd_get_slpte_addr(slpte, s->aw_bits) & page_mask;
     entry->addr_mask = ~page_mask;

@@ -1210,6 +1234,7 @@ out:
     return true;
 
 error:
+    vtd_iommu_unlock(s);
     entry->iova = 0;
     entry->translated_addr = 0;
     entry->addr_mask = 0;

@@ -1258,10 +1283,13 @@ static void vtd_iommu_replay_all(IntelIOMMUState *s)
 static void vtd_context_global_invalidate(IntelIOMMUState *s)
 {
     trace_vtd_inv_desc_cc_global();
+    /* Protects context cache */
+    vtd_iommu_lock(s);
     s->context_cache_gen++;
     if (s->context_cache_gen == VTD_CONTEXT_CACHE_GEN_MAX) {
         vtd_reset_context_cache(s);
     }
+    vtd_iommu_unlock(s);
     vtd_switch_address_space_all(s);
     /*
      * From VT-d spec 6.5.2.1, a global context entry invalidation

@@ -1377,8 +1405,10 @@ static void vtd_iotlb_domain_invalidate(IntelIOMMUState *s, uint16_t domain_id)
 
     trace_vtd_inv_desc_iotlb_domain(domain_id);
 
+    vtd_iommu_lock(s);
     g_hash_table_foreach_remove(s->iotlb, vtd_hash_remove_by_domain,
                                 &domain_id);
+    vtd_iommu_unlock(s);
 
     QLIST_FOREACH(vtd_as, &s->notifiers_list, next) {
         if (!vtd_dev_to_context_entry(s, pci_bus_num(vtd_as->bus),

@@ -1426,7 +1456,9 @@ static void vtd_iotlb_page_invalidate(IntelIOMMUState *s, uint16_t domain_id,
     info.domain_id = domain_id;
     info.addr = addr;
     info.mask = ~((1 << am) - 1);
+    vtd_iommu_lock(s);
     g_hash_table_foreach_remove(s->iotlb, vtd_hash_remove_by_page, &info);
+    vtd_iommu_unlock(s);
     vtd_iotlb_page_invalidate_notify(s, domain_id, addr, am);
 }
 

@@ -3072,6 +3104,7 @@ static void vtd_realize(DeviceState *dev, Error **errp)
     }
 

hw/i386/intel_iommu.c static void vtd_realize(DeviceState *dev, Error **errp) · line 3110

Quoted 6 lines

hw/i386/intel_iommu.c +1 -0

     QLIST_INIT(&s->notifiers_list);
+    qemu_mutex_init(&s->iommu_lock);
     memset(s->vtd_as_by_bus_num, 0, sizeof(s->vtd_as_by_bus_num));
     memory_region_init_io(&s->csrmem, OBJECT(s), &vtd_mem_ops, s,
                           "intel_iommu", DMAR_REG_SIZE);

Don't you need to take the lock in vtd_context_device_invalidate() which manipulates the vtd_as->context_cache_entry.context_cache_gen?

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 <[email protected]>

---
 include/hw/i386/intel_iommu.h |  2 ++
 hw/i386/intel_iommu.c         | 43 +++++++++++++++++++++++++++++++----
 2 files changed, 40 insertions(+), 5 deletions(-)

include/hw/i386/intel_iommu.h +2 -0

index ee517704e7..9e0a6c1c6a 100644
@@ -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 {

hw/i386/intel_iommu.c +0 -0

index 112971638d..9a418abfb6 100644

hw/i386/intel_iommu.c +5 -0

+{
+    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),

hw/i386/intel_iommu.c +13 -0

+                 */
+                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);

Hi Peter,

Quoted 1 line · Peter Xu

For UNMAP-only IOMMU notifiers, we don't really need to walk the page

s/really// ;-)

Quoted 30 lines

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 <[email protected]>

---
 include/hw/i386/intel_iommu.h |  2 ++
 hw/i386/intel_iommu.c         | 43 +++++++++++++++++++++++++++++++----
 2 files changed, 40 insertions(+), 5 deletions(-)

include/hw/i386/intel_iommu.h +2 -0

index ee517704e7..9e0a6c1c6a 100644
@@ -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 {

hw/i386/intel_iommu.c +0 -0

index 112971638d..9a418abfb6 100644
@@ -138,6 +138,12 @@ static inline void vtd_iommu_unlock(IntelIOMMUState *s)
     qemu_mutex_unlock(&s->iommu_lock);
 }
 

hw/i386/intel_iommu.c static inline void vtd_iommu_unlock(IntelIOMMUState *s) · line 142

Quoted 2 lines

hw/i386/intel_iommu.c +2 -0

+/* 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 ;-)

Quoted 22 lines

hw/i386/intel_iommu.c +7 -3

+{
+    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)) {
+                /*

hw/i386/intel_iommu.c static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s, · line 1452

Quoted 3 lines

hw/i386/intel_iommu.c +3 -0

+                 * 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)?

hw/i386/intel_iommu.c static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s, · line 1460

Quoted 8 lines

hw/i386/intel_iommu.c +8 -0

+                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?

Quoted 31 lines

hw/i386/intel_iommu.c +15 -2

+                 */
+                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 */

hw/i386/intel_iommu.c static void vtd_iommu_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n) · line 2930

Quoted 7 lines

hw/i386/intel_iommu.c +3 -0

+            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!

Quoted 5 lines · Auger Eric

Hi Peter,

Quoted 1 line · Peter Xu

For UNMAP-only IOMMU notifiers, we don't really need to walk the page

s/really// ;-)

Ok.

Quoted 30 lines

Quoted 30 lines

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 <[email protected]>

---
 include/hw/i386/intel_iommu.h |  2 ++
 hw/i386/intel_iommu.c         | 43 +++++++++++++++++++++++++++++++----
 2 files changed, 40 insertions(+), 5 deletions(-)

include/hw/i386/intel_iommu.h +2 -0

index ee517704e7..9e0a6c1c6a 100644
@@ -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 {

hw/i386/intel_iommu.c +0 -0

index 112971638d..9a418abfb6 100644
@@ -138,6 +138,12 @@ static inline void vtd_iommu_unlock(IntelIOMMUState *s)
     qemu_mutex_unlock(&s->iommu_lock);
 }
 

Quoted 3 lines

hw/i386/intel_iommu.c static inline void vtd_iommu_unlock(IntelIOMMUState *s) · line 142

Quoted 2 lines

hw/i386/intel_iommu.c +2 -0

+/* 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.

Quoted 22 lines

Quoted 22 lines

hw/i386/intel_iommu.c +7 -3

+{
+    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)) {
+                /*

Quoted 8 lines

hw/i386/intel_iommu.c static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s, · line 1452

Quoted 3 lines

hw/i386/intel_iommu.c +3 -0

+                 * 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.

Quoted 15 lines

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)?

Quoted 8 lines

hw/i386/intel_iommu.c +8 -0

+                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.

Quoted 1 line

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.

Quoted 32 lines

Quoted 32 lines

hw/i386/intel_iommu.c +15 -2

+                 */
+                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 */

Quoted 8 lines

hw/i386/intel_iommu.c static void vtd_iommu_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n) · line 2930

Quoted 7 lines

hw/i386/intel_iommu.c +3 -0

+            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,

Hi Peter,

Quoted 77 lines · Peter Xu

Quoted 5 lines · Auger Eric

Hi Peter,

Quoted 1 line · Peter Xu

For UNMAP-only IOMMU notifiers, we don't really need to walk the page

s/really// ;-)

Ok.

Quoted 30 lines

Quoted 30 lines

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 <[email protected]>

---
 include/hw/i386/intel_iommu.h |  2 ++
 hw/i386/intel_iommu.c         | 43 +++++++++++++++++++++++++++++++----
 2 files changed, 40 insertions(+), 5 deletions(-)

include/hw/i386/intel_iommu.h +2 -0

index ee517704e7..9e0a6c1c6a 100644
@@ -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 {

hw/i386/intel_iommu.c +0 -0

index 112971638d..9a418abfb6 100644
@@ -138,6 +138,12 @@ static inline void vtd_iommu_unlock(IntelIOMMUState *s)
     qemu_mutex_unlock(&s->iommu_lock);
 }
 

Quoted 3 lines

hw/i386/intel_iommu.c static inline void vtd_iommu_unlock(IntelIOMMUState *s) · line 142

Quoted 2 lines

hw/i386/intel_iommu.c +2 -0

+/* 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.

Quoted 30 lines

Quoted 15 lines

hw/i386/intel_iommu.c +5 -0

+{
+    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),

hw/i386/intel_iommu.c static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s, · line 1452

Quoted 10 lines

hw/i386/intel_iommu.c +5 -3

                                        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?

Quoted 2 lines

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.

Quoted 21 lines

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

Quoted 16 lines

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)?

hw/i386/intel_iommu.c static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s, · line 1460

Quoted 8 lines

hw/i386/intel_iommu.c +8 -0

+                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?

Quoted 5 lines

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.

During the recursive page walking of IOVA page tables, some stack variables are constant variables and never changed during the whole page walking procedure. Isolate them into a struct so that we don't need to pass those contants down the stack every time and multiple times.

Signed-off-by Peter Xu <[email protected]>

---
 hw/i386/intel_iommu.c | 56 ++++++++++++++++++++++++++++---------------
 1 file changed, 37 insertions(+), 19 deletions(-)

hw/i386/intel_iommu.c +37 -19

index 9a418abfb6..b2b2a0a441 100644
@@ -747,9 +747,27 @@ static int vtd_iova_to_slpte(VTDContextEntry *ce, uint64_t iova, bool is_write,
 
 typedef int (*vtd_page_walk_hook)(IOMMUTLBEntry *entry, void *private);
 
+/**
+ * Constant information used during page walking
+ *
+ * @hook_fn: hook func to be called when detected page
+ * @private: private data to be passed into hook func
+ * @notify_unmap: whether we should notify invalid entries
+ * @aw: maximum address width
+ */
+typedef struct {
+    vtd_page_walk_hook hook_fn;
+    void *private;
+    bool notify_unmap;
+    uint8_t aw;
+} vtd_page_walk_info;
+
 static int vtd_page_walk_one(IOMMUTLBEntry *entry, int level,
-                             vtd_page_walk_hook hook_fn, void *private)
+                             vtd_page_walk_info *info)
 {
+    vtd_page_walk_hook hook_fn = info->hook_fn;
+    void *private = info->private;
+
     assert(hook_fn);
     trace_vtd_page_walk_one(level, entry->iova, entry->translated_addr,
                             entry->addr_mask, entry->perm);

@@ -762,17 +780,13 @@ static int vtd_page_walk_one(IOMMUTLBEntry *entry, int level,
  * @addr: base GPA addr to start the walk
  * @start: IOVA range start address
  * @end: IOVA range end address (start <= addr < end)
- * @hook_fn: hook func to be called when detected page
- * @private: private data to be passed into hook func
  * @read: whether parent level has read permission
  * @write: whether parent level has write permission
- * @notify_unmap: whether we should notify invalid entries
- * @aw: maximum address width
+ * @info: constant information for the page walk
  */
 static int vtd_page_walk_level(dma_addr_t addr, uint64_t start,
-                               uint64_t end, vtd_page_walk_hook hook_fn,
-                               void *private, uint32_t level, bool read,
-                               bool write, bool notify_unmap, uint8_t aw)
+                               uint64_t end, uint32_t level, bool read,
+                               bool write, vtd_page_walk_info *info)
 {
     bool read_cur, write_cur, entry_valid;
     uint32_t offset;

@@ -822,24 +836,24 @@ static int vtd_page_walk_level(dma_addr_t addr, uint64_t start,
 
         if (vtd_is_last_slpte(slpte, level)) {
             /* NOTE: this is only meaningful if entry_valid == true */
-            entry.translated_addr = vtd_get_slpte_addr(slpte, aw);
-            if (!entry_valid && !notify_unmap) {
+            entry.translated_addr = vtd_get_slpte_addr(slpte, info->aw);
+            if (!entry_valid && !info->notify_unmap) {
                 trace_vtd_page_walk_skip_perm(iova, iova_next);
                 goto next;
             }
-            ret = vtd_page_walk_one(&entry, level, hook_fn, private);
+            ret = vtd_page_walk_one(&entry, level, info);
             if (ret < 0) {
                 return ret;
             }
         } else {
             if (!entry_valid) {
-                if (notify_unmap) {
+                if (info->notify_unmap) {
                     /*
                      * The whole entry is invalid; unmap it all.
                      * Translated address is meaningless, zero it.
                      */
                     entry.translated_addr = 0x0;
-                    ret = vtd_page_walk_one(&entry, level, hook_fn, private);
+                    ret = vtd_page_walk_one(&entry, level, info);
                     if (ret < 0) {
                         return ret;
                     }

@@ -848,10 +862,9 @@ static int vtd_page_walk_level(dma_addr_t addr, uint64_t start,
                 }
                 goto next;
             }
-            ret = vtd_page_walk_level(vtd_get_slpte_addr(slpte, aw), iova,
-                                      MIN(iova_next, end), hook_fn, private,
-                                      level - 1, read_cur, write_cur,
-                                      notify_unmap, aw);
+            ret = vtd_page_walk_level(vtd_get_slpte_addr(slpte, info->aw),
+                                      iova, MIN(iova_next, end), level - 1,
+                                      read_cur, write_cur, info);
             if (ret < 0) {
                 return ret;
             }

@@ -880,6 +893,12 @@ static int vtd_page_walk(VTDContextEntry *ce, uint64_t start, uint64_t end,
 {
     dma_addr_t addr = vtd_ce_get_slpt_base(ce);
     uint32_t level = vtd_ce_get_level(ce);
+    vtd_page_walk_info info = {
+        .hook_fn = hook_fn,
+        .private = private,
+        .notify_unmap = notify_unmap,
+        .aw = aw,
+    };
 
     if (!vtd_iova_range_check(start, ce, aw)) {
         return -VTD_FR_ADDR_BEYOND_MGAW;

@@ -890,8 +909,7 @@ static int vtd_page_walk(VTDContextEntry *ce, uint64_t start, uint64_t end,
         end = vtd_iova_limit(ce, aw);
     }
 
-    return vtd_page_walk_level(addr, start, end, hook_fn, private,
-                               level, true, true, notify_unmap, aw);
+    return vtd_page_walk_level(addr, start, end, level, true, true, &info);
 }
 
 /* Map a device to its corresponding domain (context-entry) */

Hi Peter,

Quoted 6 lines · Peter Xu

During the recursive page walking of IOVA page tables, some stack variables are constant variables and never changed during the whole page walking procedure. Isolate them into a struct so that we don't need to pass those contants down the stack every time and multiple times.

Signed-off-by Peter Xu <[email protected]>

Reviewed-by Eric Auger <[email protected]>

Quoted 8 lines · Auger Eric

Hi Peter,

Quoted 5 lines · Peter Xu

During the recursive page walking of IOVA page tables, some stack variables are constant variables and never changed during the whole page walking procedure. Isolate them into a struct so that we don't need to pass those contants down the stack every time and multiple times.

Quoted 2 lines

Quoted 1 line

Signed-off-by Peter Xu <[email protected]>

Reviewed-by Eric Auger <[email protected]>

Thanks for the r-b, but this patch is changed in version 3. Please feel free to have a look at that version.

(For all the review comments previously to version 2, I adopted them
when the contents are still there in version 3)

Regards,

We pass in the VTDAddressSpace to replace the aw bits when doing page walk. The VTDAddressSpace contains the aw bits information, meanwhile we'll need to do something more in the follow up patches regarding to the address spaces.

Signed-off-by Peter Xu <[email protected]>

---
 hw/i386/intel_iommu.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

hw/i386/intel_iommu.c +11 -9

index b2b2a0a441..83769f2b8c 100644
@@ -753,13 +753,13 @@ typedef int (*vtd_page_walk_hook)(IOMMUTLBEntry *entry, void *private);
  * @hook_fn: hook func to be called when detected page
  * @private: private data to be passed into hook func
  * @notify_unmap: whether we should notify invalid entries
- * @aw: maximum address width
+ * @as: VT-d address space of the device
  */
 typedef struct {
+    VTDAddressSpace *as;
     vtd_page_walk_hook hook_fn;
     void *private;
     bool notify_unmap;
-    uint8_t aw;
 } vtd_page_walk_info;
 
 static int vtd_page_walk_one(IOMMUTLBEntry *entry, int level,

@@ -796,6 +796,7 @@ static int vtd_page_walk_level(dma_addr_t addr, uint64_t start,
     uint64_t iova = start;
     uint64_t iova_next;
     int ret = 0;
+    uint8_t aw = info->as->iommu_state->aw_bits;
 
     trace_vtd_page_walk_level(addr, level, start, end);
 

@@ -836,7 +837,7 @@ static int vtd_page_walk_level(dma_addr_t addr, uint64_t start,
 
         if (vtd_is_last_slpte(slpte, level)) {
             /* NOTE: this is only meaningful if entry_valid == true */
-            entry.translated_addr = vtd_get_slpte_addr(slpte, info->aw);
+            entry.translated_addr = vtd_get_slpte_addr(slpte, aw);
             if (!entry_valid && !info->notify_unmap) {
                 trace_vtd_page_walk_skip_perm(iova, iova_next);
                 goto next;

@@ -862,7 +863,7 @@ static int vtd_page_walk_level(dma_addr_t addr, uint64_t start,
                 }
                 goto next;
             }
-            ret = vtd_page_walk_level(vtd_get_slpte_addr(slpte, info->aw),
+            ret = vtd_page_walk_level(vtd_get_slpte_addr(slpte, aw),
                                       iova, MIN(iova_next, end), level - 1,
                                       read_cur, write_cur, info);
             if (ret < 0) {

@@ -885,19 +886,20 @@ next:
  * @end: IOVA range end address (start <= addr < end)
  * @hook_fn: the hook that to be called for each detected area
  * @private: private data for the hook function
- * @aw: maximum address width
+ * @as: the VT-d address space of the device
  */
 static int vtd_page_walk(VTDContextEntry *ce, uint64_t start, uint64_t end,
                          vtd_page_walk_hook hook_fn, void *private,
-                         bool notify_unmap, uint8_t aw)
+                         bool notify_unmap, VTDAddressSpace *as)
 {
     dma_addr_t addr = vtd_ce_get_slpt_base(ce);
     uint32_t level = vtd_ce_get_level(ce);
+    uint8_t aw = as->iommu_state->aw_bits;
     vtd_page_walk_info info = {
         .hook_fn = hook_fn,
         .private = private,
         .notify_unmap = notify_unmap,
-        .aw = aw,
+        .as = as,
     };
 
     if (!vtd_iova_range_check(start, ce, aw)) {

@@ -1470,7 +1472,7 @@ static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s,
                  */
                 vtd_page_walk(&ce, addr, addr + size,
                               vtd_page_invalidate_notify_hook,
-                              (void *)&vtd_as->iommu, true, s->aw_bits);
+                              (void *)&vtd_as->iommu, true, vtd_as);
             } else {
                 /*
                  * For UNMAP-only notifiers, we don't need to walk the

@@ -2941,7 +2943,7 @@ static void vtd_iommu_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n)
         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);
+                          vtd_as);
         }
     } else {
         trace_vtd_replay_ce_invalid(bus_n, PCI_SLOT(vtd_as->devfn),

Hi Peter,

Quoted 6 lines · Peter Xu

We pass in the VTDAddressSpace to replace the aw bits when doing page walk. The VTDAddressSpace contains the aw bits information, meanwhile we'll need to do something more in the follow up patches regarding to the address spaces.

Signed-off-by Peter Xu <[email protected]>

Reviewed-by Eric Auger <[email protected]>

IOMMU replay was carried out before in many use cases, e.g., context cache invalidations, domain flushes. We used this mechanism to sync the shadow page table by firstly (1) unmap the whole address space, then
(2) walk the page table to remap what's in the table.

This is very dangerous.

The problem is that we'll have a very small window (in my measurement, it can be about 3ms) during above step (1) and (2) that the device will see no (or incomplete) device page table. Howerver the device never knows that. This can cause DMA error of devices, who assumes the page table is always there.

So the point is that, for MAP typed notifiers (vfio-pci, for example) they'll need the mapped page entries always be there. We can never unmap any existing page entries like what we did in (1) above.

The only solution is to remove step (1). We can't do that before since we didn't know what device page was mapped and what was not, so we unmap them all. Now with the new IOVA tree QEMU knows what has mapped and what has not. We don't need this step (1) any more. Remove it.

Note that after removing that global unmap flushing, we'll need to notify unmap now during page walkings.

This should fix the DMA error problem that Jintack Lim reported with nested device assignment. This problem won't not happen always, e.g., I cannot reproduce the error. However after collecting logs it shows that this is the possible cause to Jintack's problem.

Reported-by Jintack Lim <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 hw/i386/intel_iommu.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

hw/i386/intel_iommu.c +4 -4

index 16b3514afb..9439103cac 100644
@@ -3003,10 +3003,8 @@ static void vtd_iommu_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n)
 
     /*
      * The replay can be triggered by either a invalidation or a newly
-     * created entry. No matter what, we release existing mappings
-     * (it means flushing caches for UNMAP-only registers).
+     * created entry.
      */
-    vtd_address_space_unmap(vtd_as, n);
 
     if (vtd_dev_to_context_entry(s, bus_n, vtd_as->devfn, &ce) == 0) {
         trace_vtd_replay_ce_valid(bus_n, PCI_SLOT(vtd_as->devfn),

@@ -3015,8 +3013,10 @@ static void vtd_iommu_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n)
                                   ce.hi, ce.lo);
         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,
+            vtd_page_walk(&ce, 0, ~0ULL, vtd_replay_hook, (void *)n, true,
                           vtd_as);
+        } else {
+            vtd_address_space_unmap(vtd_as, n);
         }
     } else {
         trace_vtd_replay_ce_invalid(bus_n, PCI_SLOT(vtd_as->devfn),

Hi Peter,

Quoted 7 lines · Peter Xu

IOMMU replay was carried out before in many use cases, e.g., context cache invalidations, domain flushes. We used this mechanism to sync the shadow page table by firstly (1) unmap the whole address space, then
(2) walk the page table to remap what's in the table.

This is very dangerous.

Quoted 5 lines

The problem is that we'll have a very small window (in my measurement, it can be about 3ms) during above step (1) and (2) that the device will see no (or incomplete) device page table. Howerver the device never knows that. This can cause DMA error of devices, who assumes the page table is always there.

But now you have the QemuMutex can we have a translate and an invalidation that occur concurrently? Don't the iotlb flush and replay happen while the lock is held?

Quoted 15 lines · Auger Eric

Hi Peter,

Quoted 12 lines · Peter Xu

IOMMU replay was carried out before in many use cases, e.g., context cache invalidations, domain flushes. We used this mechanism to sync the shadow page table by firstly (1) unmap the whole address space, then
(2) walk the page table to remap what's in the table.

This is very dangerous.

The problem is that we'll have a very small window (in my measurement, it can be about 3ms) during above step (1) and (2) that the device will see no (or incomplete) device page table. Howerver the device never knows that. This can cause DMA error of devices, who assumes the page table is always there.

Quoted 3 lines

But now you have the QemuMutex can we have a translate and an invalidation that occur concurrently? Don't the iotlb flush and replay happen while the lock is held?

Note that when we are using vfio-pci devices we can't really know when the device started a DMA since that's totally happening only between the host IOMMU and the hardware.

translation is happening in the shadow page table, not really in QEMU. So IMO we aren't protected by anything.

Also, this patch is dropped in version 3, and I did something else to achieve similar goal (by introducing the shadow page sync helper, and then for DSIs we'll use that instead of calling IOMMU replay here). Please have a look. Thanks,

=== OUTPUT BEGIN ===
Checking PATCH 1/10: intel-iommu: send PSI always even if across PDEs... Checking PATCH 2/10: intel-iommu: remove IntelIOMMUNotifierNode... Checking PATCH 3/10: intel-iommu: add iommu lock...
Checking PATCH 4/10: intel-iommu: only do page walk for MAP notifiers... Checking PATCH 5/10: intel-iommu: introduce vtd_page_walk_info... Checking PATCH 6/10: intel-iommu: pass in address space when page walk... Checking PATCH 7/10: util: implement simple interval tree logic...
ERROR: space prohibited between function name and open parenthesis '(' #56: FILE: include/qemu/interval-tree.h:33:
+typedef gboolean (*it_tree_iterator)(ITValue start, ITValue end);

total: 1 errors, 0 warnings, 343 lines checked

Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS.

v2:
- fix patchew code style warnings
- interval tree: postpone malloc when inserting; simplify node remove a bit where proper [Jason]
- fix up comment and commit message for iommu lock patch [Kevin]
- protect context cache too using the iommu lock [Kevin, Jason]
- add vast comment in patch 8 to explain the modify-PTE problem [Jason, Kevin]

Quoted 8 lines · Peter Xu

v2:
- fix patchew code style warnings
- interval tree: postpone malloc when inserting; simplify node remove a bit where proper [Jason]
- fix up comment and commit message for iommu lock patch [Kevin]
- protect context cache too using the iommu lock [Kevin, Jason]
- add vast comment in patch 8 to explain the modify-PTE problem [Jason, Kevin]

We can hold a bit on reviewing this series. Jintack reported a scp DMAR issue that might happen even with L1 guest with this series, and the scp can stall after copied tens or hundreds of MBs randomly. I'm still investigating the problem. This problem should be related to deferred flushing of VT-d kernel driver, since the problem will go away if we use "intel_iommu=on,strict". However I'm still trying to figure out what's the thing behind the scene even with that deferred flushing feature.

Meanwhile, during the investigation I found another "possibly valid" use case about the modify-PTE problem that Jason has mentioned when with deferred flushing:

vcpu1 vcpu2
map page A
explicitly send PSI for A
queue unmap page A [1]
map the same page A [2]
explcitly send PSI for A [3]
flush unmap page A [4]

Due to deferred flushing, the UNMAP PSI might be postponed (or it can be finally a DSI) from step [1] to step [4]. If we allocate the same page somewhere else, we might trigger this modify-PTE at [2] since we haven't yet received the deferred PSI to unmap A from vcpu1.

Note that this will not happen with latest upstream Linux, since the IOVA allocation algorithm in current Linux kernel made sure that the IOVA range won't be freed until [4], so we can't really allocate the same page address at [2].

However this let me tend to agree with
Jason and Kevin's worry on future potential issues if that can be triggered easily by common guest kernel bugs. So now I'm considering to drop my mergable interval tree but just use a simpler tree to cache everything including translated addresses. The metadata will possibly take 2% of managed memory if with that.

Quoted 9 lines · Peter Xu

Quoted 8 lines · Peter Xu

v2:
- fix patchew code style warnings
- interval tree: postpone malloc when inserting; simplify node remove a bit where proper [Jason]
- fix up comment and commit message for iommu lock patch [Kevin]
- protect context cache too using the iommu lock [Kevin, Jason]
- add vast comment in patch 8 to explain the modify-PTE problem [Jason, Kevin]

Quoted 8 lines

We can hold a bit on reviewing this series. Jintack reported a scp DMAR issue that might happen even with L1 guest with this series, and the scp can stall after copied tens or hundreds of MBs randomly. I'm still investigating the problem. This problem should be related to deferred flushing of VT-d kernel driver, since the problem will go away if we use "intel_iommu=on,strict". However I'm still trying to figure out what's the thing behind the scene even with that deferred flushing feature.

I vaguely remember recent upstream vfio support delayed flush, maybe it's related.

Quoted 19 lines

Meanwhile, during the investigation I found another "possibly valid" use case about the modify-PTE problem that Jason has mentioned when with deferred flushing:

vcpu1 vcpu2
map page A
explicitly send PSI for A
queue unmap page A [1]
map the same page A [2]
explcitly send PSI for A [3]
flush unmap page A [4]

Due to deferred flushing, the UNMAP PSI might be postponed (or it can be finally a DSI) from step [1] to step [4]. If we allocate the same page somewhere else, we might trigger this modify-PTE at [2] since we haven't yet received the deferred PSI to unmap A from vcpu1.

Quoted 4 lines

Note that this will not happen with latest upstream Linux, since the IOVA allocation algorithm in current Linux kernel made sure that the IOVA range won't be freed until [4], so we can't really allocate the same page address at [2].

Yes, so the vfio + vIOMMU work will probably uncover more bugs in the IOMMU driver (especially CM mode). I suspect CM mode does not have sufficient test (since it probably wasn't used in any production environment before the vfio
+ vIOMMU work).

Quoted 8 lines

However this let me tend to agree with
Jason and Kevin's worry on future potential issues if that can be triggered easily by common guest kernel bugs. So now I'm considering to drop my mergable interval tree but just use a simpler tree to cache everything including translated addresses. The metadata will possibly take 2% of managed memory if with that.

Good to know this, we can start from the way we know correct for sure then optimizations on top.

Quoted 21 lines · Jason Wang

Quoted 17 lines · Peter Xu

Quoted 8 lines · Peter Xu

v2:
- fix patchew code style warnings
- interval tree: postpone malloc when inserting; simplify node remove a bit where proper [Jason]
- fix up comment and commit message for iommu lock patch [Kevin]
- protect context cache too using the iommu lock [Kevin, Jason]
- add vast comment in patch 8 to explain the modify-PTE problem [Jason, Kevin]

We can hold a bit on reviewing this series. Jintack reported a scp DMAR issue that might happen even with L1 guest with this series, and the scp can stall after copied tens or hundreds of MBs randomly. I'm still investigating the problem. This problem should be related to deferred flushing of VT-d kernel driver, since the problem will go away if we use "intel_iommu=on,strict". However I'm still trying to figure out what's the thing behind the scene even with that deferred flushing feature.

Quoted 2 lines

I vaguely remember recent upstream vfio support delayed flush, maybe it's related.

I'm a bit confused on why vfio is related to the deferred flushing. Could you provide a pointer for this?

Quoted 22 lines · On Thu, 17 May 2018 10:45:44 +0800 Peter Xu <[email protected]> wrote:

Quoted 21 lines · Jason Wang

Quoted 17 lines · Peter Xu

Quoted 8 lines · Peter Xu

v2:
- fix patchew code style warnings
- interval tree: postpone malloc when inserting; simplify node remove a bit where proper [Jason]
- fix up comment and commit message for iommu lock patch [Kevin]
- protect context cache too using the iommu lock [Kevin, Jason]
- add vast comment in patch 8 to explain the modify-PTE problem [Jason, Kevin]

We can hold a bit on reviewing this series. Jintack reported a scp DMAR issue that might happen even with L1 guest with this series, and the scp can stall after copied tens or hundreds of MBs randomly. I'm still investigating the problem. This problem should be related to deferred flushing of VT-d kernel driver, since the problem will go away if we use "intel_iommu=on,strict". However I'm still trying to figure out what's the thing behind the scene even with that deferred flushing feature.

Quoted 5 lines

Quoted 2 lines

I vaguely remember recent upstream vfio support delayed flush, maybe it's related.

I'm a bit confused on why vfio is related to the deferred flushing. Could you provide a pointer for this?

Perhaps referring to this:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6bd06f5a486c06023a618a86e8153b91d26f75f4

Rather than calling iommu_unmap() for each chunk of a mapping we'll make multiple calls to iommu_unmap_fast() and flush with iommu_tlb_sync() to defer and batch the hardware flushing. Thanks,