Re: [PATCH 10/14] [VOYAGER] x86: make disabling the apics functional instead of a flag

[Cyrill Gorcunov <gorcunov@gmail.com>]

[James Bottomley - Tue, Apr 14, 2009 at 10:51:36AM -0500]
| This allows the elimination of some ifdef guards in setup.c.
| Additionally probe_32.c doesn't need to run through the apics in
| generic_apic_probe() if they're disabled.
| 
| Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
| ---
|  arch/x86/include/asm/apic.h     |    6 ++++++
|  arch/x86/kernel/apic/apic.c     |    3 +++
|  arch/x86/kernel/apic/probe_32.c |    3 +++
|  arch/x86/kernel/setup.c         |    4 +---
|  4 files changed, 13 insertions(+), 3 deletions(-)
| 
| diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
| index 42f2f83..391c464 100644
| --- a/arch/x86/include/asm/apic.h
| +++ b/arch/x86/include/asm/apic.h
| @@ -246,12 +246,18 @@ static inline int apic_is_clustered_box(void)
|  extern u8 setup_APIC_eilvt_mce(u8 vector, u8 msg_type, u8 mask);
|  extern u8 setup_APIC_eilvt_ibs(u8 vector, u8 msg_type, u8 mask);
|  
| +static inline void disable_APIC(void)
| +{
| +	disable_local_APIC();
| +	disable_apic = 1;
| +}
|  
|  #else /* !CONFIG_X86_LOCAL_APIC */
|  static inline void lapic_shutdown(void) { }
|  #define local_apic_timer_c2_ok		1
|  static inline void init_apic_mappings(void) { }
|  static inline void disable_local_APIC(void) { }
| +static inline void disable_APIC(void) { }
|  
|  #endif /* !CONFIG_X86_LOCAL_APIC */
|  
| diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
| index f9e830e..aa96dbe 100644
| --- a/arch/x86/kernel/apic/apic.c
| +++ b/arch/x86/kernel/apic/apic.c
| @@ -1539,6 +1539,9 @@ void __init early_init_lapic_mapping(void)
|   */
|  void __init init_apic_mappings(void)
|  {
| +	if (disable_apic)
| +		return;
| +

No, we shouldn't do that without additional
code review (otherwise we will loose mapping for fake apic page).
And I suspect we get NULL deref on reboot procedure
if kernel was compiled _with_ SMP support but APIC disabled
by kernel option. hmm, can't find this reference in LKML.
I was telling Ingo about my suspicious on smp operations
when APIC is disabled by option and we're safe _only_ 'cause
we have fake mapping page reserved. For example call of
smp_send_stop() in kernel/panic.c if kernel was compiled _with_ SMP
support and then APIC disabled by kernel option.

|  	if (x2apic) {
|  		boot_cpu_physical_apicid = read_apic_id();
|  		return;
| diff --git a/arch/x86/kernel/apic/probe_32.c b/arch/x86/kernel/apic/probe_32.c
| index 01eda2a..049d7a0 100644
| --- a/arch/x86/kernel/apic/probe_32.c
| +++ b/arch/x86/kernel/apic/probe_32.c
| @@ -226,6 +226,9 @@ void __init generic_bigsmp_probe(void)
|  
|  void __init generic_apic_probe(void)
|  {
| +	if (disable_apic)
| +		return;
| +

Look suspicious too, sorry. If kernel compiled with local apic
set we rely on apic read ops regardless if apic facility
has been disabled by option or not.

I could be missing something /which has quite a high probability magnitude/.
Yinghai help needed :) CC'ed

|  	if (!cmdline_apic) {
|  		int i;
|  		for (i = 0; apic_probe[i]; i++) {
| diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
| index bee0914..13779e2 100644
| --- a/arch/x86/kernel/setup.c
| +++ b/arch/x86/kernel/setup.c
| @@ -770,9 +770,7 @@ void __init setup_arch(char **cmdline_p)
|  	reserve_early_setup_data();
|  
|  	if (acpi_mps_check()) {
| -#ifdef CONFIG_X86_LOCAL_APIC
| -		disable_apic = 1;
| -#endif
| +		disable_APIC();
|  		setup_clear_cpu_cap(X86_FEATURE_APIC);
|  	}
|  
| -- 
| 1.6.2.1
| 

        Cyrill