2023-11-22 22:20:06

by John Sperbeck

[permalink] [raw]
Subject: Re: [tip: x86/urgent] x86/acpi: Ignore invalid x2APIC entries

I have a platform with both LOCAL_APIC and LOCAL_X2APIC entries for
each CPU. However, the ids for the LOCAL_APIC entries are all
invalid ids of 255, so they have always been skipped in acpi_parse_lapic()
by this code from f3bf1dbe64b6 ("x86/acpi: Prevent LAPIC id 0xff from being
accounted"):

/* Ignore invalid ID */
if (processor->id == 0xff)
return 0;

With the change in this thread, the return value of 0 means that the
'count' variable in acpi_parse_entries_array() is incremented. The
positive return value means that 'has_lapic_cpus' is set, even though
no entries were actually matched. Then, when the MADT is iterated
with acpi_parse_x2apic(), the x2apic entries with ids less than 255
are skipped and most of my CPUs aren't recognized.

I think the original version of this change was okay for this case in
https://lore.kernel.org/lkml/87pm4bp54z.ffs@tglx/T/

P.S. I could be convinced that the MADT for my platform is somewhat
ill-formed and that I'm relying on pre-existing behavior. I'm not
well-versed enough in the topic to know for sure.


2023-11-23 12:51:18

by Zhang, Rui

[permalink] [raw]
Subject: Re: [tip: x86/urgent] x86/acpi: Ignore invalid x2APIC entries

Hi, John,

Thanks for catching this issue.

On Wed, 2023-11-22 at 22:19 +0000, John Sperbeck wrote:
> I have a platform with both LOCAL_APIC and LOCAL_X2APIC entries for
> each CPU.  However, the ids for the LOCAL_APIC entries are all
> invalid ids of 255, so they have always been skipped in
> acpi_parse_lapic()
> by this code from f3bf1dbe64b6 ("x86/acpi: Prevent LAPIC id 0xff from
> being
> accounted"):
>
>     /* Ignore invalid ID */
>     if (processor->id == 0xff)
>             return 0;
>
> With the change in this thread, the return value of 0 means that the
> 'count' variable in acpi_parse_entries_array() is incremented.  The
> positive return value means that 'has_lapic_cpus' is set, even though
> no entries were actually matched.

So in acpi_parse_madt_lapic_entries, without this patch,
madt_proc[0].count is a positive value on this platform, right?

This sounds like a potential issue because the following checks to fall
back to MPS mode can also break. (If all LOCAL_APIC entries have
apic_id 0xff and all LOCAL_X2APIC entries have apic_id 0xffffffff)

>   Then, when the MADT is iterated
> with acpi_parse_x2apic(), the x2apic entries with ids less than 255
> are skipped and most of my CPUs aren't recognized.
>
> I think the original version of this change was okay for this case in
> https://lore.kernel.org/lkml/87pm4bp54z.ffs@tglx/T/

Yeah.

But if we want to fix the potential issue above, we need to do
something more.

Say we can still use acpi_table_parse_entries_array() and convert
acpi_parse_lapic()/acpi_parse_x2apic() to
acpi_subtable_proc.handler_arg and save the real valid entries via the
parameter.

or can we just use num_processors & disabled_cpus to check if there is
any CPU probed when parsing LOCAL_APIC/LOCAL_X2APIC entires?

thanks,
rui
>
> P.S. I could be convinced that the MADT for my platform is somewhat
> ill-formed and that I'm relying on pre-existing behavior.  I'm not
> well-versed enough in the topic to know for sure.

2023-12-01 03:28:35

by Ashok Raj

[permalink] [raw]
Subject: Re: [tip: x86/urgent] x86/acpi: Ignore invalid x2APIC entries

On Thu, Nov 23, 2023 at 12:50:47PM +0000, Zhang Rui wrote:
> Hi, John,
>
> Thanks for catching this issue.
>
> On Wed, 2023-11-22 at 22:19 +0000, John Sperbeck wrote:
> > I have a platform with both LOCAL_APIC and LOCAL_X2APIC entries for
> > each CPU.? However, the ids for the LOCAL_APIC entries are all
> > invalid ids of 255, so they have always been skipped in
> > acpi_parse_lapic()
> > by this code from f3bf1dbe64b6 ("x86/acpi: Prevent LAPIC id 0xff from
> > being
> > accounted"):
> >
> > ??? /* Ignore invalid ID */
> > ??? if (processor->id == 0xff)
> > ??????????? return 0;
> >
> > With the change in this thread, the return value of 0 means that the
> > 'count' variable in acpi_parse_entries_array() is incremented.? The
> > positive return value means that 'has_lapic_cpus' is set, even though
> > no entries were actually matched.
>
> So in acpi_parse_madt_lapic_entries, without this patch,
> madt_proc[0].count is a positive value on this platform, right?
>
> This sounds like a potential issue because the following checks to fall
> back to MPS mode can also break. (If all LOCAL_APIC entries have
> apic_id 0xff and all LOCAL_X2APIC entries have apic_id 0xffffffff)
>
> > ? Then, when the MADT is iterated
> > with acpi_parse_x2apic(), the x2apic entries with ids less than 255
> > are skipped and most of my CPUs aren't recognized.

This smells wrong. If a BIOS is placing some in lapic and some in x2apic
table, its really messed up.

Shouldn't the kernel scan them in some priority and only consider one set of
tables?

Shouldn't the code stop looking once something once a type is found?

sapic is only for IA64 correct?

sapic_entries = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_SAPIC,....)

if sapic_entries
return sapic_entries;

x2apic_count = ...

if x2apic_count
return x2apic_count;

apic_count = ...

Maybe you should add if all entries are marked INVALID for APIC, just
ignore the whole table?

Cheers,
Ashok

2023-12-01 08:32:42

by Zhang, Rui

[permalink] [raw]
Subject: Re: [tip: x86/urgent] x86/acpi: Ignore invalid x2APIC entries

On Thu, 2023-11-23 at 20:50 +0800, Exchange Online wrote:
> Hi, John,
>
> Thanks for catching this issue.
>
> On Wed, 2023-11-22 at 22:19 +0000, John Sperbeck wrote:
> > I have a platform with both LOCAL_APIC and LOCAL_X2APIC entries for
> > each CPU.  However, the ids for the LOCAL_APIC entries are all
> > invalid ids of 255, so they have always been skipped in
> > acpi_parse_lapic()
> > by this code from f3bf1dbe64b6 ("x86/acpi: Prevent LAPIC id 0xff
> > from
> > being
> > accounted"):
> >
> >     /* Ignore invalid ID */
> >     if (processor->id == 0xff)
> >             return 0;
> >
> > With the change in this thread, the return value of 0 means that
> > the
> > 'count' variable in acpi_parse_entries_array() is incremented.  The
> > positive return value means that 'has_lapic_cpus' is set, even
> > though
> > no entries were actually matched.
>
> So in acpi_parse_madt_lapic_entries, without this patch,
> madt_proc[0].count is a positive value on this platform, right?
>
> This sounds like a potential issue because the following checks to
> fall
> back to MPS mode can also break. (If all LOCAL_APIC entries have
> apic_id 0xff and all LOCAL_X2APIC entries have apic_id 0xffffffff)
>
> >   Then, when the MADT is iterated
> > with acpi_parse_x2apic(), the x2apic entries with ids less than 255
> > are skipped and most of my CPUs aren't recognized.
> >
> > I think the original version of this change was okay for this case
> > in
> > https://lore.kernel.org/lkml/87pm4bp54z.ffs@tglx/T/
>
> Yeah.
>
> But if we want to fix the potential issue above, we need to do
> something more.
>
> Say we can still use acpi_table_parse_entries_array() and convert
> acpi_parse_lapic()/acpi_parse_x2apic() to
> acpi_subtable_proc.handler_arg and save the real valid entries via
> the
> parameter.
>
> or can we just use num_processors & disabled_cpus to check if there
> is
> any CPU probed when parsing LOCAL_APIC/LOCAL_X2APIC entires?
>

Hi, John,

As a quick fix, I'm not going to fix the "potential issue" describes
above because we have not seen a real problem caused by this yet.

Can you please try the below patch to confirm if the problem is gone on
your system?
This patch falls back to the previous way as sent at
https://lore.kernel.org/lkml/87pm4bp54z.ffs@tglx/T/

thanks,
rui

From bdb45e241b4fea8a12b958e490979e96b064e43d Mon Sep 17 00:00:00 2001
From: Zhang Rui <[email protected]>
Date: Fri, 1 Dec 2023 15:06:34 +0800
Subject: [PATCH] x86/acpi: Do strict X2APIC ID check only when an enabled CPU
is enumerated via LAPIC

Commit 8e9c42d776d6 ("x86/acpi: Ignore invalid x2APIC entries") does
strict X2APIC ID check if LAPIC contains valid CPUs by checking the
acpi_table_parse_madt() return value.

This is wrong because acpi_table_parse_madt() return value only
represents the number of legal entries parsed. For example, LAPIC entry
with LAPIC ID 0xff is counted as a legal entry, but it doesn't describe
a valid CPU.

This causes issues on a system which has 0xff LAPIC ID in all LAPIC
entries. Because the code does strict X2APIC IDs check and ignores most
of the CPUs in X2APIC entries.

Fix the problem by doing strict X2APIC ID check less aggressively, say
only when an enabled CPU is enumerated via LAPIC.

Fixes: 8e9c42d776d6 ("x86/acpi: Ignore invalid x2APIC entries")
Link: https://lore.kernel.org/all/[email protected]/
Reported-by: John Sperbeck <[email protected]>
Signed-off-by: Zhang Rui <[email protected]>
---
arch/x86/kernel/acpi/boot.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 1a0dd80d81ac..8cc566ce486a 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -266,6 +266,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;

@@ -289,9 +290,13 @@ 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 */
- processor->processor_id, /* ACPI ID */
- processor->lapic_flags & ACPI_MADT_ENABLED);
+ cpu = acpi_register_lapic(processor->id, /* APIC ID */
+ processor->processor_id, /* ACPI ID */
+ processor->lapic_flags & ACPI_MADT_ENABLED);
+
+ /* Do strict X2APIC ID check only when an enabled CPU is enumerated via LAPIC */
+ if (cpu >= 0 )
+ has_lapic_cpus = true;

return 0;
}
@@ -1134,7 +1139,6 @@ static int __init acpi_parse_madt_lapic_entries(void)
if (!count) {
count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC,
acpi_parse_lapic, MAX_LOCAL_APIC);
- has_lapic_cpus = count > 0;
x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC,
acpi_parse_x2apic, MAX_LOCAL_APIC);
}
--
2.34.1








