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

[Dou Liyang]

Hi Yinghai,

At 10/06/2016 12:53 PM, Yinghai Lu wrote:
On Wed, Oct 5, 2016 at 7:04 AM, Thomas Gleixner <t...@linutronix.de> wrote:
@@ -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?
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



No, That does not fix the issue.

the system have x2apic pre_enabled from BIOS, so at the time
apic is set to &apic_x2apic_cluster.

early_acpi_boot_init ==> early_acpi_process_madt ==> acpi_parse_madt
==> default_acpi_madt_oem_check

default_acpi_madt_oem_check
      ==> apic_x2apic_cluster/x2apic_acpi_madt_oem_check ==> x2apic_enabled
      ==> apic = &apic_x2apic_cluster

and
static int x2apic_apic_id_valid(int apicid)
{
        return 1;
}

To make your change work, may need to update x2apic_apic_id_valid to

static int x2apic_apic_id_valid(int apicid)
{
        if (apicid == 0xff || apicid == -1)
            return 0;


I seem to remember that in x2APIC Spec the x2APIC ID may be at 255 or
greater.
If we do that judgment, it may be affect x2APIC's work in some other places.

I saw the MADT, the main reason may be that we define 0xff to acpi_id
in LAPIC mode.
As you said, it was like:
[   42.107902] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled)
[   42.120125] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled)
[   42.132361] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled)
...

How about doing the acpi_id check when we parse it in
acpi_parse_lapic().

8<----------------

--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -233,6 +233,11 @@ acpi_parse_lapic(struct acpi_subtable_header * 
header, const unsigned long end)

        acpi_table_print_madt_entry(header);

+       if (processor->id >= 255) {
+               ++disabled_cpus;
+               return -EINVAL;
+       }
+
        /*
         * We need to register disabled CPU as well to permit
         * counting disabled CPUs. This allows us to size


Thanks

Dou

        return 1;
}


Thanks

Yinghai