x86/sfi: fix ioapic gsi range

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.

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.

Quoted 4 lines · Jacob Pan

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.

Quoted 5 lines

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.

arch/x86/kernel/apic/io_apic.c +0 -0

index 33f3563..5de84e5 100644

Quoted 34 lines · Eric W. Biederman

Quoted 4 lines · Jacob Pan

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.

Quoted 5 lines

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.

arch/x86/kernel/apic/io_apic.c +0 -0

index 33f3563..5de84e5 100644
@@ -90,7 +90,7 @@ int nr_ioapics;
 struct mp_ioapic_gsi  mp_gsi_routing[MAX_IO_APICS];
 

arch/x86/kernel/apic/io_apic.c int nr_ioapics; · line 94

Quoted 4 lines

arch/x86/kernel/apic/io_apic.c +1 -1

 /* 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.

Quoted 2 lines

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

Quoted 2 lines

Quoted 2 lines

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

Linux IRQ 0 is "no IRQ assigned", except buried in certain bits of arch specific historical knowledge.

Also calling 1-15 ISA IRQ lines is also somewhat misleading given they are almost certainly routing for PCI devices. "PIC IRQ routing" maybe - but even that is not really true on a lot of PC hardware today except by convention.

Quoted 6 lines · Alan Cox

Quoted 2 lines

Quoted 2 lines

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

Linux IRQ 0 is "no IRQ assigned", except buried in certain bits of arch specific historical knowledge.

Quoted 4 lines

Also calling 1-15 ISA IRQ lines is also somewhat misleading given they are almost certainly routing for PCI devices. "PIC IRQ routing" maybe - but even that is not really true on a lot of PC hardware today except by convention.

Yes, but I gather IRQ/GSI 0 is an early-acquire primary timer on MRST on Moorestown just as on PC/AT... just a different one. Hence "special" in the same sort of way. I don't really care, personally, though.

Quoted 6 lines

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.

Quoted 6 lines

Quoted 5 lines

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.

Quoted 7 lines

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

The issue is what acpi calls bus 0 irqs, and how drivers deal with them. We wind up having well know irqs: irqs 3 and 4 for serial ports, irq 7 for parallel ports. irqs 14, and 15 for ide.

A bunch of these hardware devices we can get if someone connects up a lpc superio chip.

Even if sfi is never implemented on a platform where that kind of hardware exists, the current sfi code is setup to coexist simultaneously in the kernel with all of the infrastructure of other platforms where those kinds of devices exist. Which means there can be drivers compiled into your kernel that make assumptions about special properties of the irqs 0-15.

With the current code you should get all of the remapping of the gsi's out of the legacy irq space without needing to lift a finger, and if someone later decides we need an irq override so we can have an isa irq present on a weird embedded system on a chip the code will be able to handle that easily.

IRQ 14/15 is wrong for ATA today as its AHCI based on modern boxes

Quoted 1 line

IRQ 14/15 is wrong for ATA today as its AHCI based on modern boxes

Not to mention even before that there was native mode IDE!In fact, XP SP1 and later support switching to native mode IDE on BIOSes supporting it as described in this document:http://www.microsoft.com/whdc/device/storage/Native-modeATA.mspx

Until then I guess we get things like Moorestown which are effectively a reinvention of ISA based systems, with different firmware, and

At the ioapic and gsi level, and in your firmware interface reusing the numbers is fine.

The issue is what acpi calls bus 0 irqs, and how drivers deal with them. We wind up having well know irqs: irqs 3 and 4 for serial ports, irq 7 for parallel ports. irqs 14, and 15 for ide.

A bunch of these hardware devices we can get if someone connects up a lpc superio chip.

Even if sfi is never implemented on a platform where that kind of hardware exists, the current sfi code is setup to coexist simultaneously in the kernel with all of the infrastructure of other platforms where those kinds of devices exist. Which means there can be drivers compiled into your kernel that make assumptions about special properties of the irqs 0-15.

I have seen a lot of weird hard to track issues, because of a conflict in assumptions over the ISA irqs. It is easiest and safest just to let the first 16 linux irq numbers be reserved for the legacy oddness, so code can make assumptions and we don't have to worry about it.

As for the question about using legacy_pic to detect the absence of an irq controller that Peter raised. We can't do that because it should be possible for an acpi system with all of the legacy hardware to exist without needing to implement an i8259, or ever run in the historical interrupt delivery mode of pcs.

With the current code you should get all of the remapping of the gsi's out of the legacy irq space without needing to lift a finger, and if someone later decides we need an irq override so we can have an isa irq present on a weird embedded system on a chip the code will be able to handle that easily.

Quoted 29 lines · Eric W. Biederman

At the ioapic and gsi level, and in your firmware interface reusing the numbers is fine.

The issue is what acpi calls bus 0 irqs, and how drivers deal with them. We wind up having well know irqs: irqs 3 and 4 for serial ports, irq 7 for parallel ports. irqs 14, and 15 for ide.

A bunch of these hardware devices we can get if someone connects up a lpc superio chip.

Even if sfi is never implemented on a platform where that kind of hardware exists, the current sfi code is setup to coexist simultaneously in the kernel with all of the infrastructure of other platforms where those kinds of devices exist. Which means there can be drivers compiled into your kernel that make assumptions about special properties of the irqs 0-15.

I have seen a lot of weird hard to track issues, because of a conflict in assumptions over the ISA irqs. It is easiest and safest just to let the first 16 linux irq numbers be reserved for the legacy oddness, so code can make assumptions and we don't have to worry about it.

As for the question about using legacy_pic to detect the absence of an irq controller that Peter raised. We can't do that because it should be possible for an acpi system with all of the legacy hardware to exist without needing to implement an i8259, or ever run in the historical interrupt delivery mode of pcs.

Quoted 5 lines

With the current code you should get all of the remapping of the gsi's out of the legacy irq space without needing to lift a finger, and if someone later decides we need an irq override so we can have an isa irq present on a weird embedded system on a chip the code will be able to handle that easily.

OK, let me ask this, then: if we do that, and we hardcode this as 16 magic IRQs indefinitely, does that mean we lose 16 IDT entries indefinitely as well? We are already shy on interrupt vectors, and that would make me unhappy.

No. There is no reason to loose 16 IDT entries indefinitely. We may need a boot time allocation when we see we have isa irqs, to replace the static allocation that we have. But for the most part we dynamically idt entries aka vector numbers today, and there is no reason we can't generalize that in the future.

Quoted 7 lines · Eric W. Biederman

No. There is no reason to loose 16 IDT entries indefinitely. We may need a boot time allocation when we see we have isa irqs, to replace the static allocation that we have. But for the most part we dynamically idt entries aka vector numbers today, and there is no reason we can't generalize that in the future.

Well, that boot time allocation is one of the things legacy_pic->nr_legacy_irq is used for, and it really makes sense, I think. I would really like to move away from a compile-time allocation, and I still find it hard to believe it has a reason to exist.

Even if sfi is never implemented on a platform where that kind of hardware exists, the current sfi code is setup to coexist simultaneously in the kernel with all of the infrastructure of other platforms where those kinds of devices exist. Which means there can be drivers compiled into your kernel that make assumptions about special properties of the irqs 0-15.

As for the question about using legacy_pic to detect the absence of an irq controller that Peter raised. We can't do that because it should be possible for an acpi system with all of the legacy hardware to exist without needing to implement an i8259, or ever run in the historical interrupt delivery mode of pcs.

Quoted 7 lines · On Tue, 08 Jun 2010 12:41:45 -0700 [email protected] (Eric W. Biederman)…

Even if sfi is never implemented on a platform where that kind of hardware exists, the current sfi code is setup to coexist simultaneously in the kernel with all of the infrastructure of other platforms where those kinds of devices exist. Which means there can be drivers compiled into your kernel that make assumptions about special properties of the irqs 0-15.

SFI code can be compiled in with ACPI at the same time but at runtime there is only one used, ACPI take precedence. So there wouldn't be any additional conflict caused by SFI added APIC tables.

Quoted 5 lines

As for the question about using legacy_pic to detect the absence of an irq controller that Peter raised. We can't do that because it should be possible for an acpi system with all of the legacy hardware to exist without needing to implement an i8259, or ever run in the historical interrupt delivery mode of pcs.

In your case, I don't understand how would it change the calculation of irq mapping. Even if you don't use i8259 on a x86 PC platform, you still have NR_LEGACY_IRQS=legacy_pic->nr_legacy_irqs.

On the other side, use NR_LEGACY_IRQS breaks the existing code for Moorestown in terms of irq-gsi lookup and nr_irqs_gsi.

Quoted 20 lines · jacob pan

Quoted 7 lines · On Tue, 08 Jun 2010 12:41:45 -0700 [email protected] (Eric W. Biederman)…

Even if sfi is never implemented on a platform where that kind of hardware exists, the current sfi code is setup to coexist simultaneously in the kernel with all of the infrastructure of other platforms where those kinds of devices exist. Which means there can be drivers compiled into your kernel that make assumptions about special properties of the irqs 0-15.

SFI code can be compiled in with ACPI at the same time but at runtime there is only one used, ACPI take precedence. So there wouldn't be any additional conflict caused by SFI added APIC tables.

Quoted 5 lines

As for the question about using legacy_pic to detect the absence of an irq controller that Peter raised. We can't do that because it should be possible for an acpi system with all of the legacy hardware to exist without needing to implement an i8259, or ever run in the historical interrupt delivery mode of pcs.

Quoted 6 lines

In your case, I don't understand how would it change the calculation of irq mapping. Even if you don't use i8259 on a x86 PC platform, you still have NR_LEGACY_IRQS=legacy_pic->nr_legacy_irqs.

On the other side, use NR_LEGACY_IRQS breaks the existing code for Moorestown in terms of irq-gsi lookup and nr_irqs_gsi.

Is this code merged where I should have fixed it in my patchset?

We went through this with acpi having an identity mapping of irq to gsi mapping and the result is that we (a) developed weird platform specific hooks for things that should have been handled by generic code, and on other systems we lost access to 16 irqs.

It took probably 10 years to sort the acpi irq handling out. What I have learned along the way is:
- Sharing irq in software is madness, so a one to one mapping with hardware irq is required.
- An identity mapping with gsis is nice but we can't count on the hardware designers or the spec designers always doing sane and reasonable things so not guaranteeing a particular mapping is important.

If I have actually broken any sfi drivers because you assumed a particular we are back where we were with ISA, and still haven't completely escaped. The abstraction layer should provide all of the mapping so drivers only see linux irq numbers.

Eric

[jacob pan]

In arch/x86/kernel/mrst.c we parse SFI MTMR table then add timer irqs to mp_irqs. what is broken by this patch is pin_2_irq() lookup for the legacy irq range since we want NR_IRQS_LEGACY to be 0 on Moorestown. We do have the assumption that mp_irqs from SFI is 1:1 mapped to IRQs.

Doing this can fix the problem, but you mentioned you have to use NR_IRQS_LEGACY, which i still don't understand.

Quoted 7 lines · jacob pan

[jacob pan]

In arch/x86/kernel/mrst.c we parse SFI MTMR table then add timer irqs to mp_irqs. what is broken by this patch is pin_2_irq() lookup for the legacy irq range since we want NR_IRQS_LEGACY to be 0 on Moorestown. We do have the assumption that mp_irqs from SFI is 1:1 mapped to IRQs.

NR_IRQS_LEGACY is a constant of 16.

Quoted 3 lines

Doing this can fix the problem, but you mentioned you have to use NR_IRQS_LEGACY, which i still don't understand.

Looking at the code in io_apic.c there is a relatively clean way to handle this. The actual concept in io_apic.c today is mp_bus_not_pci.

So you just need to do:

arch/x86/kernel/mrst.c +1 -0

index e796448..9377fda 100644
@@ -242,4 +242,5 @@ void __init x86_mrst_early_setup(void)
 	x86_init.mpparse.find_smp_config = x86_init_noop;
 	x86_init.mpparse.get_smp_config = x86_init_uint_noop;
 
+	set_bit(0, mp_bus_not_pci);
 }

Then you get treated as ISA for purposes of pin_2_irq, and everything is a pass through. Since that seems to be what you want anyway I don't see a problem with that for now.

Using legacy pic to even talk about this behavior is wrong as that is hardware abstraction and the presence or absence of an i8259 has nothing to do with the presence of ISA irqs and their descendants.

Eric

Eric W. Biederman Wed, 09 Jun 2010 16:44:11 -0700

Quoted 4 lines

Quoted 2 lines · jacob pan

[jacob pan]

Quoted 7 lines

Quoted 5 lines

In arch/x86/kernel/mrst.c we parse SFI MTMR table then add timer irqs to mp_irqs. what is broken by this patch is pin_2_irq() lookup for the legacy irq range since we want NR_IRQS_LEGACY to be 0 on Moorestown. We do have the assumption that mp_irqs from SFI is 1:1 mapped to IRQs.

NR_IRQS_LEGACY is a constant of 16.

i know, what i meant is we want this to be a variable.

Quoted 20 lines

Quoted 2 lines

Doing this can fix the problem, but you mentioned you have to use NR_IRQS_LEGACY, which i still don't understand.

Looking at the code in io_apic.c there is a relatively clean way to handle this. The actual concept in io_apic.c today is mp_bus_not_pci.

So you just need to do:

arch/x86/kernel/mrst.c +1 -0

index e796448..9377fda 100644
@@ -242,4 +242,5 @@ void __init x86_mrst_early_setup(void)
 	x86_init.mpparse.find_smp_config = x86_init_noop;
 	x86_init.mpparse.get_smp_config = x86_init_uint_noop;
 
+	set_bit(0, mp_bus_not_pci);
 }

Quoted 4 lines

Then you get treated as ISA for purposes of pin_2_irq, and everything is a pass through. Since that seems to be what you want anyway I don't see a problem with that for now.

I think this may work better. Since we already assign srcbus = MP_ISA_BUS to use the mp_irqs, this may be more inline with the current code. Thanks for the pointer.

But the nr_irqs_gsi would still be wrong for Moorestown if don't use legacy_pic or some sort of runtime detection.