mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH] x86/acpi/boot: Correct acpi_is_processor_usable() check again
@ 2025-11-11 14:53 Yazen Ghannam
  2025-11-12 13:56 ` Borislav Petkov
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Yazen Ghannam @ 2025-11-11 14:53 UTC (permalink / raw)
  To: x86
  Cc: linux-kernel, Yazen Ghannam, Michal Pecio, Eric DeVolder,
	Mario Limonciello

ACPI v6.3 defined a new "Online Capable" MADT LAPIC flag. This bit is
used in conjunction with the "Enabled" MADT LAPIC flag to determine if a
CPU can be enabled/hotplugged by the OS after boot.

Before the new bit was defined, the "Enabled" bit was explicitly
described like this (ACPI v6.0 wording provided):
"If zero, this processor is unusable, and the operating system
support will not attempt to use it"

This means that CPU hotplug (based on MADT) is not possible. Many BIOS
implementations follow this guidance. They may include LAPIC entries in
MADT for unavailable CPUs, but since these entries are marked with
"Enabled=0" it is expected that the OS will completely ignore these
entries.

However, QEMU will do the same (include entries with "Enabled=0") for
the purpose of allowing CPU hotplug within the guest.

Comment from QEMU function pc_madt_cpu_entry():
    /* ACPI spec says that LAPIC entry for non present
     * CPU may be omitted from MADT or it must be marked
     * as disabled. However omitting non present CPU from
     * MADT breaks hotplug on linux. So possible CPUs
     * should be put in MADT but kept disabled.
     */

Recent Linux topology changes broke the QEMU use case. A following fix
for the QEMU use case broke bare metal topology enumeration.

Rework the Linux MADT LAPIC flags check to allow the QEMU use case only
for guests and to maintain the ACPI spec behavior for bare metal.

Remove an unnecessary check added to fix a bare metal case introduced by
the QEMU "fix".

Fixes: fed8d8773b8e ("x86/acpi/boot: Correct acpi_is_processor_usable() check")
Fixes: f0551af02130 ("x86/topology: Ignore non-present APIC IDs in a present package")
Reported-by: Michal Pecio <michal.pecio@gmail.com>
Closes: https://lore.kernel.org/r/20251024204658.3da9bf3f.michal.pecio@gmail.com
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
Cc: stable@vger.kernel.org
Cc: Eric DeVolder <eric.devolder@oracle.com>
Cc: Mario Limonciello <mario.limonciello@amd.com>
---

Notes:
    Link:
    https://lore.kernel.org/r/20251024204658.3da9bf3f.michal.pecio@gmail.com
    
    Hi all,
    
    This patch came out of the discussion above.
    
    A number of folks (myself included) understood the ACPI MADT LAPIC
    "Enabled" flag to be potentially used for CPU hotplug. This is
    explicitly false based on the wording in older revisions of the ACPI
    spec.
    
    However, this understanding is used for QEMU. Hence we need a check to
    differentiate the virtualization and bare metal use cases.
    
    Thanks,
    Yazen

 arch/x86/kernel/acpi/boot.c    | 12 ++++++++----
 arch/x86/kernel/cpu/topology.c | 15 ---------------
 2 files changed, 8 insertions(+), 19 deletions(-)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 9fa321a95eb3..bc99398852c4 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -35,6 +35,7 @@
 #include <asm/smp.h>
 #include <asm/i8259.h>
 #include <asm/setup.h>
+#include <asm/hypervisor.h>
 
 #include "sleep.h" /* To include x86_acpi_suspend_lowlevel */
 static int __initdata acpi_force = 0;
@@ -164,11 +165,14 @@ 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))
-		return true;
+	/*
+	 * QEMU expects legacy "Enabled=0" LAPIC entries to be counted as usable
+	 * in order to support CPU hotplug in guests.
+	 */
+	if (!acpi_support_online_capable)
+		return !hypervisor_is_type(X86_HYPER_NATIVE);
 
-	return false;
+	return (lapic_flags & ACPI_MADT_ONLINE_CAPABLE);
 }
 
 static int __init
diff --git a/arch/x86/kernel/cpu/topology.c b/arch/x86/kernel/cpu/topology.c
index 6073a16628f9..425404e7b7b4 100644
--- a/arch/x86/kernel/cpu/topology.c
+++ b/arch/x86/kernel/cpu/topology.c
@@ -27,7 +27,6 @@
 #include <xen/xen.h>
 
 #include <asm/apic.h>
-#include <asm/hypervisor.h>
 #include <asm/io_apic.h>
 #include <asm/mpspec.h>
 #include <asm/msr.h>
@@ -240,20 +239,6 @@ static __init void topo_register_apic(u32 apic_id, u32 acpi_id, bool present)
 		cpuid_to_apicid[cpu] = apic_id;
 		topo_set_cpuids(cpu, apic_id, acpi_id);
 	} else {
-		u32 pkgid = topo_apicid(apic_id, TOPO_PKG_DOMAIN);
-
-		/*
-		 * Check for present APICs in the same package when running
-		 * on bare metal. Allow the bogosity in a guest.
-		 */
-		if (hypervisor_is_type(X86_HYPER_NATIVE) &&
-		    topo_unit_count(pkgid, TOPO_PKG_DOMAIN, phys_cpu_present_map)) {
-			pr_info_once("Ignoring hot-pluggable APIC ID %x in present package.\n",
-				     apic_id);
-			topo_info.nr_rejected_cpus++;
-			return;
-		}
-
 		topo_info.nr_disabled_cpus++;
 	}
 

base-commit: ed4f9638d905a97cebd49ecea85cc9b706b73761
-- 
2.51.2


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/acpi/boot: Correct acpi_is_processor_usable() check again
  2025-11-11 14:53 [PATCH] x86/acpi/boot: Correct acpi_is_processor_usable() check again Yazen Ghannam
@ 2025-11-12 13:56 ` Borislav Petkov
  2025-11-12 16:35   ` Mario Limonciello
  2025-11-12 18:05 ` Michal Pecio
  2025-11-14 16:23 ` Ricardo Neri
  2 siblings, 1 reply; 12+ messages in thread
From: Borislav Petkov @ 2025-11-12 13:56 UTC (permalink / raw)
  To: Yazen Ghannam
  Cc: x86, linux-kernel, Michal Pecio, Eric DeVolder, Mario Limonciello

On Tue, Nov 11, 2025 at 02:53:57PM +0000, Yazen Ghannam wrote:
> ACPI v6.3 defined a new "Online Capable" MADT LAPIC flag. This bit is
> used in conjunction with the "Enabled" MADT LAPIC flag to determine if a
> CPU can be enabled/hotplugged by the OS after boot.
> 
> Before the new bit was defined, the "Enabled" bit was explicitly
> described like this (ACPI v6.0 wording provided):
> "If zero, this processor is unusable, and the operating system
> support will not attempt to use it"
> 
> This means that CPU hotplug (based on MADT) is not possible. Many BIOS
> implementations follow this guidance. They may include LAPIC entries in
> MADT for unavailable CPUs, but since these entries are marked with
> "Enabled=0" it is expected that the OS will completely ignore these
> entries.
> 
> However, QEMU will do the same (include entries with "Enabled=0") for
> the purpose of allowing CPU hotplug within the guest.
> 
> Comment from QEMU function pc_madt_cpu_entry():
>     /* ACPI spec says that LAPIC entry for non present
>      * CPU may be omitted from MADT or it must be marked
>      * as disabled. However omitting non present CPU from
>      * MADT breaks hotplug on linux. So possible CPUs
>      * should be put in MADT but kept disabled.
>      */
> 
> Recent Linux topology changes broke the QEMU use case. A following fix
> for the QEMU use case broke bare metal topology enumeration.
> 
> Rework the Linux MADT LAPIC flags check to allow the QEMU use case only
> for guests and to maintain the ACPI spec behavior for bare metal.
> 
> Remove an unnecessary check added to fix a bare metal case introduced by
> the QEMU "fix".
> 
> Fixes: fed8d8773b8e ("x86/acpi/boot: Correct acpi_is_processor_usable() check")
> Fixes: f0551af02130 ("x86/topology: Ignore non-present APIC IDs in a present package")
> Reported-by: Michal Pecio <michal.pecio@gmail.com>
> Closes: https://lore.kernel.org/r/20251024204658.3da9bf3f.michal.pecio@gmail.com
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> Cc: stable@vger.kernel.org
> Cc: Eric DeVolder <eric.devolder@oracle.com>
> Cc: Mario Limonciello <mario.limonciello@amd.com>
> ---
> 
> Notes:
>     Link:
>     https://lore.kernel.org/r/20251024204658.3da9bf3f.michal.pecio@gmail.com
>     
>     Hi all,
>     
>     This patch came out of the discussion above.
>     
>     A number of folks (myself included) understood the ACPI MADT LAPIC
>     "Enabled" flag to be potentially used for CPU hotplug. This is
>     explicitly false based on the wording in older revisions of the ACPI
>     spec.
>     
>     However, this understanding is used for QEMU. Hence we need a check to
>     differentiate the virtualization and bare metal use cases.

...

> +       return (lapic_flags & ACPI_MADT_ONLINE_CAPABLE);

Superfluous brackets.

In any case, yah, I'm going to queue this soon but I'll give Eric some time to
complain before I do.

I did play with:

https://www.qemu.org/docs/master/system/cpu-hotplug.html

and that works with that patch:

$ ./qmp-shell -p -v localhost:4444
Welcome to the QMP low-level shell!
Connected to QEMU 10.1.0
 
(QEMU) query-hotpluggable-cpus
{
    "arguments": {},
    "execute": "query-hotpluggable-cpus"
}
{
    "return": [
        {
            "props": {
                "core-id": 1,
                "socket-id": 0,
                "thread-id": 0
            },
            "type": "host-x86_64-cpu",
            "vcpus-count": 1
        },
        {
            "props": {
                "core-id": 0,
                "socket-id": 0,
                "thread-id": 0
            },
            "qom-path": "/machine/unattached/device[0]",
            "type": "host-x86_64-cpu",
            "vcpus-count": 1
        }
    ]
}
(QEMU) device_add id=cpu-2 driver=host-x86_64-cpu socket-id=0 core-id=1 thread-id=0
{
    "arguments": {
        "core-id": 1,
        "driver": "host-x86_64-cpu",
        "id": "cpu-2",
        "socket-id": 0,
        "thread-id": 0
    },
    "execute": "device_add"
}
{
    "return": {}
}
(QEMU)

and dmesg has:

[   33.281150] ACPI: acpi_map_cpu: cpu: 1, physid: 0x1, acpi_id: 0x1
[   33.289650] ACPI: CPU1 has been hot-added

But man oh man, if this ain't wagging the dog I don't know what is - we're
fixing the kernel so that qemu can hotplug because, oh well, "but then there
is this thing called reality^Wvirtualization which ruins everything."...

-- 
Regards/Gruss,
    Boris.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/acpi/boot: Correct acpi_is_processor_usable() check again
  2025-11-12 13:56 ` Borislav Petkov
