| Message ID | 20250110161057.445-1-alireza.sanaee@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: of: handle multiple threads in ARM cpu node | expand |
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 > >
On Fri, 10 Jan 2025 16:23:00 +0000 Mark Rutland <mark.rutland@arm.com> wrote: Hi Mark, Thanks for prompt feedback. Please look inline. > 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? For the spec, this is what I looked into https://github.com/devicetree-org/devicetree-specification/releases/download/v0.4/devicetree-specification-v0.4.pdf Section 3.8.1 Sorry I didn't put the link in there. One limitation with the existing approach is that it is not really possible to describe shared caches for SMT cores as they will be seen as separate CPU cores in the device tree. Is there anyway to do so? More discussion over sharing caches for threads here https://lore.kernel.org/kvm/20241219083237.265419-1-zhao1.liu@intel.com/ > > > 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. I assume we can generate unique values even in reg array, but probably makes things more complicated. > > 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. Hi Rob, If this approach is too disruptive, then shall we fallback to the approach where go share L1 at next-level-cache entry? Thanks, Alireza > > 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 > > > > >
On Fri, Jan 10, 2025 at 05:02:11PM +0000, Alireza Sanaee wrote: > On Fri, 10 Jan 2025 16:23:00 +0000 > Mark Rutland <mark.rutland@arm.com> wrote: > > Hi Mark, > > Thanks for prompt feedback. > > Please look inline. > > > 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? > > For the spec, this is what I looked > into https://github.com/devicetree-org/devicetree-specification/releases/download/v0.4/devicetree-specification-v0.4.pdf > Section 3.8.1 > > Sorry I didn't put the link in there. Ok, so that's "The devicetree specification v0.4 from ${URL}", rather than "Spec v0.4". > One limitation with the existing approach is that it is not really > possible to describe shared caches for SMT cores as they will be seen > as separate CPU cores in the device tree. Is there anyway to do so? Can't the existing cache bindings handle that? e.g. give both threads a next-level-cache pointing to the shared L1? > More discussion over sharing caches for threads > here https://lore.kernel.org/kvm/20241219083237.265419-1-zhao1.liu@intel.com/ In that thread Rob refers to earlier discussions, so I don't think that thread alone has enough context. > > > 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. > > I assume we can generate unique values even in reg array, but probably > makes things more complicated. The other bindings use phandles to refer to threads, and phandles point to nodes in the dt, so it's necessary for threads to be given separate nodes. Note that the CPU topology bindings use that to describe threads, see Documentation/devicetree/bindings/cpu/cpu-topology.txt > > 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. > > 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. > > Hi Rob, > > If this approach is too disruptive, then shall we fallback to the > approach where go share L1 at next-level-cache entry? Ah, was that previously discussed, and were there any concerns against that approach? To be clear, my main concern here is that threads remain represented as distinct nodes under /cpus; I'm not wedded to the precise solution for representing shared caches. Mark.
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++; + } } }
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: 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. 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. Signed-off-by: Alireza Sanaee <alireza.sanaee@huawei.com> --- arch/arm64/kernel/smp.c | 74 +++++++++++++++++++++++------------------ 1 file changed, 41 insertions(+), 33 deletions(-)