2023-12-01 18:09:48

by Zhang, Rui

[permalink] [raw]
Subject: Re: [tip: x86/urgent] x86/acpi: Ignore invalid x2APIC entries

On Thu, 2023-11-30 at 19:25 -0800, Ashok Raj wrote:
> On Thu, Nov 23, 2023 at 12:50:47PM +0000, Zhang Rui wrote:
> > Hi, John,
> >
> > Thanks for catching this issue.
> >
> > On Wed, 2023-11-22 at 22:19 +0000, John Sperbeck wrote:
> > > I have a platform with both LOCAL_APIC and LOCAL_X2APIC entries
> > > for
> > > each CPU.  However, the ids for the LOCAL_APIC entries are all
> > > invalid ids of 255, so they have always been skipped in
> > > acpi_parse_lapic()
> > > by this code from f3bf1dbe64b6 ("x86/acpi: Prevent LAPIC id 0xff
> > > from
> > > being
> > > accounted"):
> > >
> > >     /* Ignore invalid ID */
> > >     if (processor->id == 0xff)
> > >             return 0;
> > >
> > > With the change in this thread, the return value of 0 means that
> > > the
> > > 'count' variable in acpi_parse_entries_array() is incremented. 
> > > The
> > > positive return value means that 'has_lapic_cpus' is set, even
> > > though
> > > no entries were actually matched.
> >
> > So in acpi_parse_madt_lapic_entries, without this patch,
> > madt_proc[0].count is a positive value on this platform, right?
> >
> > This sounds like a potential issue because the following checks to
> > fall
> > back to MPS mode can also break. (If all LOCAL_APIC entries have
> > apic_id 0xff and all LOCAL_X2APIC entries have apic_id 0xffffffff)
> >
> > >   Then, when the MADT is iterated
> > > with acpi_parse_x2apic(), the x2apic entries with ids less than
> > > 255
> > > are skipped and most of my CPUs aren't recognized.
>
> This smells wrong. If a BIOS is placing some in lapic and some in
> x2apic
> table, its really messed up.
>
> Shouldn't the kernel scan them in some priority and only consider one
> set of
> tables?
>
> Shouldn't the code stop looking once something once a type is found?
>

I also want to get this clarified but there is no spec saying this. And
instead, as mentioned in the comment, we do have something in
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."

so it is possible to enumerate CPUs from both LAPIC and X2APIC.

thanks,
rui

> sapic is only for IA64 correct?
>
> sapic_entries =
> acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_SAPIC,....)
>
> if sapic_entries
>         return sapic_entries;
>
> x2apic_count = ...
>
> if x2apic_count
>         return x2apic_count;
>
> apic_count = ...
>
> Maybe you should add if all entries are marked INVALID for APIC, just
> ignore the whole table?
>
> Cheers,
> Ashok

2023-12-01 20:23:19

by Raj, Ashok

[permalink] [raw]
Subject: Re: [tip: x86/urgent] x86/acpi: Ignore invalid x2APIC entries

On Fri, Dec 01, 2023 at 10:08:55AM -0800, Zhang, Rui wrote:
> On Thu, 2023-11-30 at 19:25 -0800, Ashok Raj wrote:
> > On Thu, Nov 23, 2023 at 12:50:47PM +0000, Zhang Rui wrote:
> > > Hi, John,
> > >
> > > Thanks for catching this issue.
> > >
> > > On Wed, 2023-11-22 at 22:19 +0000, John Sperbeck wrote:
> > > > I have a platform with both LOCAL_APIC and LOCAL_X2APIC entries
> > > > for
> > > > each CPU.? However, the ids for the LOCAL_APIC entries are all
> > > > invalid ids of 255, so they have always been skipped in
> > > > acpi_parse_lapic()
> > > > by this code from f3bf1dbe64b6 ("x86/acpi: Prevent LAPIC id 0xff
> > > > from
> > > > being
> > > > accounted"):
> > > >
> > > > ??? /* Ignore invalid ID */
> > > > ??? if (processor->id == 0xff)
> > > > ??????????? return 0;
> > > >
> > > > With the change in this thread, the return value of 0 means that
> > > > the
> > > > 'count' variable in acpi_parse_entries_array() is incremented.?
> > > > The
> > > > positive return value means that 'has_lapic_cpus' is set, even
> > > > though
> > > > no entries were actually matched.
> > >
> > > So in acpi_parse_madt_lapic_entries, without this patch,
> > > madt_proc[0].count is a positive value on this platform, right?
> > >
> > > This sounds like a potential issue because the following checks to
> > > fall
> > > back to MPS mode can also break. (If all LOCAL_APIC entries have
> > > apic_id 0xff and all LOCAL_X2APIC entries have apic_id 0xffffffff)
> > >
> > > > ? Then, when the MADT is iterated
> > > > with acpi_parse_x2apic(), the x2apic entries with ids less than
> > > > 255
> > > > are skipped and most of my CPUs aren't recognized.
> >
> > This smells wrong. If a BIOS is placing some in lapic and some in
> > x2apic
> > table, its really messed up.
> >
> > Shouldn't the kernel scan them in some priority and only consider one
> > set of
> > tables?
> >
> > Shouldn't the code stop looking once something once a type is found?
> >
>
> I also want to get this clarified but there is no spec saying this. And
> instead, as mentioned in the comment, we do have something in
> 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."
>
> so it is possible to enumerate CPUs from both LAPIC and X2APIC.
>

Ah, so this looks like the legacy case, old OS can atleast boot the APIC
entries and not process the x2apic ones.

So you can potentially have duplicates

APIC = has all APIC id's < 255
X2apic has all entries > 255 OR
It can contain everything, so you might need to weed out
duplicates?

2023-12-01 23:32:52

by John Sperbeck

[permalink] [raw]
Subject: Re: [tip: x86/urgent] x86/acpi: Ignore invalid x2APIC entries

On Fri, Dec 1, 2023 at 12:32 AM Zhang, Rui <[email protected]> wrote:
>
> On Thu, 2023-11-23 at 20:50 +0800, Exchange Online wrote:
> > Hi, John,
> >
> > Thanks for catching this issue.
> >
> > On Wed, 2023-11-22 at 22:19 +0000, John Sperbeck wrote:
> > > I have a platform with both LOCAL_APIC and LOCAL_X2APIC entries for
> > > each CPU. However, the ids for the LOCAL_APIC entries are all
> > > invalid ids of 255, so they have always been skipped in
> > > acpi_parse_lapic()
> > > by this code from f3bf1dbe64b6 ("x86/acpi: Prevent LAPIC id 0xff
> > > from
> > > being
> > > accounted"):
> > >
> > > /* Ignore invalid ID */
> > > if (processor->id == 0xff)
> > > return 0;
> > >
> > > With the change in this thread, the return value of 0 means that
> > > the
> > > 'count' variable in acpi_parse_entries_array() is incremented. The
> > > positive return value means that 'has_lapic_cpus' is set, even
> > > though
> > > no entries were actually matched.
> >
> > So in acpi_parse_madt_lapic_entries, without this patch,
> > madt_proc[0].count is a positive value on this platform, right?
> >
> > This sounds like a potential issue because the following checks to
> > fall
> > back to MPS mode can also break. (If all LOCAL_APIC entries have
> > apic_id 0xff and all LOCAL_X2APIC entries have apic_id 0xffffffff)
> >
> > > Then, when the MADT is iterated
> > > with acpi_parse_x2apic(), the x2apic entries with ids less than 255
> > > are skipped and most of my CPUs aren't recognized.
> > >
> > > I think the original version of this change was okay for this case
> > > in
> > > https://lore.kernel.org/lkml/87pm4bp54z.ffs@tglx/T/
> >
> > Yeah.
> >
> > But if we want to fix the potential issue above, we need to do
> > something more.
> >
> > Say we can still use acpi_table_parse_entries_array() and convert
> > acpi_parse_lapic()/acpi_parse_x2apic() to
> > acpi_subtable_proc.handler_arg and save the real valid entries via
> > the
> > parameter.
> >
> > or can we just use num_processors & disabled_cpus to check if there
> > is
> > any CPU probed when parsing LOCAL_APIC/LOCAL_X2APIC entires?
> >
>
> Hi, John,
>
> As a quick fix, I'm not going to fix the "potential issue" describes
> above because we have not seen a real problem caused by this yet.
>
> Can you please try the below patch to confirm if the problem is gone on
> your system?
> This patch falls back to the previous way as sent at
> https://lore.kernel.org/lkml/87pm4bp54z.ffs@tglx/T/
>
> thanks,
> rui
>
> From bdb45e241b4fea8a12b958e490979e96b064e43d Mon Sep 17 00:00:00 2001
> From: Zhang Rui <[email protected]>
> Date: Fri, 1 Dec 2023 15:06:34 +0800
> Subject: [PATCH] x86/acpi: Do strict X2APIC ID check only when an enabled CPU
> is enumerated via LAPIC
>
> Commit 8e9c42d776d6 ("x86/acpi: Ignore invalid x2APIC entries") does
> strict X2APIC ID check if LAPIC contains valid CPUs by checking the
> acpi_table_parse_madt() return value.
>
> This is wrong because acpi_table_parse_madt() return value only
> represents the number of legal entries parsed. For example, LAPIC entry
> with LAPIC ID 0xff is counted as a legal entry, but it doesn't describe
> a valid CPU.
>
> This causes issues on a system which has 0xff LAPIC ID in all LAPIC
> entries. Because the code does strict X2APIC IDs check and ignores most
> of the CPUs in X2APIC entries.
>
> Fix the problem by doing strict X2APIC ID check less aggressively, say
> only when an enabled CPU is enumerated via LAPIC.
>
> Fixes: 8e9c42d776d6 ("x86/acpi: Ignore invalid x2APIC entries")
> Link: https://lore.kernel.org/all/[email protected]/
> Reported-by: John Sperbeck <[email protected]>
> Signed-off-by: Zhang Rui <[email protected]>
> ---
> arch/x86/kernel/acpi/boot.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> index 1a0dd80d81ac..8cc566ce486a 100644
> --- a/arch/x86/kernel/acpi/boot.c
> +++ b/arch/x86/kernel/acpi/boot.c
> @@ -266,6 +266,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;
>
> @@ -289,9 +290,13 @@ 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 */
> - processor->processor_id, /* ACPI ID */
> - processor->lapic_flags & ACPI_MADT_ENABLED);
> + cpu = acpi_register_lapic(processor->id, /* APIC ID */
> + processor->processor_id, /* ACPI ID */
> + processor->lapic_flags & ACPI_MADT_ENABLED);
> +
> + /* Do strict X2APIC ID check only when an enabled CPU is enumerated via LAPIC */
> + if (cpu >= 0 )
> + has_lapic_cpus = true;
>
> return 0;
> }
> @@ -1134,7 +1139,6 @@ static int __init acpi_parse_madt_lapic_entries(void)
> if (!count) {
> count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC,
> acpi_parse_lapic, MAX_LOCAL_APIC);
> - has_lapic_cpus = count > 0;
> x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC,
> acpi_parse_x2apic, MAX_LOCAL_APIC);
> }
> --
> 2.34.1
>

Yes, with that patch, the problem is gone on my system. All of the
CPUs are recognized and active.

Before the patch (only one CPU active):

# cat /proc/cpuinfo | grep '^processor' | wc -l
1

With the patch (all CPUs active):

oxco8:~# cat /proc/cpuinfo | grep '^processor' | wc -l
112

2023-12-02 02:54:01

by Zhang, Rui

[permalink] [raw]
Subject: Re: [tip: x86/urgent] x86/acpi: Ignore invalid x2APIC entries

On Fri, 2023-12-01 at 12:23 -0800, Ashok Raj wrote:
> On Fri, Dec 01, 2023 at 10:08:55AM -0800, Zhang, Rui wrote:
> > On Thu, 2023-11-30 at 19:25 -0800, Ashok Raj wrote:
> > > On Thu, Nov 23, 2023 at 12:50:47PM +0000, Zhang Rui wrote:
> > > > Hi, John,
> > > >
> > > > Thanks for catching this issue.
> > > >
> > > > On Wed, 2023-11-22 at 22:19 +0000, John Sperbeck wrote:
> > > > > I have a platform with both LOCAL_APIC and LOCAL_X2APIC
> > > > > entries
> > > > > for
> > > > > each CPU.  However, the ids for the LOCAL_APIC entries are
> > > > > all
> > > > > invalid ids of 255, so they have always been skipped in
> > > > > acpi_parse_lapic()
> > > > > by this code from f3bf1dbe64b6 ("x86/acpi: Prevent LAPIC id
> > > > > 0xff
> > > > > from
> > > > > being
> > > > > accounted"):
> > > > >
> > > > >     /* Ignore invalid ID */
> > > > >     if (processor->id == 0xff)
> > > > >             return 0;
> > > > >
> > > > > With the change in this thread, the return value of 0 means
> > > > > that
> > > > > the
> > > > > 'count' variable in acpi_parse_entries_array() is
> > > > > incremented. 
> > > > > The
> > > > > positive return value means that 'has_lapic_cpus' is set,
> > > > > even
> > > > > though
> > > > > no entries were actually matched.
> > > >
> > > > So in acpi_parse_madt_lapic_entries, without this patch,
> > > > madt_proc[0].count is a positive value on this platform, right?
> > > >
> > > > This sounds like a potential issue because the following checks
> > > > to
> > > > fall
> > > > back to MPS mode can also break. (If all LOCAL_APIC entries
> > > > have
> > > > apic_id 0xff and all LOCAL_X2APIC entries have apic_id
> > > > 0xffffffff)
> > > >
> > > > >   Then, when the MADT is iterated
> > > > > with acpi_parse_x2apic(), the x2apic entries with ids less
> > > > > than
> > > > > 255
> > > > > are skipped and most of my CPUs aren't recognized.
> > >
> > > This smells wrong. If a BIOS is placing some in lapic and some in
> > > x2apic
> > > table, its really messed up.
> > >
> > > Shouldn't the kernel scan them in some priority and only consider
> > > one
> > > set of
> > > tables?
> > >
> > > Shouldn't the code stop looking once something once a type is
> > > found?
> > >
> >
> > I also want to get this clarified but there is no spec saying this.
> > And
> > instead, as mentioned in the comment, we do have something in
> > 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."
> >
> > so it is possible to enumerate CPUs from both LAPIC and X2APIC.
> >
>
> Ah, so this looks like the legacy case, old OS can atleast boot the
> APIC
> entries and not process the x2apic ones.
>
> So you can potentially have duplicates
>
> APIC = has all APIC id's < 255
> X2apic has all entries > 255 OR
>         It can contain everything, so you might need to weed out
>         duplicates?
>
That is what this patch tries to do.
Say, if we have valid CPUs in LAPIC, probe X2APIC CPUs with ID >= 255
only.

thanks,
rui

2023-12-06 06:59:19

by Andres Freund

[permalink] [raw]
Subject: Re: [tip: x86/urgent] x86/acpi: Ignore invalid x2APIC entries

Hi,

On 2023-12-01 08:31:48 +0000, Zhang, Rui wrote:
> As a quick fix, I'm not going to fix the "potential issue" describes
> above because we have not seen a real problem caused by this yet.
>
> Can you please try the below patch to confirm if the problem is gone on
> your system?
> This patch falls back to the previous way as sent at
> https://lore.kernel.org/lkml/87pm4bp54z.ffs@tglx/T/


I've just spent a couple hours bisecting why upgrading to 6.7-rc4 left me with
just a single CPU core on my dual socket workstation.


before:
[ 0.000000] Linux version 6.6.0-andres-00003-g31255e072b2e ...
...
[ 0.022960] ACPI: Using ACPI (MADT) for SMP configuration information
...
[ 0.022968] smpboot: Allowing 40 CPUs, 0 hotplug CPUs
...
[ 0.345921] smpboot: CPU0: Intel(R) Xeon(R) Gold 5215 CPU @ 2.50GHz (family: 0x6, model: 0x55, stepping: 0x7)
...
[ 0.347229] .... node #0, CPUs: #1 #2 #3 #4 #5 #6 #7 #8 #9
[ 0.349082] .... node #1, CPUs: #10 #11 #12 #13 #14 #15 #16 #17 #18 #19
[ 0.003190] smpboot: CPU 10 Converting physical 0 to logical die 1

[ 0.361053] .... node #0, CPUs: #20 #21 #22 #23 #24 #25 #26 #27 #28 #29
[ 0.363990] .... node #1, CPUs: #30 #31 #32 #33 #34 #35 #36 #37 #38 #39
...
[ 0.370886] smp: Brought up 2 nodes, 40 CPUs
[ 0.370891] smpboot: Max logical packages: 2
[ 0.370896] smpboot: Total of 40 processors activated (200000.00 BogoMIPS)
[ 0.403905] node 0 deferred pages initialised in 32ms
[ 0.408865] node 1 deferred pages initialised in 37ms


after:
[ 0.000000] Linux version 6.6.0-andres-00004-gec9aedb2aa1a ...
...
[ 0.022935] ACPI: Using ACPI (MADT) for SMP configuration information
...
[ 0.022942] smpboot: Allowing 1 CPUs, 0 hotplug CPUs
...
[ 0.356424] smpboot: CPU0: Intel(R) Xeon(R) Gold 5215 CPU @ 2.50GHz (family: 0x6, model: 0x55, stepping: 0x7)
...
[ 0.357098] smp: Bringing up secondary CPUs ...
[ 0.357107] smp: Brought up 2 nodes, 1 CPU
[ 0.357108] smpboot: Max logical packages: 1
[ 0.357110] smpboot: Total of 1 processors activated (5000.00 BogoMIPS)
[ 0.726283] node 0 deferred pages initialised in 368ms
[ 0.774704] node 1 deferred pages initialised in 418ms


There does seem to be something off with the ACPI data, when booting without
the patch, I do see messages like:
[ 0.715228] APIC: NR_CPUS/possible_cpus limit of 40 reached. Processor 40/0x7f00 ignored.
[ 0.715231] ACPI: Unable to map lapic to logical cpu number

But other than that, the system has worked for a couple years.


It's obviously not good to regress from 2x10/20 cores/threads to a single
core. I guess it's at least somewhat funny to imagine a 2 socket system with
a single core...


It seems particularly worrying that this patch has apparently been selected
for -stable:
https://lore.kernel.org/all/[email protected]/

Even if it didn't have these unintended consequences, it seems like a commit
like this hardly is -stable material?


I've attached .config, dmesg of a boot with gec9aedb2aa1a and one with
gec9aedb2aa1a^.

Greetings,

Andres Freund


Attachments:
(No filename) (3.11 kB)
dmesg-6.7-ec9aedb2aa1a-onecpu (177.70 kB)
dmesg-6.7-ec9aedb2aa1a^-onecpu (177.69 kB)
.config (171.87 kB)
Download all attachments

2023-12-07 02:41:45

by Zhang, Rui

[permalink] [raw]
Subject: Re: [tip: x86/urgent] x86/acpi: Ignore invalid x2APIC entries

Hi, Andres,

On Tue, 2023-12-05 at 22:58 -0800, Andres Freund wrote:
> Hi,
>
> On 2023-12-01 08:31:48 +0000, Zhang, Rui wrote:
> > As a quick fix, I'm not going to fix the "potential issue"
> > describes
> > above because we have not seen a real problem caused by this yet.
> >
> > Can you please try the below patch to confirm if the problem is
> > gone on
> > your system?
> > This patch falls back to the previous way as sent at
> > https://lore.kernel.org/lkml/87pm4bp54z.ffs@tglx/T/
>
>
> I've just spent a couple hours bisecting why upgrading to 6.7-rc4
> left me with
> just a single CPU core on my dual socket workstation.
>
>
> before:
> [    0.000000] Linux version 6.6.0-andres-00003-g31255e072b2e ...
> ...
> [    0.022960] ACPI: Using ACPI (MADT) for SMP configuration
> information
> ...
> [    0.022968] smpboot: Allowing 40 CPUs, 0 hotplug CPUs
> ...
> [    0.345921] smpboot: CPU0: Intel(R) Xeon(R) Gold 5215 CPU @
> 2.50GHz (family: 0x6, model: 0x55, stepping: 0x7)
> ...
> [    0.347229] .... node  #0, CPUs:        #1  #2  #3  #4  #5  #6 
> #7  #8  #9
> [    0.349082] .... node  #1, CPUs:   #10 #11 #12 #13 #14 #15 #16 #17
> #18 #19
> [    0.003190] smpboot: CPU 10 Converting physical 0 to logical die 1
>
> [    0.361053] .... node  #0, CPUs:   #20 #21 #22 #23 #24 #25 #26 #27
> #28 #29
> [    0.363990] .... node  #1, CPUs:   #30 #31 #32 #33 #34 #35 #36 #37
> #38 #39
> ...
> [    0.370886] smp: Brought up 2 nodes, 40 CPUs
> [    0.370891] smpboot: Max logical packages: 2
> [    0.370896] smpboot: Total of 40 processors activated (200000.00
> BogoMIPS)
> [    0.403905] node 0 deferred pages initialised in 32ms
> [    0.408865] node 1 deferred pages initialised in 37ms
>
>
> after:
> [    0.000000] Linux version 6.6.0-andres-00004-gec9aedb2aa1a ...
> ...
> [    0.022935] ACPI: Using ACPI (MADT) for SMP configuration
> information
> ...
> [    0.022942] smpboot: Allowing 1 CPUs, 0 hotplug CPUs
> ...
> [    0.356424] smpboot: CPU0: Intel(R) Xeon(R) Gold 5215 CPU @
> 2.50GHz (family: 0x6, model: 0x55, stepping: 0x7)
> ...
> [    0.357098] smp: Bringing up secondary CPUs ...
> [    0.357107] smp: Brought up 2 nodes, 1 CPU
> [    0.357108] smpboot: Max logical packages: 1
> [    0.357110] smpboot: Total of 1 processors activated (5000.00
> BogoMIPS)
> [    0.726283] node 0 deferred pages initialised in 368ms
> [    0.774704] node 1 deferred pages initialised in 418ms
>
>
> There does seem to be something off with the ACPI data, when booting
> without
> the patch,

which patch are you referring to? the original patch in this thread?

Does the second patch fixes the problem? I mean the patch at
https://lore.kernel.org/all/[email protected]/

thanks,
rui


> I do see messages like:
> [    0.715228] APIC: NR_CPUS/possible_cpus limit of 40 reached.
> Processor 40/0x7f00 ignored.
> [    0.715231] ACPI: Unable to map lapic to logical cpu number
>
> But other than that, the system has worked for a couple years.
>
>
> It's obviously not good to regress from 2x10/20 cores/threads to a
> single
> core.   I guess it's at least somewhat funny to imagine a 2 socket
> system with
> a single core...
>
>
> It seems particularly worrying that this patch has apparently been
> selected
> for -stable:
> https://lore.kernel.org/all/[email protected]/
>
> Even if it didn't have these unintended consequences, it seems like a
> commit
> like this hardly is -stable material?
>
>
> I've attached .config, dmesg of a boot with gec9aedb2aa1a and one
> with
> gec9aedb2aa1a^.
>
> Greetings,
>
> Andres Freund

2023-12-07 05:11:11

by Andres Freund

[permalink] [raw]
Subject: Re: [tip: x86/urgent] x86/acpi: Ignore invalid x2APIC entries

Hi,

On 2023-12-07 02:41:34 +0000, Zhang, Rui wrote:
> On Tue, 2023-12-05 at 22:58 -0800, Andres Freund wrote:
> > Hi,
> >
> > On 2023-12-01 08:31:48 +0000, Zhang, Rui wrote:
> > > As a quick fix, I'm not going to fix the "potential issue"
> > > describes
> > > above because we have not seen a real problem caused by this yet.
> > >
> > > Can you please try the below patch to confirm if the problem is
> > > gone on
> > > your system?
> > > This patch falls back to the previous way as sent at
> > > https://lore.kernel.org/lkml/87pm4bp54z.ffs@tglx/T/
> >
> >
> > I've just spent a couple hours bisecting why upgrading to 6.7-rc4
> > left me with
> > just a single CPU core on my dual socket workstation.
> >
> >
> > before:
> > [??? 0.000000] Linux version 6.6.0-andres-00003-g31255e072b2e ...
> > ...
> > [??? 0.022960] ACPI: Using ACPI (MADT) for SMP configuration
> > information
> > ...
> > [??? 0.022968] smpboot: Allowing 40 CPUs, 0 hotplug CPUs
> > ...
> > [??? 0.345921] smpboot: CPU0: Intel(R) Xeon(R) Gold 5215 CPU @
> > 2.50GHz (family: 0x6, model: 0x55, stepping: 0x7)
> > ...
> > [??? 0.347229] .... node? #0, CPUs:??????? #1? #2? #3? #4? #5? #6?
> > #7? #8? #9
> > [??? 0.349082] .... node? #1, CPUs:?? #10 #11 #12 #13 #14 #15 #16 #17
> > #18 #19
> > [??? 0.003190] smpboot: CPU 10 Converting physical 0 to logical die 1
> >
> > [??? 0.361053] .... node? #0, CPUs:?? #20 #21 #22 #23 #24 #25 #26 #27
> > #28 #29
> > [??? 0.363990] .... node? #1, CPUs:?? #30 #31 #32 #33 #34 #35 #36 #37
> > #38 #39
> > ...
> > [??? 0.370886] smp: Brought up 2 nodes, 40 CPUs
> > [??? 0.370891] smpboot: Max logical packages: 2
> > [??? 0.370896] smpboot: Total of 40 processors activated (200000.00
> > BogoMIPS)
> > [??? 0.403905] node 0 deferred pages initialised in 32ms
> > [??? 0.408865] node 1 deferred pages initialised in 37ms
> >
> >
> > after:
> > [??? 0.000000] Linux version 6.6.0-andres-00004-gec9aedb2aa1a ...
> > ...
> > [??? 0.022935] ACPI: Using ACPI (MADT) for SMP configuration
> > information
> > ...
> > [??? 0.022942] smpboot: Allowing 1 CPUs, 0 hotplug CPUs
> > ...
> > [??? 0.356424] smpboot: CPU0: Intel(R) Xeon(R) Gold 5215 CPU @
> > 2.50GHz (family: 0x6, model: 0x55, stepping: 0x7)
> > ...
> > [??? 0.357098] smp: Bringing up secondary CPUs ...
> > [??? 0.357107] smp: Brought up 2 nodes, 1 CPU
> > [??? 0.357108] smpboot: Max logical packages: 1
> > [??? 0.357110] smpboot: Total of 1 processors activated (5000.00
> > BogoMIPS)
> > [??? 0.726283] node 0 deferred pages initialised in 368ms
> > [??? 0.774704] node 1 deferred pages initialised in 418ms
> >
> >
> > There does seem to be something off with the ACPI data, when booting
> > without
> > the patch,
>
> which patch are you referring to? the original patch in this thread?

Yes, the the original patch / the state in 6.7-rc4.


> Does the second patch fixes the problem? I mean the patch at
> https://lore.kernel.org/all/[email protected]/

Yes.

Greetings,

Andres Freund

Subject: Re: [tip: x86/urgent] x86/acpi: Ignore invalid x2APIC entries

[TLDR: I'm adding this report to the list of tracked Linux kernel
regressions; the text you find below is based on a few templates
paragraphs you might have encountered already in similar form.
See link in footer if these mails annoy you.]

On 22.11.23 23:19, John Sperbeck wrote:
> I have a platform with both LOCAL_APIC and LOCAL_X2APIC entries for
> each CPU. However, the ids for the LOCAL_APIC entries are all
> invalid ids of 255, so they have always been skipped in acpi_parse_lapic()
> by this code from f3bf1dbe64b6 ("x86/acpi: Prevent LAPIC id 0xff from being
> accounted"):
>
> /* Ignore invalid ID */
> if (processor->id == 0xff)
> return 0;
>
> With the change in this thread, the return value of 0 means that the
> 'count' variable in acpi_parse_entries_array() is incremented. The
> positive return value means that 'has_lapic_cpus' is set, even though
> no entries were actually matched. Then, when the MADT is iterated
> with acpi_parse_x2apic(), the x2apic entries with ids less than 255
> are skipped and most of my CPUs aren't recognized.
>
> I think the original version of this change was okay for this case in
> https://lore.kernel.org/lkml/87pm4bp54z.ffs@tglx/T/
>
> P.S. I could be convinced that the MADT for my platform is somewhat
> ill-formed and that I'm relying on pre-existing behavior. I'm not
> well-versed enough in the topic to know for sure.

To be sure the issue doesn't fall through the cracks unnoticed, I'm
adding it to regzbot, the Linux kernel regression tracking bot:

#regzbot ^introduced 8e9c42d776d6
#regzbot title x86/acpi: CPU core aren't recognized
#regzbot ignore-activity

This isn't a regression? This issue or a fix for it are already
discussed somewhere else? It was fixed already? You want to clarify when
the regression started to happen? Or point out I got the title or
something else totally wrong? Then just reply and tell me -- ideally
while also telling regzbot about it, as explained by the page listed in
the footer of this mail.

Developers: When fixing the issue, remember to add 'Link:' tags pointing
to the report (the parent of this mail). See page linked in footer for
details.

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.

2023-12-12 17:35:00

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [tip: x86/urgent] x86/acpi: Ignore invalid x2APIC entries

On Thu, Nov 23 2023 at 12:50, Rui Zhang wrote:
> On Wed, 2023-11-22 at 22:19 +0000, John Sperbeck wrote:
>> I have a platform with both LOCAL_APIC and LOCAL_X2APIC entries for
>> each CPU.  However, the ids for the LOCAL_APIC entries are all
>> invalid ids of 255, so they have always been skipped in
>> acpi_parse_lapic()
>> by this code from f3bf1dbe64b6 ("x86/acpi: Prevent LAPIC id 0xff from
>> being
>> accounted"):
>>
>>     /* Ignore invalid ID */
>>     if (processor->id == 0xff)
>>             return 0;
>>
>> With the change in this thread, the return value of 0 means that the
>> 'count' variable in acpi_parse_entries_array() is incremented.  The
>> positive return value means that 'has_lapic_cpus' is set, even though
>> no entries were actually matched.
>
> So in acpi_parse_madt_lapic_entries, without this patch,
> madt_proc[0].count is a positive value on this platform, right?
>
> This sounds like a potential issue because the following checks to fall
> back to MPS mode can also break. (If all LOCAL_APIC entries have
> apic_id 0xff and all LOCAL_X2APIC entries have apic_id 0xffffffff)
>
>>   Then, when the MADT is iterated
>> with acpi_parse_x2apic(), the x2apic entries with ids less than 255
>> are skipped and most of my CPUs aren't recognized.
>>
>> I think the original version of this change was okay for this case in
>> https://lore.kernel.org/lkml/87pm4bp54z.ffs@tglx/T/
>
> Yeah.
>
> But if we want to fix the potential issue above, we need to do
> something more.
>
> Say we can still use acpi_table_parse_entries_array() and convert
> acpi_parse_lapic()/acpi_parse_x2apic() to
> acpi_subtable_proc.handler_arg and save the real valid entries via the
> parameter.

Nah.

> or can we just use num_processors & disabled_cpus to check if there is
> any CPU probed when parsing LOCAL_APIC/LOCAL_X2APIC entires?

No, we are not going to do that because that's just a proliferation of
boundary violations.

Let ACPI deal with it's own problems and not depend on something which
is subject to change.

The simple change below should do the trick.

Thanks,

tglx
---
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 1a0dd80d81ac..85a3ce2a3666 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -293,6 +293,7 @@ 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;
}

@@ -1134,7 +1135,6 @@ static int __init acpi_parse_madt_lapic_entries(void)
if (!count) {
count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC,
acpi_parse_lapic, MAX_LOCAL_APIC);
- has_lapic_cpus = count > 0;
x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC,
acpi_parse_x2apic, MAX_LOCAL_APIC);
}


