[PATCH 0/5] Misc VT-d changes

[PATCH 0/5] Misc VT-d changes

[Teddy Astie]

[-- Attachment #1: Type: text/plain, Size: 1457 bytes --]

A small series of misc VT-d changes.

The first two sound like a potential security issue (compiler-related),
but not occuring in practice (discussed in security mailing list and
asked to be sent to public).

The 3rd one displays from which IOMMU orginated a fault to help debugging.

The 4th one is a bugfix for machines with unhandled SAGAW (e.g 5-level
pagetable support) to avoid "disabling passthrough" while we were errorneously
disabling the ability to configure Dom0 devices in "passthrough mode".

Last patch moves the intremap table to xenheap to simplify the logic to access
the entries and make it less error-prone.

No intended functionnal changes.

CI: https://gitlab.com/xen-project/people/tsnake41/xen/-/pipelines/2714830581

Teddy Astie (5):
  vtd: Ensure root entry is updated consistently
  vtd: Ensure context entry is cleared properly
  vtd: Print originating iommu on faults
  vtd: Don't disable hwdom passthrough on unhandled SAGAW bits
  vtd: Move intremap table to xenheap

 xen/drivers/passthrough/vtd/intremap.c | 61 +++++++++-----------------
 xen/drivers/passthrough/vtd/iommu.c    | 38 ++++++++--------
 xen/drivers/passthrough/vtd/iommu.h    | 16 +------
 xen/drivers/passthrough/vtd/utils.c    | 22 +++-------
 4 files changed, 45 insertions(+), 92 deletions(-)

-- 
2.54.0



--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

[PATCH 2/5] vtd: Ensure context entry is cleared properly

[Teddy Astie]

[-- Attachment #1: Type: text/plain, Size: 1803 bytes --]

When removing a context entry for a device, the present bit needs to
be cleared first, then we can clear the rest of the field. In the current
logic, the compiler is allowed to perform optimizations in a way where high
is cleared before the present bit (which is in low part) is, leading to a
window where the context entry is invalid and would make the IOMMU fault
(as address width would be set to a reserved value).

Fix the logic by ensuring we clear the low part first (which also clears
the present bit) then the high part afterward.

Fixes: cada0c18f8d1 ("vtd: Move dom0 RMRR check to intel_iommu_remove_device()")
Reported-by: Teddy Astie <teddy.astie@vates.tech>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
---
 xen/drivers/passthrough/vtd/iommu.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index c314ce1db8..1005e200c1 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1889,8 +1889,14 @@ int domain_context_unmap_one(
 
     iommu_domid = context_domain_id(*context);
 
-    context_clear_present(*context);
-    context_clear_entry(*context);
+    /*
+     * Clear the context entry.
+     *
+     * As this is performed with two stores, ensure lo (containing the present
+     * bit) is cleared first.
+     */
+    ACCESS_ONCE(context->lo) = 0;
+    ACCESS_ONCE(context->hi) = 0;
     iommu_sync_cache(context, sizeof(struct context_entry));
 
     rc = iommu_flush_context_device(iommu, iommu_domid,
-- 
2.54.0



--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

[PATCH 3/5] vtd: Print originating iommu on faults

[Teddy Astie]

[-- Attachment #1: Type: text/plain, Size: 1070 bytes --]

When a fault occurs on a IOMMU, print the IOMMU the fault is coming from.

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

diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 1005e200c1..5e9232d4b1 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -992,8 +992,8 @@ static int iommu_page_fault_do_one(struct vtd_iommu *iommu, int type,
         break;
     }
 
-    printk(XENLOG_G_WARNING VTDPREFIX "%s: reason %02x - %s\n",
-           kind, fault_reason, reason);
+    printk(XENLOG_G_WARNING VTDPREFIX "%s: reason %02x - %s on iommu:%u\n",
+           kind, fault_reason, reason, iommu->index);
 
     if ( iommu_verbose && fault_type == DMA_REMAP )
         print_vtd_entries(iommu, PCI_BUS(source_id), PCI_DEVFN(source_id),
-- 
2.54.0



--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

[PATCH 4/5] vtd: Don't disable hwdom passthrough on unhandled SAGAW bits

[Teddy Astie]

[-- Attachment #1: Type: text/plain, Size: 1495 bytes --]

On recent VT-d spec, bit 3 indicates support for 5-level pagetables,
this is currently considered unhandled and causes "hwdom passthrough"
to be disabled, even though it's unrelated.

Given these are is capability bits, we don't need to consider unhandled
bits, but only make sure that the ones we want (e.g 39-bit or 48-bit AGAW)
are set.

Fixes: 474fc7d3c652 ("iommu/vt-d: fix SAGAW capability parsing")
Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
---
 xen/drivers/passthrough/vtd/iommu.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 5e9232d4b1..a3b718d0cc 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1327,14 +1327,7 @@ int __init iommu_alloc(struct acpi_drhd_unit *drhd)
     }
 
     if ( sagaw >> 3 )
-    {
-        printk_once(XENLOG_WARNING VTDPREFIX
-                    " Unhandled bits in SAGAW %#x%s\n",
-                    sagaw,
-                    iommu_hwdom_passthrough ? ", disabling passthrough" : "");
-
-        iommu_hwdom_passthrough = false;
-    }
+        printk_once(XENLOG_WARNING VTDPREFIX " Unhandled bits in SAGAW %#x\n", sagaw);
 
     iommu->nr_pt_levels = agaw_to_level(agaw);
     if ( min_pt_levels > iommu->nr_pt_levels )
-- 
2.54.0



--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

[PATCH 5/5] vtd: Move intremap table to xenheap

[Teddy Astie]

[-- Attachment #1: Type: text/plain, Size: 13020 bytes --]

Interrupt remapping entries often needs to be accessed, and we're creating
pointers to it on demand, which brings a lot of complexity (e.g
GET_IREMAP_ENTRY() macro), move it to xenheap such that it's persistently
mapped and we won't have to worry about mapping and unmapping individual
intremap table pages.

Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
---
 xen/drivers/passthrough/vtd/intremap.c | 61 +++++++++-----------------
 xen/drivers/passthrough/vtd/iommu.h    | 16 +------
 xen/drivers/passthrough/vtd/utils.c    | 22 +++-------
 3 files changed, 26 insertions(+), 73 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/intremap.c b/xen/drivers/passthrough/vtd/intremap.c
index dd2788efd7..fc683516cb 100644
--- a/xen/drivers/passthrough/vtd/intremap.c
+++ b/xen/drivers/passthrough/vtd/intremap.c
@@ -17,6 +17,7 @@
  * Copyright (C) Xiaohui Xin <xiaohui.xin@intel.com>
  */
 
+#include <xen/mm.h>
 #include <xen/irq.h>
 #include <xen/sched.h>
 #include <xen/iommu.h>
@@ -208,21 +209,19 @@ static void update_irte(struct vtd_iommu *iommu, struct iremap_entry *entry,
 /* Mark specified intr remap entry as free */
 static void free_remap_entry(struct vtd_iommu *iommu, int index)
 {
-    struct iremap_entry *iremap_entry = NULL, *iremap_entries, new_ire = { };
+    struct iremap_entry *iremap_entry = NULL, new_ire = { };
 
     if ( index < 0 || index > IREMAP_ENTRY_NR - 1 )
         return;
 
     ASSERT(spin_is_locked(&iommu->intremap.lock));
 
-    GET_IREMAP_ENTRY(iommu->intremap.maddr, index,
-                     iremap_entries, iremap_entry);
+    iremap_entry = &iommu->intremap.entries[index];
 
     update_irte(iommu, iremap_entry, &new_ire, false);
     iommu_sync_cache(iremap_entry, sizeof(*iremap_entry));
     iommu_flush_iec_index(iommu, 0, index);
 
-    unmap_vtd_domain_page(iremap_entries);
     iommu->intremap.num--;
 }
 
@@ -232,25 +231,13 @@ static void free_remap_entry(struct vtd_iommu *iommu, int index)
  */
 static unsigned int alloc_remap_entry(struct vtd_iommu *iommu, unsigned int nr)
 {
-    struct iremap_entry *iremap_entries = NULL;
     unsigned int i, found;
 
     ASSERT(spin_is_locked(&iommu->intremap.lock));
 
     for ( found = i = 0; i < IREMAP_ENTRY_NR; i++ )
     {
-        struct iremap_entry *p;
-        if ( i % (1 << IREMAP_ENTRY_ORDER) == 0 )
-        {
-            /* This entry across page boundry */
-            if ( iremap_entries )
-                unmap_vtd_domain_page(iremap_entries);
-
-            GET_IREMAP_ENTRY(iommu->intremap.maddr, i,
-                             iremap_entries, p);
-        }
-        else
-            p = &iremap_entries[i % (1 << IREMAP_ENTRY_ORDER)];
+        struct iremap_entry *p = &iommu->intremap.entries[i];
 
         if ( p->val ) /* not a free entry */
             found = 0;
@@ -258,9 +245,6 @@ static unsigned int alloc_remap_entry(struct vtd_iommu *iommu, unsigned int nr)
             break;
     }
 
-    if ( iremap_entries )
-        unmap_vtd_domain_page(iremap_entries);
-
     if ( i < IREMAP_ENTRY_NR )
         iommu->intremap.num += nr;
 
@@ -270,7 +254,7 @@ static unsigned int alloc_remap_entry(struct vtd_iommu *iommu, unsigned int nr)
 static int remap_entry_to_ioapic_rte(
     struct vtd_iommu *iommu, int index, struct IO_APIC_route_entry *old_rte)
 {
-    struct iremap_entry *iremap_entry = NULL, *iremap_entries;
+    struct iremap_entry *iremap_entry = NULL;
     unsigned long flags;
 
     if ( index < 0 || index > IREMAP_ENTRY_NR - 1 )
@@ -283,15 +267,13 @@ static int remap_entry_to_ioapic_rte(
 
     spin_lock_irqsave(&iommu->intremap.lock, flags);
 
-    GET_IREMAP_ENTRY(iommu->intremap.maddr, index,
-                     iremap_entries, iremap_entry);
+    iremap_entry = &iommu->intremap.entries[index];
 
     if ( iremap_entry->val == 0 )
     {
         dprintk(XENLOG_ERR VTDPREFIX,
                 "IO-APIC index (%d) has an empty entry\n",
                 index);
-        unmap_vtd_domain_page(iremap_entries);
         spin_unlock_irqrestore(&iommu->intremap.lock, flags);
         return -EFAULT;
     }
@@ -309,7 +291,6 @@ static int remap_entry_to_ioapic_rte(
         old_rte->dest.logical.logical_dest = iremap_entry->remap.dst >> 8;
     }
 
-    unmap_vtd_domain_page(iremap_entries);
     spin_unlock_irqrestore(&iommu->intremap.lock, flags);
 
     return 0;
@@ -319,7 +300,7 @@ static int ioapic_rte_to_remap_entry(struct vtd_iommu *iommu,
     int apic, unsigned int ioapic_pin, struct IO_APIC_route_entry *old_rte,
     struct IO_APIC_route_entry new_rte)
 {
-    struct iremap_entry *iremap_entry = NULL, *iremap_entries;
+    struct iremap_entry *iremap_entry = NULL;
     struct iremap_entry new_ire;
     struct IO_APIC_route_remap_entry *remap_rte;
     int index;
@@ -347,8 +328,7 @@ static int ioapic_rte_to_remap_entry(struct vtd_iommu *iommu,
         return -EFAULT;
     }
 
-    GET_IREMAP_ENTRY(iommu->intremap.maddr, index,
-                     iremap_entries, iremap_entry);
+    iremap_entry = &iommu->intremap.entries[index];
 
     new_ire = *iremap_entry;
 
@@ -388,7 +368,6 @@ static int ioapic_rte_to_remap_entry(struct vtd_iommu *iommu,
     iommu_sync_cache(iremap_entry, sizeof(*iremap_entry));
     iommu_flush_iec_index(iommu, 0, index);
 
-    unmap_vtd_domain_page(iremap_entries);
     spin_unlock_irqrestore(&iommu->intremap.lock, flags);
     return 0;
 }
@@ -500,7 +479,7 @@ static int msi_msg_to_remap_entry(
     struct vtd_iommu *iommu, struct pci_dev *pdev,
     struct msi_desc *msi_desc, struct msi_msg *msg)
 {
-    struct iremap_entry *iremap_entry = NULL, *iremap_entries, new_ire = { };
+    struct iremap_entry *iremap_entry = NULL, new_ire = { };
     struct msi_msg_remap_entry *remap_rte;
     unsigned int index, i, nr = 1;
     unsigned long flags;
@@ -556,8 +535,7 @@ static int msi_msg_to_remap_entry(
         return -EFAULT;
     }
 
-    GET_IREMAP_ENTRY(iommu->intremap.maddr, index,
-                     iremap_entries, iremap_entry);
+    iremap_entry = &iommu->intremap.entries[index];
 
     if ( !pi_desc )
     {
@@ -604,7 +582,6 @@ static int msi_msg_to_remap_entry(
     iommu_sync_cache(iremap_entry, sizeof(*iremap_entry));
     iommu_flush_iec_index(iommu, 0, index);
 
-    unmap_vtd_domain_page(iremap_entries);
     spin_unlock_irqrestore(&iommu->intremap.lock, flags);
 
     return alloc;
@@ -628,7 +605,7 @@ int __init cf_check intel_setup_hpet_msi(struct msi_desc *msi_desc)
     unsigned long flags;
     int rc = 0;
 
-    if ( !iommu->intremap.maddr )
+    if ( !iommu->intremap.entries )
         return 0;
 
     spin_lock_irqsave(&iommu->intremap.lock, flags);
@@ -663,7 +640,7 @@ int enable_intremap(struct vtd_iommu *iommu, int eim)
     sts = dmar_readl(iommu->reg, DMAR_GSTS_REG);
 
     /* Return if already enabled by Xen */
-    if ( (sts & DMA_GSTS_IRES) && iommu->intremap.maddr )
+    if ( (sts & DMA_GSTS_IRES) && iommu->intremap.entries )
         return 0;
 
     if ( !(sts & DMA_GSTS_QIES) )
@@ -679,16 +656,18 @@ int enable_intremap(struct vtd_iommu *iommu, int eim)
                " Compatibility Format Interrupts permitted on IOMMU #%u:"
                " Device pass-through will be insecure\n", iommu->index);
 
-    if ( iommu->intremap.maddr == 0 )
+    if ( !iommu->intremap.entries )
     {
-        iommu->intremap.maddr = alloc_pgtable_maddr(IREMAP_ARCH_PAGE_NR,
-                                                    iommu->node);
-        if ( iommu->intremap.maddr == 0 )
+        iommu->intremap.entries =
+            alloc_xenheap_pages(get_order_from_pages(IREMAP_ARCH_PAGE_NR),
+                                                     MEMF_node(iommu->node));
+        if ( !iommu->intremap.entries )
         {
             dprintk(XENLOG_WARNING VTDPREFIX,
-                    "Cannot allocate memory for ir_ctrl->iremap_maddr\n");
+                    "Cannot allocate memory for iommu->intremap.entries\n");
             return -ENOMEM;
         }
+        memset(iommu->intremap.entries, 0, IREMAP_ARCH_PAGE_NR * PAGE_SIZE);
 
         iommu->intremap.num = 0;
     }
@@ -700,7 +679,7 @@ int enable_intremap(struct vtd_iommu *iommu, int eim)
      * Interrupt Mode.
      */
     dmar_writeq(iommu->reg, DMAR_IRTA_REG,
-                iommu->intremap.maddr | IRTA_REG_TABLE_SIZE |
+                virt_to_maddr(iommu->intremap.entries) | IRTA_REG_TABLE_SIZE |
                 (eim ? IRTA_EIME : 0));
 
     /* set SIRTP */
diff --git a/xen/drivers/passthrough/vtd/iommu.h b/xen/drivers/passthrough/vtd/iommu.h
index ccb00889d7..0cd2c2ef3a 100644
--- a/xen/drivers/passthrough/vtd/iommu.h
+++ b/xen/drivers/passthrough/vtd/iommu.h
@@ -357,20 +357,6 @@ struct iremap_entry {
 #define iremap_set_present(v) do {(v).lo |= 1;} while(0)
 #define iremap_clear_present(v) do {(v).lo &= ~1;} while(0)
 
-/*
- * Get the intr remap entry:
- * maddr   - machine addr of the table
- * index   - index of the entry
- * entries - return addr of the page holding this entry, need unmap it
- * entry   - return required entry
- */
-#define GET_IREMAP_ENTRY(maddr, index, entries, entry)                        \
-do {                                                                          \
-    entries = (struct iremap_entry *)map_vtd_domain_page(                     \
-              (maddr) + (( (index) >> IREMAP_ENTRY_ORDER ) << PAGE_SHIFT ) ); \
-    entry = &entries[(index) % (1 << IREMAP_ENTRY_ORDER)];                    \
-} while(0)
-
 /* queue invalidation entry */
 struct qinval_entry {
     union {
@@ -491,7 +477,7 @@ struct vtd_iommu {
     uint64_t qinval_maddr;   /* queue invalidation page machine address */
 
     struct {
-        uint64_t maddr;   /* interrupt remap table machine address */
+        struct iremap_entry *entries; /* interrupt remap table entries */
         unsigned int num; /* total num of used interrupt remap entry */
         spinlock_t lock;  /* lock for irq remapping table */
     } intremap;
diff --git a/xen/drivers/passthrough/vtd/utils.c b/xen/drivers/passthrough/vtd/utils.c
index 7c4d032f4f..3ae4d35e2d 100644
--- a/xen/drivers/passthrough/vtd/utils.c
+++ b/xen/drivers/passthrough/vtd/utils.c
@@ -187,11 +187,12 @@ void cf_check vtd_dump_iommu_info(unsigned char key)
         {
             /* Dump interrupt remapping table. */
             uint64_t irta = dmar_readq(iommu->reg, DMAR_IRTA_REG);
-            uint64_t iremap_maddr = irta & PAGE_MASK;
             unsigned int nr_entry = 1 << ((irta & 0xF) + 1);
-            struct iremap_entry *iremap_entries = NULL;
             unsigned int print_cnt = 0;
 
+            /* Hardware must be using the same table as us. */
+            ASSERT((irta & PAGE_MASK) == virt_to_maddr(iommu->intremap.entries));
+
             printk("  Interrupt remapping table (nr_entry=%#x. "
                 "Only dump P=1 entries here):\n", nr_entry);
             printk("R means remapped format, P means posted format.\n");
@@ -199,18 +200,7 @@ void cf_check vtd_dump_iommu_info(unsigned char key)
             printk("P:       SVT  SQ   SID  V  AVL FPD              PDA  URG P\n");
             for ( i = 0; i < nr_entry; i++ )
             {
-                struct iremap_entry *p;
-                if ( i % (1 << IREMAP_ENTRY_ORDER) == 0 )
-                {
-                    /* This entry across page boundry */
-                    if ( iremap_entries )
-                        unmap_vtd_domain_page(iremap_entries);
-
-                    GET_IREMAP_ENTRY(iremap_maddr, i,
-                                     iremap_entries, p);
-                }
-                else
-                    p = &iremap_entries[i % (1 << IREMAP_ENTRY_ORDER)];
+                struct iremap_entry *p = &iommu->intremap.entries[i];
 
                 if ( !p->remap.p )
                     continue;
@@ -231,8 +221,6 @@ void cf_check vtd_dump_iommu_info(unsigned char key)
 
                 print_cnt++;
             }
-            if ( iremap_entries )
-                unmap_vtd_domain_page(iremap_entries);
             if ( iommu->intremap.num != print_cnt )
                 printk("Warning: Print %u IRTE (actually have %u)!\n",
                         print_cnt, iommu->intremap.num);
@@ -251,7 +239,7 @@ void cf_check vtd_dump_iommu_info(unsigned char key)
         {
             iommu = ioapic_to_iommu(mp_ioapics[apic].mpc_apicid);
 
-            if ( !iommu->intremap.maddr || !iommu->intremap.num )
+            if ( !iommu->intremap.entries || !iommu->intremap.num )
                 continue;
 
             printk( "\nRedirection table of IOAPIC %x:\n", apic);
-- 
2.54.0



--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

[RESEND PATCH 1/5] vtd: Ensure root entry is updated consistently

[Teddy Astie]

[-- Attachment #1: Type: text/plain, Size: 2324 bytes --]

Make sure the root entry is updated consistently such that the compiler
is not allowed to perform optimizations around setting present bit (and
i.e set present bit before setting up the pagetable addresss, which would
cause the IOMMU to use a pagetable at address 0 instead).

This issue can only occurs at boot time where present bit is initially cleared
or when a bus is first introduced in the machine.

Fixes: ccfe4e08455f ("Intel vt-d specific changes in arch/x86/hvm/vmx/vtd.")
Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
---
 xen/drivers/passthrough/vtd/iommu.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 3d6f787b3c..c314ce1db8 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -281,13 +281,13 @@ void free_pgtable_maddr(u64 maddr)
 /* context entry handling */
 static u64 bus_to_context_maddr(struct vtd_iommu *iommu, u8 bus)
 {
-    struct root_entry *root, *root_entries;
+    struct root_entry root, *root_entries;
     u64 maddr;
 
     ASSERT(spin_is_locked(&iommu->lock));
     root_entries = (struct root_entry *)map_vtd_domain_page(iommu->root_maddr);
-    root = &root_entries[bus];
-    if ( !root_present(*root) )
+    root.val = ACCESS_ONCE(root_entries[bus].val);
+    if ( !root_present(root) )
     {
         maddr = alloc_pgtable_maddr(1, iommu->node);
         if ( maddr == 0 )
@@ -295,11 +295,12 @@ static u64 bus_to_context_maddr(struct vtd_iommu *iommu, u8 bus)
             unmap_vtd_domain_page(root_entries);
             return 0;
         }
-        set_root_value(*root, maddr);
-        set_root_present(*root);
-        iommu_sync_cache(root, sizeof(struct root_entry));
+        set_root_value(root, maddr);
+        set_root_present(root);
+        ACCESS_ONCE(root_entries[bus].val) = root.val;
+        iommu_sync_cache(&root_entries[bus], sizeof(struct root_entry));
     }
-    maddr = (u64) get_context_addr(*root);
+    maddr = (u64) get_context_addr(root);
     unmap_vtd_domain_page(root_entries);
     return maddr;
 }
-- 
2.54.0



--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [RESEND PATCH 1/5] vtd: Ensure root entry is updated consistently

[Jan Beulich]

On 29.07.2026 12:05, Teddy Astie wrote:
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -281,13 +281,13 @@ void free_pgtable_maddr(u64 maddr)
>  /* context entry handling */
>  static u64 bus_to_context_maddr(struct vtd_iommu *iommu, u8 bus)
>  {
> -    struct root_entry *root, *root_entries;
> +    struct root_entry root, *root_entries;
>      u64 maddr;
>  
>      ASSERT(spin_is_locked(&iommu->lock));
>      root_entries = (struct root_entry *)map_vtd_domain_page(iommu->root_maddr);
> -    root = &root_entries[bus];
> -    if ( !root_present(*root) )
> +    root.val = ACCESS_ONCE(root_entries[bus].val);

I'm pretty concerned about this: You're reading only half of the entry here,
and you're writing only half of it further down. The other half is reserved
right now, but there's not even a comment being added to this effect. (Yet
even with a comment, I'd still be concerned, just not as much.)

> @@ -295,11 +295,12 @@ static u64 bus_to_context_maddr(struct vtd_iommu *iommu, u8 bus)
>              unmap_vtd_domain_page(root_entries);
>              return 0;
>          }
> -        set_root_value(*root, maddr);
> -        set_root_present(*root);
> -        iommu_sync_cache(root, sizeof(struct root_entry));
> +        set_root_value(root, maddr);
> +        set_root_present(root);
> +        ACCESS_ONCE(root_entries[bus].val) = root.val;
> +        iommu_sync_cache(&root_entries[bus], sizeof(struct root_entry));

sizeof(<expression>) please in favor of sizeof(<type>), whenever possible.

>      }
> -    maddr = (u64) get_context_addr(*root);
> +    maddr = (u64) get_context_addr(root);

While there, drop the pointless casts, thus getting rid of two style issues
as well?

Jan

Re: [PATCH 2/5] vtd: Ensure context entry is cleared properly

[Jan Beulich]

On 29.07.2026 11:59, Teddy Astie wrote:
> When removing a context entry for a device, the present bit needs to
> be cleared first, then we can clear the rest of the field. In the current
> logic, the compiler is allowed to perform optimizations in a way where high
> is cleared before the present bit (which is in low part) is, leading to a
> window where the context entry is invalid and would make the IOMMU fault
> (as address width would be set to a reserved value).
> 
> Fix the logic by ensuring we clear the low part first (which also clears
> the present bit) then the high part afterward.
> 
> Fixes: cada0c18f8d1 ("vtd: Move dom0 RMRR check to intel_iommu_remove_device()")
> Reported-by: Teddy Astie <teddy.astie@vates.tech>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
> ---
>  xen/drivers/passthrough/vtd/iommu.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
> index c314ce1db8..1005e200c1 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1889,8 +1889,14 @@ int domain_context_unmap_one(
>  
>      iommu_domid = context_domain_id(*context);
>  
> -    context_clear_present(*context);
> -    context_clear_entry(*context);
> +    /*
> +     * Clear the context entry.
> +     *
> +     * As this is performed with two stores, ensure lo (containing the present
> +     * bit) is cleared first.
> +     */
> +    ACCESS_ONCE(context->lo) = 0;
> +    ACCESS_ONCE(context->hi) = 0;

Implying the placement of the present bit is again something I'm a little uneasy
with.

With the uses of context_clear_{present,entry}() dropped, the macros are unused.
I think they would better be dropped right away, to prevent misguided use
elsewhere.

Jan

Re: [PATCH 3/5] vtd: Print originating iommu on faults

[Jan Beulich]

On 29.07.2026 11:59, Teddy Astie wrote:
> When a fault occurs on a IOMMU, print the IOMMU the fault is coming from.
> 
> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>

Acked-by: Jan Beulich <jbeulich@suse.com>

Nevertheless I think this doesn't go quite far enough - this internal index,
to be helpful, would need associating back to the respective firmware data.
What iommu_alloc() logs is not only limited to verbose mode, but also doesn't
include the index. I think something need doing there, and then we need to be
more consistent throughout to include the IOMMU index in log messages
pertaining to a particular IOMMU.

Jan

Re: [PATCH 4/5] vtd: Don't disable hwdom passthrough on unhandled SAGAW bits

[Jan Beulich]

On 29.07.2026 11:59, Teddy Astie wrote:
> On recent VT-d spec, bit 3 indicates support for 5-level pagetables,
> this is currently considered unhandled and causes "hwdom passthrough"
> to be disabled, even though it's unrelated.
> 
> Given these are is capability bits, we don't need to consider unhandled
> bits, but only make sure that the ones we want (e.g 39-bit or 48-bit AGAW)
> are set.
> 
> Fixes: 474fc7d3c652 ("iommu/vt-d: fix SAGAW capability parsing")

The description of that commit explains pretty well why pass-through mode
does need disabling in that case. If there's anything wrong with that
explanation, this would need calling out here.

Additionally I can only repeat my proposal to finally default to strict
mode. In strict mode, pass-through mode is disabled anyway. (IOW there's
the additional question of why you need pass-through mode in the first
place.)

> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1327,14 +1327,7 @@ int __init iommu_alloc(struct acpi_drhd_unit *drhd)
>      }
>  
>      if ( sagaw >> 3 )
> -    {
> -        printk_once(XENLOG_WARNING VTDPREFIX
> -                    " Unhandled bits in SAGAW %#x%s\n",
> -                    sagaw,
> -                    iommu_hwdom_passthrough ? ", disabling passthrough" : "");
> -
> -        iommu_hwdom_passthrough = false;
> -    }
> +        printk_once(XENLOG_WARNING VTDPREFIX " Unhandled bits in SAGAW %#x\n", sagaw);

Also please adhere to the line length limit.

Jan

Re: [PATCH 5/5] vtd: Move intremap table to xenheap

[Jan Beulich]

On 29.07.2026 11:59, Teddy Astie wrote:
> Interrupt remapping entries often needs to be accessed, and we're creating
> pointers to it on demand, which brings a lot of complexity (e.g
> GET_IREMAP_ENTRY() macro), move it to xenheap such that it's persistently
> mapped and we won't have to worry about mapping and unmapping individual
> intremap table pages.

Afaic: No movement from domheap to xenheap except for _very_ good reasons.
For the case here that is - maybe establish a permanent mapping using
vmap(), but no change in where the memory is to come from. Whether such a
permanent mapping is really worthwhile may also want supporting by numbers.
You say "often", but you don't qualify / quantify this any further.

Jan

Re: [PATCH 5/5] vtd: Move intremap table to xenheap

[Andrew Cooper]

On 05/08/2026 10:44 am, Jan Beulich wrote:
> On 29.07.2026 11:59, Teddy Astie wrote:
>> Interrupt remapping entries often needs to be accessed, and we're creating
>> pointers to it on demand, which brings a lot of complexity (e.g
>> GET_IREMAP_ENTRY() macro), move it to xenheap such that it's persistently
>> mapped and we won't have to worry about mapping and unmapping individual
>> intremap table pages.
> Afaic: No movement from domheap to xenheap except for _very_ good reasons.
> For the case here that is - maybe establish a permanent mapping using
> vmap(), but no change in where the memory is to come from. Whether such a
> permanent mapping is really worthwhile may also want supporting by numbers.
> You say "often", but you don't qualify / quantify this any further.

To expand on the "why" a bit more.

For systems with all RAM below the 4T boundary, domheap and xenheap are
equivalent.  We have 5T of directmap, but xenheap allocations have a
width restriction which is a power-of-2.

For systems with any RAM above the 4T boundary, you can't have xenheap
allocations be NUMA-local for all NUMA nodes.


As for "often", the IRTEs are modified every time a vCPU moves to a
different PCPU, because the target addresses need updating.  While it
probably doesn't matter much today, in the context of ASI it's something
which would want mapping permanently, rather than on-demand.

~Andrew

Re: [PATCH 5/5] vtd: Move intremap table to xenheap

[Teddy Astie]

[-- Attachment #1.1.1: Type: text/plain, Size: 1763 bytes --]

Le 05/08/2026 à 12:09, Andrew Cooper a écrit :
> On 05/08/2026 10:44 am, Jan Beulich wrote:
>> On 29.07.2026 11:59, Teddy Astie wrote:
>>> Interrupt remapping entries often needs to be accessed, and we're creating
>>> pointers to it on demand, which brings a lot of complexity (e.g
>>> GET_IREMAP_ENTRY() macro), move it to xenheap such that it's persistently
>>> mapped and we won't have to worry about mapping and unmapping individual
>>> intremap table pages.
>> Afaic: No movement from domheap to xenheap except for _very_ good reasons.
>> For the case here that is - maybe establish a permanent mapping using
>> vmap(), but no change in where the memory is to come from. Whether such a
>> permanent mapping is really worthwhile may also want supporting by numbers.
>> You say "often", but you don't qualify / quantify this any further.
> 

I think the reduction the complexity of the logic is a good reason into 
using a persistent mapping here (especially since it's not a 
particularly large one).

> To expand on the "why" a bit more.
> 
> For systems with all RAM below the 4T boundary, domheap and xenheap are
> equivalent.  We have 5T of directmap, but xenheap allocations have a
> width restriction which is a power-of-2.
> 
> For systems with any RAM above the 4T boundary, you can't have xenheap
> allocations be NUMA-local for all NUMA nodes.
> 
> 
> As for "often", the IRTEs are modified every time a vCPU moves to a
> different PCPU, because the target addresses need updating.  While it
> probably doesn't matter much today, in the context of ASI it's something
> which would want mapping permanently, rather than on-demand.
> 

Ok, so domheap with vmap would be preferable here.

> ~Andrew>

Teddy

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 2489 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 665 bytes --]

Re: [RESEND PATCH 1/5] vtd: Ensure root entry is updated consistently

[Teddy Astie]

[-- Attachment #1.1.1: Type: text/plain, Size: 2250 bytes --]

Le 04/08/2026 à 18:00, Jan Beulich a écrit :
> On 29.07.2026 12:05, Teddy Astie wrote:
>> --- a/xen/drivers/passthrough/vtd/iommu.c
>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>> @@ -281,13 +281,13 @@ void free_pgtable_maddr(u64 maddr)
>>   /* context entry handling */
>>   static u64 bus_to_context_maddr(struct vtd_iommu *iommu, u8 bus)
>>   {
>> -    struct root_entry *root, *root_entries;
>> +    struct root_entry root, *root_entries;
>>       u64 maddr;
>>   
>>       ASSERT(spin_is_locked(&iommu->lock));
>>       root_entries = (struct root_entry *)map_vtd_domain_page(iommu->root_maddr);
>> -    root = &root_entries[bus];
>> -    if ( !root_present(*root) )
>> +    root.val = ACCESS_ONCE(root_entries[bus].val);
> 
> I'm pretty concerned about this: You're reading only half of the entry here,
> and you're writing only half of it further down. The other half is reserved
> right now, but there's not even a comment being added to this effect. (Yet
> even with a comment, I'd still be concerned, just not as much.)
> 

The idea is to match the original logic while making it always compile 
correctly. As we don't interact with the other part of the root entry 
(reserved, or upper context table with Scalable-Mode).

Ideally, it should be written using a bitfield structure instead, but 
it's a much larger change.

>> @@ -295,11 +295,12 @@ static u64 bus_to_context_maddr(struct vtd_iommu *iommu, u8 bus)
>>               unmap_vtd_domain_page(root_entries);
>>               return 0;
>>           }
>> -        set_root_value(*root, maddr);
>> -        set_root_present(*root);
>> -        iommu_sync_cache(root, sizeof(struct root_entry));
>> +        set_root_value(root, maddr);
>> +        set_root_present(root);
>> +        ACCESS_ONCE(root_entries[bus].val) = root.val;
>> +        iommu_sync_cache(&root_entries[bus], sizeof(struct root_entry));
> 
> sizeof(<expression>) please in favor of sizeof(<type>), whenever possible.
> 
>>       }
>> -    maddr = (u64) get_context_addr(*root);
>> +    maddr = (u64) get_context_addr(root);
> 
> While there, drop the pointless casts, thus getting rid of two style issues
> as well?
> 

Fixed locally.

> Jan
> 

Teddy

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 2489 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 665 bytes --]