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 +30 -12

index fb31de9416..b359efd6f9 100644
@@ -722,6 +722,15 @@ static int vtd_iova_to_slpte(VTDContextEntry *ce, uint64_t iova, bool is_write,
 
 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)
+{
+    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);
+                if (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);
+                    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,

Signature · 1 line

2.17.0

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 +28 -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);
+                if (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);
+                    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,

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 | 84 ++++++++++++++++++++++++++-----------------
 1 file changed, 52 insertions(+), 32 deletions(-)

hw/i386/intel_iommu.c +0 -0

index 9a418abfb6..4953d02ed0 100644

hw/i386/intel_iommu.c +41 -32

+ * @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;
             }

@@ -870,28 +883,24 @@ next:
  * @ce: context entry to walk upon
  * @start: IOVA address to start the walk
  * @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
+ * @info: page walking information struct
  */
 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)
+                         vtd_page_walk_info *info)
 {
     dma_addr_t addr = vtd_ce_get_slpt_base(ce);
     uint32_t level = vtd_ce_get_level(ce);
 
-    if (!vtd_iova_range_check(start, ce, aw)) {
+    if (!vtd_iova_range_check(start, ce, info->aw)) {
         return -VTD_FR_ADDR_BEYOND_MGAW;
     }
 
-    if (!vtd_iova_range_check(end, ce, aw)) {
+    if (!vtd_iova_range_check(end, ce, info->aw)) {
         /* Fix end so that it reaches the maximum */
-        end = vtd_iova_limit(ce, aw);
+        end = vtd_iova_limit(ce, info->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) */

@@ -1446,13 +1455,18 @@ static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s,
                                        vtd_as->devfn, &ce);
         if (!ret && domain_id == VTD_CONTEXT_ENTRY_DID(ce.hi)) {
             if (vtd_as_notify_mappings(vtd_as)) {
+                vtd_page_walk_info info = {
+                    .hook_fn = vtd_page_invalidate_notify_hook,
+                    .private = (void *)&vtd_as->iommu,
+                    .notify_unmap = true,
+                    .aw = s->aw_bits,
+                };
+
                 /*
                  * For MAP-inclusive notifiers, we need to walk the
                  * page table to sync the shadow page table.
                  */
-                vtd_page_walk(&ce, addr, addr + size,
-                              vtd_page_invalidate_notify_hook,
-                              (void *)&vtd_as->iommu, true, s->aw_bits);
+                vtd_page_walk(&ce, addr, addr + size, &info);
             } else {
                 /*
                  * For UNMAP-only notifiers, we don't need to walk the

@@ -2922,8 +2936,14 @@ 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,
-                          s->aw_bits);
+            vtd_page_walk_info info = {

We pass in the VTDAddressSpace too. It'll be used in the follow up patches.

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

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

hw/i386/intel_iommu.c +4 -0

index 4953d02ed0..fe5ee77d46 100644
@@ -753,9 +753,11 @@ 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
+ * @as: VT-d address space of the device
  * @aw: maximum address width
  */
 typedef struct {
+    VTDAddressSpace *as;
     vtd_page_walk_hook hook_fn;
     void *private;
     bool notify_unmap;

@@ -1460,6 +1462,7 @@ static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s,
                     .private = (void *)&vtd_as->iommu,
                     .notify_unmap = true,
                     .aw = s->aw_bits,
+                    .as = vtd_as,
                 };
 
                 /*

@@ -2941,6 +2944,7 @@ static void vtd_iommu_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n)
                 .private = (void *)n,
                 .notify_unmap = false,
                 .aw = s->aw_bits,
+                .as = vtd_as,
             };
 
             vtd_page_walk(&ce, 0, ~0ULL, &info);

hw/i386/intel_iommu.c +6 -0

+    /* Update local IOVA mapped ranges */
+    if (entry->perm) {
+        if (mapped) {
+            /* If it's exactly the same translation, skip */
+            if (!memcmp(mapped, &target, sizeof(target))) {
+                trace_vtd_page_walk_one_skip_map(entry->iova, entry->addr_mask,

hw/i386/intel_iommu.c +36 -0

+                 * our best to remap.  Note that there will be a small
+                 * window that we don't have map at all.  But that's
+                 * the best effort we can do, and logically
+                 * well-behaved guests should not really using this
+                 * DMA region yet so we should be very safe.
+                 */
+                IOMMUAccessFlags cache_perm = entry->perm;
+                int ret;
+
+                /* Emulate an UNMAP */
+                entry->perm = IOMMU_NONE;
+                trace_vtd_page_walk_one(info->domain_id,
+                                        entry->iova,
+                                        entry->translated_addr,
+                                        entry->addr_mask,
+                                        entry->perm);
+                ret = hook_fn(entry, private);
+                if (ret) {
+                    return ret;
+                }
+                /* Drop any existing mapping */
+                iova_tree_remove(as->iova_tree, &target);
+                /* Recover the correct permission */
+                entry->perm = cache_perm;
+            }
+        }
+        iova_tree_insert(as->iova_tree, &target);
+    } else {
+        if (!mapped) {
+            /* Skip since we didn't map this range at all */
+            trace_vtd_page_walk_one_skip_unmap(entry->iova, entry->addr_mask);
+            return 0;
+        }
+        iova_tree_remove(as->iova_tree, &target);
+    }
+
     trace_vtd_page_walk_one(info->domain_id, entry->iova,
                             entry->translated_addr, entry->addr_mask,
                             entry->perm);

Firstly, introduce the sync_shadow_page_table() helper to resync the whole shadow page table of an IOMMU address space. Meanwhile, when we receive domain invalidation or similar requests (for example, context entry invalidations, global invalidations, ...), we should not really run the replay logic, instead we can now use the new sync shadow page table API to resync the whole shadow page table.

There will be two major differences:

1. We don't unmap-all before walking the page table, we just sync. The global unmap-all can create a very small window that the page table is invalid or incomplete

2. We only walk the page table once now (while replay can be triggered multiple times depending on how many notifiers there are)

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

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

hw/i386/intel_iommu.c +9 -5

index a1a2a009c1..fbb2f763f0 100644
@@ -1065,6 +1065,11 @@ static int vtd_sync_shadow_page_table_range(VTDAddressSpace *vtd_as,
     return vtd_page_walk(&ce_cache, addr, addr + size, &info);
 }
 
+static int vtd_sync_shadow_page_table(VTDAddressSpace *vtd_as)
+{
+    return vtd_sync_shadow_page_table_range(vtd_as, NULL, 0, UINT64_MAX);
+}
+
 /*
  * Fetch translation type for specific device. Returns <0 if error
  * happens, otherwise return the shifted type to check against

@@ -1397,7 +1402,7 @@ static void vtd_iommu_replay_all(IntelIOMMUState *s)
     VTDAddressSpace *vtd_as;
 
     QLIST_FOREACH(vtd_as, &s->notifiers_list, next) {
-        memory_region_iommu_replay_all(&vtd_as->iommu);
+        vtd_sync_shadow_page_table(vtd_as);
     }
 }
 

@@ -1470,14 +1475,13 @@ static void vtd_context_device_invalidate(IntelIOMMUState *s,
                 vtd_switch_address_space(vtd_as);
                 /*
                  * So a device is moving out of (or moving into) a
-                 * domain, a replay() suites here to notify all the
-                 * IOMMU_NOTIFIER_MAP registers about this change.
+                 * domain, resync the shadow page table.
                  * This won't bring bad even if we have no such
                  * notifier registered - the IOMMU notification
                  * framework will skip MAP notifications if that
                  * happened.
                  */
-                memory_region_iommu_replay_all(&vtd_as->iommu);
+                vtd_sync_shadow_page_table(vtd_as);
             }
         }
     }

@@ -1535,7 +1539,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_CONTEXT_ENTRY_DID(ce.hi)) {
-            memory_region_iommu_replay_all(&vtd_as->iommu);
+            vtd_sync_shadow_page_table(vtd_as);
         }
     }
 }

Signature · 1 line

2.17.0

Quoted 7 lines · Peter Xu

Firstly, introduce the sync_shadow_page_table() helper to resync the whole shadow page table of an IOMMU address space. Meanwhile, when we receive domain invalidation or similar requests (for example, context entry invalidations, global invalidations, ...), we should not really run the replay logic, instead we can now use the new sync shadow page table API to resync the whole shadow page table.

Quoted 5 lines

There will be two major differences:

1. We don't unmap-all before walking the page table, we just sync. The global unmap-all can create a very small window that the page table is invalid or incomplete

The implication is that with vfio, device might stop working without this change.

(Hello, Jintack, Feel free to test this branch again against your scp
error case when you got free time)

I rewrote some of the patches in V3. Major changes:

- Dropped mergable interval tree, instead introduced IOVA tree, which is even simpler.

- Fix the scp error issue that Jintack reported. Please see patches for detailed information. That's the major reason to rewrite a few of the patches. We use replay for domain flushes are possibly incorrect in the past. The thing is that IOMMU replay has an "definition" that "we should only send MAP when new page detected", while for shadow page syncing we actually need something else than that. So in this version I started to use a new vtd_sync_shadow_page_table() helper to do the page sync.

- Some other refines after the refactoring.

I'll add unit test for the IOVA tree after this series merged to make sure we won't switch to another new tree implementaion...

