diff mbox series

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

Message ID 20230327191026.3454-2-eric.devolder@oracle.com
State New
Headers
Series x86/acpi: acpi_is_processor_usable() dropping possible cpus |

Commit Message

Eric DeVolder March 27, 2023, 7:10 p.m. UTC 
  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(-)

Comments

Borislav Petkov March 27, 2023, 7:57 p.m. UTC | #1 
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.
Eric DeVolder March 27, 2023, 8:07 p.m. UTC | #2 
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.
>
Borislav Petkov March 27, 2023, 8:25 p.m. UTC | #3 
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.
Mario Limonciello March 29, 2023, 7:34 p.m. UTC | #4 
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.
Mario Limonciello March 29, 2023, 7:35 p.m. UTC | #5 
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;
diff mbox series

Patch

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;