diff mbox series

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

Message ID 20230216142338.82982-4-philmd@linaro.org
State New
Headers show
Series hw/cpu/cluster: Restrict CPU cluster to a particular CPU type | expand

Commit Message

Philippe Mathieu-Daudé Feb. 16, 2023, 2:23 p.m. UTC 
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(-)

Comments

Peter Maydell Feb. 21, 2023, 5:59 p.m. UTC | #1 
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
diff mbox series

Patch

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