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 <pmorel@xxxxxxxxxxxxx>
---
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
