On 10/18/22 18:43, Cédric Le Goater wrote:
Hello Pierre,
On 10/12/22 18:20, 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.
s390x/cpu topology: core_id sets s390x CPU topology
In the S390x CPU topology the core_id specifies the CPU address
and the position of the cpu withing the topology.
Let's build the topology based on the core_id.
The commit log is doubled.
Yes, thanks.
Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
include/hw/s390x/cpu-topology.h | 45 +++++++++++
hw/s390x/cpu-topology.c | 132 ++++++++++++++++++++++++++++++++
hw/s390x/s390-virtio-ccw.c | 21 +++++
hw/s390x/meson.build | 1 +
4 files changed, 199 insertions(+)
create mode 100644 include/hw/s390x/cpu-topology.h
create mode 100644 hw/s390x/cpu-topology.c
diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
new file mode 100644
index 0000000000..66c171d0bc
--- /dev/null
+++ b/include/hw/s390x/cpu-topology.h
@@ -0,0 +1,45 @@
+/*
+ * 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
+
+#include "hw/qdev-core.h"
+#include "qom/object.h"
+
+typedef struct S390TopoContainer {
+ int active_count;
+} S390TopoContainer;
This structure does not seem very useful.
+
+#define S390_TOPOLOGY_CPU_IFL 0x03
+#define S390_TOPOLOGY_MAX_ORIGIN ((63 + S390_MAX_CPUS) / 64)
+typedef struct S390TopoTLE {
The 'Topo' is redundant as TLE stands for 'topology-list entry'. This is minor.
+ uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
+} S390TopoTLE;
+
+struct S390Topology {
+ SysBusDevice parent_obj;
+ int cpus;
+ S390TopoContainer *socket;
+ S390TopoTLE *tle;
+ MachineState *ms;
hmm, it would be cleaner to introduce the fields and properties needed
by the S390Topology model and avoid dragging the machine object pointer.
AFAICT, these properties would be :
"nr-cpus"
"max-cpus"
"nr-sockets"
OK
+};
+
+#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);
+
+static inline bool s390_has_topology(void)
+{
+ return false;
+}
+
+#endif
diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
new file mode 100644
index 0000000000..42b22a1831
--- /dev/null
+++ b/hw/s390x/cpu-topology.c
@@ -0,0 +1,132 @@
+/*
+ * CPU Topology
+ *
+ * Copyright IBM Corp. 2022
The Copyright tag is different in the .h file.
OK, I change this to be like in the header file it seems to be the most used
format.
+ * Author(s): Pierre Morel <pmorel@linux.ibm.com>
+
+ * 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.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu/error-report.h"
+#include "hw/sysbus.h"
+#include "hw/qdev-properties.h"
+#include "hw/boards.h"
+#include "qemu/typedefs.h"
+#include "target/s390x/cpu.h"
+#include "hw/s390x/s390-virtio-ccw.h"
+#include "hw/s390x/cpu-topology.h"
+
+S390Topology *s390_get_topology(void)
+{
+ static S390Topology *s390Topology;
+
+ if (!s390Topology) {
+ s390Topology = S390_CPU_TOPOLOGY(
+ object_resolve_path(TYPE_S390_CPU_TOPOLOGY, NULL));
+ }
+
+ return s390Topology;
I am not convinced this routine is useful. The s390Topology pointer
could be stored under the machine state I think. It wouldn't be a
problem when CPUs are hot plugged since we have access to the machine
in the hot plug handler.
OK, I add a pointer to the machine state that will be initialised during
s390_init_topology()
For the stsi call, 'struct ArchCPU' probably lacks a back pointer to
the machine objects with which CPU interact. These are typically
interrupt controllers or this new s390Topology model. You could add
the pointer there or, better, under a generic 'void *opaque' attribute.
That said, what you did works fine. The modeling could be cleaner.
Yes. I think you are right and I add a opaque pointer to the topology.