| Message ID | 20220902075531.188916-3-pmorel@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: CPU Topology | expand |
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@linux.ibm.com> > --- > 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. [...] > > 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. > +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. > + 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. > + > +#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
Quoting Pierre Morel (2022-09-02 09:55:23) [...] > diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c > new file mode 100644 > index 0000000000..a6ca006ec5 > --- /dev/null > +++ b/hw/s390x/cpu-topology.c [...] > +void s390_topology_new_cpu(int core_id) > +{ [...] > + socket_id = core_id / topo->cores; The comment below is essential for understanding all of this. Move it before this line. > + > + bit = core_id; > + origin = bit / 64; > + bit %= 64; > + bit = 63 - bit; > + > + /* > + * At the core level, each CPU is represented by a bit in a 64bit > + * unsigned long. Set on plug and clear on unplug of a CPU. cleared ^ [...] > + * In that case the origin field, representing the offset of the first CPU > + * in the CPU container allows to represent up to the maximal number of > + * CPU inside several CPU containers inside the socket container. How about: "In that case the origin variable represents the offset of the first CPU in the CPU container. More than 64 CPUs per socket are represented in several CPU containers inside the socket container." > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index b5ca154e2f..15cefd104b 100644 [...] > @@ -247,6 +248,12 @@ static void ccw_init(MachineState *machine) > /* init memory + setup max page size. Required for the CPU model */ > s390_memory_init(machine->ram); > > + /* Adding the topology must be done before CPU intialization*/ space ^
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@linux.ibm.com> >> --- >> 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 >
On 9/6/22 07:58, Nico Boehr wrote: > Quoting Pierre Morel (2022-09-02 09:55:23) > [...] >> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c >> new file mode 100644 >> index 0000000000..a6ca006ec5 >> --- /dev/null >> +++ b/hw/s390x/cpu-topology.c > [...] >> +void s390_topology_new_cpu(int core_id) >> +{ > [...] >> + socket_id = core_id / topo->cores; > > The comment below is essential for understanding all of this. Move it before this line. > OK >> + >> + bit = core_id; >> + origin = bit / 64; >> + bit %= 64; >> + bit = 63 - bit; >> + >> + /* >> + * At the core level, each CPU is represented by a bit in a 64bit >> + * unsigned long. Set on plug and clear on unplug of a CPU. > > cleared ^ > > [...] >> + * In that case the origin field, representing the offset of the first CPU >> + * in the CPU container allows to represent up to the maximal number of >> + * CPU inside several CPU containers inside the socket container. > > How about: > "In that case the origin variable represents the offset of the first CPU in the > CPU container. More than 64 CPUs per socket are represented in several CPU > containers inside the socket container." Yes, better, thanks I take it > >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >> index b5ca154e2f..15cefd104b 100644 > [...] >> @@ -247,6 +248,12 @@ static void ccw_init(MachineState *machine) >> /* init memory + setup max page size. Required for the CPU model */ >> s390_memory_init(machine->ram); >> >> + /* Adding the topology must be done before CPU intialization*/ > > space ^ > Yes thanks regards, Pierre
On 9/2/22 09:55, 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@linux.ibm.com> > --- > 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 > new file mode 100644 > index 0000000000..a6ca006ec5 > --- /dev/null > +++ b/hw/s390x/cpu-topology.c > @@ -0,0 +1,135 @@ > +/* > + * CPU Topology > + * > + * Copyright IBM Corp. 2022 > + * 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; > +} > + > +/* > + * s390_topology_new_cpu: > + * @core_id: the core ID is machine wide > + * > + * The topology returned by s390_get_topology(), gives us the CPU > + * topology established by the -smp QEMU aruments. > + * The core-id gives: > + * - the Container TLE (Topology List Entry) containing the CPU TLE. > + * - in the CPU TLE the origin, or offset of the first bit in the core mask > + * - the bit in the CPU TLE core mask > + */ > +void s390_topology_new_cpu(int core_id) > +{ > + S390Topology *topo = s390_get_topology(); > + int socket_id; > + int bit, origin; > + > + /* In the case no Topology is used nothing is to be done here */ > + if (!topo) { > + return; > + } > + > + socket_id = core_id / topo->cores; > + > + bit = core_id; > + origin = bit / 64; > + bit %= 64; > + bit = 63 - bit; > + > + /* > + * At the core level, each CPU is represented by a bit in a 64bit > + * unsigned long. Set on plug and clear on unplug of a CPU. Do we have CPU unplug on s390x ? > + * The firmware assume that all CPU in a CPU TLE have the same > + * type, polarization and are all dedicated or shared. > + * In the case a socket contains CPU with different type, polarization > + * or entitlement then they will be defined in different CPU containers. > + * Currently we assume all CPU are identical IFL CPUs and that they are > + * all dedicated CPUs. > + * The only reason to have several S390TopologyCores inside a socket is > + * to have more than 64 CPUs. > + * In that case the origin field, representing the offset of the first CPU > + * in the CPU container allows to represent up to the maximal number of > + * CPU inside several CPU containers inside the socket container. > + */ > + topo->socket[socket_id].active_count++; > + topo->tle[socket_id].active_count++; > + set_bit(bit, &topo->tle[socket_id].mask[origin]); > +} > + > +/** > + * 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()); Using qdev_get_machine() is suspicious :) > + S390Topology *topo = S390_CPU_TOPOLOGY(dev); > + int n; > + > + topo->sockets = ms->smp.sockets; > + topo->cores = ms->smp.cores; > + topo->tles = ms->smp.max_cpus; These look like object properties to me. > + > + n = topo->sockets; > + topo->socket = g_malloc0(n * sizeof(S390TopoContainer)); > + topo->tle = g_malloc0(topo->tles * sizeof(S390TopoTLE)); > +} > + > +/** > + * topology_class_init: > + * @oc: Object class > + * @data: (not used) > + * > + * A very simple object we will need for reset and migration. > + */ > +static void topology_class_init(ObjectClass *oc, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(oc); > + > + dc->realize = s390_topology_realize; > + set_bit(DEVICE_CATEGORY_MISC, dc->categories); > +} no vmstate ? > +static const TypeInfo cpu_topology_info = { > + .name = TYPE_S390_CPU_TOPOLOGY, > + .parent = TYPE_SYS_BUS_DEVICE, > + .instance_size = sizeof(S390Topology), > + .class_init = topology_class_init, > +}; > + > +static void topology_register(void) > +{ > + type_register_static(&cpu_topology_info); > +} > +type_init(topology_register); > diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build > index de28a90a57..96d7d7d231 100644 > --- a/hw/s390x/meson.build > +++ b/hw/s390x/meson.build > @@ -2,6 +2,7 @@ s390x_ss = ss.source_set() > s390x_ss.add(files( > 'ap-bridge.c', > 'ap-device.c', > + 'cpu-topology.c', > 'ccw-device.c', > 'css-bridge.c', > 'css.c', > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index b5ca154e2f..15cefd104b 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -43,6 +43,7 @@ > #include "sysemu/sysemu.h" > #include "hw/s390x/pv.h" > #include "migration/blocker.h" > +#include "hw/s390x/cpu-topology.h" > > static Error *pv_mig_blocker; > > @@ -247,6 +248,12 @@ static void ccw_init(MachineState *machine) > /* init memory + setup max page size. Required for the CPU model */ > s390_memory_init(machine->ram); > > + /* Adding the topology must be done before CPU intialization*/ > + dev = qdev_new(TYPE_S390_CPU_TOPOLOGY); > + object_property_add_child(qdev_get_machine(), TYPE_S390_CPU_TOPOLOGY, No need to use qdev_get_machine(), you have 'machine' above. > + OBJECT(dev)); > + sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); why not store a TYPE_S390_CPU_TOPOLOGY object pointer under the machine state for later use ? > + > /* init CPUs (incl. CPU model) early so s390_has_feature() works */ > s390_init_cpus(machine); > > @@ -309,6 +316,9 @@ static void s390_cpu_plug(HotplugHandler *hotplug_dev, > g_assert(!ms->possible_cpus->cpus[cpu->env.core_id].cpu); > ms->possible_cpus->cpus[cpu->env.core_id].cpu = OBJECT(dev); > > + /* Inserting the CPU in the Topology can not fail */ > + s390_topology_new_cpu(cpu->env.core_id); > + in which case, we could use the topo object pointer to insert a new CPU id and drop s390_get_topology() which looks overkill. I would add the test : if (!S390_CCW_MACHINE(machine)->topology_disable) { before inserting to be consistent. But I am anticipating some other patch. C. > if (dev->hotplugged) { > raise_irq_cpu_hotplug(); > } > 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 > + > +typedef struct S390TopoContainer { > + int active_count; > +} S390TopoContainer; > + > +#define S390_TOPOLOGY_MAX_ORIGIN (1 + S390_MAX_CPUS / 64) > +typedef struct S390TopoTLE { > + int active_count; > + 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; > + int tles; > + S390TopoContainer *socket; > + S390TopoTLE *tle; > +}; > +typedef struct S390Topology S390Topology; > + > +#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
On 9/27/22 14:03, Cédric Le Goater wrote: > On 9/2/22 09:55, 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@linux.ibm.com> >> --- >> 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 >> new file mode 100644 >> index 0000000000..a6ca006ec5 >> --- /dev/null >> +++ b/hw/s390x/cpu-topology.c >> @@ -0,0 +1,135 @@ >> +/* >> + * CPU Topology >> + * >> + * Copyright IBM Corp. 2022 >> + * 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; >> +} >> + >> +/* >> + * s390_topology_new_cpu: >> + * @core_id: the core ID is machine wide >> + * >> + * The topology returned by s390_get_topology(), gives us the CPU >> + * topology established by the -smp QEMU aruments. >> + * The core-id gives: >> + * - the Container TLE (Topology List Entry) containing the CPU TLE. >> + * - in the CPU TLE the origin, or offset of the first bit in the >> core mask >> + * - the bit in the CPU TLE core mask >> + */ >> +void s390_topology_new_cpu(int core_id) >> +{ >> + S390Topology *topo = s390_get_topology(); >> + int socket_id; >> + int bit, origin; >> + >> + /* In the case no Topology is used nothing is to be done here */ >> + if (!topo) { >> + return; >> + } >> + >> + socket_id = core_id / topo->cores; >> + >> + bit = core_id; >> + origin = bit / 64; >> + bit %= 64; >> + bit = 63 - bit; >> + >> + /* >> + * At the core level, each CPU is represented by a bit in a 64bit >> + * unsigned long. Set on plug and clear on unplug of a CPU. > > Do we have CPU unplug on s390x ? not in QEMU. I will change this sentence. > >> + * The firmware assume that all CPU in a CPU TLE have the same >> + * type, polarization and are all dedicated or shared. >> + * In the case a socket contains CPU with different type, >> polarization >> + * or entitlement then they will be defined in different CPU >> containers. >> + * Currently we assume all CPU are identical IFL CPUs and that >> they are >> + * all dedicated CPUs. >> + * The only reason to have several S390TopologyCores inside a >> socket is >> + * to have more than 64 CPUs. >> + * In that case the origin field, representing the offset of the >> first CPU >> + * in the CPU container allows to represent up to the maximal >> number of >> + * CPU inside several CPU containers inside the socket container. >> + */ >> + topo->socket[socket_id].active_count++; >> + topo->tle[socket_id].active_count++; >> + set_bit(bit, &topo->tle[socket_id].mask[origin]); >> +} >> + >> +/** >> + * 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()); > > Using qdev_get_machine() is suspicious :) OK > >> + S390Topology *topo = S390_CPU_TOPOLOGY(dev); >> + int n; >> + >> + topo->sockets = ms->smp.sockets; >> + topo->cores = ms->smp.cores; >> + topo->tles = ms->smp.max_cpus; > > These look like object properties to me. It is a temporary store to keep them at hand. I will see if I keep them afterall. If I keep them I will make them property. > >> + >> + n = topo->sockets; >> + topo->socket = g_malloc0(n * sizeof(S390TopoContainer)); >> + topo->tle = g_malloc0(topo->tles * sizeof(S390TopoTLE)); >> +} >> + >> +/** >> + * topology_class_init: >> + * @oc: Object class >> + * @data: (not used) >> + * >> + * A very simple object we will need for reset and migration. >> + */ >> +static void topology_class_init(ObjectClass *oc, void *data) >> +{ >> + DeviceClass *dc = DEVICE_CLASS(oc); >> + >> + dc->realize = s390_topology_realize; >> + set_bit(DEVICE_CATEGORY_MISC, dc->categories); >> +} > > no vmstate ? it is added in a later patch > >> +static const TypeInfo cpu_topology_info = { >> + .name = TYPE_S390_CPU_TOPOLOGY, >> + .parent = TYPE_SYS_BUS_DEVICE, >> + .instance_size = sizeof(S390Topology), >> + .class_init = topology_class_init, >> +}; >> + >> +static void topology_register(void) >> +{ >> + type_register_static(&cpu_topology_info); >> +} >> +type_init(topology_register); >> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build >> index de28a90a57..96d7d7d231 100644 >> --- a/hw/s390x/meson.build >> +++ b/hw/s390x/meson.build >> @@ -2,6 +2,7 @@ s390x_ss = ss.source_set() >> s390x_ss.add(files( >> 'ap-bridge.c', >> 'ap-device.c', >> + 'cpu-topology.c', >> 'ccw-device.c', >> 'css-bridge.c', >> 'css.c', >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >> index b5ca154e2f..15cefd104b 100644 >> --- a/hw/s390x/s390-virtio-ccw.c >> +++ b/hw/s390x/s390-virtio-ccw.c >> @@ -43,6 +43,7 @@ >> #include "sysemu/sysemu.h" >> #include "hw/s390x/pv.h" >> #include "migration/blocker.h" >> +#include "hw/s390x/cpu-topology.h" >> static Error *pv_mig_blocker; >> @@ -247,6 +248,12 @@ static void ccw_init(MachineState *machine) >> /* init memory + setup max page size. Required for the CPU model */ >> s390_memory_init(machine->ram); >> + /* Adding the topology must be done before CPU intialization*/ >> + dev = qdev_new(TYPE_S390_CPU_TOPOLOGY); >> + object_property_add_child(qdev_get_machine(), >> TYPE_S390_CPU_TOPOLOGY, > > No need to use qdev_get_machine(), you have 'machine' above. OK > >> + OBJECT(dev)); >> + sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); > > > why not store a TYPE_S390_CPU_TOPOLOGY object pointer under the machine > state for later use ? May be, I will think about it. > >> + >> /* init CPUs (incl. CPU model) early so s390_has_feature() works */ >> s390_init_cpus(machine); >> @@ -309,6 +316,9 @@ static void s390_cpu_plug(HotplugHandler >> *hotplug_dev, >> g_assert(!ms->possible_cpus->cpus[cpu->env.core_id].cpu); >> ms->possible_cpus->cpus[cpu->env.core_id].cpu = OBJECT(dev); >> + /* Inserting the CPU in the Topology can not fail */ >> + s390_topology_new_cpu(cpu->env.core_id); >> + > > in which case, we could use the topo object pointer to insert a new CPU > id and drop s390_get_topology() which looks overkill. > > I would add the test : > > if (!S390_CCW_MACHINE(machine)->topology_disable) { > > before inserting to be consistent. But I am anticipating some other > patch. It belongs here for bisect so I will add it here. Thanks. > > C. > > Thanks, Pierre
diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c new file mode 100644 index 0000000000..a6ca006ec5 --- /dev/null +++ b/hw/s390x/cpu-topology.c @@ -0,0 +1,135 @@ +/* + * CPU Topology + * + * Copyright IBM Corp. 2022 + * 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; +} + +/* + * s390_topology_new_cpu: + * @core_id: the core ID is machine wide + * + * The topology returned by s390_get_topology(), gives us the CPU + * topology established by the -smp QEMU aruments. + * The core-id gives: + * - the Container TLE (Topology List Entry) containing the CPU TLE. + * - in the CPU TLE the origin, or offset of the first bit in the core mask + * - the bit in the CPU TLE core mask + */ +void s390_topology_new_cpu(int core_id) +{ + S390Topology *topo = s390_get_topology(); + int socket_id; + int bit, origin; + + /* In the case no Topology is used nothing is to be done here */ + if (!topo) { + return; + } + + socket_id = core_id / topo->cores; + + bit = core_id; + origin = bit / 64; + bit %= 64; + bit = 63 - bit; + + /* + * At the core level, each CPU is represented by a bit in a 64bit + * unsigned long. Set on plug and clear on unplug of a CPU. + * The firmware assume that all CPU in a CPU TLE have the same + * type, polarization and are all dedicated or shared. + * In the case a socket contains CPU with different type, polarization + * or entitlement then they will be defined in different CPU containers. + * Currently we assume all CPU are identical IFL CPUs and that they are + * all dedicated CPUs. + * The only reason to have several S390TopologyCores inside a socket is + * to have more than 64 CPUs. + * In that case the origin field, representing the offset of the first CPU + * in the CPU container allows to represent up to the maximal number of + * CPU inside several CPU containers inside the socket container. + */ + topo->socket[socket_id].active_count++; + topo->tle[socket_id].active_count++; + set_bit(bit, &topo->tle[socket_id].mask[origin]); +} + +/** + * 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)); +} + +/** + * topology_class_init: + * @oc: Object class + * @data: (not used) + * + * A very simple object we will need for reset and migration. + */ +static void topology_class_init(ObjectClass *oc, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(oc); + + dc->realize = s390_topology_realize; + set_bit(DEVICE_CATEGORY_MISC, dc->categories); +} + +static const TypeInfo cpu_topology_info = { + .name = TYPE_S390_CPU_TOPOLOGY, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_size = sizeof(S390Topology), + .class_init = topology_class_init, +}; + +static void topology_register(void) +{ + type_register_static(&cpu_topology_info); +} +type_init(topology_register); diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build index de28a90a57..96d7d7d231 100644 --- a/hw/s390x/meson.build +++ b/hw/s390x/meson.build @@ -2,6 +2,7 @@ s390x_ss = ss.source_set() s390x_ss.add(files( 'ap-bridge.c', 'ap-device.c', + 'cpu-topology.c', 'ccw-device.c', 'css-bridge.c', 'css.c', diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index b5ca154e2f..15cefd104b 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -43,6 +43,7 @@ #include "sysemu/sysemu.h" #include "hw/s390x/pv.h" #include "migration/blocker.h" +#include "hw/s390x/cpu-topology.h" static Error *pv_mig_blocker; @@ -247,6 +248,12 @@ static void ccw_init(MachineState *machine) /* init memory + setup max page size. Required for the CPU model */ s390_memory_init(machine->ram); + /* Adding the topology must be done before CPU intialization*/ + dev = qdev_new(TYPE_S390_CPU_TOPOLOGY); + object_property_add_child(qdev_get_machine(), TYPE_S390_CPU_TOPOLOGY, + OBJECT(dev)); + sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); + /* init CPUs (incl. CPU model) early so s390_has_feature() works */ s390_init_cpus(machine); @@ -309,6 +316,9 @@ static void s390_cpu_plug(HotplugHandler *hotplug_dev, g_assert(!ms->possible_cpus->cpus[cpu->env.core_id].cpu); ms->possible_cpus->cpus[cpu->env.core_id].cpu = OBJECT(dev); + /* Inserting the CPU in the Topology can not fail */ + s390_topology_new_cpu(cpu->env.core_id); + if (dev->hotplugged) { raise_irq_cpu_hotplug(); } 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 + +typedef struct S390TopoContainer { + int active_count; +} S390TopoContainer; + +#define S390_TOPOLOGY_MAX_ORIGIN (1 + S390_MAX_CPUS / 64) +typedef struct S390TopoTLE { + int active_count; + 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; + int tles; + S390TopoContainer *socket; + S390TopoTLE *tle; +}; +typedef struct S390Topology S390Topology; + +#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
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@linux.ibm.com> --- 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