[PATCH v4 0/1] ARM Sbsa-ref: Enable CPU cluster topology

[PATCH v4 0/1] ARM Sbsa-ref: Enable CPU cluster topology

[Xiong Yining]

Enable CPU cluster support on SbsaQemu platform, so that users can
specify a 4-level CPU hierarchy sockets/clusters/cores/threads. And this
topology can be passed to the firmware through DT cpu-map.

Signed-off-by: Xiong Yining <xiongyining1480@phytium.com.cn>

---
Changes in v4:
- align the machine-version-minor to 4

Changes in v3:
- squash the two patches together into one
- add the DTB information in docs/system/arm/sbsa.rst
- Link to v2: https://lists.gnu.org/archive/html/qemu-devel/2024-03/msg02951.html

Changes in v2:
- put this code before sbsa_fdt_add_gic_node().
- Link to v1: https://lists.gnu.org/archive/html/qemu-devel/2023-12/msg03390.html

xiongyining1480 (1):
  hw/arm/sbsa-ref: Enable CPU cluster on ARM sbsa machine

 docs/system/arm/sbsa.rst |  4 ++++
 hw/arm/sbsa-ref.c        | 37 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 40 insertions(+), 1 deletion(-)

-- 
2.34.1

[PATCH v4 1/1] hw/arm/sbsa-ref: Enable CPU cluster on ARM sbsa machine

[Xiong Yining]

From: xiongyining1480 <xiongyining1480@phytium.com.cn>

Enable CPU cluster support on SbsaQemu platform, so that users can
specify a 4-level CPU hierarchy sockets/clusters/cores/threads. And
this topology can be passed to the firmware through DT cpu-map.

Signed-off-by: Xiong Yining <xiongyining1480@phytium.com.cn>
tested-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
---
 docs/system/arm/sbsa.rst |  4 ++++
 hw/arm/sbsa-ref.c        | 37 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/docs/system/arm/sbsa.rst b/docs/system/arm/sbsa.rst
index 2bf22a1d0b..783b87cad7 100644
--- a/docs/system/arm/sbsa.rst
+++ b/docs/system/arm/sbsa.rst
@@ -62,6 +62,7 @@ The devicetree reports:
    - platform version
    - GIC addresses
    - NUMA node id for CPUs and memory
+   - CPU topology information
 
 Platform version
 ''''''''''''''''
@@ -88,3 +89,6 @@ Platform version changes:
 
 0.3
   The USB controller is an XHCI device, not EHCI.
+
+0.4
+  CPU topology information is present in devicetree
diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index f5709d6c14..0c2ebe0417 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -210,7 +210,7 @@ static void create_fdt(SBSAMachineState *sms)
      *                        fw compatibility.
      */
     qemu_fdt_setprop_cell(fdt, "/", "machine-version-major", 0);
-    qemu_fdt_setprop_cell(fdt, "/", "machine-version-minor", 3);
+    qemu_fdt_setprop_cell(fdt, "/", "machine-version-minor", 4);
 
     if (ms->numa_state->have_numa_distance) {
         int size = nb_numa_nodes * nb_numa_nodes * 3 * sizeof(uint32_t);
@@ -264,9 +264,43 @@ static void create_fdt(SBSAMachineState *sms)
                 ms->possible_cpus->cpus[cs->cpu_index].props.node_id);
         }
 
+        qemu_fdt_setprop_cell(sms->fdt, nodename, "phandle",
+                        qemu_fdt_alloc_phandle(sms->fdt));
+
         g_free(nodename);
     }
 
+    /*
+     * Add vCPU topology description through fdt node cpu-map.
+     * See fdt_add_cpu_nodes() on hw/arm/virt.c for longer description.
+     */
+    qemu_fdt_add_subnode(sms->fdt, "/cpus/cpu-map");
+
+    for (cpu = sms->smp_cpus - 1; cpu >= 0; cpu--) {
+        char *cpu_path = g_strdup_printf("/cpus/cpu@%d", cpu);
+        char *map_path;
+
+        if (ms->smp.threads > 1) {
+            map_path = g_strdup_printf(
+                "/cpus/cpu-map/socket%d/cluster%d/core%d/thread%d",
+                cpu / (ms->smp.clusters * ms->smp.cores * ms->smp.threads),
+                (cpu / (ms->smp.cores * ms->smp.threads)) % ms->smp.clusters,
+                (cpu / ms->smp.threads) % ms->smp.cores,
+                cpu % ms->smp.threads);
+        } else {
+            map_path = g_strdup_printf(
+                "/cpus/cpu-map/socket%d/cluster%d/core%d",
+                cpu / (ms->smp.clusters * ms->smp.cores),
+                (cpu / ms->smp.cores) % ms->smp.clusters,
+                cpu % ms->smp.cores);
+        }
+        qemu_fdt_add_path(sms->fdt, map_path);
+        qemu_fdt_setprop_phandle(sms->fdt, map_path, "cpu", cpu_path);
+
+        g_free(map_path);
+        g_free(cpu_path);
+    }
+
     sbsa_fdt_add_gic_node(sms);
 }
 
@@ -886,6 +920,7 @@ static void sbsa_ref_class_init(ObjectClass *oc, void *data)
     mc->default_ram_size = 1 * GiB;
     mc->default_ram_id = "sbsa-ref.ram";
     mc->default_cpus = 4;
+    mc->smp_props.clusters_supported = true;
     mc->possible_cpu_arch_ids = sbsa_ref_possible_cpu_arch_ids;
     mc->cpu_index_to_instance_props = sbsa_ref_cpu_index_to_props;
     mc->get_default_cpu_node_id = sbsa_ref_get_default_cpu_node_id;
-- 
2.34.1

Re: [PATCH v4 1/1] hw/arm/sbsa-ref: Enable CPU cluster on ARM sbsa machine

[Marcin Juszkiewicz]

W dniu 26.04.2024 o 09:35, Xiong Yining pisze:
> From: xiongyining1480<xiongyining1480@phytium.com.cn>
> 
> Enable CPU cluster support on SbsaQemu platform, so that users can
> specify a 4-level CPU hierarchy sockets/clusters/cores/threads. And
> this topology can be passed to the firmware through DT cpu-map.
> 
> Signed-off-by: Xiong Yining<xiongyining1480@phytium.com.cn>
> tested-by: Marcin Juszkiewicz<marcin.juszkiewicz@linaro.org>

Thanks. Checked with TF-A and EDK2 patches applied. PPTT table will be 
more detailed now.

Reviewed-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>

Re: [PATCH v4 1/1] hw/arm/sbsa-ref: Enable CPU cluster on ARM sbsa machine

[Richard Henderson]

On 4/26/24 00:35, Xiong Yining wrote:
> From: xiongyining1480 <xiongyining1480@phytium.com.cn>
> 
> Enable CPU cluster support on SbsaQemu platform, so that users can
> specify a 4-level CPU hierarchy sockets/clusters/cores/threads. And
> this topology can be passed to the firmware through DT cpu-map.
> 
> Signed-off-by: Xiong Yining <xiongyining1480@phytium.com.cn>
> tested-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
> ---
>   docs/system/arm/sbsa.rst |  4 ++++
>   hw/arm/sbsa-ref.c        | 37 ++++++++++++++++++++++++++++++++++++-
>   2 files changed, 40 insertions(+), 1 deletion(-)

Isn't this basically what MPIDR_EL1 is supposed to indicate?
We do not yet implement all of that in QEMU, but should.

Why does the same info need to be replicated in devicetree?


r~

Re: [PATCH v4 1/1] hw/arm/sbsa-ref: Enable CPU cluster on ARM sbsa machine

[xiongyining1480]

[-- Attachment #1: Type: text/plain, Size: 1611 bytes --]





xiongyining1480@phytium.com.cn
 
From: Richard Henderson
Date: 2024-04-27 00:06
To: Xiong Yining; qemu-arm; qemu-devel
CC: rad; peter.maydell; quic_llindhol; marcin.juszkiewicz
Subject: Re: [PATCH v4 1/1] hw/arm/sbsa-ref: Enable CPU cluster on ARM sbsa machine
On 4/26/24 00:35, Xiong Yining wrote:
> From: xiongyining1480 <xiongyining1480@phytium.com.cn>
> 
> Enable CPU cluster support on SbsaQemu platform, so that users can
> specify a 4-level CPU hierarchy sockets/clusters/cores/threads. And
> this topology can be passed to the firmware through DT cpu-map.
> 
> Signed-off-by: Xiong Yining <xiongyining1480@phytium.com.cn>
> tested-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
> ---
>   docs/system/arm/sbsa.rst |  4 ++++
>   hw/arm/sbsa-ref.c        | 37 ++++++++++++++++++++++++++++++++++++-
>   2 files changed, 40 insertions(+), 1 deletion(-)
 
> Isn't this basically what MPIDR_EL1 is supposed to indicate?
> We do not yet implement all of that in QEMU, but should.
 
> Why does the same info need to be replicated in devicetree?
 
sbsa uses PPTT to indicate the cpu topology, and OS use the ACPI to get hardware infomation. We add the related information in devicetree, and TF-A parses devicetree
and extract data form it , so EDK2 can gather data form TF-A to create PPTT tables via SMC calls. 

Now the PPTT tables created by EDK2 lose some detailed information, so the cpu topology OS identified cannot align with the qemu configure. We hope to add the topology 
information in device tree, so EKD2 can create more detailed PPTT tables.
 
> r~

[-- Attachment #2: Type: text/html, Size: 3481 bytes --]

Re: [PATCH v4 1/1] hw/arm/sbsa-ref: Enable CPU cluster on ARM sbsa machine

[Marcin Juszkiewicz]

W dniu 26.04.2024 o 18:06, Richard Henderson pisze:
> 
> Isn't this basically what MPIDR_EL1 is supposed to indicate?
> We do not yet implement all of that in QEMU, but should.

QEMU has socket/cluster/core/thread model which could map to
aff3/aff2/aff1/aff0 (or aff0/1/2/3) of MPIDR_EL1 register, right? But it 
does not.

Nevermind which combination of socket/cluster/core/thread I use all I 
have is this:

cpu 0x000 mpidr 00000000 00000000
cpu 0x001 mpidr 00000000 00000001
cpu 0x002 mpidr 00000000 00000010
cpu 0x003 mpidr 00000000 00000011
cpu 0x004 mpidr 00000000 00000100
cpu 0x005 mpidr 00000000 00000101
cpu 0x006 mpidr 00000000 00000110
cpu 0x007 mpidr 00000000 00000111

cpu 0x008 mpidr 00000001 00000000
cpu 0x009 mpidr 00000001 00000001
cpu 0x00a mpidr 00000001 00000010
cpu 0x00b mpidr 00000001 00000011
cpu 0x00c mpidr 00000001 00000100
cpu 0x00d mpidr 00000001 00000101
cpu 0x00e mpidr 00000001 00000110
cpu 0x00f mpidr 00000001 00000111

Eight cpu cores per unit. Probably leftover from GICv2 times where there 
was 8 cores per GIC limit.

So looks like adding mapping of topology to MPIDR_EL1 into QEMU would be 
a better option. May require checking for more than 256 of one kind then.

> Why does the same info need to be replicated in devicetree?

One of things we had on todolist: export cpu topology in PPTT table. 
With MPIDR being 2 level while topology can be 4 level.

Re: [PATCH v4 1/1] hw/arm/sbsa-ref: Enable CPU cluster on ARM sbsa machine

[Richard Henderson]

On 4/28/24 23:35, Marcin Juszkiewicz wrote:
> W dniu 26.04.2024 o 18:06, Richard Henderson pisze:
>>
>> Isn't this basically what MPIDR_EL1 is supposed to indicate?
>> We do not yet implement all of that in QEMU, but should.
> 
> QEMU has socket/cluster/core/thread model which could map to
> aff3/aff2/aff1/aff0 (or aff0/1/2/3) of MPIDR_EL1 register, right? But it does not.

Yes, I know, but there's patches on list that started to deal with MPIDR.SMT,

   https://lore.kernel.org/qemu-devel/20240419183135.12276-1-dorjoychy111@gmail.com/

and I suggested that we go whole hog and support all of the -smp options.


r~

Re: [PATCH v4 1/1] hw/arm/sbsa-ref: Enable CPU cluster on ARM sbsa machine

[Marcin Juszkiewicz]

W dniu 26.04.2024 o 09:35, Xiong Yining pisze:
> From: xiongyining1480<xiongyining1480@phytium.com.cn>
> 
> Enable CPU cluster support on SbsaQemu platform, so that users can
> specify a 4-level CPU hierarchy sockets/clusters/cores/threads. And
> this topology can be passed to the firmware through DT cpu-map.
> 
> Signed-off-by: Xiong Yining<xiongyining1480@phytium.com.cn>
> tested-by: Marcin Juszkiewicz<marcin.juszkiewicz@linaro.org>

I had some thinking about it recently. This patch exported whole 
/cpus/cpu-map/ tree which we then parse in TF-A to get amount of 
sockets/clusters/cores/threads.

Why not export them directly? Kind of:

         cpus {
                 topology {
                         threads = <0x01>;
                         cores = <0x04>;
                         clusters = <0x01>;
                         sockets = <0x01>;
                 };

It gives everything we need.

Had some thinking about exporting amount of cores per cluster (8 now, 
virt uses 16 which is architecture maximum now) in case we would use it 
in generation of PPTT in EDK2.

Re: [PATCH v4 1/1] hw/arm/sbsa-ref: Enable CPU cluster on ARM sbsa machine

[xiongyining1480]

[-- Attachment #1: Type: text/plain, Size: 1939 bytes --]





xiongyining1480@phytium.com.cn
 
From: Marcin Juszkiewicz
Date: 2024-05-24 02:47
To: Xiong Yining; qemu-arm; qemu-devel
CC: rad; peter.maydell; quic_llindhol
Subject: Re: [PATCH v4 1/1] hw/arm/sbsa-ref: Enable CPU cluster on ARM sbsa machine
W dniu 26.04.2024 o 09:35, Xiong Yining pisze:
> From: xiongyining1480<xiongyining1480@phytium.com.cn>
> 
> Enable CPU cluster support on SbsaQemu platform, so that users can
> specify a 4-level CPU hierarchy sockets/clusters/cores/threads. And
> this topology can be passed to the firmware through DT cpu-map.
> 
> Signed-off-by: Xiong Yining<xiongyining1480@phytium.com.cn>
> tested-by: Marcin Juszkiewicz<marcin.juszkiewicz@linaro.org>
 
>I had some thinking about it recently. This patch exported whole 
>/cpus/cpu-map/ tree which we then parse in TF-A to get amount of 
>sockets/clusters/cores/threads.
 
>Why not export them directly? Kind of:
> 
>        cpus {
>                 topology {
>                         threads = <0x01>;
>                         cores = <0x04>;
>                         clusters = <0x01>;
>                         sockets = <0x01>;
>                 };
> 
>It gives everything we need.

Fully agree.
 
>Had some thinking about exporting amount of cores per cluster (8 now, 
>virt uses 16 which is architecture maximum now) in case we would use it 
>in generation of PPTT in EDK2.

I don't fully understand "the amount of cores per cluster", does it means the user config: -smp clusters=X, cores=Y which indicates
the number of cores in one cluster is Y, or the AFF0/AFF1 in MPDIR?

The cpu topology is defined by the user on command line which PPTT want to align with.  And the value of AFF0/AFF1 is generally 
8 or 16 which will not affected by  option -smp command.   Considering that MPDIR cannot reflect the cpu topology configurated by 
user, i guess the PPTT  don't need to use the MPDIR value.


[-- Attachment #2: Type: text/html, Size: 4993 bytes --]