[patch 0/2] apic snipptes on latest -tip/master

[patch 0/2] apic snipptes on latest -tip/master

[Cyrill Gorcunov]

Please review, comments/complains are quite welcome.
First patch -- to add checking if we have apic being active
why trying to setup thermal monitor interrupt, and the second
one -- my attempt to explain why noop'ified apic read/write
do check for apic being active/allowed to operate in a different manner.

Cyrill

[patch 1/2] x86,mcheck: Thermal monitoring depends on APIC being enabled

[Cyrill Gorcunov]

[-- Attachment #1: x86-thermal-mon --]
[-- Type: text/plain, Size: 1466 bytes --]

Add check if APIC is not disabled since thermal
monitoring depends on it. As only apic gets disabled
we should not try to install "thermal monitor" vector,
print out that thermal monitoring is enabled and etc...

Note that "Intel Correct Machine Check Interrupts" already
has such a check as well.

Also I decided to not add cpu_has_apic check into mcheck_intel_therm_init
since even if it'll call apic_read on disabled apic -- it's safe
here and allow us to save a few code bytes.

Reported-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---

Please review. This should close a number of WARNs triggered
by intel_init_thermal when apic gets disabled state.

 arch/x86/kernel/cpu/mcheck/therm_throt.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Index: linux-2.6.git/arch/x86/kernel/cpu/mcheck/therm_throt.c
=====================================================================
--- linux-2.6.git.orig/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ linux-2.6.git/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -274,8 +274,9 @@ void intel_init_thermal(struct cpuinfo_x
 	int tm2 = 0;
 	u32 l, h;
 
-	/* Thermal monitoring depends on ACPI and clock modulation*/
-	if (!cpu_has(c, X86_FEATURE_ACPI) || !cpu_has(c, X86_FEATURE_ACC))
+	/* Thermal monitoring depends on APIC, ACPI and clock modulation */
+	if (!cpu_has_apic || !cpu_has(c, X86_FEATURE_ACPI) ||
+		!cpu_has(c, X86_FEATURE_ACC))
 		return;
 
 	/*

Re: [patch 1/2] x86,mcheck: Thermal monitoring depends on APIC being enabled

[Cyrill Gorcunov]

On Tue, Dec 08, 2009 at 06:53:17PM +0300, Cyrill Gorcunov wrote:
> Add check if APIC is not disabled since thermal
> monitoring depends on it. As only apic gets disabled
> we should not try to install "thermal monitor" vector,
> print out that thermal monitoring is enabled and etc...
> 
> Note that "Intel Correct Machine Check Interrupts" already
> has such a check as well.
> 
> Also I decided to not add cpu_has_apic check into mcheck_intel_therm_init
> since even if it'll call apic_read on disabled apic -- it's safe
> here and allow us to save a few code bytes.
> 
> Reported-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> ---
>

Though the second patch is not for merging (i'm working on it) --
this one is still valid.
 
	-- Cyrill

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

[Cyrill Gorcunov]

[-- Attachment #1: x86-apic-noop-read-write --]
[-- Type: text/plain, Size: 2991 bytes --]

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.

In turn noop'ified read operation remains using logical
AND statement. This will catch _only_ future code bugs
since:

1) The old 32bit systems without apic presence bit use
   disable_apic only as a flag that chip was turned off
   via boot option but not due to MP-table (BIOS) bug where
   we may try to re-enable apic via MSR registers. The further
   SMP setup code already prepared for a such situation
   (ie it disables SMP support) and do not issue write
   operations but still needs to issue apic_read. So instead
   of deforming code with "if" we just allow reads until
   SMP support gets disabled. But even then we still need
   apic_read issued on a such machine at shutdown procedure.

2) x86-64 at moment properly uses cpu_has_apic and turn
   this feature off as only chip is disabled together
   with disable_apic flag.

CC: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---

Please review, I hope this explanation is more or less good
though I would be glad to hear complains a well :)

 arch/x86/kernel/apic/apic_noop.c |   30 ++++++++++++++++++++++++++++--
 1 file changed, 28 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,41 @@ int noop_apicid_to_node(int logical_apic
 	return 0;
 }
 
+/*
+ * we use logical AND here just to suppress a
+ * number of WARNs which would take place all
+ * over the code if the kernel is SMP compatible
+ * and we would need to deform code with a lot
+ * of "if" statements then (especially on x86-32
+ * with discrete apic chip)
+ *
+ * note that even if apic was not disabled via
+ * boot option we still may have apic broken
+ * due to buggy BIOS/MP-table/ACPI so we allow
+ * any read operation on a such systems 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;
 }
 
+/*
+ * we don't use logical AND here because otherwise
+ * any writes on 486DXSL (which has no apic presence
+ * bit retrieved via cpuid) will never trigger
+ * this WARN allowing a (possible buggy) code to
+ * continue executing, so consider this WARN as
+ * a strict guard against unpredicted/inaccurate
+ * apic code modifications or/and buggy callers
+ *
+ * in short -- warn every write on enabled apic
+ * (both 82489DX and integrated LAPICs)
+ */
 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: [patch 2/2] x86,apic: Use logical OR in noop-write operation

[H. Peter Anvin]

On 12/08/2009 07:53 AM, 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.
> 
> In turn noop'ified read operation remains using logical
> AND statement. This will catch _only_ future code bugs
> since:
> 
> 1) The old 32bit systems without apic presence bit use
>    disable_apic only as a flag that chip was turned off
>    via boot option but not due to MP-table (BIOS) bug where
>    we may try to re-enable apic via MSR registers. The further
>    SMP setup code already prepared for a such situation
>    (ie it disables SMP support) and do not issue write
>    operations but still needs to issue apic_read. So instead
>    of deforming code with "if" we just allow reads until
>    SMP support gets disabled. But even then we still need
>    apic_read issued on a such machine at shutdown procedure.
> 
> 2) x86-64 at moment properly uses cpu_has_apic and turn
>    this feature off as only chip is disabled together
>    with disable_apic flag.
> 

