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