[RFC -tip] x86,apic -- reduce disable_apic usage

[RFC -tip] x86,apic -- reduce disable_apic usage

[Cyrill Gorcunov]

Here is an attempt to bring more consistency for apic
presence check. I hope I covered all the code flows.
Though it's RFC. Please review and test if possible.

	-- Cyrill
---
Instead of messing with (cpu_has_apic|disable_apic)
to check if we have apic functional, better to distinguish
them by logical kind. cpu_has_apic -- to check if we may
use apic functionality, disable_apic -- to note that
apic was disabled via command line.

Other interesting (and a side one) issue I guess is that
X86_FEATURE_APIC feature bit is always cleared if apic
disabled via command line but not the reverse.

Eventually this allow us to save a few lines of code.

Also:

1) No need to clear bit twice in APIC_init_uniprocessor.
   cpu_has_apic check for boot_cpu_data bit anyway.
2) Don't check for disable_apic in print_all_ICs. It's
   already set. Always.
3) Don't set disable_apic if there is no MPS support built in
   with acpi enabled. Even having SMP built kernel we're
   protected by acpi_lapic check in smp_sanity_check.
   At least we should be.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
 arch/x86/kernel/apic/apic.c    |    3 +--
 arch/x86/kernel/apic/io_apic.c |    6 +++---
 arch/x86/kernel/setup.c        |    6 +-----
 3 files changed, 5 insertions(+), 10 deletions(-)

Index: linux-2.6.git/arch/x86/kernel/apic/apic.c
=====================================================================
--- linux-2.6.git.orig/arch/x86/kernel/apic/apic.c
+++ linux-2.6.git/arch/x86/kernel/apic/apic.c
@@ -1173,7 +1173,7 @@ void __cpuinit setup_local_APIC(void)
 	unsigned int value;
 	int i, j;
 
-	if (disable_apic) {
+	if (!cpu_has_apic) {
 		arch_disable_smp_support();
 		return;
 	}
@@ -1651,7 +1651,6 @@ int __init APIC_init_uniprocessor(void)
 	    APIC_INTEGRATED(apic_version[boot_cpu_physical_apicid])) {
 		pr_err("BIOS bug, local APIC 0x%x not detected!...\n",
 			boot_cpu_physical_apicid);
-		clear_cpu_cap(&boot_cpu_data, X86_FEATURE_APIC);
 		return -1;
 	}
 #endif
Index: linux-2.6.git/arch/x86/kernel/apic/io_apic.c
=====================================================================
--- linux-2.6.git.orig/arch/x86/kernel/apic/io_apic.c
+++ linux-2.6.git/arch/x86/kernel/apic/io_apic.c
@@ -1884,7 +1884,7 @@ __apicdebuginit(int) print_all_ICs(void)
 	print_PIC();
 
 	/* don't print out if apic is not there */
-	if (!cpu_has_apic || disable_apic)
+	if (!cpu_has_apic)
 		return 0;
 
 	print_all_local_APICs();
@@ -3261,7 +3261,7 @@ static int msi_compose_msg(struct pci_de
 	int err;
 	unsigned dest;
 
-	if (disable_apic)
+	if (!cpu_has_apic)
 		return -ENXIO;
 
 	cfg = irq_cfg(irq);
@@ -3714,7 +3714,7 @@ int arch_setup_ht_irq(unsigned int irq, 
 	struct irq_cfg *cfg;
 	int err;
 
-	if (disable_apic)
+	if (!cpu_has_apic)
 		return -ENXIO;
 
 	cfg = irq_cfg(irq);
Index: linux-2.6.git/arch/x86/kernel/setup.c
=====================================================================
--- linux-2.6.git.orig/arch/x86/kernel/setup.c
+++ linux-2.6.git/arch/x86/kernel/setup.c
@@ -795,12 +795,8 @@ void __init setup_arch(char **cmdline_p)
 	/* after early param, so could get panic from serial */
 	reserve_early_setup_data();
 
-	if (acpi_mps_check()) {
-#ifdef CONFIG_X86_LOCAL_APIC
-		disable_apic = 1;
-#endif
+	if (acpi_mps_check())
 		setup_clear_cpu_cap(X86_FEATURE_APIC);
-	}
 
 #ifdef CONFIG_PCI
 	if (pci_early_dump_regs)

Re: [RFC -tip] x86,apic -- reduce disable_apic usage

[Maciej W. Rozycki]

On Sun, 5 Jul 2009, Cyrill Gorcunov wrote:

> Here is an attempt to bring more consistency for apic
> presence check. I hope I covered all the code flows.
> Though it's RFC. Please review and test if possible.
> 
> 	-- Cyrill
> ---
> Instead of messing with (cpu_has_apic|disable_apic)
> to check if we have apic functional, better to distinguish
> them by logical kind. cpu_has_apic -- to check if we may
> use apic functionality, disable_apic -- to note that
> apic was disabled via command line.

 How do you set cpu_has_apic for systems with discrete local APICs?  The 
CPUID flag is not set in this case.

  Maciej

Re: [RFC -tip] x86,apic -- reduce disable_apic usage

[Cyrill Gorcunov]

[Maciej W. Rozycki - Sun, Jul 05, 2009 at 05:38:52PM +0100]
| On Sun, 5 Jul 2009, Cyrill Gorcunov wrote:
| 
| > Here is an attempt to bring more consistency for apic
| > presence check. I hope I covered all the code flows.
| > Though it's RFC. Please review and test if possible.
| > 
| > 	-- Cyrill
| > ---
| > Instead of messing with (cpu_has_apic|disable_apic)
| > to check if we have apic functional, better to distinguish
| > them by logical kind. cpu_has_apic -- to check if we may
| > use apic functionality, disable_apic -- to note that
| > apic was disabled via command line.
| 
|  How do you set cpu_has_apic for systems with discrete local APICs?  The 
| CPUID flag is not set in this case.
| 
|   Maciej
| --
| 

Well, indeed, somehow forgot about this case. Thanks Maciej!
The patch should be dropped.

	-- Cyrill

Re: [RFC -tip] x86,apic -- reduce disable_apic usage

[Cyrill Gorcunov]

[Cyrill Gorcunov - Sun, Jul 05, 2009 at 08:59:47PM +0400]
| [Maciej W. Rozycki - Sun, Jul 05, 2009 at 05:38:52PM +0100]
| | On Sun, 5 Jul 2009, Cyrill Gorcunov wrote:
| | 
| | > Here is an attempt to bring more consistency for apic
| | > presence check. I hope I covered all the code flows.
| | > Though it's RFC. Please review and test if possible.
| | > 
| | > 	-- Cyrill
| | > ---
| | > Instead of messing with (cpu_has_apic|disable_apic)
| | > to check if we have apic functional, better to distinguish
| | > them by logical kind. cpu_has_apic -- to check if we may
| | > use apic functionality, disable_apic -- to note that
| | > apic was disabled via command line.
| | 
| |  How do you set cpu_has_apic for systems with discrete local APICs?  The 
| | CPUID flag is not set in this case.
| | 
| |   Maciej
| | --
| | 
| 
| Well, indeed, somehow forgot about this case. Thanks Maciej!
| The patch should be dropped.
| 
| 	-- Cyrill

Hmm... But if we have no MP table parsed and no "lapic" option
passed we should leave execution in pure PIC mode. Or I miss
something?

	-- Cyrill

Re: [RFC -tip] x86,apic -- reduce disable_apic usage

[Cyrill Gorcunov]

[Cyrill Gorcunov - Sun, Jul 05, 2009 at 09:12:51PM +0400]
| [Cyrill Gorcunov - Sun, Jul 05, 2009 at 08:59:47PM +0400]
| | [Maciej W. Rozycki - Sun, Jul 05, 2009 at 05:38:52PM +0100]
| | | On Sun, 5 Jul 2009, Cyrill Gorcunov wrote:
| | | 
| | | > Here is an attempt to bring more consistency for apic
| | | > presence check. I hope I covered all the code flows.
| | | > Though it's RFC. Please review and test if possible.
| | | > 
| | | > 	-- Cyrill
| | | > ---
| | | > Instead of messing with (cpu_has_apic|disable_apic)
| | | > to check if we have apic functional, better to distinguish
| | | > them by logical kind. cpu_has_apic -- to check if we may
| | | > use apic functionality, disable_apic -- to note that
| | | > apic was disabled via command line.
| | | 
| | |  How do you set cpu_has_apic for systems with discrete local APICs?  The 
| | | CPUID flag is not set in this case.
| | | 
| | |   Maciej
| | | --
| | | 
| | 
| | Well, indeed, somehow forgot about this case. Thanks Maciej!
| | The patch should be dropped.
| | 
| | 	-- Cyrill
| 
| Hmm... But if we have no MP table parsed and no "lapic" option
| passed we should leave execution in pure PIC mode. Or I miss
| something?
| 
| 	-- Cyrill

To be precise -- for discrete apic we set this bit manually.

	-- Cyrill

Re: [RFC -tip] x86,apic -- reduce disable_apic usage

[H. Peter Anvin]

Maciej W. Rozycki wrote:
> 
>  How do you set cpu_has_apic for systems with discrete local APICs?  The 
> CPUID flag is not set in this case.
> 

Well, should it be?  We do set flags when they're appropriate to us, and
if the semantics are such as that is inappropriate we can set a custom bit.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [RFC -tip] x86,apic -- reduce disable_apic usage

[Maciej W. Rozycki]

On Sun, 5 Jul 2009, Cyrill Gorcunov wrote:

> To be precise -- for discrete apic we set this bit manually.

 Ah, OK then.  That didn't use to be the case.

  Maciej

Re: [RFC -tip] x86,apic -- reduce disable_apic usage

[Cyrill Gorcunov]

[Maciej W. Rozycki - Sun, Jul 05, 2009 at 06:47:53PM +0100]
| On Sun, 5 Jul 2009, Cyrill Gorcunov wrote:
| 
| > To be precise -- for discrete apic we set this bit manually.
| 
|  Ah, OK then.  That didn't use to be the case.
| 
|   Maciej
| 

Hmm.. Just reread code. The user are to pass "lapic" option
for this case. At least I didn't find explicit assignment
for cpu_has_apic bit even if MP table has been read successfully.

But I didn't change anything in detect_init_APIC so this case
was here before me :)

	-- Cyrill

Re: [RFC -tip] x86,apic -- reduce disable_apic usage

[Cyrill Gorcunov]

[H. Peter Anvin - Sun, Jul 05, 2009 at 10:30:11AM -0700]
| Maciej W. Rozycki wrote:
| > 
| >  How do you set cpu_has_apic for systems with discrete local APICs?  The 
| > CPUID flag is not set in this case.
| > 
| 
| Well, should it be?  We do set flags when they're appropriate to us, and
| if the semantics are such as that is inappropriate we can set a custom bit.
| 
| 	-hpa
| 

Peter, Maciej but how kernel behaved on older cpu long time ago?

I mean -- should user pass "lapic" cmdline option too be
able to use apic functionality? (i'm asking since I just
don't know how it had been working before). Current
code (if only I'm not _missing_ something) relies on cpu_has_apic
bit. And even if MP table has been parsed and APIC base found,
cpu_has_apic could be not set so detect_init_APIC will fail
if no "lapic" option passed (in case of old cpu without this cpuid
bit produced) as we eventually stay with pic mode.

	-- Cyrill

Re: [RFC -tip] x86,apic -- reduce disable_apic usage

[H. Peter Anvin]

Cyrill Gorcunov wrote:
> 
> Peter, Maciej but how kernel behaved on older cpu long time ago?
> 
> I mean -- should user pass "lapic" cmdline option too be
> able to use apic functionality? (i'm asking since I just
> don't know how it had been working before). Current
> code (if only I'm not _missing_ something) relies on cpu_has_apic
> bit. And even if MP table has been parsed and APIC base found,
> cpu_has_apic could be not set so detect_init_APIC will fail
> if no "lapic" option passed (in case of old cpu without this cpuid
> bit produced) as we eventually stay with pic mode.
> 

Well, systems with discrete APICs were few and far between.  I'm not
sure if there are any such systems still in meaningful existence (kind
of like Voyager.)  That makes it a bit hard to test things, and
certainly means we shouldn't bend over backwards in doing *anything*
that could possibly break other machines.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [RFC -tip] x86,apic -- reduce disable_apic usage

[Cyrill Gorcunov]

[H. Peter Anvin - Sun, Jul 05, 2009 at 12:18:00PM -0700]
| Cyrill Gorcunov wrote:
| > 
| > Peter, Maciej but how kernel behaved on older cpu long time ago?
| > 
| > I mean -- should user pass "lapic" cmdline option too be
| > able to use apic functionality? (i'm asking since I just
| > don't know how it had been working before). Current
| > code (if only I'm not _missing_ something) relies on cpu_has_apic
| > bit. And even if MP table has been parsed and APIC base found,
| > cpu_has_apic could be not set so detect_init_APIC will fail
| > if no "lapic" option passed (in case of old cpu without this cpuid
| > bit produced) as we eventually stay with pic mode.
| > 
| 
| Well, systems with discrete APICs were few and far between.  I'm not
| sure if there are any such systems still in meaningful existence (kind
| of like Voyager.)  That makes it a bit hard to test things, and
| certainly means we shouldn't bend over backwards in doing *anything*
| that could possibly break other machines.
| 
| 	-hpa
| 

Then this patch (eventually) should be dropped. I need to re-check
all this.

	-- Cyrill

Re: [RFC -tip] x86,apic -- reduce disable_apic usage

[Maciej W. Rozycki]

On Sun, 5 Jul 2009, H. Peter Anvin wrote:

> >  How do you set cpu_has_apic for systems with discrete local APICs?  The 
> > CPUID flag is not set in this case.
> > 
> 
> Well, should it be?  We do set flags when they're appropriate to us, and
> if the semantics are such as that is inappropriate we can set a custom bit.

 Hmm, that might simplify things here and there and the less special cases 
in code -- and thus effort needed -- for the discrete APIC, the better.  
I think there is no reason why it couldn't be done -- all the places which 
need version-specific APIC features have to check the LVR register anyway.  
And the availability of the APICBASE MSR has to be validated separately 
too as it comes with P6+ only.

 The only place which could care I believe is code to set X86_FEATURE_11AP 
-- this should obviously be disabled for the discrete APIC as it is now, 
as the chip does not suffer from the erratum and the workaround is costly 
performance-wise.  That piece of code would have to be checked -- I don't 
know what the order of setting of these bits would be and thus if one 
could affect the other.  The dependency would better be well documented 
then too -- my observation is the knowledge about the APIC subsystem among 
people typically only covers a narrow subset of implementations.

  Maciej

Re: [RFC -tip] x86,apic -- reduce disable_apic usage

[Cyrill Gorcunov]

[Maciej W. Rozycki - Wed, Jul 08, 2009 at 12:49:11AM +0100]
| On Sun, 5 Jul 2009, H. Peter Anvin wrote:
| 
| > >  How do you set cpu_has_apic for systems with discrete local APICs?  The 
| > > CPUID flag is not set in this case.
| > > 
| > 
| > Well, should it be?  We do set flags when they're appropriate to us, and
| > if the semantics are such as that is inappropriate we can set a custom bit.
| 
|  Hmm, that might simplify things here and there and the less special cases 
| in code -- and thus effort needed -- for the discrete APIC, the better.  
| I think there is no reason why it couldn't be done -- all the places which 
| need version-specific APIC features have to check the LVR register anyway.  
| And the availability of the APICBASE MSR has to be validated separately 
| too as it comes with P6+ only.
| 
|  The only place which could care I believe is code to set X86_FEATURE_11AP 
| -- this should obviously be disabled for the discrete APIC as it is now, 
| as the chip does not suffer from the erratum and the workaround is costly 
| performance-wise.  That piece of code would have to be checked -- I don't 
| know what the order of setting of these bits would be and thus if one 
| could affect the other.  The dependency would better be well documented 
| then too -- my observation is the knowledge about the APIC subsystem among 
| people typically only covers a narrow subset of implementations.
| 
|   Maciej
| 

Thanks a lot for hints, Maciej! I've had an idea to set this bit
in verify_local_APIC (or something like that) since at this point
if discrete APIC happens -- we already complained in case of APIC
related BIOS problems. So that check-point should be safe. Anyway,
will recheck and put a big comment into patch.

	-- Cyrill