The element size in the new IOVA tree should be around sizeof(GTreeNode + IOMMUTLBEntry) ~= (5*8+4*8) = 72 bytes. So the worst case usage ratio would be 72/4K=2%, which still seems acceptable (it means 8G L2 guest will use 8G*2%=160MB as metadata to maintain the mapping in QEMU).

I did explicit test with scp this time, copying 1G sized file for >10 times on each of the following case:

- L1 guest, with vIOMMU and with assigned device
- L2 guest, without vIOMMU and with assigned device
- L2 guest, with vIOMMU (so 3-layer nested IOMMU) and with assigned device

Please review. Thanks,

(Below are old content from previous cover letter)

==========================

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]

Online repo:

https://github.com/xzpeter/qemu/tree/fix-vtd-dma

This series fixes several major problems that current code has:

- Issue 1: when getting very big PSI UNMAP invalidations, the current code is buggy in that we might skip the notification while actually we should always send that notification.

- Issue 2: IOTLB is not thread safe, while block dataplane can be accessing and updating it in parallel.

- Issue 3: For devices that only registered with UNMAP-only notifiers, we don't really need to do page walking for PSIs, we can directly deliver the notification down. For example, vhost.

- Issue 4: unsafe window for MAP notified devices like vfio-pci (and in the future, vDPA as well). The problem is that, now for domain invalidations we do this to make sure the shadow page tables are correctly synced:

1. unmap the whole address space
2. replay the whole address space, map existing pages