Could you clarify the question I asked yesterday: how is the "or"
different from just warning unconditionally (which would at least be a
lot more clear)?  Can the situation that !cpu_has_apic && disable_apic
actually happen and we *still* end up in apic_write?

	-hpa

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

[Cyrill Gorcunov]

On Tue, Dec 08, 2009 at 12:10:24PM -0800, H. Peter Anvin wrote:
> On 12/08/2009 07:53 AM, 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.
> > 
> > In turn noop'ified read operation remains using logical
> > AND statement. This will catch _only_ future code bugs
> > since:
> > 
> > 1) The old 32bit systems without apic presence bit use
> >    disable_apic only as a flag that chip was turned off
> >    via boot option but not due to MP-table (BIOS) bug where
> >    we may try to re-enable apic via MSR registers. The further
> >    SMP setup code already prepared for a such situation
> >    (ie it disables SMP support) and do not issue write
> >    operations but still needs to issue apic_read. So instead
> >    of deforming code with "if" we just allow reads until
> >    SMP support gets disabled. But even then we still need
> >    apic_read issued on a such machine at shutdown procedure.
> > 
> > 2) x86-64 at moment properly uses cpu_has_apic and turn
> >    this feature off as only chip is disabled together
> >    with disable_apic flag.
> > 
> 
> Could you clarify the question I asked yesterday: how is the "or"
> different from just warning unconditionally (which would at least be a
> lot more clear)?

Hmm, indeed it seems that unconditional WARN would be more clean
and reliable. Will recheck all apic_write calls.

> Can the situation that !cpu_has_apic && disable_apic
> actually happen and we *still* end up in apic_write?

No, this situation should not take place as far as I remember.

> 
> 	-hpa
> 
	-- Cyrill

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

[Cyrill Gorcunov]

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

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

[Cyrill Gorcunov]

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 = {