[patch 2/2] x2apic/intr-remap: decouple interrupt remapping from x2apic

[patch 2/2] x2apic/intr-remap: decouple interrupt remapping from x2apic

[Han, Weidong]

interrupt remapping must be enabled before enabling x2apic, but
interrupt remapping doesn't depend on x2apic, it can be used
separately. Enable interrupt remapping in init_dmars even x2apic
is not supported.

Signed-off-by: Weidong Han <weidong.han@intel.com>
---
 arch/x86/kernel/apic/io_apic.c |   27 ++++++++++++++++++++++-----
 drivers/pci/intel-iommu.c      |    9 +++++++++
 2 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 00e6071..8c09f4c 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -2528,7 +2528,7 @@ static void irq_complete_move(struct irq_desc **descp)
 static inline void irq_complete_move(struct irq_desc **descp) {}
 #endif
 
-#ifdef CONFIG_INTR_REMAP
+#ifdef CONFIG_X86_X2APIC
 static void ack_x2apic_level(unsigned int irq)
 {
 	ack_x2APIC_irq();
@@ -2538,7 +2538,6 @@ static void ack_x2apic_edge(unsigned int irq)
 {
 	ack_x2APIC_irq();
 }
-
 #endif
 
 static void ack_apic_edge(unsigned int irq)
@@ -2663,13 +2662,31 @@ static struct irq_chip ioapic_chip __read_mostly = {
 };
 
 #ifdef CONFIG_INTR_REMAP
+static void ir_ack_apic_edge(unsigned int irq)
+{
+#ifdef CONFIG_X86_X2APIC
+	if (x2apic_enabled())
+		return ack_x2apic_edge(irq);
+#endif
+	return ack_apic_edge(irq);
+}
+
+static void ir_ack_apic_level(unsigned int irq)
+{
+#ifdef CONFIG_X86_X2APIC
+	if (x2apic_enabled())
+		return ack_x2apic_level(irq);
+#endif
+	return ack_apic_level(irq);
+}
+
 static struct irq_chip ir_ioapic_chip __read_mostly = {
 	.name		= "IR-IO-APIC",
 	.startup	= startup_ioapic_irq,
 	.mask		= mask_IO_APIC_irq,
 	.unmask		= unmask_IO_APIC_irq,
-	.ack		= ack_x2apic_edge,
-	.eoi		= ack_x2apic_level,
+	.ack		= ir_ack_apic_edge,
+	.eoi		= ir_ack_apic_level,
 #ifdef CONFIG_SMP
 	.set_affinity	= set_ir_ioapic_affinity_irq,
 #endif
@@ -3399,7 +3416,7 @@ static struct irq_chip msi_ir_chip = {
 	.name		= "IR-PCI-MSI",
 	.unmask		= unmask_msi_irq,
 	.mask		= mask_msi_irq,
-	.ack		= ack_x2apic_edge,
+	.ack		= ir_ack_apic_edge,
 #ifdef CONFIG_SMP
 	.set_affinity	= ir_set_msi_irq_affinity,
 #endif
diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
index f3f6865..6e48298 100644
--- a/drivers/pci/intel-iommu.c
+++ b/drivers/pci/intel-iommu.c
@@ -2067,6 +2067,15 @@ static int __init init_dmars(void)
 		}
 	}
 
+#ifdef CONFIG_INTR_REMAP
+		if (!intr_remapping_enabled) {
+			ret = enable_intr_remapping(0);
+			if (ret)
+				printk(KERN_ERR
+				"IOMMU: enable interrupt remapping failed\n");
+		}
+#endif
+
 	/*
 	 * For each rmrr
 	 *   for each dev attached to rmrr
-- 
1.6.0.4

Re: [patch 2/2] x2apic/intr-remap: decouple interrupt remapping from x2apic

[Suresh Siddha]

On Fri, 2009-04-03 at 02:15 -0700, Han, Weidong wrote:
>  
> -#ifdef CONFIG_INTR_REMAP
> +#ifdef CONFIG_X86_X2APIC

Weidong, Can we also fix these config options?

Today, selecting CONFIG_INTR_REMAP automatically selects
CONFIG_X86_X2APIC. It should be the other way round.

i.e., turning on CONFIG_X86_X2APIC should automatically enable
CONFIG_INTR_REMAP and just enabling CONFIG_INTR_REMAP shouldn't enable
CONFIG_X86_X2APIC.

Also please make sure that the code compiles (and works :)) in different
combinations of these config settings.

thanks,
suresh

Re: [patch 2/2] x2apic/intr-remap: decouple interrupt remapping from x2apic

[David Woodhouse]

On Fri, 2009-04-03 at 09:57 -0800, Suresh Siddha wrote:
> On Fri, 2009-04-03 at 02:15 -0700, Han, Weidong wrote:
> >  
> > -#ifdef CONFIG_INTR_REMAP
> > +#ifdef CONFIG_X86_X2APIC
> 
> Weidong, Can we also fix these config options?
> 
> Today, selecting CONFIG_INTR_REMAP automatically selects
> CONFIG_X86_X2APIC. It should be the other way round.
> 
> i.e., turning on CONFIG_X86_X2APIC should automatically enable
> CONFIG_INTR_REMAP and just enabling CONFIG_INTR_REMAP shouldn't enable
> CONFIG_X86_X2APIC.
> 
> Also please make sure that the code compiles (and works :)) in different
> combinations of these config settings.

Which should SGI Ultraviolet be selecting?

-- 
dwmw2

Re: [patch 2/2] x2apic/intr-remap: decouple interrupt remapping from x2apic

[Suresh Siddha]

On Fri, 2009-04-03 at 11:08 -0700, David Woodhouse wrote:
> On Fri, 2009-04-03 at 09:57 -0800, Suresh Siddha wrote:
> > On Fri, 2009-04-03 at 02:15 -0700, Han, Weidong wrote:
> > >  
> > > -#ifdef CONFIG_INTR_REMAP
> > > +#ifdef CONFIG_X86_X2APIC
> > 
> > Weidong, Can we also fix these config options?
> > 
> > Today, selecting CONFIG_INTR_REMAP automatically selects
> > CONFIG_X86_X2APIC. It should be the other way round.
> > 
> > i.e., turning on CONFIG_X86_X2APIC should automatically enable
> > CONFIG_INTR_REMAP and just enabling CONFIG_INTR_REMAP shouldn't enable
> > CONFIG_X86_X2APIC.
> > 
> > Also please make sure that the code compiles (and works :)) in different
> > combinations of these config settings.
> 
> Which should SGI Ultraviolet be selecting?

Both X2APIC and INTR_REMAP. They already select X2APIC today. With above
change, they automatically get INTR_REMAP too.

RE: [patch 2/2] x2apic/intr-remap: decouple interrupt remapping from x2apic

[Han, Weidong]

Siddha, Suresh B wrote:
> On Fri, 2009-04-03 at 02:15 -0700, Han, Weidong wrote:
>> 
>> -#ifdef CONFIG_INTR_REMAP
>> +#ifdef CONFIG_X86_X2APIC
> 
> Weidong, Can we also fix these config options?
> 
> Today, selecting CONFIG_INTR_REMAP automatically selects
> CONFIG_X86_X2APIC. It should be the other way round.
> 
> i.e., turning on CONFIG_X86_X2APIC should automatically enable
> CONFIG_INTR_REMAP and just enabling CONFIG_INTR_REMAP shouldn't enable
> CONFIG_X86_X2APIC.
> 
> Also please make sure that the code compiles (and works :)) in
> different combinations of these config settings.


Thanks for your reminder. Following patch changes the config options selection. The code can compiles in different combinations.


Subject: [PATCH] Change X2APIC and INTR_REMAP config selection

Currently, selecting CONFIG_INTR_REMAP automatically selects
CONFIG_X86_X2APIC. It should be the other way round. i.e. selecting
CONFIG_X86_X2APIC automatically enables CONFIG_INTR_REMAP, but
selecting CONFIG_INTR_REMAP won't enable CONFIG_X86_X2APIC.

Signed-off-by: Weidong Han <weidong.han@intel.com>
---
 arch/x86/Kconfig |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 5696cec..0e522a4 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -251,6 +251,7 @@ config SMP
 config X86_X2APIC
 	bool "Support x2apic"
 	depends on X86_LOCAL_APIC && X86_64
+	select INTR_REMAP
 	---help---
 	  This enables x2apic support on CPUs that have this feature.
 
@@ -1879,7 +1880,6 @@ config DMAR_FLOPPY_WA
 config INTR_REMAP
 	bool "Support for Interrupt Remapping (EXPERIMENTAL)"
 	depends on X86_64 && X86_IO_APIC && PCI_MSI && ACPI && EXPERIMENTAL
-	select X86_X2APIC
 	---help---
 	  Supports Interrupt remapping for IO-APIC and MSI devices.
 	  To use x2apic mode in the CPU's which support x2APIC enhancements or
-- 
1.6.0.4

RE: [patch 2/2] x2apic/intr-remap: decouple interrupt remapping from x2apic

[David Woodhouse]

On Sat, 2009-04-04 at 11:37 +0800, Han, Weidong wrote:
> Siddha, Suresh B wrote:
> > On Fri, 2009-04-03 at 02:15 -0700, Han, Weidong wrote:
> >> 
> >> -#ifdef CONFIG_INTR_REMAP
> >> +#ifdef CONFIG_X86_X2APIC
> > 
> > Weidong, Can we also fix these config options?
> > 
> > Today, selecting CONFIG_INTR_REMAP automatically selects
> > CONFIG_X86_X2APIC. It should be the other way round.
> > 
> > i.e., turning on CONFIG_X86_X2APIC should automatically enable
> > CONFIG_INTR_REMAP and just enabling CONFIG_INTR_REMAP shouldn't enable
> > CONFIG_X86_X2APIC.
> > 
> > Also please make sure that the code compiles (and works :)) in
> > different combinations of these config settings.
> 
> 
> Thanks for your reminder. Following patch changes the config options
> selection. The code can compiles in different combinations.

Doesn't build for me.

Firstly, your patch 2/2 doesn't apply against git HEAD. I needed to
massage it to work. I think you're testing against an old kernel.

Then (with the massaged version in the iommu-2.6.git tree) if I apply
your third patch below I get this error when building with INTR_REMAP
&& !X2APIC:

arch/x86/kernel/apic/apic.c: In function ‘lapic_resume’:
arch/x86/kernel/apic/apic.c:2054: error: ‘EIM_32BIT_APIC_ID’ undeclared (first use in this function)
arch/x86/kernel/apic/apic.c:2054: error: (Each undeclared identifier is reported only once
arch/x86/kernel/apic/apic.c:2054: error: for each function it appears in.)
make[2]: *** [arch/x86/kernel/apic/apic.o] Error 1

If I apply the naïve fix of just changing a couple more #ifdef
CONFIG_INTR_REMAP to #ifdef CONFIG_X86_X2APIC (in enable_IR_x2apic() and
lapic_resume()), it builds but doesn't boot -- it just hangs after
reporting Queued Invalidation, at the code you added in dmar.c to call
enable_intr_remapping().

Please make sure you're testing against a current kernel, and test that
it builds and boots in all three configurations (no remapping, remapping
but no x2apic, and with both enabled).

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

RE: [patch 2/2] x2apic/intr-remap: decouple interrupt remapping from x2apic

[Han, Weidong]

David Woodhouse wrote:
> On Sat, 2009-04-04 at 11:37 +0800, Han, Weidong wrote:
>> Siddha, Suresh B wrote:
>>> On Fri, 2009-04-03 at 02:15 -0700, Han, Weidong wrote:
>>>> 
>>>> -#ifdef CONFIG_INTR_REMAP
>>>> +#ifdef CONFIG_X86_X2APIC
>>> 
>>> Weidong, Can we also fix these config options?
>>> 
>>> Today, selecting CONFIG_INTR_REMAP automatically selects
>>> CONFIG_X86_X2APIC. It should be the other way round.
>>> 
>>> i.e., turning on CONFIG_X86_X2APIC should automatically enable
>>> CONFIG_INTR_REMAP and just enabling CONFIG_INTR_REMAP shouldn't
>>> enable CONFIG_X86_X2APIC. 
>>> 
>>> Also please make sure that the code compiles (and works :)) in
>>> different combinations of these config settings.
>> 
>> 
>> Thanks for your reminder. Following patch changes the config options
>> selection. The code can compiles in different combinations.
> 
> Doesn't build for me.
> 
> Firstly, your patch 2/2 doesn't apply against git HEAD. I needed to
> massage it to work. I think you're testing against an old kernel.
> 

Now I'm working on linux head:
	commit 6bb597507f9839b13498781e481f5458aea33620	
	Merge: 09f38dc... c5c67c7...
	Author: Linus Torvalds <torvalds@linux-foundation.org>
	Date:   Fri Apr 3 17:36:21 2009 -0700

I massaged the patch 2/2 like what you did in iommu-2.6.git, and applied it on linux head. 

> Then (with the massaged version in the iommu-2.6.git tree) if I apply
> your third patch below I get this error when building with INTR_REMAP
> && !X2APIC:
> 
> arch/x86/kernel/apic/apic.c: In function 'lapic_resume':
> arch/x86/kernel/apic/apic.c:2054: error: 'EIM_32BIT_APIC_ID'
> undeclared (first use in this function)
> arch/x86/kernel/apic/apic.c:2054: error: (Each undeclared identifier
> is reported only once arch/x86/kernel/apic/apic.c:2054: error: for
> each function it appears in.)  
> make[2]: *** [arch/x86/kernel/apic/apic.o] Error 1
> 

On linux head, I cannot find EIM_32BIT_APIC_ID in apic.c. I suspect iommu-2.6.git doesn't sync up with linux head on apic.c. I also fixed a bug which causes system hang due to queue invalidation is disabled when enable interrupt remapping. I will resend the patches. Do you want patches based on linux head or your iommu-2.6.git? BTW, I pull iommu-2.6.git very slow due to office proxy. But I can do it if you want. 

Regards,
Weidong


> If I apply the naïve fix of just changing a couple more #ifdef
> CONFIG_INTR_REMAP to #ifdef CONFIG_X86_X2APIC (in enable_IR_x2apic()
> and 
> lapic_resume()), it builds but doesn't boot -- it just hangs after
> reporting Queued Invalidation, at the code you added in dmar.c to call
> enable_intr_remapping().
> 
> Please make sure you're testing against a current kernel, and test
> that 
> it builds and boots in all three configurations (no remapping,
> remapping 
> but no x2apic, and with both enabled).

RE: [patch 2/2] x2apic/intr-remap: decouple interrupt remapping from x2apic

[David Woodhouse]

On Sat, 2009-04-04 at 16:48 +0800, Han, Weidong wrote:
> On linux head, I cannot find EIM_32BIT_APIC_ID in apic.c. I suspect
> iommu-2.6.git doesn't sync up with linux head on apic.c. I also fixed
> a bug which causes system hang due to queue invalidation is disabled
> when enable interrupt remapping. I will resend the patches. Do you
> want patches based on linux head or your iommu-2.6.git? BTW, I pull
> iommu-2.6.git very slow due to office proxy. But I can do it if you
> want. 

The conflict is with Fenghua's suspend/resume patches: 
http://git.infradead.org/iommu-2.6.git?a=commitdiff;h=b24696bc5

A simpler build fix is just to move the definitions of  EIM_8BIT_APIC_ID
and  EIM_32BIT_APIC_ID  outside #ifdef CONFIG_X86_X2APIC. I'll try that
and see if it boots.

-- 
dwmw2

RE: [patch 2/2] x2apic/intr-remap: decouple interrupt remapping from x2apic

[David Woodhouse]

On Sat, 2009-04-04 at 10:07 +0100, David Woodhouse wrote:
> On Sat, 2009-04-04 at 16:48 +0800, Han, Weidong wrote:
> > On linux head, I cannot find EIM_32BIT_APIC_ID in apic.c. I suspect
> > iommu-2.6.git doesn't sync up with linux head on apic.c. I also fixed
> > a bug which causes system hang due to queue invalidation is disabled
> > when enable interrupt remapping. I will resend the patches. Do you
> > want patches based on linux head or your iommu-2.6.git? BTW, I pull
> > iommu-2.6.git very slow due to office proxy. But I can do it if you
> > want. 
> 
> The conflict is with Fenghua's suspend/resume patches: 
> http://git.infradead.org/iommu-2.6.git?a=commitdiff;h=b24696bc5
> 
> A simpler build fix is just to move the definitions of  EIM_8BIT_APIC_ID
> and  EIM_32BIT_APIC_ID  outside #ifdef CONFIG_X86_X2APIC. I'll try that
> and see if it boots.

... which it doesn't. Same failure mode (the faults are normal; crappy
BIOS lacks RMRRs):

[   34.061991] DMAR:DRHD (flags: 0x00000001)base: 0x00000000fe710000
[   34.068076] DMAR:RMRR base: 0x00000000000e9000 end: 0x00000000000e9fff
[   34.074592] DMAR:RMRR base: 0x00000000000ea000 end: 0x00000000000eafff
[   34.081105] DMAR:RMRR base: 0x00000000000eb000 end: 0x00000000000ebfff
[   34.087618] DMAR:RMRR base: 0x00000000000e6000 end: 0x00000000000e6fff
[   34.094131] DMAR:RMRR base: 0x00000000000e7000 end: 0x00000000000e7fff
[   34.100644] DMAR:RMRR base: 0x00000000000e8000 end: 0x00000000000e8fff
[   34.107157] DMAR:Unknown DMAR structure type
[   34.111413] IOAPIC id 8 under DRHD base 0xfe710000
[   34.116220] DRHD: handling fault status reg 2
[   34.120567] DMAR:[DMA Read] Request device [00:1a.2] fault addr ec000 
[   34.120568] DMAR:[fault reason 06] PTE Read access is not set
[   34.133298] DMAR:[DMA Read] Request device [00:1d.1] fault addr ec000 
[   34.133298] DMAR:[fault reason 06] PTE Read access is not set
[   34.145547] DMAR:[DMA Read] Request device [00:1a.1] fault addr ec000 
[   34.145548] DMAR:[fault reason 06] PTE Read access is not set
[   34.157796] DMAR:[DMA Read] Request device [00:1d.0] fault addr ec000 
[   34.157796] DMAR:[fault reason 06] PTE Read access is not set
[   34.170045] DMAR:[DMA Read] Request device [00:1a.0] fault addr ec000 
[   34.170045] DMAR:[fault reason 06] PTE Read access is not set
[   34.182294] DMAR:[DMA Read] Request device [00:1d.2] fault addr ec000 
[   34.182295] DMAR:[fault reason 06] PTE Read access is not set
[   34.194546] IOMMU 0xfe710000: using Queued invalidation


-- 
dwmw2

RE: [patch 2/2] x2apic/intr-remap: decouple interrupt remapping from x2apic

[Han, Weidong]

David Woodhouse wrote:
> On Sat, 2009-04-04 at 10:07 +0100, David Woodhouse wrote:
>> On Sat, 2009-04-04 at 16:48 +0800, Han, Weidong wrote:
>>> On linux head, I cannot find EIM_32BIT_APIC_ID in apic.c. I suspect
>>> iommu-2.6.git doesn't sync up with linux head on apic.c. I also
>>> fixed a bug which causes system hang due to queue invalidation is
>>> disabled when enable interrupt remapping. I will resend the
>>> patches. Do you want patches based on linux head or your
>>> iommu-2.6.git? BTW, I pull iommu-2.6.git very slow due to office
>>> proxy. But I can do it if you want.
>> 
>> The conflict is with Fenghua's suspend/resume patches:
>> http://git.infradead.org/iommu-2.6.git?a=commitdiff;h=b24696bc5
>> 
>> A simpler build fix is just to move the definitions of 
>> EIM_8BIT_APIC_ID and  EIM_32BIT_APIC_ID  outside #ifdef
>> CONFIG_X86_X2APIC. I'll try that and see if it boots.
> 
> ... which it doesn't. Same failure mode (the faults are normal; crappy
> BIOS lacks RMRRs):
> 
> [   34.061991] DMAR:DRHD (flags: 0x00000001)base: 0x00000000fe710000
> [   34.068076] DMAR:RMRR base: 0x00000000000e9000 end:
> 0x00000000000e9fff [   34.074592] DMAR:RMRR base: 0x00000000000ea000
> end: 0x00000000000eafff [   34.081105] DMAR:RMRR base:
> 0x00000000000eb000 end: 0x00000000000ebfff [   34.087618] DMAR:RMRR
> base: 0x00000000000e6000 end: 0x00000000000e6fff [   34.094131]
> DMAR:RMRR base: 0x00000000000e7000 end: 0x00000000000e7fff [  
> 34.100644] DMAR:RMRR base: 0x00000000000e8000 end: 0x00000000000e8fff
> [   34.107157] DMAR:Unknown DMAR structure type [   34.111413] IOAPIC
> id 8 under DRHD base 0xfe710000 [   34.116220] DRHD: handling fault
> status reg 2 [   34.120567] DMAR:[DMA Read] Request device [00:1a.2]
> fault addr ec000 [   34.120568] DMAR:[fault reason 06] PTE Read
> access is not set [   34.133298] DMAR:[DMA Read] Request device
> [00:1d.1] fault addr ec000 [   34.133298] DMAR:[fault reason 06] PTE
> Read access is not set [   34.145547] DMAR:[DMA Read] Request device
> [00:1a.1] fault addr ec000 [   34.145548] DMAR:[fault reason 06] PTE
> Read access is not set [   34.157796] DMAR:[DMA Read] Request device
> [00:1d.0] fault addr ec000 [   34.157796] DMAR:[fault reason 06] PTE
> Read access is not set [   34.170045] DMAR:[DMA Read] Request device
> [00:1a.0] fault addr ec000 [   34.170045] DMAR:[fault reason 06] PTE
> Read access is not set [   34.182294] DMAR:[DMA Read] Request device
> [00:1d.2] fault addr ec000 [   34.182295] DMAR:[fault reason 06] PTE
> Read access is not set [   34.194546] IOMMU 0xfe710000: using Queued
> invalidation 


Following patch fixes it. I tested it with different combinations, it works fine for me. pls try on your side. 


Subject: [PATCH] x86, dmar: check if it's initialized before disable queue invalidation

If queue invalidation is disabled after it's already initialized,
dmar_enable_qi won't re-enable it due to iommu->qi is allocated.
It may result in system hang when use queue invalidation. Add this
check to avoid this case.

Signed-off-by: Weidong Han <weidong.han@intel.com>
---
 drivers/pci/intr_remapping.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/intr_remapping.c b/drivers/pci/intr_remapping.c
index b041a40..5eb72e0 100644
--- a/drivers/pci/intr_remapping.c
+++ b/drivers/pci/intr_remapping.c
@@ -502,6 +502,13 @@ int __init enable_intr_remapping(int eim)
 	for_each_drhd_unit(drhd) {
 		struct intel_iommu *iommu = drhd->iommu;
 
+		/* 
+		 * If the queued invalidation is already initialized,
+		 * shouldn't disable it.
+		 */
+		if (iommu->qi)
+			continue;
+
 		/*
 		 * Clear previous faults.
 		 */
-- 
1.6.0.4

RE: [patch 2/2] x2apic/intr-remap: decouple interrupt remapping from x2apic

[David Woodhouse]

On Sat, 2009-04-04 at 17:21 +0800, Han, Weidong wrote:
> Following patch fixes it. I tested it with different combinations, it
> works fine for me. pls try on your side. 

Works without x2apic; just about to test with it. Thanks.

-- 
dwmw2