[PATCH 0/5] hw/cpu/cluster: Restrict CPU cluster to a particular CPU type

[PATCH 0/5] hw/cpu/cluster: Restrict CPU cluster to a particular CPU type

[Philippe Mathieu-Daudé]

Following Alex's comment [1] on "Introduce hybrid CPU topology"
RFC [2], this series strengthen a bit the CPU cluster by
restricting it to a particular CPU type.

We'd rather have a single way of creating heterogeneous (hybrid)
CPU topology. Note the CPU cluster is not user-creatable, so few
more work is required in this area.

Based-on: <20230216122524.67212-1-philmd@linaro.org>
  [3] "Have object_child_foreach() take Error* and return boolean"

[1] https://lore.kernel.org/qemu-devel/87y1p1c18a.fsf@linaro.org/
[2] https://lore.kernel.org/qemu-devel/20230213095035.158240-1-zhao1.liu@linux.intel.com/
[3] https://lore.kernel.org/qemu-devel/20230216122524.67212-1-philmd@linaro.org/

Philippe Mathieu-Daudé (5):
  hw/cpu: Extend CPUState::cluster_index documentation
  hw/cpu/cluster: Only add CPU objects to CPU cluster
  hw/cpu/cluster: Restrict CPU cluster to a particular CPU type
  hw/arm: Restrict CPU clusters to the expected type
  hw/riscv: Restrict CPU clusters to the expected type

 accel/tcg/tcg-accel-ops.c  | 13 ++++++++++++-
 hw/arm/armsse.c            |  1 +
 hw/arm/xlnx-zynqmp.c       |  4 ++++
 hw/cpu/cluster.c           | 33 +++++++++++++++++++++++++++------
 hw/riscv/microchip_pfsoc.c |  4 ++++
 hw/riscv/sifive_u.c        |  2 ++
 include/hw/core/cpu.h      |  2 ++
 include/hw/cpu/cluster.h   |  1 +
 8 files changed, 53 insertions(+), 7 deletions(-)

-- 
2.38.1

[PATCH 1/5] hw/cpu: Extend CPUState::cluster_index documentation

[Philippe Mathieu-Daudé]