2023-12-13 07:39:12

by Zhang, Rui

[permalink] [raw]
Subject: Re: [tip: x86/urgent] x86/acpi: Ignore invalid x2APIC entries

>
> The simple change below should do the trick.
>
Yeah, I agree.

I have posted a patch to do more strict check
https://lore.kernel.org/all/[email protected]/
in case there are some weird cases that LAPIC fails to probe any
enabled CPU and we also lose the X2APIC cpus.

Either of the patches should fix these two regressions reported.

thanks,
rui

> Thanks,
>
>         tglx
> ---
> diff --git a/arch/x86/kernel/acpi/boot.c
> b/arch/x86/kernel/acpi/boot.c
> index 1a0dd80d81ac..85a3ce2a3666 100644
> --- a/arch/x86/kernel/acpi/boot.c
> +++ b/arch/x86/kernel/acpi/boot.c
> @@ -293,6 +293,7 @@ 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;
>  }
>  
> @@ -1134,7 +1135,6 @@ static int __init
> acpi_parse_madt_lapic_entries(void)
>         if (!count) {
>                 count =
> acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC,
>                                         acpi_parse_lapic,
> MAX_LOCAL_APIC);
> -               has_lapic_cpus = count > 0;
>                 x2count =
> acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC,
>                                         acpi_parse_x2apic,
> MAX_LOCAL_APIC);
>         }
>
>

