[PATCH][VTD][QUIRK] added quirks for Sandybridge errata workaround, WLAN, VT-d fault escalation

[PATCH][VTD][QUIRK] added quirks for Sandybridge errata workaround, WLAN, VT-d fault escalation

[Kay, Allen M]

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

Adding errata workaround for newly released Sandybridge processor graphics, additional WLAN device ID's for WLAN quirk, a quirk for masking VT-d fault escalation to IOH HW that can cause system hangs on some OEM hardware where the BIOS erroneously escalates VT-d faults to the platform.

Signed-off-by: Allen Kay <allen.m.kay@intel.com>

[-- Attachment #2: snb01133.patch --]
[-- Type: application/octet-stream, Size: 6819 bytes --]

diff -r f1a5ac39c15e xen/drivers/passthrough/vtd/extern.h
--- a/xen/drivers/passthrough/vtd/extern.h	Thu Jan 13 16:00:59 2011 +0000
+++ b/xen/drivers/passthrough/vtd/extern.h	Fri Sep 17 09:29:45 2010 -0700
@@ -86,5 +86,6 @@ void vtd_ops_preamble_quirk(struct iommu
 void vtd_ops_preamble_quirk(struct iommu* iommu);
 void vtd_ops_postamble_quirk(struct iommu* iommu);
 void me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map);
+void pci_vtd_quirk(struct pci_dev *pdev);
 
 #endif // _VTD_EXTERN_H_
diff -r f1a5ac39c15e xen/drivers/passthrough/vtd/iommu.c
--- a/xen/drivers/passthrough/vtd/iommu.c	Thu Jan 13 16:00:59 2011 +0000
+++ b/xen/drivers/passthrough/vtd/iommu.c	Fri Sep 17 09:29:45 2010 -0700
@@ -1910,6 +1910,7 @@ static void __init setup_dom0_devices(st
             list_add(&pdev->domain_list, &d->arch.pdev_list);
             domain_context_mapping(d, pdev->bus, pdev->devfn);
             pci_enable_acs(pdev);
+            pci_vtd_quirk(pdev);
         }
     }
     spin_unlock(&pcidevs_lock);
diff -r f1a5ac39c15e xen/drivers/passthrough/vtd/quirks.c
--- a/xen/drivers/passthrough/vtd/quirks.c	Thu Jan 13 16:00:59 2011 +0000
+++ b/xen/drivers/passthrough/vtd/quirks.c	Fri Sep 17 09:29:45 2010 -0700
@@ -47,11 +47,13 @@
 #define IS_CTG(id)    (id == 0x2a408086)
 #define IS_ILK(id)    (id == 0x00408086 || id == 0x00448086 || id== 0x00628086 || id == 0x006A8086)
 #define IS_CPT(id)    (id == 0x01008086 || id == 0x01048086)
+#define IS_SNB_GFX(id) (id == 0x01068086 || id == 0x01168086 || id == 0x01268086 || id == 0x01028086 || id == 0x01128086 || id == 0x01228086 || id == 0x010A8086)
 
 u32 ioh_id;
 u32 igd_id;
 bool_t rwbf_quirk;
 static int is_cantiga_b3;
+static int is_snb_gfx;
 static u8 *igd_reg_va;
 
 /*
@@ -92,6 +94,12 @@ static void cantiga_b3_errata_init(void)
         is_cantiga_b3 = 1;
 }
 
+/* check for Sandybridge IGD device ID's */
+static void snb_errata_init(void)
+{
+    is_snb_gfx = IS_SNB_GFX(igd_id);
+}
+
 /*
  * QUIRK to workaround Cantiga IGD VT-d low power errata.
  * This errata impacts IGD assignment on Cantiga systems
@@ -104,12 +112,15 @@ static void cantiga_b3_errata_init(void)
 /*
  * map IGD MMIO+0x2000 page to allow Xen access to IGD 3D register.
  */
-static void map_igd_reg(void)
+static void*  map_igd_reg(void)
 {
     u64 igd_mmio, igd_reg;
 
-    if ( !is_cantiga_b3 || igd_reg_va != NULL )
-        return;
+    if ( !is_cantiga_b3 && !is_snb_gfx )
+        return NULL;
+
+    if ( igd_reg_va )
+        return igd_reg_va;
 
     /* get IGD mmio address in PCI BAR */
     igd_mmio = ((u64)pci_conf_read32(0, IGD_DEV, 0, 0x14) << 32) +
@@ -125,6 +136,7 @@ static void map_igd_reg(void)
 #else
     igd_reg_va = ioremap_nocache(igd_reg, 0x100);
 #endif
+    return igd_reg_va;
 }
 
 /*
@@ -138,6 +150,9 @@ static int cantiga_vtd_ops_preamble(stru
     if ( !is_igd_drhd(drhd) || !is_cantiga_b3 )
         return 0;
 
+    if ( !map_igd_reg() )
+        return 0;
+
     /*
      * read IGD register at IGD MMIO + 0x20A4 to force IGD
      * to exit low power state.  Since map_igd_reg()
@@ -148,11 +163,64 @@ static int cantiga_vtd_ops_preamble(stru
 }
 
 /*
+ * Sandybridge RC6 power management inhibit state erratum.
+ * This can cause power high power consumption.
+ * Workaround is to prevent graphics get into RC6
+ * state when doing VT-d IOTLB operations, do the VT-d
+ * IOTLB operation, and then re-enable RC6 state.
+ */
+static void snb_vtd_ops_preamble(struct iommu* iommu)
+{
+    struct intel_iommu *intel = iommu->intel;
+    struct acpi_drhd_unit *drhd = intel ? intel->drhd : NULL;
+    s_time_t start_time;
+
+    if ( !is_igd_drhd(drhd) || !is_snb_gfx )
+        return;
+
+    if ( !map_igd_reg() )
+        return;
+
+    *((volatile u32 *)(igd_reg_va + 0x54)) = 0x000FFFFF;
+    *((volatile u32 *)(igd_reg_va + 0x700)) = 0;
+
+    start_time = NOW();
+    while ( (*((volatile u32 *)(igd_reg_va + 0x2AC)) & 0xF) != 0 )
+    {
+        if ( NOW() > start_time + DMAR_OPERATION_TIMEOUT )
+        {
+            dprintk(XENLOG_INFO VTDPREFIX,
+                    "snb_vtd_ops_preamble: failed to disable idle handshake\n");
+            break;
+        }
+        cpu_relax();
+    }
+
+    *((volatile u32*)(igd_reg_va + 0x50)) = 0x10001;
+}
+
+static void snb_vtd_ops_postamble(struct iommu* iommu)
+{
+    struct intel_iommu *intel = iommu->intel;
+    struct acpi_drhd_unit *drhd = intel ? intel->drhd : NULL;
+
+    if ( !is_igd_drhd(drhd) || !is_snb_gfx )
+        return;
+
+    if ( !map_igd_reg() )
+        return;
+
+    *((volatile u32 *)(igd_reg_va + 0x54)) = 0xA;
+    *((volatile u32 *)(igd_reg_va + 0x50)) = 0x10000;
+}
+
+/*
  * call before VT-d translation enable and IOTLB flush operations.
  */
 void vtd_ops_preamble_quirk(struct iommu* iommu)
 {
     cantiga_vtd_ops_preamble(iommu);
+    snb_vtd_ops_preamble(iommu);
 }
 
 /*
@@ -160,7 +228,7 @@ void vtd_ops_preamble_quirk(struct iommu
  */
 void vtd_ops_postamble_quirk(struct iommu* iommu)
 {
-    return;
+    snb_vtd_ops_postamble(iommu);
 }
 
 /* initialize platform identification flags */
@@ -178,6 +246,8 @@ void __init platform_quirks_init(void)
 
     /* initialize cantiga B3 identification */
     cantiga_b3_errata_init();
+
+    snb_errata_init();
 
     /* ioremap IGD MMIO+0x2000 page */
     map_igd_reg();
@@ -250,11 +320,14 @@ void me_wifi_quirk(struct domain *domain
         id = pci_conf_read32(bus, PCI_SLOT(devfn), PCI_FUNC(devfn), 0);
         switch (id)
         {
-            case 0x00878086:
+            case 0x00878086:        /* Kilmer Peak */
             case 0x00898086:
-            case 0x00828086:
+            case 0x00828086:        /* Taylor Peak */
             case 0x00858086:
-            case 0x42388086:
+            case 0x008F8086:        /* Rainbow Peak */
+            case 0x00908086:
+            case 0x00918086:
+            case 0x42388086:        /* Puma Peak */
             case 0x422b8086:
             case 0x422c8086:
                 map_me_phantom_function(domain, 22, map);
@@ -262,6 +335,26 @@ void me_wifi_quirk(struct domain *domain
             default:
                 break;
         }
-
     }
 }
+
+/*
+ * Mask reporting Intel VT-d faults to IOH core logic:
+ *   - Some platform escalates VT-d faults to platform errors 
+ *   - This can cause system failure upon non-fatal VT-d faults
+ *   - Potential security issue if malicious guest trigger VT-d faults
+ */
+void pci_vtd_quirk(struct pci_dev *pdev)
+{
+    int bus = pdev->bus;
+    int dev = PCI_SLOT(pdev->devfn);
+    int func = PCI_FUNC(pdev->devfn);
+    int id, val;
+
+    id = pci_conf_read32(bus, dev, func, 0);
+    if ( id == 0x342e8086 || id == 0x3c288086 )
+    {
+        val = pci_conf_read32(bus, dev, func, 0x1AC);
+        pci_conf_write32(bus, dev, func, 0x1AC, val | (1 << 31));
+    }
+}

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH][VTD][QUIRK] added quirks for Sandybridge errata workaround, WLAN, VT-d fault escalation

