RE: IO-APIC + timer doesn't work

RE: IO-APIC + timer doesn't work

[Lu, Yinghai]

-----Original Message-----
From: ebiederm@xmission.com [mailto:ebiederm@xmission.com] 
Sent: Thursday, December 21, 2006 12:47 PM
To: Lu, Yinghai
>> +static int add_irq_entry(int type, int irqflag, int bus, int irq,
int apic, int
>> pin)

>This is fairly sane but probably belongs in mptable.c as a helper.

mparse.c?


>I am still trying to understand this enable_8259A_irq(0) case.
>As far as I can tell this is a very backwards way of enabling
>an ExtINT, as such it shouldn't be used until later.

>YH do you have any insight why on some Nvidia chipsets we apic 0 pin 2
doesn't
>work for the timer interrupt.  I thought that was what we were using in
LinuxBIOS
>for the mptable.

CK804's has problem. But later one seems fixed that problem.

YH

Re: IO-APIC + timer doesn't work

[Eric W. Biederman]

"Lu, Yinghai" <yinghai.lu@amd.com> writes:

> -----Original Message-----
> From: ebiederm@xmission.com [mailto:ebiederm@xmission.com] 
> Sent: Thursday, December 21, 2006 12:47 PM
> To: Lu, Yinghai
>>> +static int add_irq_entry(int type, int irqflag, int bus, int irq,
> int apic, int
>>> pin)
>
>>This is fairly sane but probably belongs in mptable.c as a helper.
>
> mparse.c?

yep.

>>I am still trying to understand this enable_8259A_irq(0) case.
>>As far as I can tell this is a very backwards way of enabling
>>an ExtINT, as such it shouldn't be used until later.
>
>>YH do you have any insight why on some Nvidia chipsets we apic 0 pin 2
> doesn't
>>work for the timer interrupt.  I thought that was what we were using in
> LinuxBIOS
>>for the mptable.
>
> CK804's has problem. But later one seems fixed that problem.

Do you have any details?

Eric

Re: IO-APIC + timer doesn't work

[Yinghai Lu]

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

Please check the revised patch

YH

[-- Attachment #2: timers_12312006.diff --]
[-- Type: text/x-patch, Size: 7190 bytes --]

[PATCH] x86_64: check_timer with io apic setup before try_apic_pin

add io apic setup before try_apic_pin for check_timer

also add remove_irq_to_pin call in io_apic.c

cc: Andi Kleen <ak@suse.de>
cc: Eric W. Biederman <ebiederm@xmission.com>
Signed-off-by: Yinghai Lu <yinghai.lu@amd.com>

diff --git a/include/asm-x86_64/mpspec.h b/include/asm-x86_64/mpspec.h
index 017fddb..1ddfd4d 100644
--- a/include/asm-x86_64/mpspec.h
+++ b/include/asm-x86_64/mpspec.h
@@ -165,6 +165,7 @@ extern int mp_bus_id_to_pci_bus [MAX_MP_BUSSES];
 extern unsigned int boot_cpu_physical_apicid;
 extern int smp_found_config;
 extern void find_smp_config (void);
+extern int add_irq_entry (int type, int irqflag, int bus, int irq, int apic, int pin);
 extern void get_smp_config (void);
 extern int nr_ioapics;
 extern unsigned char apic_version [MAX_APICS];
diff --git a/arch/x86_64/kernel/mpparse.c b/arch/x86_64/kernel/mpparse.c
index 0807256..a054798 100644
--- a/arch/x86_64/kernel/mpparse.c
+++ b/arch/x86_64/kernel/mpparse.c
@@ -314,6 +314,34 @@ static int __init ELCR_trigger(unsigned int irq)
 	return (inb(port) >> (irq & 7)) & 1;
 }
 
