[XEN PATCH v5 0/3] x86/iommu: Drop IOMMU support when cx16 isn't supported

[XEN PATCH v5 0/3] x86/iommu: Drop IOMMU support when cx16 isn't supported

[Teddy Astie]

All hardware that supports VT-d/AMD-Vi that exists also supports cx16 (aside
specifically crafted virtual machines).

Some IOMMU code paths in Xen consider cases where VT-d/AMD-Vi is supported
while cx16 isn't, those paths may be bugged and are barely tested, dead code
in practice.

Disable IOMMU in case we have IOMMU hardware but no cx16, then cleanup
no-cx16 handling logic from VT-d and AMD-Vi drivers. Also disable
interrupt remapping that also relies on cx16.

Teddy

---

Changed in v2:

  * Added cleanup no-cx16 code for x2APIC
  * Fixed commit and code formatting
  * Added missing Suggested-by note

Changed in v3:

  * Use -ENODEV instead of -ENOSYS.

Changed in v4:

  * Reworded "Disable IOMMU if cx16 isn't supported"
  * Moved interrupt remapping cleanup in separate patches
  * Check cx16 for interrupt remapping in driver's callbacks rather than 
in x2apic_bsp_setup

Changed in v5:

 * Folded VT-d/AMD-Vi cx16 cleanup patches

Teddy Astie (3):
  x86/iommu: Disable IOMMU if cx16 isn't supported
  VT-d: Cleanup MAP_SINGLE_DEVICE and related code
  x86/iommu: Disable intrerrupt remapping if cx16 is not supported

 xen/drivers/passthrough/amd/iommu_intr.c    |  6 ++
 xen/drivers/passthrough/amd/iommu_map.c     | 42 ++++------
 xen/drivers/passthrough/amd/pci_amd_iommu.c |  6 ++
 xen/drivers/passthrough/vtd/intremap.c      | 70 +++++-----------
 xen/drivers/passthrough/vtd/iommu.c         | 92 +++++++--------------
 xen/drivers/passthrough/vtd/vtd.h           |  5 +-
 6 files changed, 77 insertions(+), 144 deletions(-)

-- 
2.44.0



Teddy Astie | Vates XCP-ng Intern

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

[XEN PATCH v5 1/3] x86/iommu: Disable IOMMU if cx16 isn't supported

[Teddy Astie]

All hardware with VT-d/AMD-Vi has CMPXCHG16B support. Check this at
initialisation time, and remove the effectively-dead logic for the
non-cx16 case.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
---
 xen/drivers/passthrough/amd/iommu_map.c     | 42 ++++--------
 xen/drivers/passthrough/amd/pci_amd_iommu.c |  6 ++
 xen/drivers/passthrough/vtd/iommu.c         | 73 +++++++--------------
 3 files changed, 45 insertions(+), 76 deletions(-)

