x86/sfi: fix ioapic gsi range
in [Kernel]
|
Prev: Fwd: EXT3 File System Corruption 2.6.34
Next: 2.6.35-rc2-git1 - include/linux/cgroup.h:534 invoked rcu_dereference_check() without protection!
From: jacob pan on 7 Jun 2010 20:10 Jacob Pan Mon, 7 Jun 2010 16:07:24 -0700 >SFI based platforms should have zero based gsi_base for IOAPICs found in SFI >tables. The current code sets gsi_base starting from 1 when registering ioapic. >The result is that Moorestown platform would have wrong mp_gsi_routing for each >ioapic. > >Background: >In Moorestown/Medfield platforms, there is no legacy IRQs, all gsis and irqs >are one to one mapped, including those < 16. Specifically, IRQ0 and IRQ1 are >used for per-cpu timers. So without this patch, IOAPIC pin to IRQ mapping is >off by one. > Clarifiction/correction about IRQ0,1. I refer to IOAPIC IRQ #, which is the IOAPIC RTE entry #. Not in the sense of kernel IRQ# which can be assigned differently on Moorestown. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Eric W. Biederman on 7 Jun 2010 20:30 Jacob Pan <jacob.jun.pan(a)linux.intel.com> writes: > SFI based platforms should have zero based gsi_base for IOAPICs found in SFI > tables. The current code sets gsi_base starting from 1 when registering ioapic. > The result is that Moorestown platform would have wrong mp_gsi_routing for each > ioapic. Yes starting at 1 is a bug. > Background: > In Moorestown/Medfield platforms, there is no legacy IRQs, all gsis and irqs > are one to one mapped, including those < 16. Specifically, IRQ0 and IRQ1 are > used for per-cpu timers. So without this patch, IOAPIC pin to IRQ mapping is > off by one. The patch looks mostly reasonable the comment is wrong. You may not use a 1-1 mapping if you don't have legacy irqs. Linux irqs 0-15 are the ISA irqs you may not use those irq numbers for something different on any architecture, but especially not on x86. The gsi numbers are firmware specific and you may treat however you want. Does the following patch work for you? It appears I goofed when it was pointed out that gsi_end was inclusive and didn't change the initialize. Eric --- diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c index 33f3563..5de84e5 100644 --- a/arch/x86/kernel/apic/io_apic.c +++ b/arch/x86/kernel/apic/io_apic.c @@ -90,7 +90,7 @@ int nr_ioapics; struct mp_ioapic_gsi mp_gsi_routing[MAX_IO_APICS]; /* The last gsi number used */ -u32 gsi_end; +u32 gsi_end = -1; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: H. Peter Anvin on 7 Jun 2010 20:40 On 06/07/2010 05:24 PM, Eric W. Biederman wrote: > Jacob Pan <jacob.jun.pan(a)linux.intel.com> writes: > >> SFI based platforms should have zero based gsi_base for IOAPICs found in SFI >> tables. The current code sets gsi_base starting from 1 when registering ioapic. >> The result is that Moorestown platform would have wrong mp_gsi_routing for each >> ioapic. > > Yes starting at 1 is a bug. > >> Background: >> In Moorestown/Medfield platforms, there is no legacy IRQs, all gsis and irqs >> are one to one mapped, including those < 16. Specifically, IRQ0 and IRQ1 are >> used for per-cpu timers. So without this patch, IOAPIC pin to IRQ mapping is >> off by one. > > The patch looks mostly reasonable the comment is wrong. > > You may not use a 1-1 mapping if you don't have legacy irqs. Linux > irqs 0-15 are the ISA irqs you may not use those irq numbers for > something different on any architecture, but especially not on x86. > The gsi numbers are firmware specific and you may treat however you want. > > Does the following patch work for you? > > It appears I goofed when it was pointed out that gsi_end was inclusive and > didn't change the initialize. > > diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c > index 33f3563..5de84e5 100644 > --- a/arch/x86/kernel/apic/io_apic.c > +++ b/arch/x86/kernel/apic/io_apic.c > @@ -90,7 +90,7 @@ int nr_ioapics; > struct mp_ioapic_gsi mp_gsi_routing[MAX_IO_APICS]; > > /* The last gsi number used */ > -u32 gsi_end; > +u32 gsi_end = -1; > This seems like asking for signedness problems, especially since this is used in range compares all the time. The real problem here is that gsi_end is inclusive, which is almost always the wrong thing for the endpoint of a range. Instead we should have the last number used plus one; perhaps it should be called gsi_next or gsi_free. -hpa -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Eric W. Biederman on 7 Jun 2010 21:20 "H. Peter Anvin" <hpa(a)zytor.com> writes: > On 06/07/2010 05:24 PM, Eric W. Biederman wrote: >> Jacob Pan <jacob.jun.pan(a)linux.intel.com> writes: >> >>> SFI based platforms should have zero based gsi_base for IOAPICs found in SFI >>> tables. The current code sets gsi_base starting from 1 when registering ioapic. >>> The result is that Moorestown platform would have wrong mp_gsi_routing for each >>> ioapic. >> >> Yes starting at 1 is a bug. >> >>> Background: >>> In Moorestown/Medfield platforms, there is no legacy IRQs, all gsis and irqs >>> are one to one mapped, including those < 16. Specifically, IRQ0 and IRQ1 are >>> used for per-cpu timers. So without this patch, IOAPIC pin to IRQ mapping is >>> off by one. >> >> The patch looks mostly reasonable the comment is wrong. >> >> You may not use a 1-1 mapping if you don't have legacy irqs. Linux >> irqs 0-15 are the ISA irqs you may not use those irq numbers for >> something different on any architecture, but especially not on x86. >> The gsi numbers are firmware specific and you may treat however you want. >> >> Does the following patch work for you? >> >> It appears I goofed when it was pointed out that gsi_end was inclusive and >> didn't change the initialize. >> >> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c >> index 33f3563..5de84e5 100644 >> --- a/arch/x86/kernel/apic/io_apic.c >> +++ b/arch/x86/kernel/apic/io_apic.c >> @@ -90,7 +90,7 @@ int nr_ioapics; >> struct mp_ioapic_gsi mp_gsi_routing[MAX_IO_APICS]; >> >> /* The last gsi number used */ >> -u32 gsi_end; >> +u32 gsi_end = -1; >> > > This seems like asking for signedness problems, especially since this is > used in range compares all the time. The real problem here is that > gsi_end is inclusive, which is almost always the wrong thing for the > endpoint of a range. Instead we should have the last number used plus > one; perhaps it should be called gsi_next or gsi_free. That does sound better. Let me see if I can find a few minutes to implement it that way. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: jacob pan on 8 Jun 2010 02:00
> > Background: > > In Moorestown/Medfield platforms, there is no legacy IRQs, all gsis > > and irqs are one to one mapped, including those < 16. Specifically, > > IRQ0 and IRQ1 are used for per-cpu timers. So without this patch, > > IOAPIC pin to IRQ mapping is off by one. > > The patch looks mostly reasonable the comment is wrong. > > You may not use a 1-1 mapping if you don't have legacy irqs. Linux > irqs 0-15 are the ISA irqs you may not use those irq numbers for > something different on any architecture, but especially not on x86. > The gsi numbers are firmware specific and you may treat however you > want. [jacob pan] If we don't have ISA irqs, why can't we have gsi# = irq# for the legacy IRQ range? On Moorestown, we are re-using legacy irqs. e.g. sh-4.0# cat /proc/interrupts CPU0 CPU1 0: 1512 0 IO-APIC-edge apbt0 1: 0 1482 IO-APIC-edge apbt1 9: 0 0 IO-APIC-fasteoi dw_spi 10: 0 0 IO-APIC-fasteoi mrst_i2c 11: 0 0 IO-APIC-fasteoi mrst_i2c 12: 0 0 IO-APIC-fasteoi mrst_i2c 23: 0 0 IO-APIC-fasteoi intel_scu_ipc 27: 21 0 IO-APIC-fasteoi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |