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

Alireza Sanaee
2025-01-10T17:02:20Z

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
> >
> >
>