x86/acpi: acpi_is_processor_usable() dropping possible cpus
cover x86/acpi: acpi_is_processor_usable() dropping possible cpus
I've been working on a patch series for cpu hotplug and kdump and noticed recently that utilizing a QEMU guest to test cpu hotplug was causing the following error messages:
APIC: NR_CPUS/possible_cpus limit of 30 reached. Processor 30/0x.
ACPI: Unable to map lapic to logical cpu number
acpi LNXCPU:1e: Enumeration failure
where prior cpu hotplug would typically result in messages similar to:
CPU30 has been hot-added
smpboot: Booting Node 0 Processor 30 APIC 0x1e Will online and init hotplugged CPU: 30
The QEMU guest incantation was:
qemu-system-x86_64 -smp 30,maxcpus=32 ...
A simple guest with 30 cpus and possiblly up to 32, meaning two hot pluggable.
An investigation lead to finding the following relevant changes:
commit aa06e20f1be6 ("x86/acpi: Don't add CPUs that are not online capable") commit e2869bd7af60 ("x86/acpi/boot: Do not register processors that cannot be onlined for x2APIC")
The first patch codes the fact that ACPI 6.3 now features an Online Capable bit in MADT.revision >= 5. This Online Capable bit explicitly indicates that the cpu can be hotplugged.
The second patch refactors the first a bit in order to apply the same logic for both lapic and x2apic. However, there is a subtle change in the logic to this patch that breaks for MADT.revision prior to 5.
As it turns out, QEMU reports revision 1 in the MADT table for x86. (Technically QEMU is at revision 4, and I'll be addressing that with the QEMU folks soon.)
In looking at these patches and understanding the logic, I can sum it up by stating that prior to MADT revision 5, the presence of a lapic/x2apic structure _implicitly_ was used to determine that a cpu might be hotpluggable, and it would count towards possible cpus. With MADT.revision >= 5, that implicit assumption is now replaced with an explicit indication.
The manner in which the latest patch is coded, it essentially requires the Online Capable bit introduced at MADT.revision >= 5 be set for present-but-offline cpus, else it excludes the cpu from the possible cpus. Thus for MADT.revision < 5, possible cpus are being dropped.
This patch aims to restore the behavior for MADT.revision prior to 5 which facilitates again cpu hotplug for QEMU guests.
Regards,
eric
---
Eric DeVolder (1):
x86/acpi: acpi_is_processor_usable() dropping possible cpus
arch/x86/kernel/acpi/boot.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
Signature · 1 line
2.31.1
PATCH 1/1 x86/acpi: acpi_is_processor_usable() dropping possible cpus
The logic in acpi_is_processor_usable() requires the Online Capable bit be set for hotpluggable cpus. The Online Capable bit is introduced in ACPI 6.3 and MADT.revision 5.
However, as currently coded, for MADT.revision < 5, acpi_is_processor_usable() no longer allows for possible hot pluggable cpus, which is a regressive change in behavior.
This patch restores the behavior where for MADT.revision < 5, the presence of the lapic/x2apic structure implies a possible hotpluggable cpu.
--- arch/x86/kernel/acpi/boot.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
arch/x86/kernel/acpi/boot.c +7 -1
@@ -193,7 +193,13 @@ static bool __init acpi_is_processor_usable(u32 lapic_flags)if (lapic_flags &ACPI_MADT_ENABLED )return true; -if (acpi_support_online_capable && (lapic_flags &ACPI_MADT_ONLINE_CAPABLE )) +/* +* Prior to MADT.revision 5, the presence of the Local x2/APIC +* structure _implicitly_ noted a possible hotpluggable cpu. +* Starting with MADT.revision 5, the Online Capable bit +* _explicitly_ indicates a hotpluggable cpu. +*/ +if (!acpi_support_online_capable || (lapic_flags &ACPI_MADT_ONLINE_CAPABLE ))return true;return false;
Signature · 1 line
2.31.1
Borislav Petkov
maintainerQuoted 3 lines · Eric DeVolder
The logic in
acpi_is_processor_usable()requires the Online Capable bit be set for hotpluggable cpus. The Online Capable bit is introduced in ACPI 6.3 and MADT.revision 5.
I can't find where in the spec it says that MADT.revision 5 means that bit is present?
I'm looking at:
aa06e20f1be6 ("x86/ACPI: Don't add CPUs that are not online capable")
Mario?
I see in the 6.3 spec it says:
"1948 Adds a “Hot-plug Capable” flag to the Local APIC and x2APIC structures in MADT"
and the MADT.revision is 5 and in the 6.2 spec the MADT revision is "45"
- 4.5 maybe?
But I don't see the connection between MADT.revision 5 and the presence of the online capable bit.
Anyone got a better quote?
Quoted 5 lines
However, as currently coded, for MADT.revision < 5,
acpi_is_processor_usable()no longer allows for possible hot pluggable cpus, which is a regressive change in behavior.This patch restores the behavior where for MADT.revision < 5, the
Avoid having "This patch" or "This commit" in the commit message. It is tautologically useless.
Also, do
$ git grep 'This patch' Documentation/process
for more details.
Quoted 16 lines
presence of the lapic/x2apic structure implies a possible hotpluggable cpu.
Fixes e2869bd7af60 ("x86/acpi/boot: Do not register processors that cannot be onlined for x2APIC")Suggested-by Miguel Luis <[email protected]>Suggested-by Boris Ostrovsky <[email protected]>Signed-off-by Eric DeVolder <[email protected]>--- arch/x86/kernel/acpi/boot.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)arch/x86/kernel/acpi/boot.c +0
-0
arch/x86/kernel/acpi/boot.c static bool __init acpi_is_processor_usable(u32 lapic_flags) · line 201
Quoted 10 lines
arch/x86/kernel/acpi/boot.c +6
-1@@ -193,7 +193,13 @@ static bool __init acpi_is_processor_usable(u32 lapic_flags)if (lapic_flags &ACPI_MADT_ENABLED )return true; -if (acpi_support_online_capable && (lapic_flags &ACPI_MADT_ONLINE_CAPABLE )) +/* +* Prior to MADT.revision 5, the presence of the Local x2/APIC +* structure _implicitly_ noted a possible hotpluggable cpu. +* Starting with MADT.revision 5, the Online Capable bit +* _explicitly_ indicates a hotpluggable cpu. +*/
In all your text
s/cpu/CPU/g
arch/x86/kernel/acpi/boot.c static bool __init acpi_is_processor_usable(u32 lapic_flags) · line 205
Quoted 5 lines
arch/x86/kernel/acpi/boot.c +1
-0+if (!acpi_support_online_capable || (lapic_flags &ACPI_MADT_ONLINE_CAPABLE ))return true;return false;
Otherwise, the change makes sense to me.
Thx.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteQuoted 18 lines · Borislav Petkov
Quoted 3 lines · Eric DeVolder
The logic in
acpi_is_processor_usable()requires the Online Capable bit be set for hotpluggable cpus. The Online Capable bit is introduced in ACPI 6.3 and MADT.revision 5.I can't find where in the spec it says that MADT.revision 5 means that bit is present?
I'm looking at:
aa06e20f1be6("x86/ACPI: Don't add CPUs that are not online capable")Mario?
I see in the 6.3 spec it says:
"1948 Adds a “Hot-plug Capable” flag to the Local APIC and x2APIC structures in MADT"
Quoted 7 lines
and the MADT.revision is 5 and in the 6.2 spec the MADT revision is "45"
- 4.5 maybe?But I don't see the connection between MADT.revision 5 and the presence of the online capable bit.
Anyone got a better quote?
Boris,
https://ueif.org/sites/default/files/resources/ACPI_6_3_May16.pdf
Section 5.2.12 MADT. Table 5-43 is the MADT Revision is numbered 5. However, ACPI 6.x specs got a little "sloppy" with Revision, as this is what I uncovered:
ACPI MADT Changes
6.0 3 Section 5.2.12
6.0a 4 Section 5.2.12
Adds ARM GIC structure types 0xB-0xF
6.2a 45 Section 5.2.12 <--- yep it says version 45!
6.2b 5 Section 5.2.12 GIC ITS last Reserved offset changed to 16 from 20 (typo)
6.3 5 Section 5.2.12
Adds Local APIC Flags Online Capable!
Adds GICC SPE Overflow Interrupt field
6.4 5 Section 5.2.12
Adds Multiprocessor Wakeup Structure type 0x10
6.5 5 Section 5.2.12
At any rate, Section 5.2.12.2 Processor Local APIC structure, has Table 5-47 which is the Local APIC Structure Flags, and this is where Online Capable is introduced for the first time.
Quoted 9 lines
Quoted 6 lines
However, as currently coded, for MADT.revision < 5,
acpi_is_processor_usable()no longer allows for possible hot pluggable cpus, which is a regressive change in behavior.This patch restores the behavior where for MADT.revision < 5, the
Avoid having "This patch" or "This commit" in the commit message. It is tautologically useless.
ok!
Quoted 6 lines
Also, do
$ git grep 'This patch' Documentation/processfor more details.
ok, will do!
Quoted 30 lines
Quoted 17 lines
presence of the lapic/x2apic structure implies a possible hotpluggable cpu.
Fixes e2869bd7af60 ("x86/acpi/boot: Do not register processors that cannot be onlined for x2APIC")Suggested-by Miguel Luis <[email protected]>Suggested-by Boris Ostrovsky <[email protected]>Signed-off-by Eric DeVolder <[email protected]>--- arch/x86/kernel/acpi/boot.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)arch/x86/kernel/acpi/boot.c +0
-0arch/x86/kernel/acpi/boot.c static bool __init acpi_is_processor_usable(u32 lapic_flags) · line 201
Quoted 10 lines
arch/x86/kernel/acpi/boot.c +6
-1@@ -193,7 +193,13 @@ static bool __init acpi_is_processor_usable(u32 lapic_flags)if (lapic_flags &ACPI_MADT_ENABLED )return true; -if (acpi_support_online_capable && (lapic_flags &ACPI_MADT_ONLINE_CAPABLE )) +/* +* Prior to MADT.revision 5, the presence of the Local x2/APIC +* structure _implicitly_ noted a possible hotpluggable cpu. +* Starting with MADT.revision 5, the Online Capable bit +* _explicitly_ indicates a hotpluggable cpu. +*/ In all your text
Quoted 1 line
s/cpu/CPU/g
ok, will do!
Quoted 8 lines
arch/x86/kernel/acpi/boot.c static bool __init acpi_is_processor_usable(u32 lapic_flags) · line 205
Quoted 6 lines
arch/x86/kernel/acpi/boot.c +1
-0+if (!acpi_support_online_capable || (lapic_flags &ACPI_MADT_ONLINE_CAPABLE ))return true;return false;Otherwise, the change makes sense to me.
ok!
Quoted 3 lines
Thx.
Borislav Petkov
maintainerQuoted 3 lines · Eric DeVolder
https://ueif.org/sites/default/files/resources/ACPI_6_3_May16.pdf
Section 5.2.12 MADT. Table 5-43 is the MADT Revision is numbered 5. However, ACPI 6.x specs got a little "sloppy" with Revision,
Yes, I've found what you're pointing out too. But exactly because of this sloppiness I'd like to see this more explicitly. Because we're basing functionality off of it and it is not some meaningless paperweight anymore.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteQuoted 10 lines · Borislav Petkov
Quoted 3 lines · Eric DeVolder
https://ueif.org/sites/default/files/resources/ACPI_6_3_May16.pdf
Section 5.2.12 MADT. Table 5-43 is the MADT Revision is numbered 5. However, ACPI 6.x specs got a little "sloppy" with Revision,Yes, I've found what you're pointing out too. But exactly because of this sloppiness I'd like to see this more explicitly. Because we're basing functionality off of it and it is not some meaningless paperweight anymore.
Boris and I talked offline and got the situation clarified. The corrected patch for MADT version handling is sent here:
https://lore.kernel.org/linux-pm/[email protected]/T/#u
Eric - Your patch is still needed though.
Quoted 12 lines · Eric DeVolder
The logic in
acpi_is_processor_usable()requires the Online Capable bit be set for hotpluggable cpus. The Online Capable bit is introduced in ACPI 6.3 and MADT.revision 5.However, as currently coded, for MADT.revision < 5,
acpi_is_processor_usable()no longer allows for possible hot pluggable cpus, which is a regressive change in behavior.This patch restores the behavior where for MADT.revision < 5, the presence of the lapic/x2apic structure implies a possible hotpluggable cpu.
Quoted 5 lines
Fixes e2869bd7af60 ("x86/acpi/boot: Do not register processors that cannot be onlined for x2APIC")Suggested-by Miguel Luis <[email protected]>Suggested-by Boris Ostrovsky <[email protected]>Signed-off-by Eric DeVolder <[email protected]>---
Quoted 22 lines
arch/x86/kernel/acpi/boot.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)arch/x86/kernel/acpi/boot.c +7
-1@@ -193,7 +193,13 @@ static bool __init acpi_is_processor_usable(u32 lapic_flags)if (lapic_flags &ACPI_MADT_ENABLED )return true; -if (acpi_support_online_capable && (lapic_flags &ACPI_MADT_ONLINE_CAPABLE )) +/* +* Prior to MADT.revision 5, the presence of the Local x2/APIC +* structure _implicitly_ noted a possible hotpluggable cpu. +* Starting with MADT.revision 5, the Online Capable bit +* _explicitly_ indicates a hotpluggable cpu. +*/ +if (!acpi_support_online_capable || (lapic_flags &ACPI_MADT_ONLINE_CAPABLE ))return true;return false;
[tip: x86/urgent] x86/acpi/boot: Correct acpi_is_processor_usable() check
In reply to Eric DeVolderThe following commit has been merged into the x86/urgent branch of tip:
The following commit has been merged into the x86/urgent branch of tip:
Commit-ID: fed8d8773b8ea68ad99d9eee8c8343bef9da2c2c Gitweb: https://git.kernel.org/tip/fed8d8773b8ea68ad99d9eee8c8343bef9da2c2c Author: Eric DeVolder <[email protected]> AuthorDate: Mon, 27 Mar 2023 15:10:26 -04:00 Committer: Borislav Petkov (AMD) <[email protected]> CommitterDate: Thu, 30 Mar 2023 11:07:30 +02:00
x86/acpi/boot: Correct acpi_is_processor_usable() check
The logic in acpi_is_processor_usable() requires the online capable bit be set for hotpluggable CPUs. The online capable bit has been introduced in ACPI 6.3.
However, for ACPI revisions < 6.3 which do not support that bit, CPUs should be reported as usable, not the other way around.
Reverse the check.
[ bp: Rewrite commit message. ]
--- arch/x86/kernel/acpi/boot.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
arch/x86/kernel/acpi/boot.c +2 -1
@@ -197,7 +197,8 @@ static bool __init acpi_is_processor_usable(u32 lapic_flags)if (lapic_flags &ACPI_MADT_ENABLED )return true; -if (acpi_support_online_capable && (lapic_flags &ACPI_MADT_ONLINE_CAPABLE )) +if (!acpi_support_online_capable || + (lapic_flags &ACPI_MADT_ONLINE_CAPABLE ))return true;return false;