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

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>


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

>>> 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


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

> 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



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

>>> 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


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

> 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


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

>>> 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


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

> 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

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