intel-iommu: send PSI always when notify_unmap set

PATCH In review Peter Xu 3 participants 8 messages Apr 18, 2018 – Apr 20, 2018

lore

Share & tools

lore

Peter Xu

author

2018-04-18 04:51 UTC #0 lore raw

PATCH 1/1 intel-iommu: send PSI always when notify_unmap set

During IOVA page table walk, there is a special case when:

- notify_unmap is set, meanwhile
- entry is invalid

In the past, we skip the entry always. This is not correct. We should send UNMAP notification to registered notifiers in this case. Otherwise some stall pages will still be mapped in the host even if L1 guest unmapped them already.

Without this patch, nested device assignment to L2 guests might 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)

To fix this, we need to apply this patch to L1 QEMU (L2 QEMU is not affected by this problem).

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

---

To test nested assignment, one also needs to apply below patchset:
https://lkml.org/lkml/2018/4/18/5

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

Peter Xu

author

2018-04-18 05:28 UTC #1 lore raw

Quoted 13 lines · Peter Xu

During IOVA page table walk, there is a special case when:
- notify_unmap is set, meanwhile
- entry is invalid
In the past, we skip the entry always. This is not correct. We should send UNMAP notification to registered notifiers in this case. Otherwise some stall pages will still be mapped in the host even if L1 guest unmapped them already.
Without this patch, nested device assignment to L2 guests might dump some errors like:

Quoted 8 lines

qemu-system-x86_64: VFIO_MAP_DMA: -17
qemu-system-x86_64: vfio_dma_map(0x557305420c30, 0xad000, 0x1000,
                    0x7f89a920d000) = -17 (File exists)
To fix this, we need to apply this patch to L1 QEMU (L2 QEMU is not affected by this problem).
Signed-off-by Peter Xu <[email protected]>

This should really be 2.12 material, it fixes a real bug, but not sure whether it's too late already. Michael, what do you think?

Thanks,

Signature · 1 line

Peter Xu

Liu, Yi L

2018-04-18 05:30 UTC #2 lore raw

In reply to Peter Xu

Quoted 8 lines

Sent: Wednesday, April 18, 2018 12:51 PM
Subject: [Qemu-devel] [PATCH] intel-iommu: send PSI always when notify_unmap set
During IOVA page table walk, there is a special case when:
- notify_unmap is set, meanwhile
- entry is invalid

This is very brief description, would you mind talk a little bit more.

Quoted 6 lines

In the past, we skip the entry always. This is not correct. We should send UNMAP notification to registered notifiers in this case. Otherwise some stall pages will still be mapped in the host even if L1 guest unmapped them already.
Without this patch, nested device assignment to L2 guests might dump some errors like:

Should it be physical device assigned from L0 host? Or emulated devices could also trigger this problem?

Quoted 6 lines

qemu-system-x86_64: VFIO_MAP_DMA: -17
qemu-system-x86_64: vfio_dma_map(0x557305420c30, 0xad000, 0x1000,
                    0x7f89a920d000) = -17 (File exists)
To fix this, we need to apply this patch to L1 QEMU (L2 QEMU is not affected by this problem).

Does this fix also apply to L0 QEMU?

Quoted 80 lines

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

---

To test nested assignment, one also needs to apply below patchset:
https://lkml.org/lkml/2018/4/18/5

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

hw/i386/intel_iommu.c index renamed from hw/i386/intel_iommu.c +0 -0

fb31de9416..b359efd6f9 100644

hw/i386/intel_iommu.c +9 -0

@@ -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

hw/i386/intel_iommu.c +21 ~~-12~~

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

Quoted 4 lines