[Jan Beulich]

>>> On 14.01.11 at 00:40, "Kay, Allen M" <allen.m.kay@intel.com> wrote:
>+static void snb_vtd_ops_preamble(struct iommu* iommu)
>+{
>+    struct intel_iommu *intel = iommu->intel;
>+    struct acpi_drhd_unit *drhd = intel ? intel->drhd : NULL;
>+    s_time_t start_time;
>+
>+    if ( !is_igd_drhd(drhd) || !is_snb_gfx )
>+        return;
>+
>+    if ( !map_igd_reg() )
>+        return;
>+
>+    *((volatile u32 *)(igd_reg_va + 0x54)) = 0x000FFFFF;
>+    *((volatile u32 *)(igd_reg_va + 0x700)) = 0;
>+
>+    start_time = NOW();
>+    while ( (*((volatile u32 *)(igd_reg_va + 0x2AC)) & 0xF) != 0 )
>+    {
>+        if ( NOW() > start_time + DMAR_OPERATION_TIMEOUT )
>+        {
>+            dprintk(XENLOG_INFO VTDPREFIX,
>+                    "snb_vtd_ops_preamble: failed to disable idle handshake\n");
>+            break;
>+        }
>+        cpu_relax();
>+    }
>+
>+    *((volatile u32*)(igd_reg_va + 0x50)) = 0x10001;
>+}
>+
>+static void snb_vtd_ops_postamble(struct iommu* iommu)
>+{
>+    struct intel_iommu *intel = iommu->intel;
>+    struct acpi_drhd_unit *drhd = intel ? intel->drhd : NULL;
>+
>+    if ( !is_igd_drhd(drhd) || !is_snb_gfx )
>+        return;
>+
>+    if ( !map_igd_reg() )
>+        return;
>+
>+    *((volatile u32 *)(igd_reg_va + 0x54)) = 0xA;
>+    *((volatile u32 *)(igd_reg_va + 0x50)) = 0x10000;
>+}

Isn't there a risk that these MMIO writes interfere with the
operation of the actual driver running in a domain?

And even just in Xen itself, how do these writes get
synchronized? Callers of vtd_ops_preamble_quirk() don't
appear to be required to hold any particular lock.

Jan

RE: [PATCH][VTD][QUIRK] added quirks for Sandybridge errata workaround, WLAN, VT-d fault escalation

[Kay, Allen M]

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

> Isn't there a risk that these MMIO writes interfere with the
> operation of the actual driver running in a domain?

I have checked drivers/gpu/drm/i915/i915_reg.h in the kernel and I don't see any usage of these MMIO registers.  Do you think we should add a boot switch to allow optionally turn off these workarounds just in case?  If so, what default value should it be?

> And even just in Xen itself, how do these writes get
> synchronized? Callers of vtd_ops_preamble_quirk() don't
> appear to be required to hold any particular lock.

I have added a igd_lock in the attached patch.  Can you take a look?

Allen