@ 2025-11-12 16:35   ` Mario Limonciello
  2025-11-12 17:21     ` Borislav Petkov
  0 siblings, 1 reply; 12+ messages in thread
From: Mario Limonciello @ 2025-11-12 16:35 UTC (permalink / raw)
  To: Borislav Petkov, Yazen Ghannam
  Cc: x86, linux-kernel, Michal Pecio, Eric DeVolder

On 11/12/25 7:56 AM, Borislav Petkov wrote:
> On Tue, Nov 11, 2025 at 02:53:57PM +0000, Yazen Ghannam wrote:
>> ACPI v6.3 defined a new "Online Capable" MADT LAPIC flag. This bit is
>> used in conjunction with the "Enabled" MADT LAPIC flag to determine if a
>> CPU can be enabled/hotplugged by the OS after boot.
>>
>> Before the new bit was defined, the "Enabled" bit was explicitly
>> described like this (ACPI v6.0 wording provided):
>> "If zero, this processor is unusable, and the operating system
>> support will not attempt to use it"
>>
>> This means that CPU hotplug (based on MADT) is not possible. Many BIOS
>> implementations follow this guidance. They may include LAPIC entries in
>> MADT for unavailable CPUs, but since these entries are marked with
>> "Enabled=0" it is expected that the OS will completely ignore these
>> entries.
>>
>> However, QEMU will do the same (include entries with "Enabled=0") for
>> the purpose of allowing CPU hotplug within the guest.
>>
>> Comment from QEMU function pc_madt_cpu_entry():
>>      /* ACPI spec says that LAPIC entry for non present
>>       * CPU may be omitted from MADT or it must be marked
>>       * as disabled. However omitting non present CPU from
>>       * MADT breaks hotplug on linux. So possible CPUs
>>       * should be put in MADT but kept disabled.
>>       */
>>
>> Recent Linux topology changes broke the QEMU use case. A following fix
>> for the QEMU use case broke bare metal topology enumeration.
>>
>> Rework the Linux MADT LAPIC flags check to allow the QEMU use case only
>> for guests and to maintain the ACPI spec behavior for bare metal.
>>
>> Remove an unnecessary check added to fix a bare metal case introduced by
>> the QEMU "fix".
>>
>> Fixes: fed8d8773b8e ("x86/acpi/boot: Correct acpi_is_processor_usable() check")
>> Fixes: f0551af02130 ("x86/topology: Ignore non-present APIC IDs in a present package")
>> Reported-by: Michal Pecio <michal.pecio@gmail.com>
>> Closes: https://lore.kernel.org/r/20251024204658.3da9bf3f.michal.pecio@gmail.com
>> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
>> Cc: stable@vger.kernel.org
>> Cc: Eric DeVolder <eric.devolder@oracle.com>
>> Cc: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>
>> Notes:
>>      Link:
>>      https://lore.kernel.org/r/20251024204658.3da9bf3f.michal.pecio@gmail.com
>>      
>>      Hi all,
>>      
>>      This patch came out of the discussion above.
>>      
>>      A number of folks (myself included) understood the ACPI MADT LAPIC
>>      "Enabled" flag to be potentially used for CPU hotplug. This is
>>      explicitly false based on the wording in older revisions of the ACPI
>>      spec.
>>      
>>      However, this understanding is used for QEMU. Hence we need a check to
>>      differentiate the virtualization and bare metal use cases.
> 
> ...
> 
>> +       return (lapic_flags & ACPI_MADT_ONLINE_CAPABLE);
> 
> Superfluous brackets.
> 
> In any case, yah, I'm going to queue this soon but I'll give Eric some time to
> complain before I do.
> 
> I did play with:
> 
> https://www.qemu.org/docs/master/system/cpu-hotplug.html
> 
> and that works with that patch:
> 
> $ ./qmp-shell -p -v localhost:4444
> Welcome to the QMP low-level shell!
> Connected to QEMU 10.1.0
>   
> (QEMU) query-hotpluggable-cpus
> {
>      "arguments": {},
>      "execute": "query-hotpluggable-cpus"
> }
> {
>      "return": [
>          {
>              "props": {
>                  "core-id": 1,
>                  "socket-id": 0,
>                  "thread-id": 0
>              },
>              "type": "host-x86_64-cpu",
>              "vcpus-count": 1
>          },
>          {
>              "props": {
>                  "core-id": 0,
>                  "socket-id": 0,
>                  "thread-id": 0
>              },
>              "qom-path": "/machine/unattached/device[0]",
>              "type": "host-x86_64-cpu",
>              "vcpus-count": 1
>          }
>      ]
> }
> (QEMU) device_add id=cpu-2 driver=host-x86_64-cpu socket-id=0 core-id=1 thread-id=0
> {
>      "arguments": {
>          "core-id": 1,
>          "driver": "host-x86_64-cpu",
>          "id": "cpu-2",
>          "socket-id": 0,
>          "thread-id": 0
>      },
>      "execute": "device_add"
> }
> {
>      "return": {}
> }
> (QEMU)
> 
> and dmesg has:
> 
> [   33.281150] ACPI: acpi_map_cpu: cpu: 1, physid: 0x1, acpi_id: 0x1
> [   33.289650] ACPI: CPU1 has been hot-added
> 
> But man oh man, if this ain't wagging the dog I don't know what is - we're
> fixing the kernel so that qemu can hotplug because, oh well, "but then there
> is this thing called reality^Wvirtualization which ruins everything."...
> 

