[Xen-devel] [PATCH v2 2/2] iommu: translate IO-APIC pins when enabling interrupt remapping

Roger Pau Monne posted 2 patches 5 weeks ago

[Xen-devel] [PATCH v2 2/2] iommu: translate IO-APIC pins when enabling interrupt remapping

Posted by Roger Pau Monne 5 weeks ago
On Intel hardware there's currently no translation of already enabled
IO-APIC pins when interrupt remapping is enabled on the IOMMU, hence
introduce a logic similar to the one used in x2apic_bsp_setup in order
to save and mask all IO-APIC pins, and then translate and restore them
after interrupt remapping has been enabled.

With this change the AMD specific logic to deal with enabled pins
(amd_iommu_setup_ioapic_remapping) can be removed, thus unifying the
handling of IO-APIC when enabling interrupt remapping regardless of
the IOMMU vendor.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Juergen Gross <jgross@suse.com>
---
 xen/drivers/passthrough/amd/iommu_init.c      | 11 ++-
 xen/drivers/passthrough/amd/iommu_intr.c      | 90 +------------------
 xen/drivers/passthrough/x86/iommu.c           | 34 ++++++-
 xen/include/asm-x86/hvm/svm/amd-iommu-proto.h |  1 +
 4 files changed, 40 insertions(+), 96 deletions(-)

diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index 6f53c7ec08..3c244619b9 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -19,6 +19,7 @@
 
 #include <xen/errno.h>
 #include <xen/acpi.h>
+#include <xen/keyhandler.h>
 #include <xen/pci.h>
 #include <xen/pci_regs.h>
 #include <xen/irq.h>
@@ -1435,12 +1436,6 @@ int __init amd_iommu_init(bool xt)
     if ( rc )
         goto error_out;
 
-    /* initialize io-apic interrupt remapping entries */
-    if ( iommu_intremap )
-        rc = amd_iommu_setup_ioapic_remapping();
-    if ( rc )
-        goto error_out;
-
     /* Allocate and initialize device table(s). */
     pci_init = !xt;
     rc = iterate_ivrs_mappings(amd_iommu_setup_device_table);
@@ -1469,6 +1464,10 @@ int __init amd_iommu_init(bool xt)
             goto error_out;
     }
 
+    if ( iommu_intremap )
+        register_keyhandler('V', &amd_iommu_dump_intremap_tables,
+                            "dump IOMMU intremap tables", 0);
+
     return 0;
 
 error_out:
diff --git a/xen/drivers/passthrough/amd/iommu_intr.c b/xen/drivers/passthrough/amd/iommu_intr.c
index fb71073c84..1eed60f265 100644
--- a/xen/drivers/passthrough/amd/iommu_intr.c
+++ b/xen/drivers/passthrough/amd/iommu_intr.c
@@ -21,7 +21,6 @@
 #include <asm/amd-iommu.h>
 #include <asm/hvm/svm/amd-iommu-proto.h>
 #include <asm/io_apic.h>