2023-12-13 14:52:12

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [tip: x86/urgent] x86/acpi: Ignore invalid x2APIC entries

On Wed, Dec 13 2023 at 07:39, Zhang, Rui wrote:
> Yeah, I agree.
>
> I have posted a patch to do more strict check
> https://lore.kernel.org/all/[email protected]/
> in case there are some weird cases that LAPIC fails to probe any
> enabled CPU and we also lose the X2APIC cpus.

The return value of acpi_register_lapic() is not really useful.

It returns an error if

1) the number of registered CPUs reached the limit.
2) the APIC entry is not enabled

#1: any further X2APIC CPU will be ignored

#2: the return value is bogus as the CPU is accounted for as disabled
and will eventually lead to #1

In fact even 'disabled' entries are valid as they can be brought
in later (that's what "physical" hotplug uses)

The topology evaluation rework gets rid of this return value completely,
so I really don't want to add an dependency on it.

Thanks,

tglx



2023-12-14 15:01:33

by Zhang, Rui

[permalink] [raw]
Subject: Re: [tip: x86/urgent] x86/acpi: Ignore invalid x2APIC entries

On Wed, 2023-12-13 at 15:51 +0100, Thomas Gleixner wrote:
> On Wed, Dec 13 2023 at 07:39, Zhang, Rui wrote:
> > Yeah, I agree.
> >
> > I have posted a patch to do more strict check
> > https://lore.kernel.org/all/[email protected]/
> > in case there are some weird cases that LAPIC fails to probe any
> > enabled CPU and we also lose the X2APIC cpus.
>
> The return value of acpi_register_lapic() is not really useful.
>
> It returns an error if
>
>   1) the number of registered CPUs reached the limit.
>   2) the APIC entry is not enabled
>
> #1: any further X2APIC CPU will be ignored
>
> #2: the return value is bogus as the CPU is accounted for as disabled
>     and will eventually lead to #1
>
>     In fact even 'disabled' entries are valid as they can be brought
>     in later (that's what "physical" hotplug uses)

Agreed.

>
> The topology evaluation rework gets rid of this return value
> completely,
> so I really don't want to add an dependency on it.

Great to know that the topology evaluation rework is still in your
plan. We have tested the latest version and it indeed solves some real
issues we observed, and I'm willing to test if we have a new version
posted.

thanks,
rui

2023-12-14 21:12:03

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [tip: x86/urgent] x86/acpi: Ignore invalid x2APIC entries

On Thu, Dec 14 2023 at 15:00, Zhang, Rui wrote:
> On Wed, 2023-12-13 at 15:51 +0100, Thomas Gleixner wrote:
>> The topology evaluation rework gets rid of this return value
>> completely,
>> so I really don't want to add an dependency on it.
>
> Great to know that the topology evaluation rework is still in your
> plan. We have tested the latest version and it indeed solves some real
> issues we observed, and I'm willing to test if we have a new version
> posted.

If I ever get a breather :)