What version does the MADT in QEMU report today?  Presumably not all 
virtualization solutions make this assumption about this bit.

IE rather than clump all virtualization should we just detect QEMU and < 
ACPI 6.3?


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/acpi/boot: Correct acpi_is_processor_usable() check again
  2025-11-12 16:35   ` Mario Limonciello
@ 2025-11-12 17:21     ` Borislav Petkov
  2025-11-13 14:27       ` Borislav Petkov
  0 siblings, 1 reply; 12+ messages in thread
From: Borislav Petkov @ 2025-11-12 17:21 UTC (permalink / raw)
  To: Mario Limonciello
  Cc: Yazen Ghannam, x86, linux-kernel, Michal Pecio, Eric DeVolder

On Wed, Nov 12, 2025 at 10:35:13AM -0600, Mario Limonciello wrote:
> What version does the MADT in QEMU report today?  Presumably not all
> virtualization solutions make this assumption about this bit.
> 
> IE rather than clump all virtualization should we just detect QEMU and <
> ACPI 6.3?

Version 3. Yah, no idea what that means.

There's some discussion here:

https://lore.kernel.org/qemu-devel/20230510174510.089ecb14@imammedo.users.ipa.redhat.com/

but I'm not fixing the kernel because qemu is doing what it desires.
 
-- 
Regards/Gruss,
    Boris.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/acpi/boot: Correct acpi_is_processor_usable() check again
  2025-11-11 14:53 [PATCH] x86/acpi/boot: Correct acpi_is_processor_usable() check again Yazen Ghannam
  2025-11-12 13:56 ` Borislav Petkov
