Re: [kvm-unit-tests PATCH v3 4/4] s390x: topology: Checking Configuration Topology Information

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

On 1/11/22 14:30, Janosch Frank wrote:
> On 1/10/22 14:37, Pierre Morel wrote:
>> STSI with function code 15 is used to store the CPU configuration
>> topology.
>>
>> We check :
>> - if the topology stored is coherent between the QEMU -smp
>>    parameters and kernel parameters.
>> - the number of CPUs
>> - the maximum number of CPUs
>> - the number of containers of each levels for every STSI(15.1.x)
>>    instruction allowed by the machine.
> 
> The full review of this will take some time.
> 
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/stsi.h    |  44 +++++++++
>>   s390x/topology.c    | 231 ++++++++++++++++++++++++++++++++++++++++++++
>>   s390x/unittests.cfg |   1 +
>>   3 files changed, 276 insertions(+)
>>
>> diff --git a/lib/s390x/stsi.h b/lib/s390x/stsi.h
>> index 02cc94a6..e3fc7ac0 100644
>> --- a/lib/s390x/stsi.h
>> +++ b/lib/s390x/stsi.h
>> @@ -29,4 +29,48 @@ struct sysinfo_3_2_2 {
>>       uint8_t ext_names[8][256];
>>   };
>> +struct topology_core {
>> +    uint8_t nl;
>> +    uint8_t reserved1[3];
>> +    uint8_t reserved4:5;
>> +    uint8_t d:1;
>> +    uint8_t pp:2;
>> +    uint8_t type;
>> +    uint16_t origin;
>> +    uint64_t mask;
>> +};
>> +
>> +struct topology_container {
>> +    uint8_t nl;
>> +    uint8_t reserved[6];
>> +    uint8_t id;
>> +};
>> +
>> +union topology_entry {
>> +    uint8_t nl;
>> +    struct topology_core cpu;
>> +    struct topology_container container;
>> +};
>> +
>> +#define CPU_TOPOLOGY_MAX_LEVEL 6
>> +struct sysinfo_15_1_x {
>> +    uint8_t reserved0[2];
>> +    uint16_t length;
>> +    uint8_t mag[CPU_TOPOLOGY_MAX_LEVEL];
>> +    uint8_t reserved10;
> 
> reserved0a?

OK

> 
>> +    uint8_t mnest;
>> +    uint8_t reserved12[4];
> 
> reserved0c?

OK

> 
>> +    union topology_entry tle[0];

...snip...

>> +static void stsi_check_tle_coherency(struct sysinfo_15_1_x *info, int 
>> sel2)
>> +{
>> +    struct topology_container *tc, *end;
>> +    struct topology_core *cpus;
>> +    int n = 0;
>> +    int i;
>> +
>> +    report_prefix_push("TLE coherency");
>> +
>> +    tc = (void *)&info->tle[0];
> 
> tc = &info->tle[0].container ?

Yes, clearly better than a cast.

> 
>> +    end = (struct topology_container *)((unsigned long)info + 
...snip...
>> +    /* For each level found in STSI */
>> +    for (i = 1; i < CPU_TOPOLOGY_MAX_LEVEL; i++) {
>> +        /*
>> +         * For non QEMU/KVM hypervizor the concatanation of the levels
> 
> hypervisor
> 
> concatenation

Yes, thanks.

> 
>> +         * above level 1 are architecture dependent.

...snip...

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen