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

[Thomas Huth <thuth@redhat.com>]

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?

>>> +    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 
ordering? ... that's very ugly! I'd prefer to be more verbose and compare 
book by book, drawer by drawer, etc. instread. Or is this function that 
performance critical that we must save every possible CPU cycle here?

  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
>>
>>
>