-#include <xen/keyhandler.h>
 #include <xen/softirq.h>
 
 union irte32 {
@@ -79,8 +78,6 @@ unsigned long *shared_intremap_inuse;
 static DEFINE_SPINLOCK(shared_intremap_lock);
 unsigned int nr_ioapic_sbdf;
 
-static void dump_intremap_tables(unsigned char key);
-
 #define intremap_page_order(irt) PFN_ORDER(virt_to_page(irt))
 
 unsigned int amd_iommu_intremap_table_order(
@@ -354,91 +351,6 @@ static int update_intremap_entry_from_ioapic(
     return 0;
 }
 
-int __init amd_iommu_setup_ioapic_remapping(void)
-{
-    struct IO_APIC_route_entry rte;
-    unsigned long flags;
-    union irte_ptr entry;
-    int apic, pin;
-    u8 delivery_mode, dest, vector, dest_mode;
-    u16 seg, bdf, req_id;
-    struct amd_iommu *iommu;
-    spinlock_t *lock;
-    unsigned int offset;
-
-    /* Read ioapic entries and update interrupt remapping table accordingly */
-    for ( apic = 0; apic < nr_ioapics; apic++ )
-    {
-        for ( pin = 0; pin < nr_ioapic_entries[apic]; pin++ )
-        {
-            unsigned int idx;
-
-            rte = __ioapic_read_entry(apic, pin, 1);
-            if ( rte.mask == 1 )
-                continue;
-
-            /* get device id of ioapic devices */
-            idx = ioapic_id_to_index(IO_APIC_ID(apic));
-            if ( idx == MAX_IO_APICS )
-                return -EINVAL;
-
-            bdf = ioapic_sbdf[idx].bdf;
-            seg = ioapic_sbdf[idx].seg;
-            iommu = find_iommu_for_device(seg, bdf);
-            if ( !iommu )
-            {
-                AMD_IOMMU_DEBUG("Fail to find iommu for ioapic "
-                                "device id = %04x:%04x\n", seg, bdf);
-                continue;
-            }
-
-            req_id = get_intremap_requestor_id(iommu->seg, bdf);
-            lock = get_intremap_lock(iommu->seg, req_id);
-
-            delivery_mode = rte.delivery_mode;
-            vector = rte.vector;
-            dest_mode = rte.dest_mode;
-            dest = rte.dest.logical.logical_dest;
-
-            if ( iommu->ctrl.xt_en )
-            {
-                /*
-                 * In x2APIC mode we have no way of discovering the high 24
-                 * bits of the destination of an already enabled interrupt.
-                 * We come here earlier than for xAPIC mode, so no interrupts
-                 * should have been set up before.
-                 */
-                AMD_IOMMU_DEBUG("Unmasked IO-APIC#%u entry %u in x2APIC mode\n",
-                                IO_APIC_ID(apic), pin);
-            }
-
-            spin_lock_irqsave(lock, flags);
-            offset = alloc_intremap_entry(iommu, req_id, 1);
-            BUG_ON(offset >= INTREMAP_MAX_ENTRIES);
-            entry = get_intremap_entry(iommu, req_id, offset);
-            update_intremap_entry(iommu, entry, vector,
-                                  delivery_mode, dest_mode, dest);
-            spin_unlock_irqrestore(lock, flags);
-
-            set_rte_index(&rte, offset);
-            ioapic_sbdf[idx].pin_2_idx[pin] = offset;
-            __ioapic_write_entry(apic, pin, 1, rte);
-
-            if ( iommu->enabled )
-            {
-                spin_lock_irqsave(&iommu->lock, flags);
-                amd_iommu_flush_intremap(iommu, req_id);
-                spin_unlock_irqrestore(&iommu->lock, flags);
-            }
-        }
-    }
-
-    register_keyhandler('V', &dump_intremap_tables,
-                        "dump IOMMU intremap tables", 0);
-
-    return 0;
-}
-
 void amd_iommu_ioapic_update_ire(
     unsigned int apic, unsigned int reg, unsigned int value)
 {
@@ -982,7 +894,7 @@ static int dump_intremap_mapping(const struct amd_iommu *iommu,
     return 0;
 }
 
-static void dump_intremap_tables(unsigned char key)
+void amd_iommu_dump_intremap_tables(unsigned char key)
 {
     if ( !shared_intremap_table )
     {
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index 59905629e1..2cf528e760 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -21,6 +21,7 @@
 #include <xsm/xsm.h>
 
 #include <asm/hvm/io.h>
+#include <asm/io_apic.h>
 #include <asm/setup.h>
 
 const struct iommu_init_ops *__initdata iommu_init_ops;
@@ -28,6 +29,7 @@ struct iommu_ops __read_mostly iommu_ops;
 
 int __init iommu_hardware_setup(void)
 {
+    struct IO_APIC_route_entry **ioapic_entries = NULL;
     int rc;
 
     if ( !iommu_init_ops )
@@ -43,7 +45,37 @@ int __init iommu_hardware_setup(void)
         /* x2apic setup may have previously initialised the struct. */
         ASSERT(iommu_ops.init == iommu_init_ops->ops->init);
 
-    return iommu_init_ops->setup();
+    if ( !x2apic_enabled && iommu_intremap )
+    {
+        /*
+         * If x2APIC is enabled interrupt remapping is already enabled, so
+         * there's no need to mess with the IO-APIC because the remapping
+         * entries are already correctly setup by x2apic_bsp_setup.
+         */
+        ioapic_entries = alloc_ioapic_entries();
+        if ( !ioapic_entries )
+            return -ENOMEM;
+        rc = save_IO_APIC_setup(ioapic_entries);
+        if ( rc )
+        {
+            free_ioapic_entries(ioapic_entries);
+            return rc;
+        }
+
+        mask_8259A();
+        mask_IO_APIC_setup(ioapic_entries);
+    }
+
+    rc = iommu_init_ops->setup();
+
+    if ( ioapic_entries )
+    {
+        restore_IO_APIC_setup(ioapic_entries, rc);
+        unmask_8259A();
+        free_ioapic_entries(ioapic_entries);
+    }
+
+    return rc;
 }
 
 int iommu_enable_x2apic(void)
diff --git a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
index 07d25a585d..8ed9482791 100644
--- a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
+++ b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
@@ -114,6 +114,7 @@ int amd_iommu_msi_msg_update_ire(
 void amd_iommu_read_msi_from_ire(
     struct msi_desc *msi_desc, struct msi_msg *msg);
 int amd_setup_hpet_msi(struct msi_desc *msi_desc);
+void amd_iommu_dump_intremap_tables(unsigned char key);
 
 extern struct ioapic_sbdf {
     u16 bdf, seg;
-- 
2.23.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/2] iommu: translate IO-APIC pins when enabling interrupt remapping

Posted by Jan Beulich 5 weeks ago
On 10.10.2019 13:33, Roger Pau Monne wrote:
> On Intel hardware there's currently no translation of already enabled
> IO-APIC pins when interrupt remapping is enabled on the IOMMU, hence
> introduce a logic similar to the one used in x2apic_bsp_setup in order
> to save and mask all IO-APIC pins, and then translate and restore them
> after interrupt remapping has been enabled.
> 
> With this change the AMD specific logic to deal with enabled pins
> (amd_iommu_setup_ioapic_remapping) can be removed, thus unifying the
> handling of IO-APIC when enabling interrupt remapping regardless of
> the IOMMU vendor.
> 
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

The actual code change
Reviewed-by: Jan Beulich <jbeulich@suse.com>
but please mention here as well that the ExtInt case continues to be
broken in the AMD case.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel