Re: [konrad.wilk@oracle.com: [PATCH 1 of 1] Intel VT-D: Don't turn x2APIC if there is a missing DRHD entry for the IOAPIC]

Re: [konrad.wilk@oracle.com: [PATCH 1 of 1] Intel VT-D: Don't turn x2APIC if there is a missing DRHD entry for the IOAPIC]

[Jan Beulich]

>>> Konrad Rzeszutek Wilk  03/09/10 6:34 PM >>>
>
>What are you guys feeling about this patch?

My main question is whether just disabling x2apic in this case is sufficient (i.e. whether instead we shouldn't disable iommu, or at least interrupt remapping, altogether).

Jan

RE: [konrad.wilk@oracle.com: [PATCH 1 of 1] Intel VT-D: Don't turn x2APIC if there is a missing DRHD entry for the IOAPIC]

[Zhang, Xiantao]

Jan Beulich wrote:
>>>> Konrad Rzeszutek Wilk  03/09/10 6:34 PM >>>
>> 
>> What are you guys feeling about this patch?
> 
> My main question is whether just disabling x2apic in this case is
> sufficient (i.e. whether instead we shouldn't disable iommu, or at
> least interrupt remapping, altogether).  

iommu_enable is called after this point, and it also has sanity-check to determine whether enable or disable iommu and its related features. This early check is only used to determine whether to enable x2apic because it must  be enabled at very early time if supported.
Xiantao

Re: [konrad.wilk@oracle.com: [PATCH 1 of 1] Intel VT-D: Don't turn x2APIC if there is a missing DRHD entry for the IOAPIC]

[Konrad Rzeszutek Wilk]

On Wed, Mar 10, 2010 at 04:42:19PM +0800, Zhang, Xiantao wrote:
> Jan Beulich wrote:
> >>>> Konrad Rzeszutek Wilk  03/09/10 6:34 PM >>>
> >> 
> >> What are you guys feeling about this patch?
> > 
> > My main question is whether just disabling x2apic in this case is
> > sufficient (i.e. whether instead we shouldn't disable iommu, or at
> > least interrupt remapping, altogether).  
> 
> iommu_enable is called after this point, and it also has sanity-check to determine whether enable or disable iommu and its related features. This early check is only used to determine whether to enable x2apic because it must  be enabled at very early time if supported.

Exactly. So the x2APIC is turned off here and then later on the
Interrupt Remaping is turned off as well.

RE: [konrad.wilk@oracle.com: [PATCH 1 of 1] Intel VT-D: Don't turn x2APIC if there is a missing DRHD entry for the IOAPIC]

[Cui, Dexuan]

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

Hi Konrad,
Thanks for the patch! (Sorry for the late reply --  I was on leave...)
BTW, the same sanity checking code also appears in init_vtd_hw(). Maybe we could write a function for the check to remove the duplication in future. :-)

Thanks,
-- Dexuan 

-----Original Message-----
From: xen-devel-bounces@lists.xensource.com [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Konrad Rzeszutek Wilk
Sent: 2010年3月10日 23:38
To: Zhang, Xiantao
Cc: xen-devel@lists.xensource.com
Subject: Re: [konrad.wilk@oracle.com: [Xen-devel] [PATCH 1 of 1] Intel VT-D: Don't turn x2APIC if there is a missing DRHD entry for the IOAPIC]

On Wed, Mar 10, 2010 at 04:42:19PM +0800, Zhang, Xiantao wrote:
> Jan Beulich wrote:
> >>>> Konrad Rzeszutek Wilk  03/09/10 6:34 PM >>>
> >> 
> >> What are you guys feeling about this patch?
> > 
> > My main question is whether just disabling x2apic in this case is
> > sufficient (i.e. whether instead we shouldn't disable iommu, or at
> > least interrupt remapping, altogether).  
> 
> iommu_enable is called after this point, and it also has sanity-check to determine whether enable or disable iommu and its related features. This early check is only used to determine whether to enable x2apic because it must  be enabled at very early time if supported.

Exactly. So the x2APIC is turned off here and then later on the
Interrupt Remaping is turned off as well.

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

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

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

[konrad.wilk@oracle.com: [PATCH 1 of 1] Intel VT-D: Don't turn x2APIC if there is a missing DRHD entry for the IOAPIC]

[Konrad Rzeszutek Wilk]

What are you guys feeling about this patch?

----- Forwarded message from Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> -----

Date: Tue, 09 Mar 2010 12:04:01 -0500
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
To: xen-devel@lists.xensource.com, keir.fraser@eu.citrix.com
Cc: konrad.wilk@oracle.com
Subject: [Xen-devel] [PATCH 1 of 1] Intel VT-D: Don't turn x2APIC if there
	is a missing DRHD entry for the IOAPIC

# 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 )
+		return 0;
+    }
     for_each_drhd_unit ( drhd )
         if ( !ecap_queued_inval(drhd->ecap) ||
              !ecap_intr_remap(drhd->ecap) ||



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

----- End forwarded message -----