+int add_irq_entry(int type, int irqflag, int bus, int irq, int apic, int pin)
+{
+        struct mpc_config_intsrc intsrc;
+	int idx;
+
+        intsrc.mpc_type = MP_INTSRC;
+        intsrc.mpc_irqflag = irqflag; /* conforming */
+        intsrc.mpc_srcbus = bus;
+        intsrc.mpc_dstapic = (apic != -1) ? mp_ioapics[apic].mpc_apicid: MP_APIC_ALL;
+
+        intsrc.mpc_irqtype = type;
+
+        intsrc.mpc_srcbusirq = irq;
+        intsrc.mpc_dstirq = pin;
+
+        mp_irqs [mp_irq_entries] = intsrc;
+        Dprintk("Int: type %d, pol %d, trig %d, bus %d,"
+                " IRQ %02x, APIC ID %x, APIC INT %02x\n",
+                        intsrc.mpc_irqtype, intsrc.mpc_irqflag & 3,
+                        (intsrc.mpc_irqflag >> 2) & 3, intsrc.mpc_srcbus,
+                        intsrc.mpc_srcbusirq, intsrc.mpc_dstapic, intsrc.mpc_dstirq);
+        idx = mp_irq_entries;
+	if (++mp_irq_entries >= MAX_IRQ_SOURCES)
+                panic("Max # of irq sources exceeded!!\n");
+	return idx;
+
+}
+
 static void __init construct_default_ioirq_mptable(int mpc_default_type)
 {
 	struct mpc_config_intsrc intsrc;
diff --git a/arch/x86_64/kernel/io_apic.c b/arch/x86_64/kernel/io_apic.c
index 2a1dcd5..ad1a28a 100644
--- a/arch/x86_64/kernel/io_apic.c
+++ b/arch/x86_64/kernel/io_apic.c
@@ -273,10 +273,17 @@ static void add_pin_to_irq(unsigned int irq, int apic, int pin)
 	struct irq_pin_list *entry = irq_2_pin + irq;
 
 	BUG_ON(irq >= NR_IRQS);
-	while (entry->next)
+	while (entry->next) {
+		if (entry->apic == apic && entry->pin == pin) 
+			return;
+		if (entry->pin == -1) 
+			break;
 		entry = irq_2_pin + entry->next;
+	}
 
 	if (entry->pin != -1) {
+		if (entry->apic == apic && entry->pin == pin) 
+			return;
 		entry->next = first_free_entry;
 		entry = irq_2_pin + entry->next;
 		if (++first_free_entry >= PIN_MAP_SIZE)
@@ -286,6 +293,39 @@ static void add_pin_to_irq(unsigned int irq, int apic, int pin)
 	entry->pin = pin;
 }
 
+static void remove_pin_to_irq(unsigned int irq, int apic, int pin)
+{
+	struct irq_pin_list *entry = irq_2_pin + irq;
+	struct irq_pin_list *pri;
+	struct irq_pin_list *next;
+
+	BUG_ON(irq >= NR_IRQS);
+
+	for (;;) {
+		if (entry->apic == apic && entry->pin == pin) {
+			if(entry->next) {
+				next = irq_2_pin + entry->next;
+				entry->apic = next->apic;
+				entry->pin = next->pin;
+				entry->next = next->next;
+				next->apic = -1;
+				next->pin = -1;
+				next->next = 0;
+			} else {
+				entry->apic = -1;
+				entry->pin = -1;
+			}
+			return;
+		}
+		pri = entry;
+		if (pri->next) 
+			entry = irq_2_pin + pri->next;
+		else
+			break;
+	} 
+
+}
+
 
 #define DO_ACTION(name,R,ACTION, FINAL)					\
 									\
@@ -1570,6 +1610,22 @@ static inline void unlock_ExtINT_logic(void)
  * fanatically on his truly buggy board.
  */
 
+static void set_try_apic_pin(int apic, int pin, int type)
+{
+	int idx;
+	int irq = 0;
+	int bus = 0; /* MP_ISA_BUS */
+	int irqflag = 5; /* MP_IRQ_TRIGGER_EDGE|MP_IRQ_POLARITY_HIGH */
+
+	idx = find_irq_entry(apic,pin,type);
+
+	if (idx == -1) 
+		idx = add_irq_entry(type, irqflag, bus, irq, apic, pin);
+
+	add_pin_to_irq(irq, apic, pin);
+	setup_IO_APIC_irq(apic, pin, idx, irq);
+}
+
 static int try_apic_pin(int apic, int pin, char *msg)
 {
 	apic_printk(APIC_VERBOSE, KERN_INFO
@@ -1588,7 +1644,7 @@ static int try_apic_pin(int apic, int pin, char *msg)
 		}
 		return 1;
 	}
-	clear_IO_APIC_pin(apic, pin);
+
 	apic_printk(APIC_QUIET, KERN_ERR " .. failed\n");
 	return 0;
 }
@@ -1599,6 +1655,7 @@ static void check_timer(void)
 	int apic1, pin1, apic2, pin2;
 	int vector;
 	cpumask_t mask;
+	int i;
 
 	/*
 	 * get/set the timer IRQ vector:
@@ -1621,33 +1678,51 @@ static void check_timer(void)
 	pin2  = ioapic_i8259.pin;
 	apic2 = ioapic_i8259.apic;
 
-	/* Do this first, otherwise we get double interrupts on ATI boards */
-	if ((pin1 != -1) && try_apic_pin(apic1, pin1,"with 8259 IRQ0 disabled"))
-		return;
+	apic_printk(APIC_VERBOSE,KERN_INFO "..TIMER: vector=0x%02X apic1=%d pin1=%d apic2=%d pin2=%d\n",
+		vector, apic1, pin1, apic2, pin2);
 
-	/* Now try again with IRQ0 8259A enabled.
-	   Assumes timer is on IO-APIC 0 ?!? */
-	enable_8259A_irq(0);
-	unmask_IO_APIC_irq(0);
-	if (try_apic_pin(apic1, pin1, "with 8259 IRQ0 enabled"))
-		return;
-	disable_8259A_irq(0);
+	if (pin1 != -1) {
+		/* Do this first, otherwise we get double interrupts on ATI boards */
+		/* set_try_apic_pin will call disable_8259A_irq */
+		set_try_apic_pin(apic1, pin1, mp_INT);
+		unmask_IO_APIC_irq(0);
+		if (try_apic_pin(apic1, pin1,"with 8259 IRQ0 disabled"))
+			return;
 
-	/* Always try pin0 and pin2 on APIC 0 to handle buggy timer overrides
-	   on Nvidia boards */
-	if (!(apic1 == 0 && pin1 == 0) &&
-	    try_apic_pin(0, 0, "fallback with 8259 IRQ0 disabled"))
-		return;
-	if (!(apic1 == 0 && pin1 == 2) &&
-	    try_apic_pin(0, 2, "fallback with 8259 IRQ0 disabled"))
-		return;
+		/* Now try again with IRQ0 8259A enabled.
+		   Assumes timer is on IO-APIC 0 ?!? */
+		enable_8259A_irq(0);
+		if (try_apic_pin(apic1, pin1, "with 8259 IRQ0 enabled"))
+			return;
+		disable_8259A_irq(0);
+
+    	        clear_IO_APIC_pin(apic1, pin1);
+		remove_pin_to_irq(0, apic1, pin1);
+	}
 
 	/* Then try pure 8259A routing on the 8259 as reported by BIOS*/
