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

[Nina Schoetterl-Glausch]

On Wed, 2023-09-06 at 10:21 +0200, Thomas Huth wrote:
> On 05/09/2023 17.25, Nina Schoetterl-Glausch wrote:
> > 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>
> > > > ---
> ...
> > > > +/**
> > > > + * 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);
> 
> Since you don't do swapping for entry->id.id below, why do you do it here?

Because an integer in cpu endianess is converted to the big endian
storage format. So then there is a cpu -> big -> cpu round trip with
the comparison below and the value is the max.
Of course this is entirely cosmetic, since UINT64_MAX is all ones.

> > > > +    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)
> >   */
> 
> Ugh, so this swapping is not due to real endianness issues, but just due to

Yeah.

> ordering? ... that's very ugly! I'd prefer to be more verbose and compare 

I kinda didn't like the verbosity of it, since I then need to copy
paste the whole thing because I also need an equality check.
I considered implementing <=, then a == b as a <= b && b <= a, which
seems fine on second thought, so I'll do that. 
And maybe help the compiler by putting __attribute__((pure)) on there.

> book by book, drawer by drawer, etc. instread. Or is this function that 
> performance critical that we must save every possible CPU cycle here?

No.

> 
>   Thomas
> 
> 
> > 
> > > > +            } 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
> > > 
> > > 
> > 
>