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

[Pierre Morel <pmorel@linux.ibm.com>]

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@linux.ibm.com>
>> ---
>>   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@linux.ibm.com>
>>   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.


>>       CpuTopology *smp;
>> +    bool vertical_polarization;
>>   } S390Topology;
> [...]
>
>> +/*
>> + * Macro to check that the size of data after increment
>> + * will not get bigger than the size of the SysIB.
>> + */
>> +#define SYSIB_GUARD(data, x) do {       \
>> +        data += x;                      \
>> +        if (data  > sizeof(SysIB)) {    \
>                      ^ two spaces

right, thanks


>
>> +            return 0;                   \
>> +        }                               \
>> +    } while (0)
>> +
> [...]
>
>> +/**
>> + * s390_topology_from_cpu:
>> + * @cpu: The S390CPU
>> + *
>> + * Initialize the topology id from the CPU environment.
>> + */
>> +static s390_topology_id s390_topology_from_cpu(S390CPU *cpu)
>> +{
>> +    s390_topology_id topology_id = {0};
>> +
>> +    topology_id.drawer = cpu->env.drawer_id;
>> +    topology_id.book = cpu->env.book_id;
>> +    topology_id.socket = cpu->env.socket_id;
>> +    topology_id.origin = cpu->env.core_id / 64;
>> +    topology_id.type = S390_TOPOLOGY_CPU_IFL;
>> +    topology_id.dedicated = cpu->env.dedicated;
>> +
>> +    if (s390_topology.vertical_polarization) {
>> +        /*
>> +         * Vertical polarization with dedicated CPU implies
>> +         * vertical high entitlement.
>> +         */
> This has already been adjusted or rejected when the entitlement was set.
>
>> +        if (topology_id.dedicated) {
>> +            topology_id.entitlement = S390_CPU_ENTITLEMENT_HIGH;
>> +        } else {
>> +            topology_id.entitlement = cpu->env.entitlement;
> You only need this assignment.


Right, thanks


Regards,

Pierre