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

[Janis Schoetterl-Glausch]

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 <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
> > > 
> > > diff --git a/include/hw/s390x/cpu-topology.h 
> > > b/include/hw/s390x/cpu-topology.h
> > > new file mode 100644
> > > index 0000000000..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
> 
>