| Message ID | 20230913163823.7880-17-james.morse@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | ACPI/arm64: add support for virtual cpuhotplug | expand |
On Wed, 13 Sep 2023 16:38:04 +0000 James Morse <james.morse@arm.com> wrote: > ACPI has two descriptions of CPUs, one in the MADT/APIC table, the other > in the DSDT. Both are required. (ACPI 6.5's 8.4 "Declaring Processors" > says "Each processor in the system must be declared in the ACPI > namespace"). Having two descriptions allows firmware authors to get > this wrong. > > If CPUs are described in the MADT/APIC, they will be brought online > early during boot. Once the register_cpu() calls are moved to ACPI, > they will be based on the DSDT description of the CPUs. When CPUs are > missing from the DSDT description, they will end up online, but not > registered. > > Add a helper that runs after acpi_init() has completed to register > CPUs that are online, but weren't found in the DSDT. Any CPU that > is registered by this code triggers a firmware-bug warning and kernel > taint. > > Qemu TCG only describes the first CPU in the DSDT, unless cpu-hotplug > is configured. We should fix that as who likes warnings and taint :) I dread to think how common this will turn out to be. > > Signed-off-by: James Morse <james.morse@arm.com> LGTM Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/acpi/acpi_processor.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c > index b4bde78121bb..a01e315aa16a 100644 > --- a/drivers/acpi/acpi_processor.c > +++ b/drivers/acpi/acpi_processor.c > @@ -790,6 +790,25 @@ void __init acpi_processor_init(void) > acpi_pcc_cpufreq_init(); > } > > +static int __init acpi_processor_register_missing_cpus(void) > +{ > + int cpu; > + > + if (acpi_disabled) > + return 0; > + > + for_each_online_cpu(cpu) { > + if (!get_cpu_device(cpu)) { > + pr_err_once(FW_BUG "CPU %u has no ACPI namespace description!\n", cpu); > + add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK); > + arch_register_cpu(cpu); > + } > + } > + > + return 0; > +} > +subsys_initcall_sync(acpi_processor_register_missing_cpus); > + > #ifdef CONFIG_ACPI_PROCESSOR_CSTATE > /** > * acpi_processor_claim_cst_control - Request _CST control from the platform.
On 9/14/23 02:38, James Morse wrote: > ACPI has two descriptions of CPUs, one in the MADT/APIC table, the other > in the DSDT. Both are required. (ACPI 6.5's 8.4 "Declaring Processors" > says "Each processor in the system must be declared in the ACPI > namespace"). Having two descriptions allows firmware authors to get > this wrong. > > If CPUs are described in the MADT/APIC, they will be brought online > early during boot. Once the register_cpu() calls are moved to ACPI, > they will be based on the DSDT description of the CPUs. When CPUs are > missing from the DSDT description, they will end up online, but not > registered. > > Add a helper that runs after acpi_init() has completed to register > CPUs that are online, but weren't found in the DSDT. Any CPU that > is registered by this code triggers a firmware-bug warning and kernel > taint. > > Qemu TCG only describes the first CPU in the DSDT, unless cpu-hotplug > is configured. > > Signed-off-by: James Morse <james.morse@arm.com> > --- > drivers/acpi/acpi_processor.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > Reviewed-by: Gavin Shan <gshan@redhat.com> > diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c > index b4bde78121bb..a01e315aa16a 100644 > --- a/drivers/acpi/acpi_processor.c > +++ b/drivers/acpi/acpi_processor.c > @@ -790,6 +790,25 @@ void __init acpi_processor_init(void) > acpi_pcc_cpufreq_init(); > } > > +static int __init acpi_processor_register_missing_cpus(void) > +{ > + int cpu; > + > + if (acpi_disabled) > + return 0; > + > + for_each_online_cpu(cpu) { > + if (!get_cpu_device(cpu)) { > + pr_err_once(FW_BUG "CPU %u has no ACPI namespace description!\n", cpu); > + add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK); > + arch_register_cpu(cpu); > + } > + } > + > + return 0; > +} > +subsys_initcall_sync(acpi_processor_register_missing_cpus); > + > #ifdef CONFIG_ACPI_PROCESSOR_CSTATE > /** > * acpi_processor_claim_cst_control - Request _CST control from the platform.
diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c index b4bde78121bb..a01e315aa16a 100644 --- a/drivers/acpi/acpi_processor.c +++ b/drivers/acpi/acpi_processor.c @@ -790,6 +790,25 @@ void __init acpi_processor_init(void) acpi_pcc_cpufreq_init(); } +static int __init acpi_processor_register_missing_cpus(void) +{ + int cpu; + + if (acpi_disabled) + return 0; + + for_each_online_cpu(cpu) { + if (!get_cpu_device(cpu)) { + pr_err_once(FW_BUG "CPU %u has no ACPI namespace description!\n", cpu); + add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK); + arch_register_cpu(cpu); + } + } + + return 0; +} +subsys_initcall_sync(acpi_processor_register_missing_cpus); + #ifdef CONFIG_ACPI_PROCESSOR_CSTATE /** * acpi_processor_claim_cst_control - Request _CST control from the platform.
ACPI has two descriptions of CPUs, one in the MADT/APIC table, the other in the DSDT. Both are required. (ACPI 6.5's 8.4 "Declaring Processors" says "Each processor in the system must be declared in the ACPI namespace"). Having two descriptions allows firmware authors to get this wrong. If CPUs are described in the MADT/APIC, they will be brought online early during boot. Once the register_cpu() calls are moved to ACPI, they will be based on the DSDT description of the CPUs. When CPUs are missing from the DSDT description, they will end up online, but not registered. Add a helper that runs after acpi_init() has completed to register CPUs that are online, but weren't found in the DSDT. Any CPU that is registered by this code triggers a firmware-bug warning and kernel taint. Qemu TCG only describes the first CPU in the DSDT, unless cpu-hotplug is configured. Signed-off-by: James Morse <james.morse@arm.com> --- drivers/acpi/acpi_processor.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)