[-- Attachment #2: lock.patch --]
[-- Type: application/octet-stream, Size: 1535 bytes --]

diff -r 75b6287626ee xen/drivers/passthrough/vtd/quirks.c
--- a/xen/drivers/passthrough/vtd/quirks.c	Fri Jan 14 16:38:51 2011 +0000
+++ b/xen/drivers/passthrough/vtd/quirks.c	Wed Sep 22 09:57:26 2010 -0700
@@ -29,6 +29,7 @@
 #include <xen/pci.h>
 #include <xen/pci_regs.h>
 #include <xen/keyhandler.h>
+#include <xen/spinlock.h>
 #include <asm/msi.h>
 #include <asm/irq.h>
 #include <mach_apic.h>
@@ -55,6 +56,7 @@ static int is_cantiga_b3;
 static int is_cantiga_b3;
 static int is_snb_gfx;
 static u8 *igd_reg_va;
+static spinlock_t igd_lock;
 
 /*
  * QUIRK to workaround Xen boot issue on Calpella/Ironlake OEM BIOS
@@ -136,6 +138,7 @@ static void *map_igd_reg(void)
 #else
     igd_reg_va = ioremap_nocache(igd_reg, 0x100);
 #endif
+    spin_lock_init(&igd_lock);
     return igd_reg_va;
 }
 
@@ -181,6 +184,7 @@ static void snb_vtd_ops_preamble(struct 
     if ( !map_igd_reg() )
         return;
 
+    spin_lock(&igd_lock);
     *((volatile u32 *)(igd_reg_va + 0x54)) = 0x000FFFFF;
     *((volatile u32 *)(igd_reg_va + 0x700)) = 0;
 
@@ -197,6 +201,7 @@ static void snb_vtd_ops_preamble(struct 
     }
 
     *((volatile u32*)(igd_reg_va + 0x50)) = 0x10001;
+    spin_unlock(&igd_lock);
 }
 
 static void snb_vtd_ops_postamble(struct iommu* iommu)
@@ -210,8 +215,10 @@ static void snb_vtd_ops_postamble(struct
     if ( !map_igd_reg() )
         return;
 
+    spin_lock(&igd_lock);
     *((volatile u32 *)(igd_reg_va + 0x54)) = 0xA;
     *((volatile u32 *)(igd_reg_va + 0x50)) = 0x10000;
+    spin_unlock(&igd_lock);
 }
 
 /*

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

RE: [PATCH][VTD][QUIRK] added quirks for Sandybridge errata workaround, WLAN, VT-d fault escalation

[Jan Beulich]

>>> On 19.01.11 at 01:12, "Kay, Allen M" <allen.m.kay@intel.com> wrote:
>>  Isn't there a risk that these MMIO writes interfere with the
>> operation of the actual driver running in a domain?
> 
> I have checked drivers/gpu/drm/i915/i915_reg.h in the kernel and I don't see 
> any usage of these MMIO registers.  Do you think we should add a boot switch 
> to allow optionally turn off these workarounds just in case?  If so, what 
> default value should it be?

Not sure. A fundamental question is whether these registers could
ever be of use to a driver (and if they wouldn't, I would wonder why
they're there).

One thing that might help at least detect possible collisions could be
to read the state prior to writing it in the preamble code, and check
whether it matches expectations (and perhaps force to known
values during initialization).

>> And even just in Xen itself, how do these writes get
>> synchronized? Callers of vtd_ops_preamble_quirk() don't
>> appear to be required to hold any particular lock.
> 
> I have added a igd_lock in the attached patch.  Can you take a look?

That doesn't really look sufficient. You could still have multiple CPUs
going through these code paths simultaneously, and e.g. CPU A
executing snb_vtd_ops_postamble() while CPU B still wants the
result of snb_vtd_ops_preamble() in effect.

To me it would seem more logical to add a usage counter: When
transitioning from zero, suppress RC6, and when transitioning to
zero, re-enable RC6 (whatever this is). If what gets written in
the preamble is some sort of counter the hardware decrements,
you would need to extend the period each time execution flows
through the preamble.

Jan

RE: [PATCH][VTD][QUIRK] added quirks for Sandybridge errata workaround, WLAN, VT-d fault escalation

[Kay, Allen M]

> To me it would seem more logical to add a usage counter: When
> transitioning from zero, suppress RC6, and when transitioning to
> zero, re-enable RC6 (whatever this is). If what gets written in
> the preamble is some sort of counter the hardware decrements,
> you would need to extend the period each time execution flows
> through the preamble.

I see what you are getting at ...  Another alternative is to simply putting the lock around the "preamble->iotlb_flush->postamble" sequence in iommu.c.  It spills more of the quirk specific code into iommu.c but this should be much simpler logic wise.  What do you think?

Allen

RE: [PATCH][VTD][QUIRK] added quirks for Sandybridge errata workaround, WLAN, VT-d fault escalation

[Jan Beulich]

>>> On 20.01.11 at 00:40, "Kay, Allen M" <allen.m.kay@intel.com> wrote:
>>  To me it would seem more logical to add a usage counter: When
>> transitioning from zero, suppress RC6, and when transitioning to
>> zero, re-enable RC6 (whatever this is). If what gets written in
>> the preamble is some sort of counter the hardware decrements,
>> you would need to extend the period each time execution flows
>> through the preamble.
> 
> I see what you are getting at ...  Another alternative is to simply putting 
> the lock around the "preamble->iotlb_flush->postamble" sequence in iommu.c.  It 
> spills more of the quirk specific code into iommu.c but this should be much 
> simpler logic wise.  What do you think?

Wouldn't that result in excessive lock holding time?

Jan

RE: [PATCH][VTD][QUIRK] added quirks for Sandybridge errata workaround, WLAN, VT-d fault escalation

[Kay, Allen M]

> Wouldn't that result in excessive lock holding time?

It would be a problem for older platforms like Weybridge but Sandybridge has fixed the IGD IOTLB invalidation times.  Although I have not done actual measurement on this yet.

Given this quirks is for a corner case power management issue (not easily duplicated), I will submit a patch to disable it by default until we can answer you about the uses of the MMIO registers and implement the locking mechanism properly.

Allen