[PATCH 0/1] x86/acpi: acpi_is_processor_usable() dropping possible cpus

[PATCH 0/1] x86/acpi: acpi_is_processor_usable() dropping possible cpus

[Eric DeVolder]

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(-)

-- 
2.31.1

[PATCH 1/1] x86/acpi: acpi_is_processor_usable() dropping possible cpus

[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.

Fixes: e2869bd7af60 ("x86/acpi/boot: Do not register processors that cannot be onlined for x2APIC")
Suggested-by: Miguel Luis <miguel.luis@oracle.com>
Suggested-by: Boris Ostrovsky <boris.ovstrosky@oracle.com>
Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
---
 arch/x86/kernel/acpi/boot.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 1c38174b5f01..7b5b8ed018b0 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -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;
-- 
2.31.1

Re: [PATCH 1/1] x86/acpi: acpi_is_processor_usable() dropping possible cpus

[Borislav Petkov]

On Mon, Mar 27, 2023 at 03:10:26PM -0400, Eric DeVolder wrote:
> 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?

> 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.

> 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 <miguel.luis@oracle.com>
> Suggested-by: Boris Ostrovsky <boris.ovstrosky@oracle.com>
> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
> ---
>  arch/x86/kernel/acpi/boot.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> index 1c38174b5f01..7b5b8ed018b0 100644
> --- a/arch/x86/kernel/acpi/boot.c
> +++ b/arch/x86/kernel/acpi/boot.c
> @@ -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

> +	if (!acpi_support_online_capable || (lapic_flags & ACPI_MADT_ONLINE_CAPABLE))
>  		return true;
>  
>  	return false;
> -- 

Otherwise, the change makes sense to me.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 1/1] x86/acpi: acpi_is_processor_usable() dropping possible cpus

[Eric DeVolder]

On 3/27/23 14:57, Borislav Petkov wrote:
> On Mon, Mar 27, 2023 at 03:10:26PM -0400, Eric DeVolder wrote:
>> 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?

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.


> 
>> 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!

> 
> Also, do
> 
> $ git grep 'This patch' Documentation/process
> 
> for more details.
ok, will do!

> 
>> 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 <miguel.luis@oracle.com>
>> Suggested-by: Boris Ostrovsky <boris.ovstrosky@oracle.com>
>> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
>> ---
>>   arch/x86/kernel/acpi/boot.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
>> index 1c38174b5f01..7b5b8ed018b0 100644
>> --- a/arch/x86/kernel/acpi/boot.c
>> +++ b/arch/x86/kernel/acpi/boot.c
>> @@ -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
ok, will do!
> 
>> +	if (!acpi_support_online_capable || (lapic_flags & ACPI_MADT_ONLINE_CAPABLE))
>>   		return true;
>>   
>>   	return false;
>> -- 
> 
> Otherwise, the change makes sense to me.
ok!

> 
> Thx.
> 

Re: [PATCH 1/1] x86/acpi: acpi_is_processor_usable() dropping possible cpus

[Borislav Petkov]

On Mon, Mar 27, 2023 at 03:07:23PM -0500, Eric DeVolder wrote:
> 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.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 1/1] x86/acpi: acpi_is_processor_usable() dropping possible cpus

[Limonciello, Mario]

On 3/27/2023 15:25, Borislav Petkov wrote:
> On Mon, Mar 27, 2023 at 03:07:23PM -0500, Eric DeVolder wrote:
>> 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/20230329174536.6931-1-mario.limonciello@amd.com/T/#u

Eric - Your patch is still needed though.

Re: [PATCH 1/1] x86/acpi: acpi_is_processor_usable() dropping possible cpus

[Limonciello, Mario]

On 3/27/2023 14:10, Eric DeVolder wrote:
> 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.
> 
> Fixes: e2869bd7af60 ("x86/acpi/boot: Do not register processors that cannot be onlined for x2APIC")
> Suggested-by: Miguel Luis <miguel.luis@oracle.com>
> Suggested-by: Boris Ostrovsky <boris.ovstrosky@oracle.com>
> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
> ---

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Tested-by: David R <david@unsolicited.net>
Link: 
https://lore.kernel.org/all/a15c6a64-dcd7-b102-9d9f-0225dfa1172b@unsolicited.net/T/

>   arch/x86/kernel/acpi/boot.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> index 1c38174b5f01..7b5b8ed018b0 100644
> --- a/arch/x86/kernel/acpi/boot.c
> +++ b/arch/x86/kernel/acpi/boot.c
> @@ -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

[tip-bot2 for Eric DeVolder]

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 <eric.devolder@oracle.com>
AuthorDate:    Mon, 27 Mar 2023 15:10:26 -04:00
Committer:     Borislav Petkov (AMD) <bp@alien8.de>
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. ]

Fixes: e2869bd7af60 ("x86/acpi/boot: Do not register processors that cannot be onlined for x2APIC")
Suggested-by: Miguel Luis <miguel.luis@oracle.com>
Suggested-by: Boris Ostrovsky <boris.ovstrosky@oracle.com>
Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Tested-by: David R <david@unsolicited.net>
Cc: <stable@kernel.org>
Link: https://lore.kernel.org/r/20230327191026.3454-2-eric.devolder@oracle.com
---
 arch/x86/kernel/acpi/boot.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 7292184..0dac4ab 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -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;