On Mon, May 12, 2025 at 10:54:15AM -0500, Rob Herring wrote:
> On Sat, May 03, 2025 at 12:15:07PM -0700, Ricardo Neri wrote:
> > Add functionality to parse and validate the `enable-method` property for
> > platforms that use alternative methods to wakeup secondary CPUs (e.g., a
> > wakeup mailbox).
> >
> > Most x86 platforms boot secondary CPUs using INIT assert, de-assert
> > followed by a Start-Up IPI messages. These systems do no need to specify an
> > `enable-method` property in the cpu@N nodes of the DeviceTree.
> >
> > Although it is possible to specify a different `enable-method` for each
> > secondary CPU, the existing functionality relies on using the
> > APIC wakeup_secondary_cpu{ (), _64()} callback to wake up all CPUs. Ensure
> > that either all CPUs specify the same `enable-method` or none at all.
> >
> > Signed-off-by: Ricardo Neri <ricardo.neri-calde...@linux.intel.com>
> > ---
> > Changes since v2:
> > - Introduced this patch.
> >
> > Changes since v1:
> > - N/A
> > ---
> > arch/x86/kernel/devicetree.c | 88 +++++++++++++++++++++++++++++++++++-
> > 1 file changed, 86 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kernel/devicetree.c b/arch/x86/kernel/devicetree.c
> > index dd8748c45529..5835afc74acd 100644
> > --- a/arch/x86/kernel/devicetree.c
> > +++ b/arch/x86/kernel/devicetree.c
> > @@ -127,8 +127,59 @@ static void __init dtb_setup_hpet(void)
> >
> > #ifdef CONFIG_X86_LOCAL_APIC
> >
> > +#ifdef CONFIG_SMP
> > +static const char *dtb_supported_enable_methods[] __initconst = { };
>
> If you expect this list to grow, I would say the firmware should support
> "spin-table" enable-method and let's stop the list before it starts.
Actually, I was thinking on dropping this patch altogether. It does not
seem to be needed: if there is a reserved-memory region for the mailbox,
use it. Otherwise, keep using the INIT-!INIT-SIPI messages. No need to
add extra complexity and maintainance burden with checks for an `enable-
method`.
> Look at the mess that's arm32 enable-methods... Considering you have no
> interrupts, I imagine what you have is not much different from a
> spin-table (write a jump address to an address)? Maybe it would already
> work as long as jump table is reserved (And in that case you don't need
> the compatible or any binding other than for cpu nodes).
Correct, the spin-table is similar to the ACPI mailbox but there are
differences: the latter lets you send a command to control when, if ever,
secondary CPUs are booted.
>
> OTOH, as the wakeup-mailbox seems to be defined by ACPI, that seems
> fine to add,
Yes, and Linux for x86 already supports the ACPI mailbox and that code can
be reused.
> but I would simplify the code here to not invite more
> entries. Further ones should be rejected IMO.
Unconditionally checking for the presence of mailbox works in this sense
too.
>
> > +
> > +static bool __init dtb_enable_method_is_valid(const char *enable_method_a,
> > + const char *enable_method_b)
> > +{
> > + int i;
> > +
> > + if (!enable_method_a && !enable_method_b)
> > + return true;
> > +
> > + if (strcmp(enable_method_a, enable_method_b))
> > + return false;
> > +
> > + for (i = 0; i < ARRAY_SIZE(dtb_supported_enable_methods); i++) {
> > + if (!strcmp(enable_method_a, dtb_supported_enable_methods[i]))
> > + return true;
> > + }
> > +
> > + return false;
> > +}
> > +
> > +static int __init dtb_configure_enable_method(const char *enable_method)
> > +{
> > + /* Nothing to do for a missing enable-method or if the system has one
> > CPU */
> > + if (!enable_method || IS_ERR(enable_method))
> > + return 0;
> > +
> > + return -ENOTSUPP;
> > +}
> > +#else /* !CONFIG_SMP */
> > +static inline bool dtb_enable_method_is_valid(const char *enable_method_a,
> > + const char *enable_method_b)
> > +{
> > + /* No secondary CPUs. We do not care about the enable-method. */
> > + return true;
> > +}
> > +
> > +static inline int dtb_configure_enable_method(const char *enable_method)
> > +{
> > + return 0;
> > +}
> > +#endif /* CONFIG_SMP */
> > +
> > +static void __init dtb_register_apic_id(u32 apic_id, struct device_node
> > *dn)
> > +{
> > + topology_register_apic(apic_id, CPU_ACPIID_INVALID, true);
> > + set_apicid_to_node(apic_id, of_node_to_nid(dn));
> > +}
> > +
> > static void __init dtb_cpu_setup(void)
> > {
> > + const char *enable_method = ERR_PTR(-EINVAL), *this_em;
> > struct device_node *dn;
> > u32 apic_id;
> >
> > @@ -138,9 +189,42 @@ static void __init dtb_cpu_setup(void)
> > pr_warn("%pOF: missing local APIC ID\n", dn);
> > continue;
> > }
> > - topology_register_apic(apic_id, CPU_ACPIID_INVALID, true);
> > - set_apicid_to_node(apic_id, of_node_to_nid(dn));
> > +
> > + /*
> > + * Also check the enable-method of the secondary CPUs, if
> > present.
> > + *
> > + * Systems that use the INIT-!INIT-StartUp IPI sequence to boot
> > + * secondary CPUs do not need to define an enable-method.
> > + *
> > + * All CPUs must have the same enable-method. The enable-method
> > + * must be supported. If absent in one secondary CPU, it must be
> > + * absent for all CPUs.
> > + *
> > + * Compare the first secondary CPU with the rest. We do not care
> > + * about the boot CPU, as it is enabled already.
> > + */
> > +
> > + if (apic_id == boot_cpu_physical_apicid) {
> > + dtb_register_apic_id(apic_id, dn);
> > + continue;
> > + }
> > +
> > + this_em = of_get_property(dn, "enable-method", NULL);
>
> Use typed accessors. of_property_match_string() would be good here.
> There's some desire to avoid more of_property_read_string() calls too
> because that leaks un-refcounted DT data to the caller (really only an
> issue in overlays).
Thanks for this information! However, I plan to scrap this code and
unconditionally use the mailbox if detected.
I would still like to get your inputs on the next submission with updated
code to use the mailbox if you agree.
BR,
Ricardo