2023-12-15 14:19:47

by Thomas Gleixner

[permalink] [raw]
Subject: [PATCH] x86/acpi: Handle bogus MADT APIC tables gracefully

The recent fix to ignore invalid x2APIC entries inadvertently broke systems
with creative MADT APIC tables. The affected systems have APIC MADT tables
where all entries have invalid APIC IDs (0xFF), which means they register
exactly zero CPUs.

But the condition to ignore the entries of APIC IDs < 255 in the X2APIC
MADT table is solely based on the count of MADT APIC table entries.

As a consequence the affected machines enumerate no secondary CPUs at
all because the APIC table has entries and therefore the X2APIC table
entries with APIC IDs < 255 are ignored.

Change the condition so that the APIC table preference for APIC IDs <
255 only becomes effective when the APIC table has valid APIC ID
entries. IOW a APIC table full of invalid APIC IDs is considered to be
empty which in consequence enables the X2APIC table entries with a APIC
ID < 255 and restores the expected behaviour.

Fixes: ec9aedb2aa1a ("x86/acpi: Ignore invalid x2APIC entries")
Reported-by: John Sperbeck <[email protected]>
Reported-by: Andres Freund <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Cc: [email protected]
---
arch/x86/kernel/acpi/boot.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -293,6 +293,7 @@ acpi_parse_lapic(union acpi_subtable_hea
processor->processor_id, /* ACPI ID */
processor->lapic_flags & ACPI_MADT_ENABLED);

+ has_lapic_cpus = true;
return 0;
}

@@ -1134,7 +1135,6 @@ static int __init acpi_parse_madt_lapic_
if (!count) {
count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC,
acpi_parse_lapic, MAX_LOCAL_APIC);
- has_lapic_cpus = count > 0;
x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC,
acpi_parse_x2apic, MAX_LOCAL_APIC);
}

2023-12-15 14:20:38

by Andres Freund

[permalink] [raw]
Subject: Re: [tip: x86/urgent] x86/acpi: Ignore invalid x2APIC entries

Hi,

On 2023-12-12 18:34:48 +0100, Thomas Gleixner wrote:
> The simple change below should do the trick.

Yep, can confirm that that suffices to boot with all CPUs brought up.

Greetings,

Andres Freund