Re: [PATCH v8 02/12] s390x/cpu_topology: CPU topology objects and structures

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

On 7/4/22 13:47, Janosch Frank wrote:
> On 6/29/22 17:25, Pierre Morel wrote:
>>
>>
>> On 6/27/22 15:31, Janosch Frank wrote:
>>> On 6/20/22 16:03, Pierre Morel wrote:
>>>> We use new objects to have a dynamic administration of the CPU 
>>>> topology.
>>>> The highest level object in this implementation is the s390 book and
>>>> in this first implementation of CPU topology for S390 we have a single
>>>> book.
>>>> The book is built as a SYSBUS bridge during the CPU initialization.
>>>> Other objects, sockets and core will be built after the parsing
>>>> of the QEMU -smp argument.
>>>>
>>>> Every object under this single book will be build dynamically
>>>> immediately after a CPU has be realized if it is needed.
>>>> The CPU will fill the sockets once after the other, according to the
>>>> number of core per socket defined during the smp parsing.
>>>>
>>>> Each CPU inside a socket will be represented by a bit in a 64bit
>>>> unsigned long. Set on plug and clear on unplug of a CPU.
>>>>
>>>> For the S390 CPU topology, thread and cores are merged into
>>>> topology cores and the number of topology cores is the multiplication
>>>> of cores by the numbers of threads.
>>>>
>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>
>>> [...]
>>>
>>>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>>>> index 7d6d01325b..216adfde26 100644
>>>> --- a/target/s390x/cpu.h
>>>> +++ b/target/s390x/cpu.h
>>>> @@ -565,6 +565,53 @@ typedef union SysIB {
>>>>    } SysIB;
>>>>    QEMU_BUILD_BUG_ON(sizeof(SysIB) != 4096);
>>>> +/* CPU type Topology List Entry */
>>>> +typedef struct SysIBTl_cpu {
>>>> +        uint8_t nl;
>>>> +        uint8_t reserved0[3];
>>>> +        uint8_t reserved1:5;
>>>> +        uint8_t dedicated:1;
>>>> +        uint8_t polarity:2;
>>>> +        uint8_t type;
>>>> +        uint16_t origin;
>>>> +        uint64_t mask;
>>>> +} SysIBTl_cpu;
>>>> +QEMU_BUILD_BUG_ON(sizeof(SysIBTl_cpu) != 16);
>>>> +
>>>> +/* Container type Topology List Entry */
>>>> +typedef struct SysIBTl_container {
>>>> +        uint8_t nl;
>>>> +        uint8_t reserved[6];
>>>> +        uint8_t id;
>>>> +} QEMU_PACKED SysIBTl_container;
>>>> +QEMU_BUILD_BUG_ON(sizeof(SysIBTl_container) != 8);
>>>> +
>>>> +/* Generic Topology List Entry */
>>>> +typedef union SysIBTl_entry {
>>>> +        uint8_t nl;
>>>
>>> This union member is unused, isn't it?
>>>
>>>> +        SysIBTl_container container;
>>>> +        SysIBTl_cpu cpu;
>>>> +} SysIBTl_entry;
>>>> +
>>>> +#define TOPOLOGY_NR_MAG  6
>>>
>>> TOPOLOGY_TOTAL_NR_MAGS ?
>>>
>>>> +#define TOPOLOGY_NR_MAG6 0
>>>
>>> TOPOLOGY_NR_TLES_MAG6 ?
>>>
>>> I'm open to other suggestions but we need to differentiate between the
>>> number of mag array entries and the number of TLEs in the MAGs.
>>
>>
>> typedef enum {
>>           TOPOLOGY_MAG6 = 0,
>>           TOPOLOGY_MAG5 = 1,
>>           TOPOLOGY_MAG4 = 2,
>>           TOPOLOGY_MAG3 = 3,
>>           TOPOLOGY_MAG2 = 4,
>>           TOPOLOGY_MAG1 = 5,
>>           TOPOLOGY_TOTAL_MAGS = 6,
>> };
>>
>>
>> oder enum with TOPOLOGY_NR_TLES_MAGx ?
> 
> I'd stick with the shorter first variant.


OK, thanks

> 
>>
>>>
>>>> +#define TOPOLOGY_NR_MAG5 1
>>>> +#define TOPOLOGY_NR_MAG4 2
>>>> +#define TOPOLOGY_NR_MAG3 3
>>>> +#define TOPOLOGY_NR_MAG2 4
>>>> +#define TOPOLOGY_NR_MAG1 5
>>>
>>> I'd appreciate a \n here.
>>
>> OK
>>
>>>
>>>> +/* Configuration topology */
>>>> +typedef struct SysIB_151x {
>>>> +    uint8_t  res0[2];
>>>
>>> You're using "reserved" everywhere but now it's "rev"?
>>
>> OK I will keep reserved
>>
>>>
>>>> +    uint16_t length;
>>>> +    uint8_t  mag[TOPOLOGY_NR_MAG];
>>>> +    uint8_t  res1;
>>>> +    uint8_t  mnest;
>>>> +    uint32_t res2;
>>>> +    SysIBTl_entry tle[0];
>>>> +} SysIB_151x;
>>>> +QEMU_BUILD_BUG_ON(sizeof(SysIB_151x) != 16);
>>>> +
>>>>    /* MMU defines */
>>>>    #define ASCE_ORIGIN           (~0xfffULL) /* segment table
>>>> origin             */
>>>>    #define ASCE_SUBSPACE         0x200       /* subspace group
>>>> control           */
>>>
>>>
>>
> 

-- 
Pierre Morel
IBM Lab Boeblingen