[RFC PATCH] x86/acpi: Ignore invalid x2APIC entries

[RFC PATCH] x86/acpi: Ignore invalid x2APIC entries

[Zhang Rui]

Currently, kernel enumerates the possible CPUs by parsing both ACPI MADT
Local APIC entries and x2APIC entries. So CPUs with "valid" APIC IDs,
even if they have duplicated APIC IDs in Local APIC and x2APIC, are
always enumerated.

Below is what ACPI MADT Local APIC and x2APIC describes on an
Ivebridge-EP system,

[02Ch 0044   1]                Subtable Type : 00 [Processor Local APIC]
[02Fh 0047   1]                Local Apic ID : 00
...
[164h 0356   1]                Subtable Type : 00 [Processor Local APIC]
[167h 0359   1]                Local Apic ID : 39
[16Ch 0364   1]                Subtable Type : 00 [Processor Local APIC]
[16Fh 0367   1]                Local Apic ID : FF
...
[3ECh 1004   1]                Subtable Type : 09 [Processor Local x2APIC]
[3F0h 1008   4]                Processor x2Apic ID : 00000000
...
[B5Ch 2908   1]                Subtable Type : 09 [Processor Local x2APIC]
[B60h 2912   4]                Processor x2Apic ID : 00000077

As a result, kernel shows "smpboot: Allowing 168 CPUs, 120 hotplug CPUs".
And this wastes significant amount of memory for the per-cpu data.
Plus this also breaks https://lore.kernel.org/all/87edm36qqb.ffs@tglx/,
because __max_logical_packages is over-estimated by the APIC IDs in
the x2APIC entries.

According to https://uefi.org/specs/ACPI/6.5/05_ACPI_Software_Programming_Model.html#processor-local-x2apic-structure,
"[Compatibility note] On some legacy OSes, Logical processors with APIC
ID values less than 255 (whether in XAPIC or X2APIC mode) must use the
Processor Local APIC structure to convey their APIC information to OSPM,
and those processors must be declared in the DSDT using the Processor()
keyword. Logical processors with APIC ID values 255 and greater must use
the Processor Local x2APIC structure and be declared using the Device()
keyword.".

Enumerate CPUs from x2APIC enties with APIC ID values 255 or greater,
when valid CPU from Local APIC is already detected.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
I didn't find any clear statement in the ACPI spec about if a mixture of
Local APIC and x2APIC entries is allowed or not. So it would be great if
this can be clarified.

And FYI, I have auditted a series of Intel servers, and one IVB-EP in
LKP lab and one IVB-EP from PeterZ are the only ones with a mixture of
Local APIC entries + x2APIC entries.

Plat    Status
IVB-EP  valid LAPIC + invalid LAPIC (APIC ID 0xFF) + unknown x2APIC entries (valid APIC ID + Enable bit cleared)
IVB-EP  valid LAPIC + invalid LAPIC (APIC ID 0xFF) + unknown x2APIC entries (valid APIC ID + Enable bit cleared)
CLX     valid LAPIC + invalid LAPIC (APIC ID 0xFF) + invalid x2APIC entries (APIC ID 0xFFFFFFFF)
CLX     valid LAPIC + invalid LAPIC (APIC ID 0xFF) + invalid x2APIC entries (APIC ID 0xFFFFFFFF)
ICX     valid LAPIC only
SPR     valid LAPIC only
SPR     valid x2APIC only
---
 arch/x86/kernel/acpi/boot.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 21b542a6866c..a41124d58e29 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -204,6 +204,8 @@ static bool __init acpi_is_processor_usable(u32 lapic_flags)
 	return false;
 }
 