However during step 1 and 2 there will be a very tiny window (it can be as big as 3ms) that the shadow page table is either invalid or incomplete (since we're rebuilding it up). That's fatal error since devices never know that happending and it's still possible to DMA to memories.

Patch 1 fixes issue 1. I put it at the first since it's picked from an old post.

Patch 2 is a cleanup to remove useless IntelIOMMUNotifierNode struct.

Patch 3 fixes issue 2.

Patch 4 fixes issue 3.

Patch 5-9 fix issue 4. Here a very simple interval tree is implemented based on Gtree. It's different with general interval tree in that it does not allow user to pass in private data (e.g., translated addresses). However that benefits us that then we can merge adjacent interval leaves so that hopefully we won't consume much memory even if the mappings are a lot (that happens for nested virt - when mapping the whole L2 guest RAM range, it can be at least in GBs).

Patch 10 is another big cleanup only can work after patch 9.

Tests:

- device assignments to L1, even L2 guests. With this series applied (and the kernel IOMMU patches: https://lkml.org/lkml/2018/4/18/5),
we can even nest vIOMMU now, e.g., we can specify vIOMMU in L2 guest with assigned devices and things will work. We can't before.

- vhost smoke test for regression.

Please review. Thanks,

Peter Xu (12):
intel-iommu: send PSI always even if across PDEs
intel-iommu: remove IntelIOMMUNotifierNode
intel-iommu: add iommu lock
intel-iommu: only do page walk for MAP notifiers
intel-iommu: introduce vtd_page_walk_info
intel-iommu: pass in address space when page walk
intel-iommu: trace domain id during page walk
util: implement simple iova tree
intel-iommu: maintain per-device iova ranges
intel-iommu: simplify page walk logic
intel-iommu: new vtd_sync_shadow_page_table_range
intel-iommu: new sync_shadow_page_table

 include/hw/i386/intel_iommu.h |  19 +-
 include/qemu/iova-tree.h      | 134 ++++++++++++
 hw/i386/intel_iommu.c         | 381 +++++++++++++++++++++++++---------
 util/iova-tree.c              | 114 ++++++++++
 MAINTAINERS                   |   6 +
 hw/i386/trace-events          |   5 +-
 util/Makefile.objs            |   1 +
 7 files changed, 556 insertions(+), 104 deletions(-)
 create mode 100644 include/qemu/iova-tree.h
 create mode 100644 util/iova-tree.c

--
2.17.0

Quoted 2 lines · Peter Xu

(Hello, Jintack, Feel free to test this branch again against your scp
error case when you got free time)

Hi Peter,

Quoted 6 lines

I rewrote some of the patches in V3. Major changes:

- Dropped mergable interval tree, instead introduced IOVA tree, which is even simpler.

Quoted 8 lines

- Fix the scp error issue that Jintack reported. Please see patches for detailed information. That's the major reason to rewrite a few of the patches. We use replay for domain flushes are possibly incorrect in the past. The thing is that IOMMU replay has an "definition" that "we should only send MAP when new page detected", while for shadow page syncing we actually need something else than that. So in this version I started to use a new vtd_sync_shadow_page_table() helper to do the page sync.

I checked that the scp problem I had (i.e. scp from the host to the guest having virtual IOMMU and an assigned network device) was gone with this patch series. Cool!

Please feel free to move this tag if this is not the right place!

Tested-by Jintack Lim <[email protected]>

I'll temporarily consider putting it at the last patch of series if no one jumps out and tells me another more correct way. Also I'll possibly make bold to append your suggested-by too to further claim

your contribution to this problem.

(Hello, Jintack, Feel free to test this branch again against your scp
error case when you got free time)

I rewrote some of the patches in V3. Major changes:

- Dropped mergable interval tree, instead introduced IOVA tree, which is even simpler.

- Fix the scp error issue that Jintack reported. Please see patches for detailed information. That's the major reason to rewrite a few of the patches. We use replay for domain flushes are possibly incorrect in the past. The thing is that IOMMU replay has an "definition" that "we should only send MAP when new page detected", while for shadow page syncing we actually need something else than that. So in this version I started to use a new vtd_sync_shadow_page_table() helper to do the page sync.

- Some other refines after the refactoring.

I'll add unit test for the IOVA tree after this series merged to make sure we won't switch to another new tree implementaion...

The element size in the new IOVA tree should be around sizeof(GTreeNode + IOMMUTLBEntry) ~= (5*8+4*8) = 72 bytes. So the worst case usage ratio would be 72/4K=2%, which still seems acceptable (it means 8G L2 guest will use 8G*2%=160MB as metadata to maintain the mapping in QEMU).

I did explicit test with scp this time, copying 1G sized file for >10 times on each of the following case:

- L1 guest, with vIOMMU and with assigned device
- L2 guest, without vIOMMU and with assigned device
- L2 guest, with vIOMMU (so 3-layer nested IOMMU) and with assigned device

Please review. Thanks,

(Below are old content from previous cover letter)

==========================

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]

Online repo:

https://github.com/xzpeter/qemu/tree/fix-vtd-dma

This series fixes several major problems that current code has:

- Issue 1: when getting very big PSI UNMAP invalidations, the current code is buggy in that we might skip the notification while actually we should always send that notification.

- Issue 2: IOTLB is not thread safe, while block dataplane can be accessing and updating it in parallel.

- Issue 3: For devices that only registered with UNMAP-only notifiers, we don't really need to do page walking for PSIs, we can directly deliver the notification down. For example, vhost.

- Issue 4: unsafe window for MAP notified devices like vfio-pci (and in the future, vDPA as well). The problem is that, now for domain invalidations we do this to make sure the shadow page tables are correctly synced:

1. unmap the whole address space
2. replay the whole address space, map existing pages

However during step 1 and 2 there will be a very tiny window (it can be as big as 3ms) that the shadow page table is either invalid or incomplete (since we're rebuilding it up). That's fatal error since devices never know that happending and it's still possible to DMA to memories.

Patch 1 fixes issue 1. I put it at the first since it's picked from an old post.

Patch 2 is a cleanup to remove useless IntelIOMMUNotifierNode struct.

Patch 3 fixes issue 2.

Patch 4 fixes issue 3.

Patch 5-9 fix issue 4. Here a very simple interval tree is implemented based on Gtree. It's different with general interval tree in that it does not allow user to pass in private data (e.g., translated addresses). However that benefits us that then we can merge adjacent interval leaves so that hopefully we won't consume much memory even if the mappings are a lot (that happens for nested virt - when mapping the whole L2 guest RAM range, it can be at least in GBs).

Patch 10 is another big cleanup only can work after patch 9.

Tests:

- device assignments to L1, even L2 guests. With this series applied (and the kernel IOMMU patches: https://lkml.org/lkml/2018/4/18/5),
we can even nest vIOMMU now, e.g., we can specify vIOMMU in L2 guest with assigned devices and things will work. We can't before.

- vhost smoke test for regression.

Please review. Thanks,

Peter Xu (12):
intel-iommu: send PSI always even if across PDEs
intel-iommu: remove IntelIOMMUNotifierNode
intel-iommu: add iommu lock
intel-iommu: only do page walk for MAP notifiers
intel-iommu: introduce vtd_page_walk_info
intel-iommu: pass in address space when page walk
intel-iommu: trace domain id during page walk
util: implement simple iova tree
intel-iommu: maintain per-device iova ranges
intel-iommu: simplify page walk logic
intel-iommu: new vtd_sync_shadow_page_table_range
intel-iommu: new sync_shadow_page_table

 include/hw/i386/intel_iommu.h |  19 +-
 include/qemu/iova-tree.h      | 134 ++++++++++++
 hw/i386/intel_iommu.c         | 381 +++++++++++++++++++++++++---------
 util/iova-tree.c              | 114 ++++++++++
 MAINTAINERS                   |   6 +
 hw/i386/trace-events          |   5 +-
 util/Makefile.objs            |   1 +
 7 files changed, 556 insertions(+), 104 deletions(-)
 create mode 100644 include/qemu/iova-tree.h
 create mode 100644 util/iova-tree.c

Signature · 1 line

2.17.0

Quoted 52 lines · Peter Xu

(Hello, Jintack, Feel free to test this branch again against your scp
error case when you got free time)

I rewrote some of the patches in V3. Major changes:

- Dropped mergable interval tree, instead introduced IOVA tree, which is even simpler.

- Fix the scp error issue that Jintack reported. Please see patches for detailed information. That's the major reason to rewrite a few of the patches. We use replay for domain flushes are possibly incorrect in the past. The thing is that IOMMU replay has an "definition" that "we should only send MAP when new page detected", while for shadow page syncing we actually need something else than that. So in this version I started to use a new vtd_sync_shadow_page_table() helper to do the page sync.

- Some other refines after the refactoring.

I'll add unit test for the IOVA tree after this series merged to make sure we won't switch to another new tree implementaion...

The element size in the new IOVA tree should be around sizeof(GTreeNode + IOMMUTLBEntry) ~= (5*8+4*8) = 72 bytes. So the worst case usage ratio would be 72/4K=2%, which still seems acceptable (it means 8G L2 guest will use 8G*2%=160MB as metadata to maintain the mapping in QEMU).

I did explicit test with scp this time, copying 1G sized file for >10 times on each of the following case:

- L1 guest, with vIOMMU and with assigned device
- L2 guest, without vIOMMU and with assigned device
- L2 guest, with vIOMMU (so 3-layer nested IOMMU) and with assigned device

Please review. Thanks,

(Below are old content from previous cover letter)

==========================

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]

Online repo:

Quoted 7 lines

https://github.com/xzpeter/qemu/tree/fix-vtd-dma

This series fixes several major problems that current code has:

- Issue 1: when getting very big PSI UNMAP invalidations, the current code is buggy in that we might skip the notification while actually we should always send that notification.

security issue

Quoted 2 lines

- Issue 2: IOTLB is not thread safe, while block dataplane can be accessing and updating it in parallel.

security issue

Quoted 3 lines

- Issue 3: For devices that only registered with UNMAP-only notifiers, we don't really need to do page walking for PSIs, we can directly deliver the notification down. For example, vhost.

optimization

Quoted 5 lines

- Issue 4: unsafe window for MAP notified devices like vfio-pci (and in the future, vDPA as well). The problem is that, now for domain invalidations we do this to make sure the shadow page tables are correctly synced:

Quoted 8 lines

1. unmap the whole address space
2. replay the whole address space, map existing pages

However during step 1 and 2 there will be a very tiny window (it can be as big as 3ms) that the shadow page table is either invalid or incomplete (since we're rebuilding it up). That's fatal error since devices never know that happending and it's still possible to DMA to memories.

correctness but not a security issue

Quoted 17 lines

Patch 1 fixes issue 1. I put it at the first since it's picked from an old post.

Patch 2 is a cleanup to remove useless IntelIOMMUNotifierNode struct.

Patch 3 fixes issue 2.

Patch 4 fixes issue 3.

Patch 5-9 fix issue 4. Here a very simple interval tree is implemented based on Gtree. It's different with general interval tree in that it does not allow user to pass in private data (e.g., translated addresses). However that benefits us that then we can merge adjacent interval leaves so that hopefully we won't consume much memory even if the mappings are a lot (that happens for nested virt - when mapping the whole L2 guest RAM range, it can be at least in GBs).

Quoted 1 line

Patch 10 is another big cleanup only can work after patch 9.

So 1-2 are needed on stable. 1-9 would be nice to have there too, even though they are big and it looks risky.

(Hello, Jintack, Feel free to test this branch again against your scp
error case when you got free time)

I rewrote some of the patches in V3. Major changes:

- Dropped mergable interval tree, instead introduced IOVA tree, which is even simpler.

- Fix the scp error issue that Jintack reported. Please see patches for detailed information. That's the major reason to rewrite a few of the patches. We use replay for domain flushes are possibly incorrect in the past. The thing is that IOMMU replay has an "definition" that "we should only send MAP when new page detected", while for shadow page syncing we actually need something else than that. So in this version I started to use a new vtd_sync_shadow_page_table() helper to do the page sync.

- Some other refines after the refactoring.

I'll add unit test for the IOVA tree after this series merged to make sure we won't switch to another new tree implementaion...

The element size in the new IOVA tree should be around sizeof(GTreeNode + IOMMUTLBEntry) ~= (5*8+4*8) = 72 bytes. So the worst case usage ratio would be 72/4K=2%, which still seems acceptable (it means 8G L2 guest will use 8G*2%=160MB as metadata to maintain the mapping in QEMU).

I did explicit test with scp this time, copying 1G sized file for >10 times on each of the following case:

- L1 guest, with vIOMMU and with assigned device
- L2 guest, without vIOMMU and with assigned device
- L2 guest, with vIOMMU (so 3-layer nested IOMMU) and with assigned device

Please review. Thanks,

(Below are old content from previous cover letter)

==========================

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]

Online repo:

https://github.com/xzpeter/qemu/tree/fix-vtd-dma

This series fixes several major problems that current code has:

- Issue 1: when getting very big PSI UNMAP invalidations, the current code is buggy in that we might skip the notification while actually we should always send that notification.

- Issue 2: IOTLB is not thread safe, while block dataplane can be accessing and updating it in parallel.

- Issue 3: For devices that only registered with UNMAP-only notifiers, we don't really need to do page walking for PSIs, we can directly deliver the notification down. For example, vhost.

- Issue 4: unsafe window for MAP notified devices like vfio-pci (and in the future, vDPA as well). The problem is that, now for domain invalidations we do this to make sure the shadow page tables are correctly synced:

1. unmap the whole address space
2. replay the whole address space, map existing pages

However during step 1 and 2 there will be a very tiny window (it can be as big as 3ms) that the shadow page table is either invalid or incomplete (since we're rebuilding it up). That's fatal error since devices never know that happending and it's still possible to DMA to memories.

Patch 1 fixes issue 1. I put it at the first since it's picked from an old post.

Patch 2 is a cleanup to remove useless IntelIOMMUNotifierNode struct.

Patch 3 fixes issue 2.

Patch 4 fixes issue 3.

Patch 5-9 fix issue 4. Here a very simple interval tree is implemented based on Gtree. It's different with general interval tree in that it does not allow user to pass in private data (e.g., translated addresses). However that benefits us that then we can merge adjacent interval leaves so that hopefully we won't consume much memory even if the mappings are a lot (that happens for nested virt - when mapping the whole L2 guest RAM range, it can be at least in GBs).

Patch 10 is another big cleanup only can work after patch 9.

Tests:

- device assignments to L1, even L2 guests. With this series applied (and the kernel IOMMU patches: https://lkml.org/lkml/2018/4/18/5),
we can even nest vIOMMU now, e.g., we can specify vIOMMU in L2 guest with assigned devices and things will work. We can't before.

- vhost smoke test for regression.

Please review. Thanks,

Peter Xu (12):
intel-iommu: send PSI always even if across PDEs
intel-iommu: remove IntelIOMMUNotifierNode
intel-iommu: add iommu lock
intel-iommu: only do page walk for MAP notifiers
intel-iommu: introduce vtd_page_walk_info
intel-iommu: pass in address space when page walk
intel-iommu: trace domain id during page walk
util: implement simple iova tree
intel-iommu: maintain per-device iova ranges
intel-iommu: simplify page walk logic
intel-iommu: new vtd_sync_shadow_page_table_range
intel-iommu: new sync_shadow_page_table

 include/hw/i386/intel_iommu.h |  19 +-
 include/qemu/iova-tree.h      | 134 ++++++++++++
 hw/i386/intel_iommu.c         | 381 +++++++++++++++++++++++++---------
 util/iova-tree.c              | 114 ++++++++++
 MAINTAINERS                   |   6 +
 hw/i386/trace-events          |   5 +-
 util/Makefile.objs            |   1 +
 7 files changed, 556 insertions(+), 104 deletions(-)
 create mode 100644 include/qemu/iova-tree.h
 create mode 100644 util/iova-tree.c

Signature · 1 line

2.17.0

Quoted 98 lines · Peter Xu

(Hello, Jintack, Feel free to test this branch again against your scp
error case when you got free time)

I rewrote some of the patches in V3. Major changes:

- Dropped mergable interval tree, instead introduced IOVA tree, which is even simpler.

- Fix the scp error issue that Jintack reported. Please see patches for detailed information. That's the major reason to rewrite a few of the patches. We use replay for domain flushes are possibly incorrect in the past. The thing is that IOMMU replay has an "definition" that "we should only send MAP when new page detected", while for shadow page syncing we actually need something else than that. So in this version I started to use a new vtd_sync_shadow_page_table() helper to do the page sync.

- Some other refines after the refactoring.

I'll add unit test for the IOVA tree after this series merged to make sure we won't switch to another new tree implementaion...

The element size in the new IOVA tree should be around sizeof(GTreeNode + IOMMUTLBEntry) ~= (5*8+4*8) = 72 bytes. So the worst case usage ratio would be 72/4K=2%, which still seems acceptable (it means 8G L2 guest will use 8G*2%=160MB as metadata to maintain the mapping in QEMU).

I did explicit test with scp this time, copying 1G sized file for >10 times on each of the following case:

- L1 guest, with vIOMMU and with assigned device
- L2 guest, without vIOMMU and with assigned device
- L2 guest, with vIOMMU (so 3-layer nested IOMMU) and with assigned device

Please review. Thanks,

(Below are old content from previous cover letter)

==========================

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]

Online repo:

https://github.com/xzpeter/qemu/tree/fix-vtd-dma

This series fixes several major problems that current code has:

- Issue 1: when getting very big PSI UNMAP invalidations, the current code is buggy in that we might skip the notification while actually we should always send that notification.

- Issue 2: IOTLB is not thread safe, while block dataplane can be accessing and updating it in parallel.

- Issue 3: For devices that only registered with UNMAP-only notifiers, we don't really need to do page walking for PSIs, we can directly deliver the notification down. For example, vhost.

- Issue 4: unsafe window for MAP notified devices like vfio-pci (and in the future, vDPA as well). The problem is that, now for domain invalidations we do this to make sure the shadow page tables are correctly synced:

1. unmap the whole address space
2. replay the whole address space, map existing pages

However during step 1 and 2 there will be a very tiny window (it can be as big as 3ms) that the shadow page table is either invalid or incomplete (since we're rebuilding it up). That's fatal error since devices never know that happending and it's still possible to DMA to memories.

Patch 1 fixes issue 1. I put it at the first since it's picked from an old post.

Patch 2 is a cleanup to remove useless IntelIOMMUNotifierNode struct.

Patch 3 fixes issue 2.

Patch 4 fixes issue 3.

Patch 5-9 fix issue 4. Here a very simple interval tree is implemented based on Gtree. It's different with general interval tree in that it does not allow user to pass in private data (e.g., translated addresses). However that benefits us that then we can merge adjacent interval leaves so that hopefully we won't consume much memory even if the mappings are a lot (that happens for nested virt - when mapping the whole L2 guest RAM range, it can be at least in GBs).

Quoted 1 line

Patch 10 is another big cleanup only can work after patch 9.

I think patch numbers are wrong somehow.

Given you also want to tweak one comment, could you please repost with this fix, and also in commit log for each patch
- Cc stable
- for security patches mention as much, if possible add data about the issue and its severity