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

[Thomas Huth <thuth@redhat.com>]

On 12/07/2023 16.24, Pierre Morel wrote:
> 
> On 7/4/23 13:40, Thomas Huth wrote:
>> On 30/06/2023 11.17, 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>
>>> ---
>> ...
>>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>>> index 7ebd5e05b6..6e7d041b01 100644
>>> --- a/target/s390x/cpu.h
>>> +++ b/target/s390x/cpu.h
>>> @@ -569,6 +569,29 @@ typedef struct SysIB_322 {
>>>   } SysIB_322;
>>>   QEMU_BUILD_BUG_ON(sizeof(SysIB_322) != 4096);
>>>   +/*
>>> + * Topology Magnitude fields (MAG) indicates the maximum number of
>>> + * topology list entries (TLE) at the corresponding nesting level.
>>> + */
>>> +#define S390_TOPOLOGY_MAG  6
>>> +#define S390_TOPOLOGY_MAG6 0
>>> +#define S390_TOPOLOGY_MAG5 1
>>> +#define S390_TOPOLOGY_MAG4 2
>>> +#define S390_TOPOLOGY_MAG3 3
>>> +#define S390_TOPOLOGY_MAG2 4
>>> +#define S390_TOPOLOGY_MAG1 5
>>> +/* Configuration topology */
>>> +typedef struct SysIB_151x {
>>> +    uint8_t  reserved0[2];
>>> +    uint16_t length;
>>> +    uint8_t  mag[S390_TOPOLOGY_MAG];
>>> +    uint8_t  reserved1;
>>> +    uint8_t  mnest;
>>> +    uint32_t reserved2;
>>> +    char tle[];
>>> +} SysIB_151x;
>>> +QEMU_BUILD_BUG_ON(sizeof(SysIB_151x) != 16);
>>> +
>>>   typedef union SysIB {
>>>       SysIB_111 sysib_111;
>>>       SysIB_121 sysib_121;
>>> @@ -576,9 +599,62 @@ typedef union SysIB {
>>>       SysIB_221 sysib_221;
>>>       SysIB_222 sysib_222;
>>>       SysIB_322 sysib_322;
>>> +    SysIB_151x sysib_151x;
>>>   } SysIB;
>>>   QEMU_BUILD_BUG_ON(sizeof(SysIB) != 4096);
>>>   +/*
>>> + * 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 with a nesting level (NL) greater than 1 can only
>>> + * 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;
>>> +} SysIBTl_container;
>>
>> Why mixing CamelCase with underscore-style here? SysIBTlContainer would 
>> look more natural, I think?
> 
> 
> OK, what about SYSIBContainerListEntry ?

Sounds fine!

> 
>>
>>> +QEMU_BUILD_BUG_ON(sizeof(SysIBTl_container) != 8);
>>> +
>>> +/* CPU type Topology List Entry */
>>> +typedef struct SysIBTl_cpu {
>>> +        uint8_t nl;
>>> +        uint8_t reserved0[3];
>>> +#define SYSIB_TLE_POLARITY_MASK 0x03
>>> +#define SYSIB_TLE_DEDICATED     0x04
>>> +        uint8_t flags;
>>> +        uint8_t type;
>>> +        uint16_t origin;
>>> +        uint64_t mask;
>>> +} SysIBTl_cpu;
>>
>> dito, maybe better SysIBTlCpu ?
> 
> 
> What about SysIBCPUListEntry ?

Ack.

  Thomas