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 <pmo...@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 <pmo...@linux.ibm.com>
> > > > Co-developed-by: Nina Schoetterl-Glausch <n...@linux.ibm.com>
> > > > Signed-off-by: Nina Schoetterl-Glausch <n...@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
> > >
> > >
> >
>