+static bool has_lapic_cpus;
+
 static int __init
 acpi_parse_x2apic(union acpi_subtable_headers *header, const unsigned long end)
 {
@@ -232,6 +234,14 @@ acpi_parse_x2apic(union acpi_subtable_headers *header, const unsigned long end)
 	if (!acpi_is_processor_usable(processor->lapic_flags))
 		return 0;
 
+	/*
+	 * According to https://uefi.org/specs/ACPI/6.5/05_ACPI_Software_Programming_Model.html#processor-local-x2apic-structure
+	 * when MADT provides both valid LAPIC and x2APIC entries, the APIC ID
+	 * in x2APIC must be equal or greater than 0xff.
+	 */
+	if (has_lapic_cpus && apic_id < 0xff)
+		return 0;
+
 	/*
 	 * We need to register disabled CPU as well to permit
 	 * counting disabled CPUs. This allows us to size
@@ -257,6 +267,7 @@ static int __init
 acpi_parse_lapic(union acpi_subtable_headers * header, const unsigned long end)
 {
 	struct acpi_madt_local_apic *processor = NULL;
+	int cpu;
 
 	processor = (struct acpi_madt_local_apic *)header;
 
@@ -280,10 +291,11 @@ acpi_parse_lapic(union acpi_subtable_headers * header, const unsigned long end)
 	 * to not preallocating memory for all NR_CPUS
 	 * when we use CPU hotplug.
 	 */
-	acpi_register_lapic(processor->id,	/* APIC ID */
+	cpu = acpi_register_lapic(processor->id,	/* APIC ID */
 			    processor->processor_id, /* ACPI ID */
 			    processor->lapic_flags & ACPI_MADT_ENABLED);
-
+	if (cpu >= 0)
+		has_lapic_cpus = true;
 	return 0;
 }
 
@@ -1123,21 +1135,10 @@ static int __init acpi_parse_madt_lapic_entries(void)
 				      acpi_parse_sapic, MAX_LOCAL_APIC);
 
 	if (!count) {
-		memset(madt_proc, 0, sizeof(madt_proc));
-		madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_APIC;
-		madt_proc[0].handler = acpi_parse_lapic;
-		madt_proc[1].id = ACPI_MADT_TYPE_LOCAL_X2APIC;
-		madt_proc[1].handler = acpi_parse_x2apic;
-		ret = acpi_table_parse_entries_array(ACPI_SIG_MADT,
-				sizeof(struct acpi_table_madt),
-				madt_proc, ARRAY_SIZE(madt_proc), MAX_LOCAL_APIC);
-		if (ret < 0) {
-			pr_err("Error parsing LAPIC/X2APIC entries\n");
-			return ret;
-		}
-
-		count = madt_proc[0].count;
-		x2count = madt_proc[1].count;
+		count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC,
+					acpi_parse_lapic, MAX_LOCAL_APIC);
+		x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC,
+					acpi_parse_x2apic, MAX_LOCAL_APIC);
 	}
 	if (!count && !x2count) {
 		pr_err("No LAPIC entries present\n");
-- 
2.34.1

Re: [RFC PATCH] x86/acpi: Ignore invalid x2APIC entries

[Thomas Gleixner]

On Mon, Jul 03 2023 at 00:28, Zhang Rui wrote:
>  
> +static bool has_lapic_cpus;

Yet another random flag. Sigh.

I really hate this. Why not doing the obvious?

--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -2452,6 +2452,9 @@ int generic_processor_info(int apicid, i
 	bool boot_cpu_detected = physid_isset(boot_cpu_physical_apicid,
 				phys_cpu_present_map);
 
+	if (physid_isset(apicid, phys_cpu_present_map))
+		return -EBUSY;
+
 	/*
 	 * boot_cpu_physical_apicid is designed to have the apicid
 	 * returned by read_apic_id(), i.e, the apicid of the

As the call sites during MADT parsing ignore the return value anyway,
there is no harm and this is a proper defense against broken tables
which enumerate an APIC twice.

Thanks,

        tglx

Re: [RFC PATCH] x86/acpi: Ignore invalid x2APIC entries

[Thomas Gleixner]

On Fri, Jul 28 2023 at 14:51, Thomas Gleixner wrote:
> On Mon, Jul 03 2023 at 00:28, Zhang Rui wrote:
>>  
>> +static bool has_lapic_cpus;
>
> Yet another random flag. Sigh.
>
> I really hate this. Why not doing the obvious?
>
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -2452,6 +2452,9 @@ int generic_processor_info(int apicid, i
>  	bool boot_cpu_detected = physid_isset(boot_cpu_physical_apicid,
>  				phys_cpu_present_map);
>  
> +	if (physid_isset(apicid, phys_cpu_present_map))
> +		return -EBUSY;
> +
>  	/*
>  	 * boot_cpu_physical_apicid is designed to have the apicid
>  	 * returned by read_apic_id(), i.e, the apicid of the
>
> As the call sites during MADT parsing ignore the return value anyway,
> there is no harm and this is a proper defense against broken tables
> which enumerate an APIC twice.

In fact that function should not have a return value at all, but because
it's not clearly separated between boot time and physical hotplug, it
has to have one ...

Re: [RFC PATCH] x86/acpi: Ignore invalid x2APIC entries

[Zhang, Rui]

On Fri, 2023-07-28 at 14:51 +0200, Thomas Gleixner wrote:
> On Mon, Jul 03 2023 at 00:28, Zhang Rui wrote:
> >  
> > +static bool has_lapic_cpus;
> 
> Yet another random flag. Sigh.
> 
> I really hate this. Why not doing the obvious?
> 
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -2452,6 +2452,9 @@ int generic_processor_info(int apicid, i
>         bool boot_cpu_detected =
> physid_isset(boot_cpu_physical_apicid,
>                                 phys_cpu_present_map);
>  
> +       if (physid_isset(apicid, phys_cpu_present_map))
> +               return -EBUSY;
> +
>         /*
>          * boot_cpu_physical_apicid is designed to have the apicid
>          * returned by read_apic_id(), i.e, the apicid of the
> 
> As the call sites during MADT parsing ignore the return value anyway,
> there is no harm and this is a proper defense against broken tables
> which enumerate an APIC twice.

Yeah, this can fix the duplicate APIC ID issue.

But for x2APIC CPUs with unique APIC ID, but smaller than 255, should
we still enumerate them when we already have valid LAPIC entries?

For the Ivebridge-EP 2-socket system,

LAPIC: APIC ID from 0x0 - 0xB, 0x10 - 0x1B, 0x20 - 0x2B, 0x30 - 0x3B
x2APIC: APIC ID from 0x0 - 0x77

# cpuid -1 -l 0xb -s 1
CPU:
      --- level 1 (core) ---
      bits to shift APIC ID to get next = 0x5 (5)
      logical processors at this level  = 0x18 (24)
      level number                      = 0x1 (1)
      level type                        = core (2)
      extended APIC ID                  = 0

If we still enumerates all the x2APIC entries,
1. we got 72 extra possible CPUs from x2APIC
2. with the patch at https://lore.kernel.org/all/87edm36qqb.ffs@tglx/ ,
_max_logical_packages is set to 4 instead of 2.

this is still a problem, right?

thanks,
rui

Re: [RFC PATCH] x86/acpi: Ignore invalid x2APIC entries

[Thomas Gleixner]

On Fri, Jul 28 2023 at 16:47, Rui Zhang wrote:
> On Fri, 2023-07-28 at 14:51 +0200, Thomas Gleixner wrote:
>> As the call sites during MADT parsing ignore the return value anyway,
>> there is no harm and this is a proper defense against broken tables
>> which enumerate an APIC twice.
>
> Yeah, this can fix the duplicate APIC ID issue.

We want it independent of the below.

> But for x2APIC CPUs with unique APIC ID, but smaller than 255, should
> we still enumerate them when we already have valid LAPIC entries?
>
> For the Ivebridge-EP 2-socket system,
>
> LAPIC: APIC ID from 0x0 - 0xB, 0x10 - 0x1B, 0x20 - 0x2B, 0x30 - 0x3B
> x2APIC: APIC ID from 0x0 - 0x77
>
> # cpuid -1 -l 0xb -s 1
> CPU:
>       --- level 1 (core) ---
>       bits to shift APIC ID to get next = 0x5 (5)
>       logical processors at this level  = 0x18 (24)
>       level number                      = 0x1 (1)
>       level type                        = core (2)
>       extended APIC ID                  = 0
>
> If we still enumerates all the x2APIC entries,
> 1. we got 72 extra possible CPUs from x2APIC
> 2. with the patch at https://lore.kernel.org/all/87edm36qqb.ffs@tglx/ ,
> _max_logical_packages is set to 4 instead of 2.
>
> this is still a problem, right?

Yes, you are right.

But I still don't like the indirection of the returned CPU number. It's
an ACPI selfcontained issue, no?

So something like this should do the trick:

+		count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC,
+					acpi_parse_lapic, MAX_LOCAL_APIC);
+		if (count)
+			has_lapic_cpus = true;
+		x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC,
+					acpi_parse_x2apic, MAX_LOCAL_APIC);
 	}
 	if (!count && !x2count) {
 		pr_err("No LAPIC entries present\n");

Re: [RFC PATCH] x86/acpi: Ignore invalid x2APIC entries

[Zhang, Rui]

On Sat, 2023-07-29 at 09:07 +0200, Thomas Gleixner wrote:
> On Fri, Jul 28 2023 at 16:47, Rui Zhang wrote:
> > On Fri, 2023-07-28 at 14:51 +0200, Thomas Gleixner wrote:
> > > As the call sites during MADT parsing ignore the return value
> > > anyway,
> > > there is no harm and this is a proper defense against broken
> > > tables
> > > which enumerate an APIC twice.
> > 
> > Yeah, this can fix the duplicate APIC ID issue.
> 
> We want it independent of the below.
> 
> > But for x2APIC CPUs with unique APIC ID, but smaller than 255,
> > should
> > we still enumerate them when we already have valid LAPIC entries?
> > 
> > For the Ivebridge-EP 2-socket system,
> > 
> > LAPIC: APIC ID from 0x0 - 0xB, 0x10 - 0x1B, 0x20 - 0x2B, 0x30 -
> > 0x3B
> > x2APIC: APIC ID from 0x0 - 0x77
> > 
> > # cpuid -1 -l 0xb -s 1
> > CPU:
> >       --- level 1 (core) ---
> >       bits to shift APIC ID to get next = 0x5 (5)
> >       logical processors at this level  = 0x18 (24)
> >       level number                      = 0x1 (1)
> >       level type                        = core (2)
> >       extended APIC ID                  = 0
> > 
> > If we still enumerates all the x2APIC entries,
> > 1. we got 72 extra possible CPUs from x2APIC
> > 2. with the patch at
> > https://lore.kernel.org/all/87edm36qqb.ffs@tglx/ ,
> > _max_logical_packages is set to 4 instead of 2.
> > 
> > this is still a problem, right?
> 
> Yes, you are right.
> 
> But I still don't like the indirection of the returned CPU number.
> It's
> an ACPI selfcontained issue, no?
> 
> So something like this should do the trick:
> 
> +               count =
> acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC,
> +                                       acpi_parse_lapic,
> MAX_LOCAL_APIC);
> +               if (count)
> +                       has_lapic_cpus = true;
> +               x2count =
> acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC,
> +                                       acpi_parse_x2apic,
> MAX_LOCAL_APIC);
>         }
>         if (!count && !x2count) {
>                 pr_err("No LAPIC entries present\n");

Agreed, thanks for the advice.
Let me try to do this in v2 patch series.

thanks,
rui

Re: [RFC PATCH] x86/acpi: Ignore invalid x2APIC entries

[Jim Mattson]

> Currently, kernel enumerates the possible CPUs by parsing both ACPI MADT
> Local APIC entries and x2APIC entries. So CPUs with "valid" APIC IDs,
> even if they have duplicated APIC IDs in Local APIC and x2APIC, are
> always enumerated.
> 
> Below is what ACPI MADT Local APIC and x2APIC describes on an
> Ivebridge-EP system,
> 
> [02Ch 0044   1]                Subtable Type : 00 [Processor Local APIC]
> [02Fh 0047   1]                Local Apic ID : 00
> ...
> [164h 0356   1]                Subtable Type : 00 [Processor Local APIC]
> [167h 0359   1]                Local Apic ID : 39
> [16Ch 0364   1]                Subtable Type : 00 [Processor Local APIC]
> [16Fh 0367   1]                Local Apic ID : FF
> ...
> [3ECh 1004   1]                Subtable Type : 09 [Processor Local x2APIC]
> [3F0h 1008   4]                Processor x2Apic ID : 00000000
> ...
> [B5Ch 2908   1]                Subtable Type : 09 [Processor Local x2APIC]
> [B60h 2912   4]                Processor x2Apic ID : 00000077
> 
> As a result, kernel shows "smpboot: Allowing 168 CPUs, 120 hotplug CPUs".
> And this wastes significant amount of memory for the per-cpu data.
> Plus this also breaks https://lore.kernel.org/all/87edm36qqb.ffs@tglx/,
> because __max_logical_packages is over-estimated by the APIC IDs in
> the x2APIC entries.
> 
> According to https://uefi.org/specs/ACPI/6.5/05_ACPI_Software_Programming_Model.html#processor-local-x2apic-structure,
> "[Compatibility note] On some legacy OSes, Logical processors with APIC
> ID values less than 255 (whether in XAPIC or X2APIC mode) must use the
> Processor Local APIC structure to convey their APIC information to OSPM,
> and those processors must be declared in the DSDT using the Processor()
> keyword. Logical processors with APIC ID values 255 and greater must use
> the Processor Local x2APIC structure and be declared using the Device()
> keyword.".
> 
> Enumerate CPUs from x2APIC enties with APIC ID values 255 or greater,
> when valid CPU from Local APIC is already detected.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
> I didn't find any clear statement in the ACPI spec about if a mixture of
> Local APIC and x2APIC entries is allowed or not. So it would be great if
> this can be clarified.

Has this been clarified?

The reason that I ask is that Google Cloud has a 360 vCPU Zen4 VM
occupying two virtual sockets, and the corresponding MADT table has a
mixture of Local APIC and X2APIC entries.

All of the LPUs in virtual socket 0 have extended APIC IDs below 255,
and they have Local APIC entries. All of the LPUs in virtual socket 1
have extended APIC IDs above 255, and they have X2APIC entries.

Prior to this change, Linux assigned CPU numbers to all even-numbered
LPUs on virtual socket 0, followed by all even-numbered LPUs on
virtual socket 1, followed by all odd-numbered LPUs on virtual socket
0, followed by all odd-numbered LPUs on virtual socket 1.

node  #0, CPUs:          #1   #2  ...   #87  #88  #89
node  #1, CPUs:    #90  #91  #92  ...  #177 #178 #179
node  #0, CPUs:   #180 #181 #182  ...  #267 #268 #269
node  #1, CPUs:   #270 #271 #272  ...  #357 #358 #359

After this change, however, Linux assigns CPU numbers to all LPUs on
virtual socket 0 before assigning any CPU numbers to LPUs on virtual
socket 1.

node  #0, CPUs:          #1   #2  ...   #87  #88  #89
node  #1, CPUs:   #180 #181 #182  ...  #267 #268 #269
node  #0, CPUs:    #90  #91  #92  ...  #177 #178 #179
node  #1, CPUs:   #270 #271 #272  ...  #357 #358 #359

I suspect that this is because all Local APIC MADT entries are now
processed before all X2APIC MADT entries, whereas they may have been
interleaved before.

TBH, I'm not sure that there is actually anything wrong with the new
numbering scheme. The topology is reported correctly (e.g. in
/sys/devices/system/cpu/cpu0/topology/thread_siblings_list). Yet, the
new enumeration does seem to contradict user expectations.

Thanks,

--jim

Re: [RFC PATCH] x86/acpi: Ignore invalid x2APIC entries

[Zhang, Rui]

On Thu, 2024-10-10 at 14:31 -0700, Jim Mattson wrote:
> > Currently, kernel enumerates the possible CPUs by parsing both ACPI
> > MADT
> > Local APIC entries and x2APIC entries. So CPUs with "valid" APIC
> > IDs,
> > even if they have duplicated APIC IDs in Local APIC and x2APIC, are
> > always enumerated.
> > 
> > Below is what ACPI MADT Local APIC and x2APIC describes on an
> > Ivebridge-EP system,
> > 
> > [02Ch 0044   1]                Subtable Type : 00 [Processor Local
> > APIC]
> > [02Fh 0047   1]                Local Apic ID : 00
> > ...
> > [164h 0356   1]                Subtable Type : 00 [Processor Local
> > APIC]
> > [167h 0359   1]                Local Apic ID : 39
> > [16Ch 0364   1]                Subtable Type : 00 [Processor Local
> > APIC]
> > [16Fh 0367   1]                Local Apic ID : FF
> > ...
> > [3ECh 1004   1]                Subtable Type : 09 [Processor Local
> > x2APIC]
> > [3F0h 1008   4]                Processor x2Apic ID : 00000000
> > ...
> > [B5Ch 2908   1]                Subtable Type : 09 [Processor Local
> > x2APIC]
> > [B60h 2912   4]                Processor x2Apic ID : 00000077
> > 
> > As a result, kernel shows "smpboot: Allowing 168 CPUs, 120 hotplug
> > CPUs".
> > And this wastes significant amount of memory for the per-cpu data.
> > Plus this also breaks
> > https://lore.kernel.org/all/87edm36qqb.ffs@tglx/,
> > because __max_logical_packages is over-estimated by the APIC IDs in
> > the x2APIC entries.
> > 
> > According to
> > https://uefi.org/specs/ACPI/6.5/05_ACPI_Software_Programming_Model.html#processor-local-x2apic-structure
> > ,
> > "[Compatibility note] On some legacy OSes, Logical processors with
> > APIC
> > ID values less than 255 (whether in XAPIC or X2APIC mode) must use
> > the
> > Processor Local APIC structure to convey their APIC information to
> > OSPM,
> > and those processors must be declared in the DSDT using the
> > Processor()
> > keyword. Logical processors with APIC ID values 255 and greater
> > must use
> > the Processor Local x2APIC structure and be declared using the
> > Device()
> > keyword.".
> > 
> > Enumerate CPUs from x2APIC enties with APIC ID values 255 or
> > greater,
> > when valid CPU from Local APIC is already detected.
> > 
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> > I didn't find any clear statement in the ACPI spec about if a
> > mixture of
> > Local APIC and x2APIC entries is allowed or not. So it would be
> > great if
> > this can be clarified.
> 
> Has this been clarified?
> 
> The reason that I ask is that Google Cloud has a 360 vCPU Zen4 VM
> occupying two virtual sockets, and the corresponding MADT table has a
> mixture of Local APIC and X2APIC entries.
> 
> All of the LPUs in virtual socket 0 have extended APIC IDs below 255,
> and they have Local APIC entries. All of the LPUs in virtual socket 1
> have extended APIC IDs above 255, and they have X2APIC entries.
> 
> Prior to this change, Linux assigned CPU numbers to all even-numbered
> LPUs on virtual socket 0, followed by all even-numbered LPUs on
> virtual socket 1, followed by all odd-numbered LPUs on virtual socket
> 0, followed by all odd-numbered LPUs on virtual socket 1.
> 
> node  #0, CPUs:          #1   #2  ...   #87  #88  #89
> node  #1, CPUs:    #90  #91  #92  ...  #177 #178 #179
> node  #0, CPUs:   #180 #181 #182  ...  #267 #268 #269
> node  #1, CPUs:   #270 #271 #272  ...  #357 #358 #359
> 
> After this change, however, Linux assigns CPU numbers to all LPUs on
> virtual socket 0 before assigning any CPU numbers to LPUs on virtual
> socket 1.
> 
> node  #0, CPUs:          #1   #2  ...   #87  #88  #89
> node  #1, CPUs:   #180 #181 #182  ...  #267 #268 #269
> node  #0, CPUs:    #90  #91  #92  ...  #177 #178 #179
> node  #1, CPUs:   #270 #271 #272  ...  #357 #358 #359
> 
> I suspect that this is because all Local APIC MADT entries are now
> processed before all X2APIC MADT entries, whereas they may have been
> interleaved before.

agreed.
can you attach the acpidump to confirm this?

> 
> TBH, I'm not sure that there is actually anything wrong with the new
> numbering scheme.
>  The topology is reported correctly (e.g. in
> /sys/devices/system/cpu/cpu0/topology/thread_siblings_list). Yet, the
> new enumeration does seem to contradict user expectations.
> 

Well, we can say this is a violation of the ACPI spec.
"OSPM should initialize processors in the order that they appear in the
MADT." even for interleaved LAPIC and X2APIC entries.

Maybe we need two steps for LAPIC/X2APIC parsing.
1. check if there is valid LAPIC entry by going through all LAPIC
entries first
2. parse LAPIC/X2APIC strictly following the order in MADT. (like we do
before)

thanks,
rui

Re: [RFC PATCH] x86/acpi: Ignore invalid x2APIC entries

[Jim Mattson]

On Thu, Oct 10, 2024 at 6:37 PM Zhang, Rui <rui.zhang@intel.com> wrote:
>
> On Thu, 2024-10-10 at 14:31 -0700, Jim Mattson wrote:
> > > Currently, kernel enumerates the possible CPUs by parsing both ACPI
> > > MADT
> > > Local APIC entries and x2APIC entries. So CPUs with "valid" APIC
> > > IDs,
> > > even if they have duplicated APIC IDs in Local APIC and x2APIC, are
> > > always enumerated.
> > >
> > > Below is what ACPI MADT Local APIC and x2APIC describes on an
> > > Ivebridge-EP system,
> > >
> > > [02Ch 0044   1]                Subtable Type : 00 [Processor Local
> > > APIC]
> > > [02Fh 0047   1]                Local Apic ID : 00
> > > ...
> > > [164h 0356   1]                Subtable Type : 00 [Processor Local
> > > APIC]
> > > [167h 0359   1]                Local Apic ID : 39
> > > [16Ch 0364   1]                Subtable Type : 00 [Processor Local
> > > APIC]
> > > [16Fh 0367   1]                Local Apic ID : FF
> > > ...
> > > [3ECh 1004   1]                Subtable Type : 09 [Processor Local
> > > x2APIC]
> > > [3F0h 1008   4]                Processor x2Apic ID : 00000000
> > > ...
> > > [B5Ch 2908   1]                Subtable Type : 09 [Processor Local
> > > x2APIC]
> > > [B60h 2912   4]                Processor x2Apic ID : 00000077
> > >
> > > As a result, kernel shows "smpboot: Allowing 168 CPUs, 120 hotplug
> > > CPUs".
> > > And this wastes significant amount of memory for the per-cpu data.
> > > Plus this also breaks
> > > https://lore.kernel.org/all/87edm36qqb.ffs@tglx/,
> > > because __max_logical_packages is over-estimated by the APIC IDs in
> > > the x2APIC entries.
> > >
> > > According to
> > > https://uefi.org/specs/ACPI/6.5/05_ACPI_Software_Programming_Model.html#processor-local-x2apic-structure
> > > ,
> > > "[Compatibility note] On some legacy OSes, Logical processors with
> > > APIC
> > > ID values less than 255 (whether in XAPIC or X2APIC mode) must use
> > > the
> > > Processor Local APIC structure to convey their APIC information to
> > > OSPM,
> > > and those processors must be declared in the DSDT using the
> > > Processor()
> > > keyword. Logical processors with APIC ID values 255 and greater
> > > must use
> > > the Processor Local x2APIC structure and be declared using the
> > > Device()
> > > keyword.".
> > >
> > > Enumerate CPUs from x2APIC enties with APIC ID values 255 or
> > > greater,
> > > when valid CPU from Local APIC is already detected.
> > >
> > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > > ---
> > > I didn't find any clear statement in the ACPI spec about if a
> > > mixture of
> > > Local APIC and x2APIC entries is allowed or not. So it would be
> > > great if
> > > this can be clarified.
> >
> > Has this been clarified?
> >
> > The reason that I ask is that Google Cloud has a 360 vCPU Zen4 VM
> > occupying two virtual sockets, and the corresponding MADT table has a
> > mixture of Local APIC and X2APIC entries.
> >
> > All of the LPUs in virtual socket 0 have extended APIC IDs below 255,
> > and they have Local APIC entries. All of the LPUs in virtual socket 1
> > have extended APIC IDs above 255, and they have X2APIC entries.
> >
> > Prior to this change, Linux assigned CPU numbers to all even-numbered
> > LPUs on virtual socket 0, followed by all even-numbered LPUs on
> > virtual socket 1, followed by all odd-numbered LPUs on virtual socket
> > 0, followed by all odd-numbered LPUs on virtual socket 1.
> >
> > node  #0, CPUs:          #1   #2  ...   #87  #88  #89
> > node  #1, CPUs:    #90  #91  #92  ...  #177 #178 #179
> > node  #0, CPUs:   #180 #181 #182  ...  #267 #268 #269
> > node  #1, CPUs:   #270 #271 #272  ...  #357 #358 #359
> >
> > After this change, however, Linux assigns CPU numbers to all LPUs on
> > virtual socket 0 before assigning any CPU numbers to LPUs on virtual
> > socket 1.
> >
> > node  #0, CPUs:          #1   #2  ...   #87  #88  #89
> > node  #1, CPUs:   #180 #181 #182  ...  #267 #268 #269
> > node  #0, CPUs:    #90  #91  #92  ...  #177 #178 #179
> > node  #1, CPUs:   #270 #271 #272  ...  #357 #358 #359
> >
> > I suspect that this is because all Local APIC MADT entries are now
> > processed before all X2APIC MADT entries, whereas they may have been
> > interleaved before.
>
> agreed.
> can you attach the acpidump to confirm this?

I'm pretty sure LKML rejects attachments. Here's the parsed MADT:

ACPI: APIC (v005 Google GOOGAPIC 0x00000001 GOOG 0x00000001) @ 0x(nil)
ACPI: LAPIC (acpi_id[0x00] lapic_id[0x00] enabled)
ACPI: LAPIC (acpi_id[0x02] lapic_id[0x02] enabled)
ACPI: LAPIC (acpi_id[0x04] lapic_id[0x04] enabled)
ACPI: LAPIC (acpi_id[0x06] lapic_id[0x06] enabled)
ACPI: LAPIC (acpi_id[0x08] lapic_id[0x08] enabled)
ACPI: LAPIC (acpi_id[0x0a] lapic_id[0x0a] enabled)
ACPI: LAPIC (acpi_id[0x0c] lapic_id[0x0c] enabled)
ACPI: LAPIC (acpi_id[0x0e] lapic_id[0x0e] enabled)
ACPI: LAPIC (acpi_id[0x10] lapic_id[0x10] enabled)
ACPI: LAPIC (acpi_id[0x12] lapic_id[0x12] enabled)
ACPI: LAPIC (acpi_id[0x14] lapic_id[0x14] enabled)
ACPI: LAPIC (acpi_id[0x16] lapic_id[0x16] enabled)
ACPI: LAPIC (acpi_id[0x18] lapic_id[0x18] enabled)
ACPI: LAPIC (acpi_id[0x1a] lapic_id[0x1a] enabled)
ACPI: LAPIC (acpi_id[0x1c] lapic_id[0x1c] enabled)
ACPI: LAPIC (acpi_id[0x20] lapic_id[0x20] enabled)
ACPI: LAPIC (acpi_id[0x22] lapic_id[0x22] enabled)
ACPI: LAPIC (acpi_id[0x24] lapic_id[0x24] enabled)
ACPI: LAPIC (acpi_id[0x26] lapic_id[0x26] enabled)
ACPI: LAPIC (acpi_id[0x28] lapic_id[0x28] enabled)
ACPI: LAPIC (acpi_id[0x2a] lapic_id[0x2a] enabled)
ACPI: LAPIC (acpi_id[0x2c] lapic_id[0x2c] enabled)
ACPI: LAPIC (acpi_id[0x2e] lapic_id[0x2e] enabled)
ACPI: LAPIC (acpi_id[0x30] lapic_id[0x30] enabled)
ACPI: LAPIC (acpi_id[0x32] lapic_id[0x32] enabled)
ACPI: LAPIC (acpi_id[0x34] lapic_id[0x34] enabled)
ACPI: LAPIC (acpi_id[0x36] lapic_id[0x36] enabled)
ACPI: LAPIC (acpi_id[0x38] lapic_id[0x38] enabled)
ACPI: LAPIC (acpi_id[0x3a] lapic_id[0x3a] enabled)
ACPI: LAPIC (acpi_id[0x3c] lapic_id[0x3c] enabled)
ACPI: LAPIC (acpi_id[0x40] lapic_id[0x40] enabled)
ACPI: LAPIC (acpi_id[0x42] lapic_id[0x42] enabled)
ACPI: LAPIC (acpi_id[0x44] lapic_id[0x44] enabled)
ACPI: LAPIC (acpi_id[0x46] lapic_id[0x46] enabled)
ACPI: LAPIC (acpi_id[0x48] lapic_id[0x48] enabled)
ACPI: LAPIC (acpi_id[0x4a] lapic_id[0x4a] enabled)
ACPI: LAPIC (acpi_id[0x4c] lapic_id[0x4c] enabled)
ACPI: LAPIC (acpi_id[0x4e] lapic_id[0x4e] enabled)
ACPI: LAPIC (acpi_id[0x50] lapic_id[0x50] enabled)
ACPI: LAPIC (acpi_id[0x52] lapic_id[0x52] enabled)
ACPI: LAPIC (acpi_id[0x54] lapic_id[0x54] enabled)
ACPI: LAPIC (acpi_id[0x56] lapic_id[0x56] enabled)
ACPI: LAPIC (acpi_id[0x58] lapic_id[0x58] enabled)
ACPI: LAPIC (acpi_id[0x5a] lapic_id[0x5a] enabled)
ACPI: LAPIC (acpi_id[0x5c] lapic_id[0x5c] enabled)
ACPI: LAPIC (acpi_id[0x60] lapic_id[0x60] enabled)
ACPI: LAPIC (acpi_id[0x62] lapic_id[0x62] enabled)
ACPI: LAPIC (acpi_id[0x64] lapic_id[0x64] enabled)
ACPI: LAPIC (acpi_id[0x66] lapic_id[0x66] enabled)
ACPI: LAPIC (acpi_id[0x68] lapic_id[0x68] enabled)
ACPI: LAPIC (acpi_id[0x6a] lapic_id[0x6a] enabled)
ACPI: LAPIC (acpi_id[0x6c] lapic_id[0x6c] enabled)
ACPI: LAPIC (acpi_id[0x6e] lapic_id[0x6e] enabled)
ACPI: LAPIC (acpi_id[0x70] lapic_id[0x70] enabled)
ACPI: LAPIC (acpi_id[0x72] lapic_id[0x72] enabled)
ACPI: LAPIC (acpi_id[0x74] lapic_id[0x74] enabled)
ACPI: LAPIC (acpi_id[0x76] lapic_id[0x76] enabled)
ACPI: LAPIC (acpi_id[0x78] lapic_id[0x78] enabled)
ACPI: LAPIC (acpi_id[0x7a] lapic_id[0x7a] enabled)
ACPI: LAPIC (acpi_id[0x7c] lapic_id[0x7c] enabled)
ACPI: LAPIC (acpi_id[0x80] lapic_id[0x80] enabled)
ACPI: LAPIC (acpi_id[0x82] lapic_id[0x82] enabled)
ACPI: LAPIC (acpi_id[0x84] lapic_id[0x84] enabled)
ACPI: LAPIC (acpi_id[0x86] lapic_id[0x86] enabled)
ACPI: LAPIC (acpi_id[0x88] lapic_id[0x88] enabled)
ACPI: LAPIC (acpi_id[0x8a] lapic_id[0x8a] enabled)
ACPI: LAPIC (acpi_id[0x8c] lapic_id[0x8c] enabled)
ACPI: LAPIC (acpi_id[0x8e] lapic_id[0x8e] enabled)
ACPI: LAPIC (acpi_id[0x90] lapic_id[0x90] enabled)
ACPI: LAPIC (acpi_id[0x92] lapic_id[0x92] enabled)
ACPI: LAPIC (acpi_id[0x94] lapic_id[0x94] enabled)
ACPI: LAPIC (acpi_id[0x96] lapic_id[0x96] enabled)
ACPI: LAPIC (acpi_id[0x98] lapic_id[0x98] enabled)
ACPI: LAPIC (acpi_id[0x9a] lapic_id[0x9a] enabled)
ACPI: LAPIC (acpi_id[0x9c] lapic_id[0x9c] enabled)
ACPI: LAPIC (acpi_id[0xa0] lapic_id[0xa0] enabled)
ACPI: LAPIC (acpi_id[0xa2] lapic_id[0xa2] enabled)
ACPI: LAPIC (acpi_id[0xa4] lapic_id[0xa4] enabled)
ACPI: LAPIC (acpi_id[0xa6] lapic_id[0xa6] enabled)
ACPI: LAPIC (acpi_id[0xa8] lapic_id[0xa8] enabled)
ACPI: LAPIC (acpi_id[0xaa] lapic_id[0xaa] enabled)
ACPI: LAPIC (acpi_id[0xac] lapic_id[0xac] enabled)
ACPI: LAPIC (acpi_id[0xae] lapic_id[0xae] enabled)
ACPI: LAPIC (acpi_id[0xb0] lapic_id[0xb0] enabled)
ACPI: LAPIC (acpi_id[0xb2] lapic_id[0xb2] enabled)
ACPI: LAPIC (acpi_id[0xb4] lapic_id[0xb4] enabled)
ACPI: LAPIC (acpi_id[0xb6] lapic_id[0xb6] enabled)
ACPI: LAPIC (acpi_id[0xb8] lapic_id[0xb8] enabled)
ACPI: LAPIC (acpi_id[0xba] lapic_id[0xba] enabled)
ACPI: LAPIC (acpi_id[0xbc] lapic_id[0xbc] enabled)
ACPI: X2APIC (apic_id[0x100] uid[0x100] enabled)
ACPI: X2APIC (apic_id[0x102] uid[0x102] enabled)
ACPI: X2APIC (apic_id[0x104] uid[0x104] enabled)
ACPI: X2APIC (apic_id[0x106] uid[0x106] enabled)
ACPI: X2APIC (apic_id[0x108] uid[0x108] enabled)
ACPI: X2APIC (apic_id[0x10a] uid[0x10a] enabled)
ACPI: X2APIC (apic_id[0x10c] uid[0x10c] enabled)
ACPI: X2APIC (apic_id[0x10e] uid[0x10e] enabled)
ACPI: X2APIC (apic_id[0x110] uid[0x110] enabled)
ACPI: X2APIC (apic_id[0x112] uid[0x112] enabled)
ACPI: X2APIC (apic_id[0x114] uid[0x114] enabled)
ACPI: X2APIC (apic_id[0x116] uid[0x116] enabled)
ACPI: X2APIC (apic_id[0x118] uid[0x118] enabled)
ACPI: X2APIC (apic_id[0x11a] uid[0x11a] enabled)
ACPI: X2APIC (apic_id[0x11c] uid[0x11c] enabled)
ACPI: X2APIC (apic_id[0x120] uid[0x120] enabled)
ACPI: X2APIC (apic_id[0x122] uid[0x122] enabled)
ACPI: X2APIC (apic_id[0x124] uid[0x124] enabled)
ACPI: X2APIC (apic_id[0x126] uid[0x126] enabled)
ACPI: X2APIC (apic_id[0x128] uid[0x128] enabled)
ACPI: X2APIC (apic_id[0x12a] uid[0x12a] enabled)
ACPI: X2APIC (apic_id[0x12c] uid[0x12c] enabled)
ACPI: X2APIC (apic_id[0x12e] uid[0x12e] enabled)
ACPI: X2APIC (apic_id[0x130] uid[0x130] enabled)
ACPI: X2APIC (apic_id[0x132] uid[0x132] enabled)
ACPI: X2APIC (apic_id[0x134] uid[0x134] enabled)
ACPI: X2APIC (apic_id[0x136] uid[0x136] enabled)
ACPI: X2APIC (apic_id[0x138] uid[0x138] enabled)
ACPI: X2APIC (apic_id[0x13a] uid[0x13a] enabled)
ACPI: X2APIC (apic_id[0x13c] uid[0x13c] enabled)
ACPI: X2APIC (apic_id[0x140] uid[0x140] enabled)
ACPI: X2APIC (apic_id[0x142] uid[0x142] enabled)
ACPI: X2APIC (apic_id[0x144] uid[0x144] enabled)
ACPI: X2APIC (apic_id[0x146] uid[0x146] enabled)
ACPI: X2APIC (apic_id[0x148] uid[0x148] enabled)
ACPI: X2APIC (apic_id[0x14a] uid[0x14a] enabled)
ACPI: X2APIC (apic_id[0x14c] uid[0x14c] enabled)
ACPI: X2APIC (apic_id[0x14e] uid[0x14e] enabled)
ACPI: X2APIC (apic_id[0x150] uid[0x150] enabled)
ACPI: X2APIC (apic_id[0x152] uid[0x152] enabled)
ACPI: X2APIC (apic_id[0x154] uid[0x154] enabled)
ACPI: X2APIC (apic_id[0x156] uid[0x156] enabled)
ACPI: X2APIC (apic_id[0x158] uid[0x158] enabled)
ACPI: X2APIC (apic_id[0x15a] uid[0x15a] enabled)
ACPI: X2APIC (apic_id[0x15c] uid[0x15c] enabled)
ACPI: X2APIC (apic_id[0x160] uid[0x160] enabled)
ACPI: X2APIC (apic_id[0x162] uid[0x162] enabled)
ACPI: X2APIC (apic_id[0x164] uid[0x164] enabled)
ACPI: X2APIC (apic_id[0x166] uid[0x166] enabled)
ACPI: X2APIC (apic_id[0x168] uid[0x168] enabled)
ACPI: X2APIC (apic_id[0x16a] uid[0x16a] enabled)
ACPI: X2APIC (apic_id[0x16c] uid[0x16c] enabled)
ACPI: X2APIC (apic_id[0x16e] uid[0x16e] enabled)
ACPI: X2APIC (apic_id[0x170] uid[0x170] enabled)
ACPI: X2APIC (apic_id[0x172] uid[0x172] enabled)
ACPI: X2APIC (apic_id[0x174] uid[0x174] enabled)
ACPI: X2APIC (apic_id[0x176] uid[0x176] enabled)
ACPI: X2APIC (apic_id[0x178] uid[0x178] enabled)
ACPI: X2APIC (apic_id[0x17a] uid[0x17a] enabled)
ACPI: X2APIC (apic_id[0x17c] uid[0x17c] enabled)
ACPI: X2APIC (apic_id[0x180] uid[0x180] enabled)
ACPI: X2APIC (apic_id[0x182] uid[0x182] enabled)
ACPI: X2APIC (apic_id[0x184] uid[0x184] enabled)
ACPI: X2APIC (apic_id[0x186] uid[0x186] enabled)
ACPI: X2APIC (apic_id[0x188] uid[0x188] enabled)
ACPI: X2APIC (apic_id[0x18a] uid[0x18a] enabled)
ACPI: X2APIC (apic_id[0x18c] uid[0x18c] enabled)
ACPI: X2APIC (apic_id[0x18e] uid[0x18e] enabled)
ACPI: X2APIC (apic_id[0x190] uid[0x190] enabled)
ACPI: X2APIC (apic_id[0x192] uid[0x192] enabled)
ACPI: X2APIC (apic_id[0x194] uid[0x194] enabled)
ACPI: X2APIC (apic_id[0x196] uid[0x196] enabled)
ACPI: X2APIC (apic_id[0x198] uid[0x198] enabled)
ACPI: X2APIC (apic_id[0x19a] uid[0x19a] enabled)
ACPI: X2APIC (apic_id[0x19c] uid[0x19c] enabled)
ACPI: X2APIC (apic_id[0x1a0] uid[0x1a0] enabled)
ACPI: X2APIC (apic_id[0x1a2] uid[0x1a2] enabled)
ACPI: X2APIC (apic_id[0x1a4] uid[0x1a4] enabled)
ACPI: X2APIC (apic_id[0x1a6] uid[0x1a6] enabled)
ACPI: X2APIC (apic_id[0x1a8] uid[0x1a8] enabled)
ACPI: X2APIC (apic_id[0x1aa] uid[0x1aa] enabled)
ACPI: X2APIC (apic_id[0x1ac] uid[0x1ac] enabled)
ACPI: X2APIC (apic_id[0x1ae] uid[0x1ae] enabled)
ACPI: X2APIC (apic_id[0x1b0] uid[0x1b0] enabled)
ACPI: X2APIC (apic_id[0x1b2] uid[0x1b2] enabled)
ACPI: X2APIC (apic_id[0x1b4] uid[0x1b4] enabled)
ACPI: X2APIC (apic_id[0x1b6] uid[0x1b6] enabled)
ACPI: X2APIC (apic_id[0x1b8] uid[0x1b8] enabled)
ACPI: X2APIC (apic_id[0x1ba] uid[0x1ba] enabled)
ACPI: X2APIC (apic_id[0x1bc] uid[0x1bc] enabled)
ACPI: LAPIC (acpi_id[0x01] lapic_id[0x01] enabled)
ACPI: LAPIC (acpi_id[0x03] lapic_id[0x03] enabled)
ACPI: LAPIC (acpi_id[0x05] lapic_id[0x05] enabled)
ACPI: LAPIC (acpi_id[0x07] lapic_id[0x07] enabled)
ACPI: LAPIC (acpi_id[0x09] lapic_id[0x09] enabled)
ACPI: LAPIC (acpi_id[0x0b] lapic_id[0x0b] enabled)
ACPI: LAPIC (acpi_id[0x0d] lapic_id[0x0d] enabled)
ACPI: LAPIC (acpi_id[0x0f] lapic_id[0x0f] enabled)
ACPI: LAPIC (acpi_id[0x11] lapic_id[0x11] enabled)
ACPI: LAPIC (acpi_id[0x13] lapic_id[0x13] enabled)
ACPI: LAPIC (acpi_id[0x15] lapic_id[0x15] enabled)
ACPI: LAPIC (acpi_id[0x17] lapic_id[0x17] enabled)
ACPI: LAPIC (acpi_id[0x19] lapic_id[0x19] enabled)
ACPI: LAPIC (acpi_id[0x1b] lapic_id[0x1b] enabled)
ACPI: LAPIC (acpi_id[0x1d] lapic_id[0x1d] enabled)
ACPI: LAPIC (acpi_id[0x21] lapic_id[0x21] enabled)
ACPI: LAPIC (acpi_id[0x23] lapic_id[0x23] enabled)
ACPI: LAPIC (acpi_id[0x25] lapic_id[0x25] enabled)
ACPI: LAPIC (acpi_id[0x27] lapic_id[0x27] enabled)
ACPI: LAPIC (acpi_id[0x29] lapic_id[0x29] enabled)
ACPI: LAPIC (acpi_id[0x2b] lapic_id[0x2b] enabled)
ACPI: LAPIC (acpi_id[0x2d] lapic_id[0x2d] enabled)
ACPI: LAPIC (acpi_id[0x2f] lapic_id[0x2f] enabled)
ACPI: LAPIC (acpi_id[0x31] lapic_id[0x31] enabled)
ACPI: LAPIC (acpi_id[0x33] lapic_id[0x33] enabled)
ACPI: LAPIC (acpi_id[0x35] lapic_id[0x35] enabled)
ACPI: LAPIC (acpi_id[0x37] lapic_id[0x37] enabled)
ACPI: LAPIC (acpi_id[0x39] lapic_id[0x39] enabled)
ACPI: LAPIC (acpi_id[0x3b] lapic_id[0x3b] enabled)
ACPI: LAPIC (acpi_id[0x3d] lapic_id[0x3d] enabled)
ACPI: LAPIC (acpi_id[0x41] lapic_id[0x41] enabled)
ACPI: LAPIC (acpi_id[0x43] lapic_id[0x43] enabled)
ACPI: LAPIC (acpi_id[0x45] lapic_id[0x45] enabled)
ACPI: LAPIC (acpi_id[0x47] lapic_id[0x47] enabled)
ACPI: LAPIC (acpi_id[0x49] lapic_id[0x49] enabled)
ACPI: LAPIC (acpi_id[0x4b] lapic_id[0x4b] enabled)
ACPI: LAPIC (acpi_id[0x4d] lapic_id[0x4d] enabled)
ACPI: LAPIC (acpi_id[0x4f] lapic_id[0x4f] enabled)
ACPI: LAPIC (acpi_id[0x51] lapic_id[0x51] enabled)
ACPI: LAPIC (acpi_id[0x53] lapic_id[0x53] enabled)
ACPI: LAPIC (acpi_id[0x55] lapic_id[0x55] enabled)
ACPI: LAPIC (acpi_id[0x57] lapic_id[0x57] enabled)
ACPI: LAPIC (acpi_id[0x59] lapic_id[0x59] enabled)
ACPI: LAPIC (acpi_id[0x5b] lapic_id[0x5b] enabled)
ACPI: LAPIC (acpi_id[0x5d] lapic_id[0x5d] enabled)
ACPI: LAPIC (acpi_id[0x61] lapic_id[0x61] enabled)
ACPI: LAPIC (acpi_id[0x63] lapic_id[0x63] enabled)
ACPI: LAPIC (acpi_id[0x65] lapic_id[0x65] enabled)
ACPI: LAPIC (acpi_id[0x67] lapic_id[0x67] enabled)
ACPI: LAPIC (acpi_id[0x69] lapic_id[0x69] enabled)
ACPI: LAPIC (acpi_id[0x6b] lapic_id[0x6b] enabled)
ACPI: LAPIC (acpi_id[0x6d] lapic_id[0x6d] enabled)
ACPI: LAPIC (acpi_id[0x6f] lapic_id[0x6f] enabled)
ACPI: LAPIC (acpi_id[0x71] lapic_id[0x71] enabled)
ACPI: LAPIC (acpi_id[0x73] lapic_id[0x73] enabled)
ACPI: LAPIC (acpi_id[0x75] lapic_id[0x75] enabled)
ACPI: LAPIC (acpi_id[0x77] lapic_id[0x77] enabled)
ACPI: LAPIC (acpi_id[0x79] lapic_id[0x79] enabled)
ACPI: LAPIC (acpi_id[0x7b] lapic_id[0x7b] enabled)
ACPI: LAPIC (acpi_id[0x7d] lapic_id[0x7d] enabled)
ACPI: LAPIC (acpi_id[0x81] lapic_id[0x81] enabled)
ACPI: LAPIC (acpi_id[0x83] lapic_id[0x83] enabled)
ACPI: LAPIC (acpi_id[0x85] lapic_id[0x85] enabled)
ACPI: LAPIC (acpi_id[0x87] lapic_id[0x87] enabled)
ACPI: LAPIC (acpi_id[0x89] lapic_id[0x89] enabled)
ACPI: LAPIC (acpi_id[0x8b] lapic_id[0x8b] enabled)
ACPI: LAPIC (acpi_id[0x8d] lapic_id[0x8d] enabled)
ACPI: LAPIC (acpi_id[0x8f] lapic_id[0x8f] enabled)
ACPI: LAPIC (acpi_id[0x91] lapic_id[0x91] enabled)
ACPI: LAPIC (acpi_id[0x93] lapic_id[0x93] enabled)
ACPI: LAPIC (acpi_id[0x95] lapic_id[0x95] enabled)
ACPI: LAPIC (acpi_id[0x97] lapic_id[0x97] enabled)
ACPI: LAPIC (acpi_id[0x99] lapic_id[0x99] enabled)
ACPI: LAPIC (acpi_id[0x9b] lapic_id[0x9b] enabled)
ACPI: LAPIC (acpi_id[0x9d] lapic_id[0x9d] enabled)
ACPI: LAPIC (acpi_id[0xa1] lapic_id[0xa1] enabled)
ACPI: LAPIC (acpi_id[0xa3] lapic_id[0xa3] enabled)
ACPI: LAPIC (acpi_id[0xa5] lapic_id[0xa5] enabled)
ACPI: LAPIC (acpi_id[0xa7] lapic_id[0xa7] enabled)
ACPI: LAPIC (acpi_id[0xa9] lapic_id[0xa9] enabled)
ACPI: LAPIC (acpi_id[0xab] lapic_id[0xab] enabled)
ACPI: LAPIC (acpi_id[0xad] lapic_id[0xad] enabled)
ACPI: LAPIC (acpi_id[0xaf] lapic_id[0xaf] enabled)
ACPI: LAPIC (acpi_id[0xb1] lapic_id[0xb1] enabled)
ACPI: LAPIC (acpi_id[0xb3] lapic_id[0xb3] enabled)
ACPI: LAPIC (acpi_id[0xb5] lapic_id[0xb5] enabled)
ACPI: LAPIC (acpi_id[0xb7] lapic_id[0xb7] enabled)
ACPI: LAPIC (acpi_id[0xb9] lapic_id[0xb9] enabled)
ACPI: LAPIC (acpi_id[0xbb] lapic_id[0xbb] enabled)
ACPI: LAPIC (acpi_id[0xbd] lapic_id[0xbd] enabled)
ACPI: X2APIC (apic_id[0x101] uid[0x101] enabled)
ACPI: X2APIC (apic_id[0x103] uid[0x103] enabled)
ACPI: X2APIC (apic_id[0x105] uid[0x105] enabled)
ACPI: X2APIC (apic_id[0x107] uid[0x107] enabled)
ACPI: X2APIC (apic_id[0x109] uid[0x109] enabled)
ACPI: X2APIC (apic_id[0x10b] uid[0x10b] enabled)
ACPI: X2APIC (apic_id[0x10d] uid[0x10d] enabled)
ACPI: X2APIC (apic_id[0x10f] uid[0x10f] enabled)
ACPI: X2APIC (apic_id[0x111] uid[0x111] enabled)
ACPI: X2APIC (apic_id[0x113] uid[0x113] enabled)
ACPI: X2APIC (apic_id[0x115] uid[0x115] enabled)
ACPI: X2APIC (apic_id[0x117] uid[0x117] enabled)
ACPI: X2APIC (apic_id[0x119] uid[0x119] enabled)
ACPI: X2APIC (apic_id[0x11b] uid[0x11b] enabled)
ACPI: X2APIC (apic_id[0x11d] uid[0x11d] enabled)
ACPI: X2APIC (apic_id[0x121] uid[0x121] enabled)
ACPI: X2APIC (apic_id[0x123] uid[0x123] enabled)
ACPI: X2APIC (apic_id[0x125] uid[0x125] enabled)
ACPI: X2APIC (apic_id[0x127] uid[0x127] enabled)
ACPI: X2APIC (apic_id[0x129] uid[0x129] enabled)
ACPI: X2APIC (apic_id[0x12b] uid[0x12b] enabled)
ACPI: X2APIC (apic_id[0x12d] uid[0x12d] enabled)
ACPI: X2APIC (apic_id[0x12f] uid[0x12f] enabled)
ACPI: X2APIC (apic_id[0x131] uid[0x131] enabled)
ACPI: X2APIC (apic_id[0x133] uid[0x133] enabled)
ACPI: X2APIC (apic_id[0x135] uid[0x135] enabled)
ACPI: X2APIC (apic_id[0x137] uid[0x137] enabled)
ACPI: X2APIC (apic_id[0x139] uid[0x139] enabled)
ACPI: X2APIC (apic_id[0x13b] uid[0x13b] enabled)
ACPI: X2APIC (apic_id[0x13d] uid[0x13d] enabled)
ACPI: X2APIC (apic_id[0x141] uid[0x141] enabled)
ACPI: X2APIC (apic_id[0x143] uid[0x143] enabled)
ACPI: X2APIC (apic_id[0x145] uid[0x145] enabled)
ACPI: X2APIC (apic_id[0x147] uid[0x147] enabled)
ACPI: X2APIC (apic_id[0x149] uid[0x149] enabled)
ACPI: X2APIC (apic_id[0x14b] uid[0x14b] enabled)
ACPI: X2APIC (apic_id[0x14d] uid[0x14d] enabled)
ACPI: X2APIC (apic_id[0x14f] uid[0x14f] enabled)
ACPI: X2APIC (apic_id[0x151] uid[0x151] enabled)
ACPI: X2APIC (apic_id[0x153] uid[0x153] enabled)
ACPI: X2APIC (apic_id[0x155] uid[0x155] enabled)
ACPI: X2APIC (apic_id[0x157] uid[0x157] enabled)
ACPI: X2APIC (apic_id[0x159] uid[0x159] enabled)
ACPI: X2APIC (apic_id[0x15b] uid[0x15b] enabled)
ACPI: X2APIC (apic_id[0x15d] uid[0x15d] enabled)
ACPI: X2APIC (apic_id[0x161] uid[0x161] enabled)
ACPI: X2APIC (apic_id[0x163] uid[0x163] enabled)
ACPI: X2APIC (apic_id[0x165] uid[0x165] enabled)
ACPI: X2APIC (apic_id[0x167] uid[0x167] enabled)
ACPI: X2APIC (apic_id[0x169] uid[0x169] enabled)
ACPI: X2APIC (apic_id[0x16b] uid[0x16b] enabled)
ACPI: X2APIC (apic_id[0x16d] uid[0x16d] enabled)
ACPI: X2APIC (apic_id[0x16f] uid[0x16f] enabled)
ACPI: X2APIC (apic_id[0x171] uid[0x171] enabled)
ACPI: X2APIC (apic_id[0x173] uid[0x173] enabled)
ACPI: X2APIC (apic_id[0x175] uid[0x175] enabled)
ACPI: X2APIC (apic_id[0x177] uid[0x177] enabled)
ACPI: X2APIC (apic_id[0x179] uid[0x179] enabled)
ACPI: X2APIC (apic_id[0x17b] uid[0x17b] enabled)
ACPI: X2APIC (apic_id[0x17d] uid[0x17d] enabled)
ACPI: X2APIC (apic_id[0x181] uid[0x181] enabled)
ACPI: X2APIC (apic_id[0x183] uid[0x183] enabled)
ACPI: X2APIC (apic_id[0x185] uid[0x185] enabled)
ACPI: X2APIC (apic_id[0x187] uid[0x187] enabled)
ACPI: X2APIC (apic_id[0x189] uid[0x189] enabled)
ACPI: X2APIC (apic_id[0x18b] uid[0x18b] enabled)
ACPI: X2APIC (apic_id[0x18d] uid[0x18d] enabled)
ACPI: X2APIC (apic_id[0x18f] uid[0x18f] enabled)
ACPI: X2APIC (apic_id[0x191] uid[0x191] enabled)
ACPI: X2APIC (apic_id[0x193] uid[0x193] enabled)
ACPI: X2APIC (apic_id[0x195] uid[0x195] enabled)
ACPI: X2APIC (apic_id[0x197] uid[0x197] enabled)
ACPI: X2APIC (apic_id[0x199] uid[0x199] enabled)
ACPI: X2APIC (apic_id[0x19b] uid[0x19b] enabled)
ACPI: X2APIC (apic_id[0x19d] uid[0x19d] enabled)
ACPI: X2APIC (apic_id[0x1a1] uid[0x1a1] enabled)
ACPI: X2APIC (apic_id[0x1a3] uid[0x1a3] enabled)
ACPI: X2APIC (apic_id[0x1a5] uid[0x1a5] enabled)
ACPI: X2APIC (apic_id[0x1a7] uid[0x1a7] enabled)
ACPI: X2APIC (apic_id[0x1a9] uid[0x1a9] enabled)
ACPI: X2APIC (apic_id[0x1ab] uid[0x1ab] enabled)
ACPI: X2APIC (apic_id[0x1ad] uid[0x1ad] enabled)
ACPI: X2APIC (apic_id[0x1af] uid[0x1af] enabled)
ACPI: X2APIC (apic_id[0x1b1] uid[0x1b1] enabled)
ACPI: X2APIC (apic_id[0x1b3] uid[0x1b3] enabled)
ACPI: X2APIC (apic_id[0x1b5] uid[0x1b5] enabled)
ACPI: X2APIC (apic_id[0x1b7] uid[0x1b7] enabled)
ACPI: X2APIC (apic_id[0x1b9] uid[0x1b9] enabled)
ACPI: X2APIC (apic_id[0x1bb] uid[0x1bb] enabled)
ACPI: X2APIC (apic_id[0x1bd] uid[0x1bd] enabled)
ACPI: IOAPIC (id[0x00] address[0xfec00000] global_irq_base[0x0])
ACPI: INT_SRC_OVR (bus 0 bus_irq 5 global_irq 5 high level)
ACPI: INT_SRC_OVR (bus 0 bus_irq 9 global_irq 9 high level)
ACPI: INT_SRC_OVR (bus 0 bus_irq 10 global_irq 10 high level)
ACPI: INT_SRC_OVR (bus 0 bus_irq 11 global_irq 11 high level)
ACPI: LAPIC_NMI (acpi_id[0xff] dfl dfl lint[0x1])

> >
> > TBH, I'm not sure that there is actually anything wrong with the new
> > numbering scheme.
> >  The topology is reported correctly (e.g. in
> > /sys/devices/system/cpu/cpu0/topology/thread_siblings_list). Yet, the
> > new enumeration does seem to contradict user expectations.
> >
>
> Well, we can say this is a violation of the ACPI spec.
> "OSPM should initialize processors in the order that they appear in the
> MADT." even for interleaved LAPIC and X2APIC entries.

Ah. Thanks. I didn't know that.

> Maybe we need two steps for LAPIC/X2APIC parsing.
> 1. check if there is valid LAPIC entry by going through all LAPIC
> entries first
> 2. parse LAPIC/X2APIC strictly following the order in MADT. (like we do
> before)

That makes sense to me.

Thanks,

--jim

Re: [RFC PATCH] x86/acpi: Ignore invalid x2APIC entries

[Zhang, Rui]

> > > 
> > > TBH, I'm not sure that there is actually anything wrong with the
> > > new
> > > numbering scheme.
> > >  The topology is reported correctly (e.g. in
> > > /sys/devices/system/cpu/cpu0/topology/thread_siblings_list). Yet,
> > > the
> > > new enumeration does seem to contradict user expectations.
> > > 
> > 
> > Well, we can say this is a violation of the ACPI spec.
> > "OSPM should initialize processors in the order that they appear in
> > the
> > MADT." even for interleaved LAPIC and X2APIC entries.
> 
> Ah. Thanks. I didn't know that.
> 
> > Maybe we need two steps for LAPIC/X2APIC parsing.
> > 1. check if there is valid LAPIC entry by going through all LAPIC
> > entries first
> > 2. parse LAPIC/X2APIC strictly following the order in MADT. (like
> > we do
> > before)
> 
> That makes sense to me.
> 
> Thanks,
> 
> --jim

Hi, Jim,

Please check if below patch restores the CPU IDs or not.

thanks,
rui

From ec786dfe693cad2810b54b0d8afbfc7e4c4b3f8a Mon Sep 17 00:00:00 2001
From: Zhang Rui <rui.zhang@intel.com>
Date: Mon, 14 Oct 2024 13:26:55 +0800
Subject: [PATCH] x86/acpi: Fix LAPIC/x2APIC parsing order

On some systems, the same CPU (with same APIC ID) is assigned with a
different logical CPU id after commit ec9aedb2aa1a ("x86/acpi: Ignore
invalid x2APIC entries").

This means Linux enumerates the CPUs in a different order and it is a
violation of https://uefi.org/specs/ACPI/6.5/05_ACPI_Software_Programming_Model.html#madt-processor-local-apic-sapic-structure-entry-order,

  "OSPM should initialize processors in the order that they appear in
   the MADT"

The offending commit wants to ignore x2APIC entries with APIC ID < 255
when valid LAPIC entries exist, so it parses all LAPIC entries before
parsing any x2APIC entries. This breaks the CPU enumeration order for
systems that have x2APIC entries listed before LAPIC entries in MADT.

Fix the problem by checking the valid LAPIC entries separately, before
parsing any LAPIC/x2APIC entries.

Cc: stable@vger.kernel.org
Reported-by: Jim Mattson <jmattson@google.com>
Closes: https://lore.kernel.org/all/20241010213136.668672-1-jmattson@google.com/
Fixes: ec9aedb2aa1a ("x86/acpi: Ignore invalid x2APIC entries")
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 arch/x86/kernel/acpi/boot.c | 50 +++++++++++++++++++++++++++++++++----
 1 file changed, 45 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 4efecac49863..c70b86f1f295 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -226,6 +226,28 @@ acpi_parse_x2apic(union acpi_subtable_headers *header, const unsigned long end)
 	return 0;
 }
 
+static int __init
+acpi_check_lapic(union acpi_subtable_headers *header, const unsigned long end)
+{
+	struct acpi_madt_local_apic *processor = NULL;
+
+	processor = (struct acpi_madt_local_apic *)header;
+
+	if (BAD_MADT_ENTRY(processor, end))
+		return -EINVAL;
+
+	/* Ignore invalid ID */
+	if (processor->id == 0xff)
+		return 0;
+
+	/* Ignore processors that can not be onlined */
+	if (!acpi_is_processor_usable(processor->lapic_flags))
+		return 0;
+
+	has_lapic_cpus = true;
+	return 0;
+}
+
 static int __init
 acpi_parse_lapic(union acpi_subtable_headers * header, const unsigned long end)
 {
@@ -257,7 +279,6 @@ acpi_parse_lapic(union acpi_subtable_headers * header, const unsigned long end)
 			       processor->processor_id, /* ACPI ID */
 			       processor->lapic_flags & ACPI_MADT_ENABLED);
 
-	has_lapic_cpus = true;
 	return 0;
 }
 