Copy part of the description of commit f7b78602fd ("accel/tcg:
Add cluster number to TCG TB hash") in tcg_cpu_init_cflags(),
improving a bit CPUState::cluster_index documentation.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 accel/tcg/tcg-accel-ops.c | 13 ++++++++++++-
 include/hw/core/cpu.h     |  2 ++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
index 19cbf1db3a..654aeec04c 100644
--- a/accel/tcg/tcg-accel-ops.c
+++ b/accel/tcg/tcg-accel-ops.c
@@ -44,7 +44,18 @@
 
 void tcg_cpu_init_cflags(CPUState *cpu, bool parallel)
 {
-    uint32_t cflags = cpu->cluster_index << CF_CLUSTER_SHIFT;
+    uint32_t cflags;
+
+    /*
+     * Include the cluster number in the hash we use to look up TBs.
+     * This is important because a TB that is valid for one cluster at
+     * a given physical address and set of CPU flags is not necessarily
+     * valid for another:
+     * the two clusters may have different views of physical memory, or
+     * may have different CPU features (eg FPU present or absent).
+     */
+    cflags = cpu->cluster_index << CF_CLUSTER_SHIFT;
+
     cflags |= parallel ? CF_PARALLEL : 0;
     cflags |= icount_enabled() ? CF_USE_ICOUNT : 0;
     cpu->tcg_cflags = cflags;
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 2417597236..d427db0bc7 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -272,6 +272,8 @@ struct qemu_work_item;
  *   to a cluster this will be UNASSIGNED_CLUSTER_INDEX; otherwise it will
  *   be the same as the cluster-id property of the CPU object's TYPE_CPU_CLUSTER
  *   QOM parent.
+ *   Under TCG this value is propagated to @tcg_cflags.
+ *   See TranslationBlock::TCG CF_CLUSTER_MASK.
  * @tcg_cflags: Pre-computed cflags for this cpu.
  * @nr_cores: Number of cores within this CPU package.
  * @nr_threads: Number of threads within this CPU.
-- 
2.38.1

Re: [PATCH 1/5] hw/cpu: Extend CPUState::cluster_index documentation

[Peter Maydell]

On Thu, 16 Feb 2023 at 14:23, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Copy part of the description of commit f7b78602fd ("accel/tcg:
> Add cluster number to TCG TB hash") in tcg_cpu_init_cflags(),
> improving a bit CPUState::cluster_index documentation.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 2/5] hw/cpu/cluster: Only add CPU objects to CPU cluster

[Philippe Mathieu-Daudé]

Do not recursively add CPU and all their children objects.
Simply iterate on the cluster direct children, which must
be of TYPE_CPU. Otherwise raise an error.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/cpu/cluster.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/hw/cpu/cluster.c b/hw/cpu/cluster.c
index bf3e27e945..b0cdf7d931 100644
--- a/hw/cpu/cluster.c
+++ b/hw/cpu/cluster.c
@@ -39,12 +39,19 @@ typedef struct CallbackData {
 static bool add_cpu_to_cluster(Object *obj, void *opaque, Error **errp)
 {
     CallbackData *cbdata = opaque;
-    CPUState *cpu = (CPUState *)object_dynamic_cast(obj, TYPE_CPU);
+    CPUState *cpu;
 
-    if (cpu) {
-        cpu->cluster_index = cbdata->cluster->cluster_id;
-        cbdata->cpu_count++;
+    cpu = (CPUState *)object_dynamic_cast(obj, TYPE_CPU);
+    if (!cpu) {
+        error_setg(errp, "cluster %s can only accept CPU types (got %s)",
+                   object_get_canonical_path(OBJECT(cbdata->cluster)),
+                   object_get_typename(obj));
+        return false;
     }
+
+    cpu->cluster_index = cbdata->cluster->cluster_id;
+    cbdata->cpu_count++;
+
     return true;
 }
 
@@ -63,8 +70,9 @@ static void cpu_cluster_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    object_child_foreach_recursive(cluster_obj, add_cpu_to_cluster,
-                                   &cbdata, NULL);
+    if (!object_child_foreach(cluster_obj, add_cpu_to_cluster, &cbdata, errp)) {
+        return;
+    }
 
     /*
      * A cluster with no CPUs is a bug in the board/SoC code that created it;
-- 
2.38.1

Re: [PATCH 2/5] hw/cpu/cluster: Only add CPU objects to CPU cluster

[Peter Maydell]

On Thu, 16 Feb 2023 at 14:23, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Do not recursively add CPU and all their children objects.
> Simply iterate on the cluster direct children, which must
> be of TYPE_CPU. Otherwise raise an error.

The documentation in include/hw/cpu/cluster.h says:

 * The CPUs may be either direct children of the cluster object, or indirect
 * children (e.g. children of children of the cluster object).

If we want to change that we need to update the documentation too.

I'm not sure why this doesn't hit the error on the armsse.c
use of TYPE_CLUSTER -- there the objects we have to put in
the cluster are TYPE_ARMV7M, which are not themselves
TYPE_CPU. They're a container which contains a TYPE_CPU.
This is why the docs say that it's OK to have the CPU
be an indirect child. I think one of the riscv boards
may be also using this facility, but I'm less sure there.

thanks
-- PMM

[PATCH 3/5] hw/cpu/cluster: Restrict CPU cluster to a particular CPU type

[Philippe Mathieu-Daudé]

CPU cluster id is used by TCG accelerator to "group" CPUs
sharing the same ISA features, so TranslationBlock can be
shared between the cluster (see commit f7b78602fd "accel/tcg:
Add cluster number to TCG TB hash"). This mean we shouldn't
mix different kind of CPUs into the same cluster.

Enforce that by adding a 'cpu-type' property. The cluster's
realize() method will check all children are of that 'cpu-type'
class.

If the property is not set, the first CPU added to a cluster
sets its CPU type, and only that type fo CPU can be added.

Example of error:

  qemu-system-aarch64: cluster /machine/soc/rpu-cluster can only accept cortex-r5f-arm-cpu CPUs (got cortex-a9-arm-cpu)

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/cpu/cluster.c         | 19 ++++++++++++++++---
 include/hw/cpu/cluster.h |  1 +
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/hw/cpu/cluster.c b/hw/cpu/cluster.c
index b0cdf7d931..0d06944824 100644
--- a/hw/cpu/cluster.c
+++ b/hw/cpu/cluster.c
@@ -28,6 +28,7 @@
 
 static Property cpu_cluster_properties[] = {
     DEFINE_PROP_UINT32("cluster-id", CPUClusterState, cluster_id, 0),
+    DEFINE_PROP_STRING("cpu-type", CPUClusterState, cpu_type),
     DEFINE_PROP_END_OF_LIST()
 };
 
@@ -41,11 +42,17 @@ static bool add_cpu_to_cluster(Object *obj, void *opaque, Error **errp)
     CallbackData *cbdata = opaque;
     CPUState *cpu;
 
-    cpu = (CPUState *)object_dynamic_cast(obj, TYPE_CPU);
+    if (cbdata->cluster->cpu_type == NULL) {
+        /* If no 'cpu-type' property set, enforce it with the first CPU added */
+        assert(object_dynamic_cast(obj, TYPE_CPU) != NULL);
+        cbdata->cluster->cpu_type = g_strdup(object_get_typename(obj));
+    }
+
+    cpu = (CPUState *)object_dynamic_cast(obj, cbdata->cluster->cpu_type);
     if (!cpu) {
-        error_setg(errp, "cluster %s can only accept CPU types (got %s)",
+        error_setg(errp, "cluster %s can only accept %s CPUs (got %s)",
                    object_get_canonical_path(OBJECT(cbdata->cluster)),
-                   object_get_typename(obj));
+                   cbdata->cluster->cpu_type, object_get_typename(obj));
         return false;
     }
 
@@ -69,6 +76,12 @@ static void cpu_cluster_realize(DeviceState *dev, Error **errp)
         error_setg(errp, "cluster-id must be less than %d", MAX_CLUSTERS);
         return;
     }
+    if (cluster->cpu_type) {
+        if (object_class_is_abstract(object_class_by_name(cluster->cpu_type))) {
+            error_setg(errp, "cpu-type must be a concrete class");
+            return;
+        }
+    }
 
     if (!object_child_foreach(cluster_obj, add_cpu_to_cluster, &cbdata, errp)) {
         return;
diff --git a/include/hw/cpu/cluster.h b/include/hw/cpu/cluster.h
index 53fbf36af5..c9792d6f05 100644
--- a/include/hw/cpu/cluster.h
+++ b/include/hw/cpu/cluster.h
@@ -76,6 +76,7 @@ struct CPUClusterState {
 
     /*< public >*/
     uint32_t cluster_id;
+    char *cpu_type;
 };
 
 #endif
-- 
2.38.1

Re: [PATCH 3/5] hw/cpu/cluster: Restrict CPU cluster to a particular CPU type

[Peter Maydell]

On Thu, 16 Feb 2023 at 14:23, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> CPU cluster id is used by TCG accelerator to "group" CPUs
> sharing the same ISA features, so TranslationBlock can be
> shared between the cluster (see commit f7b78602fd "accel/tcg:
> Add cluster number to TCG TB hash"). This mean we shouldn't
> mix different kind of CPUs into the same cluster.
>
> Enforce that by adding a 'cpu-type' property. The cluster's
> realize() method will check all children are of that 'cpu-type'
> class.
>
> If the property is not set, the first CPU added to a cluster
> sets its CPU type, and only that type fo CPU can be added.

This seems like a reasonable extra assertion to add.
It won't catch all accidental "wrong thing put into
cluster" bugs (you can still put two differently
configured CPUs of the same type in a cluster) but it's
better than nothing.

Personally I think I would just have the "check they're
all the same type" guard, rather than having the board
code set a property explicitly.

thanks
-- PMM

[PATCH 4/5] hw/arm: Restrict CPU clusters to the expected type

[Philippe Mathieu-Daudé]

ARM SSE expects v7-M cores; the ZynqMP SoC expects Cortex-A53/R5F.

Do not allow any other CPU type by setting the cluster 'cpu-type'
property.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/armsse.c      | 1 +
 hw/arm/xlnx-zynqmp.c | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/hw/arm/armsse.c b/hw/arm/armsse.c
index 0202bad787..1132fdcbe2 100644
--- a/hw/arm/armsse.c
+++ b/hw/arm/armsse.c
@@ -715,6 +715,7 @@ static void armsse_init(Object *obj)
         name = g_strdup_printf("cluster%d", i);
         object_initialize_child(obj, name, &s->cluster[i], TYPE_CPU_CLUSTER);
         qdev_prop_set_uint32(DEVICE(&s->cluster[i]), "cluster-id", i);
+        qdev_prop_set_string(DEVICE(&s->cluster[i]), "cpu-type", TYPE_ARMV7M);
         g_free(name);
 
         name = g_strdup_printf("armv7m%d", i);
diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index 335cfc417d..e45cf88625 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -224,6 +224,8 @@ static void xlnx_zynqmp_create_rpu(MachineState *ms, XlnxZynqMPState *s,
     object_initialize_child(OBJECT(s), "rpu-cluster", &s->rpu_cluster,
                             TYPE_CPU_CLUSTER);
     qdev_prop_set_uint32(DEVICE(&s->rpu_cluster), "cluster-id", 1);
+    qdev_prop_set_string(DEVICE(&s->rpu_cluster), "cpu-type",
+                         ARM_CPU_TYPE_NAME("cortex-r5f"));
 
     for (i = 0; i < num_rpus; i++) {
         const char *name;
@@ -381,6 +383,8 @@ static void xlnx_zynqmp_init(Object *obj)
     object_initialize_child(obj, "apu-cluster", &s->apu_cluster,
                             TYPE_CPU_CLUSTER);
     qdev_prop_set_uint32(DEVICE(&s->apu_cluster), "cluster-id", 0);
+    qdev_prop_set_string(DEVICE(&s->apu_cluster), "cpu-type",
+                         ARM_CPU_TYPE_NAME("cortex-a53"));
 
     for (i = 0; i < num_apus; i++) {
         object_initialize_child(OBJECT(&s->apu_cluster), "apu-cpu[*]",
-- 
2.38.1

[PATCH 5/5] hw/riscv: Restrict CPU clusters to the expected type

[Philippe Mathieu-Daudé]

Microchip PolarFire SoC expects U51/U54 cores,
the SiFive Freedom board: the E31/E51 and U34/U54.

Do not allow any other CPU type by setting the cluster
'cpu-type' property.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/riscv/microchip_pfsoc.c | 4 ++++
 hw/riscv/sifive_u.c        | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
index 2b91e49561..658307fdfb 100644
--- a/hw/riscv/microchip_pfsoc.c
+++ b/hw/riscv/microchip_pfsoc.c
@@ -150,6 +150,8 @@ static void microchip_pfsoc_soc_instance_init(Object *obj)
 
     object_initialize_child(obj, "e-cluster", &s->e_cluster, TYPE_CPU_CLUSTER);
     qdev_prop_set_uint32(DEVICE(&s->e_cluster), "cluster-id", 0);
+    qdev_prop_set_string(DEVICE(&s->e_cluster), "cpu-type",
+                         TYPE_RISCV_CPU_SIFIVE_E51);
 
     object_initialize_child(OBJECT(&s->e_cluster), "e-cpus", &s->e_cpus,
                             TYPE_RISCV_HART_ARRAY);
@@ -161,6 +163,8 @@ static void microchip_pfsoc_soc_instance_init(Object *obj)
 
     object_initialize_child(obj, "u-cluster", &s->u_cluster, TYPE_CPU_CLUSTER);
     qdev_prop_set_uint32(DEVICE(&s->u_cluster), "cluster-id", 1);
+    qdev_prop_set_string(DEVICE(&s->e_cluster), "cpu-type",
+                         TYPE_RISCV_CPU_SIFIVE_U54);
 
     object_initialize_child(OBJECT(&s->u_cluster), "u-cpus", &s->u_cpus,
                             TYPE_RISCV_HART_ARRAY);
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index d3ab7a9cda..d0535746ca 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -763,6 +763,7 @@ static void sifive_u_soc_instance_init(Object *obj)
 
     object_initialize_child(obj, "e-cluster", &s->e_cluster, TYPE_CPU_CLUSTER);
     qdev_prop_set_uint32(DEVICE(&s->e_cluster), "cluster-id", 0);
+    qdev_prop_set_string(DEVICE(&s->e_cluster), "cpu-type", SIFIVE_E_CPU);
 
     object_initialize_child(OBJECT(&s->e_cluster), "e-cpus", &s->e_cpus,
                             TYPE_RISCV_HART_ARRAY);
@@ -813,6 +814,7 @@ static void sifive_u_soc_realize(DeviceState *dev, Error **errp)
      * CPU must exist and have been parented into the cluster before the
      * cluster is realized.
      */
+    qdev_prop_set_string(DEVICE(&s->u_cluster), "cpu-type", s->cpu_type);
     qdev_realize(DEVICE(&s->e_cluster), NULL, &error_abort);
     qdev_realize(DEVICE(&s->u_cluster), NULL, &error_abort);
 
-- 
2.38.1