Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping

[Thomas Gleixner <tglx@linutronix.de>]

On Mon, 3 Oct 2016, Yinghai Lu wrote:
> init_cpu_node become:
> [   55.477160] init_cpu_to_node:
> [   55.483280] cpu 0 -> apicid 0x0 -> node 0
> [   55.491558] cpu 1 -> apicid 0xff -> node 1
> [   55.500017] cpu 2 -> apicid 0x2 -> node 0
> [   55.508296] cpu 3 -> apicid 0x4 -> node 0
> [   55.516575] cpu 4 -> apicid 0x6 -> node 0
> ...
> 
> looks like problem is
> 
> acpi_parse_lapic==>acpi_register_lapic==>__generic_processor_info==>allocate_logical_cpuid
> 
> it will take lapic_id[0xff] take cpu index 1.
> 
> Then will have not /dev/cpu/1/msr, that will make the MLC not happy.

That's pretty irrelevant whether that MLC thing - whatever it is - is
unhappy or not. If it cannot deal with something missing, it's surely not
the kernels problem. Unplug the cpu and the file will be missing as well.

> -static int acpi_register_lapic(int id, u32 acpiid, u8 enabled)
> +static int acpi_register_lapic(int id, u32 acpiid, u8 enabled, int disabled_id)
>  {
>      unsigned int ver = 0;
>      int cpu;

This is whitespace damaged. The patch does not apply. Can you finaly after
doing 10 years of kernel development start being more careful?

> @@ -176,6 +177,11 @@ static int acpi_register_lapic(int id, u
>          return -EINVAL;
>      }
> 
> +    if (!enabled && (id == disabled_id)) {
> +        ++disabled_cpus;
> +        return -EINVAL;
> +    }

Why would you need that disabled_id thing at all? The proper fix is to let
the apic driver detect the issue and this boils down to a 5 lines
change. Does the patch below fix the issue for you?

Thanks,

	tglx

8<----------------
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -2076,6 +2076,11 @@ int __generic_processor_info(int apicid,
 	bool boot_cpu_detected = physid_isset(boot_cpu_physical_apicid,
 				phys_cpu_present_map);
 
+	if (!apic->apic_id_valid(apicid)) {
+		disabled_cpus++;
+		return -EINVAL;
+	}
+
 	/*
 	 * boot_cpu_physical_apicid is designed to have the apicid
 	 * returned by read_apic_id(), i.e, the apicid of the