-	enable_8259A_irq(0);
 	if (pin2 != -1) {
 		setup_ExtINT_IRQ0_pin(apic2, pin2, vector);
+		add_pin_to_irq(0, apic2, pin2);
+		enable_8259A_irq(0);
 		if (try_apic_pin(apic2,pin2,"8259A broadcast ExtINT from BIOS"))
 			return;
+		clear_IO_APIC_pin(apic2, pin2);
+		remove_pin_to_irq(0, apic2, pin2);
+	}
+
+	/* Always try pin0 and pin2 on APIC 0 to handle buggy timer overrides
+	   on Nvidia boards */
+	for (i = 0; i <= 2; i += 2) 
+	if (!(apic1 == 0 && pin1 == i)) {
+		/* set_try_apic_pin will call disable_8259A_irq */
+		set_try_apic_pin(0, i, mp_INT); 
+		unmask_IO_APIC_irq(0);
+		if (try_apic_pin(0, i, "fallback with 8259 IRQ0 disabled"))
+			return;
+
+		clear_IO_APIC_pin(0, i);
+		remove_pin_to_irq(0, 0, i);
 	}
 
 	/* Tried all possibilities to go through the IO-APIC. Now come the

Re: IO-APIC + timer doesn't work

[Eric W. Biederman]

"Yinghai Lu" <yinghai.lu@amd.com> writes:

> On 12/19/06, Eric W. Biederman <ebiederm@xmission.com> wrote:
>> So the pin2 case should be tested right after the pin1 case as we do
>> currently.  On most new boards that will be a complete noop.
>>
>> But it is better than our current blind guess at using ExtINT mode.
>>
>> I figure after we try what the BIOS has told us about and that
>> has failed we should first try the common irq 0 apic mappings,
>> and then try the common ExtINT mappings.
>
> Please check if this one is ok.
>
> [PATCH] x86_64: check_timer with io apic setup before try_apic_pin
>
> add io apic setup before try_apic_pin
>
> cc: Andi Kleen <ak@suse.de>
> cc: Eric W. Biederman <ebiederm@xmission.com>
> Signed-off-by: Yinghai Lu <yinghai.lu@amd.com>
>
> diff --git a/arch/x86_64/kernel/io_apic.c b/arch/x86_64/kernel/io_apic.c
> index 2a1dcd5..6d09fc0 100644
> --- a/arch/x86_64/kernel/io_apic.c
> +++ b/arch/x86_64/kernel/io_apic.c
> @@ -273,10 +273,17 @@ static void add_pin_to_irq(unsigned int irq, int apic, int
> pin)
>  	struct irq_pin_list *entry = irq_2_pin + irq;
>  
>  	BUG_ON(irq >= NR_IRQS);
> -	while (entry->next)
> +	while (entry->next) {
> +		if (entry->apic == apic && entry->pin == pin) 
> +			return;
> +		if (entry->pin == -1) 
> +			break;
>  		entry = irq_2_pin + entry->next;
> +	}
>  
>  	if (entry->pin != -1) {
> +		if (entry->apic == apic && entry->pin == pin) 
> +			return;
>  		entry->next = first_free_entry;
>  		entry = irq_2_pin + entry->next;
>  		if (++first_free_entry >= PIN_MAP_SIZE)

This change to add_pin_to_irq looks dubious.

We especially shouldn't hit a pin == -1 while next is still valid.
The problem is that the code that reads this at irq time does not
skip entries with entry->pin == -1.

Fixing the infrastructure should probably be a separate patch
so we don't get too many concepts confused in here.

> @@ -286,6 +293,24 @@ static void add_pin_to_irq(unsigned int irq, int apic, int
> pin)
>  	entry->pin = pin;
>  }
>  
> +static void remove_pin_to_irq(unsigned int irq, int apic, int pin)
> +{
> +	struct irq_pin_list *entry = irq_2_pin + irq;
> +
> +	BUG_ON(irq >= NR_IRQS);
> +
> +	while (entry) {
> +		if (entry->apic == apic && entry->pin == pin) {
> +			entry->apic = -1;
> +			entry->pin = -1;
> +			break;
> +		}
> +		if (entry->next) 
> +			entry = irq_2_pin + entry->next;
> +	}
> +
> +}
> +
This change to remove_pin_to_irq is simply wrong.

> +static int add_irq_entry(int type, int irqflag, int bus, int irq, int apic, int
> pin)
> +{
> +        struct mpc_config_intsrc intsrc;
> +	int idx;
> +
> +        intsrc.mpc_type = MP_INTSRC;
> +        intsrc.mpc_irqflag = irqflag; /* conforming */
> +        intsrc.mpc_srcbus = bus;
> + intsrc.mpc_dstapic = (apic != -1) ? mp_ioapics[apic].mpc_apicid: MP_APIC_ALL;
> +
> +        intsrc.mpc_irqtype = type;
> +
> +        intsrc.mpc_srcbusirq = irq;
> +        intsrc.mpc_dstirq = pin;
> +
> +        mp_irqs [mp_irq_entries] = intsrc;
> +        Dprintk("Int: type %d, pol %d, trig %d, bus %d,"
> +                " IRQ %02x, APIC ID %x, APIC INT %02x\n",
> +                        intsrc.mpc_irqtype, intsrc.mpc_irqflag & 3,
> +                        (intsrc.mpc_irqflag >> 2) & 3, intsrc.mpc_srcbus,
> + intsrc.mpc_srcbusirq, intsrc.mpc_dstapic, intsrc.mpc_dstirq);
> +        idx = mp_irq_entries;
> +	if (++mp_irq_entries >= MAX_IRQ_SOURCES)
> +                panic("Max # of irq sources exceeded!!\n");
> +	return idx;

This is fairly sane but probably belongs in mptable.c as a helper.

