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

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

On 2/27/23 14:21, Nina Schoetterl-Glausch wrote:
> On Wed, 2023-02-22 at 15:20 +0100, 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>
>> ---
>>   include/hw/s390x/cpu-topology.h |  21 +++
>>   include/hw/s390x/sclp.h         |   1 +
>>   target/s390x/cpu.h              |  72 ++++++++
>>   hw/s390x/cpu-topology.c         |  14 +-
>>   target/s390x/kvm/cpu_topology.c | 312 ++++++++++++++++++++++++++++++++
>>   target/s390x/kvm/kvm.c          |   5 +-
>>   target/s390x/kvm/meson.build    |   3 +-
>>   7 files changed, 425 insertions(+), 3 deletions(-)
>>   create mode 100644 target/s390x/kvm/cpu_topology.c
>>
>> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
>> index fa7f885a9f..8dc42d2942 100644
>> --- a/include/hw/s390x/cpu-topology.h
>> +++ b/include/hw/s390x/cpu-topology.h
>> @@ -16,8 +16,29 @@
>>   
>>   #define S390_TOPOLOGY_CPU_IFL   0x03
>>   
>> +typedef union s390_topology_id {
>> +    uint64_t id;
>> +    struct {
>> +        uint8_t level5;
> You could rename this to sentinel, since that's the only use case and
> if there ever is another level the sentinel implementation might need
> to be changed anyway.


OK


>
>> +        uint8_t drawer;
>> +        uint8_t book;
>> +        uint8_t socket;
>> +        uint8_t dedicated;
>> +        uint8_t entitlement;
>> +        uint8_t type;
>> +        uint8_t origin;
>> +    };
>> +} s390_topology_id;
>> +
>>
> [...]
>
>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>> index d654267a71..c899f4e04b 100644
>> --- a/target/s390x/cpu.h
>> +++ b/target/s390x/cpu.h
>> @@ -560,6 +560,25 @@ typedef struct SysIB_322 {
>>
> [...]
>>   
>> +/*
>> + * CPU Topology List provided by STSI with fc=15 provides a list
>> + * of two different Topology List Entries (TLE) types to specify
>> + * the topology hierarchy.
>> + *
>> + * - Container Topology List Entry
>> + *   Defines a container to contain other Topology List Entries
>> + *   of any type, nested containers or CPU.
>> + * - CPU Topology List Entry
>> + *   Specifies the CPUs position, type, entitlement and polarization
>> + *   of the CPUs contained in the last Container TLE.
>> + *
>> + * There can be theoretically up to five levels of containers, QEMU
>> + * uses only three levels, the drawer's, book's and socket's level.
>> + *
>> + * A container of with a nesting level (NL) greater than 1 can only
> s/of//


thanks.


>
>> + * contain another container of nesting level NL-1.
>> + *
>> + * A container of nesting level 1 (socket), contains as many CPU TLE
>> + * as needed to describe the position and qualities of all CPUs inside
>> + * the container.
>> + * The qualities of a CPU are polarization, entitlement and type.
>> + *
>> + * The CPU TLE defines the position of the CPUs of identical qualities
>> + * using a 64bits mask which first bit has its offset defined by
>> + * the CPU address orgin field of the CPU TLE like in:
>> + * CPU address = origin * 64 + bit position within the mask
>> + *
>> + */
>> +/* Container type Topology List Entry */
>> +typedef struct SysIBTl_container {
>> +        uint8_t nl;
>> +        uint8_t reserved[6];
>> +        uint8_t id;
>> +} QEMU_PACKED QEMU_ALIGNED(8) SysIBTl_container;
>> +QEMU_BUILD_BUG_ON(sizeof(SysIBTl_container) != 8);
>> +
> [...]
>> +
>> +/**
>> + * 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.polarization == S390_CPU_POLARIZATION_VERTICAL) {
>> +        /*
>> +         * Vertical polarization with dedicated CPU implies
>> +         * vertical high entitlement.
>> +         */
>> +        if (topology_id.dedicated) {
>> +            topology_id.entitlement = S390_CPU_ENTITLEMENT_HIGH;
>> +        } else {
>> +            topology_id.entitlement = cpu->env.entitlement;
>> +        }
> I don't see why you need this if, it should already be correct.
>
>> +    }
> I'd suggest the following:
> * rename entitlement in s390_topology_id back to polarization, but keep entitlement everywhere else.
> * remove horizontal/none from CpuS390Entitlement, this way the user cannot set it,
> 	and it doesn't show up in the output of query-cpus-fast.
> * this is where you convert between the two, so:
> 	if horizontal, id.polarization = 0,
> 	otherwise id.polarization = entitlement + 1, or a switch case.
> * in patch 6 in s390_topology_set_cpus_entitlement you don't set the entitlement if the polarization
> 	is horizontal, which is ok because of the conversion above.

I do not like to remove the horizontal entitlement from the enum because 
every
existing s390 tool show "horizontal" entitlement when the polarization 
is horizontal.

See, lscpu -e or "/sys/devices/system/cpu/cpu*/polarization"

Also, the user may find strange that a field is missing depending of the 
polarization in the output of query-cpu-fast.

Regards,

Pierre