@ 2025-11-12 18:05 ` Michal Pecio
  2025-11-12 19:17   ` Borislav Petkov
  2025-11-14 16:23 ` Ricardo Neri
  2 siblings, 1 reply; 12+ messages in thread
From: Michal Pecio @ 2025-11-12 18:05 UTC (permalink / raw)
  To: Yazen Ghannam; +Cc: x86, linux-kernel, Eric DeVolder, Mario Limonciello

On Tue, 11 Nov 2025 14:53:57 +0000, Yazen Ghannam wrote:
> ACPI v6.3 defined a new "Online Capable" MADT LAPIC flag. This bit is
> used in conjunction with the "Enabled" MADT LAPIC flag to determine if a
> CPU can be enabled/hotplugged by the OS after boot.
> 
> Before the new bit was defined, the "Enabled" bit was explicitly
> described like this (ACPI v6.0 wording provided):
> "If zero, this processor is unusable, and the operating system
> support will not attempt to use it"
> 
> This means that CPU hotplug (based on MADT) is not possible. Many BIOS
> implementations follow this guidance. They may include LAPIC entries in
> MADT for unavailable CPUs, but since these entries are marked with
> "Enabled=0" it is expected that the OS will completely ignore these
> entries.
> 
> However, QEMU will do the same (include entries with "Enabled=0") for
> the purpose of allowing CPU hotplug within the guest.
> 
> Comment from QEMU function pc_madt_cpu_entry():
>     /* ACPI spec says that LAPIC entry for non present
>      * CPU may be omitted from MADT or it must be marked
>      * as disabled. However omitting non present CPU from
>      * MADT breaks hotplug on linux. So possible CPUs
>      * should be put in MADT but kept disabled.
>      */
> 
> Recent Linux topology changes broke the QEMU use case. A following fix
> for the QEMU use case broke bare metal topology enumeration.
> 
> Rework the Linux MADT LAPIC flags check to allow the QEMU use case only
> for guests and to maintain the ACPI spec behavior for bare metal.
> 
> Remove an unnecessary check added to fix a bare metal case introduced by
> the QEMU "fix".
> 
> Fixes: fed8d8773b8e ("x86/acpi/boot: Correct acpi_is_processor_usable() check")
> Fixes: f0551af02130 ("x86/topology: Ignore non-present APIC IDs in a present package")
> Reported-by: Michal Pecio <michal.pecio@gmail.com>
> Closes: https://lore.kernel.org/r/20251024204658.3da9bf3f.michal.pecio@gmail.com
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> Cc: stable@vger.kernel.org
> Cc: Eric DeVolder <eric.devolder@oracle.com>
> Cc: Mario Limonciello <mario.limonciello@amd.com>

Tested-by: Michal Pecio <michal.pecio@gmail.com>

Confirmed to fix the original machine where I discovered this bug and
looks like it would fix all the others too.

My BIOS claims conformance to ACPI 3.0 and the definition of "enable"
bit was the same as in 6.0 quoted here.

> ---
> 
> Notes:
>     Link:
>     https://lore.kernel.org/r/20251024204658.3da9bf3f.michal.pecio@gmail.com
>     
>     Hi all,
>     
>     This patch came out of the discussion above.
>     
>     A number of folks (myself included) understood the ACPI MADT LAPIC
>     "Enabled" flag to be potentially used for CPU hotplug. This is
>     explicitly false based on the wording in older revisions of the ACPI
>     spec.
>     
>     However, this understanding is used for QEMU. Hence we need a check to
>     differentiate the virtualization and bare metal use cases.
>     
>     Thanks,
>     Yazen
> 
>  arch/x86/kernel/acpi/boot.c    | 12 ++++++++----
>  arch/x86/kernel/cpu/topology.c | 15 ---------------
>  2 files changed, 8 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> index 9fa321a95eb3..bc99398852c4 100644
> --- a/arch/x86/kernel/acpi/boot.c
> +++ b/arch/x86/kernel/acpi/boot.c
> @@ -35,6 +35,7 @@
>  #include <asm/smp.h>
>  #include <asm/i8259.h>
>  #include <asm/setup.h>
> +#include <asm/hypervisor.h>
>  
>  #include "sleep.h" /* To include x86_acpi_suspend_lowlevel */
>  static int __initdata acpi_force = 0;
> @@ -164,11 +165,14 @@ 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))
> -		return true;
> +	/*
> +	 * QEMU expects legacy "Enabled=0" LAPIC entries to be counted as usable
> +	 * in order to support CPU hotplug in guests.
> +	 */
> +	if (!acpi_support_online_capable)
> +		return !hypervisor_is_type(X86_HYPER_NATIVE);
>  
> -	return false;
> +	return (lapic_flags & ACPI_MADT_ONLINE_CAPABLE);
>  }

Nitpick: IMO logic would be easier to follow if written this way:

	if (lapic_flags & ACPI_MADT_ENABLED)
		return true;

	if (acpi_support_online_capable)
		return lapic_flags & ACPI_MADT_ONLINE_CAPABLE;

	/* we should say 'no' at this point, but VMs are crazy */
	return !hypervisor_is_type(X86_HYPER_NATIVE);

>  
>  static int __init
> diff --git a/arch/x86/kernel/cpu/topology.c b/arch/x86/kernel/cpu/topology.c
> index 6073a16628f9..425404e7b7b4 100644
> --- a/arch/x86/kernel/cpu/topology.c
> +++ b/arch/x86/kernel/cpu/topology.c
> @@ -27,7 +27,6 @@
>  #include <xen/xen.h>
>  
>  #include <asm/apic.h>
> -#include <asm/hypervisor.h>
>  #include <asm/io_apic.h>
>  #include <asm/mpspec.h>
>  #include <asm/msr.h>
> @@ -240,20 +239,6 @@ static __init void topo_register_apic(u32 apic_id, u32 acpi_id, bool present)
>  		cpuid_to_apicid[cpu] = apic_id;
>  		topo_set_cpuids(cpu, apic_id, acpi_id);
>  	} else {
> -		u32 pkgid = topo_apicid(apic_id, TOPO_PKG_DOMAIN);
> -
> -		/*
> -		 * Check for present APICs in the same package when running
> -		 * on bare metal. Allow the bogosity in a guest.
> -		 */
> -		if (hypervisor_is_type(X86_HYPER_NATIVE) &&
> -		    topo_unit_count(pkgid, TOPO_PKG_DOMAIN, phys_cpu_present_map)) {
> -			pr_info_once("Ignoring hot-pluggable APIC ID %x in present package.\n",
> -				     apic_id);
> -			topo_info.nr_rejected_cpus++;
> -			return;
> -		}
> -
>  		topo_info.nr_disabled_cpus++;
>  	}
>  
> 
> base-commit: ed4f9638d905a97cebd49ecea85cc9b706b73761
> -- 
> 2.51.2
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/acpi/boot: Correct acpi_is_processor_usable() check again
  2025-11-12 18:05 ` Michal Pecio
