[v3,05/12] intel-iommu: introduce vtd_page_walk_info

Message ID 20180517085927.24925-6-peterx@redhat.com
State New
Headers show
Series
  • intel-iommu: nested vIOMMU, cleanups, bug fixes
Related show

Commit Message

Peter Xu May 17, 2018, 8:59 a.m.
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 <peterx@redhat.com>
---
 hw/i386/intel_iommu.c | 84 ++++++++++++++++++++++++++-----------------
 1 file changed, 52 insertions(+), 32 deletions(-)

Comments

Auger Eric May 18, 2018, 8:23 a.m. | #1
On 05/17/2018 10:59 AM, Peter Xu wrote:
> 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.
s/contants/constants
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/i386/intel_iommu.c | 84 ++++++++++++++++++++++++++-----------------
>  1 file changed, 52 insertions(+), 32 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 9a418abfb6..4953d02ed0 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -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
page table walk?
> + *
> + * @hook_fn: hook func to be called when detected page
nit: when detected page? Not necessarily as it may be called on invalid
entries. on leaf ptw entries?
> + * @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 = {
> +                .hook_fn = vtd_replay_hook,
> +                .private = (void *)n,
> +                .notify_unmap = false,
> +                .aw = s->aw_bits,
> +            };
> +
> +            vtd_page_walk(&ce, 0, ~0ULL, &info);
>          }
>      } else {
>          trace_vtd_replay_ce_invalid(bus_n, PCI_SLOT(vtd_as->devfn),
> 
Besides the nit
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

Patch

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 9a418abfb6..4953d02ed0 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -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;
             }
@@ -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 = {
+                .hook_fn = vtd_replay_hook,
+                .private = (void *)n,
+                .notify_unmap = false,
+                .aw = s->aw_bits,
+            };
+
+            vtd_page_walk(&ce, 0, ~0ULL, &info);
         }
     } else {
         trace_vtd_replay_ce_invalid(bus_n, PCI_SLOT(vtd_as->devfn),