ret = vtd_page_walk_level(vtd_get_slpte_addr(slpte, aw), iova,
--
2.14.3

Thanks,
Yi Liu

Peter Xu

author

2018-04-18 06:26 UTC #3 lore raw

Quoted 10 lines · Yi L

Quoted 8 lines
Sent: Wednesday, April 18, 2018 12:51 PM
Subject: [Qemu-devel] [PATCH] intel-iommu: send PSI always when notify_unmap set
During IOVA page table walk, there is a special case when:
- notify_unmap is set, meanwhile
- entry is invalid
This is very brief description, would you mind talk a little bit more.

It means the case when the program reaches [1] below.

Quoted 10 lines

Quoted 7 lines
In the past, we skip the entry always. This is not correct. We should send UNMAP notification to registered notifiers in this case. Otherwise some stall pages will still be mapped in the host even if L1 guest unmapped them already.
Without this patch, nested device assignment to L2 guests might dump some errors like:
Should it be physical device assigned from L0 host? Or emulated devices could also trigger this problem?

If using emulated devices, we possibly need three levels, so I think we can also see this warning if you assign a emulated device from L1 guest to L2 then to L3, and you should be able to see this warning dumped from the QEMU that runs L2.

Quoted 9 lines

Quoted 7 lines
qemu-system-x86_64: VFIO_MAP_DMA: -17
qemu-system-x86_64: vfio_dma_map(0x557305420c30, 0xad000, 0x1000,
                    0x7f89a920d000) = -17 (File exists)
To fix this, we need to apply this patch to L1 QEMU (L2 QEMU is not affected by this problem).
Does this fix also apply to L0 QEMU?

Sorry I wasn't clear. When I say L1 QEMU I did mean the QEMU that runs as L1. I believe it means your "L0 QEMU" here.

And yes, this fix should also be valid even if without nesting, however we can hardly trigger this (that's why I just found it recently when people reported nested breakage, since it is hardly seen without nested), but AFAIU it's still possible.

Quoted 57 lines

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

---

To test nested assignment, one also needs to apply below patchset:
https://lkml.org/lkml/2018/4/18/5

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

hw/i386/intel_iommu.c index renamed from hw/i386/intel_iommu.c +0 -0

fb31de9416..b359efd6f9 100644

hw/i386/intel_iommu.c +9 -0

@@ -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

hw/i386/intel_iommu.c +5 -9

@@ -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) {

Quoted 8 lines

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

Quoted 8 lines

hw/i386/intel_iommu.c +3 -2

-                    return ret;
-                }
+            ret = vtd_page_walk_one(&entry, level, hook_fn, private);
+            if (ret < 0) {
+                return ret;
             }
         } else {
             if (!entry_valid) {

[1]

Quoted 23 lines

Quoted 16 lines

hw/i386/intel_iommu.c +13 -1

-                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;
             }

Quoted 4 lines

ret = vtd_page_walk_level(vtd_get_slpte_addr(slpte, aw), iova,
--
2.14.3

Thanks,
Yi Liu

Signature · 1 line

Peter Xu

Jason Wang

2018-04-20 04:57 UTC #4 lore raw

In reply to Peter Xu

Quoted 8 lines · Peter Xu

During IOVA page table walk, there is a special case when:
- notify_unmap is set, meanwhile
- entry is invalid
In the past, we skip the entry always. This is not correct. We should send UNMAP notification to registered notifiers in this case. Otherwise some stall pages will still be mapped in the host even if L1 guest

You mean 'stale' here?

Quoted 1 line

unmapped them already.

It looks like some IOTLB invalidation were missed? If not, the page should be unmaped during invalidation.

Quoted 8 lines

Without this patch, nested device assignment to L2 guests might 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)

Quoted 8 lines

To fix this, we need to apply this patch to L1 QEMU (L2 QEMU is not affected by this problem).
Signed-off-by Peter Xu <[email protected]>
---
To test nested assignment, one also needs to apply below patchset:
https://lkml.org/lkml/2018/4/18/5

Have a quick glance at it, looks like there's no need for this patch is we had that kernel patch applied.

Thanks

Quoted 74 lines

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

Peter Xu

author

2018-04-20 05:11 UTC #5 lore raw

Quoted 5 lines · Jason Wang

Quoted 2 lines · Peter Xu
During IOVA page table walk, there is a special case when:

Quoted 8 lines

Quoted 6 lines
- notify_unmap is set, meanwhile
- entry is invalid
In the past, we skip the entry always. This is not correct. We should send UNMAP notification to registered notifiers in this case. Otherwise some stall pages will still be mapped in the host even if L1 guest
You mean 'stale' here?

Ah, yes.

Quoted 5 lines

Quoted 2 lines
unmapped them already.
It looks like some IOTLB invalidation were missed? If not, the page should be unmaped during invalidation.

I'm afraid it's not the same problem, and that's why I think we need two fixes.

Even if the guest sends the correct invalidations, it's still possible that QEMU skips the PSI with current code.

Quoted 12 lines

Quoted 12 lines
Without this patch, nested device assignment to L2 guests might 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)
To fix this, we need to apply this patch to L1 QEMU (L2 QEMU is not affected by this problem).

