RE: [Xen-devel] Problem: Pattern with vertical colored lines on the dom0 screen

["Jan Beulich" <JBeulich@xxxxxxxxxx>]

>>> On 30.09.10 at 01:14, "Kay, Allen M" <allen.m.kay@xxxxxxxxx> wrote:
> Attached patch fixes dom0 graphics problem on Levnovo T410 when VT-d is 
> enabled.  The patch is derived from a similar quirk in Linux kernel by David 
> Woodhouse and Adam Jackson.  It checks for VT enabling bit in IGD GGC 
> register.  If VT is not enabled correctly in the IGD, Xen does not enable 
> VT-d 
> translation for IGD VT-d engine.  In case where iommu boot parameter is set 
> to 
> force, Xen calls panic().
> 
> Signed-off-by: Allen Kay allen.m.kay@xxxxxxxxx 

Hmm, I had hoped this patch wouldn't get applied as-is, but I just
saw it's in the staging tree already. It seems more like a hack than
a real (permanent) solution.

>@@ -333,6 +340,8 @@
>             if ( iommu_verbose )
>                 dprintk(VTDPREFIX, "  endpoint: %x:%x.%x\n",
>                         bus, path->dev, path->fn);
>+            if ( (bus == 0) && (path->dev == 2) && (path->fn == 0) )
>+                *igd = 1;
>             break;
> 
>         case ACPI_DEV_IOAPIC:

The use of magic numbers (0, 2, and 0) here looks like being
tailored to just a very limited set of systems. If it was really
known that all current and future systems are going to have this
arrangement, a comment saying so would be the minimum I'd
expect.

Also, if a check of igd against NULL or a check of type against
DMAR_TYPE was added here, the two call sites that don't really
need this output could simply pass NULL instead of adding a new
local variable.

>@@ -689,10 +689,34 @@
>     return 0;
> }
> 
>-static void iommu_enable_translation(struct iommu *iommu)
>+#define GGC 0x52
>+#define GGC_MEMORY_VT_ENABLED  (0x8 << 8)
>+static int is_igd_vt_enabled(void)
>+{
>+    unsigned short ggc;
>+
>+    /* integrated graphics on Intel platforms is located at 0:2.0 */
>+    ggc = pci_conf_read16(0, 2, 0, GGC);
>+    return ( ggc & GGC_MEMORY_VT_ENABLED ? 1 : 0 );

Similarly here (a brief comment is there, but to me this doesn't
mean it's going to be that way forever).

Also, if this ever needs updating, matching the magic numbers
here and above is going to be a matter of luck. If they are to be
hard coded, they should be #define-s, so that locating all uses
is possible.

>+}
>+
>+static void iommu_enable_translation(struct acpi_drhd_unit *drhd)
> {
>     u32 sts;
>     unsigned long flags;
>+    struct iommu *iommu = drhd->iommu;
>+
>+    if ( !is_igd_vt_enabled() && is_igd_drhd(drhd) ) 

I would also have suggested switching the checks here: The first
involves a PCI config space read (and even unconditional upon
anything), while the second really just is a compare of two
variables.

>+    {
>+        if ( force_iommu )
>+            panic("BIOS did not enable IGD for VT properly, crash Xen for 
>security purpose!\n");
>+        else
>+        {
>+            dprintk(XENLOG_WARNING VTDPREFIX,
>+                    "BIOS did not enable IGD for VT properly.  Disabling IGD 
>VT-d engine.\n");
>+            return;
>+        }
>+    }
> 
>     if ( iommu_verbose )
>         dprintk(VTDPREFIX,

Jan


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