[PATCH 1/5] Skip timer works.patch

[PATCH 1/5] Skip timer works.patch

[Zachary Amsden]

Add a way to disable the timer IRQ routing check via a boot option.  The
VMI timer code uses this to avoid triggering the pester Mingo code, which
probes for some very unusual and broken motherboard routings.  It fires
100% of the time when using a paravirtual delay mechanism instead of
using a realtime delay, since there is no elapsed real time, and the 4 timer
IRQs have not yet been delivered.

In addition, it is entirely possible, though improbable, that this bug
could surface on real hardware which picks a particularly bad time to enter
SMM mode, causing a long latency during one of the timer IRQs.

While here, make check_timer be __init.

Signed-off-by: Zachary Amsden <zach@vmware.com>

===================================================================
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -603,8 +603,6 @@ and is between 256 and 4096 characters. 
 
 	hugepages=	[HW,IA-32,IA-64] Maximal number of HugeTLB pages.
 
-	noirqbalance	[IA-32,SMP,KNL] Disable kernel irq balancing
-
 	i8042.direct	[HW] Put keyboard port into non-translated mode
 	i8042.dumbkbd	[HW] Pretend that controller can only read data from
 			     keyboard and cannot control its state
@@ -1060,8 +1058,13 @@ and is between 256 and 4096 characters. 
 			in certain environments such as networked servers or
 			real-time systems.
 
+	noirqbalance	[IA-32,SMP,KNL] Disable kernel irq balancing
+
 	noirqdebug	[IA-32] Disables the code which attempts to detect and
 			disable unhandled interrupt sources.
+
+	noirqtest	[IA-32,APIC] Disables the code which tests for broken
+			timer IRQ sources.
 
 	noisapnp	[ISAPNP] Disables ISA PnP code.
 
===================================================================
--- a/arch/i386/kernel/io_apic.c
+++ b/arch/i386/kernel/io_apic.c
@@ -1864,6 +1864,15 @@ static void __init setup_ioapic_ids_from
 static void __init setup_ioapic_ids_from_mpc(void) { }
 #endif
 
+int timer_irq_really_works __initdata;
+int __init irqtest_disable(char *str)
+{
+	timer_irq_really_works = 1;
+	return 1;
+}
+
+__setup("noirqtest", irqtest_disable);
+
 /*
  * There is a nasty bug in some older SMP boards, their mptable lies
  * about the timer IRQ. We do the following to work around the situation:
@@ -1872,9 +1881,12 @@ static void __init setup_ioapic_ids_from
  *	- if this function detects that timer IRQs are defunct, then we fall
  *	  back to ISA timer IRQs
  */
