On 9/5/22 20:11, Janis Schoetterl-Glausch wrote:
On Fri, 2022-09-02 at 09:55 +0200, Pierre Morel wrote:
In the S390x CPU topology the core_id specifies the CPU address
and the position of the core withing the topology.
Let's build the topology based on the core_id.
Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
---
hw/s390x/cpu-topology.c | 135 ++++++++++++++++++++++++++++++++
hw/s390x/meson.build | 1 +
hw/s390x/s390-virtio-ccw.c | 10 +++
include/hw/s390x/cpu-topology.h | 42 ++++++++++
4 files changed, 188 insertions(+)
create mode 100644 hw/s390x/cpu-topology.c
create mode 100644 include/hw/s390x/cpu-topology.h
diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
[...]
+/**
+ * s390_topology_realize:
+ * @dev: the device state
+ * @errp: the error pointer (not used)
+ *
+ * During realize the machine CPU topology is initialized with the
+ * QEMU -smp parameters.
+ * The maximum count of CPU TLE in the all Topology can not be greater
+ * than the maximum CPUs.
+ */
+static void s390_topology_realize(DeviceState *dev, Error **errp)
+{
+ MachineState *ms = MACHINE(qdev_get_machine());
+ S390Topology *topo = S390_CPU_TOPOLOGY(dev);
+ int n;
+
+ topo->sockets = ms->smp.sockets;
+ topo->cores = ms->smp.cores;
+ topo->tles = ms->smp.max_cpus;
+
+ n = topo->sockets;
+ topo->socket = g_malloc0(n * sizeof(S390TopoContainer));
+ topo->tle = g_malloc0(topo->tles * sizeof(S390TopoTLE));
Seems like a good use case for g_new0.
Yes
[...]
diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
new file mode 100644
index 0000000000..6911f975f4
--- /dev/null
+++ b/include/hw/s390x/cpu-topology.h
@@ -0,0 +1,42 @@
+/*
+ * CPU Topology
+ *
+ * Copyright 2022 IBM Corp.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+#ifndef HW_S390X_CPU_TOPOLOGY_H
+#define HW_S390X_CPU_TOPOLOGY_H
Is there a reason this is before the includes?
+
+typedef struct S390TopoContainer {
+ int active_count;
+} S390TopoContainer;
+
+#define S390_TOPOLOGY_MAX_ORIGIN (1 + S390_MAX_CPUS / 64)
This is correct because cpu_id == core_id for s390, right?
So the cpu limit also applies to the core id.
You could do ((S390_MAX_CPUS + 63) / 64) instead.
But if you chose this for simplicity's sake, I'm fine with it.
hum, I think your proposition is right and mine is false.
If S390_MAX_CPUS is 64 we have only 1 origin.
I count 2 but you count 1.
So I change this.
+typedef struct S390TopoTLE {
+ int active_count;
Do you use (read) this field somewhere?
Is this in anticipation of there being multiple TLE arrays, for
different polarizations, etc? If so I would defer this for later.
OK
+ uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
+} S390TopoTLE;
+
+#include "hw/qdev-core.h"
+#include "qom/object.h"
+
+struct S390Topology {
+ SysBusDevice parent_obj;
+ int sockets;
+ int cores;
These are just cached values from machine_state.smp, right?
Not sure if I like the redundancy, it doesn't aid in comprehension.
+ int tles;
+ S390TopoContainer *socket;
+ S390TopoTLE *tle;
+};
+typedef struct S390Topology S390Topology;
The DECLARE macro takes care of this typedef.
right
+
+#define TYPE_S390_CPU_TOPOLOGY "s390-topology"
+OBJECT_DECLARE_SIMPLE_TYPE(S390Topology, S390_CPU_TOPOLOGY)
+
+S390Topology *s390_get_topology(void);
+void s390_topology_new_cpu(int core_id);
+
+#endif
--
Pierre Morel
IBM Lab Boeblingen
