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

[Nina Schoetterl-Glausch]

On Tue, 2023-09-05 at 15:26 +0200, Thomas Huth wrote:
> On 01/09/2023 17.57, Nina Schoetterl-Glausch wrote:
> > From: Pierre Morel <pmorel@linux.ibm.com>
> > 
> > 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>
> > Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> > ---
> >   MAINTAINERS                      |   1 +
> >   qapi/machine-target.json         |  14 ++
> >   include/hw/s390x/cpu-topology.h  |  25 +++
> >   include/hw/s390x/sclp.h          |   1 +
> >   target/s390x/cpu.h               |  76 ++++++++
> >   hw/s390x/cpu-topology.c          |   2 +
> >   target/s390x/kvm/kvm.c           |   5 +-
> >   target/s390x/kvm/stsi-topology.c | 296 +++++++++++++++++++++++++++++++
> >   target/s390x/kvm/meson.build     |   3 +-
> >   9 files changed, 421 insertions(+), 2 deletions(-)
> >   create mode 100644 target/s390x/kvm/stsi-topology.c

[...]

> > diff --git a/include/hw/s390x/cpu-topology.h 
> > b/include/hw/s390x/cpu-topology.h
> > index 97b0af2795..fc15acf297 100644
> > --- a/include/hw/s390x/cpu-topology.h
> > +++ b/include/hw/s390x/cpu-topology.h
> > @@ -15,10 +15,35 @@
> >   #include "hw/boards.h"
> >   #include "qapi/qapi-types-machine-target.h"
> >   
> > +#define S390_TOPOLOGY_CPU_IFL   0x03
> > +
> > +typedef union s390_topology_id {
> > +    uint64_t id;
> > +    struct {
> > +        uint8_t _reserved0;
> > +        uint8_t drawer;
> > +        uint8_t book;
> > +        uint8_t socket;
> > +        uint8_t type;
> > +        uint8_t inv_polarization;
> 
> What sense does it make to store the polarization in an inverted way? ... I 
> don't get that ... could you please at least add a comment somewhere for the 
> rationale?
> 

It inverts the ordering with regards to polarization, as required by
the  PoP. The dedication is inverted for the same reason, dedicated
CPUs show up before non dedicated ones, so the id must have a lower
value.
I will add a comment.

> > +        uint8_t not_dedicated;
> > +        uint8_t origin;
> > +    };
> > +} s390_topology_id;

[...]

> > + * fill_tle_cpu:
> > + * @p: The address of the CPU TLE to fill
> > + * @entry: a pointer to the S390TopologyEntry defining this
> > + *         CPU container.
> > + *
> > + * Returns the next free TLE entry.
> > + */
> > +static char *fill_tle_cpu(char *p, S390TopologyEntry *entry)
> > +{
> > +    SysIBCPUListEntry *tle = (SysIBCPUListEntry *)p;
> > +    s390_topology_id topology_id = entry->id;
> > +
> > +    tle->nl = 0;
> > +    tle->flags = 3 - topology_id.inv_polarization;
> 
> Can you avoid the magic number 3 here?

Hmm, any number larger than 2 will do.
I could also use a int8_t and just negate, but relying on the
reinterpretation of two's complement is also magical.
I guess S390_CPU_ENTITLEMENT_HIGH makes the most sense.

[...]

> > +/**
> > + * s390_topology_fill_list_sorted:
> > + *
> > + * Loop over all CPU and insert it at the right place
> > + * inside the TLE entry list.
> > + * Fill the S390Topology list with entries according to the order
> > + * specified by the PoP.
> > + */
> > +static void s390_topology_fill_list_sorted(S390TopologyList *topology_list)
> > +{
> > +    CPUState *cs;
> > +    S390TopologyEntry sentinel;
> > +
> > +    QTAILQ_INIT(topology_list);
> > +
> > +    sentinel.id.id = cpu_to_be64(UINT64_MAX);
> > +    QTAILQ_INSERT_HEAD(topology_list, &sentinel, next);
> > +
> > +    CPU_FOREACH(cs) {
> > +        s390_topology_id id = s390_topology_from_cpu(S390_CPU(cs));
> > +        S390TopologyEntry *entry, *tmp;
> > +
> > +        QTAILQ_FOREACH(tmp, topology_list, next) {
> > +            if (id.id == tmp->id.id) {
> > +                entry = tmp;
> > +                break;

I think I'll add a comment here.

/*
 * Earlier bytes have higher order -> big endian.
 * E.g. an entry with higher drawer number should be later in the list,
 * no matter the later fields (book, socket, etc)
 */


> > +            } else if (be64_to_cpu(id.id) < be64_to_cpu(tmp->id.id)) {
> > +                entry = g_malloc0(sizeof(*entry));
> 
> Maybe nicer to use g_new0 here instead?

I don't think it makes much of a difference.

> 
> > +                entry->id.id = id.id;
> 
> Should this get a cpu_to_be64() ?

No, there is no interpretation of the value here, just a copy.
> 
> > +                QTAILQ_INSERT_BEFORE(tmp, entry, next);
> > +                break;
> > +            }
> > +        }
> > +        s390_topology_add_cpu_to_entry(entry, S390_CPU(cs));
> > +    }
> > +
> > +    QTAILQ_REMOVE(topology_list, &sentinel, next);
> > +}
> 
>   Thomas
> 
>