On Thu, 15 Feb 2024 20:22:29 +0100
"Rafael J. Wysocki" <rafael@xxxxxxxxxx> wrote:
> On Wed, Jan 31, 2024 at 5:50 PM Russell King <rmk+kernel@xxxxxxxxxxxxxxx> wrote:
> >
> > From: James Morse <james.morse@xxxxxxx>
> >
> > To allow ACPI to skip the call to arch_register_cpu() when the _STA
> > value indicates the CPU can't be brought online right now, move the
> > arch_register_cpu() call into acpi_processor_get_info().
> >
> > Systems can still be booted with 'acpi=off', or not include an
> > ACPI description at all. For these, the CPUs continue to be
> > registered by cpu_dev_register_generic().
> >
> > This moves the CPU register logic back to a subsys_initcall(),
> > while the memory nodes will have been registered earlier.
> >
> > Signed-off-by: James Morse <james.morse@xxxxxxx>
> > Reviewed-by: Gavin Shan <gshan@xxxxxxxxxx>
> > Tested-by: Miguel Luis <miguel.luis@xxxxxxxxxx>
> > Tested-by: Vishnu Pajjuri <vishnu@xxxxxxxxxxxxxxxxxxxxxx>
> > Tested-by: Jianyong Wu <jianyong.wu@xxxxxxx>
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@xxxxxxxxxxxxxxx>
> > ---
> > Changes since RFC v2:
> > * Fixup comment in acpi_processor_get_info() (Gavin Shan)
> > * Add comment in cpu_dev_register_generic() (Gavin Shan)
> > ---
> > drivers/acpi/acpi_processor.c | 12 ++++++++++++
> > drivers/base/cpu.c | 6 +++++-
> > 2 files changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> > index cf7c1cca69dd..a68c475cdea5 100644
> > --- a/drivers/acpi/acpi_processor.c
> > +++ b/drivers/acpi/acpi_processor.c
> > @@ -314,6 +314,18 @@ static int acpi_processor_get_info(struct acpi_device *device)
> > cpufreq_add_device("acpi-cpufreq");
> > }
> >
> > + /*
> > + * Register CPUs that are present. get_cpu_device() is used to skip
> > + * duplicate CPU descriptions from firmware.
> > + */
> > + if (!invalid_logical_cpuid(pr->id) && cpu_present(pr->id) &&
> > + !get_cpu_device(pr->id)) {
> > + int ret = arch_register_cpu(pr->id);
> > +
> > + if (ret)
> > + return ret;
> > + }
> > +
> > /*
> > * Extra Processor objects may be enumerated on MP systems with
> > * less than the max # of CPUs. They should be ignored _iff
>
> This is interesting, because right below there is the following code:
>
> if (invalid_logical_cpuid(pr->id) || !cpu_present(pr->id)) {
> int ret = acpi_processor_hotadd_init(pr);
>
> if (ret)
> return ret;
> }
>
> and acpi_processor_hotadd_init() essentially calls arch_register_cpu()
> with some extra things around it (more about that below).
>
> I do realize that acpi_processor_hotadd_init() is defined under
> CONFIG_ACPI_HOTPLUG_CPU, so for the sake of the argument let's
> consider an architecture where CONFIG_ACPI_HOTPLUG_CPU is set.
>
> So why are the two conditionals that almost contradict each other both
> needed? It looks like the new code could be combined with
> acpi_processor_hotadd_init() to do the right thing in all cases.
I jumped on to the end of this series to look at this as the two legs
look more similar at that point. I'll figure out how to drive
any changes through the series once the end goal is clear.
To make testing easy I made the acpi_process_make_enabled() look as
much like acpi_process_make_present() as possible.
>
> Now, acpi_processor_hotadd_init() does some extra things that look
> like they should be done by the new code too.
>
> 1. It checks invalid_phys_cpuid() which appears to be a good idea to me.
Indeed that is sensible. Not sure there is a path to here where it fails,
but defense in depth is good.
>
> 2. It uses locking around arch_register_cpu() which doesn't seem
> unreasonable either.
Seems reasonable, though exactly what this protecting is unclear to me
- is the arch_register_cpu() and/or the acpi_map_cpu().
Whilst it would be nice to be sure, appears harmless, so let us
take it for consistency if nothing else.
The cpu_maps_update_begin()/end() calls though aren't necessary as
we aren't touching the cpu_present or cpu_online masks.
>
> 3. It calls acpi_map_cpu() and I'm not sure why this is not done by
> the new code.
Doesn't exist except on x86 and longarch as Russell mentioned. So let's
see what it does (on x86) So we are into the realm of interfaces that
look generic but really aren't :( I particularly like the
generic_processor_info() which isn't particularly generic.
1. cpu = acpi_register_lapic()
Docs say: Register a local apic and generates a logic cpu number
2. generic_processor_info() in arch/x86/kernel/acpi/acpi.c
Checks against nr_cpus_ids - maybe that bit is useful
Allocate_logical_cpuid().
Digging in, it seems to do similar to setting __cpu_logical_map on arm64.
That's done in acpi_map_gic_cpu_interface, which happens when MADT is
parsed and I believe it's one of the the things we need to do whether
or not the CPU is enabled at boot. So already done.
acpi_processor_set_pdc() -- configure _PDC support (which I'd never heard
of before now). Deprecated in ACPI 3.0. Given we are using stuff only added
in 6.5 we can probably skip that even if it would be harmless.
acpi_map_cpu2node() -- evalulate _PXM and set __apicid_to_node[]
entry. That is only used from x86 code. Not sure what equivalent would be.
Also numa_set_node(cpu, nid); Which again sounds a lot more generic than
it is. Load of x86 specific stuff + set_cpu_numa_node() which is generic
and for ARM64 (and anything using CONFIG_GENERIC_ARCH_NUMA) is called
by numa_store_cpu_info() either from early_map_cpu_to_node() or smp_prepare_cpus()
which is called for_each_possible_cpu() and hence has already been done.
So conclusion on this one is there doesn't seem to be anything to do.
We could provide a __weak function or an ARM64 specific one that does
nothing or gate it on an appropriate config variable. However, given
I presume 'future' ARM64 support for CPU hotplug will want to do something
in these calls, perhaps a better bet is to pass a bool into the function
to indicate these should be skipped if present is not changing.
Having done that, we end up with code that is messy enough we are
better off keeping them as separate functions, though they may
look a little more similar than in this version.
There is a final thing in here you didn't mention
setting pr->flags.need_hotplug_init
which causes extra stuff to occur in processor_driver.c
The extra stuff doesn't seem to be necessary for the enable case
despite being needed for change of present status.
I haven't figured this bit out yet (I need to mess around on x86
to understand what goes wrong if you don't use that flag).
>
> The only thing that can be dropped from it is the _STA check AFAICS,
> because acpi_processor_add() won't even be called if the CPU is not
> present (and not enabled after the first patch).
>
> So why does the code not do 1 - 3 above?
I agree with 1 and 2, reasoning for 3 given above.
>
> > diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
> > index 47de0f140ba6..13d052bf13f4 100644
> > --- a/drivers/base/cpu.c
> > +++ b/drivers/base/cpu.c
> > @@ -553,7 +553,11 @@ static void __init cpu_dev_register_generic(void)
> > {
> > int i, ret;
> >
> > - if (!IS_ENABLED(CONFIG_GENERIC_CPU_DEVICES))
> > + /*
> > + * When ACPI is enabled, CPUs are registered via
> > + * acpi_processor_get_info().
> > + */
> > + if (!IS_ENABLED(CONFIG_GENERIC_CPU_DEVICES) || !acpi_disabled)
> > return;
>
> Honestly, this looks like a quick hack to me and it absolutely
> requires an ACK from the x86 maintainers to go anywhere.
Will address this separately.
>
> >
> > for_each_present_cpu(i) {
> > --
|