[PATCH v3] x86/ACPI: Ignore CPUs that are not online capable for x2apic, entries as well

[PATCH v3] x86/ACPI: Ignore CPUs that are not online capable for x2apic, entries as well

[James Puthukattukaran]

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;
 
-- 
2.31.1

Re: [PATCH v3] x86/ACPI: Ignore CPUs that are not online capable for x2apic, entries as well

[James Puthukattukaran]

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;
>  

Re: [PATCH v3] x86/ACPI: Ignore CPUs that are not online capable for x2apic, entries as well

[Borislav Petkov]

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...

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v3] x86/ACPI: Ignore CPUs that are not online capable for x2apic, entries as well

[Rafael J. Wysocki]

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;
>
> --

Re: [PATCH v3] x86/ACPI: Ignore CPUs that are not online capable for x2apic, entries as well

[Rafael J. Wysocki]

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.