@@ -1029,6 +1050,8 @@ static int __init early_acpi_parse_madt_lapic_addr_ovr(void)
 static int __init acpi_parse_madt_lapic_entries(void)
 {
 	int count, x2count = 0;
+	struct acpi_subtable_proc madt_proc[2];
+	int ret;
 
 	if (!boot_cpu_has(X86_FEATURE_APIC))
 		return -ENODEV;
@@ -1037,10 +1060,27 @@ static int __init acpi_parse_madt_lapic_entries(void)
 				      acpi_parse_sapic, MAX_LOCAL_APIC);
 
 	if (!count) {
-		count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC,
-					acpi_parse_lapic, MAX_LOCAL_APIC);
-		x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC,
-					acpi_parse_x2apic, MAX_LOCAL_APIC);
+		/* Check if there are valid LAPIC entries */
+		acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC, acpi_check_lapic, MAX_LOCAL_APIC);
+
+		/*
+		 * Enumerate the APIC IDs in the order that they appear in the
+		 * MADT, no matter LAPIC entry or x2APIC entry is used.
+		 */
+		memset(madt_proc, 0, sizeof(madt_proc));
+		madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_APIC;
+		madt_proc[0].handler = acpi_parse_lapic;
+		madt_proc[1].id = ACPI_MADT_TYPE_LOCAL_X2APIC;
+		madt_proc[1].handler = acpi_parse_x2apic;
+		ret = acpi_table_parse_entries_array(ACPI_SIG_MADT,
+				sizeof(struct acpi_table_madt),
+				madt_proc, ARRAY_SIZE(madt_proc), MAX_LOCAL_APIC);
+		if (ret < 0) {
+			pr_err("Error parsing LAPIC/X2APIC entries\n");
+			return ret;
+		}
+		count = madt_proc[0].count;
+		x2count = madt_proc[1].count;
 	}
 	if (!count && !x2count) {
 		pr_err("No LAPIC entries present\n");
-- 
2.34.1

Re: [RFC PATCH] x86/acpi: Ignore invalid x2APIC entries

[Jim Mattson]

On Mon, Oct 14, 2024 at 6:05 AM Zhang, Rui <rui.zhang@intel.com> wrote:
>
> > > >
> > > > TBH, I'm not sure that there is actually anything wrong with the
> > > > new
> > > > numbering scheme.
> > > >  The topology is reported correctly (e.g. in
> > > > /sys/devices/system/cpu/cpu0/topology/thread_siblings_list). Yet,
> > > > the
> > > > new enumeration does seem to contradict user expectations.
> > > >
> > >
> > > Well, we can say this is a violation of the ACPI spec.
> > > "OSPM should initialize processors in the order that they appear in
> > > the
> > > MADT." even for interleaved LAPIC and X2APIC entries.
> >
> > Ah. Thanks. I didn't know that.
> >
> > > Maybe we need two steps for LAPIC/X2APIC parsing.
> > > 1. check if there is valid LAPIC entry by going through all LAPIC
> > > entries first
> > > 2. parse LAPIC/X2APIC strictly following the order in MADT. (like
> > > we do
> > > before)
> >
> > That makes sense to me.
> >
> > Thanks,
> >
> > --jim
>
> Hi, Jim,
>
> Please check if below patch restores the CPU IDs or not.
>
> thanks,
> rui
>
> From ec786dfe693cad2810b54b0d8afbfc7e4c4b3f8a Mon Sep 17 00:00:00 2001
> From: Zhang Rui <rui.zhang@intel.com>
> Date: Mon, 14 Oct 2024 13:26:55 +0800
> Subject: [PATCH] x86/acpi: Fix LAPIC/x2APIC parsing order
>
> On some systems, the same CPU (with same APIC ID) is assigned with a
> different logical CPU id after commit ec9aedb2aa1a ("x86/acpi: Ignore
> invalid x2APIC entries").
>
> This means Linux enumerates the CPUs in a different order and it is a
> violation of https://uefi.org/specs/ACPI/6.5/05_ACPI_Software_Programming_Model.html#madt-processor-local-apic-sapic-structure-entry-order,
>
>   "OSPM should initialize processors in the order that they appear in
>    the MADT"
>
> The offending commit wants to ignore x2APIC entries with APIC ID < 255
> when valid LAPIC entries exist, so it parses all LAPIC entries before
> parsing any x2APIC entries. This breaks the CPU enumeration order for
> systems that have x2APIC entries listed before LAPIC entries in MADT.
>
> Fix the problem by checking the valid LAPIC entries separately, before
> parsing any LAPIC/x2APIC entries.
>
> Cc: stable@vger.kernel.org
> Reported-by: Jim Mattson <jmattson@google.com>
> Closes: https://lore.kernel.org/all/20241010213136.668672-1-jmattson@google.com/
> Fixes: ec9aedb2aa1a ("x86/acpi: Ignore invalid x2APIC entries")
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  arch/x86/kernel/acpi/boot.c | 50 +++++++++++++++++++++++++++++++++----
>  1 file changed, 45 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> index 4efecac49863..c70b86f1f295 100644
> --- a/arch/x86/kernel/acpi/boot.c
> +++ b/arch/x86/kernel/acpi/boot.c
> @@ -226,6 +226,28 @@ acpi_parse_x2apic(union acpi_subtable_headers *header, const unsigned long end)
>         return 0;
>  }
>
> +static int __init
> +acpi_check_lapic(union acpi_subtable_headers *header, const unsigned long end)
> +{
> +       struct acpi_madt_local_apic *processor = NULL;
> +
> +       processor = (struct acpi_madt_local_apic *)header;
> +
> +       if (BAD_MADT_ENTRY(processor, end))
> +               return -EINVAL;
> +
> +       /* Ignore invalid ID */
> +       if (processor->id == 0xff)
> +               return 0;
> +
> +       /* Ignore processors that can not be onlined */
> +       if (!acpi_is_processor_usable(processor->lapic_flags))
> +               return 0;
> +
> +       has_lapic_cpus = true;
> +       return 0;
> +}
> +
>  static int __init
>  acpi_parse_lapic(union acpi_subtable_headers * header, const unsigned long end)
>  {
> @@ -257,7 +279,6 @@ acpi_parse_lapic(union acpi_subtable_headers * header, const unsigned long end)
>                                processor->processor_id, /* ACPI ID */
>                                processor->lapic_flags & ACPI_MADT_ENABLED);
>
> -       has_lapic_cpus = true;
>         return 0;
>  }
>
> @@ -1029,6 +1050,8 @@ static int __init early_acpi_parse_madt_lapic_addr_ovr(void)
>  static int __init acpi_parse_madt_lapic_entries(void)
>  {
>         int count, x2count = 0;
> +       struct acpi_subtable_proc madt_proc[2];
> +       int ret;
>
>         if (!boot_cpu_has(X86_FEATURE_APIC))
>                 return -ENODEV;
> @@ -1037,10 +1060,27 @@ static int __init acpi_parse_madt_lapic_entries(void)
>                                       acpi_parse_sapic, MAX_LOCAL_APIC);
>
>         if (!count) {
> -               count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC,
> -                                       acpi_parse_lapic, MAX_LOCAL_APIC);
> -               x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC,
> -                                       acpi_parse_x2apic, MAX_LOCAL_APIC);