Quoted 8 lines

Quoted 5 lines
Signed-off-by Peter Xu <[email protected]>
---
To test nested assignment, one also needs to apply below patchset:
https://lkml.org/lkml/2018/4/18/5
Have a quick glance at it, looks like there's no need for this patch is we had that kernel patch applied.

Even if guest kernel applies above fix, QEMU might still drop some of the PSIs. Above error messages were captured with kernel patches applied already (otherwise we see no error messages, we directly miss them from guest).

Thanks,

Signature · 1 line

Peter Xu

Jason Wang

2018-04-20 08:29 UTC #6 lore raw

Quoted 37 lines · Peter Xu

Quoted 10 lines · Jason Wang
Quoted 8 lines · Peter Xu
During IOVA page table walk, there is a special case when:
- notify_unmap is set, meanwhile
- entry is invalid
In the past, we skip the entry always. This is not correct. We should send UNMAP notification to registered notifiers in this case. Otherwise some stall pages will still be mapped in the host even if L1 guest
You mean 'stale' here?
Ah, yes.
Quoted 3 lines
Quoted 1 line
unmapped them already.
It looks like some IOTLB invalidation were missed? If not, the page should be unmaped during invalidation.
I'm afraid it's not the same problem, and that's why I think we need two fixes.
Even if the guest sends the correct invalidations, it's still possible that QEMU skips the PSI with current code.
Quoted 15 lines
Quoted 15 lines
Without this patch, nested device assignment to L2 guests might 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)
To fix this, we need to apply this patch to L1 QEMU (L2 QEMU is not affected by this problem).
Signed-off-by Peter Xu<[email protected]>
---
To test nested assignment, one also needs to apply below patchset:
https://lkml.org/lkml/2018/4/18/5

Quoted 8 lines

Quoted 2 lines
Have a quick glance at it, looks like there's no need for this patch is we had that kernel patch applied.
Even if guest kernel applies above fix, QEMU might still drop some of the PSIs. Above error messages were captured with kernel patches applied already (otherwise we see no error messages, we directly miss them from guest).
Thanks,

I see so this actually try to trigger unmap notifier for non last level slpte.

Thanks

Jason Wang

Acked-by on v1 1/1

2018-04-20 08:31 UTC #7 lore raw

In reply to Peter Xu

Quoted 89 lines · Peter Xu

During IOVA page table walk, there is a special case when:

- notify_unmap is set, meanwhile
- entry is invalid

Without this patch, nested device assignment to L2 guests might 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)

To fix this, we need to apply this patch to L1 QEMU (L2 QEMU is not affected by this problem).

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

---

To test nested assignment, one also needs to apply below patchset:
https://lkml.org/lkml/2018/4/18/5

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

hw/i386/intel_iommu.c +23 ~~-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;

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

Quoted 10 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,

Acked-by Jason Wang <[email protected]>