On Fri, Jan 10, 2025 at 04:10:57PM +0000, Alireza Sanaee wrote:
> Update `of_parse_and_init_cpus` to parse reg property of CPU node as
> an array based as per spec for SMT threads.
>
> Spec v0.4 Section 3.8.1:
Which spec, and why do we care?
> The value of reg is a <prop-encoded-**array**> that defines a unique
> CPU/thread id for the CPU/threads represented by the CPU node. **If a CPU
> supports more than one thread (i.e. multiple streams of execution) the
> reg property is an array with 1 element per thread**. The address-cells
> on the /cpus node specifies how many cells each element of the array
> takes. Software can determine the number of threads by dividing the size
> of reg by the parent node's address-cells.
We already have systems where each thread gets a unique CPU node under
/cpus, so we can't rely on this to determine the topology.
Further, there are bindings which rely on being able to address each
CPU/thread with a unique phandle (e.g. for affinity of PMU interrupts),
which this would break.
> An accurate example of 1 core with 2 SMTs:
>
> cpus {
> #size-cells = <0x00>;
> #address-cells = <0x01>;
>
> cpu@0 {
> phandle = <0x8000>;
> **reg = <0x00 0x01>;**
> enable-method = "psci";
> compatible = "arm,cortex-a57";
> device_type = "cpu";
> };
> };
>
> Instead of:
>
> cpus {
> #size-cells = <0x00>;
> #address-cells = <0x01>;
>
> cpu@0 {
> phandle = <0x8000>;
> reg = <0x00>;
> enable-method = "psci";
> compatible = "arm,cortex-a57";
> device_type = "cpu";
> };
>
> cpu@1 {
> phandle = <0x8001>;
> reg = <0x01>;
> enable-method = "psci";
> compatible = "arm,cortex-a57";
> device_type = "cpu";
> };
> };
>
> which is **NOT** accurate.
It might not follow "the spec" you reference (and haven't named), but I
think it's a stretch to say it's inaccurate.
Regardless, as above I do not think this is a good idea. While it allows
the DT to be written in a marginally simpler way, it makes things more
complicated for the kernel and is incompatible with bindings that we
already support.
If anything "the spec" should be relaxed here.
Mark.
>
> Signed-off-by: Alireza Sanaee <alireza.sanaee@xxxxxxxxxx>
> ---
> arch/arm64/kernel/smp.c | 74 +++++++++++++++++++++++------------------
> 1 file changed, 41 insertions(+), 33 deletions(-)
>
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index 3b3f6b56e733..8dd3b3c82967 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -689,53 +689,61 @@ static void __init acpi_parse_and_init_cpus(void)
> static void __init of_parse_and_init_cpus(void)
> {
> struct device_node *dn;
> + u64 hwid;
> + u32 tid;
>
> for_each_of_cpu_node(dn) {
> - u64 hwid = of_get_cpu_hwid(dn, 0);
> + tid = 0;
>
> - if (hwid & ~MPIDR_HWID_BITMASK)
> - goto next;
> + while (1) {
> + hwid = of_get_cpu_hwid(dn, tid++);
> + if (hwid == ~0ULL)
> + break;
>
> - if (is_mpidr_duplicate(cpu_count, hwid)) {
> - pr_err("%pOF: duplicate cpu reg properties in the DT\n",
> - dn);
> - goto next;
> - }
> + if (hwid & ~MPIDR_HWID_BITMASK)
> + goto next;
>
> - /*
> - * The numbering scheme requires that the boot CPU
> - * must be assigned logical id 0. Record it so that
> - * the logical map built from DT is validated and can
> - * be used.
> - */
> - if (hwid == cpu_logical_map(0)) {
> - if (bootcpu_valid) {
> - pr_err("%pOF: duplicate boot cpu reg property in DT\n",
> - dn);
> + if (is_mpidr_duplicate(cpu_count, hwid)) {
> + pr_err("%pOF: duplicate cpu reg properties in the DT\n",
> + dn);
> goto next;
> }
>
> - bootcpu_valid = true;
> - early_map_cpu_to_node(0, of_node_to_nid(dn));
> -
> /*
> - * cpu_logical_map has already been
> - * initialized and the boot cpu doesn't need
> - * the enable-method so continue without
> - * incrementing cpu.
> + * The numbering scheme requires that the boot CPU
> + * must be assigned logical id 0. Record it so that
> + * the logical map built from DT is validated and can
> + * be used.
> */
> - continue;
> - }
> + if (hwid == cpu_logical_map(0)) {
> + if (bootcpu_valid) {
> + pr_err("%pOF: duplicate boot cpu reg property in DT\n",
> + dn);
> + goto next;
> + }
> +
> + bootcpu_valid = true;
> + early_map_cpu_to_node(0, of_node_to_nid(dn));
> +
> + /*
> + * cpu_logical_map has already been
> + * initialized and the boot cpu doesn't need
> + * the enable-method so continue without
> + * incrementing cpu.
> + */
> + continue;
> + }
>
> - if (cpu_count >= NR_CPUS)
> - goto next;
> + if (cpu_count >= NR_CPUS)
> + goto next;
>
> - pr_debug("cpu logical map 0x%llx\n", hwid);
> - set_cpu_logical_map(cpu_count, hwid);
> + pr_debug("cpu logical map 0x%llx\n", hwid);
> + set_cpu_logical_map(cpu_count, hwid);
>
> - early_map_cpu_to_node(cpu_count, of_node_to_nid(dn));
> + early_map_cpu_to_node(cpu_count, of_node_to_nid(dn));
> next:
> - cpu_count++;
> + cpu_count++;
> + }
> }
> }
>
> --
> 2.43.0
>
>