The point is moot now, but I don't think the previous code did the
right thing when acpi_table_parse_madt() returned a negative value
(for errors).

> +               /* Check if there are valid LAPIC entries */
> +               acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC, acpi_check_lapic, MAX_LOCAL_APIC);

Two comments:

1) Should we check for a return value < 0 here, or just wait for one
of the later walks to error out?
2) It seems unfortunate to walk the entire table when the first entry
may give you the answer, but perhaps modern systems have only X2APIC
entries, so we will typically have to walk the entire table anyway.

Reviewed-and-tested-by: Jim Mattson <jmattson@google.com>

Re: [RFC PATCH] x86/acpi: Ignore invalid x2APIC entries

[Zhang, Rui]

On Mon, 2024-10-14 at 11:00 -0700, Jim Mattson wrote:
> On Mon, Oct 14, 2024 at 6:05 AM Zhang, Rui <rui.zhang@intel.com>
> wrote:
> > 
> > > > > 
> > > > > TBH, I'm not sure that there is actually anything wrong with
> > > > > the
> > > > > new
> > > > > numbering scheme.
> > > > >  The topology is reported correctly (e.g. in
> > > > > /sys/devices/system/cpu/cpu0/topology/thread_siblings_list).
> > > > > Yet,
> > > > > the
> > > > > new enumeration does seem to contradict user expectations.
> > > > > 
> > > > 
> > > > Well, we can say this is a violation of the ACPI spec.
> > > > "OSPM should initialize processors in the order that they
> > > > appear in
> > > > the
> > > > MADT." even for interleaved LAPIC and X2APIC entries.
> > > 
> > > Ah. Thanks. I didn't know that.
> > > 
> > > > Maybe we need two steps for LAPIC/X2APIC parsing.
> > > > 1. check if there is valid LAPIC entry by going through all
> > > > LAPIC
> > > > entries first
> > > > 2. parse LAPIC/X2APIC strictly following the order in MADT.
> > > > (like
> > > > we do
> > > > before)
> > > 
> > > That makes sense to me.
> > > 
> > > Thanks,
> > > 
> > > --jim
> > 
> > Hi, Jim,
> > 
> > Please check if below patch restores the CPU IDs or not.
> > 
> > thanks,
> > rui
> > 
> > From ec786dfe693cad2810b54b0d8afbfc7e4c4b3f8a Mon Sep 17 00:00:00
> > 2001
> > From: Zhang Rui <rui.zhang@intel.com>
> > Date: Mon, 14 Oct 2024 13:26:55 +0800
> > Subject: [PATCH] x86/acpi: Fix LAPIC/x2APIC parsing order
> > 
> > On some systems, the same CPU (with same APIC ID) is assigned with
> > a
> > different logical CPU id after commit ec9aedb2aa1a ("x86/acpi:
> > Ignore
> > invalid x2APIC entries").
> > 
> > This means Linux enumerates the CPUs in a different order and it is
> > a
> > violation of
> > https://uefi.org/specs/ACPI/6.5/05_ACPI_Software_Programming_Model.html#madt-processor-local-apic-sapic-structure-entry-order
> > ,
> > 
> >   "OSPM should initialize processors in the order that they appear
> > in
> >    the MADT"
> > 
> > The offending commit wants to ignore x2APIC entries with APIC ID <
> > 255
> > when valid LAPIC entries exist, so it parses all LAPIC entries
> > before
> > parsing any x2APIC entries. This breaks the CPU enumeration order
> > for
> > systems that have x2APIC entries listed before LAPIC entries in
> > MADT.
> > 
> > Fix the problem by checking the valid LAPIC entries separately,
> > before
> > parsing any LAPIC/x2APIC entries.
> > 
> > Cc: stable@vger.kernel.org
> > Reported-by: Jim Mattson <jmattson@google.com>
> > Closes:
> > https://lore.kernel.org/all/20241010213136.668672-1-jmattson@google.com/
> > Fixes: ec9aedb2aa1a ("x86/acpi: Ignore invalid x2APIC entries")
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  arch/x86/kernel/acpi/boot.c | 50
> > +++++++++++++++++++++++++++++++++----
> >  1 file changed, 45 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/acpi/boot.c
> > b/arch/x86/kernel/acpi/boot.c
> > index 4efecac49863..c70b86f1f295 100644
> > --- a/arch/x86/kernel/acpi/boot.c
> > +++ b/arch/x86/kernel/acpi/boot.c
> > @@ -226,6 +226,28 @@ acpi_parse_x2apic(union acpi_subtable_headers
> > *header, const unsigned long end)
> >         return 0;
> >  }
> > 
> > +static int __init
> > +acpi_check_lapic(union acpi_subtable_headers *header, const
> > unsigned long end)
> > +{
> > +       struct acpi_madt_local_apic *processor = NULL;
> > +
> > +       processor = (struct acpi_madt_local_apic *)header;
> > +
> > +       if (BAD_MADT_ENTRY(processor, end))
> > +               return -EINVAL;
> > +
> > +       /* Ignore invalid ID */
> > +       if (processor->id == 0xff)
> > +               return 0;
> > +
> > +       /* Ignore processors that can not be onlined */
> > +       if (!acpi_is_processor_usable(processor->lapic_flags))
> > +               return 0;
> > +
> > +       has_lapic_cpus = true;
> > +       return 0;
> > +}
> > +
> >  static int __init
> >  acpi_parse_lapic(union acpi_subtable_headers * header, const
> > unsigned long end)
> >  {
> > @@ -257,7 +279,6 @@ acpi_parse_lapic(union acpi_subtable_headers *
> > header, const unsigned long end)
> >                                processor->processor_id, /* ACPI ID
> > */
> >                                processor->lapic_flags &
> > ACPI_MADT_ENABLED);
> > 
> > -       has_lapic_cpus = true;
> >         return 0;
> >  }
> > 
> > @@ -1029,6 +1050,8 @@ static int __init
> > early_acpi_parse_madt_lapic_addr_ovr(void)
> >  static int __init acpi_parse_madt_lapic_entries(void)
> >  {
> >         int count, x2count = 0;
> > +       struct acpi_subtable_proc madt_proc[2];
> > +       int ret;
> > 
> >         if (!boot_cpu_has(X86_FEATURE_APIC))
> >                 return -ENODEV;
> > @@ -1037,10 +1060,27 @@ static int __init
> > acpi_parse_madt_lapic_entries(void)
> >                                       acpi_parse_sapic,
> > MAX_LOCAL_APIC);
> > 
> >         if (!count) {
> > -               count =
> > acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC,
> > -                                       acpi_parse_lapic,
> > MAX_LOCAL_APIC);
> > -               x2count =
> > acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC,
> > -                                       acpi_parse_x2apic,
> > MAX_LOCAL_APIC);
> 
> The point is moot now, but I don't think the previous code did the
> right thing when acpi_table_parse_madt() returned a negative value
> (for errors).

Previous and current code checks for the negative value later after
parsing both LAPIC and x2APIC.
so what is the problem you're referring to?
Do you mean we should error out immediately when parsing LAPIC fails?

> 
> > +               /* Check if there are valid LAPIC entries */
> > +               acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC,
> > acpi_check_lapic, MAX_LOCAL_APIC);
> 
> Two comments:
> 
> 1) Should we check for a return value < 0 here, or just wait for one
> of the later walks to error out?

I'm okay with both.

> 2) It seems unfortunate to walk the entire table when the first entry
> may give you the answer, but perhaps modern systems have only X2APIC
> entries, so we will typically have to walk the entire table anyway.

yeah. There are systems with invalid LAPIC entries first, and
acpi_parse_entries_array() doesn't support graceful early termination,
so we have to check all the entries.

> 
> Reviewed-and-tested-by: Jim Mattson <jmattson@google.com>

Thanks. I will submit the current version to keep your tags.

-rui

Re: [RFC PATCH] x86/acpi: Ignore invalid x2APIC entries

[Jim Mattson]

On Mon, Oct 14, 2024 at 8:23 PM Zhang, Rui <rui.zhang@intel.com> wrote:
>
> On Mon, 2024-10-14 at 11:00 -0700, Jim Mattson wrote:
> > On Mon, Oct 14, 2024 at 6:05 AM Zhang, Rui <rui.zhang@intel.com>
> > wrote:
> > >
> > > > > >
> > > > > > TBH, I'm not sure that there is actually anything wrong with
> > > > > > the
> > > > > > new
> > > > > > numbering scheme.
> > > > > >  The topology is reported correctly (e.g. in
> > > > > > /sys/devices/system/cpu/cpu0/topology/thread_siblings_list).
> > > > > > Yet,
> > > > > > the
> > > > > > new enumeration does seem to contradict user expectations.
> > > > > >
> > > > >
> > > > > Well, we can say this is a violation of the ACPI spec.
> > > > > "OSPM should initialize processors in the order that they
> > > > > appear in
> > > > > the
> > > > > MADT." even for interleaved LAPIC and X2APIC entries.
> > > >
> > > > Ah. Thanks. I didn't know that.
> > > >
> > > > > Maybe we need two steps for LAPIC/X2APIC parsing.
> > > > > 1. check if there is valid LAPIC entry by going through all
> > > > > LAPIC
> > > > > entries first
> > > > > 2. parse LAPIC/X2APIC strictly following the order in MADT.
> > > > > (like
> > > > > we do
> > > > > before)
> > > >
> > > > That makes sense to me.
> > > >
> > > > Thanks,
> > > >
> > > > --jim
> > >
> > > Hi, Jim,
> > >
> > > Please check if below patch restores the CPU IDs or not.
> > >
> > > thanks,
> > > rui
> > >
> > > From ec786dfe693cad2810b54b0d8afbfc7e4c4b3f8a Mon Sep 17 00:00:00
> > > 2001
> > > From: Zhang Rui <rui.zhang@intel.com>
> > > Date: Mon, 14 Oct 2024 13:26:55 +0800
> > > Subject: [PATCH] x86/acpi: Fix LAPIC/x2APIC parsing order
> > >
> > > On some systems, the same CPU (with same APIC ID) is assigned with
> > > a
> > > different logical CPU id after commit ec9aedb2aa1a ("x86/acpi:
> > > Ignore
> > > invalid x2APIC entries").
> > >
> > > This means Linux enumerates the CPUs in a different order and it is
> > > a
> > > violation of
> > > https://uefi.org/specs/ACPI/6.5/05_ACPI_Software_Programming_Model.html#madt-processor-local-apic-sapic-structure-entry-order
> > > ,
> > >
> > >   "OSPM should initialize processors in the order that they appear
> > > in
> > >    the MADT"
> > >
> > > The offending commit wants to ignore x2APIC entries with APIC ID <
> > > 255
> > > when valid LAPIC entries exist, so it parses all LAPIC entries
> > > before
> > > parsing any x2APIC entries. This breaks the CPU enumeration order
> > > for
> > > systems that have x2APIC entries listed before LAPIC entries in
> > > MADT.
> > >
> > > Fix the problem by checking the valid LAPIC entries separately,
> > > before
> > > parsing any LAPIC/x2APIC entries.
> > >
> > > Cc: stable@vger.kernel.org
> > > Reported-by: Jim Mattson <jmattson@google.com>
> > > Closes:
> > > https://lore.kernel.org/all/20241010213136.668672-1-jmattson@google.com/
> > > Fixes: ec9aedb2aa1a ("x86/acpi: Ignore invalid x2APIC entries")
> > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > > ---
> > >  arch/x86/kernel/acpi/boot.c | 50
> > > +++++++++++++++++++++++++++++++++----
> > >  1 file changed, 45 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/arch/x86/kernel/acpi/boot.c
> > > b/arch/x86/kernel/acpi/boot.c
> > > index 4efecac49863..c70b86f1f295 100644
> > > --- a/arch/x86/kernel/acpi/boot.c
> > > +++ b/arch/x86/kernel/acpi/boot.c
> > > @@ -226,6 +226,28 @@ acpi_parse_x2apic(union acpi_subtable_headers
> > > *header, const unsigned long end)
> > >         return 0;
> > >  }
> > >
> > > +static int __init
> > > +acpi_check_lapic(union acpi_subtable_headers *header, const
> > > unsigned long end)
> > > +{
> > > +       struct acpi_madt_local_apic *processor = NULL;
> > > +
> > > +       processor = (struct acpi_madt_local_apic *)header;
> > > +
> > > +       if (BAD_MADT_ENTRY(processor, end))
> > > +               return -EINVAL;
> > > +
> > > +       /* Ignore invalid ID */
> > > +       if (processor->id == 0xff)
> > > +               return 0;
> > > +
> > > +       /* Ignore processors that can not be onlined */
> > > +       if (!acpi_is_processor_usable(processor->lapic_flags))
> > > +               return 0;
> > > +
> > > +       has_lapic_cpus = true;
> > > +       return 0;
> > > +}
> > > +
> > >  static int __init
> > >  acpi_parse_lapic(union acpi_subtable_headers * header, const
> > > unsigned long end)
> > >  {
> > > @@ -257,7 +279,6 @@ acpi_parse_lapic(union acpi_subtable_headers *
> > > header, const unsigned long end)
> > >                                processor->processor_id, /* ACPI ID
> > > */
> > >                                processor->lapic_flags &
> > > ACPI_MADT_ENABLED);
> > >
> > > -       has_lapic_cpus = true;
> > >         return 0;
> > >  }
> > >
> > > @@ -1029,6 +1050,8 @@ static int __init
> > > early_acpi_parse_madt_lapic_addr_ovr(void)
> > >  static int __init acpi_parse_madt_lapic_entries(void)
> > >  {
> > >         int count, x2count = 0;
> > > +       struct acpi_subtable_proc madt_proc[2];
> > > +       int ret;
> > >
> > >         if (!boot_cpu_has(X86_FEATURE_APIC))
> > >                 return -ENODEV;
> > > @@ -1037,10 +1060,27 @@ static int __init
> > > acpi_parse_madt_lapic_entries(void)
> > >                                       acpi_parse_sapic,
> > > MAX_LOCAL_APIC);
> > >
> > >         if (!count) {
> > > -               count =
> > > acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC,
> > > -                                       acpi_parse_lapic,
> > > MAX_LOCAL_APIC);
> > > -               x2count =
> > > acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC,
> > > -                                       acpi_parse_x2apic,
> > > MAX_LOCAL_APIC);
> >
> > The point is moot now, but I don't think the previous code did the
> > right thing when acpi_table_parse_madt() returned a negative value
> > (for errors).
>
> Previous and current code checks for the negative value later after
> parsing both LAPIC and x2APIC.
> so what is the problem you're referring to?
> Do you mean we should error out immediately when parsing LAPIC fails?

My mistake. I should have looked at the full context rather than just
the context of the patch.

> >
> > > +               /* Check if there are valid LAPIC entries */
> > > +               acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC,
> > > acpi_check_lapic, MAX_LOCAL_APIC);
> >
> > Two comments:
> >
> > 1) Should we check for a return value < 0 here, or just wait for one
> > of the later walks to error out?
>
> I'm okay with both.
>
> > 2) It seems unfortunate to walk the entire table when the first entry
> > may give you the answer, but perhaps modern systems have only X2APIC
> > entries, so we will typically have to walk the entire table anyway.
>
> yeah. There are systems with invalid LAPIC entries first, and
> acpi_parse_entries_array() doesn't support graceful early termination,
> so we have to check all the entries.
>
> >
> > Reviewed-and-tested-by: Jim Mattson <jmattson@google.com>
>
> Thanks. I will submit the current version to keep your tags.
>
> -rui

Thanks!

--jim