[PATCH 0/3][4.16?] VT-d: misc (regression) fixes

[PATCH 0/3][4.16?] VT-d: misc (regression) fixes

[Jan Beulich]

1: per-domain IOMMU bitmap needs to have dynamic size
2: fix reduced page table levels support when sharing tables
3: don't needlessly engage the untrusted-MSI workaround

As to 4.16 considerations: Only patch 1 addresses a regression
introduced after 4.15, the others are older. Patch 3 additionally
only addresses an inefficiency; the code we have is correct from
a functional pov.

Jan

[PATCH 1/3][4.16?] VT-d: per-domain IOMMU bitmap needs to have dynamic size

[Jan Beulich]

With no upper bound (anymore) on the number of IOMMUs, a fixed-size
64-bit map may be insufficient (systems with 40 IOMMUs have already been
observed).

Fixes: 27713fa2aa21 ("VT-d: improve save/restore of registers across S3")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -54,7 +54,7 @@ bool __read_mostly iommu_qinval = true;
 bool __read_mostly iommu_snoop = true;
 #endif
 
-static unsigned int __initdata nr_iommus;
+static unsigned int __read_mostly nr_iommus;
 
 static struct iommu_ops vtd_ops;
 static struct tasklet vtd_fault_tasklet;
@@ -645,7 +645,7 @@ static int __must_check iommu_flush_iotl
 
         iommu = drhd->iommu;
 
-        if ( !test_bit(iommu->index, &hd->arch.vtd.iommu_bitmap) )
+        if ( !test_bit(iommu->index, hd->arch.vtd.iommu_bitmap) )
             continue;
 
         flush_dev_iotlb = !!find_ats_dev_drhd(iommu);
