Xen devel - Mar 2013 - [PATCH 0/3] IOMMU errata treatment adjustments

1: IOMMU: properly check whether interrupt remapping is enabled
2: AMD IOMMU: only disable when certain IVRS consistency checks fail
3: VT-d: deal with 5500/5520/X58 errata

Patch 1 and 2 are version 2 of a previously submitted, then
withdrawn patch following up after XSA-36. Patch 3 is version 3 of
a patch previously sent by Malcolm and Andrew.

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

... rather than the IOMMU as a whole.

That in turn required to make sure iommu_intremap gets properly
cleared when the respective initialization fails (or isn''t being
done at all).

Along with making sure interrupt remapping doesn''t get inconsistently
enabled on some IOMMUs and not on others in the VT-d code, this in turn
allowed quite a bit of cleanup on the VT-d side (if desired, that
cleanup could of course be broken out into a separate patch).

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

--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -210,7 +210,7 @@ static void read_msi_msg(struct msi_desc
         BUG();
     }
 
-    if ( iommu_enabled )
+    if ( iommu_intremap )
         iommu_read_msi_from_ire(entry, msg);
 }
 
@@ -218,7 +218,7 @@ static void write_msi_msg(struct msi_des
 {
     entry->msg = *msg;
 
-    if ( iommu_enabled )
+    if ( iommu_intremap )
     {
         ASSERT(msg != &entry->msg);
         iommu_update_ire_from_msi(entry, msg);
@@ -492,7 +492,7 @@ int msi_free_irq(struct msi_desc *entry)
     }
 
     /* Free the unused IRTE if intr remap enabled */
-    if ( iommu_enabled )
+    if ( iommu_intremap )
         iommu_update_ire_from_msi(entry, NULL);
 
     list_del(&entry->list);
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -470,6 +470,8 @@ int __init iommu_setup(void)
         rc = iommu_hardware_setup();
         iommu_enabled = (rc == 0);
     }
+    if ( !iommu_enabled )
+        iommu_intremap = 0;
 
     if ( (force_iommu && !iommu_enabled) ||
          (force_intremap && !iommu_intremap) )
@@ -484,9 +486,12 @@ int __init iommu_setup(void)
     }
     printk("I/O virtualisation %sabled\n", iommu_enabled ?
"en" : "dis");
     if ( iommu_enabled )
+    {
         printk(" - Dom0 mode: %s\n",
                iommu_passthrough ? "Passthrough" :
                iommu_dom0_strict ? "Strict" : "Relaxed");
+        printk("Interrupt remapping %sabled\n", iommu_intremap ?
"en" : "dis");
+    }
 
     return rc;
 }
--- a/xen/drivers/passthrough/vtd/intremap.c
+++ b/xen/drivers/passthrough/vtd/intremap.c
@@ -373,7 +373,7 @@ unsigned int io_apic_read_remap_rte(
     struct iommu *iommu = ioapic_to_iommu(IO_APIC_ID(apic));
     struct ir_ctrl *ir_ctrl = iommu_ir_ctrl(iommu);
 
-    if ( !ir_ctrl || !ir_ctrl->iremap_maddr || !ir_ctrl->iremap_num ||
+    if ( !ir_ctrl->iremap_num ||
         ( (index = apic_pin_2_ir_idx[apic][ioapic_pin]) < 0 ) )
         return __io_apic_read(apic, reg);
 
@@ -396,15 +396,8 @@ void io_apic_write_remap_rte(
     struct IO_APIC_route_remap_entry *remap_rte;
     unsigned int rte_upper = (reg & 1) ? 1 : 0;
     struct iommu *iommu = ioapic_to_iommu(IO_APIC_ID(apic));
-    struct ir_ctrl *ir_ctrl = iommu_ir_ctrl(iommu);
     int saved_mask;
 
-    if ( !ir_ctrl || !ir_ctrl->iremap_maddr )
-    {
-        __io_apic_write(apic, reg, value);
-        return;
-    }
-
     old_rte = __ioapic_read_entry(apic, ioapic_pin, 1);
 
     remap_rte = (struct IO_APIC_route_remap_entry *) &old_rte;
@@ -653,20 +646,11 @@ void msi_msg_read_remap_rte(
 {
     struct pci_dev *pdev = msi_desc->dev;
     struct acpi_drhd_unit *drhd = NULL;
-    struct iommu *iommu = NULL;
-    struct ir_ctrl *ir_ctrl;
 
     drhd = pdev ? acpi_find_matched_drhd_unit(pdev)
                 : hpet_to_drhd(msi_desc->hpet_id);
-    if ( !drhd )
-        return;
-    iommu = drhd->iommu;
-
-    ir_ctrl = iommu_ir_ctrl(iommu);
-    if ( !ir_ctrl || !ir_ctrl->iremap_maddr )
-        return;
-
-    remap_entry_to_msi_msg(iommu, msg);
+    if ( drhd )
+        remap_entry_to_msi_msg(drhd->iommu, msg);
 }
 
 void msi_msg_write_remap_rte(
@@ -674,20 +658,11 @@ void msi_msg_write_remap_rte(
 {
     struct pci_dev *pdev = msi_desc->dev;
     struct acpi_drhd_unit *drhd = NULL;
-    struct iommu *iommu = NULL;
-    struct ir_ctrl *ir_ctrl;
 
     drhd = pdev ? acpi_find_matched_drhd_unit(pdev)
                 : hpet_to_drhd(msi_desc->hpet_id);
-    if ( !drhd )
-        return;
-    iommu = drhd->iommu;
-
-    ir_ctrl = iommu_ir_ctrl(iommu);
-    if ( !ir_ctrl || !ir_ctrl->iremap_maddr )
-        return;
-
-    msi_msg_to_remap_entry(iommu, pdev, msi_desc, msg);
+    if ( drhd )
+        msi_msg_to_remap_entry(drhd->iommu, pdev, msi_desc, msg);
 }
 
 int __init intel_setup_hpet_msi(struct msi_desc *msi_desc)
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2065,6 +2065,9 @@ static int init_vtd_hw(void)
                 break;
             }
         }
+        if ( !iommu_intremap )
+            for_each_drhd_unit ( drhd )
+                disable_intremap(drhd->iommu);
     }
 
     /*
--- a/xen/include/asm-x86/io_apic.h
+++ b/xen/include/asm-x86/io_apic.h
@@ -129,7 +129,7 @@ struct IO_APIC_route_entry {
 extern struct mpc_config_ioapic mp_ioapics[MAX_IO_APICS];
 
 /* Only need to remap ioapic RTE (reg: 10~3Fh) */
-#define ioapic_reg_remapped(reg) (iommu_enabled && ((reg) >= 0x10))
+#define ioapic_reg_remapped(reg) (iommu_intremap && ((reg) >= 0x10))
 
 static inline unsigned int __io_apic_read(unsigned int apic, unsigned int reg)
 {




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

After some more thought on the XSA-36 and specifically the comments we
got regarding disabling the IOMMU in this situation altogether making
things worse instead of better, I came to the conclusion that we can
actually restrict the action in affected cases to just disabling
interrupt remapping. That doesn''t make the situation worse than prior
to the XSA-36 fixes (where interrupt remapping didn''t really protect
domains from one another), but allows at least DMA isolation to still
be utilized.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Split off generic and VT-d code changes to a separate, prerequisite
    patch.
    Rather than disabling interrupt remapping here, do the respective
    checks only when interrupt remapping is to be enabled, thus
    allowing "iommu=no-intremap" to be used to enable DMA remapping
    inspite of the errata detected here.

--- a/xen/drivers/passthrough/amd/iommu_acpi.c
+++ b/xen/drivers/passthrough/amd/iommu_acpi.c
@@ -659,6 +659,8 @@ static u16 __init parse_ivhd_device_spec
     switch ( special->variety )
     {
     case ACPI_IVHD_IOAPIC:
+        if ( !iommu_intremap )
+            break;
         /*
          * Some BIOSes have IOAPIC broken entries so we check for IVRS
          * consistency here --- whether entry''s IOAPIC ID is valid and
@@ -921,7 +923,7 @@ static int __init parse_ivrs_table(struc
     }
 
     /* Each IO-APIC must have been mentioned in the table. */
-    for ( apic = 0; !error && apic < nr_ioapics; ++apic )
+    for ( apic = 0; !error && iommu_intremap && apic <
nr_ioapics; ++apic )
     {
         if ( !nr_ioapic_entries[apic] ||
              ioapic_sbdf[IO_APIC_ID(apic)].pin_setup )
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -1194,7 +1194,8 @@ int __init amd_iommu_init(void)
 
     BUG_ON( !iommu_found() );
 
-    if ( amd_iommu_perdev_intremap && amd_sp5100_erratum28() )
+    if ( iommu_intremap && amd_iommu_perdev_intremap &&
+         amd_sp5100_erratum28() )
         goto error_out;
 
     ivrs_bdf_entries = amd_iommu_get_ivrs_dev_entries();
@@ -1211,7 +1212,7 @@ int __init amd_iommu_init(void)
         goto error_out;
 
     /* initialize io-apic interrupt remapping entries */
-    if ( amd_iommu_setup_ioapic_remapping() != 0 )
+    if ( iommu_intremap && amd_iommu_setup_ioapic_remapping() != 0 )
         goto error_out;
 
     /* allocate and initialize a global device table shared by all iommus */





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

http://www.intel.com/content/www/us/en/chipsets/5520-and-5500-chipset-ioh-specification-update.html

Stepping B-3 has two errata (#47 and #53) related to Interrupt
remapping, to which the workaround is for the BIOS to completely disable
interrupt remapping.  These errata are fixed in stepping C-2.

Unfortunately this chipset stepping is very common and many BIOSes are
not disabling interrupt remapping on this stepping .  We can detect this in
Xen and prevent Xen from using the problematic interrupt remapping feature.

The Intel 5500/5520/X58 chipset does not support VT-d
Extended Interrupt Mode(EIM). This means the iommu_supports_eim() check
always fails and so x2apic mode cannot be enabled in Xen before this quirk
disables the interrupt remapping feature.

Signed-off-by: Malcolm Crossley <malcolm.crossley@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Gate the function call to check the quirk on interrupt remapping being
requested to get enabled, and upon failure disable the IOMMU to be in
line with what the changes for XSA-36 (plus follow-ups) did.

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

--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2125,6 +2125,11 @@ int __init intel_vtd_setup(void)
     }
 
     platform_quirks_init();
+    if ( !iommu_enable )
+    {
+        ret = -ENODEV;
+        goto error;
+    }
 
     /* We enable the following features only if they are supported by all VT-d
      * engines: Snoop Control, DMA passthrough, Queued Invalidation and
--- a/xen/drivers/passthrough/vtd/quirks.c
+++ b/xen/drivers/passthrough/vtd/quirks.c
@@ -244,6 +244,29 @@ void vtd_ops_postamble_quirk(struct iomm
     }
 }
 
+/* 5500/5520/X58 Chipset Interrupt remapping errata, for stepping B-3.
+ * Fixed in stepping C-2. */
+static void __init tylersburg_intremap_quirk(void)
+{
+    uint32_t bus, device;
+    uint8_t rev;
+
+    for ( bus = 0; bus < 0x100; bus++ )
+    {
+        /* Match on System Management Registers on Device 20 Function 0 */
+        device = pci_conf_read32(0, bus, 20, 0, PCI_VENDOR_ID);
+        rev = pci_conf_read8(0, bus, 20, 0, PCI_REVISION_ID);
+
+        if ( rev == 0x13 && device == 0x342e8086 )
+        {
+            printk(XENLOG_WARNING VTDPREFIX
+                   "Disabling IOMMU due to Intel 5500/5520/X58 Chipset
errata #47, #53\n");
+            iommu_enable = 0;
+            break;
+        }
+    }
+}
+
 /* initialize platform identification flags */
 void __init platform_quirks_init(void)
 {
@@ -264,6 +287,10 @@ void __init platform_quirks_init(void)
 
     /* ioremap IGD MMIO+0x2000 page */
     map_igd_reg();
+
+    /* Tylersburg interrupt remap quirk */
+    if ( iommu_intremap )
+        tylersburg_intremap_quirk();
 }
 
 /*





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

>>> On 19.03.13 at 17:19, "Jan Beulich"
<JBeulich@suse.com> wrote:
> 1: IOMMU: properly check whether interrupt remapping is enabled
> 2: AMD IOMMU: only disable when certain IVRS consistency checks fail
> 3: VT-d: deal with 5500/5520/X58 errata
> 
> Patch 1 and 2 are version 2 of a previously submitted, then
> withdrawn patch following up after XSA-36. Patch 3 is version 3 of
> a patch previously sent by Malcolm and Andrew.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Maintainers of that code - any opinion? ACKs? NAKs?

Thanks, Jan

Sorry for late.   They make sense to me.  Acked!   Thanks!
Xiantao
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Monday, March 25, 2013 6:18 PM
> To: Jacob Shin; suravee.suthikulpanit@amd.com; Zhang, Xiantao
> Cc: Andrew Cooper; Malcolm Crossley; xen-devel
> Subject: Ping: [PATCH 0/3] IOMMU errata treatment adjustments
> 
> >>> On 19.03.13 at 17:19, "Jan Beulich"
<JBeulich@suse.com> wrote:
> > 1: IOMMU: properly check whether interrupt remapping is enabled
> > 2: AMD IOMMU: only disable when certain IVRS consistency checks fail
> > 3: VT-d: deal with 5500/5520/X58 errata
> >
> > Patch 1 and 2 are version 2 of a previously submitted, then
> > withdrawn patch following up after XSA-36. Patch 3 is version 3 of
> > a patch previously sent by Malcolm and Andrew.
> >
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Maintainers of that code - any opinion? ACKs? NAKs?
> 
> Thanks, Jan

Sorry for late reply.  This works looks ok and works fine.  Acked.

Thank you

Suravee
On 3/25/2013 6:11 AM, Zhang, Xiantao wrote:
> Sorry for late.   They make sense to me.  Acked!   Thanks!
> Xiantao
>
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Monday, March 25, 2013 6:18 PM
>> To: Jacob Shin; suravee.suthikulpanit@amd.com; Zhang, Xiantao
>> Cc: Andrew Cooper; Malcolm Crossley; xen-devel
>> Subject: Ping: [PATCH 0/3] IOMMU errata treatment adjustments
>>
>>>>> On 19.03.13 at 17:19, "Jan Beulich"
<JBeulich@suse.com> wrote:
>>> 1: IOMMU: properly check whether interrupt remapping is enabled
>>> 2: AMD IOMMU: only disable when certain IVRS consistency checks
fail
>>> 3: VT-d: deal with 5500/5520/X58 errata
>>>
>>> Patch 1 and 2 are version 2 of a previously submitted, then
>>> withdrawn patch following up after XSA-36. Patch 3 is version 3 of
>>> a patch previously sent by Malcolm and Andrew.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Maintainers of that code - any opinion? ACKs? NAKs?
>>
>> Thanks, Jan
>

Looks fine, Acked! Thanks! 
Xiantao
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, March 20, 2013 12:48 AM
> To: xen-devel
> Cc: Jacob Shin; suravee.suthikulpanit@amd.com; Andrew Cooper; Malcolm
> Crossley; Zhang, Xiantao
> Subject: [PATCH 3/3] VT-d: deal with 5500/5520/X58 errata
> 
> http://www.intel.com/content/www/us/en/chipsets/5520-and-5500-chipset-ioh-
> specification-update.html
> 
> Stepping B-3 has two errata (#47 and #53) related to Interrupt
> remapping, to which the workaround is for the BIOS to completely disable
> interrupt remapping.  These errata are fixed in stepping C-2.
> 
> Unfortunately this chipset stepping is very common and many BIOSes are
> not disabling interrupt remapping on this stepping .  We can detect this in
> Xen and prevent Xen from using the problematic interrupt remapping feature.
> 
> The Intel 5500/5520/X58 chipset does not support VT-d
> Extended Interrupt Mode(EIM). This means the iommu_supports_eim() check
> always fails and so x2apic mode cannot be enabled in Xen before this quirk
> disables the interrupt remapping feature.
> 
> Signed-off-by: Malcolm Crossley <malcolm.crossley@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Gate the function call to check the quirk on interrupt remapping being
> requested to get enabled, and upon failure disable the IOMMU to be in
> line with what the changes for XSA-36 (plus follow-ups) did.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -2125,6 +2125,11 @@ int __init intel_vtd_setup(void)
>      }
> 
>      platform_quirks_init();
> +    if ( !iommu_enable )
> +    {
> +        ret = -ENODEV;
> +        goto error;
> +    }
> 
>      /* We enable the following features only if they are supported by all
VT-d
>       * engines: Snoop Control, DMA passthrough, Queued Invalidation and
> --- a/xen/drivers/passthrough/vtd/quirks.c
> +++ b/xen/drivers/passthrough/vtd/quirks.c
> @@ -244,6 +244,29 @@ void vtd_ops_postamble_quirk(struct iomm
>      }
>  }
> 
> +/* 5500/5520/X58 Chipset Interrupt remapping errata, for stepping B-3.
> + * Fixed in stepping C-2. */
> +static void __init tylersburg_intremap_quirk(void)
> +{
> +    uint32_t bus, device;
> +    uint8_t rev;
> +
> +    for ( bus = 0; bus < 0x100; bus++ )
> +    {
> +        /* Match on System Management Registers on Device 20 Function 0 */
> +        device = pci_conf_read32(0, bus, 20, 0, PCI_VENDOR_ID);
> +        rev = pci_conf_read8(0, bus, 20, 0, PCI_REVISION_ID);
> +
> +        if ( rev == 0x13 && device == 0x342e8086 )
> +        {
> +            printk(XENLOG_WARNING VTDPREFIX
> +                   "Disabling IOMMU due to Intel 5500/5520/X58
Chipset errata #47,
> #53\n");
> +            iommu_enable = 0;
> +            break;
> +        }
> +    }
> +}
> +
>  /* initialize platform identification flags */
>  void __init platform_quirks_init(void)
>  {
> @@ -264,6 +287,10 @@ void __init platform_quirks_init(void)
> 
>      /* ioremap IGD MMIO+0x2000 page */
>      map_igd_reg();
> +
> +    /* Tylersburg interrupt remap quirk */
> +    if ( iommu_intremap )
> +        tylersburg_intremap_quirk();
>  }
> 
>  /*
> 
>