@ 2025-11-12 19:17   ` Borislav Petkov
  0 siblings, 0 replies; 12+ messages in thread
From: Borislav Petkov @ 2025-11-12 19:17 UTC (permalink / raw)
  To: Michal Pecio
  Cc: Yazen Ghannam, x86, linux-kernel, Eric DeVolder, Mario Limonciello

On Wed, Nov 12, 2025 at 07:05:18PM +0100, Michal Pecio wrote:
> Nitpick: IMO logic would be easier to follow if written this way:
> 
> 	if (lapic_flags & ACPI_MADT_ENABLED)
> 		return true;
> 
> 	if (acpi_support_online_capable)
> 		return lapic_flags & ACPI_MADT_ONLINE_CAPABLE;
> 
> 	/* we should say 'no' at this point, but VMs are crazy */
> 	return !hypervisor_is_type(X86_HYPER_NATIVE);

Sure, except I'd like to keep the original comment as that leaves more
breadcrumbs about why we're doing this insanity.

Thx.

-- 
Regards/Gruss,
    Boris.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH] x86/acpi/boot: Correct acpi_is_processor_usable() check again
  2025-11-12 17:21     ` Borislav Petkov
@ 2025-11-13 14:27       ` Borislav Petkov
  2025-11-13 14:30         ` Mario Limonciello
  0 siblings, 1 reply; 12+ messages in thread
From: Borislav Petkov @ 2025-11-13 14:27 UTC (permalink / raw)
  To: Mario Limonciello
  Cc: Yazen Ghannam, x86, linux-kernel, Michal Pecio, Eric DeVolder

Seems to work here, I will run it on a couple more machines and even if it is
stable material, I'd let it go through a full cycle merge window so that it
sees more testing.

Thx.

---
From: Yazen Ghannam <yazen.ghannam@amd.com>
Date: Tue, 11 Nov 2025 14:53:57 +0000

ACPI v6.3 defined a new "Online Capable" MADT LAPIC flag. This bit is
used in conjunction with the "Enabled" MADT LAPIC flag to determine if
a CPU can be enabled/hotplugged by the OS after boot.

Before the new bit was defined, the "Enabled" bit was explicitly
described like this (ACPI v6.0 wording provided):

  "If zero, this processor is unusable, and the operating system
  support will not attempt to use it"

This means that CPU hotplug (based on MADT) is not possible. Many BIOS
implementations follow this guidance. They may include LAPIC entries in
MADT for unavailable CPUs, but since these entries are marked with
"Enabled=0" it is expected that the OS will completely ignore these
entries.

However, QEMU will do the same (include entries with "Enabled=0") for
the purpose of allowing CPU hotplug within the guest.

Comment from QEMU function pc_madt_cpu_entry():

  /* ACPI spec says that LAPIC entry for non present
   * CPU may be omitted from MADT or it must be marked
   * as disabled. However omitting non present CPU from
   * MADT breaks hotplug on linux. So possible CPUs
   * should be put in MADT but kept disabled.
   */

Recent Linux topology changes broke the QEMU use case. A following fix
for the QEMU use case broke bare metal topology enumeration.

Rework the Linux MADT LAPIC flags check to allow the QEMU use case only
for guests and to maintain the ACPI spec behavior for bare metal.

Remove an unnecessary check added to fix a bare metal case introduced by
the QEMU "fix".

  [ bp: Change logic as Michal suggested. ]

Fixes: fed8d8773b8e ("x86/acpi/boot: Correct acpi_is_processor_usable() check")
Fixes: f0551af02130 ("x86/topology: Ignore non-present APIC IDs in a present package")
Closes: https://lore.kernel.org/r/20251024204658.3da9bf3f.michal.pecio@gmail.com
Reported-by: Michal Pecio <michal.pecio@gmail.com>
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
Tested-by: Michal Pecio <michal.pecio@gmail.com>
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/20251111145357.4031846-1-yazen.ghannam@amd.com
---
 arch/x86/kernel/acpi/boot.c    | 12 ++++++++----
 arch/x86/kernel/cpu/topology.c | 15 ---------------
 2 files changed, 8 insertions(+), 19 deletions(-)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 9fa321a95eb3..d6138b2b633a 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -35,6 +35,7 @@
 #include <asm/smp.h>
 #include <asm/i8259.h>
 #include <asm/setup.h>
+#include <asm/hypervisor.h>
 
 #include "sleep.h" /* To include x86_acpi_suspend_lowlevel */
 static int __initdata acpi_force = 0;
@@ -164,11 +165,14 @@ 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))
-		return true;
+	if (acpi_support_online_capable)
+		return lapic_flags & ACPI_MADT_ONLINE_CAPABLE;
 
-	return false;
+	/*
+	 * QEMU expects legacy "Enabled=0" LAPIC entries to be counted as usable
+	 * in order to support CPU hotplug in guests.
+	 */
+	return !hypervisor_is_type(X86_HYPER_NATIVE);
 }
 
 static int __init
diff --git a/arch/x86/kernel/cpu/topology.c b/arch/x86/kernel/cpu/topology.c
index 6073a16628f9..425404e7b7b4 100644
--- a/arch/x86/kernel/cpu/topology.c
+++ b/arch/x86/kernel/cpu/topology.c
@@ -27,7 +27,6 @@
 #include <xen/xen.h>
 
 #include <asm/apic.h>
-#include <asm/hypervisor.h>
 #include <asm/io_apic.h>
 #include <asm/mpspec.h>
 #include <asm/msr.h>
@@ -240,20 +239,6 @@ static __init void topo_register_apic(u32 apic_id, u32 acpi_id, bool present)
 		cpuid_to_apicid[cpu] = apic_id;
 		topo_set_cpuids(cpu, apic_id, acpi_id);
 	} else {
-		u32 pkgid = topo_apicid(apic_id, TOPO_PKG_DOMAIN);
-
-		/*
-		 * Check for present APICs in the same package when running
-		 * on bare metal. Allow the bogosity in a guest.
-		 */
-		if (hypervisor_is_type(X86_HYPER_NATIVE) &&
-		    topo_unit_count(pkgid, TOPO_PKG_DOMAIN, phys_cpu_present_map)) {
-			pr_info_once("Ignoring hot-pluggable APIC ID %x in present package.\n",
-				     apic_id);
-			topo_info.nr_rejected_cpus++;
-			return;
-		}
-
 		topo_info.nr_disabled_cpus++;
 	}
 
-- 
2.51.0


