Arch: Arm: SMP: Boot & Voting Refactor #90895
Conversation
|
Hello @urvashivs11, and thank you very much for your first pull request to the Zephyr project! |
github-actions
bot
requested review from
carlocaione,
ithinuel,
JiafeiPan,
npitre,
povergoing,
SgrrZhf and
wearyzen
There was a problem hiding this comment.
Firstly, you say:
However, with the above change(1) allowing more flexible core
configurations, logic_id values may now exceed this limit. This breaks
the assumption that the voting array size is bounded by
CONFIG_MP_MAX_NUM_CPUS. To resolve this limitation, the voting mechanism
should be modified to use CPU_NUM instead of logic_id. This ensures that
voting remains consistent, scalable, and accurate, regardless of the
number of entries in cpu_node_list.
The very problem this voting mechanism is meant to solve consists in the
selection of a single CPU that will 1) pick the CPU number 0 for itself
and 2) assign CPU numbers to the other CPUs. This means CPU numbers aren't
available before the election mechanism is complete and therefore can't be
used by the election mechanism.
Secondly, you have:
ldr \xreg2, =cpu_node_list_length
ldr \xreg4, [\xreg2]
This is needlessly enlarging the macro register footprint. Please consider
something like this instead:
ldr \xreg2, =cpu_node_list_length
ldr \xreg2, [\xreg2]
And lastly, please avoid merge commits in your pull requests. You're
encouraged to rebase your branch on top of the latest upstream main branch
instead.
urvashivs11
force-pushed
the
main
branch
3 times, most recently
from
03a4e45 to
dc206d0
Compare
There was a problem hiding this comment.
-
You should update the comment above the
get_cpu_logic_idmacro
definition. It currently says
"xreg1: logic id (0 ~ CONFIG_MP_MAX_NUM_CPUS - 1)". -
You still need to explain why you're playing with
BOOT_PARAM_CPU_NUM_OFFSET. That part still makes no sense to me.
The "ldr x2, [x0, #BOOT_PARAM_CPU_NUM_OFFSET]" you added is overwriting
the actual CPU number returned byget_cpu_logic_idwith some memory
content that has not been initialized yet.
|
Hi, Sorry for the delay in response. Thank you for your feedback. Sure, I will update the comment in
To explain the second change, let me present one of the usecase that I have. Given this device combination, the cpu_node_list would be structured as follows: Assume
The Problem During boot, we iterate through each cluster to determine which cores can be brought out of reset. In this example, without any change in voting mechanism, the
This logic ID ( The Solution To address this, I propose using the
This approach ensures that the index used for voting remains within the bounds of the voting array. I’d also like to highlight that we support a variety of device configurations, where cores from any cluster can be fused off—not necessarily booting from a specific cluster. This means the boot core can vary depending on the hardware configuration. To ensure robustness, I’ve validated the proposed logic across multiple combinations of fused clusters and boot scenarios. In all tested cases, the approach of using cpu_num instead of logic_id for voting has worked reliably and as expected. That said, I’m open to suggestions—if you have a more efficient or elegant approach to handle this use case, I’d be glad to consider and implement it. Thank you. |
Right. And that is what needs fixing.
No. That's where you may be confused. The voting mechanism is there to let only one CPU sequentially enumerate the Anyway, in that scenario, the If CPUs are brought out of reset sequentially then they don't need any Now the actual fix should probably look like this: |
urvashivs11
force-pushed
the
main
branch
2 times, most recently
from
49a1b2a to
7b09b19
Compare
|
Thank you for the insightful explanation. It really helped clarify the purpose and behavior of the voting mechanism during simultaneous CPU reset. I now understand why my earlier solution appeared to work — it was due to CPUs being brought out of reset sequentially, which bypassed the race condition that the voting mechanism is designed to handle. I’ve implemented the fix you suggested, using Appreciate your guidance and support! |
|
Looks fine now... except for the commit log that needs updating.
|
|
Thank you for pointing out. Updated the commit log as per the fix.
|
Support booting from any usable core in systems with partially fused-off CPUs. Update get_cpu_logic_id to iterate over the actual number of enabled CPUs using DT_CHILD_NUM_STATUS_OKAY(DT_PATH(cpus)) instead of CONFIG_MP_MAX_NUM_CPUS. Resize the voting[] array based on DT_CHILD_NUM_STATUS_OKAY to ensure each CPU can vote correctly. Signed-off-by: urvashi sharma <[email protected]>
|
|
Thank you for the review and guidance. |
|
|
Thanks for the response. |
|
Hi @urvashivs11! To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge. Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁 |
To support boot-up from parts where usable cores are limited to subset of total cores in HW we expect the boot/primary core to configured to any of the usable clusterXcoreN. For example, in a system with 3 clusters, each having 6 cores, if on a particular device the Cluster0Core0 and Cluster0Core1 are non-functional due to issues like silicon yield defects or other hardware-related faults, booting from Cluster0Core2 may be necessary. To accommodate various cluster configurations, all the core's MPID must be explicitly listed in the cpu_node_list (all 18 cores for the system mentioned in example above). However, currently, the get_cpu_logic_id function assumes that CONFIG_MP_MAX_NUM_CPUS will always match to the length of cpu_node_list.
To correctly iterate over all MPIDs in cpu_node_list, this assumption needs to be revised. The logic should be updated to dynamically iterate through the all the entries in cpu_node_list.
Required change:
Update get_cpu_logic_id Macro: Instead of comparing xreg1 (iterator) with CONFIG_MP_MAX_NUM_CPUS, compare it with the length of cpu_node_list updated in smp.c. This ensures the search covers the entire cpu_node_list to find cpu_logic_id.
As a result of the change described above, the voting mechanism also requires an update. Currently, voting relies on the logic_id obtained from get_cpu_logic_id, which is derived from the index of the MPID in the cpu_node_list. This approach assumes that logic_id values do not exceed CONFIG_MP_MAX_NUM_CPUS.
However, with the above change(1) allowing more flexible core configurations, logic_id values may now exceed this limit. This breaks the assumption that the voting array size is bounded by CONFIG_MP_MAX_NUM_CPUS.
To resolve this limitation, the voting mechanism should be modified to use CPU_NUM instead of logic_id. This ensures that voting remains consistent, scalable, and accurate, regardless of the number of entries in cpu_node_list.
Required change:
Update to boot_params Structure : To make CPU_NUM accessible during the early boot phase (e.g., in reset.s), it must be moved above the voting[CONFIG_MP_MAX_NUM_CPUS] array within the boot_params structure. Since the size of the voting array is variable and depends on CONFIG_MP_MAX_NUM_CPUS, placing CPU_NUM above it guarantees a fixed offset. This allows reliable access using the newly defined BOOT_PARAM_CPU_NUM_OFFSET in boot.h.