| Message ID | 20250503191515.24041-6-ricardo.neri-calderon@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | x86/hyperv/hv_vtl: Use a wakeup mailbox to boot secondary CPUs | expand |
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-calderon@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. 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). OTOH, as the wakeup-mailbox seems to be defined by ACPI, that seems fine to add, but I would simplify the code here to not invite more entries. Further ones should be rejected IMO. > + > +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). > + > + if (IS_ERR(enable_method)) { > + enable_method = this_em; > + dtb_register_apic_id(apic_id, dn); > + continue; > + } > + > + if (!dtb_enable_method_is_valid(enable_method, this_em)) > + continue; > + > + dtb_register_apic_id(apic_id, dn); > } > + > + if (dtb_configure_enable_method(enable_method)) > + pr_err("enable-method '%s' needed but not configured\n", enable_method); > } > > static void __init dtb_lapic_setup(void) > -- > 2.43.0 >
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-calderon@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
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 = { }; + +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); + + if (IS_ERR(enable_method)) { + enable_method = this_em; + dtb_register_apic_id(apic_id, dn); + continue; + } + + if (!dtb_enable_method_is_valid(enable_method, this_em)) + continue; + + dtb_register_apic_id(apic_id, dn); } + + if (dtb_configure_enable_method(enable_method)) + pr_err("enable-method '%s' needed but not configured\n", enable_method); } static void __init dtb_lapic_setup(void)
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-calderon@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(-)