linux-kernel - Re: [PATCH] x86/acpi/boot: Do not register processors that cannot be onlined for x2apic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0e299876606352ca868d7b71991c66a1c1d361cb.camel@intel.com>
Date:   Fri, 30 Dec 2022 13:23:01 +0000
From:   "Zhang, Rui" <rui.zhang@...el.com>
To:     "Brown, Len" <len.brown@...el.com>,
        "kvijayab@....com" <kvijayab@....com>,
        "rafael@...nel.org" <rafael@...nel.org>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "pavel@....cz" <pavel@....cz>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "Mario.Limonciello@....com" <Mario.Limonciello@....com>,
        "bp@...en8.de" <bp@...en8.de>
CC:     "hpa@...or.com" <hpa@...or.com>,
        "suravee.suthikulpanit@....com" <suravee.suthikulpanit@....com>,
        "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
        "bpetkov@....com" <bpetkov@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        "santosh.shukla@....com" <santosh.shukla@....com>
Subject: Re: [PATCH] x86/acpi/boot: Do not register processors that cannot be
 onlined for x2apic

Hi, Kishon,

On Wed, 2022-12-28 at 11:45 +0000, Kishon Vijay Abraham I wrote:
> Section 5.2.12.12 Processor Local x2APIC Structure in the ACPI v6.3
> spec mandates that both "enabled" and "online capable" Local APIC
> Flags
> should be used to determine if the processor is usable or not.

ACPI spec 6.4 is released, so better to refer to the latest ACPI spec,
say,
https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/05_ACPI_Software_Programming_Model/ACPI_Software_Programming_Model.html#processor-local-x2apic-structure
or
https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/05_ACPI_Software_Programming_Model/ACPI_Software_Programming_Model.html#local-apic-flags

> However, Linux doesn't use the "online capable" flag for x2APIC to
> determine if the processor is usable. As a result, cpu_possible_mask
> has incorrect value and results in more memory getting allocated for
> per_cpu variables than it is going to be used.

Thanks for catching this. I had the same question when I was reading
this piece of code recently.

> Make sure Linux parses both "enabled" and "online capable" flags for
> x2APIC to correctly determine if the processor is usable.

A dumb question, the Local SAPIC structure also uses the Local APIC
flags, and should we add the same check in acpi_parse_sapic()?

> Fixes: 7237d3de78ff ("x86, ACPI: add support for x2apic ACPI
> extensions")

I'm not sure if this "Fixes" tag is accurate or not.

Checking for the Local APIC flags was just added last year, by commit
aa06e20f1be6 ("x86/ACPI: Don't add CPUs that are not online capable"),
and the variable 'acpi_support_online_capable' used in this patch is
also introduced by that commit. So, to me, this patch fixes a gap in aa
06e20f1be6, rather than the original x2apic support commit.

thanks,
rui

> Reviewed-by: Borislav Petkov (AMD) <bp@...en8.de>
> Signed-off-by: Kishon Vijay Abraham I <kvijayab@....com>
> ---
>  arch/x86/kernel/acpi/boot.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/acpi/boot.c
> b/arch/x86/kernel/acpi/boot.c
> index 907cc98b1938..518bda50068c 100644
> --- a/arch/x86/kernel/acpi/boot.c
> +++ b/arch/x86/kernel/acpi/boot.c
> @@ -188,6 +188,17 @@ static int acpi_register_lapic(int id, u32
> acpiid, u8 enabled)
>  	return cpu;
>  }
>  
> +static bool __init acpi_is_processor_usable(u32 lapic_flags)
> +{
> +	if (lapic_flags & ACPI_MADT_ENABLED)
> +		return true;
> +
> +	if (acpi_support_online_capable && (lapic_flags &
> ACPI_MADT_ONLINE_CAPABLE))
> +		return true;
> +
> +	return false;
> +}
> +
>  static int __init
>  acpi_parse_x2apic(union acpi_subtable_headers *header, const
> unsigned long end)
>  {
> @@ -212,6 +223,10 @@ acpi_parse_x2apic(union acpi_subtable_headers
> *header, const unsigned long end)
>  	if (apic_id == 0xffffffff)
>  		return 0;
>  
> +	/* don't register processors that cannot be onlined */
> +	if (!acpi_is_processor_usable(processor->lapic_flags))
> +		return 0;
> +
>  	/*
>  	 * We need to register disabled CPU as well to permit
>  	 * counting disabled CPUs. This allows us to size
> @@ -250,9 +265,7 @@ acpi_parse_lapic(union acpi_subtable_headers *
> header, const unsigned long end)
>  		return 0;
>  
>  	/* don't register processors that can not be onlined */
> -	if (acpi_support_online_capable &&
> -	    !(processor->lapic_flags & ACPI_MADT_ENABLED) &&
> -	    !(processor->lapic_flags & ACPI_MADT_ONLINE_CAPABLE))
> +	if (!acpi_is_processor_usable(processor->lapic_flags))
>  		return 0;
>  
>  	/*