| Message ID | 20221228114558.3504-1-kvijayab@amd.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/acpi/boot: Do not register processors that cannot be onlined for x2apic | expand |
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@alien8.de> > Signed-off-by: Kishon Vijay Abraham I <kvijayab@amd.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; > > /*
On Fri, Dec 30, 2022 at 2:23 PM Zhang, Rui <rui.zhang@intel.com> wrote: > > 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 ACPI 6.5 is out even. > > 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()? I'm not sure if this matters in practice, because SAPIC is only used on IA64 anyway. Tony, what do you think? > > 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. Agreed.
>> A dumb question, the Local SAPIC structure also uses the Local APIC >> flags, and should we add the same check in acpi_parse_sapic()? > > I'm not sure if this matters in practice, because SAPIC is only used > on IA64 anyway. > > Tony, what do you think? I'm not maintaining IA64 anymore. If this change is only about saving a small amount of memory for "impossible" CPUs, then it probably isn't worth the churn. -Tony
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; /*
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. 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. Make sure Linux parses both "enabled" and "online capable" flags for x2APIC to correctly determine if the processor is usable. Fixes: 7237d3de78ff ("x86, ACPI: add support for x2apic ACPI extensions") Reviewed-by: Borislav Petkov (AMD) <bp@alien8.de> Signed-off-by: Kishon Vijay Abraham I <kvijayab@amd.com> --- arch/x86/kernel/acpi/boot.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)