[PATCH] x86: Fix bogus warning in apic_noop.apic_write()

[PATCH] x86: Fix bogus warning in apic_noop.apic_write()

[Thomas Gleixner]

apic_noop is used to provide dummy apic functions. It's installed when
the CPU has no APIC or when the APIC is disabled on the kernel command
line.

The apic_noop implementation of apic_write() warns when the CPU has an
APIC or when the APIC is not disabled.

That's bogus. The warning should only happen when the CPU has an APIC
_AND_ the APIC is not disabled. apic_noop.apic_read() has the correct
check.

[ @stable: in <= .32 this typo resides in native_apic_write_dummy(). ]

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@kernel.org
---
 arch/x86/kernel/apic/apic_noop.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6/arch/x86/kernel/apic/apic_noop.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/apic/apic_noop.c
+++ linux-2.6/arch/x86/kernel/apic/apic_noop.c
@@ -127,7 +127,7 @@ static u32 noop_apic_read(u32 reg)
 
 static void noop_apic_write(u32 reg, u32 v)
 {
-	WARN_ON_ONCE((cpu_has_apic || !disable_apic));
+	WARN_ON_ONCE((cpu_has_apic && !disable_apic));
 }
 
 struct apic apic_noop = {

[tip:x86/urgent] x86: Fix bogus warning in apic_noop.apic_write()

[tip-bot for Thomas Gleixner]

Commit-ID:  a946d8f11f0da9cfc714248036fcfd3a794d1e27
Gitweb:     http://git.kernel.org/tip/a946d8f11f0da9cfc714248036fcfd3a794d1e27
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Mon, 7 Dec 2009 12:59:46 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Mon, 7 Dec 2009 13:16:37 +0100

x86: Fix bogus warning in apic_noop.apic_write()

apic_noop is used to provide dummy apic functions. It's installed
when the CPU has no APIC or when the APIC is disabled on the kernel
command line.

The apic_noop implementation of apic_write() warns when the CPU has
an APIC or when the APIC is not disabled.

That's bogus. The warning should only happen when the CPU has an
APIC _AND_ the APIC is not disabled. apic_noop.apic_read() has the
correct check.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Cyrill Gorcunov <gorcunov@openvz.org>
Cc: <stable@kernel.org> # in <= .32 this typo resides in native_apic_write_dummy()
LKML-Reference: <alpine.LFD.2.00.0912071255420.3089@localhost.localdomain>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/apic/apic_noop.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/apic/apic_noop.c b/arch/x86/kernel/apic/apic_noop.c
index d9acc3b..e31b9ff 100644
--- a/arch/x86/kernel/apic/apic_noop.c
+++ b/arch/x86/kernel/apic/apic_noop.c
@@ -127,7 +127,7 @@ static u32 noop_apic_read(u32 reg)
 
 static void noop_apic_write(u32 reg, u32 v)
 {
-	WARN_ON_ONCE((cpu_has_apic || !disable_apic));
+	WARN_ON_ONCE(cpu_has_apic && !disable_apic);
 }
 
 struct apic apic_noop = {

Re: [tip:x86/urgent] x86: Fix bogus warning in apic_noop.apic_write()

[Cyrill Gorcunov]

On Mon, Dec 07, 2009 at 12:18:37PM +0000, tip-bot for Thomas Gleixner wrote:
> Commit-ID:  a946d8f11f0da9cfc714248036fcfd3a794d1e27
> Gitweb:     http://git.kernel.org/tip/a946d8f11f0da9cfc714248036fcfd3a794d1e27
> Author:     Thomas Gleixner <tglx@linutronix.de>
> AuthorDate: Mon, 7 Dec 2009 12:59:46 +0100
> Committer:  Ingo Molnar <mingo@elte.hu>
> CommitDate: Mon, 7 Dec 2009 13:16:37 +0100
> 
> x86: Fix bogus warning in apic_noop.apic_write()
> 
> apic_noop is used to provide dummy apic functions. It's installed
> when the CPU has no APIC or when the APIC is disabled on the kernel
> command line.
> 
> The apic_noop implementation of apic_write() warns when the CPU has
> an APIC or when the APIC is not disabled.
> 
> That's bogus. The warning should only happen when the CPU has an
> APIC _AND_ the APIC is not disabled. apic_noop.apic_read() has the
> correct check.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Cyrill Gorcunov <gorcunov@openvz.org>
> Cc: <stable@kernel.org> # in <= .32 this typo resides in native_apic_write_dummy()
> LKML-Reference: <alpine.LFD.2.00.0912071255420.3089@localhost.localdomain>
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> ---
>  arch/x86/kernel/apic/apic_noop.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
...

Hi Thomas, Ingo,

please do not change it. There are still machines without
cpu_has_apic bit support so with this patch any attempt
to write to 82489DX will success. So the former code has
been using "OR" by a purpose, there is no error.

	-- Cyrill

Re: [tip:x86/urgent] x86: Fix bogus warning in apic_noop.apic_write()

[Thomas Gleixner]

On Mon, 7 Dec 2009, Cyrill Gorcunov wrote:
> On Mon, Dec 07, 2009 at 12:18:37PM +0000, tip-bot for Thomas Gleixner wrote:
> > Commit-ID:  a946d8f11f0da9cfc714248036fcfd3a794d1e27
> > Gitweb:     http://git.kernel.org/tip/a946d8f11f0da9cfc714248036fcfd3a794d1e27
> > Author:     Thomas Gleixner <tglx@linutronix.de>
> > AuthorDate: Mon, 7 Dec 2009 12:59:46 +0100
> > Committer:  Ingo Molnar <mingo@elte.hu>
> > CommitDate: Mon, 7 Dec 2009 13:16:37 +0100
> > 
> > x86: Fix bogus warning in apic_noop.apic_write()
> > 
> > apic_noop is used to provide dummy apic functions. It's installed
> > when the CPU has no APIC or when the APIC is disabled on the kernel
> > command line.
> > 
> > The apic_noop implementation of apic_write() warns when the CPU has
> > an APIC or when the APIC is not disabled.
> > 
> > That's bogus. The warning should only happen when the CPU has an
> > APIC _AND_ the APIC is not disabled. apic_noop.apic_read() has the
> > correct check.
> > 
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Cyrill Gorcunov <gorcunov@openvz.org>
> > Cc: <stable@kernel.org> # in <= .32 this typo resides in native_apic_write_dummy()
> > LKML-Reference: <alpine.LFD.2.00.0912071255420.3089@localhost.localdomain>
> > Signed-off-by: Ingo Molnar <mingo@elte.hu>
> > ---
> >  arch/x86/kernel/apic/apic_noop.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> ...
> 
> Hi Thomas, Ingo,
> 
> please do not change it. There are still machines without
> cpu_has_apic bit support so with this patch any attempt
> to write to 82489DX will success. So the former code has
> been using "OR" by a purpose, there is no error.

Err, your warning has the following false positive:

     cpu_has_apic == true and disable_apic == true

Which is crap, as it warns just because someone disabled the APIC on
the command line and the kernel did the right thing of installing
apic_noop.

And I have a hard time to see how this is related to 82489DX.
 
Thanks,

	tglx

Re: [tip:x86/urgent] x86: Fix bogus warning in apic_noop.apic_write()

[Cyrill Gorcunov]

On Mon, Dec 07, 2009 at 04:48:51PM +0100, Thomas Gleixner wrote:
> On Mon, 7 Dec 2009, Cyrill Gorcunov wrote:
> > On Mon, Dec 07, 2009 at 12:18:37PM +0000, tip-bot for Thomas Gleixner wrote:
> > > Commit-ID:  a946d8f11f0da9cfc714248036fcfd3a794d1e27
> > > Gitweb:     http://git.kernel.org/tip/a946d8f11f0da9cfc714248036fcfd3a794d1e27
> > > Author:     Thomas Gleixner <tglx@linutronix.de>
> > > AuthorDate: Mon, 7 Dec 2009 12:59:46 +0100
> > > Committer:  Ingo Molnar <mingo@elte.hu>
> > > CommitDate: Mon, 7 Dec 2009 13:16:37 +0100
> > > 
> > > x86: Fix bogus warning in apic_noop.apic_write()
> > > 
> > > apic_noop is used to provide dummy apic functions. It's installed
> > > when the CPU has no APIC or when the APIC is disabled on the kernel
> > > command line.
> > > 
> > > The apic_noop implementation of apic_write() warns when the CPU has
> > > an APIC or when the APIC is not disabled.
> > > 
> > > That's bogus. The warning should only happen when the CPU has an
> > > APIC _AND_ the APIC is not disabled. apic_noop.apic_read() has the
> > > correct check.
> > > 
> > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > > Cc: Cyrill Gorcunov <gorcunov@openvz.org>
> > > Cc: <stable@kernel.org> # in <= .32 this typo resides in native_apic_write_dummy()
> > > LKML-Reference: <alpine.LFD.2.00.0912071255420.3089@localhost.localdomain>
> > > Signed-off-by: Ingo Molnar <mingo@elte.hu>
> > > ---
> > >  arch/x86/kernel/apic/apic_noop.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > ...
> > 
> > Hi Thomas, Ingo,
> > 
> > please do not change it. There are still machines without
> > cpu_has_apic bit support so with this patch any attempt
> > to write to 82489DX will success. So the former code has
> > been using "OR" by a purpose, there is no error.
> 
> Err, your warning has the following false positive:
> 
>      cpu_has_apic == true and disable_apic == true

This combination impossible at moment (by "impossible" I mean
at moment of apic_write action). When apic disabled via boot
option cpu_has_apic cleared as well.

static int __init setup_disableapic(char *arg)
{
	disable_apic = 1;
	setup_clear_cpu_cap(X86_FEATURE_APIC);
	return 0;
}

And, btw if some code is trying to write to apic when
it's disabled via boot option -- it means the code is
buggy and this is not a false positive but rather proper
warning.

Thomas, if you've changed this code I suppose you saw some
warning triggered, right? Could you pointed me on it?

The idea was exactly to use "OR" here, so any attempts to
make WRITE on dosabled apic were captured.

> 
> Which is crap, as it warns just because someone disabled the APIC on
> the command line and the kernel did the right thing of installing
> apic_noop.
> 
> And I have a hard time to see how this is related to 82489DX.

On 82489DX there is no X86_FEATURE_APIC at all, so cpu_has_apic
is never set and disable_apic is the only flag we inspect to find
out if we're allowed to operate over apic.

An example you may find in APIC_init_uniprocessor, the first check
is done for disable_apic, not cpu_has_apic.

>  
> Thanks,
> 
> 	tglx
> 

	-- Cyrill

Re: [tip:x86/urgent] x86: Fix bogus warning in apic_noop.apic_write()

[Thomas Gleixner]

On Mon, 7 Dec 2009, Cyrill Gorcunov wrote:
> On Mon, Dec 07, 2009 at 04:48:51PM +0100, Thomas Gleixner wrote:
> > On Mon, 7 Dec 2009, Cyrill Gorcunov wrote:
> > > On Mon, Dec 07, 2009 at 12:18:37PM +0000, tip-bot for Thomas Gleixner wrote:
> > > > Commit-ID:  a946d8f11f0da9cfc714248036fcfd3a794d1e27
> > > > Gitweb:     http://git.kernel.org/tip/a946d8f11f0da9cfc714248036fcfd3a794d1e27
> > > > Author:     Thomas Gleixner <tglx@linutronix.de>
> > > > AuthorDate: Mon, 7 Dec 2009 12:59:46 +0100
> > > > Committer:  Ingo Molnar <mingo@elte.hu>
> > > > CommitDate: Mon, 7 Dec 2009 13:16:37 +0100
> > > > 
> > > > x86: Fix bogus warning in apic_noop.apic_write()
> > > > 
> > > > apic_noop is used to provide dummy apic functions. It's installed
> > > > when the CPU has no APIC or when the APIC is disabled on the kernel
> > > > command line.
> > > > 
> > > > The apic_noop implementation of apic_write() warns when the CPU has
> > > > an APIC or when the APIC is not disabled.
> > > > 
> > > > That's bogus. The warning should only happen when the CPU has an
> > > > APIC _AND_ the APIC is not disabled. apic_noop.apic_read() has the
> > > > correct check.
> > > > 
> > > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > > > Cc: Cyrill Gorcunov <gorcunov@openvz.org>
> > > > Cc: <stable@kernel.org> # in <= .32 this typo resides in native_apic_write_dummy()
> > > > LKML-Reference: <alpine.LFD.2.00.0912071255420.3089@localhost.localdomain>
> > > > Signed-off-by: Ingo Molnar <mingo@elte.hu>
> > > > ---
> > > >  arch/x86/kernel/apic/apic_noop.c |    2 +-
> > > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > ...
> > > 
> > > Hi Thomas, Ingo,
> > > 
> > > please do not change it. There are still machines without
> > > cpu_has_apic bit support so with this patch any attempt
> > > to write to 82489DX will success. So the former code has
> > > been using "OR" by a purpose, there is no error.
> > 
> > Err, your warning has the following false positive:
> > 
> >      cpu_has_apic == true and disable_apic == true
> 
> This combination impossible at moment (by "impossible" I mean
> at moment of apic_write action). When apic disabled via boot
> option cpu_has_apic cleared as well.
> 
> static int __init setup_disableapic(char *arg)
> {
> 	disable_apic = 1;
> 	setup_clear_cpu_cap(X86_FEATURE_APIC);
> 	return 0;
> }
> 
> And, btw if some code is trying to write to apic when
> it's disabled via boot option -- it means the code is
> buggy and this is not a false positive but rather proper
> warning.
> 
> Thomas, if you've changed this code I suppose you saw some
> warning triggered, right? Could you pointed me on it?

http://www.kerneloops.org/searchweek.php?search=native_apic_write_dummy

Re: [tip:x86/urgent] x86: Fix bogus warning in apic_noop.apic_write()

[Cyrill Gorcunov]

On Mon, Dec 07, 2009 at 05:55:37PM +0100, Thomas Gleixner wrote:
...
> > And, btw if some code is trying to write to apic when
> > it's disabled via boot option -- it means the code is
> > buggy and this is not a false positive but rather proper
> > warning.
> > 
> > Thomas, if you've changed this code I suppose you saw some
> > warning triggered, right? Could you pointed me on it?
> 
> http://www.kerneloops.org/searchweek.php?search=native_apic_write_dummy
> 

Doh!

The most cases show inapropriate usage of apic->write() operation.
set_perf_event_pending() already fixed by
7d42896628202a551ad1107697cd215dc5fca099,
intel_init_thermal() fixed as well with
5ce4243dcefbbc43791ffc36e1be55067ceec916

(all was in -tip).

Though throttling code is just buggy and intel_init_thermal() should
check if cpu_has_apic. So the former code does exactly what
it should -- it catches inapropriate writes.

Thomas, I just don't know -- from my pov, write() is really different
from read(), since it implies that APIC changes it behaviour, it could
be timer setup, vector setup operation or whatever. I even doubt if
enabling IPI in apic-noop is a good idea (since perf code already
implemented to check apic presence by Peter and IPI is not called).

Though I'm not insisting, I simply don't have spare time at moment
to check all apic_writes() again :(

	-- Cyrill