linux-kernel - Re: [PATCH 2/4] [APIC] Allow disabling of UP APIC/IO-APIC by default, with command line option to turn it on.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20061201083900.GA26703@elte.hu>
Date:	Fri, 1 Dec 2006 09:39:00 +0100
From:	Ingo Molnar <mingo@...e.hu>
To:	Ben Collins <bcollins@...ntu.com>
Cc:	linux-kernel@...r.kernel.org, torvalds@...l.org,
	Andrew Morton <akpm@...l.org>
Subject: Re: [PATCH 2/4] [APIC] Allow disabling of UP APIC/IO-APIC by default, with command line option to turn it on.


* Ben Collins <bcollins@...ntu.com> wrote:

> +config X86_UP_APIC_DEFAULT_OFF
> +	bool "APIC support on uniprocessors defaults to off"
> +	depends on X86_UP_APIC
> +	default n

'n' is the default

>  /*
>   * Knob to control our willingness to enable the local APIC.
> + * -2=default-disable, -1=force-disable, 1=force-enable, 0=automatic
>   */
> -static int enable_local_apic __initdata = 0; /* -1=force-disable, +1=force-enable */
> +static int enable_local_apic __initdata = (X86_APIC_DEFAULT_OFF ? -2 : 0);

i guess this begs for enums?

>  		if (enable_local_apic <= 0) {
> -			printk("Local APIC disabled by BIOS -- "
> +			printk("Local APIC disabled by BIOS (or by default) -- "
>  			       "you can enable it with \"lapic\"\n");

that message should be more intelligent, depending on whether the value 
is 0, -1 or -2.

> +	/* If local apic is off due to config_x86_apic_off option, jump
> +	 * out here. */

nitpick: proper comment style for new code is:

	/*
	 * If local APIC is off due to config_x86_apic_off option, jump
	 * out here.
	 */

> +	if (enable_local_apic < -1) {
> +		printk(KERN_INFO "Local APIC disabled by default -- "
> +		       "use 'lapic' to enable it.\n");
> +		return -1;
> +	}

this should be enable_local_apic == -2. (and should use the enum)

> -int skip_ioapic_setup;
> +int skip_ioapic_setup = X86_APIC_DEFAULT_OFF;

nitpick: should be X86_IOAPIC_DEFAULT_VALUE - if the config option is 
not set then this 'OFF' value will mean 'on' ...

> +static int __init parse_apic(char *arg)
> +{
> +	/* enable IO-APIC */
> +	enable_ioapic_setup();
> +	return 0;
> +}
> +early_param("apic", parse_apic);

that should be "ioapic", not "apic". The CPU has a piece of silicon 
called the "local APIC" - enabled via the 'lapic' option, and disabled 
via noapic. What the option above wants to enable is the IO-APIC in the 
chipset (a different piece of silicon) and the interrupt routing 
capabilities attached to it. That piece is what is causing the installer 
problems.

looks good in principle, but needs these cleanups.

	Ingo
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/