-- 
Regards/Gruss,
    Boris.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/acpi/boot: Correct acpi_is_processor_usable() check again
  2025-11-13 14:27       ` Borislav Petkov
@ 2025-11-13 14:30         ` Mario Limonciello
  2025-11-13 14:34           ` Borislav Petkov
  0 siblings, 1 reply; 12+ messages in thread
From: Mario Limonciello @ 2025-11-13 14:30 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Yazen Ghannam, x86, linux-kernel, Michal Pecio, Eric DeVolder

On 11/13/25 8:27 AM, Borislav Petkov wrote:
> Seems to work here, I will run it on a couple more machines and even if it is
> stable material, I'd let it go through a full cycle merge window so that it
> sees more testing.
> 
> Thx.
> 
> ---
> From: Yazen Ghannam <yazen.ghannam@amd.com>
> Date: Tue, 11 Nov 2025 14:53:57 +0000
> 
> ACPI v6.3 defined a new "Online Capable" MADT LAPIC flag. This bit is
> used in conjunction with the "Enabled" MADT LAPIC flag to determine if
> a CPU can be enabled/hotplugged by the OS after boot.
> 
> Before the new bit was defined, the "Enabled" bit was explicitly
> described like this (ACPI v6.0 wording provided):
> 
>    "If zero, this processor is unusable, and the operating system
>    support will not attempt to use it"
> 
> This means that CPU hotplug (based on MADT) is not possible. Many BIOS
> implementations follow this guidance. They may include LAPIC entries in
> MADT for unavailable CPUs, but since these entries are marked with
> "Enabled=0" it is expected that the OS will completely ignore these
> entries.
> 
> However, QEMU will do the same (include entries with "Enabled=0") for
> the purpose of allowing CPU hotplug within the guest.
> 
> Comment from QEMU function pc_madt_cpu_entry():
> 
>    /* ACPI spec says that LAPIC entry for non present
>     * CPU may be omitted from MADT or it must be marked
>     * as disabled. However omitting non present CPU from
>     * MADT breaks hotplug on linux. So possible CPUs
>     * should be put in MADT but kept disabled.
>     */
> 
> Recent Linux topology changes broke the QEMU use case. A following fix
> for the QEMU use case broke bare metal topology enumeration.
> 
> Rework the Linux MADT LAPIC flags check to allow the QEMU use case only
> for guests and to maintain the ACPI spec behavior for bare metal.
> 
> Remove an unnecessary check added to fix a bare metal case introduced by
> the QEMU "fix".
> 
>    [ bp: Change logic as Michal suggested. ]
> 
> Fixes: fed8d8773b8e ("x86/acpi/boot: Correct acpi_is_processor_usable() check")
> Fixes: f0551af02130 ("x86/topology: Ignore non-present APIC IDs in a present package")
> Closes: https://lore.kernel.org/r/20251024204658.3da9bf3f.michal.pecio@gmail.com
> Reported-by: Michal Pecio <michal.pecio@gmail.com>
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> Tested-by: Michal Pecio <michal.pecio@gmail.com>
> Cc: stable@vger.kernel.org
> Link: https://lore.kernel.org/20251111145357.4031846-1-yazen.ghannam@amd.com
> ---
>   arch/x86/kernel/acpi/boot.c    | 12 ++++++++----
>   arch/x86/kernel/cpu/topology.c | 15 ---------------
>   2 files changed, 8 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> index 9fa321a95eb3..d6138b2b633a 100644
> --- a/arch/x86/kernel/acpi/boot.c
> +++ b/arch/x86/kernel/acpi/boot.c
> @@ -35,6 +35,7 @@
>   #include <asm/smp.h>
>   #include <asm/i8259.h>
>   #include <asm/setup.h>
> +#include <asm/hypervisor.h>
>   
>   #include "sleep.h" /* To include x86_acpi_suspend_lowlevel */
>   static int __initdata acpi_force = 0;
> @@ -164,11 +165,14 @@ 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))
> -		return true;
> +	if (acpi_support_online_capable)
> +		return lapic_flags & ACPI_MADT_ONLINE_CAPABLE;
>   
> -	return false;
> +	/*
> +	 * QEMU expects legacy "Enabled=0" LAPIC entries to be counted as usable
> +	 * in order to support CPU hotplug in guests.
> +	 */

I think it would be good to amend this comment to indicate that the 
behavior of the "enabled bit" changed and that QEMU follows the newer 
behavior but advertises the older version.

> +	return !hypervisor_is_type(X86_HYPER_NATIVE);
>   }
>   
>   static int __init
> diff --git a/arch/x86/kernel/cpu/topology.c b/arch/x86/kernel/cpu/topology.c
> index 6073a16628f9..425404e7b7b4 100644
> --- a/arch/x86/kernel/cpu/topology.c
> +++ b/arch/x86/kernel/cpu/topology.c
> @@ -27,7 +27,6 @@
>   #include <xen/xen.h>
>   
>   #include <asm/apic.h>
> -#include <asm/hypervisor.h>
>   #include <asm/io_apic.h>
>   #include <asm/mpspec.h>
>   #include <asm/msr.h>
> @@ -240,20 +239,6 @@ static __init void topo_register_apic(u32 apic_id, u32 acpi_id, bool present)
>   		cpuid_to_apicid[cpu] = apic_id;
>   		topo_set_cpuids(cpu, apic_id, acpi_id);
>   	} else {
> -		u32 pkgid = topo_apicid(apic_id, TOPO_PKG_DOMAIN);
> -
> -		/*
> -		 * Check for present APICs in the same package when running
> -		 * on bare metal. Allow the bogosity in a guest.
> -		 */
> -		if (hypervisor_is_type(X86_HYPER_NATIVE) &&
> -		    topo_unit_count(pkgid, TOPO_PKG_DOMAIN, phys_cpu_present_map)) {
> -			pr_info_once("Ignoring hot-pluggable APIC ID %x in present package.\n",
> -				     apic_id);
> -			topo_info.nr_rejected_cpus++;
> -			return;
> -		}
> -
>   		topo_info.nr_disabled_cpus++;
>   	}
>   


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/acpi/boot: Correct acpi_is_processor_usable() check again
  2025-11-13 14:30         ` Mario Limonciello
@ 2025-11-13 14:34           ` Borislav Petkov
  2025-11-13 14:55             ` Mario Limonciello
  0 siblings, 1 reply; 12+ messages in thread
From: Borislav Petkov @ 2025-11-13 14:34 UTC (permalink / raw)
  To: Mario Limonciello
  Cc: Yazen Ghannam, x86, linux-kernel, Michal Pecio, Eric DeVolder

On Thu, Nov 13, 2025 at 08:30:27AM -0600, Mario Limonciello wrote:
> > +	/*
> > +	 * QEMU expects legacy "Enabled=0" LAPIC entries to be counted as usable
> > +	 * in order to support CPU hotplug in guests.
> > +	 */
> 
> I think it would be good to amend this comment to indicate that the behavior
> of the "enabled bit" changed and that QEMU follows the newer behavior but
> advertises the older version.

This is just there to hint why we're doing this fix to the kernel - i.e.,
try to salvage an already shit situation. Any other deviation from the spec is
not the kernel's problem.

-- 
Regards/Gruss,
    Boris.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/acpi/boot: Correct acpi_is_processor_usable() check again
  2025-11-13 14:34           ` Borislav Petkov
@ 2025-11-13 14:55             ` Mario Limonciello
  0 siblings, 0 replies; 12+ messages in thread
From: Mario Limonciello @ 2025-11-13 14:55 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Yazen Ghannam, x86, linux-kernel, Michal Pecio, Eric DeVolder

On 11/13/25 8:34 AM, Borislav Petkov wrote:
> On Thu, Nov 13, 2025 at 08:30:27AM -0600, Mario Limonciello wrote:
>>> +	/*
>>> +	 * QEMU expects legacy "Enabled=0" LAPIC entries to be counted as usable
>>> +	 * in order to support CPU hotplug in guests.
>>> +	 */
>>
>> I think it would be good to amend this comment to indicate that the behavior
>> of the "enabled bit" changed and that QEMU follows the newer behavior but
>> advertises the older version.
> 
> This is just there to hint why we're doing this fix to the kernel - i.e.,
> try to salvage an already shit situation. Any other deviation from the spec is
> not the kernel's problem.
> 

