From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Russell King <rmk+kernel@armlinux.org.uk>,
<linux-pm@vger.kernel.org>, <loongarch@lists.linux.dev>,
<linux-acpi@vger.kernel.org>, <linux-arch@vger.kernel.org>,
<linux-kernel@vger.kernel.org>,
<linux-arm-kernel@lists.infradead.org>,
<linux-riscv@lists.infradead.org>, <kvmarm@lists.linux.dev>,
<x86@kernel.org>, <acpica-devel@lists.linuxfoundation.org>,
<linux-csky@vger.kernel.org>, <linux-doc@vger.kernel.org>,
<linux-ia64@vger.kernel.org>, <linux-parisc@vger.kernel.org>,
Salil Mehta <salil.mehta@huawei.com>,
Jean-Philippe Brucker <jean-philippe@linaro.org>,
<jianyong.wu@arm.com>, <justin.he@arm.com>,
James Morse <james.morse@arm.com>
Subject: Re: [PATCH RFC v4 02/15] ACPI: processor: Register all CPUs from acpi_processor_get_info()
Date: Fri, 22 Mar 2024 18:53:27 +0000 [thread overview]
Message-ID: <20240322185327.00002416@Huawei.com> (raw)
In-Reply-To: <CAJZ5v0iiJpUWq5GMSnKFWQTzn_bdwoQz9m=hDaXNg4Lj_ePF4g@mail.gmail.com>
On Thu, 15 Feb 2024 20:22:29 +0100
"Rafael J. Wysocki" <rafael@kernel.org> wrote:
> On Wed, Jan 31, 2024 at 5:50 PM Russell King <rmk+kernel@armlinux.org.uk> wrote:
> >
> > From: James Morse <james.morse@arm.com>
> >
> > 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@arm.com>
> > Reviewed-by: Gavin Shan <gshan@redhat.com>
> > Tested-by: Miguel Luis <miguel.luis@oracle.com>
> > Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
> > Tested-by: Jianyong Wu <jianyong.wu@arm.com>
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> > 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) {
> > --
next prev parent reply other threads:[~2024-03-22 18:53 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-31 16:48 [RFC PATCH v4 00/15] ACPI/arm64: add support for virtual cpu hotplug Russell King (Oracle)
2024-01-31 16:49 ` [PATCH RFC v4 01/15] ACPI: Only enumerate enabled (or functional) processor devices Russell King
2024-01-31 17:25 ` Miguel Luis
2024-02-15 20:10 ` Rafael J. Wysocki
2024-02-19 9:45 ` Jonathan Cameron
2024-02-20 11:30 ` Russell King (Oracle)
2024-02-21 13:01 ` Rafael J. Wysocki
2024-01-31 16:49 ` [PATCH RFC v4 02/15] ACPI: processor: Register all CPUs from acpi_processor_get_info() Russell King
2024-02-15 19:22 ` Rafael J. Wysocki
2024-02-20 11:27 ` Russell King (Oracle)
2024-02-20 15:13 ` Russell King (Oracle)
2024-02-20 16:24 ` Jonathan Cameron
2024-02-20 19:59 ` Rafael J. Wysocki
2024-02-21 12:04 ` Rafael J. Wysocki
2024-02-20 20:59 ` Thomas Gleixner
2024-03-22 18:53 ` Jonathan Cameron [this message]
2024-04-10 12:43 ` Jonathan Cameron
2024-04-10 13:28 ` Rafael J. Wysocki
2024-04-10 13:50 ` Jonathan Cameron
2024-04-10 14:19 ` Rafael J. Wysocki
2024-04-10 15:58 ` Jonathan Cameron
2024-04-10 18:56 ` Russell King (Oracle)
2024-04-10 19:08 ` Rafael J. Wysocki
2024-04-10 21:07 ` Jonathan Cameron
2024-01-31 16:49 ` [PATCH RFC v4 03/15] ACPI: Move acpi_bus_trim_one() before acpi_scan_hot_remove() Russell King
2024-01-31 16:49 ` [PATCH RFC v4 04/15] ACPI: Rename acpi_processor_hotadd_init and remove pre-processor guards Russell King
2024-01-31 16:50 ` [PATCH RFC v4 05/15] ACPI: Add post_eject to struct acpi_scan_handler for cpu hotplug Russell King
2024-01-31 16:50 ` [PATCH RFC v4 06/15] ACPI: convert acpi_processor_post_eject() to use IS_ENABLED() Russell King (Oracle)
2024-01-31 16:50 ` [PATCH RFC v4 07/15] ACPI: Check _STA present bit before making CPUs not present Russell King
2024-01-31 16:50 ` [PATCH RFC v4 08/15] ACPI: Warn when the present bit changes but the feature is not enabled Russell King
2024-01-31 16:50 ` [PATCH RFC v4 09/15] arm64: acpi: Move get_cpu_for_acpi_id() to a header Russell King
2024-01-31 16:50 ` [PATCH RFC v4 10/15] irqchip/gic-v3: Don't return errors from gic_acpi_match_gicc() Russell King
2024-02-02 16:44 ` Jonathan Cameron
2024-01-31 16:50 ` [PATCH RFC v4 11/15] irqchip/gic-v3: Add support for ACPI's disabled but 'online capable' CPUs Russell King
2024-02-02 16:47 ` Jonathan Cameron
2024-01-31 16:50 ` [PATCH RFC v4 12/15] arm64: psci: Ignore DENIED CPUs Russell King
2024-04-11 11:35 ` Jonathan Cameron
2024-04-11 13:25 ` Jonathan Cameron
2024-01-31 16:50 ` [PATCH RFC v4 13/15] ACPI: add support to (un)register CPUs based on the _STA enabled bit Russell King
2024-01-31 16:50 ` [PATCH RFC v4 14/15] arm64: document virtual CPU hotplug's expectations Russell King
2024-02-02 17:04 ` Jonathan Cameron
2024-01-31 16:50 ` [PATCH RFC v4 15/15] cpumask: Add enabled cpumask for present CPUs that can be brought online Russell King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240322185327.00002416@Huawei.com \
--to=jonathan.cameron@huawei.com \
--cc=acpica-devel@lists.linuxfoundation.org \
--cc=james.morse@arm.com \
--cc=jean-philippe@linaro.org \
--cc=jianyong.wu@arm.com \
--cc=justin.he@arm.com \
--cc=kvmarm@lists.linux.dev \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-arch@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-csky@vger.kernel.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-ia64@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-parisc@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=loongarch@lists.linux.dev \
--cc=rafael@kernel.org \
--cc=rmk+kernel@armlinux.org.uk \
--cc=salil.mehta@huawei.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).