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.
>
>
> > 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;
> > > +}
> > > +
> > [...]
>