>  /*
>   * Find the pin to which IRQ[irq] (ISA) is connected
>   */
> @@ -1570,6 +1658,22 @@ static inline void unlock_ExtINT_logic(void)
>   * fanatically on his truly buggy board.
>   */
>  
> +static void set_try_apic_pin(int apic, int pin, int type)
> +{
> +	int idx;
> +	int irq = 0;
> +	int bus = 0; /* MP_ISA_BUS */
> +	int irqflag = 5; /* MP_IRQ_TRIGGER_EDGE|MP_IRQ_POLARITY_HIGH */
> +
> +	idx = find_irq_entry(apic,pin,type);
> +
> +	if (idx == -1) 
> +		idx = add_irq_entry(type, irqflag, bus, irq, apic, pin);
> +
> +	add_pin_to_irq(irq, apic, pin);
> +	setup_IO_APIC_irq(apic, pin, idx, irq);
> +}
> +
>  static int try_apic_pin(int apic, int pin, char *msg)
>  {
>  	apic_printk(APIC_VERBOSE, KERN_INFO
> @@ -1588,7 +1692,7 @@ static int try_apic_pin(int apic, int pin, char *msg)
>  		}
>  		return 1;
>  	}
> -	clear_IO_APIC_pin(apic, pin);
> +
>  	apic_printk(APIC_QUIET, KERN_ERR " .. failed\n");
>  	return 0;
>  }
> @@ -1599,12 +1703,13 @@ static void check_timer(void)
>  	int apic1, pin1, apic2, pin2;
>  	int vector;
>  	cpumask_t mask;
> +	int i;
>  
>  	/*
>  	 * get/set the timer IRQ vector:
>  	 */
> -	disable_8259A_irq(0);
>  	vector = assign_irq_vector(0, TARGET_CPUS, &mask);
> +	disable_8259A_irq(0);

Moving disable_8259A_irq(0) appears to be useless code motion.
  
>  	/*
>  	 * Subtle, code in do_timer_interrupt() expects an AEOI
> @@ -1621,33 +1726,51 @@ static void check_timer(void)
>  	pin2  = ioapic_i8259.pin;
>  	apic2 = ioapic_i8259.apic;
>  
> -	/* Do this first, otherwise we get double interrupts on ATI boards */
> - if ((pin1 != -1) && try_apic_pin(apic1, pin1,"with 8259 IRQ0 disabled"))
> -		return;
> + apic_printk(APIC_VERBOSE,KERN_INFO "..TIMER: vector=0x%02X apic1=%d pin1=%d
> apic2=%d pin2=%d\n",
> +		vector, apic1, pin1, apic2, pin2);
>  
> -	/* Now try again with IRQ0 8259A enabled.
> -	   Assumes timer is on IO-APIC 0 ?!? */
> -	enable_8259A_irq(0);
> -	unmask_IO_APIC_irq(0);
> -	if (try_apic_pin(apic1, pin1, "with 8259 IRQ0 enabled"))
> -		return;
> -	disable_8259A_irq(0);
> +	if (pin1 != -1) {
> + /* Do this first, otherwise we get double interrupts on ATI boards */
> +		/* set_try_apic_pin will call disable_8259A_irq */
> +		set_try_apic_pin(apic1, pin1, mp_INT);
> +		unmask_IO_APIC_irq(0);
> +		if (try_apic_pin(apic1, pin1,"with 8259 IRQ0 disabled"))
> +			return;
>  
> -	/* Always try pin0 and pin2 on APIC 0 to handle buggy timer overrides
> -	   on Nvidia boards */
> -	if (!(apic1 == 0 && pin1 == 0) &&
> -	    try_apic_pin(0, 0, "fallback with 8259 IRQ0 disabled"))
> -		return;
> -	if (!(apic1 == 0 && pin1 == 2) &&
> -	    try_apic_pin(0, 2, "fallback with 8259 IRQ0 disabled"))
> -		return;
> +		/* Now try again with IRQ0 8259A enabled.
> +		   Assumes timer is on IO-APIC 0 ?!? */
> +		enable_8259A_irq(0);
> +		if (try_apic_pin(apic1, pin1, "with 8259 IRQ0 enabled"))
> +			return;
> +		disable_8259A_irq(0);

I am still trying to understand this enable_8259A_irq(0) case.
As far as I can tell this is a very backwards way of enabling
an ExtINT, as such it shouldn't be used until later.

YH do you have any insight why on some Nvidia chipsets we apic 0 pin 2 doesn't
work for the timer interrupt.  I thought that was what we were using in LinuxBIOS
for the mptable.

Eric

Re: IO-APIC + timer doesn't work

[Tobias Diedrich]

Yinghai Lu wrote:
> On 12/19/06, Eric W. Biederman <ebiederm@xmission.com> wrote:
> >So the pin2 case should be tested right after the pin1 case as we do
> >currently.  On most new boards that will be a complete noop.
> >
> >But it is better than our current blind guess at using ExtINT mode.
> >
> >I figure after we try what the BIOS has told us about and that
> >has failed we should first try the common irq 0 apic mappings,
> >and then try the common ExtINT mappings.
> 
> Please check if this one is ok.

Works fine for me.

FYI I'm off to my parents from Saturday onward, so after that I
can't test any patches for the next one or two weeks.

-- 
Tobias						PGP: http://9ac7e0bc.uguu.de
このメールは十割再利用されたビットで作られています。

Re: IO-APIC + timer doesn't work

[Yinghai Lu]

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

On 12/19/06, Eric W. Biederman <ebiederm@xmission.com> wrote:
> So the pin2 case should be tested right after the pin1 case as we do
> currently.  On most new boards that will be a complete noop.
>
> But it is better than our current blind guess at using ExtINT mode.
>
> I figure after we try what the BIOS has told us about and that
> has failed we should first try the common irq 0 apic mappings,
> and then try the common ExtINT mappings.

Please check if this one is ok.

[-- Attachment #2: timers_12192006.patch --]
[-- Type: text/x-patch, Size: 6199 bytes --]

[PATCH] x86_64: check_timer with io apic setup before try_apic_pin

add io apic setup before try_apic_pin

cc: Andi Kleen <ak@suse.de>
cc: Eric W. Biederman <ebiederm@xmission.com>
Signed-off-by: Yinghai Lu <yinghai.lu@amd.com>

diff --git a/arch/x86_64/kernel/io_apic.c b/arch/x86_64/kernel/io_apic.c
index 2a1dcd5..6d09fc0 100644
--- a/arch/x86_64/kernel/io_apic.c
+++ b/arch/x86_64/kernel/io_apic.c
@@ -273,10 +273,17 @@ static void add_pin_to_irq(unsigned int irq, int apic, int pin)
 	struct irq_pin_list *entry = irq_2_pin + irq;
 
 	BUG_ON(irq >= NR_IRQS);
-	while (entry->next)
+	while (entry->next) {
+		if (entry->apic == apic && entry->pin == pin) 
+			return;
+		if (entry->pin == -1) 
+			break;
 		entry = irq_2_pin + entry->next;
+	}
 
 	if (entry->pin != -1) {
+		if (entry->apic == apic && entry->pin == pin) 
+			return;
 		entry->next = first_free_entry;
 		entry = irq_2_pin + entry->next;
 		if (++first_free_entry >= PIN_MAP_SIZE)
@@ -286,6 +293,24 @@ static void add_pin_to_irq(unsigned int irq, int apic, int pin)
 	entry->pin = pin;
 }
 
+static void remove_pin_to_irq(unsigned int irq, int apic, int pin)
+{
+	struct irq_pin_list *entry = irq_2_pin + irq;
+
+	BUG_ON(irq >= NR_IRQS);
+
+	while (entry) {
+		if (entry->apic == apic && entry->pin == pin) {
+			entry->apic = -1;
+			entry->pin = -1;
+			break;
+		}
+		if (entry->next) 
+			entry = irq_2_pin + entry->next;
+	}
+
+}
+
 
 #define DO_ACTION(name,R,ACTION, FINAL)					\
 									\
@@ -367,6 +392,34 @@ static int find_irq_entry(int apic, int pin, int type)
 	return -1;
 }
 
+static int add_irq_entry(int type, int irqflag, int bus, int irq, int apic, int pin)
+{
+        struct mpc_config_intsrc intsrc;
+	int idx;
+
+        intsrc.mpc_type = MP_INTSRC;
+        intsrc.mpc_irqflag = irqflag; /* conforming */
+        intsrc.mpc_srcbus = bus;
+        intsrc.mpc_dstapic = (apic != -1) ? mp_ioapics[apic].mpc_apicid: MP_APIC_ALL;
+
+        intsrc.mpc_irqtype = type;
+
+        intsrc.mpc_srcbusirq = irq;
+        intsrc.mpc_dstirq = pin;
+
+        mp_irqs [mp_irq_entries] = intsrc;
+        Dprintk("Int: type %d, pol %d, trig %d, bus %d,"
+                " IRQ %02x, APIC ID %x, APIC INT %02x\n",
+                        intsrc.mpc_irqtype, intsrc.mpc_irqflag & 3,
+                        (intsrc.mpc_irqflag >> 2) & 3, intsrc.mpc_srcbus,
+                        intsrc.mpc_srcbusirq, intsrc.mpc_dstapic, intsrc.mpc_dstirq);
+        idx = mp_irq_entries;
+	if (++mp_irq_entries >= MAX_IRQ_SOURCES)
+                panic("Max # of irq sources exceeded!!\n");
+	return idx;
+
+}
+
 /*
  * Find the pin to which IRQ[irq] (ISA) is connected
  */
@@ -1570,6 +1658,22 @@ static inline void unlock_ExtINT_logic(void)
  * fanatically on his truly buggy board.
  */
 
+static void set_try_apic_pin(int apic, int pin, int type)
+{
+	int idx;
+	int irq = 0;
+	int bus = 0; /* MP_ISA_BUS */
+	int irqflag = 5; /* MP_IRQ_TRIGGER_EDGE|MP_IRQ_POLARITY_HIGH */
+
+	idx = find_irq_entry(apic,pin,type);
+
+	if (idx == -1) 
+		idx = add_irq_entry(type, irqflag, bus, irq, apic, pin);
+
+	add_pin_to_irq(irq, apic, pin);
+	setup_IO_APIC_irq(apic, pin, idx, irq);
+}
+
 static int try_apic_pin(int apic, int pin, char *msg)
 {
 	apic_printk(APIC_VERBOSE, KERN_INFO
@@ -1588,7 +1692,7 @@ static int try_apic_pin(int apic, int pin, char *msg)
 		}
 		return 1;
 	}
-	clear_IO_APIC_pin(apic, pin);
+
 	apic_printk(APIC_QUIET, KERN_ERR " .. failed\n");
 	return 0;
 }
@@ -1599,12 +1703,13 @@ static void check_timer(void)
 	int apic1, pin1, apic2, pin2;
 	int vector;
 	cpumask_t mask;
+	int i;
 
 	/*
 	 * get/set the timer IRQ vector:
 	 */
-	disable_8259A_irq(0);
 	vector = assign_irq_vector(0, TARGET_CPUS, &mask);
+	disable_8259A_irq(0);
 
 	/*
 	 * Subtle, code in do_timer_interrupt() expects an AEOI
@@ -1621,33 +1726,51 @@ static void check_timer(void)
 	pin2  = ioapic_i8259.pin;
 	apic2 = ioapic_i8259.apic;
 
-	/* Do this first, otherwise we get double interrupts on ATI boards */
-	if ((pin1 != -1) && try_apic_pin(apic1, pin1,"with 8259 IRQ0 disabled"))
-		return;
+	apic_printk(APIC_VERBOSE,KERN_INFO "..TIMER: vector=0x%02X apic1=%d pin1=%d apic2=%d pin2=%d\n",
+		vector, apic1, pin1, apic2, pin2);
 
-	/* Now try again with IRQ0 8259A enabled.
-	   Assumes timer is on IO-APIC 0 ?!? */
-	enable_8259A_irq(0);
-	unmask_IO_APIC_irq(0);
-	if (try_apic_pin(apic1, pin1, "with 8259 IRQ0 enabled"))
-		return;
-	disable_8259A_irq(0);
+	if (pin1 != -1) {
+		/* Do this first, otherwise we get double interrupts on ATI boards */
+		/* set_try_apic_pin will call disable_8259A_irq */
+		set_try_apic_pin(apic1, pin1, mp_INT);
+		unmask_IO_APIC_irq(0);
+		if (try_apic_pin(apic1, pin1,"with 8259 IRQ0 disabled"))
+			return;
 
-	/* Always try pin0 and pin2 on APIC 0 to handle buggy timer overrides
-	   on Nvidia boards */
-	if (!(apic1 == 0 && pin1 == 0) &&
-	    try_apic_pin(0, 0, "fallback with 8259 IRQ0 disabled"))
-		return;
-	if (!(apic1 == 0 && pin1 == 2) &&
-	    try_apic_pin(0, 2, "fallback with 8259 IRQ0 disabled"))
-		return;
+		/* Now try again with IRQ0 8259A enabled.
+		   Assumes timer is on IO-APIC 0 ?!? */
+		enable_8259A_irq(0);
+		if (try_apic_pin(apic1, pin1, "with 8259 IRQ0 enabled"))
+			return;
+		disable_8259A_irq(0);
+
+    	        clear_IO_APIC_pin(apic1, pin1);
+		remove_pin_to_irq(0, apic1, pin1);
+	}
 
 	/* Then try pure 8259A routing on the 8259 as reported by BIOS*/
-	enable_8259A_irq(0);
 	if (pin2 != -1) {
 		setup_ExtINT_IRQ0_pin(apic2, pin2, vector);
+		add_pin_to_irq(0, apic2, pin2);
+		enable_8259A_irq(0);
 		if (try_apic_pin(apic2,pin2,"8259A broadcast ExtINT from BIOS"))
 			return;
+		clear_IO_APIC_pin(apic2, pin2);
+		remove_pin_to_irq(0, apic2, pin2);
+	}
+
+	/* Always try pin0 and pin2 on APIC 0 to handle buggy timer overrides
+	   on Nvidia boards */
+	for (i = 0; i <= 2; i += 2) 
+	if (!(apic1 == 0 && pin1 == i)) {
+		/* set_try_apic_pin will call disable_8259A_irq */
+		set_try_apic_pin(0, i, mp_INT); 
+		unmask_IO_APIC_irq(0);
+		if (try_apic_pin(0, i, "fallback with 8259 IRQ0 disabled"))
+			return;
+
+		clear_IO_APIC_pin(0, i);
+		remove_pin_to_irq(0, 0, i);
 	}
 
 	/* Tried all possibilities to go through the IO-APIC. Now come the

Re: IO-APIC + timer doesn't work

[Eric W. Biederman]

"Yinghai Lu" <yinghai.lu@amd.com> writes:

> On 12/18/06, Eric W. Biederman <ebiederm@xmission.com> wrote:
>> Thanks.  The bug is simply that the new code doesn't setup the
>> ioapic for the cases it intends to test.  But it does clear out
>> the original programming.  So if the normal good case doesn't work the
>> code is going to have problems.
>
> Please check the patch.

Getting there but I don't think we are quite there yet.

One of the issues that this does not address is that currently our probe
order in check_timer is wrong.  We should first check what the BIOS
has told us about.  And only if that fails should we start guessing,
common configurations.

So the pin2 case should be tested right after the pin1 case as we do
currently.  On most new boards that will be a complete noop.

But it is better than our current blind guess at using ExtINT mode.

I figure after we try what the BIOS has told us about and that
has failed we should first try the common irq 0 apic mappings,
and then try the common ExtINT mappings.

The current code causes me to want to scream, it is so silly.

Eric

Re: IO-APIC + timer doesn't work

[Yinghai Lu]

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

On 12/18/06, Eric W. Biederman <ebiederm@xmission.com> wrote:
> Thanks.  The bug is simply that the new code doesn't setup the
> ioapic for the cases it intends to test.  But it does clear out
> the original programming.  So if the normal good case doesn't work the
> code is going to have problems.

Please check the patch.

[-- Attachment #2: timers_12182006.patch --]
[-- Type: text/x-patch, Size: 4598 bytes --]

[PATCH] x86_64: check_timer with io apic setup before try_apic_pin

add io apic setup before try_apic_pin

cc: Andi Kleen <ak@suse.de>
cc: Eric W. Biederman <ebiederm@xmission.com>
Signed-off-by: Yinghai Lu <yinghai.lu@amd.com>

diff --git a/arch/x86_64/kernel/io_apic.c b/arch/x86_64/kernel/io_apic.c
index 2a1dcd5..06982b4 100644
--- a/arch/x86_64/kernel/io_apic.c
+++ b/arch/x86_64/kernel/io_apic.c
@@ -273,10 +273,17 @@ static void add_pin_to_irq(unsigned int irq, int apic, int pin)
 	struct irq_pin_list *entry = irq_2_pin + irq;
 
 	BUG_ON(irq >= NR_IRQS);
-	while (entry->next)
+	while (entry->next) {
+		if (entry->apic == apic && entry->pin == pin) 
+			return;
+		if (entry->pin == -1) 
+			break;
 		entry = irq_2_pin + entry->next;
+	}
 
 	if (entry->pin != -1) {
+		if (entry->apic == apic && entry->pin == pin) 
+			return;
 		entry->next = first_free_entry;
 		entry = irq_2_pin + entry->next;
 		if (++first_free_entry >= PIN_MAP_SIZE)
@@ -286,6 +293,24 @@ static void add_pin_to_irq(unsigned int irq, int apic, int pin)
 	entry->pin = pin;
 }
 
+static void remove_pin_to_irq(unsigned int irq, int apic, int pin)
+{
+	struct irq_pin_list *entry = irq_2_pin + irq;
+
+	BUG_ON(irq >= NR_IRQS);
+
+	while (entry) {
+		if (entry->apic == apic && entry->pin == pin) {
+			entry->apic = -1;
+			entry->pin = -1;
+			break;
+		}
+		if (entry->next) 
+			entry = irq_2_pin + entry->next;
+	}
+
+}
+
 
 #define DO_ACTION(name,R,ACTION, FINAL)					\
 									\
@@ -1570,6 +1630,21 @@ static inline void unlock_ExtINT_logic(void)
  * fanatically on his truly buggy board.
  */
 
