Skip to contentSkip to conversation

x86/acpi: Prevent LAPIC id 0xff from being accounted

tip:x86/urgent Yinghai Lu 2 participants 4 messages Oct 8, 2016 1 message not in the archive

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Yinghai Lu

author
#1 lore raw
Quoted 42 lines · tip-bot for Thomas Gleixner
Commit-ID:  8237bded3959c6d038798b905485d3ba94b8ea10
Gitweb:     http://git.kernel.org/tip/8237bded3959c6d038798b905485d3ba94b8ea10
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Fri, 7 Oct 2016 14:02:12 +0200
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Fri, 7 Oct 2016 15:22:15 +0200

x86/acpi: Prevent LAPIC id 0xff from being accounted

Yinghai reported that the recent changes to make the cpuid - nodeid relationship permanent causes a cpuid ordering regression on a system which has 2apic enabled..

The reason is that the ACPI local APIC parser has no sanity check for apicid 0xff, which is an invalid id. So a CPU id for this invalid local APIC id is allocated and therefor breaks the cpuid ordering.

Add a sanity check to acpi_parse_lapic() which ignores the invalid id.

Fixes 8f54969dc8d6 ("x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping")
Reported-by Yinghai Lu <[email protected]>
Signed-off-by Thomas Gleixner <[email protected]>
Cc Gu Zheng <[email protected]>,
Cc Tang Chen <[email protected]>
Cc [email protected],
Cc [email protected]
Cc Tony Luck <[email protected]>
Cc Rafael J. Wysocki <[email protected]>
Cc Len Brown <[email protected]>
Cc Lv Zheng <[email protected]>,
Cc [email protected]
Cc [email protected]
Link https://lkml.kernel.org/r/CAE9FiQVQx6FRXT-RdR7Crz4dg5LeUWHcUSy1KacjR+JgU_vGJg@mail.gmail.com open here
---
 arch/x86/kernel/acpi/boot.c | 3 +++
 1 file changed, 3 insertions(+)
arch/x86/kernel/acpi/boot.c +0 -0
index 32a7d70..6d35baf 100644

arch/x86/kernel/acpi/boot.c acpi_parse_lapic(struct acpi_subtable_header * header, const unsigned long end) · line 241

Quoted 9 lines
arch/x86/kernel/acpi/boot.c +3 -0
@@ -233,6 +233,9 @@ acpi_parse_lapic(struct acpi_subtable_header * header, const unsigned long end)
 
        acpi_table_print_madt_entry(header);
 
+       if (processor->id >= 0xff)
+               return -EINVAL;
+
        /*
         * We need to register disabled CPU as well to permit
         * counting disabled CPUs. This allows us to size

some thing is wrong:

[   71.787437] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled)
[   71.799681] ACPI: Error parsing LAPIC/X2APIC entries
[   71.809934] ACPI: Invalid BIOS MADT, disabling ACPI

looks like should change
return -EINVAL ;
==>
return 0;

author
#2 lore raw
Quoted 19 lines · Yinghai Lu

On Fri, Oct 7, 2016 at 6:28 AM, tip-bot for Thomas Gleixner

Quoted 5 lines
arch/x86/kernel/acpi/boot.c +0 -0
index 32a7d70..6d35baf 100644

arch/x86/kernel/acpi/boot.c acpi_parse_lapic(struct acpi_subtable_header * header, const unsigned long end) · line 241

Quoted 9 lines
arch/x86/kernel/acpi/boot.c +3 -0
@@ -233,6 +233,9 @@ acpi_parse_lapic(struct acpi_subtable_header * header, const unsigned long end)
 
        acpi_table_print_madt_entry(header);
 
+       if (processor->id >= 0xff)
+               return -EINVAL;
+
        /*
         * We need to register disabled CPU as well to permit
         * counting disabled CPUs. This allows us to size

some thing is wrong:

Quoted 8 lines
[   71.787437] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled)
[   71.799681] ACPI: Error parsing LAPIC/X2APIC entries
[   71.809934] ACPI: Invalid BIOS MADT, disabling ACPI

looks like should change
return -EINVAL ;
==>
return 0;

also processor->id is u8,

so the patch should be:

Quoted 3 lines
Quoted 3 lines
arch/x86/kernel/acpi/boot.c +3 -0
+       if (processor->id == 0xff)
+               return -EINVAL;
+
author
#3 lore raw
Quoted 24 lines · Yinghai Lu
Quoted 23 lines · Yinghai Lu

On Fri, Oct 7, 2016 at 6:28 AM, tip-bot for Thomas Gleixner

Quoted 5 lines
arch/x86/kernel/acpi/boot.c +0 -0
index 32a7d70..6d35baf 100644

arch/x86/kernel/acpi/boot.c acpi_parse_lapic(struct acpi_subtable_header * header, const unsigned long end) · line 241

Quoted 9 lines
arch/x86/kernel/acpi/boot.c +3 -0
@@ -233,6 +233,9 @@ acpi_parse_lapic(struct acpi_subtable_header * header, const unsigned long end)
 
        acpi_table_print_madt_entry(header);
 
+       if (processor->id >= 0xff)
+               return -EINVAL;
+
        /*
         * We need to register disabled CPU as well to permit
         * counting disabled CPUs. This allows us to size

some thing is wrong:

[   71.787437] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled)
[   71.799681] ACPI: Error parsing LAPIC/X2APIC entries
[   71.809934] ACPI: Invalid BIOS MADT, disabling ACPI
Quoted 8 lines
Quoted 4 lines

looks like should change
return -EINVAL ;
==>
return 0;

also processor->id is u8,

so the patch should be:

+ if (processor->id == 0xff)
+ return 0;
+

#4 lore raw
Quoted 21 lines · Yinghai Lu
Quoted 20 lines · Yinghai Lu
Quoted 19 lines · Yinghai Lu

On Fri, Oct 7, 2016 at 6:28 AM, tip-bot for Thomas Gleixner

Quoted 5 lines
arch/x86/kernel/acpi/boot.c +0 -0
index 32a7d70..6d35baf 100644

arch/x86/kernel/acpi/boot.c acpi_parse_lapic(struct acpi_subtable_header * header, const unsigned long end) · line 241

Quoted 9 lines
arch/x86/kernel/acpi/boot.c +3 -0
@@ -233,6 +233,9 @@ acpi_parse_lapic(struct acpi_subtable_header * header, const unsigned long end)
 
        acpi_table_print_madt_entry(header);
 
+       if (processor->id >= 0xff)
+               return -EINVAL;
+
        /*
         * We need to register disabled CPU as well to permit
         * counting disabled CPUs. This allows us to size

some thing is wrong:

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
[   71.787437] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled)
[   71.799681] ACPI: Error parsing LAPIC/X2APIC entries
[   71.809934] ACPI: Invalid BIOS MADT, disabling ACPI

looks like should change
return -EINVAL ;
==>
return 0;

Indeed.

Review

Quoted 8 lines
Quoted 4 lines

also processor->id is u8,

so the patch should be:

(quoted diff) +3 -0
+       if (processor->id == 0xff)
+               return 0;
+

Right. Amended the patch and pushed it out.

Thanks for pointing it out,

tglx