From mboxrd@z Thu Jan  1 00:00:00 1970
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Subject: Re: [PATCH 1 of 1] Intel VT-D: Don't turn x2APIC if
	there is a missing DRHD entry for the IOAPIC
Date: Thu, 11 Mar 2010 15:25:44 -0500
Message-ID: <20100311202544.GB22182@phenom.dumpdata.com>
References: <patchbomb.1268154240@phenom.dumpdata.com>
	<81272f761f631d3c0929.1268154241@phenom.dumpdata.com>
	<1268287479.3015.96.camel@2710p.home>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Return-path: <xen-devel-bounces@lists.xensource.com>
Content-Disposition: inline
In-Reply-To: <1268287479.3015.96.camel@2710p.home>
List-Unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xensource.com>
List-Help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-Subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
Sender: xen-devel-bounces@lists.xensource.com
Errors-To: xen-devel-bounces@lists.xensource.com
To: Alex Williamson <alex.williamson@hp.com>
Cc: xen-devel@lists.xensource.com, keir.fraser@eu.citrix.com
List-Id: xen-devel@lists.xenproject.org

On Wed, Mar 10, 2010 at 11:04:39PM -0700, Alex Williamson wrote:
> On Tue, 2010-03-09 at 12:04 -0500, Konrad Rzeszutek Wilk wrote:
> > # HG changeset patch
> > # User Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > # Date 1268154140 18000
> > # Node ID 81272f761f631d3c0929642fc833b66243b7d2bc
> > # Parent  b8d2a4134a6823f6d5179928a0618eaf33be4684
> > Intel VT-D: Don't turn x2APIC if there is a missing DRHD entry for the IOAPIC.
> > 
> > Follow the Linux kernel lead in which the x2APIC is
> > only turned on only if there is an DRHD entry for all
> > IOAPICs in the system. If we don't do this
> > we might enable x2APIC and see various devices not covered by the
> > IOAPIC mentioned in DRHD, not receive any interrupts.
> > 
> > Workaround is to use 'x2apic=0' on command line.
> > 
> > diff -r b8d2a4134a68 -r 81272f761f63 xen/drivers/passthrough/vtd/intremap.c
> > --- a/xen/drivers/passthrough/vtd/intremap.c	Wed Mar 03 17:41:58 2010 +0000
> > +++ b/xen/drivers/passthrough/vtd/intremap.c	Tue Mar 09 12:02:20 2010 -0500
> > @@ -127,10 +127,17 @@
> >  int iommu_supports_eim(void)
> >  {
> >      struct acpi_drhd_unit *drhd;
> > +    int apic;
> >  
> >      if ( !iommu_enabled || !iommu_qinval || !iommu_intremap )
> >          return 0;
> >  
> > +    // We MUST have a DRHD unit for each IOAPIC.
> > +    for ( apic = 0; apic < nr_ioapics; apic++ )
> > +    {
> > +       if ( ioapic_to_iommu(IO_APIC_ID(apic)) == NULL )
> 
> 
> This has a pretty serious bug.  ioapic_to_iommu() gets returned
> drhd->iommu.  However, drhd->iommu isn't allocated until part of

Yikes!

> iommu_setup(), which is called after enable_x2apic().  Has this ever
> worked?  Here's the fix.

Yes. But I only have faulty hardware (ie, the is one IOAPIC in the DRHD,
and three existing IOAPICS), so I couldn't test it on working hardware.

Thank you for spotting this mistake and coming to resuce with a proper
patch.

Much appreciated.
> 
> Signed-off-by: Alex Williamson <alex.williamson@hp.com>
> --
> 
> diff -r 132ac04cbdba xen/drivers/passthrough/vtd/intremap.c
> --- a/xen/drivers/passthrough/vtd/intremap.c	Tue Mar 09 18:18:19 2010 +0000
> +++ b/xen/drivers/passthrough/vtd/intremap.c	Wed Mar 10 22:58:08 2010 -0700
> @@ -134,7 +134,7 @@
>  
>      /* We MUST have a DRHD unit for each IOAPIC. */
>      for ( apic = 0; apic < nr_ioapics; apic++ )
> -        if ( !ioapic_to_iommu(IO_APIC_ID(apic)) )
> +        if ( !ioapic_to_drhd(IO_APIC_ID(apic)) )
>              return 0;
>  
>      if ( list_empty(&acpi_drhd_units) )
>