+static int set_try_apic_pin(int apic, int pin, int irq)
+{
+	int idx;
+	int ret = -1;
+	idx = find_irq_entry(apic,pin,mp_INT);
+
+	if(idx != -1) {
+		add_pin_to_irq(irq, apic, pin);
+		setup_IO_APIC_irq(apic, pin, idx, irq);
+		ret = 0;
+	}
+
+	return ret;
+}
+
 static int try_apic_pin(int apic, int pin, char *msg)
 {
 	apic_printk(APIC_VERBOSE, KERN_INFO
@@ -1588,7 +1663,7 @@ static int try_apic_pin(int apic, int pin, char *msg)
 		}
 		return 1;
 	}
-	clear_IO_APIC_pin(apic, pin);
+
 	apic_printk(APIC_QUIET, KERN_ERR " .. failed\n");
 	return 0;
 }
@@ -1599,12 +1674,12 @@ static void check_timer(void)
 	int apic1, pin1, apic2, pin2;
 	int vector;
 	cpumask_t mask;
+	int i;
 
 	/*
 	 * get/set the timer IRQ vector:
 	 */
 	disable_8259A_irq(0);
-	vector = assign_irq_vector(0, TARGET_CPUS, &mask);
 
 	/*
 	 * Subtle, code in do_timer_interrupt() expects an AEOI
@@ -1622,32 +1697,49 @@ static void check_timer(void)
 	apic2 = ioapic_i8259.apic;
 
 	/* Do this first, otherwise we get double interrupts on ATI boards */
-	if ((pin1 != -1) && try_apic_pin(apic1, pin1,"with 8259 IRQ0 disabled"))
-		return;
+	if (pin1 != -1) {
+		/* set_try_apic_pin will call disable_8259A_irq */
+		set_try_apic_pin(apic1, pin1, 0);
+		unmask_IO_APIC_irq(0);
+		if (try_apic_pin(apic1, pin1,"with 8259 IRQ0 disabled"))
+			return;
 
-	/* Now try again with IRQ0 8259A enabled.
-	   Assumes timer is on IO-APIC 0 ?!? */
-	enable_8259A_irq(0);
-	unmask_IO_APIC_irq(0);
-	if (try_apic_pin(apic1, pin1, "with 8259 IRQ0 enabled"))
-		return;
-	disable_8259A_irq(0);
+		/* Now try again with IRQ0 8259A enabled.
+		   Assumes timer is on IO-APIC 0 ?!? */
+		enable_8259A_irq(0);
+		if (try_apic_pin(apic1, pin1, "with 8259 IRQ0 enabled"))
+			return;
+		disable_8259A_irq(0);
+
+    	        clear_IO_APIC_pin(apic1, pin1);
+		remove_pin_to_irq(0, apic1, pin1);
+	}
 
 	/* Always try pin0 and pin2 on APIC 0 to handle buggy timer overrides
 	   on Nvidia boards */
-	if (!(apic1 == 0 && pin1 == 0) &&
-	    try_apic_pin(0, 0, "fallback with 8259 IRQ0 disabled"))
-		return;
-	if (!(apic1 == 0 && pin1 == 2) &&
-	    try_apic_pin(0, 2, "fallback with 8259 IRQ0 disabled"))
-		return;
+	for (i = 0; i <= 2; i += 2) 
+	if (!(apic1 == 0 && pin1 == i)) {
+		/* set_try_apic_pin will call disable_8259A_irq */
+		if (!set_try_apic_pin(0, i, 0) ) {
+			unmask_IO_APIC_irq(0);
+			if (try_apic_pin(0, i, "fallback with 8259 IRQ0 disabled"))
+				return;
+			clear_IO_APIC_pin(0, i);
+			remove_pin_to_irq(0, 0, i);
+	    	}
+	}
+
+	vector = assign_irq_vector(0, TARGET_CPUS, &mask);
 
 	/* Then try pure 8259A routing on the 8259 as reported by BIOS*/
