Re: [patch 2/2] x86,apic: Use logical OR in noop-write operation

[Cyrill Gorcunov <gorcunov@gmail.com>]

On Tue, Dec 08, 2009 at 11:48:38PM +0300, Cyrill Gorcunov wrote:
> On Tue, Dec 08, 2009 at 06:53:18PM +0300, Cyrill Gorcunov wrote:
> > For apic noop'ified we have to use logical OR statement,
> > otherwise any write on systems shipped with 82489DX
> > (where apic presence bit can't be retrieved via cpuid)
> > will not trigger the warning which is not desired.
> >
> ...
> 
> Ingo please dont apply this patch. Perhaps we may warn unconditionally
> (as Peter marked) which would be more clear approach indeed. Need more
> time to check all code flows.
> 
> Sorry for inconvenience.
> 
> 	-- Cyrill

Here is what done at moment. I've grepped x86 arch for apic_write and
except thermal monitoring (the patch is already sent) all other callers
do check if apic is active (either via cpu_has_apic or disable_apic).
So I think we may safely use unconditional warning if apic_write with
disabled apic is called.

Please take a look -- I would be glad to hear any comments/complains.

There is unclear moment for me with "SGI Visual Workstation" which
has ack_cobalt_irq and calls for apic_write which could trigger this
warning.

	-- Cyrill
---
x86,apic: Warn on noop_apic_write unconditionally

In apic noop'ified we should never call for write operation.
Otherwise it's a caller bug (we should be WARNed about).

Also add a comment on noop_apic_read WARN conditions.

CC: H. Peter Anvin <hpa@zytor.com>
CC: Thomas Gleixner <tglx@linutronix.de>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: Maciej W. Rozycki <macro@linux-mips.org>
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---

 arch/x86/kernel/apic/apic_noop.c |   17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

Index: linux-2.6.git/arch/x86/kernel/apic/apic_noop.c
=====================================================================
--- linux-2.6.git.orig/arch/x86/kernel/apic/apic_noop.c
+++ linux-2.6.git/arch/x86/kernel/apic/apic_noop.c
@@ -119,15 +119,28 @@ int noop_apicid_to_node(int logical_apic
 	return 0;
 }
 
+/*
+ * note that we allow this routine to be called
+ * under the following conditions:
+ *  - apic was explicitly disabled via boot option
+ *  - on old 486 machines (which has no apic presence bit
+ *    retrieved via cpuid)
+ * this is done only in a sake of callers code simplicity
+ */
 static u32 noop_apic_read(u32 reg)
 {
-	WARN_ON_ONCE((cpu_has_apic && !disable_apic));
+	WARN_ON_ONCE(cpu_has_apic && !disable_apic);
 	return 0;
 }
 
 static void noop_apic_write(u32 reg, u32 v)
 {
-	WARN_ON_ONCE(cpu_has_apic && !disable_apic);
+	/*
+	 * If someone is trying to write apic
+	 * register when it is NOOP'ified
+	 * this is a bug on caller side
+	 */
+	WARN_ON_ONCE(1);
 }
 
 struct apic apic_noop = {