2009-04-03 09:17:42

by Weidong Han

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

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 <[email protected]>
---
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


2009-04-03 17:59:00

by Suresh Siddha

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

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

2009-04-03 18:09:26

by David Woodhouse

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

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

2009-04-03 18:11:47

by Suresh Siddha

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

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.

2009-04-04 03:38:20

by Weidong Han

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

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 <[email protected]>
---
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

2009-04-04 08:29:22

by David Woodhouse

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

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
[email protected] Intel Corporation

2009-04-04 08:55:48

by Weidong Han

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

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 <[email protected]>
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).

2009-04-04 09:07:53

by David Woodhouse

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

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

2009-04-04 09:15:50

by David Woodhouse

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

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

2009-04-04 09:21:44

by Weidong Han

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

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 <[email protected]>
---
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

2009-04-04 09:46:49

by David Woodhouse

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

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