-	enable_8259A_irq(0);
 	if (pin2 != -1) {
 		setup_ExtINT_IRQ0_pin(apic2, pin2, vector);
+		add_pin_to_irq(0, apic2, pin2);
+		enable_8259A_irq(0);
 		if (try_apic_pin(apic2,pin2,"8259A broadcast ExtINT from BIOS"))
 			return;
+		clear_IO_APIC_pin(apic2, pin2);
+		remove_pin_to_irq(0, apic2, pin2);
 	}
 
 	/* Tried all possibilities to go through the IO-APIC. Now come the

Re: IO-APIC + timer doesn't work

[Eric W. Biederman]

Tobias Diedrich <ranma+kernel@tdiedrich.de> writes:
> Eric W. Biederman wrote:
>> Could you try removing the clear_IO_APIC_pin from try_io_apic_pin.
>> 
>> This isn't a complete fix but I believe for your hardware it will
>> fix the problem and it points at what the real fix is.  
>> 
>> Not properly programming the io_apic for the case we want to test.
>
> Yes, this works:

Thanks.  The bug is simply that the new code doesn't setup the
ioapic for the cases it intends to test.  But it does clear out
the original programming.  So if the normal good case doesn't work the
code is going to have problems.


> I can also report, that updating the BIOS to version 0609 (released
> last week or so, also adds the long-missing HPET support) also makes
> the problem go away since the first testcase then already works.
> I'm currently running with the BIOS downgraded to version 0402.

Nice to hear, so this is clearly a software setup problem in the BIOS.

Andi do you think you could address this problem?

Eric

Re: IO-APIC + timer doesn't work

[Len Brown]

On Sunday 17 December 2006 00:22, Eric W. Biederman wrote:

> Actually can anyone tell me how try_apic_pin is supposed to work at
> all?
> 
> It doesn't appear to be programming the io_apic.

magic:-)

ACPI can't even _describe_ the scenarios being tried by check_timer(),
which is trying to navigate the minefield of all possible undocumented
chipset dependent bugs.  (ie, tinkering with the PIT when in IOAPIC mode...)

The chipset vendors can create new bugs in this area
faster than we can fix them, and there is a reason for this.

The public info on Windows says that they use 100HZ IRQ0 8254 only for UP.
On SMP, they use 64 HZ RTC on IRQ8 instead.

This means that for the population of system vendors that validate only with Windows,
only those timers are getting validated, and Linux on IRQ0 is exposed to HW bugs.

So the RTC looks like a safe path for a validated periodic ticker
when our first choice doesn't work.  But the RTC isn't without problems.
It can tick only in powers of 2 HZ, and 100/250/300/1000 are not powers of 2.
Dunno if close counts -- 256 is close to our 250, and 1024 is close to our 1000,
but we don't have any choices close to 100 or 300 HZ.

-Len

ps.
Moving to the RTC from the PIT would move us 3 years forward
in hardware technology, from 1981 to 1984:-)

Re: IO-APIC + timer doesn't work

[Linus Torvalds]

On Sun, 17 Dec 2006, Tobias Diedrich wrote:
> 
> BTW, I'm also wondering if this secondary Oops is supposed to happen:

Well, if the timer doesn't work, then the NMI watchdog will trigger. So 
it's "supposed" to happen in the sense that yeah, it's kind of expected, 
but it's really bsically just a secondary issue. If the timer worked 
properly, you'd never see it. 

So it's just fallout from the original problem you have, and not 
interesting in itself.

		Linus

Re: IO-APIC + timer doesn't work

[Tobias Diedrich]

Linus Torvalds wrote:
> On Sun, 17 Dec 2006, Tobias Diedrich wrote:
> > 
> > No such luck, it still panics and the APIC error is also unchanged.
> 
> Ok. I don't see anything wrong off-hand, but I'll keep the patch in the 
> tree in the hopes that Andi and/or Eric can see what's wrong and solve it.
> 
> If we don't find a solution, I'll have to revert it, but let's give it a 
> few more days. 
> 
> Tobias, can you please make sure to remind me about this if nothing seems 
> to happen? 

Sure.

BTW, I'm also wondering if this secondary Oops is supposed to happen:
http://www.tdiedrich.de/~ranma/2.6.20-rc1-oops2.jpg
I guess the NMI watchdog is never disabled after the test failed?

|[68.908000] Kernel panic - not syncing: IO-APIC + timer doesn't work! Try using the 'noapic' kernel parameter
|[68.908002]
[~4 seconds later]
|[68.908300] NMI Watchdog detected LOCKUP on CPU 0
  ^^^^^^^^^ wrong timestamp?
|[73.637325] CPU 0
|[73.637451] Modules linked in:
|[73.637579] Pid: 1, comm: swapper Not tainted 2.6.20-rc1-amd64 #27
[...]

Re: IO-APIC + timer doesn't work

[Eric W. Biederman]

Linus Torvalds <torvalds@osdl.org> writes:

> On Sun, 17 Dec 2006, Tobias Diedrich wrote:
>> 
>> No such luck, it still panics and the APIC error is also unchanged.
>
> Ok. I don't see anything wrong off-hand, but I'll keep the patch in the 
> tree in the hopes that Andi and/or Eric can see what's wrong and solve it.
>
> If we don't find a solution, I'll have to revert it, but let's give it a 
> few more days. 
>
> Tobias, can you please make sure to remind me about this if nothing seems 
> to happen? 

Actually can anyone tell me how try_apic_pin is supposed to work at
all?

It doesn't appear to be programming the io_apic.

So either I am missing something or I have found a real problem.

Eric

Re: IO-APIC + timer doesn't work

[Eric W. Biederman]

Linus Torvalds <torvalds@osdl.org> writes:

> On Sun, 17 Dec 2006, Tobias Diedrich wrote:
>> 
>> No such luck, it still panics and the APIC error is also unchanged.
>
> Ok. I don't see anything wrong off-hand, but I'll keep the patch in the 
> tree in the hopes that Andi and/or Eric can see what's wrong and solve it.
>
> If we don't find a solution, I'll have to revert it, but let's give it a 
> few more days. 
>
> Tobias, can you please make sure to remind me about this if nothing seems 
> to happen? 

Just skimming for differences the first test seems to be missing an
umask_IO_APIC_irq(0);

It would be good to know which case is working before this change was made.

Eric

Re: IO-APIC + timer doesn't work

[Linus Torvalds]

On Sun, 17 Dec 2006, Tobias Diedrich wrote:
> 
> No such luck, it still panics and the APIC error is also unchanged.

Ok. I don't see anything wrong off-hand, but I'll keep the patch in the 
tree in the hopes that Andi and/or Eric can see what's wrong and solve it.

If we don't find a solution, I'll have to revert it, but let's give it a 
few more days. 

Tobias, can you please make sure to remind me about this if nothing seems 
to happen? 

Thanks,

			Linus