@@ -1308,6 +1308,11 @@ static int intel_iommu_domain_init(struc
 {
     struct domain_iommu *hd = dom_iommu(d);
 
+    hd->arch.vtd.iommu_bitmap = xzalloc_array(unsigned long,
+                                              BITS_TO_LONGS(nr_iommus));
+    if ( !hd->arch.vtd.iommu_bitmap )
+        return -ENOMEM;
+
     hd->arch.vtd.agaw = width_to_agaw(DEFAULT_DOMAIN_ADDRESS_WIDTH);
 
     return 0;
@@ -1457,7 +1462,7 @@ int domain_context_mapping_one(
     if ( rc > 0 )
         rc = 0;
 
-    set_bit(iommu->index, &hd->arch.vtd.iommu_bitmap);
+    set_bit(iommu->index, hd->arch.vtd.iommu_bitmap);
 
     unmap_vtd_domain_page(context_entries);
 
@@ -1789,7 +1794,7 @@ static int domain_context_unmap(struct d
 
     if ( !found )
     {
-        clear_bit(iommu->index, &dom_iommu(domain)->arch.vtd.iommu_bitmap);
+        clear_bit(iommu->index, dom_iommu(domain)->arch.vtd.iommu_bitmap);
         cleanup_domid_map(domain, iommu);
     }
 
@@ -1819,6 +1824,8 @@ static void iommu_domain_teardown(struct
 
     for_each_drhd_unit ( drhd )
         cleanup_domid_map(d, drhd->iommu);
+
+    XFREE(hd->arch.vtd.iommu_bitmap);
 }
 
 static int __must_check intel_iommu_map_page(struct domain *d, dfn_t dfn,
--- a/xen/include/asm-x86/iommu.h
+++ b/xen/include/asm-x86/iommu.h
@@ -58,7 +58,7 @@ struct arch_iommu
         struct {
             uint64_t pgd_maddr; /* io page directory machine address */
             unsigned int agaw; /* adjusted guest address width, 0 is level 2 30-bit */
-            uint64_t iommu_bitmap; /* bitmap of iommu(s) that the domain uses */
+            unsigned long *iommu_bitmap; /* bitmap of iommu(s) that the domain uses */
         } vtd;
         /* AMD IOMMU */
         struct {

[PATCH 2/3][4.16?] VT-d: fix reduced page table levels support when sharing tables

[Jan Beulich]

domain_pgd_maddr() contains logic to adjust the root address to be put
in the context entry in case 4-level page tables aren't supported by an
IOMMU. This logic may not be bypassed when sharing page tables.

Fixes: 25ccd093425c ("iommu: remove the share_p2m operation")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -340,19 +340,21 @@ static uint64_t domain_pgd_maddr(struct
     {
         pagetable_t pgt = p2m_get_pagetable(p2m_get_hostp2m(d));
 
-        return pagetable_get_paddr(pgt);
+        pgd_maddr = pagetable_get_paddr(pgt);
     }
-
-    if ( !hd->arch.vtd.pgd_maddr )
+    else
     {
-        /* Ensure we have pagetables allocated down to leaf PTE. */
-        addr_to_dma_page_maddr(d, 0, 1);
-
         if ( !hd->arch.vtd.pgd_maddr )
-            return 0;
-    }
+        {
+            /* Ensure we have pagetables allocated down to leaf PTE. */
+            addr_to_dma_page_maddr(d, 0, 1);
 
-    pgd_maddr = hd->arch.vtd.pgd_maddr;
+            if ( !hd->arch.vtd.pgd_maddr )
+                return 0;
+        }
+
+        pgd_maddr = hd->arch.vtd.pgd_maddr;
+    }
 
     /* Skip top levels of page tables for 2- and 3-level DRHDs. */
     for ( agaw = level_to_agaw(4);

[PATCH 3/3][4.16?] VT-d: don't needlessly engage the untrusted-MSI workaround

[Jan Beulich]

The quarantine domain doesn't count as a DomU, as it won't itself
trigger any bad behavior. The workaround only needs enabling when an
actual DomU is about to gain control of a device. This then also means
enabling of the workaround can be deferred until immediately ahead of
the call to domain_context_mapping(). While there also stop open-coding
is_hardware_domain().

Fixes: 319f9a0ba94c ("passthrough: quarantine PCI devices")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2404,14 +2404,6 @@ static int reassign_device_ownership(
     int ret;
 
     /*
-     * Devices assigned to untrusted domains (here assumed to be any domU)
-     * can attempt to send arbitrary LAPIC/MSI messages. We are unprotected
-     * by the root complex unless interrupt remapping is enabled.
-     */
-    if ( (target != hardware_domain) && !iommu_intremap )
-        untrusted_msi = true;
-
-    /*
      * If the device belongs to the hardware domain, and it has RMRR, don't
      * remove it from the hardware domain, because BIOS may use RMRR at
      * booting time.
@@ -2455,6 +2447,15 @@ static int reassign_device_ownership(
     if ( !has_arch_pdevs(target) )
         vmx_pi_hooks_assign(target);
 
+    /*
+     * Devices assigned to untrusted domains (here assumed to be any domU)
+     * can attempt to send arbitrary LAPIC/MSI messages. We are unprotected
+     * by the root complex unless interrupt remapping is enabled.
+     */
+    if ( !iommu_intremap && !is_hardware_domain(target) &&
+         !is_system_domain(target) )
+        untrusted_msi = true;
+
     ret = domain_context_mapping(target, devfn, pdev);
     if ( ret )
     {

RE: [PATCH 0/3][4.16?] VT-d: misc (regression) fixes

[Tian, Kevin]

> From: Jan Beulich <jbeulich@suse.com>
> Sent: Tuesday, November 9, 2021 10:56 PM
> 
> 1: per-domain IOMMU bitmap needs to have dynamic size
> 2: fix reduced page table levels support when sharing tables
> 3: don't needlessly engage the untrusted-MSI workaround
> 
> As to 4.16 considerations: Only patch 1 addresses a regression
> introduced after 4.15, the others are older. Patch 3 additionally
> only addresses an inefficiency; the code we have is correct from
> a functional pov.
> 

All look good to me:

	Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Re: [PATCH 0/3][4.16?] VT-d: misc (regression) fixes

[Jan Beulich]

On 09.11.2021 15:55, Jan Beulich wrote:
> 1: per-domain IOMMU bitmap needs to have dynamic size
> 2: fix reduced page table levels support when sharing tables
> 3: don't needlessly engage the untrusted-MSI workaround
> 
> As to 4.16 considerations: Only patch 1 addresses a regression
> introduced after 4.15, the others are older.

To further explain this: Without the earlier change, systems with more
than 32 IOMMUs simply would fail to enable use of the IOMMUs altogether.
Now systems with more than 64 IOMMUs would observe memory corruption
(with unclear knock-on effects). Whether systems with this many IOMMUs
actually exist I can't tell; I know of ones with 40, which isn't all
that far away from 64.

Jan

> Patch 3 additionally
> only addresses an inefficiency; the code we have is correct from
> a functional pov.
> 
> Jan