Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology

[Janis Schoetterl-Glausch]

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 <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
> > > 
[...]
> > > 
> > > +/*
> > > + * 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, &topo->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

[...]