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

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

[Jan Beulich]

(re-sending upon Ian's request with his address adjusted, including
Kevin's R-b at this occasion)

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.

As to patch 1: 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 RESEND 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>
Reviewed-by: Kevin Tian <kevin.tian@intel.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 RESEND 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>
Reviewed-by: Kevin Tian <kevin.tian@intel.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 RESEND 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>
Reviewed-by: Kevin Tian <kevin.tian@intel.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 RESEND 1/3][4.16] VT-d: per-domain IOMMU bitmap needs to have dynamic size

[Ian Jackson]

Jan Beulich writes ("[PATCH RESEND 1/3][4.16] VT-d: per-domain IOMMU bitmap needs to have dynamic size"):
> 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>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

provided it can be committed today.

Thanks,
Ian.

Re: [PATCH RESEND 0/3][4.16?] VT-d: misc (regression) fixes [and 2 more messages]

[Ian Jackson]

Jan Beulich writes ("[PATCH RESEND 0/3][4.16?] VT-d: misc (regression) fixes"):
> (re-sending upon Ian's request with his address adjusted, including
> Kevin's R-b at this occasion)
> 
> 1: per-domain IOMMU bitmap needs to have dynamic size
...
> As to patch 1: 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.

Right.  I have given my R-A provided we can commit it today.

Obviously potentail corruption, even on machines we don't know exist,
is an RC bug.  But if this patch can't go in very soon (or turns out
to be troublesome) I think we could downgrade this bug from
by detecting systems with many IOMMUs and refusing to boot ?
That's not great but if we don't know of actual affected systems, it
might be better than risking introducing bugs for everyone else.

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

I don't understand the impact of patch 2 at all.

I doubt an inefficiency would warrant a release ack at this stage,
unless the benefit of the patch is very substantial.

Thanks,
Ian.

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

[Jan Beulich]

On 12.11.2021 13:43, Ian Jackson wrote:
> Jan Beulich writes ("[PATCH RESEND 1/3][4.16] VT-d: per-domain IOMMU bitmap needs to have dynamic size"):
>> 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>
>> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> 
> Release-Acked-by: Ian Jackson <iwj@xenproject.org>
> 
> provided it can be committed today.

Committed. I'm not aware of anything else I would have needed to wait for.

Jan