OK.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/acpi/boot: Correct acpi_is_processor_usable() check again
  2025-11-11 14:53 [PATCH] x86/acpi/boot: Correct acpi_is_processor_usable() check again Yazen Ghannam
  2025-11-12 13:56 ` Borislav Petkov
  2025-11-12 18:05 ` Michal Pecio
@ 2025-11-14 16:23 ` Ricardo Neri
  2025-11-14 16:53   ` Borislav Petkov
  2 siblings, 1 reply; 12+ messages in thread
From: Ricardo Neri @ 2025-11-14 16:23 UTC (permalink / raw)
  To: Yazen Ghannam
  Cc: x86, linux-kernel, Michal Pecio, Eric DeVolder, Mario Limonciello

On Tue, Nov 11, 2025 at 02:53:57PM +0000, Yazen Ghannam wrote:
> ACPI v6.3 defined a new "Online Capable" MADT LAPIC flag. This bit is
> used in conjunction with the "Enabled" MADT LAPIC flag to determine if a
> CPU can be enabled/hotplugged by the OS after boot.
> 
> Before the new bit was defined, the "Enabled" bit was explicitly
> described like this (ACPI v6.0 wording provided):
> "If zero, this processor is unusable, and the operating system
> support will not attempt to use it"
> 
> This means that CPU hotplug (based on MADT) is not possible. Many BIOS
> implementations follow this guidance. They may include LAPIC entries in
> MADT for unavailable CPUs, but since these entries are marked with
> "Enabled=0" it is expected that the OS will completely ignore these
> entries.
> 
> However, QEMU will do the same (include entries with "Enabled=0") for
> the purpose of allowing CPU hotplug within the guest.
> 
> Comment from QEMU function pc_madt_cpu_entry():
>     /* ACPI spec says that LAPIC entry for non present
>      * CPU may be omitted from MADT or it must be marked
>      * as disabled. However omitting non present CPU from
>      * MADT breaks hotplug on linux. So possible CPUs
>      * should be put in MADT but kept disabled.
>      */
> 
> Recent Linux topology changes broke the QEMU use case. A following fix
> for the QEMU use case broke bare metal topology enumeration.
> 
> Rework the Linux MADT LAPIC flags check to allow the QEMU use case only
> for guests and to maintain the ACPI spec behavior for bare metal.
> 
> Remove an unnecessary check added to fix a bare metal case introduced by
> the QEMU "fix".
> 
> Fixes: fed8d8773b8e ("x86/acpi/boot: Correct acpi_is_processor_usable() check")
> Fixes: f0551af02130 ("x86/topology: Ignore non-present APIC IDs in a present package")
> Reported-by: Michal Pecio <michal.pecio@gmail.com>
> Closes: https://lore.kernel.org/r/20251024204658.3da9bf3f.michal.pecio@gmail.com
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> Cc: stable@vger.kernel.org
> Cc: Eric DeVolder <eric.devolder@oracle.com>
> Cc: Mario Limonciello <mario.limonciello@amd.com>
> ---
> 
> Notes:
>     Link:
>     https://lore.kernel.org/r/20251024204658.3da9bf3f.michal.pecio@gmail.com
>     
>     Hi all,
>     
>     This patch came out of the discussion above.
>     
>     A number of folks (myself included) understood the ACPI MADT LAPIC
>     "Enabled" flag to be potentially used for CPU hotplug. This is
>     explicitly false based on the wording in older revisions of the ACPI
>     spec.
>     
>     However, this understanding is used for QEMU. Hence we need a check to
>     differentiate the virtualization and bare metal use cases.
>     
>     Thanks,
>     Yazen

Tested-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>

I tested this on a 4-socket Cascade Lake server with a BIOS based on ACPI 6.1.
All the lAPIC structures had the Enable bit set. It booted successfully.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/acpi/boot: Correct acpi_is_processor_usable() check again
  2025-11-14 16:23 ` Ricardo Neri
@ 2025-11-14 16:53   ` Borislav Petkov
  0 siblings, 0 replies; 12+ messages in thread
From: Borislav Petkov @ 2025-11-14 16:53 UTC (permalink / raw)
  To: Ricardo Neri
  Cc: Yazen Ghannam, x86, linux-kernel, Michal Pecio, Eric DeVolder,
	Mario Limonciello

On Fri, Nov 14, 2025 at 08:23:58AM -0800, Ricardo Neri wrote:
> Tested-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> 
> I tested this on a 4-socket Cascade Lake server with a BIOS based on ACPI 6.1.
> All the lAPIC structures had the Enable bit set. It booted successfully.

Thanks a lot! It looks good in my testing too, so far.

I'll queue it soon.

-- 
Regards/Gruss,
    Boris.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2025-11-14 16:54 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-11-11 14:53 [PATCH] x86/acpi/boot: Correct acpi_is_processor_usable() check again Yazen Ghannam
2025-11-12 13:56 ` Borislav Petkov
2025-11-12 16:35   ` Mario Limonciello
2025-11-12 17:21     ` Borislav Petkov
2025-11-13 14:27       ` Borislav Petkov
2025-11-13 14:30         ` Mario Limonciello
2025-11-13 14:34           ` Borislav Petkov
2025-11-13 14:55             ` Mario Limonciello
2025-11-12 18:05 ` Michal Pecio
2025-11-12 19:17   ` Borislav Petkov
2025-11-14 16:23 ` Ricardo Neri
2025-11-14 16:53   ` Borislav Petkov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®