[1/3] AMD/IOMMU: fix off-by-one in amd_iommu_get_paging_mode() callers
diff mbox series

Message ID 0d4b021c-04b4-11e6-aa48-ce0e72d60824@suse.com
State New
Headers show
Series
  • AMD IOMMU: misc small adjustments
Related show

Commit Message

Jan Beulich Feb. 5, 2020, 9:42 a.m. UTC
amd_iommu_get_paging_mode() expects a count, not a "maximum possible"
value. Prior to b4f042236ae0 dropping the reference, the use of our mis-
named "max_page" in amd_iommu_domain_init() may have lead to such a
misunderstanding.

Also replace a literal 4 by an expression tying it to a wider use
constant, just like amd_iommu_quarantine_init() does.

Fixes: ea38867831da ("x86 / iommu: set up a scratch page in the quarantine domain")
Fixes: b4f042236ae0 ("AMD/IOMMU: Cease using a dynamic height for the IOMMU pagetables")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Note: I'm not at the same time adding error checking here, despite
      amd_iommu_get_paging_mode() possibly returning one, as I think
      that's a sufficiently orthogonal aspect.

Comments

Andrew Cooper Feb. 10, 2020, 1:50 p.m. UTC | #1
On 05/02/2020 09:42, Jan Beulich wrote:
> amd_iommu_get_paging_mode() expects a count, not a "maximum possible"
> value. Prior to b4f042236ae0 dropping the reference, the use of our mis-
> named "max_page" in amd_iommu_domain_init() may have lead to such a
> misunderstanding.
>
> Also replace a literal 4 by an expression tying it to a wider use
> constant, just like amd_iommu_quarantine_init() does.
>
> Fixes: ea38867831da ("x86 / iommu: set up a scratch page in the quarantine domain")
> Fixes: b4f042236ae0 ("AMD/IOMMU: Cease using a dynamic height for the IOMMU pagetables")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Note: I'm not at the same time adding error checking here, despite
>       amd_iommu_get_paging_mode() possibly returning one, as I think
>       that's a sufficiently orthogonal aspect.

It is entirely non-obvious what amd_iommu_get_paging_mode() takes, which
is presumably what has led to this confusion.

It also seems silly to force a call into another translation unit when
2/3 of the callers can be evaluated at compile time.

How about re-implementing amd_iommu_get_paging_mode() as a static inline
(seeing as it is just basic arithmetic), and naming its parameter in a
more useful, e.g. max_frames ?

~Andrew

Patch
diff mbox series

--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -455,9 +455,9 @@  int amd_iommu_reserve_domain_unity_map(s
 int __init amd_iommu_quarantine_init(struct domain *d)
 {
     struct domain_iommu *hd = dom_iommu(d);
-    unsigned long max_gfn =
-        PFN_DOWN((1ul << DEFAULT_DOMAIN_ADDRESS_WIDTH) - 1);
-    unsigned int level = amd_iommu_get_paging_mode(max_gfn);
+    unsigned long end_gfn =
+        1ul << (DEFAULT_DOMAIN_ADDRESS_WIDTH - PAGE_SHIFT);
+    unsigned int level = amd_iommu_get_paging_mode(end_gfn);
     struct amd_iommu_pte *table;
 
     if ( hd->arch.root_table )
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -259,8 +259,10 @@  static int amd_iommu_domain_init(struct
      *   physical address space we give it, but this isn't known yet so use 4
      *   unilaterally.
      */
-    hd->arch.paging_mode = is_hvm_domain(d)
-        ? 4 : amd_iommu_get_paging_mode(get_upper_mfn_bound());
+    hd->arch.paging_mode = amd_iommu_get_paging_mode(
+        is_hvm_domain(d)
+        ? 1ul << (DEFAULT_DOMAIN_ADDRESS_WIDTH - PAGE_SHIFT)
+        : get_upper_mfn_bound() + 1);
 
     return 0;
 }