diff --git a/xen/drivers/passthrough/amd/iommu_map.c b/xen/drivers/passthrough/amd/iommu_map.c
index e0f4fe736a..f67975e700 100644
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -167,15 +167,14 @@ int amd_iommu_set_root_page_table(struct amd_iommu_dte *dte,
 {
     bool valid = flags & SET_ROOT_VALID;
 
-    if ( dte->v && dte->tv &&
-         (cpu_has_cx16 || (flags & SET_ROOT_WITH_UNITY_MAP)) )
+    if ( dte->v && dte->tv )
     {
         union {
             struct amd_iommu_dte dte;
             uint64_t raw64[4];
             __uint128_t raw128[2];
         } ldte = { .dte = *dte };
-        __uint128_t old = ldte.raw128[0];
+        __uint128_t res, old = ldte.raw128[0];
         int ret = 0;
 
         ldte.dte.domain_id = domain_id;
@@ -185,33 +184,20 @@ int amd_iommu_set_root_page_table(struct amd_iommu_dte *dte,
         ldte.dte.paging_mode = paging_mode;
         ldte.dte.v = valid;
 
-        if ( cpu_has_cx16 )
-        {
-            __uint128_t res = cmpxchg16b(dte, &old, &ldte.raw128[0]);
+        res = cmpxchg16b(dte, &old, &ldte.raw128[0]);
 
-            /*
-             * Hardware does not update the DTE behind our backs, so the
-             * return value should match "old".
-             */
-            if ( res != old )
-            {
-                printk(XENLOG_ERR
-                       "Dom%d: unexpected DTE %016lx_%016lx (expected %016lx_%016lx)\n",
-                       domain_id,
-                       (uint64_t)(res >> 64), (uint64_t)res,
-                       (uint64_t)(old >> 64), (uint64_t)old);
-                ret = -EILSEQ;
-            }
-        }
-        else /* Best effort, updating domain_id last. */
+        /*
+         * Hardware does not update the DTE behind our backs, so the
+         * return value should match "old".
+         */
+        if ( res != old )
         {
-            uint64_t *ptr = (void *)dte;
-
-            write_atomic(ptr + 0, ldte.raw64[0]);
-            /* No barrier should be needed between these two. */
-            write_atomic(ptr + 1, ldte.raw64[1]);
-
-            ret = 1;
+            printk(XENLOG_ERR
+                   "Dom%d: unexpected DTE %016lx_%016lx (expected %016lx_%016lx)\n",
+                   domain_id,
+                   (uint64_t)(res >> 64), (uint64_t)res,
+                   (uint64_t)(old >> 64), (uint64_t)old);
+            ret = -EILSEQ;
         }
 
         return ret;
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index f6efd88e36..3a6a23f706 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -305,6 +305,12 @@ static int __init cf_check iov_detect(void)
     if ( !iommu_enable && !iommu_intremap )
         return 0;
 
+    if ( unlikely(!cpu_has_cx16) )
+    {
+        printk("AMD-Vi: CPU doesn't support CMPXCHG16B, disabling\n");
+        return -ENODEV;
+    }
+
     if ( (init_done ? amd_iommu_init_late()
                     : amd_iommu_init(false)) != 0 )
     {
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index c7110af7c9..9c787ba9eb 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1482,7 +1482,7 @@ int domain_context_mapping_one(
 {
     struct domain_iommu *hd = dom_iommu(domain);
     struct context_entry *context, *context_entries, lctxt;
-    __uint128_t old;
+    __uint128_t res, old;
     uint64_t maddr;
     uint16_t seg = iommu->drhd->segment, prev_did = 0;
     struct domain *prev_dom = NULL;
@@ -1580,55 +1580,23 @@ int domain_context_mapping_one(
         ASSERT(!context_fault_disable(lctxt));
     }
 
-    if ( cpu_has_cx16 )
-    {
-        __uint128_t res = cmpxchg16b(context, &old, &lctxt.full);
-
-        /*
-         * Hardware does not update the context entry behind our backs,
-         * so the return value should match "old".
-         */
-        if ( res != old )
-        {
-            if ( pdev )
-                check_cleanup_domid_map(domain, pdev, iommu);
-            printk(XENLOG_ERR
-                   "%pp: unexpected context entry %016lx_%016lx (expected %016lx_%016lx)\n",
-                   &PCI_SBDF(seg, bus, devfn),
-                   (uint64_t)(res >> 64), (uint64_t)res,
-                   (uint64_t)(old >> 64), (uint64_t)old);
-            rc = -EILSEQ;
-            goto unlock;
-        }
-    }
-    else if ( !prev_dom || !(mode & MAP_WITH_RMRR) )
-    {
-        context_clear_present(*context);
-        iommu_sync_cache(context, sizeof(*context));
+    res = cmpxchg16b(context, &old, &lctxt.full);
 
-        write_atomic(&context->hi, lctxt.hi);
-        /* No barrier should be needed between these two. */
-        write_atomic(&context->lo, lctxt.lo);
-    }
-    else /* Best effort, updating DID last. */
+    /*
+     * Hardware does not update the context entry behind our backs,
+     * so the return value should match "old".
+     */
+    if ( res != old )
     {
-         /*
-          * By non-atomically updating the context entry's DID field last,
-          * during a short window in time TLB entries with the old domain ID
-          * but the new page tables may be inserted.  This could affect I/O
-          * of other devices using this same (old) domain ID.  Such updating
-          * therefore is not a problem if this was the only device associated
-          * with the old domain ID.  Diverting I/O of any of a dying domain's
-          * devices to the quarantine page tables is intended anyway.
-          */
-        if ( !(mode & (MAP_OWNER_DYING | MAP_SINGLE_DEVICE)) )
-            printk(XENLOG_WARNING VTDPREFIX
-                   " %pp: reassignment may cause %pd data corruption\n",
-                   &PCI_SBDF(seg, bus, devfn), prev_dom);
-
-        write_atomic(&context->lo, lctxt.lo);
-        /* No barrier should be needed between these two. */
-        write_atomic(&context->hi, lctxt.hi);
+        if ( pdev )
+            check_cleanup_domid_map(domain, pdev, iommu);
+        printk(XENLOG_ERR
+                "%pp: unexpected context entry %016lx_%016lx (expected %016lx_%016lx)\n",
+                &PCI_SBDF(seg, bus, devfn),
+                (uint64_t)(res >> 64), (uint64_t)res,
+                (uint64_t)(old >> 64), (uint64_t)old);
+        rc = -EILSEQ;
+        goto unlock;
     }
 
     iommu_sync_cache(context, sizeof(struct context_entry));
@@ -2630,6 +2598,15 @@ static int __init cf_check vtd_setup(void)
     int ret;
     bool reg_inval_supported = true;
 
+    if ( unlikely(!cpu_has_cx16) )
+    {
+        printk(XENLOG_ERR VTDPREFIX
+               "IOMMU: CPU doesn't support CMPXCHG16B, disabling\n");
+
+        ret = -ENODEV;
+        goto error;
+    }
+
     if ( list_empty(&acpi_drhd_units) )
     {
         ret = -ENODEV;
-- 
2.44.0



Teddy Astie | Vates XCP-ng Intern

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

[XEN PATCH v5 2/3] VT-d: Cleanup MAP_SINGLE_DEVICE and related code

[Teddy Astie]

This flag was only used in case cx16 is not available, as those code paths no
longer exist, this flag now does basically nothing.

Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
---
 xen/drivers/passthrough/vtd/iommu.c | 12 +++---------
 xen/drivers/passthrough/vtd/vtd.h   |  5 ++---
 2 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 9c787ba9eb..7c6bae0256 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1692,15 +1692,9 @@ static int domain_context_mapping(struct domain *domain, u8 devfn,
         break;
     }
 
-    if ( domain != pdev->domain && pdev->domain != dom_io )
-    {
-        if ( pdev->domain->is_dying )
-            mode |= MAP_OWNER_DYING;
-        else if ( drhd &&
-                  !any_pdev_behind_iommu(pdev->domain, pdev, drhd->iommu) &&
-                  !pdev->phantom_stride )
-            mode |= MAP_SINGLE_DEVICE;
-    }
+    if ( domain != pdev->domain && pdev->domain != dom_io &&
+         pdev->domain->is_dying )
+        mode |= MAP_OWNER_DYING;
 
     switch ( pdev->type )
     {
diff --git a/xen/drivers/passthrough/vtd/vtd.h b/xen/drivers/passthrough/vtd/vtd.h
index cb2df76eed..43f06a353d 100644
--- a/xen/drivers/passthrough/vtd/vtd.h
+++ b/xen/drivers/passthrough/vtd/vtd.h
@@ -28,9 +28,8 @@
  */
 #define MAP_WITH_RMRR         (1u << 0)
 #define MAP_OWNER_DYING       (1u << 1)
-#define MAP_SINGLE_DEVICE     (1u << 2)
-#define MAP_ERROR_RECOVERY    (1u << 3)
-#define UNMAP_ME_PHANTOM_FUNC (1u << 4)
+#define MAP_ERROR_RECOVERY    (1u << 2)
+#define UNMAP_ME_PHANTOM_FUNC (1u << 3)
 
 /* Allow for both IOAPIC and IOSAPIC. */
 #define IO_xAPIC_route_entry IO_APIC_route_entry
-- 
2.44.0



Teddy Astie | Vates XCP-ng Intern

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

[XEN PATCH v5 3/3] x86/iommu: Disable intrerrupt remapping if cx16 is not supported

[Teddy Astie]

All hardware with VT-d/AMD-Vi has CMPXCHG16B support.  Check this at
initialisation time, and remove the effectively-dead logic for the non-cx16
case.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
---
 xen/drivers/passthrough/amd/iommu_intr.c |  6 ++
 xen/drivers/passthrough/vtd/intremap.c   | 70 +++++++-----------------
 xen/drivers/passthrough/vtd/iommu.c      |  7 +--
 3 files changed, 27 insertions(+), 56 deletions(-)

diff --git a/xen/drivers/passthrough/amd/iommu_intr.c b/xen/drivers/passthrough/amd/iommu_intr.c
index 7fc796dec2..9ab7c68749 100644
--- a/xen/drivers/passthrough/amd/iommu_intr.c
+++ b/xen/drivers/passthrough/amd/iommu_intr.c
@@ -649,6 +649,12 @@ bool __init cf_check iov_supports_xt(void)
     if ( !iommu_enable || !iommu_intremap )
         return false;
 
+    if ( unlikely(!cpu_has_cx16) )
+    {
+        AMD_IOMMU_WARN("CPU doesn't support CMPXCHG16B, disable interrupt remapping\n");
+        return false;
+    }
+
     if ( amd_iommu_prepare(true) )
         return false;
 
diff --git a/xen/drivers/passthrough/vtd/intremap.c b/xen/drivers/passthrough/vtd/intremap.c
index c504852eb8..7d4d907b4f 100644
--- a/xen/drivers/passthrough/vtd/intremap.c
+++ b/xen/drivers/passthrough/vtd/intremap.c
@@ -150,6 +150,13 @@ bool __init cf_check intel_iommu_supports_eim(void)
     if ( !iommu_qinval || !iommu_intremap || list_empty(&acpi_drhd_units) )
         return false;
 
+    if ( unlikely(!cpu_has_cx16) )
+    {
+        printk(XENLOG_WARNING VTDPREFIX
+               "CPU doesn't support CMPXCHG16B, disable interrupt remapping\n");
+        return false;
+    }
+
     /* We MUST have a DRHD unit for each IOAPIC. */
     for ( apic = 0; apic < nr_ioapics; apic++ )
         if ( !ioapic_to_drhd(IO_APIC_ID(apic)) )
@@ -173,47 +180,26 @@ bool __init cf_check intel_iommu_supports_eim(void)
  * Assume iremap_lock has been acquired. It is to make sure software will not
  * change the same IRTE behind us. With this assumption, if only high qword or
  * low qword in IRTE is to be updated, this function's atomic variant can
- * present an atomic update to VT-d hardware even when cmpxchg16b
- * instruction is not supported.
+ * present an atomic update to VT-d hardware.
  */
 static void update_irte(struct vtd_iommu *iommu, struct iremap_entry *entry,
                         const struct iremap_entry *new_ire, bool atomic)
 {
-    ASSERT(spin_is_locked(&iommu->intremap.lock));
+    __uint128_t ret;
+    struct iremap_entry old_ire;
 
-    if ( cpu_has_cx16 )
-    {
-        __uint128_t ret;
-        struct iremap_entry old_ire;
+    ASSERT(spin_is_locked(&iommu->intremap.lock));
 
-        old_ire = *entry;
-        ret = cmpxchg16b(entry, &old_ire, new_ire);
+    old_ire = *entry;
+    ret = cmpxchg16b(entry, &old_ire, new_ire);
 
-        /*
-         * In the above, we use cmpxchg16 to atomically update the 128-bit
-         * IRTE, and the hardware cannot update the IRTE behind us, so
-         * the return value of cmpxchg16 should be the same as old_ire.
-         * This ASSERT validate it.
-         */
-        ASSERT(ret == old_ire.val);
-    }
-    else
-    {
-        /*
-         * VT-d hardware doesn't update IRTEs behind us, nor the software
-         * since we hold iremap_lock. If the caller wants VT-d hardware to
-         * always see a consistent entry, but we can't meet it, a bug will
-         * be raised.
-         */
-        if ( entry->lo == new_ire->lo )
-            write_atomic(&entry->hi, new_ire->hi);
-        else if ( entry->hi == new_ire->hi )
-            write_atomic(&entry->lo, new_ire->lo);
-        else if ( !atomic )
-            *entry = *new_ire;
-        else
-            BUG();
-    }
+    /*
+     * In the above, we use cmpxchg16 to atomically update the 128-bit
+     * IRTE, and the hardware cannot update the IRTE behind us, so
+     * the return value of cmpxchg16 should be the same as old_ire.
+     * This ASSERT validate it.
+     */
+    ASSERT(ret == old_ire.val);
 }
 
 /* Mark specified intr remap entry as free */
@@ -395,7 +381,6 @@ static int ioapic_rte_to_remap_entry(struct vtd_iommu *iommu,
     /* Indicate remap format. */
     remap_rte->format = 1;
 
-    /* If cmpxchg16b is not available the caller must mask the IO-APIC pin. */
     update_irte(iommu, iremap_entry, &new_ire, !init && !masked);
     iommu_sync_cache(iremap_entry, sizeof(*iremap_entry));
     iommu_flush_iec_index(iommu, 0, index);
@@ -437,21 +422,6 @@ void cf_check io_apic_write_remap_rte(
     bool masked = true;
     int rc;
 
-    if ( !cpu_has_cx16 )
-    {
-       /*
-        * Cannot atomically update the IRTE entry: mask the IO-APIC pin to
-        * avoid interrupts seeing an inconsistent IRTE entry.
-        */
-        old_rte = __ioapic_read_entry(apic, pin, true);
-        if ( !old_rte.mask )
-        {
-            masked = false;
-            old_rte.mask = 1;
-            __ioapic_write_entry(apic, pin, true, old_rte);
-        }
-    }
-
     /* Not the initializer, for old gcc to cope. */
     new_rte.raw = rte;
 
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 7c6bae0256..a1bd3c5ff6 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2663,12 +2663,7 @@ static int __init cf_check vtd_setup(void)
             iommu_intremap = iommu_intremap_off;
 
 #ifndef iommu_intpost
-        /*
-         * We cannot use posted interrupt if X86_FEATURE_CX16 is
-         * not supported, since we count on this feature to
-         * atomically update 16-byte IRTE in posted format.
-         */
-        if ( !cap_intr_post(iommu->cap) || !iommu_intremap || !cpu_has_cx16 )
+        if ( !cap_intr_post(iommu->cap) || !iommu_intremap )
             iommu_intpost = false;
 #endif
 
-- 
2.44.0



Teddy Astie | Vates XCP-ng Intern

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [XEN PATCH v5 1/3] x86/iommu: Disable IOMMU if cx16 isn't supported

[Jan Beulich]

On 18.04.2024 13:57, Teddy Astie wrote:
> All hardware with VT-d/AMD-Vi has CMPXCHG16B support. Check this at
> initialisation time, and remove the effectively-dead logic for the
> non-cx16 case.

As before: What about Xen itself running virtualized, and the underlying
hypervisor surfacing an IOMMU but not CX16? It may be okay to ignore the
IOMMU in such an event, but by not mentioning the case you give the
appearance of not having considered it at all.

> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -305,6 +305,12 @@ static int __init cf_check iov_detect(void)
>      if ( !iommu_enable && !iommu_intremap )
>          return 0;
>  
> +    if ( unlikely(!cpu_has_cx16) )
> +    {
> +        printk("AMD-Vi: CPU doesn't support CMPXCHG16B, disabling\n");
> +        return -ENODEV;
> +    }
> +
>      if ( (init_done ? amd_iommu_init_late()
>                      : amd_iommu_init(false)) != 0 )
>      {

I did previously point out (and that's even visible in patch context here)
that the per-vendor .setup() hooks aren't necessarily the first thing to
run. Please can you make sure you address (verbally or by code) prior to
submitting new versions?

Jan

Re: [XEN PATCH v5 3/3] x86/iommu: Disable intrerrupt remapping if cx16 is not supported

[Jan Beulich]

On 18.04.2024 13:57, Teddy Astie wrote:
> All hardware with VT-d/AMD-Vi has CMPXCHG16B support.  Check this at
> initialisation time, and remove the effectively-dead logic for the non-cx16
> case.
> 
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>

Hmm, so this looks to be the code that I commented on as missing in patch 1.
I don't think this can be two separate patches. If you want to keep things
in small chunks, move the removal of "the effectively-dead logic" to a
separate, follow-on patch.

Jan

Re: [XEN PATCH v5 1/3] x86/iommu: Disable IOMMU if cx16 isn't supported

[Jan Beulich]

On 18.04.2024 13:57, Teddy Astie wrote:
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -305,6 +305,12 @@ static int __init cf_check iov_detect(void)
>      if ( !iommu_enable && !iommu_intremap )
>          return 0;
>  
> +    if ( unlikely(!cpu_has_cx16) )
> +    {
> +        printk("AMD-Vi: CPU doesn't support CMPXCHG16B, disabling\n");
> +        return -ENODEV;
> +    }

One additional nit: Here you neatly have just AMD-Vi: as a prefix for the
log message. Why ...

> @@ -2630,6 +2598,15 @@ static int __init cf_check vtd_setup(void)
>      int ret;
>      bool reg_inval_supported = true;
>  
> +    if ( unlikely(!cpu_has_cx16) )
> +    {
> +        printk(XENLOG_ERR VTDPREFIX
> +               "IOMMU: CPU doesn't support CMPXCHG16B, disabling\n");
> +
> +        ret = -ENODEV;
> +        goto error;
> +    }

... both VTDPREFIX and "IOMMU:" here?

Jan

Re: [XEN PATCH v5 1/3] x86/iommu: Disable IOMMU if cx16 isn't supported

[Roger Pau Monné]

On Wed, Apr 24, 2024 at 04:27:10PM +0200, Jan Beulich wrote:
> On 18.04.2024 13:57, Teddy Astie wrote:
> > All hardware with VT-d/AMD-Vi has CMPXCHG16B support. Check this at
> > initialisation time, and remove the effectively-dead logic for the
> > non-cx16 case.
> 
> As before: What about Xen itself running virtualized, and the underlying
> hypervisor surfacing an IOMMU but not CX16? It may be okay to ignore the
> IOMMU in such an event, but by not mentioning the case you give the
> appearance of not having considered it at all.
> 
> > --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> > +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> > @@ -305,6 +305,12 @@ static int __init cf_check iov_detect(void)
> >      if ( !iommu_enable && !iommu_intremap )
> >          return 0;
> >  
> > +    if ( unlikely(!cpu_has_cx16) )
> > +    {
> > +        printk("AMD-Vi: CPU doesn't support CMPXCHG16B, disabling\n");
> > +        return -ENODEV;
> > +    }
> > +
> >      if ( (init_done ? amd_iommu_init_late()
> >                      : amd_iommu_init(false)) != 0 )
> >      {
> 
> I did previously point out (and that's even visible in patch context here)
> that the per-vendor .setup() hooks aren't necessarily the first thing to
> run. Please can you make sure you address (verbally or by code) prior to
> submitting new versions?

I've re-visiting this as part of my other IOMMU IRTE atomic update
fix.

Would you prefer the check for CX16 be in the common x86
iommu_hardware_setup()?  That would be kind of layering violation, as
in principle a further IOMMU implementation on x86 might not require
CX16.  However I find it very unlikely, and hence I would be fine in
placing the check in iommu_hardware_setup() if you prefer it there.

Thanks, Roger.

Re: [XEN PATCH v5 1/3] x86/iommu: Disable IOMMU if cx16 isn't supported

[Jan Beulich]

On 21.01.2025 12:09, Roger Pau Monné wrote:
> On Wed, Apr 24, 2024 at 04:27:10PM +0200, Jan Beulich wrote:
>> On 18.04.2024 13:57, Teddy Astie wrote:
>>> All hardware with VT-d/AMD-Vi has CMPXCHG16B support. Check this at
>>> initialisation time, and remove the effectively-dead logic for the
>>> non-cx16 case.
>>
>> As before: What about Xen itself running virtualized, and the underlying
>> hypervisor surfacing an IOMMU but not CX16? It may be okay to ignore the
>> IOMMU in such an event, but by not mentioning the case you give the
>> appearance of not having considered it at all.
>>
>>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
>>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
>>> @@ -305,6 +305,12 @@ static int __init cf_check iov_detect(void)
>>>      if ( !iommu_enable && !iommu_intremap )
>>>          return 0;
>>>  
>>> +    if ( unlikely(!cpu_has_cx16) )
>>> +    {
>>> +        printk("AMD-Vi: CPU doesn't support CMPXCHG16B, disabling\n");
>>> +        return -ENODEV;
>>> +    }
>>> +
>>>      if ( (init_done ? amd_iommu_init_late()
>>>                      : amd_iommu_init(false)) != 0 )
>>>      {
>>
>> I did previously point out (and that's even visible in patch context here)
>> that the per-vendor .setup() hooks aren't necessarily the first thing to
>> run. Please can you make sure you address (verbally or by code) prior to
>> submitting new versions?
> 
> I've re-visiting this as part of my other IOMMU IRTE atomic update
> fix.
> 
> Would you prefer the check for CX16 be in the common x86
> iommu_hardware_setup()?  That would be kind of layering violation, as
> in principle a further IOMMU implementation on x86 might not require
> CX16.  However I find it very unlikely, and hence I would be fine in
> placing the check in iommu_hardware_setup() if you prefer it there.

No. The check needs replicating into the other hook that may run first,
->supports_x2apic(). And the ->enable_x2apic() hooks may want to gain
respective assertions then.

Jan

Re: [XEN PATCH v5 1/3] x86/iommu: Disable IOMMU if cx16 isn't supported

[Roger Pau Monné]

On Tue, Jan 21, 2025 at 12:20:17PM +0100, Jan Beulich wrote:
> On 21.01.2025 12:09, Roger Pau Monné wrote:
> > On Wed, Apr 24, 2024 at 04:27:10PM +0200, Jan Beulich wrote:
> >> On 18.04.2024 13:57, Teddy Astie wrote:
> >>> All hardware with VT-d/AMD-Vi has CMPXCHG16B support. Check this at
> >>> initialisation time, and remove the effectively-dead logic for the
> >>> non-cx16 case.
> >>
> >> As before: What about Xen itself running virtualized, and the underlying
> >> hypervisor surfacing an IOMMU but not CX16? It may be okay to ignore the
> >> IOMMU in such an event, but by not mentioning the case you give the
> >> appearance of not having considered it at all.
> >>
> >>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> >>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> >>> @@ -305,6 +305,12 @@ static int __init cf_check iov_detect(void)
> >>>      if ( !iommu_enable && !iommu_intremap )
> >>>          return 0;
> >>>  
> >>> +    if ( unlikely(!cpu_has_cx16) )
> >>> +    {
> >>> +        printk("AMD-Vi: CPU doesn't support CMPXCHG16B, disabling\n");
> >>> +        return -ENODEV;
> >>> +    }
> >>> +
> >>>      if ( (init_done ? amd_iommu_init_late()
> >>>                      : amd_iommu_init(false)) != 0 )
> >>>      {
> >>
> >> I did previously point out (and that's even visible in patch context here)
> >> that the per-vendor .setup() hooks aren't necessarily the first thing to
> >> run. Please can you make sure you address (verbally or by code) prior to
> >> submitting new versions?
> > 
> > I've re-visiting this as part of my other IOMMU IRTE atomic update
> > fix.
> > 
> > Would you prefer the check for CX16 be in the common x86
> > iommu_hardware_setup()?  That would be kind of layering violation, as
> > in principle a further IOMMU implementation on x86 might not require
> > CX16.  However I find it very unlikely, and hence I would be fine in
> > placing the check in iommu_hardware_setup() if you prefer it there.
> 
> No. The check needs replicating into the other hook that may run first,
> ->supports_x2apic(). And the ->enable_x2apic() hooks may want to gain
> respective assertions then.

Oh, I see.  So you either want patch 3 ahead of this, or both patches
merged into a single one.

Thanks, Roger.