Dont record local apic ids when they are disabled in MADT

Dont record local apic ids when they are disabled in MADT

[Ashok Raj]

Hi Andrew,

We had added additional_cpus=xx for x86_64, but apparently there were some
BIOSs that had duplicate apic ids when they were reported disabled.

It seems fair not to record them, this was causing some bad behaviour due to
the duplicate apic id. More details in the bugzilla recorded in the log.

Please help with inclusion.


-- 
Cheers,
Ashok Raj
- Open Source Technology Center


Some broken BIOS's had processors disabled, but 
same apic id as a valid processor. This causes
acpi_processor_start() to think this disabled 
cpu is ok, and croak. So we dont record bad
apicid's anymore.

http://bugzilla.kernel.org/show_bug.cgi?id=5930

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
------------------------------------------------------
 arch/i386/kernel/acpi/boot.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

Index: linux-2.6.16-rc1-mm2/arch/i386/kernel/acpi/boot.c
===================================================================
--- linux-2.6.16-rc1-mm2.orig/arch/i386/kernel/acpi/boot.c
+++ linux-2.6.16-rc1-mm2/arch/i386/kernel/acpi/boot.c
@@ -248,10 +248,17 @@ acpi_parse_lapic(acpi_table_entry_header
 
 	acpi_table_print_madt_entry(header);
 
-	/* Register even disabled CPUs for cpu hotplug */
-
-	x86_acpiid_to_apicid[processor->acpi_id] = processor->id;
+	/* Record local apic id only when enabled */
+	if (processor->flags.enabled)
+		x86_acpiid_to_apicid[processor->acpi_id] = processor->id;
 
+	/*
+	 * We need to register disabled CPU as well to permit
+	 * counting disabled CPUs. This allows us to size
+	 * cpus_possible_map more accurately, to permit
+	 * to not preallocating memory for all NR_CPUS
+	 * when we use CPU hotplug.
+	 */
 	mp_register_lapic(processor->id,	/* APIC ID */
 			  processor->flags.enabled);	/* Enabled? */
 

Re: Dont record local apic ids when they are disabled in MADT

[Andi Kleen]

On Thursday 26 January 2006 14:48, Ashok Raj wrote:
> Hi Andrew,
> 
> We had added additional_cpus=xx for x86_64, but apparently there were some
> BIOSs that had duplicate apic ids when they were reported disabled.
> 
> It seems fair not to record them, this was causing some bad behaviour due to
> the duplicate apic id. More details in the bugzilla recorded in the log.

This means CPU hotplug will require additional non existing code again - or who
will set up the APIC IDs etc. for the new CPUs then?

An alternative might be to reject any entries that have the APIC ID of
an existing entry. That would require that the entries are sorted with disabled
after enabled
(in the worst case Linux could sort again)

-Andi

Re: Dont record local apic ids when they are disabled in MADT

[Ashok Raj]

On Thu, Jan 26, 2006 at 02:55:11PM +0100, Andi Kleen wrote:
> On Thursday 26 January 2006 14:48, Ashok Raj wrote:
> > Hi Andrew,
> > 
> > We had added additional_cpus=xx for x86_64, but apparently there were some
> > BIOSs that had duplicate apic ids when they were reported disabled.
> > 
> > It seems fair not to record them, this was causing some bad behaviour due to
> > the duplicate apic id. More details in the bugzilla recorded in the log.
> 
> This means CPU hotplug will require additional non existing code again - or who
> will set up the APIC IDs etc. for the new CPUs then?


The ACPI hotplug code already would refresh this apic id when the notify
for CPU hotplug is processed. 

Possibly we could add additional check to safegaurd buggy bios reporting 
bad apicid, as a safegaurd.  

The ACPI code when it processes a physical removal would remove this entry 
and replace with invalid id, and populate it when we process a physical 
hot-add.

(although i would say we tested this only on ia64 so far, the code is 
generic to x86_64 as well, but i havent gone around testing physical hotplug
via emulation patches we have internally)

> 
> An alternative might be to reject any entries that have the APIC ID of
> an existing entry. That would require that the entries are sorted with disabled
> after enabled
> (in the worst case Linux could sort again)
> 
> -Andi

-- 
Cheers,
Ashok Raj
- Open Source Technology Center

Re: Dont record local apic ids when they are disabled in MADT

[Andi Kleen]

On Thursday 26 January 2006 15:10, Ashok Raj wrote:
> On Thu, Jan 26, 2006 at 02:55:11PM +0100, Andi Kleen wrote:
> > On Thursday 26 January 2006 14:48, Ashok Raj wrote:
> > > Hi Andrew,
> > > 
> > > We had added additional_cpus=xx for x86_64, but apparently there were some
> > > BIOSs that had duplicate apic ids when they were reported disabled.
> > > 
> > > It seems fair not to record them, this was causing some bad behaviour due to
> > > the duplicate apic id. More details in the bugzilla recorded in the log.
> > 
> > This means CPU hotplug will require additional non existing code again - or who
> > will set up the APIC IDs etc. for the new CPUs then?
> 
> 
> The ACPI hotplug code already would refresh this apic id when the notify
> for CPU hotplug is processed. 

How? All the code who could do this is __init.

> (although i would say we tested this only on ia64 so far, the code is 
> generic to x86_64 as well, but i havent gone around testing physical hotplug
> via emulation patches we have internally)

I doubt it will work on x86-64.

And you're breaking the CPU hotplug spec in Documentation/x86-64/cpu-hotplug-spec.
I think the BIOS bugs need to be workarounded without breaking that.


-Andi

Re: Dont record local apic ids when they are disabled in MADT

[Ashok Raj]

On Thu, Jan 26, 2006 at 03:34:54PM +0100, Andi Kleen wrote:
> 
> How? All the code who could do this is __init.

Where? all code that handles cpu hotplug is located in 
drivers/acpi/processor_core.c under #ifdef CONFIG_ACPI_HOTPLUG_CPU handles 
this.

Just to clarify when we say hotplug cpu support in kernel, today it means
logical cpu hotplug. I hope when physical hotplug is supported that 
platform will have supporting ACPI based or some equivalent implementation.

> 
> > (although i would say we tested this only on ia64 so far, the code is 
> > generic to x86_64 as well, but i havent gone around testing physical hotplug
> > via emulation patches we have internally)
> 
> I doubt it will work on x86-64.

In x86_64, we need to implement one mapping function acpi_map_lsapic()
defined in arch/i386/kernel/acpi/boot.c. Today this is only a stub
function. Its fully implemented for ia64. (As i mentioned this is 
untested yet, and needs some more code and test)

acpi_processor_hotadd_init() in processor_core.c also does arch_register_cpu()
to create appropriate sysfs entries in /sys/devices/system/cpu/cpuX

When a platform supports hotplug, the BIOS should support appropriate ACPI
methods, or some equilvalent to do similar things.
> 
> And you're breaking the CPU hotplug spec in Documentation/x86-64/cpu-hotplug-spec.
> I think the BIOS bugs need to be workarounded without breaking that.

ACPI spec states that if an entry is marked disabled, then dont use it at all.
We could use that to count the number of disabled CPUs in the system, which is 
an acceptable deviation from the real intent, but i dont think we can ask the 
BIOS to also have valid apic ids in them (especially platforms already 
released) 

Forcing even existing BIOSs to change wouldnt be nice.

Alternately you can add the counting of disabled CPUs logic only when 
CONFIG_ACPI_HOTPLUG_CPU is enabled, so it doesnt hurt existing platforms.

-- 
Cheers,
Ashok Raj
- Open Source Technology Center