| Message ID | ef8c7138-8ed1-d849-0ed5-e629ddcafd63@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] x86/ACPI: Ignore CPUs that are not online capable for x2apic, entries as well | expand |
Adding others that I missed on my first email. James On 12/22/22 13:26, James Puthukattukaran wrote: > Extending commit aa06e20f1be6 ("x86/ACPI: Don't add CPUs that are not > online capable") to include acpi_parse_x2apic as well. There is a check > for invalid apicid; however, there are BIOS FW with madt version >= 5 > support that do not bother setting apic id to an invalid value since they > assume the OS will check the enabled and online capable flags. > > Signed-off-by: James Puthukattukaran<james.puthukattukaran@oracle.com> > Reported-by: Benjamin Fuller<ben.fuller@oracle.com> > > v2 : use 'enabled' local variable. Also fix checkpatch.pl catches > v3 : updates as per Rafael's comments > --- > arch/x86/kernel/acpi/boot.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c > index 907cc98b1938..cf2509f9de31 100644 > --- a/arch/x86/kernel/acpi/boot.c > +++ b/arch/x86/kernel/acpi/boot.c > @@ -208,7 +208,15 @@ acpi_parse_x2apic(union acpi_subtable_headers *header, const unsigned long end) > apic_id = processor->local_apic_id; > enabled = processor->lapic_flags & ACPI_MADT_ENABLED; > > - /* Ignore invalid ID */ > + /* don't register processors that can not be onlined */ > + if (!enabled && acpi_support_online_capable && > + !(processor->lapic_flags & ACPI_MADT_ONLINE_CAPABLE)) > + return 0; > + > + /* > + * for systems older than madt version 5 (does not have > + * ACPI_MADT_ONLINE_CAPABLE defined); ignore invalid ID > + */ > if (apic_id == 0xffffffff) > return 0; >
On Tue, Jan 10, 2023 at 05:43:41PM -0500, James Puthukattukaran wrote: > Adding others that I missed on my first email. > > James > > On 12/22/22 13:26, James Puthukattukaran wrote: > > Extending commit aa06e20f1be6 ("x86/ACPI: Don't add CPUs that are not > > online capable") to include acpi_parse_x2apic as well. This doesn't look like an extension to some existing commit but like a separate fix. > > There is a check for invalid apicid; however, there are BIOS FW with madt > > version >= 5 support that do not bother setting apic id to an invalid value > > since they assume the OS will check the enabled and online capable flags. Which BIOSes are those? Also, I'm no BIOS guy but I don't see you checking MADT version anywhere? > > Signed-off-by: James Puthukattukaran<james.puthukattukaran@oracle.com> > > Reported-by: Benjamin Fuller<ben.fuller@oracle.com> > > > > v2 : use 'enabled' local variable. Also fix checkpatch.pl catches > > v3 : updates as per Rafael's comments Yah, I'd like for Rafael to decide what to do here...
On Thu, Dec 22, 2022 at 7:26 PM James Puthukattukaran <james.puthukattukaran@oracle.com> wrote: > > Extending commit aa06e20f1be6 ("x86/ACPI: Don't add CPUs that are not > online capable") to include acpi_parse_x2apic as well. There is a check > for invalid apicid; however, there are BIOS FW with madt version >= 5 It would be good to give at least one example of a platform where this happens. > support that do not bother setting apic id to an invalid value since they > assume the OS will check the enabled and online capable flags. > > Signed-off-by: James Puthukattukaran<james.puthukattukaran@oracle.com> > Reported-by: Benjamin Fuller<ben.fuller@oracle.com> > > v2 : use 'enabled' local variable. Also fix checkpatch.pl catches > v3 : updates as per Rafael's comments > --- > arch/x86/kernel/acpi/boot.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c > index 907cc98b1938..cf2509f9de31 100644 > --- a/arch/x86/kernel/acpi/boot.c > +++ b/arch/x86/kernel/acpi/boot.c > @@ -208,7 +208,15 @@ acpi_parse_x2apic(union acpi_subtable_headers *header, const unsigned long end) > apic_id = processor->local_apic_id; > enabled = processor->lapic_flags & ACPI_MADT_ENABLED; > > - /* Ignore invalid ID */ > + /* don't register processors that can not be onlined */ > + if (!enabled && acpi_support_online_capable && > + !(processor->lapic_flags & ACPI_MADT_ONLINE_CAPABLE)) I would add a MADT version check to this, because ACPI_MADT_ONLINE_CAPABLE may be set by mistake in an older BIOS too. > + return 0; > + > + /* > + * for systems older than madt version 5 (does not have Also please spell MADT in capitals. > + * ACPI_MADT_ONLINE_CAPABLE defined); ignore invalid ID > + */ > if (apic_id == 0xffffffff) > return 0; > > --
On Wed, Jan 11, 2023 at 12:24 AM Borislav Petkov <bp@alien8.de> wrote: > > On Tue, Jan 10, 2023 at 05:43:41PM -0500, James Puthukattukaran wrote: > > Adding others that I missed on my first email. > > > > James > > > > On 12/22/22 13:26, James Puthukattukaran wrote: > > > Extending commit aa06e20f1be6 ("x86/ACPI: Don't add CPUs that are not > > > online capable") to include acpi_parse_x2apic as well. > > This doesn't look like an extension to some existing commit but like a separate > fix. > > > > There is a check for invalid apicid; however, there are BIOS FW with madt > > > version >= 5 support that do not bother setting apic id to an invalid value > > > since they assume the OS will check the enabled and online capable flags. > > Which BIOSes are those? > > Also, I'm no BIOS guy but I don't see you checking MADT version anywhere? > > > > Signed-off-by: James Puthukattukaran<james.puthukattukaran@oracle.com> > > > Reported-by: Benjamin Fuller<ben.fuller@oracle.com> > > > > > > v2 : use 'enabled' local variable. Also fix checkpatch.pl catches > > > v3 : updates as per Rafael's comments > > Yah, I'd like for Rafael to decide what to do here... I've just sent my comments to the patch, but you have not been CCed, sorry. IMO the MADT version should be checked too and I would like to have at least one example of a platform affected by this problem in the changelog.
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c index 907cc98b1938..cf2509f9de31 100644 --- a/arch/x86/kernel/acpi/boot.c +++ b/arch/x86/kernel/acpi/boot.c @@ -208,7 +208,15 @@ acpi_parse_x2apic(union acpi_subtable_headers *header, const unsigned long end) apic_id = processor->local_apic_id; enabled = processor->lapic_flags & ACPI_MADT_ENABLED; - /* Ignore invalid ID */ + /* don't register processors that can not be onlined */ + if (!enabled && acpi_support_online_capable && + !(processor->lapic_flags & ACPI_MADT_ONLINE_CAPABLE)) + return 0; + + /* + * for systems older than madt version 5 (does not have + * ACPI_MADT_ONLINE_CAPABLE defined); ignore invalid ID + */ if (apic_id == 0xffffffff) return 0;
Extending commit aa06e20f1be6 ("x86/ACPI: Don't add CPUs that are not online capable") to include acpi_parse_x2apic as well. There is a check for invalid apicid; however, there are BIOS FW with madt version >= 5 support that do not bother setting apic id to an invalid value since they assume the OS will check the enabled and online capable flags. Signed-off-by: James Puthukattukaran<james.puthukattukaran@oracle.com> Reported-by: Benjamin Fuller<ben.fuller@oracle.com> v2 : use 'enabled' local variable. Also fix checkpatch.pl catches v3 : updates as per Rafael's comments --- arch/x86/kernel/acpi/boot.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)