Re: [PATCH v20 03/21] target/s390x/cpu topology: handle STSI(15) and build the SYSIB

[Nina Schoetterl-Glausch <nsg@linux.ibm.com>]

On Wed, 2023-05-03 at 10:43 +0200, Pierre Morel wrote:
> On 5/2/23 19:22, Nina Schoetterl-Glausch wrote:
> > On Tue, 2023-04-25 at 18:14 +0200, Pierre Morel wrote:
> > > On interception of STSI(15.1.x) the System Information Block
> > > (SYSIB) is built from the list of pre-ordered topology entries.
> > > 
> > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > > ---
> > >   MAINTAINERS                     |   1 +
> > >   include/hw/s390x/cpu-topology.h |  24 +++
> > >   include/hw/s390x/sclp.h         |   1 +
> > >   target/s390x/cpu.h              |  72 ++++++++
> > >   hw/s390x/cpu-topology.c         |  13 +-
> > >   target/s390x/kvm/cpu_topology.c | 308 ++++++++++++++++++++++++++++++++
> > >   target/s390x/kvm/kvm.c          |   5 +-
> > >   target/s390x/kvm/meson.build    |   3 +-
> > >   8 files changed, 424 insertions(+), 3 deletions(-)
> > >   create mode 100644 target/s390x/kvm/cpu_topology.c
> > > 
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index bb7b34d0d8..de9052f753 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -1659,6 +1659,7 @@ M: Pierre Morel <pmorel@linux.ibm.com>
> > >   S: Supported
> > >   F: include/hw/s390x/cpu-topology.h
> > >   F: hw/s390x/cpu-topology.c
> > > +F: target/s390x/kvm/cpu_topology.c
> > >   
> > >   X86 Machines
> > >   ------------
> > > diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
> > > index af36f634e0..87bfeb631e 100644
> > > --- a/include/hw/s390x/cpu-topology.h
> > > +++ b/include/hw/s390x/cpu-topology.h
> > > @@ -15,9 +15,33 @@
> > > 
> > [...]
> > 
> > > +typedef struct S390TopologyEntry {
> > > +    QTAILQ_ENTRY(S390TopologyEntry) next;
> > > +    s390_topology_id id;
> > > +    uint64_t mask;
> > > +} S390TopologyEntry;
> > > +
> > >   typedef struct S390Topology {
> > >       uint8_t *cores_per_socket;
> > > +    QTAILQ_HEAD(, S390TopologyEntry) list;
> > Since you recompute the list on every STSI, you no longer need this in here.
> > You can create it in insert_stsi_15_1_x.
> 
> Sure but why should we do that?
> 
> It does not change functionality or performance and I do not find it 
> makes the code clearer.
> On the other hand it changes the implementation and the initialization 
> of the list with the sentinel becomes tricky.

IMO, it's a local calculation, so it should be local.
It makes the question "how is this list calculated" when reading the code easier,
because, instead of having to check where a global is accessed you just have to look
at the call stack.

You can just move

+    entry = g_malloc0(sizeof(S390TopologyEntry));
+    entry->id.sentinel = 0xff;
+    QTAILQ_INSERT_HEAD(&s390_topology.list, entry, next);

to s390_topology_fill_list_sorted. And completely free the list in s390_topology_empty_list.
> 
[...]
>