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@xxxxxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxx>
> > > 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.
>
[...]
>
