Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology
On 11/7/22 19:04, Janis Schoetterl-Glausch wrote:
On Fri, 2022-10-28 at 11:30 +0200, Pierre Morel wrote:
On 10/27/22 22:20, Janis Schoetterl-Glausch wrote:
On Wed, 2022-10-26 at 10:34 +0200, Pierre Morel wrote:
On 10/25/22 21:58, Janis Schoetterl-Glausch wrote:
On Wed, 2022-10-12 at 18:20 +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.
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.
Signed-off-by: Pierre Morel
---
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
[...]
+/**
+ * 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);
+
+topo->cpus = ms->smp.cores * ms->smp.threads;
Currently threads are not supported, effectively increasing the number of cpus,
so this is currently correct. Once the machine version limits the threads to 1,
it is also correct. However, once we support multiple threads, this becomes
incorrect.
I wonder if it's ok from a backward compatibility point of view to modify the
smp values
by doing cores *= threads, threads = 1 for old machines.
Right, this will become incorrect with thread support.
What about having a dedicated function:
topo->cpus = s390_get_cpus(ms);
This function will use the S390CcwMachineClass->max_thread introduced
later to report the correct number of CPUs.
I don't think max_threads is exactly what matters here, it's if
threads are supported or not or, if max_threads == 1 it doesn't matter.
The question is how best to do the check. You could check the machine version.
I wonder if you could add a feature bit for the multithreading facility that is
always false and use that.
I don't know if using a function makes a difference, that is if it is obvious on
introduction of multithreading support that the function needs to be updated.
(If it is implemented in a way that requires updating, if you check the machine
version it doesn't)
In any case, the name you suggested isn't very descriptive.
I think we care about this machine and olders.
Olders do not support topology so this, Multithreading (MT) does not mater.
This machine support topology, if I follow Cedric advise, the
"max_thread" will/may be introduce before the topology.
This in fact is not an implementation for MT or does not allow the
implementation of MT it is only a way to get rid of the false
information given to the user that we accept MT.
So I think that when we introduce MT we will take care of making things
right at this place as in other places of the code.
What about we keep the original:
topo->cpus = ms->smp.cores * ms->smp.threads;
If topology is only supported for new machines and not the old machines
for which you set max_threads to a compatibility value (max cpus), then
you should just ignore the threads, cpus == cores.
(There might not be any point in keeping a topo->cpus member in this case, I
haven't checked)
Right but, I need the nr_cpus in the topology so I prefer to keep it.
However, smp.threads has nothing to do there anymore as you pointed.
I think that nr_cpus should may be named nr_cores and should be set to
smp.cores.
Thanks,
Regards,
Pierre
--
Pierre Morel
IBM Lab Boeblingen
Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology
On Fri, 2022-10-28 at 11:30 +0200, Pierre Morel wrote:
>
> On 10/27/22 22:20, Janis Schoetterl-Glausch wrote:
> > On Wed, 2022-10-26 at 10:34 +0200, Pierre Morel wrote:
> > >
> > > On 10/25/22 21:58, Janis Schoetterl-Glausch wrote:
> > > > On Wed, 2022-10-12 at 18:20 +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.
> > > > > 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.
> > > > >
> > > > > Signed-off-by: Pierre Morel
> > > > > ---
> > > > >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
> > > > >
> > > > [...]
> > > >
> > > > > +/**
> > > > > + * 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);
> > > > > +
> > > > > +topo->cpus = ms->smp.cores * ms->smp.threads;
> > > >
> > > > Currently threads are not supported, effectively increasing the number
> > > > of cpus,
> > > > so this is currently correct. Once the machine version limits the
> > > > threads to 1,
> > > > it is also correct. However, once we support multiple threads, this
> > > > becomes incorrect.
> > > > I wonder if it's ok from a backward compatibility point of view to
> > > > modify the smp values
> > > > by doing cores *= threads, threads = 1 for old machines.
> > >
> > > Right, this will become incorrect with thread support.
> > > What about having a dedicated function:
> > >
> > > topo->cpus = s390_get_cpus(ms);
> > >
> > > This function will use the S390CcwMachineClass->max_thread introduced
> > > later to report the correct number of CPUs.
> >
> > I don't think max_threads is exactly what matters here, it's if
> > threads are supported or not or, if max_threads == 1 it doesn't matter.
> > The question is how best to do the check. You could check the machine
> > version.
> > I wonder if you could add a feature bit for the multithreading facility
> > that is
> > always false and use that.
> >
> > I don't know if using a function makes a difference, that is if it is
> > obvious on
> > introduction of multithreading support that the function needs to be
> > updated.
> > (If it is implemented in a way that requires updating, if you check the
> > machine
> > version it doesn't)
> > In any case, the name you suggested isn't very descriptive.
>
> I think we care about this machine and olders.
> Olders do not support topology so this, Multithreading (MT) does not mater.
> This machine support topology, if I follow Cedric advise, the
> "max_thread" will/may be introduce before the topology.
>
> This in fact is not an implementation for MT or does not allow the
> implementation of MT it is only a way to get rid of the false
> information given to the user that we accept MT.
>
> So I think that when we introduce MT we will take care of making things
> right at this place as in other places of the code.
>
> What about we keep the original:
>
> topo->cpus = ms->smp.cores * ms->smp.threads;
If topology is only supported for new machines and not the old machines
for which you set max_threads to a compatibility value (max cpus), then
you should just ignore the threads, cpus == cores.
(There might not be any point in keeping a topo->cpus member in this case, I
haven't checked)
>
> Which does not do any arm to machines without MT ?
>
> Regards,
> Pierre
>
Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology
On 10/27/22 22:20, Janis Schoetterl-Glausch wrote:
On Wed, 2022-10-26 at 10:34 +0200, Pierre Morel wrote:
On 10/25/22 21:58, Janis Schoetterl-Glausch wrote:
On Wed, 2022-10-12 at 18:20 +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.
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.
Signed-off-by: Pierre Morel
---
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
[...]
+/**
+ * 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);
+
+topo->cpus = ms->smp.cores * ms->smp.threads;
Currently threads are not supported, effectively increasing the number of cpus,
so this is currently correct. Once the machine version limits the threads to 1,
it is also correct. However, once we support multiple threads, this becomes
incorrect.
I wonder if it's ok from a backward compatibility point of view to modify the
smp values
by doing cores *= threads, threads = 1 for old machines.
Right, this will become incorrect with thread support.
What about having a dedicated function:
topo->cpus = s390_get_cpus(ms);
This function will use the S390CcwMachineClass->max_thread introduced
later to report the correct number of CPUs.
I don't think max_threads is exactly what matters here, it's if
threads are supported or not or, if max_threads == 1 it doesn't matter.
The question is how best to do the check. You could check the machine version.
I wonder if you could add a feature bit for the multithreading facility that is
always false and use that.
I don't know if using a function makes a difference, that is if it is obvious on
introduction of multithreading support that the function needs to be updated.
(If it is implemented in a way that requires updating, if you check the machine
version it doesn't)
In any case, the name you suggested isn't very descriptive.
I think we care about this machine and olders.
Olders do not support topology so this, Multithreading (MT) does not mater.
This machine support topology, if I follow Cedric advise, the
"max_thread" will/may be introduce before the topology.
This in fact is not an implementation for MT or does not allow the
implementation of MT it is only a way to get rid of the false
information given to the user that we accept MT.
So I think that when we introduce MT we will take care of making things
right at this place as in other places of the code.
What about we keep the original:
topo->cpus = ms->smp.cores * ms->smp.threads;
Which does not do any arm to machines without MT ?
Regards,
Pierre
--
Pierre Morel
IBM Lab Boeblingen
Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology
On Wed, 2022-10-26 at 10:34 +0200, Pierre Morel wrote:
>
> On 10/25/22 21:58, Janis Schoetterl-Glausch wrote:
> > On Wed, 2022-10-12 at 18:20 +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.
> > > 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.
> > >
> > > Signed-off-by: Pierre Morel
> > > ---
> > > 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
> > >
> > [...]
> >
> > > +/**
> > > + * 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);
> > > +
> > > +topo->cpus = ms->smp.cores * ms->smp.threads;
> >
> > Currently threads are not supported, effectively increasing the number of
> > cpus,
> > so this is currently correct. Once the machine version limits the threads
> > to 1,
> > it is also correct. However, once we support multiple threads, this becomes
> > incorrect.
> > I wonder if it's ok from a backward compatibility point of view to modify
> > the smp values
> > by doing cores *= threads, threads = 1 for old machines.
>
> Right, this will become incorrect with thread support.
> What about having a dedicated function:
>
> topo->cpus = s390_get_cpus(ms);
>
> This function will use the S390CcwMachineClass->max_thread introduced
> later to report the correct number of CPUs.
I don't think max_threads is exactly what matters here, it's if
threads are supported or not or, if max_threads == 1 it doesn't matter.
The question is how best to do the check. You could check the machine version.
I wonder if you could add a feature bit for the multithreading facility that is
always false and use that.
I don't know if using a function makes a difference, that is if it is obvious on
introduction of multithreading support that the function needs to be updated.
(If it is implemented in a way that requires updating, if you check the machine
version it doesn't)
In any case, the name you suggested isn't very descriptive.
>
>
> > Then you can just use the cores value and it is always correct.
> > In any case, if you keep it as is, I'd like to see a comment here saying
> > that this
> > is correct only so long as we don't support threads.
> > > +
> > > +topo->socket = g_new0(S390TopoContainer, ms->smp.sockets);
> > > +topo->tle = g_new0(S390TopoTLE, ms->smp.max_cpus);
> > > +
> > > +topo->ms = ms;
> > > +}
> > > +
> > [...]
>
Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology
On Thu, 2022-10-27 at 10:05 +0200, Thomas Huth wrote:
> On 24/10/2022 21.25, Janis Schoetterl-Glausch wrote:
> > On Wed, 2022-10-12 at 18:20 +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.
> > > 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.
> > >
> > > Signed-off-by: Pierre Morel
> > > ---
> > > 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 00..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;
> > > +
> > > +#define S390_TOPOLOGY_CPU_IFL 0x03
> > > +#define S390_TOPOLOGY_MAX_ORIGIN ((63 + S390_MAX_CPUS) / 64)
> > > +typedef struct S390TopoTLE {
> > > +uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
> > > +} S390TopoTLE;
> >
> > Since this actually represents multiple TLEs, you might want to change the
> > name of the struct to reflect this. S390TopoTLEList maybe?
>
> Didn't TLE mean "Topology List Entry"? (by the way, Pierre, please explain
Yes.
> this three letter acronym somewhere in this header in a comment)...
>
> So expanding the TLE, this would mean S390TopoTopologyListEntryList ? ...
> this is getting weird...
:D indeed. So the leaves of the topology tree as stored by STSI are lists
of CPU-type TLEs which aren't empty i.e. represent some cpus.
Whereas this struct is used to track which CPU-type TLEs need to be created.
It doesn't represent a TLE and doesn't represent the list of CPU-type TLEs.
So yeah, you're right, not a good name.
Off the top of my head I'd suggest S390TopoCPUSet. It's a bitmap, which is
kind of a set. Maybe S390TopoSocketCPUSet to reflect that it is the set of
CPUs in a socket, although, if we ever support different polarizations, etc.
that wouldn't really be true anymore, since that creates additional levels,
so maybe not. (In that case the leaf list of CPU-types TLEs is a flattened
tree.)
> Also, this is not a "list" in the sense of a linked
> list, as one might expect at a first glance, so this is all very confusing
> here. Could you please come up with some better naming?
>
> Thomas
>
>
Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology
On 24/10/2022 21.25, Janis Schoetterl-Glausch wrote:
On Wed, 2022-10-12 at 18:20 +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.
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.
Signed-off-by: Pierre Morel
---
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 00..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;
+
+#define S390_TOPOLOGY_CPU_IFL 0x03
+#define S390_TOPOLOGY_MAX_ORIGIN ((63 + S390_MAX_CPUS) / 64)
+typedef struct S390TopoTLE {
+uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
+} S390TopoTLE;
Since this actually represents multiple TLEs, you might want to change the
name of the struct to reflect this. S390TopoTLEList maybe?
Didn't TLE mean "Topology List Entry"? (by the way, Pierre, please explain
this three letter acronym somewhere in this header in a comment)...
So expanding the TLE, this would mean S390TopoTopologyListEntryList ? ...
this is getting weird... Also, this is not a "list" in the sense of a linked
list, as one might expect at a first glance, so this is all very confusing
here. Could you please come up with some better naming?
Thomas
Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology
On 10/25/22 21:58, Janis Schoetterl-Glausch wrote:
On Wed, 2022-10-12 at 18:20 +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.
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.
Signed-off-by: Pierre Morel
---
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
[...]
+/**
+ * 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);
+
+topo->cpus = ms->smp.cores * ms->smp.threads;
Currently threads are not supported, effectively increasing the number of cpus,
so this is currently correct. Once the machine version limits the threads to 1,
it is also correct. However, once we support multiple threads, this becomes
incorrect.
I wonder if it's ok from a backward compatibility point of view to modify the
smp values
by doing cores *= threads, threads = 1 for old machines.
Right, this will become incorrect with thread support.
What about having a dedicated function:
topo->cpus = s390_get_cpus(ms);
This function will use the S390CcwMachineClass->max_thread introduced
later to report the correct number of CPUs.
Then you can just use the cores value and it is always correct.
In any case, if you keep it as is, I'd like to see a comment here saying that
this
is correct only so long as we don't support threads.
+
+topo->socket = g_new0(S390TopoContainer, ms->smp.sockets);
+topo->tle = g_new0(S390TopoTLE, ms->smp.max_cpus);
+
+topo->ms = ms;
+}
+
[...]
--
Pierre Morel
IBM Lab Boeblingen
Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology
On Wed, 2022-10-12 at 18:20 +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.
> 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.
>
> Signed-off-by: Pierre Morel
> ---
> 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
>
[...]
> +/**
> + * 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);
> +
> +topo->cpus = ms->smp.cores * ms->smp.threads;
Currently threads are not supported, effectively increasing the number of cpus,
so this is currently correct. Once the machine version limits the threads to 1,
it is also correct. However, once we support multiple threads, this becomes
incorrect.
I wonder if it's ok from a backward compatibility point of view to modify the
smp values
by doing cores *= threads, threads = 1 for old machines.
Then you can just use the cores value and it is always correct.
In any case, if you keep it as is, I'd like to see a comment here saying that
this
is correct only so long as we don't support threads.
> +
> +topo->socket = g_new0(S390TopoContainer, ms->smp.sockets);
> +topo->tle = g_new0(S390TopoTLE, ms->smp.max_cpus);
> +
> +topo->ms = ms;
> +}
> +
[...]
Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology
On Wed, 2022-10-19 at 17:39 +0200, Pierre Morel wrote:
>
> On 10/18/22 18:43, Cédric Le Goater wrote:
>
> > >
[...]
> > > Signed-off-by: Pierre Morel
> > > ---
> > > 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
> > >
[...]
> > >
> > > +/*
> > > + * 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;
> > > + }
> >
> > I would move this test in the caller.
>
> Check will disapear with the new implementation.
>
> >
> > > +
> > > + socket_id = core_id / topo->cpus;
> > > +
> > > + /*
> > > + * At the core level, each CPU is represented by a bit in a 64bit
> > > + * unsigned long which represent the presence 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 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.
> > > + * The only reason to have several S390TopologyCores inside a
> > > socket is
> > > + * to have more than 64 CPUs.
> > > + * 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.
> > > + */
> > > + bit = core_id;
> > > + origin = bit / 64;
> > > + bit %= 64;
> > > + bit = 63 - bit;
> > > +
> > > + topo->socket[socket_id].active_count++;
> > > + set_bit(bit, >tle[socket_id].mask[origin]);
> >
> > here, the tle array is indexed with a socket id and ...
>
> It was stupid to keep both structures.
> I will keep only the socket structure and incorparate the TLE inside.
I don't think it's stupid. Both are valid possibilities.
The first one treats sockets and books and drawers exactly the same, since
they are all just containers (once you introduce books and drawers).
The second treats sockets differently, because they're the leaf nodes of the
hierarchy in a certain sense (the leaf nodes of the "regular" hierarchy,
whereas the cpus are the real leaf nodes of the topology but special/not
"regular").
I'd say the first is more natural from reading the PoP, but it might indeed be
a bit
confusing when reading the code since there's a one to one correspondence
between
sockets and TLE(List)s.
>
> >
> > > +}
> > > +
> > > +/**
> > > + * 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);
> > > +
> > > + topo->cpus = ms->smp.cores * ms->smp.threads;> +
> > > + topo->socket = g_new0(S390TopoContainer, ms->smp.sockets);
> > > + topo->tle = g_new0(S390TopoTLE, ms->smp.max_cpus);
> >
> >
> > ... here, the tle array is allocated with max_cpus and this looks
> > weird. I will dig the specs to try to understand.
>
> ack it looks weird. I keep only the socket structure
[...]
Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology
On Wed, 2022-10-12 at 18:20 +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.
> 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.
>
> Signed-off-by: Pierre Morel
> ---
> 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 00..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;
> +
> +#define S390_TOPOLOGY_CPU_IFL 0x03
> +#define S390_TOPOLOGY_MAX_ORIGIN ((63 + S390_MAX_CPUS) / 64)
> +typedef struct S390TopoTLE {
> +uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
> +} S390TopoTLE;
Since this actually represents multiple TLEs, you might want to change the
name of the struct to reflect this. S390TopoTLEList maybe?
> +
> +struct S390Topology {
> +SysBusDevice parent_obj;
> +int cpus;
> +S390TopoContainer *socket;
> +S390TopoTLE *tle;
> +MachineState *ms;
> +};
> +
> +#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 00..42b22a1831
> --- /dev/null
> +++ b/hw/s390x/cpu-topology.c
> @@ -0,0 +1,132 @@
> +/*
> + * CPU Topology
> + *
> + * Copyright IBM Corp. 2022
> + * Author(s): Pierre Morel
> +
> + * 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.
s/aruments/arguments/
> + * 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
> + */
Not sure if that comment helps if you don't already know how the topology list
works.
> +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->cpus;
> +
> +/*
> + * At the core level, each CPU is represented by a bit in a 64bit
> + * unsigned long which represent the presence of a CPU.
> + * The firmware assume that all CPU in a CPU TLE have the same
s/firmware assume/architecture specifies/
> + * type, polarization and are all dedicated or shared.
> + * In that case the origin variable represents the offset of the first
> + * CPU in the CPU container.
This sentence is repeated further down.
> + * More than 64 CPUs per socket are represented in several CPU containers
> + * inside the socket container.
> + * The only reason to have several S390TopologyCores inside a socket is
> + * to have more than 64 CPUs.
> + * In
Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology
On 10/19/22 17:39, Pierre Morel wrote:
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
---
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 00..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 00..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
+
+ * 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()
LGTM.
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.
As an example, you could look at PPC where the PowerPCCPU CPU model is
shared between two differents machine, a baremetal one PowerNV and the
para-virtual one pSeries/sPAPR. Look for :
pnv_cpu_state(PowerPCCPU *cpu)
spapr_cpu_state(PowerPCCPU *cpu)
the machine CPU state is stored under an opaque cpu->machine_data which is
specific to each machine. It doesn't have to be as complex on s390 since
we only have one type of z-machine. An opaque is a good idea still.
Thanks,
C.
Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology
On Wed, 2022-10-19 at 17:39 +0200, Pierre Morel wrote: > > On 10/18/22 18:43, Cédric Le Goater wrote: [...] > > > > > diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c > > > new file mode 100644 > > > index 00..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. > No, this form, with the date at the end, is the correct one. > [...]
Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology
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
---
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 00..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 00..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
+
+ * 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.
+}
+
+/*
+ * 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
Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology
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.
Signed-off-by: Pierre Morel
---
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 00..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"
+};
+
+#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 00..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.
+ * Author(s): Pierre Morel
+
+ * 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.
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.
+}
+
+/*
+ * 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;
+}
I would move this test in the caller.
+
+socket_id = core_id / topo->cpus;
+
+/*
+ * At the core level, each CPU is represented by a bit in a 64bit
+ * unsigned long which represent the presence of a CPU.
+ * The firmware assume that all CPU in