-static int __init timer_irq_works(void)
+int __init timer_irq_works(void)
 {
 	unsigned long t1 = jiffies;
+
+	if (timer_irq_really_works)
+		return 1;
 
 	local_irq_enable();
 	/* Let ten ticks pass... */
@@ -2146,7 +2158,7 @@ int timer_uses_ioapic_pin_0;
  * is so screwy.  Thanks to Brian Perkins for testing/hacking this beast
  * fanatically on his truly buggy board.
  */
-static inline void check_timer(void)
+static inline void __init check_timer(void)
 {
 	int apic1, pin1, apic2, pin2;
 	int vector;

Re: [PATCH 1/5] Skip timer works.patch

[Andi Kleen]

On Thu, Oct 19, 2006 at 05:09:22PM -0700, Zachary Amsden wrote:
> Add a way to disable the timer IRQ routing check via a boot option.  The
> VMI timer code uses this to avoid triggering the pester Mingo code, which
> probes for some very unusual and broken motherboard routings.  It fires
> 100% of the time when using a paravirtual delay mechanism instead of
> using a realtime delay, since there is no elapsed real time, and the 4 timer
> IRQs have not yet been delivered.

You mean paravirtualized udelay will not actually wait? 

This implies that you can't ever use any real timer in that kind of guest,
right?

> 
> In addition, it is entirely possible, though improbable, that this bug
> could surface on real hardware which picks a particularly bad time to enter
> SMM mode, causing a long latency during one of the timer IRQs.

We already have a no timer check option. But:

> 
> While here, make check_timer be __init.

So how is this supposed to work? The hypervisor would always pass that 
option?  If yes that would seem rather hackish to me. We should probably
instead probe in some way if we have the required timer hardware.
The paravirt kernel should know anyways it is paravirt and that it doesn't
need to probe for flakey hardware.

-Andi

Re: [PATCH 1/5] Skip timer works.patch

[Zachary Amsden]

Andi Kleen wrote:
> On Thu, Oct 19, 2006 at 05:09:22PM -0700, Zachary Amsden wrote:
>   
>> Add a way to disable the timer IRQ routing check via a boot option.  The
>> VMI timer code uses this to avoid triggering the pester Mingo code, which
>> probes for some very unusual and broken motherboard routings.  It fires
>> 100% of the time when using a paravirtual delay mechanism instead of
>> using a realtime delay, since there is no elapsed real time, and the 4 timer
>> IRQs have not yet been delivered.
>>     
>
> You mean paravirtualized udelay will not actually wait? 
>   

Yes, but even putting that problem aside, the timing element here is 
tricky to get right in a VM.

> This implies that you can't ever use any real timer in that kind of guest,
> right?
>   

No.  You can use a real timer just fine.  But there is no reason ever to 
use udelay to busy wait for "hardware" in a virtual machine.  Drivers 
which are used for real hardware may turn udelay back on selectively; 
but this is another patch.


>> In addition, it is entirely possible, though improbable, that this bug
>> could surface on real hardware which picks a particularly bad time to enter
>> SMM mode, causing a long latency during one of the timer IRQs.
>>     
>
> We already have a no timer check option. But:
>   

Really?  I didn't see one that disabled the broken motherboard detection 
/ workaround code, which is what we are trying to avoid here.


>> While here, make check_timer be __init.
>>     
>
> So how is this supposed to work? The hypervisor would always pass that 
> option?  If yes that would seem rather hackish to me. We should probably
> instead probe in some way if we have the required timer hardware.
> The paravirt kernel should know anyways it is paravirt and that it doesn't
> need to probe for flakey hardware.
>   

That is what this patch is building towards, but the boot option is 
"free", so why not?  In the meantime, it helps non-paravirt kernels 
booted in a VM.

Zach

Re: [PATCH 1/5] Skip timer works.patch

[Andi Kleen]

On Friday 27 October 2006 12:09, Zachary Amsden wrote:
> Andi Kleen wrote:
> > On Thu, Oct 19, 2006 at 05:09:22PM -0700, Zachary Amsden wrote:
> >> Add a way to disable the timer IRQ routing check via a boot option.  The
> >> VMI timer code uses this to avoid triggering the pester Mingo code,
> >> which probes for some very unusual and broken motherboard routings.  It
> >> fires 100% of the time when using a paravirtual delay mechanism instead
> >> of using a realtime delay, since there is no elapsed real time, and the
> >> 4 timer IRQs have not yet been delivered.
> >
> > You mean paravirtualized udelay will not actually wait?
>
> Yes, but even putting that problem aside, the timing element here is
> tricky to get right in a VM.
>
> > This implies that you can't ever use any real timer in that kind of
> > guest, right?
>
> No.  You can use a real timer just fine.  But there is no reason ever to
> use udelay to busy wait for "hardware" in a virtual machine.  Drivers
> which are used for real hardware may turn udelay back on selectively;
> but this is another patch.
>
> >> In addition, it is entirely possible, though improbable, that this bug
> >> could surface on real hardware which picks a particularly bad time to
> >> enter SMM mode, causing a long latency during one of the timer IRQs.
> >
> > We already have a no timer check option. But:
>
> Really?  I didn't see one that disabled the broken motherboard detection
> / workaround code, which is what we are trying to avoid here.

no_timer_check. But it's only there on x86-64 in mainline - although there
were some patches to add it to i386 too.

> That is what this patch is building towards, but the boot option is
> "free", so why not?  In the meantime, it helps non-paravirt kernels
> booted in a VM.

Hmm, you meant they paniced before?  If they just fail a few tests
that is not particularly worrying (real hardware does that often too)

-Andi

Re: [PATCH 1/5] Skip timer works.patch

[Zachary Amsden]

Andi Kleen wrote:
> no_timer_check. But it's only there on x86-64 in mainline - although there
> were some patches to add it to i386 too.
>   

I can rename to match the x86-64 name.


>> That is what this patch is building towards, but the boot option is
>> "free", so why not?  In the meantime, it helps non-paravirt kernels
>> booted in a VM.
>>     
>
> Hmm, you meant they paniced before?  If they just fail a few tests
> that is not particularly worrying (real hardware does that often too)
>   

Yes, they sometimes fail to boot, and the failure message used to ask us 
to pester mingo.

Zach

Re: [PATCH 1/5] Skip timer works.patch

[Andi Kleen]

On Mon, Oct 30, 2006 at 12:54:55PM -0800, Zachary Amsden wrote:
> Andi Kleen wrote:
> >no_timer_check. But it's only there on x86-64 in mainline - although there
> >were some patches to add it to i386 too.
> >  
> 
> I can rename to match the x86-64 name.

I will do that in my tree.

> >>That is what this patch is building towards, but the boot option is
> >>"free", so why not?  In the meantime, it helps non-paravirt kernels
> >>booted in a VM.
> >>    
> >
> >Hmm, you meant they paniced before?  If they just fail a few tests
> >that is not particularly worrying (real hardware does that often too)
> >  
> 
> Yes, they sometimes fail to boot, and the failure message used to ask us 
> to pester mingo.

I still think we should figure that out automatically. Letting
the Hypervisor pass magic boot options seems somehow unclean.

But i suppose it will only work for the paravirtualized case,
not for the case of kernel running "native" under a hypervisor
I suppose? Or does that one not panic?


-Andi

Re: [PATCH 1/5] Skip timer works.patch

[Zachary Amsden]

Andi Kleen wrote:
> On Mon, Oct 30, 2006 at 12:54:55PM -0800, Zachary Amsden wrote:
>   
>> Andi Kleen wrote:
>>     
>>> no_timer_check. But it's only there on x86-64 in mainline - although there
>>> were some patches to add it to i386 too.
>>>  
>>>       
>> I can rename to match the x86-64 name.
>>     
>
> I will do that in my tree.
>
>   
>>>> That is what this patch is building towards, but the boot option is
>>>> "free", so why not?  In the meantime, it helps non-paravirt kernels
>>>> booted in a VM.
>>>>    
>>>>         
>>> Hmm, you meant they paniced before?  If they just fail a few tests
>>> that is not particularly worrying (real hardware does that often too)
>>>  
>>>       
>> Yes, they sometimes fail to boot, and the failure message used to ask us 
>> to pester mingo.
>>     
>
> I still think we should figure that out automatically. Letting
> the Hypervisor pass magic boot options seems somehow unclean.
>
> But i suppose it will only work for the paravirtualized case,
> not for the case of kernel running "native" under a hypervisor
> I suppose? Or does that one not panic?
>   

That is the one that can panic, for now.  Fixing the paravirtualized 
case is easy, but we can't assume paravirtualization just yet.

Zach

Re: [PATCH 1/5] Skip timer works.patch

[Andi Kleen]

> That is the one that can panic, for now.  Fixing the paravirtualized 
> case is easy, but we can't assume paravirtualization just yet.

Hmm, this means standard vmware boot is not reliable unless that magic option
is set?   That doesn't sound good.  

-Andi

Re: [PATCH 1/5] Skip timer works.patch

[Zachary Amsden]

Andi Kleen wrote:
>> That is the one that can panic, for now.  Fixing the paravirtualized 
>> case is easy, but we can't assume paravirtualization just yet.
>>     
>
> Hmm, this means standard vmware boot is not reliable unless that magic option
> is set?   That doesn't sound good.  
>   

It doesn't happen often, but it is a possibility that the kernel 
calibrates the delay wrong because of timing glitches caused by CPU 
migration, paging, or other phenomena which are supposed to be 
transparent to the kernel (but cause temporal lapse).  In that case, the 
kernel may not make enough progress in a spin delay loop to properly 
reach the number of microseconds required for N number of timer ticks to 
occur.  In theory this can happen on a real machine, as SMM mode could 
be active, doing USB device emulation or something that takes a while 
during the lpj calibration and throwing the computation off.

By changing the parameters (N ticks at K Hz in T seconds), it is easy to 
create an unstable measurement that can achieve high failure rates, 
although in practice the Linux parameters appear to be reasonable enough 
that it is not a major problem.

Zach

Re: [PATCH 1/5] Skip timer works.patch

[Andi Kleen]

> It doesn't happen often, but it is a possibility that the kernel 
> calibrates the delay wrong because of timing glitches caused by CPU 
> migration, paging, or other phenomena which are supposed to be 
> transparent to the kernel (but cause temporal lapse).

We're supposed to handle those because they happen on real hardware
too with long running SMM handlers. Or at least there was a effort some time ago
to do this. If it wasn't enough we'll likely need to fix the code.

> In that case, the  
> kernel may not make enough progress in a spin delay loop to properly 
> reach the number of microseconds required for N number of timer ticks to 
> occur.  

Hmm, mdelay is polling RDTSC and assumes it makes forward progress
and waits until the time that was estimated at the original TSC<->PIT
calibration passed.  While there is a spin loop it is definitely 
polling a timer that is supposed to tick properly even in virtualization.

You're saying that doesn't work on vmware? Does it have trouble
with RDTSC?

Anyways if polling against TSC doesn't work I suppose we could
change it to poll against some other timer.
 
> In theory this can happen on a real machine, as SMM mode could 
> be active, doing USB device emulation or something that takes a while 
> during the lpj calibration and throwing the computation off.

Yep.

> By changing the parameters (N ticks at K Hz in T seconds), it is easy to 
> create an unstable measurement that can achieve high failure rates, 
> although in practice the Linux parameters appear to be reasonable enough 
> that it is not a major problem.

Hmm, why exactly? 

-Andi

Re: [PATCH 1/5] Skip timer works.patch

[Chris Wright]

* Andi Kleen (ak@muc.de) wrote:
> On Mon, Oct 30, 2006 at 12:54:55PM -0800, Zachary Amsden wrote:
> > Andi Kleen wrote:
> > >no_timer_check. But it's only there on x86-64 in mainline - although there
> > >were some patches to add it to i386 too.
> > >  
> > 
> > I can rename to match the x86-64 name.
> 
> I will do that in my tree.

Looks like this one might have been lost.  Here's the renamed version
(it's against 2.6.19-rc5-mm2, which should have your tree in it).

thanks,
-chris
--

From: Zachary Amsden <zach@vmware.com>

Add a way to disable the timer IRQ routing check via a boot option.  The
VMI timer code uses this to avoid triggering the pester Mingo code, which
probes for some very unusual and broken motherboard routings.  It fires
100% of the time when using a paravirtual delay mechanism instead of
using a realtime delay, since there is no elapsed real time, and the 4 timer
IRQs have not yet been delivered.

In addition, it is entirely possible, though improbable, that this bug
could surface on real hardware which picks a particularly bad time to enter
SMM mode, causing a long latency during one of the timer IRQs.

While here, make check_timer be __init.

Signed-off-by: Zachary Amsden <zach@vmware.com>
[chrisw: use no_timer_check to bring inline with x86_64 as per Andi's request]
Signed-off-by: Chris Wright <chrisw@sous-sol.org>

===================================================================
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -610,8 +610,6 @@ and is between 256 and 4096 characters. 
 
 	hugepages=	[HW,IA-32,IA-64] Maximal number of HugeTLB pages.
 
-	noirqbalance	[IA-32,SMP,KNL] Disable kernel irq balancing
-
 	i8042.direct	[HW] Put keyboard port into non-translated mode
 	i8042.dumbkbd	[HW] Pretend that controller can only read data from
 			     keyboard and cannot control its state
@@ -1081,8 +1079,13 @@ and is between 256 and 4096 characters. 
 			in certain environments such as networked servers or
 			real-time systems.
 
+	noirqbalance	[IA-32,SMP,KNL] Disable kernel irq balancing
+
 	noirqdebug	[IA-32] Disables the code which attempts to detect and
 			disable unhandled interrupt sources.
+
+	no_timer_check	[IA-32,X86_64,APIC] Disables the code which tests for
+			broken timer IRQ sources.
 
 	noisapnp	[ISAPNP] Disables ISA PnP code.
 
===================================================================
--- a/arch/i386/kernel/io_apic.c
+++ b/arch/i386/kernel/io_apic.c
@@ -1931,6 +1931,15 @@ static void __init setup_ioapic_ids_from
 static void __init setup_ioapic_ids_from_mpc(void) { }
 #endif
 
+static int no_timer_check __initdata;
+
+static int __init notimercheck(char *s)
+{
+	no_timer_check = 1;
+	return 1;
+}
+__setup("no_timer_check", notimercheck);
+
 /*
  * There is a nasty bug in some older SMP boards, their mptable lies
  * about the timer IRQ. We do the following to work around the situation:
@@ -1939,9 +1948,12 @@ static void __init setup_ioapic_ids_from
  *	- if this function detects that timer IRQs are defunct, then we fall
  *	  back to ISA timer IRQs
  */
-static int __init timer_irq_works(void)
+int __init timer_irq_works(void)
 {
 	unsigned long t1 = jiffies;
+
+	if (no_timer_check)
+		return 1;
 
 	local_irq_enable();
 	/* Let ten ticks pass... */
@@ -2219,7 +2231,7 @@ int timer_uses_ioapic_pin_0;
  * is so screwy.  Thanks to Brian Perkins for testing/hacking this beast
  * fanatically on his truly buggy board.
  */
-static inline void check_timer(void)
+static inline void __init check_timer(void)
 {
 	int apic1, pin1, apic2, pin2;
 	int vector;

Re: [PATCH 1/5] Skip timer works.patch

[Zachary Amsden]

Chris Wright wrote:
> * Andi Kleen (ak@muc.de) wrote:
>   
>> On Mon, Oct 30, 2006 at 12:54:55PM -0800, Zachary Amsden wrote:
>>     
>>> Andi Kleen wrote:
>>>       
>>>> no_timer_check. But it's only there on x86-64 in mainline - although there
>>>> were some patches to add it to i386 too.
>>>>  
>>>>         
>>> I can rename to match the x86-64 name.
>>>       
>> I will do that in my tree.
>>     
>
> Looks like this one might have been lost.  Here's the renamed version
> (it's against 2.6.19-rc5-mm2, which should have your tree in it).
>
> thanks,
> -chris
>   

Thanks Chris, this is definitely a required patch for us.

I'm a little worried after seeing failures to apply and missed patches 
that we could have other things missing from the upstream path.  Is 
there an easy way for us to look at diffs between the trees?  Just a 
diff on arch/i386 and include/asm-i386 would tell me wonders, no matter 
how much extra activity has gone on there.

Namaste,

Zach

Re: [PATCH 1/5] Skip timer works.patch

[Chris Wright]

* Zachary Amsden (zach@vmware.com) wrote:
> I'm a little worried after seeing failures to apply and missed patches 
> that we could have other things missing from the upstream path.  Is 
> there an easy way for us to look at diffs between the trees?  Just a 
> diff on arch/i386 and include/asm-i386 would tell me wonders, no matter 
> how much extra activity has gone on there.

Last night I rebased against 2.6.15-mm2.  I'm adding four patches that
Andrew sent Andi last night.  I'll rediff and see what (if anything) is
outstanding.

thanks,
-chris

Re: [PATCH 1/5] Skip timer works.patch

[Chris Wright]

* Chris Wright (chrisw@sous-sol.org) wrote:
> Last night I rebased against 2.6.15-mm2.

should've read: 2.6.19-rc5-mm2

Re: [PATCH 1/5] Skip timer works.patch

[Andi Kleen]

On Wed, Nov 15, 2006 at 02:40:53PM -0800, Chris Wright wrote:
> * Zachary Amsden (zach@vmware.com) wrote:
> > I'm a little worried after seeing failures to apply and missed patches 
> > that we could have other things missing from the upstream path.  Is 
> > there an easy way for us to look at diffs between the trees?  Just a 
> > diff on arch/i386 and include/asm-i386 would tell me wonders, no matter 
> > how much extra activity has gone on there.
> 
> Last night I rebased against 2.6.15-mm2.  I'm adding four patches that
> Andrew sent Andi last night.  I'll rediff and see what (if anything) is
> outstanding.

I got Andrew's patches now but the result doesn't compile as earlier
(on SUSE 10.0, gcc 4.0)

I'll adapt my old make-it-compile patch that fixed the inline assembly. 
Hopefully it'll work then.

-Andi

/home/lsrc/quilt/linux/kernel/spinlock.c: In function '_spin_lock_irqsave':
include2/asm/spinlock.h:59: error: invalid 'asm': operand number missing after %
-letter
include2/asm/spinlock.h:59: error: invalid 'asm': operand number missing after %
-letter
include2/asm/spinlock.h:59: error: invalid 'asm': operand number missing after %
-letter
include2/asm/spinlock.h:59: error: invalid 'asm': operand number missing after %
-letter
include2/asm/spinlock.h:59: error: invalid 'asm': operand number missing after %
-letter
include2/asm/spinlock.h:59: error: invalid 'asm': operand number missing after %
-letter
include2/asm/spinlock.h:59: error: invalid 'asm': operand number missing after %
-letter
include2/asm/spinlock.h:59: error: invalid 'asm': operand number missing after %
-letter
{standard input}: Assembler messages:
{standard input}:593: Error: undefined symbol `paravirt_ops' in operation
{standard input}:593: Error: undefined symbol `PARAVIRT_irq_enable' in operation
{standard input}:605: Error: undefined symbol `paravirt_ops' in operation
{standard input}:605: Error: undefined symbol `PARAVIRT_irq_disable' in operatio
n
ICECREAM[14067]: Compiled on 10.23.204.76

Re: [PATCH 1/5] Skip timer works.patch

[Chris Wright]

* Andi Kleen (ak@suse.de) wrote:
> I got Andrew's patches now but the result doesn't compile as earlier
> (on SUSE 10.0, gcc 4.0)
> 
> I'll adapt my old make-it-compile patch that fixed the inline assembly. 
> Hopefully it'll work then.
> 
> -Andi
> 
> /home/lsrc/quilt/linux/kernel/spinlock.c: In function '_spin_lock_irqsave':
> include2/asm/spinlock.h:59: error: invalid 'asm': operand number missing after %
> -letter
> include2/asm/spinlock.h:59: error: invalid 'asm': operand number missing after %
> -letter
> include2/asm/spinlock.h:59: error: invalid 'asm': operand number missing after %
> -letter
> include2/asm/spinlock.h:59: error: invalid 'asm': operand number missing after %
> -letter
> include2/asm/spinlock.h:59: error: invalid 'asm': operand number missing after %
> -letter
> include2/asm/spinlock.h:59: error: invalid 'asm': operand number missing after %
> -letter
> include2/asm/spinlock.h:59: error: invalid 'asm': operand number missing after %
> -letter
> include2/asm/spinlock.h:59: error: invalid 'asm': operand number missing after %
> -letter
> {standard input}: Assembler messages:
> {standard input}:593: Error: undefined symbol `paravirt_ops' in operation
> {standard input}:593: Error: undefined symbol `PARAVIRT_irq_enable' in operation
> {standard input}:605: Error: undefined symbol `paravirt_ops' in operation
> {standard input}:605: Error: undefined symbol `PARAVIRT_irq_disable' in operation

That should be fixed (it is over here).  Is ff quilt stuff uptodate?
I can sync up with it and diff to see what you might be missing.

> ICECREAM[14067]: Compiled on 10.23.204.76

mmm, ice cream ;-)

thanks,
-chris

Re: [PATCH 1/5] Skip timer works.patch

[Andi Kleen]

On Wed, Nov 15, 2006 at 07:37:03PM -0800, Chris Wright wrote:
> * Andi Kleen (ak@suse.de) wrote:
> > I got Andrew's patches now but the result doesn't compile as earlier
> > (on SUSE 10.0, gcc 4.0)
> > 
> > I'll adapt my old make-it-compile patch that fixed the inline assembly. 
> > Hopefully it'll work then.
> > 
> > -Andi
> > 
> > /home/lsrc/quilt/linux/kernel/spinlock.c: In function '_spin_lock_irqsave':
> > include2/asm/spinlock.h:59: error: invalid 'asm': operand number missing after %
> > -letter
> > include2/asm/spinlock.h:59: error: invalid 'asm': operand number missing after %
> > -letter
> > include2/asm/spinlock.h:59: error: invalid 'asm': operand number missing after %
> > -letter
> > include2/asm/spinlock.h:59: error: invalid 'asm': operand number missing after %
> > -letter
> > include2/asm/spinlock.h:59: error: invalid 'asm': operand number missing after %
> > -letter
> > include2/asm/spinlock.h:59: error: invalid 'asm': operand number missing after %
> > -letter
> > include2/asm/spinlock.h:59: error: invalid 'asm': operand number missing after %
> > -letter
> > include2/asm/spinlock.h:59: error: invalid 'asm': operand number missing after %
> > -letter
> > {standard input}: Assembler messages:
> > {standard input}:593: Error: undefined symbol `paravirt_ops' in operation
> > {standard input}:593: Error: undefined symbol `PARAVIRT_irq_enable' in operation
> > {standard input}:605: Error: undefined symbol `paravirt_ops' in operation
> > {standard input}:605: Error: undefined symbol `PARAVIRT_irq_disable' in operation
> 
> That should be fixed (it is over here). 

Ok.

I think my old paravirt-broken diff fixed it too (among other things), but it 
would be better to use your version.


> Is ff quilt stuff uptodate?

Not yet.  I was just merging the latest -git. Will push it out soon.

> I can sync up with it and diff to see what you might be missing.

Please.

> 
> > ICECREAM[14067]: Compiled on 10.23.204.76
> 
> mmm, ice cream ;-)

It sometimes causes interesting effect with funky assembly -- e.g. .include doesn't
work because the assembler runs on a different host. But that shouldn't be the
case here.

-Andi

Re: [PATCH 1/5] Skip timer works.patch

[Andrew Morton]

On Thu, 16 Nov 2006 04:27:02 +0100
Andi Kleen <ak@suse.de> wrote:

> On Wed, Nov 15, 2006 at 02:40:53PM -0800, Chris Wright wrote:
> > * Zachary Amsden (zach@vmware.com) wrote:
> > > I'm a little worried after seeing failures to apply and missed patches 
> > > that we could have other things missing from the upstream path.  Is 
> > > there an easy way for us to look at diffs between the trees?  Just a 
> > > diff on arch/i386 and include/asm-i386 would tell me wonders, no matter 
> > > how much extra activity has gone on there.
> > 
> > Last night I rebased against 2.6.15-mm2.  I'm adding four patches that
> > Andrew sent Andi last night.  I'll rediff and see what (if anything) is
> > outstanding.
> 
> I got Andrew's patches now but the result doesn't compile as earlier
> (on SUSE 10.0, gcc 4.0)
> 
> I'll adapt my old make-it-compile patch that fixed the inline assembly. 
> Hopefully it'll work then.
> 
> -Andi
> 
> /home/lsrc/quilt/linux/kernel/spinlock.c: In function '_spin_lock_irqsave':
> include2/asm/spinlock.h:59: error: invalid 'asm': operand number missing after %
> -letter
> include2/asm/spinlock.h:59: error: invalid 'asm': operand number missing after %
> -letter
> include2/asm/spinlock.h:59: error: invalid 'asm': operand number missing after %
> -letter
> include2/asm/spinlock.h:59: error: invalid 'asm': operand number missing after %
> -letter
> include2/asm/spinlock.h:59: error: invalid 'asm': operand number missing after %
> -letter
> include2/asm/spinlock.h:59: error: invalid 'asm': operand number missing after %
> -letter
> include2/asm/spinlock.h:59: error: invalid 'asm': operand number missing after %
> -letter
> include2/asm/spinlock.h:59: error: invalid 'asm': operand number missing after %
> -letter
> {standard input}: Assembler messages:
> {standard input}:593: Error: undefined symbol `paravirt_ops' in operation
> {standard input}:593: Error: undefined symbol `PARAVIRT_irq_enable' in operation
> {standard input}:605: Error: undefined symbol `paravirt_ops' in operation
> {standard input}:605: Error: undefined symbol `PARAVIRT_irq_disable' in operatio
> n
> ICECREAM[14067]: Compiled on 10.23.204.76
> 

wtf?  How come patches leave here in working order and keep arriving broken?

Re: [PATCH 1/5] Skip timer works.patch

[Zachary Amsden]

Andrew Morton wrote:
> wtf?  How come patches leave here in working order and keep arriving broken

Patchworms?  Quilt moths?  Or perhaps, leaf cutting ants - they only 
have a 50% success rate at holding on to the proper bits because they 
are unaware of the physical geometry of the leaf in relationship to the 
branch, so half the time they hold onto the   wrong side, letting the 
leaf fall instead of bringing it back to the repository.

Zach

Re: [PATCH 1/5] Skip timer works.patch

[Andi Kleen]

> 
> wtf?  

I don't know. I applied all, except 3 non paravirt related patches you sent
me.

> How come patches leave here in working order and keep arriving broken? 

Maybe it's my toolchain (gcc 4.0) or my config (defconfig)
But looking at the asm() I'm not sure it ever worked anywhere.

-Andi

Re: [PATCH 1/5] Skip timer works.patch

[Chris Wright]

* Andi Kleen (ak@suse.de) wrote:
> Maybe it's my toolchain (gcc 4.0) or my config (defconfig)
> But looking at the asm() I'm not sure it ever worked anywhere.

That's possible, I recall this being broken before and it was not hitting
everyone.  BTW, here's what I have so far...

Up to 'config-core2' both ff and mm are the same with these minor caveats
below[1].  This includes the core paravirt patches.  I'm now digging
into the last few tidy-up type of patches.

[1] The changes below were to try and get clean diffs.  Nothing
substantial, mostly just things that have gone upstream.

in ff, not mm
-------------
i386-pci-dma-iounmap (added to mm for clean diff)

in mm, not ff
-------------
x86_64-mm-io-apic-reuse.patch (commented out from mm)
x86_64-mm-fix-exit-idle-race.patch (pushed to linus)
x86_64-mm-reserve-bootmem-beyond-end-pfn.patch (pushed to linus)
x86_64-mm-setup-saved_max_pfn-correctly-kdump.patch (pushed to linus)
x86_64-mm-ptrace-compat-threadarea.patch (pushed to linus)
x86_64-mm-pci-mcfg-reserve-e820.patch (dropped?)
x86_64-mm-fix-boot-gdt-limit.patch (pushed)
x86_64-mm-e820-small-entries.patch (dropped?)

refreshed in mm
---------------
x86_64-mm-i386-reloc-abssym.patch
x86_64-mm-try-multiple-timer-pins.patch

Re: [PATCH 1/5] Skip timer works.patch

[Andi Kleen]

> Or perhaps, leaf cutting ants - they only 
> have a 50% success rate at holding on to the proper bits because they 
> are unaware of the physical geometry of the leaf in relationship to the 
> branch, so half the time they hold onto the   wrong side, letting the 
> leaf fall instead of bringing it back to the repository.

That must it be. Perhaps they infected muc.de's mailserver.

-Andi

Re: [PATCH 1/5] Skip timer works.patch

[Chris Wright]

* Chris Wright (chrisw@sous-sol.org) wrote:
> Up to 'config-core2' both ff and mm are the same with these minor caveats
> below[1].  This includes the core paravirt patches.  I'm now digging
> into the last few tidy-up type of patches.

All the same on both sides, AFAICT.  There's a few more patches in
Andi's queue that aren't in Andrew's yet (totally unrelated to pv),
and there's 100's of patches in Andrew's tree that aren't related to pv
or x86 at all.  So, it's possible I've missed something since I stopped
careful inspection at the end of the pv bits.  The extra tidy-up patches
after core pv patches are:

in ff-speak
-----------
alloc_gdt-static
mmu-header-movement
cpu_detect-extraction
fix-bad-mmu-names
fix-missing-pte-update
fix-x86_64-mm-patch-inline-replacements-for

in mm-speak
-----------
pda-percpu-init-make-arch-i386-kernel-cpu-commoncalloc_gdt-static.patch
paravirt-mmu-header-movement.patch
paravirt-cpu-detect.patch
paravirt-pte-update-prep.patch
paravirt-pte-update-common.patch
fix-x86_64-mm-patch-inline-replacements-for.patch (hotfix)

Which still create identical trees.

The only patches lost bits that I've found:

1) pv queue has the two patches below which don't appear in either ff or mm:
   - 014-skip-timer-works.patch (patch is from this thread)
   - fix-modpost-warning.patch

2) ff has 'unused' in patches/ but not in series.  it is already folded in so
it's just a bit of noise.

thanks,
-chris
--

----- start 014-skip-timer-works.patch -----
From: Zachary Amsden <zach@vmware.com>

Add a way to disable the timer IRQ routing check via a boot option.  The
VMI timer code uses this to avoid triggering the pester Mingo code, which
probes for some very unusual and broken motherboard routings.  It fires
100% of the time when using a paravirtual delay mechanism instead of
using a realtime delay, since there is no elapsed real time, and the 4 timer
IRQs have not yet been delivered.

In addition, it is entirely possible, though improbable, that this bug
could surface on real hardware which picks a particularly bad time to enter
SMM mode, causing a long latency during one of the timer IRQs.

While here, make check_timer be __init.

Signed-off-by: Zachary Amsden <zach@vmware.com>
[chrisw: use no_timer_check to bring inline with x86_64 as per Andi's request]
Signed-off-by: Chris Wright <chrisw@sous-sol.org>

===================================================================
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -610,8 +610,6 @@ and is between 256 and 4096 characters. 
 
 	hugepages=	[HW,IA-32,IA-64] Maximal number of HugeTLB pages.
 
-	noirqbalance	[IA-32,SMP,KNL] Disable kernel irq balancing
-
 	i8042.direct	[HW] Put keyboard port into non-translated mode
 	i8042.dumbkbd	[HW] Pretend that controller can only read data from
 			     keyboard and cannot control its state
@@ -1081,8 +1079,13 @@ and is between 256 and 4096 characters. 
 			in certain environments such as networked servers or
 			real-time systems.
 
+	noirqbalance	[IA-32,SMP,KNL] Disable kernel irq balancing
+
 	noirqdebug	[IA-32] Disables the code which attempts to detect and
 			disable unhandled interrupt sources.
+
+	no_timer_check	[IA-32,X86_64,APIC] Disables the code which tests for
+			broken timer IRQ sources.
 
 	noisapnp	[ISAPNP] Disables ISA PnP code.
 
===================================================================
--- a/arch/i386/kernel/io_apic.c
+++ b/arch/i386/kernel/io_apic.c
@@ -1931,6 +1931,15 @@ static void __init setup_ioapic_ids_from
 static void __init setup_ioapic_ids_from_mpc(void) { }
 #endif
 
+static int no_timer_check __initdata;
+
+static int __init notimercheck(char *s)
+{
+	no_timer_check = 1;
+	return 1;
+}
+__setup("no_timer_check", notimercheck);
+
 /*
  * There is a nasty bug in some older SMP boards, their mptable lies
  * about the timer IRQ. We do the following to work around the situation:
@@ -1939,9 +1948,12 @@ static void __init setup_ioapic_ids_from
  *	- if this function detects that timer IRQs are defunct, then we fall
  *	  back to ISA timer IRQs
  */
-static int __init timer_irq_works(void)
+int __init timer_irq_works(void)
 {
 	unsigned long t1 = jiffies;
+
+	if (no_timer_check)
+		return 1;
 
 	local_irq_enable();
 	/* Let ten ticks pass... */
@@ -2219,7 +2231,7 @@ int timer_uses_ioapic_pin_0;
  * is so screwy.  Thanks to Brian Perkins for testing/hacking this beast
  * fanatically on his truly buggy board.
  */
-static inline void check_timer(void)
+static inline void __init check_timer(void)
 {
 	int apic1, pin1, apic2, pin2;
 	int vector;

----- end 014-skip-timer-works.patch -----

----- start fix-modpost-warning.patch -----
From: Rusty Russell <rusty@rustcorp.com.au>

modpost compains about a "section mismatch" because the
.parainstructions section sometimes refers to the .init.text section.
This is fine, put it in whitelist.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Chris Wright <chrisw@sous-sol.org>

===================================================================
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -929,6 +929,7 @@ static int init_section_ref_ok(const cha
 	const char *namelist2[] = {
 		".init.",
 		".altinstructions",
+		".parainstructions",
 		".eh_frame",
 		".debug",
 		".rodata",

----- end fix-modpost-warning.patch -----

Re: [PATCH 1/5] Skip timer works.patch

[Chris Wright]

* Andrew Morton (akpm@osdl.org) wrote:
> wtf?  How come patches leave here in working order and keep arriving broken?

I think the trees are in sync, and it's actually a bug that's .config
dependent.  Basically, the STI_STRING and CLI_STRING are not useable
the way they get used:

#define CLI_STRING paravirt_alt("pushl %ecx; pushl %edx;"		\
		     "call *paravirt_ops+PARAVIRT_irq_disable;"		\
		     "popl %edx; popl %ecx",				\
		     PARAVIRT_IRQ_DISABLE, CLBR_EAX)

#define STI_STRING paravirt_alt("pushl %ecx; pushl %edx;"		\
		     "call *paravirt_ops+PARAVIRT_irq_enable;"		\
		     "popl %edx; popl %ecx",				\
		     PARAVIRT_IRQ_ENABLE, CLBR_EAX)

#ifndef CONFIG_PROVE_LOCKING <-- that's part of the problem (must be
                                 on in most configs)
static inline void __raw_spin_lock_flags(raw_spinlock_t *lock, unsigned long flags)
{
	asm volatile(
		"\n1:\t"
		LOCK_PREFIX " ; decb %0\n\t"  <-- here the operand is 
                                                  numerically referred to
		"jns 5f\n"
		"2:\t"
		"testl $0x200, %1\n\t"
		"jz 4f\n\t"
		STI_STRING "\n"  <-- here we have code that does 
                                     "pushl %ecx; pushl %edx;", not %%ecx
		"3:\t"
		"rep;nop\n\t"
		"cmpb $0, %0\n\t"
		"jle 3b\n\t"
		CLI_STRING "\n\t"
		"jmp 1b\n"
		"4:\t"
		"rep;nop\n\t"
		"cmpb $0, %0\n\t"
		"jg 1b\n\t"
		"jmp 4b\n"
		"5:\n\t"
		: "+m" (lock->slock)
		: "r" (flags)
		: "memory" CLI_STI_CLOBBERS);
}
#endif

So, Andi's patch, is needed and correct AFAICT.  Fixes all ff, mm, pv trees...

thanks,
-chris


i386: Get paravirt ops to compile

TBD should be folded into the original patches

Signed-off-by: Andi Kleen <ak@suse.de>

===================================================================
--- linux.orig/include/asm-i386/paravirt.h
+++ linux/include/asm-i386/paravirt.h
@@ -454,16 +454,20 @@ static inline unsigned long __raw_local_
 	return f;
 }
 
-#define CLI_STRING paravirt_alt("pushl %ecx; pushl %edx;"		\
-		     "call *paravirt_ops+PARAVIRT_irq_disable;"		\
-		     "popl %edx; popl %ecx",				\
+#define CLI_STRING paravirt_alt("pushl %%ecx; pushl %%edx;"		\
+		     "call *paravirt_ops+%c[irq_disable];"		\
+		     "popl %%edx; popl %%ecx",				\
 		     PARAVIRT_IRQ_DISABLE, CLBR_EAX)
 
-#define STI_STRING paravirt_alt("pushl %ecx; pushl %edx;"		\
-		     "call *paravirt_ops+PARAVIRT_irq_enable;"		\
-		     "popl %edx; popl %ecx",				\
+#define STI_STRING paravirt_alt("pushl %%ecx; pushl %%edx;"		\
+		     "call *paravirt_ops+%c[irq_enable];"		\
+		     "popl %%edx; popl %%ecx",				\
 		     PARAVIRT_IRQ_ENABLE, CLBR_EAX)
 #define CLI_STI_CLOBBERS , "%eax"
+#define CLI_STI_INPUT_ARGS \
+	,								\
+	[irq_disable] "i" (offsetof(struct paravirt_ops, irq_disable)),	\
+	[irq_enable] "i" (offsetof(struct paravirt_ops, irq_enable))
 
 #else  /* __ASSEMBLY__ */
 
Index: linux/include/asm-i386/spinlock.h
===================================================================
--- linux.orig/include/asm-i386/spinlock.h
+++ linux/include/asm-i386/spinlock.h
@@ -13,6 +13,7 @@
 #define CLI_STRING	"cli"
 #define STI_STRING	"sti"
 #define CLI_STI_CLOBBERS
+#define CLI_STI_INPUT_ARGS
 #endif /* CONFIG_PARAVIRT */
 
 /*
@@ -58,26 +59,27 @@ static inline void __raw_spin_lock_flags
 {
 	asm volatile(
 		"\n1:\t"
-		LOCK_PREFIX " ; decb %0\n\t"
+		LOCK_PREFIX " ; decb %[slock]\n\t"
 		"jns 5f\n"
 		"2:\t"
-		"testl $0x200, %1\n\t"
+		"testl $0x200, %[flags]\n\t"
 		"jz 4f\n\t"
 		STI_STRING "\n"
 		"3:\t"
 		"rep;nop\n\t"
-		"cmpb $0, %0\n\t"
+		"cmpb $0, %[slock]\n\t"
 		"jle 3b\n\t"
 		CLI_STRING "\n\t"
 		"jmp 1b\n"
 		"4:\t"
 		"rep;nop\n\t"
-		"cmpb $0, %0\n\t"
+		"cmpb $0, %[slock]\n\t"
 		"jg 1b\n\t"
 		"jmp 4b\n"
 		"5:\n\t"
-		: "+m" (lock->slock)
-		: "r" (flags)
+		: [slock] "+m" (lock->slock)
+		: [flags] "r" (flags)
+	 	  CLI_STI_INPUT_ARGS
 		: "memory" CLI_STI_CLOBBERS);
 }
 #endif

Re: [PATCH 1/5] Skip timer works.patch

[Andi Kleen]

On Thursday 16 November 2006 11:28, Chris Wright wrote:
> * Andrew Morton (akpm@osdl.org) wrote:
> > wtf?  How come patches leave here in working order and keep arriving broken?
> 
> I think the trees are in sync, and it's actually a bug that's .config
> dependent.  Basically, the STI_STRING and CLI_STRING are not useable
> the way they get used:

I merged everything together.  Synced out to ff now.

CONFIG_PARAVIRT=n kernel runs now, but when I run a defconfig + CONFIG_PARAVIRT=y 
kernel on bare metal init still breaks with a long stream of 

INIT: Entering runlevel: 3
Inconsistency detected by ld.so: rtld.c: 1250: dl_main: Assertion `(void *) ph->
p_vaddr == _rtld_local._dl_sysinfo_dso' failed!
Inconsistency detected by ld.so: rtld.c: 1250: dl_main: Assertion `(void *) ph->
p_vaddr == _rtld_local._dl_sysinfo_dso' failed!
Inconsistency detected by ld.so: rtld.c: 1250: dl_main: Assertion `(void *) ph->
p_vaddr == _rtld_local._dl_sysinfo_dso' failed!
Inconsistency detected by ld.so: rtld.c: 1250: dl_main: Assertion `(void *) ph->
p_vaddr == _rtld_local._dl_sysinfo_dso' failed!
...

before giving up. So it looks like something is still broken


-Andi

Re: [PATCH 1/5] Skip timer works.patch

[Chris Wright]

* Andi Kleen (ak@suse.de) wrote:
> I merged everything together.  Synced out to ff now.

OK, thanks.

> CONFIG_PARAVIRT=n kernel runs now, but when I run a defconfig + CONFIG_PARAVIRT=y 
> kernel on bare metal init still breaks with a long stream of 
> 
> INIT: Entering runlevel: 3
> Inconsistency detected by ld.so: rtld.c: 1250: dl_main: Assertion `(void *) ph->
> p_vaddr == _rtld_local._dl_sysinfo_dso' failed!
> Inconsistency detected by ld.so: rtld.c: 1250: dl_main: Assertion `(void *) ph->
> p_vaddr == _rtld_local._dl_sysinfo_dso' failed!
> Inconsistency detected by ld.so: rtld.c: 1250: dl_main: Assertion `(void *) ph->
> p_vaddr == _rtld_local._dl_sysinfo_dso' failed!
> Inconsistency detected by ld.so: rtld.c: 1250: dl_main: Assertion `(void *) ph->
> p_vaddr == _rtld_local._dl_sysinfo_dso' failed!
> ...
> 
> before giving up. So it looks like something is still broken

Another one that was working here.  I'll debug with your config.

thanks,
chris

Re: [PATCH 1/5] Skip timer works.patch

[Andi Kleen]

On Thursday 16 November 2006 20:03, Chris Wright wrote:
> * Andi Kleen (ak@suse.de) wrote:
> > I merged everything together.  Synced out to ff now.
> 
> OK, thanks.
> 
> > CONFIG_PARAVIRT=n kernel runs now, but when I run a defconfig + CONFIG_PARAVIRT=y 
> > kernel on bare metal init still breaks with a long stream of 
> > 
> > INIT: Entering runlevel: 3
> > Inconsistency detected by ld.so: rtld.c: 1250: dl_main: Assertion `(void *) ph->
> > p_vaddr == _rtld_local._dl_sysinfo_dso' failed!
> > Inconsistency detected by ld.so: rtld.c: 1250: dl_main: Assertion `(void *) ph->
> > p_vaddr == _rtld_local._dl_sysinfo_dso' failed!
> > Inconsistency detected by ld.so: rtld.c: 1250: dl_main: Assertion `(void *) ph->
> > p_vaddr == _rtld_local._dl_sysinfo_dso' failed!
> > Inconsistency detected by ld.so: rtld.c: 1250: dl_main: Assertion `(void *) ph->
> > p_vaddr == _rtld_local._dl_sysinfo_dso' failed!
> > ...
> > 
> > before giving up. So it looks like something is still broken
> 
> Another one that was working here.  I'll debug with your config.

I started some bisecting (it's difficult because many intermediaries
don't compile -- something to clean up). Anyways, for this order of
paravirt patches:

header-and-stubs-for-paravirtualisation
patch-inline-replacements-for
paravirt-compile
cpu_detect-extraction
more-generic-paravirtualization
allow-selected-bug-checks-to-be
allow-disabling-legacy-power
add-apic-accessors-to-paravirt-ops.
add-mmu-virtualization-to
be-careful-about-touching-bios-address-space
mmu-header-movement
fix-bad-mmu-names
fix-missing-pte-update
fix-x86_64-mm-patch-inline-replacements-for
paravirt-pae
skip-timer-works

it's already broken at more-generic-paravirtualization.  Surprised
me a little, i was suspecting the MMU changes.

-Andi

Re: [PATCH 1/5] Skip timer works.patch

[Chris Wright]

* Andi Kleen (ak@suse.de) wrote:
> > > Inconsistency detected by ld.so: rtld.c: 1250: dl_main: Assertion `(void *) ph->
> > > p_vaddr == _rtld_local._dl_sysinfo_dso' failed!
> 
> I started some bisecting...
<snip patch list>
> it's already broken at more-generic-paravirtualization.  Surprised
> me a little, i was suspecting the MMU changes.

Looks like the VDSO issue, paravirt is not compatible if COMPAT_VDSO.
I expect you'll get the same if you disable COMPAT_VDSO on native build?

thanks,
-chris

Re: [PATCH 1/5] Skip timer works.patch

[Andrew Morton]

On Thu, 19 Oct 2006 17:09:22 -0700
Zachary Amsden <zach@vmware.com> wrote:

> Add a way to disable the timer IRQ routing check via a boot option.  The
> VMI timer code uses this to avoid triggering the pester Mingo code, which
> probes for some very unusual and broken motherboard routings.  It fires
> 100% of the time when using a paravirtual delay mechanism instead of
> using a realtime delay, since there is no elapsed real time, and the 4 timer
> IRQs have not yet been delivered.
> 
> In addition, it is entirely possible, though improbable, that this bug
> could surface on real hardware which picks a particularly bad time to enter
> SMM mode, causing a long latency during one of the timer IRQs.
> 
> While here, make check_timer be __init.
> 

Andi seems to have merged this patch but from somewhere I picked up a
different version, below.

I think the version I have is better.  Because the patch Andi has merged is
cast in terms of "irq testing", which is broad.  But that's not what the
patch does - the patch handles only timers.

IOW, this:

> +
> +	noirqtest	[IA-32,APIC] Disables the code which tests for broken
> +			timer IRQ sources.

is misleadingly named.  This:

+       no_timer_check  [IA-32,X86_64,APIC] Disables the code which tests for
+                       broken timer IRQ sources.
+

is better, no?

But right now, I'll settle for anything which usually compiles.



From: Zachary Amsden <zach@vmware.com>

Add a way to disable the timer IRQ routing check via a boot option.  The
VMI timer code uses this to avoid triggering the pester Mingo code, which
probes for some very unusual and broken motherboard routings.  It fires
100% of the time when using a paravirtual delay mechanism instead of using
a realtime delay, since there is no elapsed real time, and the 4 timer IRQs
have not yet been delivered.

In addition, it is entirely possible, though improbable, that this bug
could surface on real hardware which picks a particularly bad time to enter
SMM mode, causing a long latency during one of the timer IRQs.

While here, make check_timer be __init.

Signed-off-by: Zachary Amsden <zach@vmware.com>
[chrisw: use no_timer_check to bring inline with x86_64 as per Andi's request]
Signed-off-by: Chris Wright <chrisw@sous-sol.org>
Cc: Andi Kleen <ak@suse.de>
Signed-off-by: Andrew Morton <akpm@osdl.org>
---

 Documentation/kernel-parameters.txt |    7 +++++--
 arch/i386/kernel/io_apic.c          |   16 ++++++++++++++--
 2 files changed, 19 insertions(+), 4 deletions(-)

diff -puN arch/i386/kernel/io_apic.c~i386-add-a-way-to-disable-the-timer-irq-routing-check-via-a-boot-option arch/i386/kernel/io_apic.c
--- a/arch/i386/kernel/io_apic.c~i386-add-a-way-to-disable-the-timer-irq-routing-check-via-a-boot-option
+++ a/arch/i386/kernel/io_apic.c
@@ -1930,6 +1930,15 @@ static void __init setup_ioapic_ids_from
 static void __init setup_ioapic_ids_from_mpc(void) { }
 #endif
 
+static int no_timer_check __initdata;
+
+static int __init notimercheck(char *s)
+{
+	no_timer_check = 1;
+	return 1;
+}
+__setup("no_timer_check", notimercheck);
+
 /*
  * There is a nasty bug in some older SMP boards, their mptable lies
  * about the timer IRQ. We do the following to work around the situation:
@@ -1938,10 +1947,13 @@ static void __init setup_ioapic_ids_from
  *	- if this function detects that timer IRQs are defunct, then we fall
  *	  back to ISA timer IRQs
  */
-static int __init timer_irq_works(void)
+int __init timer_irq_works(void)
 {
 	unsigned long t1 = jiffies;
 
+	if (no_timer_check)
+		return 1;
+
 	local_irq_enable();
 	/* Let ten ticks pass... */
 	mdelay((10 * 1000) / HZ);
@@ -2212,7 +2224,7 @@ int timer_uses_ioapic_pin_0;
  * is so screwy.  Thanks to Brian Perkins for testing/hacking this beast
  * fanatically on his truly buggy board.
  */
-static inline void check_timer(void)
+static inline void __init check_timer(void)
 {
 	int apic1, pin1, apic2, pin2;
 	int vector;
diff -puN Documentation/kernel-parameters.txt~i386-add-a-way-to-disable-the-timer-irq-routing-check-via-a-boot-option Documentation/kernel-parameters.txt
--- a/Documentation/kernel-parameters.txt~i386-add-a-way-to-disable-the-timer-irq-routing-check-via-a-boot-option
+++ a/Documentation/kernel-parameters.txt
@@ -599,8 +599,6 @@ and is between 256 and 4096 characters. 
 
 	hugepages=	[HW,IA-32,IA-64] Maximal number of HugeTLB pages.
 
-	noirqbalance	[IA-32,SMP,KNL] Disable kernel irq balancing
-
 	i8042.direct	[HW] Put keyboard port into non-translated mode
 	i8042.dumbkbd	[HW] Pretend that controller can only read data from
 			     keyboard and cannot control its state
@@ -1056,9 +1054,14 @@ and is between 256 and 4096 characters. 
 			in certain environments such as networked servers or
 			real-time systems.
 
+	noirqbalance	[IA-32,SMP,KNL] Disable kernel irq balancing
+
 	noirqdebug	[IA-32] Disables the code which attempts to detect and
 			disable unhandled interrupt sources.
 
+	no_timer_check	[IA-32,X86_64,APIC] Disables the code which tests for
+			broken timer IRQ sources.
+
 	noisapnp	[ISAPNP] Disables ISA PnP code.
 
 	noinitrd	[RAM] Tells the kernel not to load any configured
_

Re: [PATCH 1/5] Skip timer works.patch

[Zachary Amsden]

Andrew Morton wrote:
> On Thu, 19 Oct 2006 17:09:22 -0700
> Zachary Amsden <zach@vmware.com> wrote:
>
>   
>> Add a way to disable the timer IRQ routing check via a boot option.  The
>> VMI timer code uses this to avoid triggering the pester Mingo code, which
>> probes for some very unusual and broken motherboard routings.  It fires
>> 100% of the time when using a paravirtual delay mechanism instead of
>> using a realtime delay, since there is no elapsed real time, and the 4 timer
>> IRQs have not yet been delivered.
>>
>> In addition, it is entirely possible, though improbable, that this bug
>> could surface on real hardware which picks a particularly bad time to enter
>> SMM mode, causing a long latency during one of the timer IRQs.
>>
>> While here, make check_timer be __init.
>>
>>     
>
> Andi seems to have merged this patch but from somewhere I picked up a
> different version, below.
>
> I think the version I have is better.  Because the patch Andi has merged is
> cast in terms of "irq testing", which is broad.  But that's not what the
> patch does - the patch handles only timers.
>
> IOW, this:
>
>   
>> +
>> +	noirqtest	[IA-32,APIC] Disables the code which tests for broken
>> +			timer IRQ sources.
>>     
>
> is misleadingly named.  This:
>
> +       no_timer_check  [IA-32,X86_64,APIC] Disables the code which tests for
> +                       broken timer IRQ sources.
> +
>
> is better, no?
>
> But right now, I'll settle for anything which usually compiles.
>
>   

Yes, the name sucks.  There is no real reason to actually have a boot 
parameter at all once the paravirt / VMI patches are in, but I wanted 
something to be able to set timer_irq_really_works until then to avoid 
someone accidentally removing it.

Zach

Re: [PATCH 1/5] Skip timer works.patch

[Chris Wright]

* Andrew Morton (akpm@osdl.org) wrote:
> Andi seems to have merged this patch but from somewhere I picked up a
> different version, below.

That was from me.  The earlier one had fallen through the cracks, so
I picked it up and reworked it (so that it uses no_timer_check) as per
Andi's suggestion.

> I think the version I have is better.

Agreed ;-)

> Because the patch Andi has merged is
> cast in terms of "irq testing", which is broad.  But that's not what the
> patch does - the patch handles only timers.
> 
> IOW, this:
> 
> > +
> > +	noirqtest	[IA-32,APIC] Disables the code which tests for broken
> > +			timer IRQ sources.
> 
> is misleadingly named.  This:
> 
> +       no_timer_check  [IA-32,X86_64,APIC] Disables the code which tests for
> +                       broken timer IRQ sources.
> +
> 
> is better, no?

Clearer, and also same as x86_64.

> But right now, I'll settle for anything which usually compiles.

I believe the current round has fixed all the know compilation issues.
There's one outstanding runtime issue from Andi, which I believe is due
to an old glibc which requires COMPAT_VDSO (and which CONFIG_PARAVIRT
must disable).  So current set is clean from the reports I have (and
assuming above vdso analysis is correct).

thanks,
-chris
--
> From: Zachary Amsden <zach@vmware.com>
> 
> Add a way to disable the timer IRQ routing check via a boot option.  The
> VMI timer code uses this to avoid triggering the pester Mingo code, which
> probes for some very unusual and broken motherboard routings.  It fires
> 100% of the time when using a paravirtual delay mechanism instead of using
> a realtime delay, since there is no elapsed real time, and the 4 timer IRQs
> have not yet been delivered.
> 
> In addition, it is entirely possible, though improbable, that this bug
> could surface on real hardware which picks a particularly bad time to enter
> SMM mode, causing a long latency during one of the timer IRQs.
> 
> While here, make check_timer be __init.
> 
> Signed-off-by: Zachary Amsden <zach@vmware.com>
> [chrisw: use no_timer_check to bring inline with x86_64 as per Andi's request]
> Signed-off-by: Chris Wright <chrisw@sous-sol.org>
> Cc: Andi Kleen <ak@suse.de>
> Signed-off-by: Andrew Morton <akpm@osdl.org>
> ---
> 
>  Documentation/kernel-parameters.txt |    7 +++++--
>  arch/i386/kernel/io_apic.c          |   16 ++++++++++++++--
>  2 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff -puN arch/i386/kernel/io_apic.c~i386-add-a-way-to-disable-the-timer-irq-routing-check-via-a-boot-option arch/i386/kernel/io_apic.c
> --- a/arch/i386/kernel/io_apic.c~i386-add-a-way-to-disable-the-timer-irq-routing-check-via-a-boot-option
> +++ a/arch/i386/kernel/io_apic.c
> @@ -1930,6 +1930,15 @@ static void __init setup_ioapic_ids_from
>  static void __init setup_ioapic_ids_from_mpc(void) { }
>  #endif
>  
> +static int no_timer_check __initdata;
> +
> +static int __init notimercheck(char *s)
> +{
> +	no_timer_check = 1;
> +	return 1;
> +}
> +__setup("no_timer_check", notimercheck);
> +
>  /*
>   * There is a nasty bug in some older SMP boards, their mptable lies
>   * about the timer IRQ. We do the following to work around the situation:
> @@ -1938,10 +1947,13 @@ static void __init setup_ioapic_ids_from
>   *	- if this function detects that timer IRQs are defunct, then we fall
>   *	  back to ISA timer IRQs
>   */
> -static int __init timer_irq_works(void)
> +int __init timer_irq_works(void)
>  {
>  	unsigned long t1 = jiffies;
>  
> +	if (no_timer_check)
> +		return 1;
> +
>  	local_irq_enable();
>  	/* Let ten ticks pass... */
>  	mdelay((10 * 1000) / HZ);
> @@ -2212,7 +2224,7 @@ int timer_uses_ioapic_pin_0;
>   * is so screwy.  Thanks to Brian Perkins for testing/hacking this beast
>   * fanatically on his truly buggy board.
>   */
> -static inline void check_timer(void)
> +static inline void __init check_timer(void)
>  {
>  	int apic1, pin1, apic2, pin2;
>  	int vector;
> diff -puN Documentation/kernel-parameters.txt~i386-add-a-way-to-disable-the-timer-irq-routing-check-via-a-boot-option Documentation/kernel-parameters.txt
> --- a/Documentation/kernel-parameters.txt~i386-add-a-way-to-disable-the-timer-irq-routing-check-via-a-boot-option
> +++ a/Documentation/kernel-parameters.txt
> @@ -599,8 +599,6 @@ and is between 256 and 4096 characters. 
>  
>  	hugepages=	[HW,IA-32,IA-64] Maximal number of HugeTLB pages.
>  
> -	noirqbalance	[IA-32,SMP,KNL] Disable kernel irq balancing
> -
>  	i8042.direct	[HW] Put keyboard port into non-translated mode
>  	i8042.dumbkbd	[HW] Pretend that controller can only read data from
>  			     keyboard and cannot control its state
> @@ -1056,9 +1054,14 @@ and is between 256 and 4096 characters. 
>  			in certain environments such as networked servers or
>  			real-time systems.
>  
> +	noirqbalance	[IA-32,SMP,KNL] Disable kernel irq balancing
> +
>  	noirqdebug	[IA-32] Disables the code which attempts to detect and
>  			disable unhandled interrupt sources.
>  
> +	no_timer_check	[IA-32,X86_64,APIC] Disables the code which tests for
> +			broken timer IRQ sources.
> +
>  	noisapnp	[ISAPNP] Disables ISA PnP code.
>  
>  	noinitrd	[RAM] Tells the kernel not to load any configured
> _

Re: [PATCH 1/5] Skip timer works.patch

[Andi Kleen]

On Thursday 16 November 2006 21:24, Chris Wright wrote:
> * Andi Kleen (ak@suse.de) wrote:
> > > > Inconsistency detected by ld.so: rtld.c: 1250: dl_main: Assertion `(void *) ph->
> > > > p_vaddr == _rtld_local._dl_sysinfo_dso' failed!
> > 
> > I started some bisecting...
> <snip patch list>
> > it's already broken at more-generic-paravirtualization.  Surprised
> > me a little, i was suspecting the MMU changes.
> 
> Looks like the VDSO issue, paravirt is not compatible if COMPAT_VDSO.

So you're not planning to support old user land with paravirt? 
Wouldn't it be better to just disable the vDSO completely then? At least
all old binaries would work then.

> I expect you'll get the same if you disable COMPAT_VDSO on native build?

Yes. This is a older SUSE 9.0 image.

-Andi

Re: [PATCH 1/5] Skip timer works.patch

[Andi Kleen]

> Andi seems to have merged this patch but from somewhere I picked up a
> different version, below.
> 
> I think the version I have is better.  Because the patch Andi has merged is
> cast in terms of "irq testing", which is broad.  But that's not what the
> patch does - the patch handles only timers.

Agreed. I updated to your version now.

-Andi

Re: [PATCH 1/5] Skip timer works.patch

[Chris Wright]

* Andi Kleen (ak@suse.de) wrote:
> On Thursday 16 November 2006 21:24, Chris Wright wrote:
> > Looks like the VDSO issue, paravirt is not compatible if COMPAT_VDSO.
> 
> So you're not planning to support old user land with paravirt? 
> Wouldn't it be better to just disable the vDSO completely then? At least
> all old binaries would work then.

Oh, it can be disabled by runtime (/proc/sys/kernel/vdso_enabled) or boot
time (vdso=0) option already.  And it should be only an interim timeslice
that is broken, since earlier glibc could only do int 0x80 and newer glibc
can cope with non fixed vDSO.  So most users can just leave it enabled.

thanks,
-chris

Re: [PATCH 1/5] Skip timer works.patch

[Andi Kleen]

> Oh, it can be disabled by runtime (/proc/sys/kernel/vdso_enabled) or boot
> time (vdso=0) option already.

So you expect everybody using para virtualization to set this option? 

> And it should be only an interim timeslice 
> that is broken, since earlier glibc could only do int 0x80 and newer glibc
> can cope with non fixed vDSO.  So most users can just leave it enabled.

I'm sure there are quite a lot of people to still use SUSE 9.0 and 
similar time frame RH and debian etc.

To be honest, I don't think this is a particularly useful position.
Nobody wants to set weird command line options. It should just work.
We should value binary compatibility out of the box higher.

I guess i'll just disable the vDSO with CONFIG_PARAVIRT unless someone
comes up with a patch to fix it in a compatible way.

-Andi