Re: [PATCH] Handle disabled local apic better

[Len Brown <len.brown-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

On Tue, 2004-03-23 at 15:35, Andi Kleen wrote:
> When a kernel is compiled with local and io apic code, but the local APIC
> is disabled before ACPI starts ACPI gets confused.  The situation
> can occur in a mainline kernel too, e.g. when a dmi_scan entry disables
> the local apic early.
> 
> The problem is that the ACPI code will do all the setup assuming there
> is a IO-APIC because the IO-APIC is not disabled, but the machine really runs in 
> PIC mode. This usually leads to lost network (devices get an unreachable interrupt) 
> or IDE hangs or other problems.
> 
> This patch adds checks for local apic enabled to all paths that need it
> (basically everybody who has a #ifdef CONFIG_X86_LOCAL_APIC needs this
> check instead)

oof, and there are lots of CONFIG_X86_LOCAL_APIC uses apparently
un-guarded at run-time...

> 
> It just handles the case there the up apic is disabled before ACPI boot,
> it can actually be disabled later too (e.g. when the APIC initialisation
> fails). This is probably still broken.
> 
> Also removes an bogus error message - this path can trigger just with
> software disabled APIC.
> 
> -Andi
> 
> diff -u linux/arch/i386/kernel/acpi/boot.c-o linux/arch/i386/kernel/acpi/boot.c
> --- linux/arch/i386/kernel/acpi/boot.c-o	2004-03-23 17:32:22.000000000 +0100
> +++ linux/arch/i386/kernel/acpi/boot.c	2004-03-23 17:34:23.000000000 +0100
> @@ -685,6 +685,13 @@
>  #ifdef CONFIG_X86_LOCAL_APIC
>  	int count, error;
>  
> +	/* it's still wrong when the apic is disabled later */
> +	extern int enable_local_apic;
> +	if (enable_local_apic < 0) { 
> +		printk(KERN_INFO "ACPI: local apic disabled\n");
> +		return;
> +	}
> +
>  	count = acpi_table_parse(ACPI_APIC, acpi_parse_madt);
>  	if (count == 1) {

Looks like i386 lacks the parse_cmdline_early() check for "nolapic" to
make this test effective.

(why the heck is the parse_args() so late, anyway?)

Looks like x86_64 has it, but then does it again at __setup() time --
which should probably be removed; along with the redundant variable
"disable_apic".

>  
> diff -u linux/arch/i386/kernel/apic.c-o linux/arch/i386/kernel/apic.c
> --- linux/arch/i386/kernel/apic.c-o	2004-03-23 17:32:22.000000000 +0100
> +++ linux/arch/i386/kernel/apic.c	2004-03-23 18:01:21.000000000 +0100
> @@ -41,6 +41,8 @@
>  
>  #include "io_ports.h"
>  
> +extern int enable_local_apic;
> +

how about declare in asm/apic.h instead?

>  static void apic_pm_activate(void);
>  
>  void __init apic_intr_init(void)
> @@ -190,6 +192,9 @@
>  {
>  	unsigned long value;
>  
> +	if (enable_local_apic < 0) 
> +		return;
> +
>  	clear_local_APIC();
>  
>  	/*
> @@ -1172,8 +1193,6 @@
>  	 * Complain if the BIOS pretends there is one.
>  	 */
>  	if (!cpu_has_apic && APIC_INTEGRATED(apic_version[boot_cpu_physical_apicid])) {
> -		printk(KERN_ERR "BIOS bug, local APIC #%d not detected!...\n",
> -			boot_cpu_physical_apicid);
>  		return -1;
>  	}
>  
> diff -u linux/arch/i386/kernel/setup.c-o linux/arch/i386/kernel/setup.c
> --- linux/arch/i386/kernel/setup.c-o	2004-03-23 17:32:00.000000000 +0100
> +++ linux/arch/i386/kernel/setup.c	2004-03-23 18:01:14.000000000 +0100
> @@ -50,6 +50,10 @@
>  #include "setup_arch_pre.h"
>  #include "mach_resources.h"
>  
> +#ifdef CONFIG_X86_LOCAL_APIC
> +extern int enable_local_apic;
> +#endif
> +

asm/apic.h

>  /* This value is set up by the early boot code to point to the value
>     immediately after the boot time page tables.  It contains a *physical*
>     address, and must not be in the .bss segment! */
> @@ -917,6 +925,7 @@
>  	acpi_reserve_bootmem();
>  #endif
>  #ifdef CONFIG_X86_FIND_SMP_CONFIG
> +	if (enable_local_apic >= 0) 
>  	/*
>  	 * Find and reserve possible boot-time SMP configuration:
>  	 */

Here we're disabling MPS with "nolapic" -- something that "maxcpus=0"
and "nosmp" never effectively did before...

> @@ -1232,7 +1241,7 @@
>  	acpi_boot_init();
>  
>  #ifdef CONFIG_X86_LOCAL_APIC
> -	if (smp_found_config)
> +	if (smp_found_config && enable_local_apic >= 0)
>  		get_smp_config();

i guess this is for the case where DMI disables the lapic, because
"nolapic" would have prevented smp_found_config from getting set in the
first place.  maybe the DMI routine should simply clear
smp_found_config?

>  #endif
>  
> diff -u linux/arch/x86_64/kernel/apic.c-o linux/arch/x86_64/kernel/apic.c
> --- linux/arch/x86_64/kernel/apic.c-o	2004-03-23 17:32:01.000000000 +0100
> +++ linux/arch/x86_64/kernel/apic.c	2004-03-23 18:41:58.000000000 +0100
> @@ -970,6 +970,7 @@
>  }
>  
>  int disable_apic; 
> +int enable_local_apic = 1;
>  
asm/apic.h;-)

>  /*
>   * This initializes the IO-APIC and APIC hardware if this is
> @@ -1009,12 +1010,14 @@
>  
>  static __init int setup_disableapic(char *str) 
>  { 
> +	enable_local_apic = -1;
>  	disable_apic = 1;
>  	return 0;
>  } 
>  
>  static __init int setup_nolapic(char *str) 
>  { 
> +	enable_local_apic = -1;
>  	disable_apic = 1;
>  	return 0;
>  } 

these routines should be deleted, since they duplicate the earlier
parse_cmdline_early().  the ambiguous "disable_lapic" should go also,
since "enable_local_apic" is on the scene, yes?

thanks,
-Len




-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click