Re: [PATCH] arm64: of: handle multiple threads in ARM cpu node

[Mark Rutland <mark.rutland@arm.com>]

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@huawei.com>
> ---
>  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
> 
>