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

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

On 3/24/23 11:59, Nico Boehr wrote:
> Quoting Pierre Morel (2023-03-20 09:56:42)
>> STSI with function code 15 is used to store the CPU configuration
>> topology.
>>
>> We retrieve the maximum nested level with SCLP and use the
>> topology tree provided by the drawers, books, sockets, cores
>> arguments.
>>
>> 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.
> [...]
>> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
>> index 390fde7..9679332 100644
>> --- a/lib/s390x/sclp.c
>> +++ b/lib/s390x/sclp.c
>> @@ -238,3 +238,8 @@ uint64_t get_max_ram_size(void)
>>   {
>>          return max_ram_size;
>>   }
>> +
>> +uint64_t sclp_get_stsi_mnest(void)
>> +{
> maybe add:
> assert(read_info);

right


>
> [...]
>> diff --git a/s390x/topology.c b/s390x/topology.c
>> index ce248f1..11ce931 100644
>> --- a/s390x/topology.c
>> +++ b/s390x/topology.c
> [...]
>> +/*
>> + * Topology level as defined by architecture, all levels exists with
>> + * a single container unless overwritten by the QEMU -smp parameter.
>> + */
>> +static int arch_topo_lvl[CPU_TOPOLOGY_MAX_LEVEL]; // = {1, 1, 1, 1, 1, 1};
> So that's what is being provided to the test on the command line, right?
>
> How about renaming this to expected_topo_lvl?
>
> What do you mean by 'defined by architecture'?

This is what is provided by the boot arguments and should correspond to 
the physical topology.

The test checks that this is corresponding to what LPAR or QEMU shows in 
the SYSIB.

If a topology level always exist physically and if it is not specified 
on the QEMU command line it is implicitly unique.

OK for expected_topo_lvl if you prefer.


>
> [...]
>> +/*
>> + * stsi_check_mag
>> + * @info: Pointer to the stsi information
>> + *
>> + * MAG field should match the architecture defined containers
>> + * when MNEST as returned by SCLP matches MNEST of the SYSIB.
>> + */
>> +static void stsi_check_mag(struct sysinfo_15_1_x *info)
>> +{
>> +       int i;
>> +
>> +       report_prefix_push("MAG");
>> +
>> +       stsi_check_maxcpus(info);
>> +
>> +       /* Explicitly skip the test if both mnest do not match */
>> +       if (max_nested_lvl != info->mnest)
>> +               goto done;
> What does it mean if the two don't match, i.e. is this an error? Or a skip? Or is it just expected?

I have no information on the representation of the MAG fields for a 
SYSIB with a nested level different than the maximum nested level.

There are examples in the documentation but I did not find, and did not 
get a clear answer, on how the MAG field are calculated.

The examples seems clear for info->mnest between MNEST -1 and 3 but the 
explication I had on info->mnest = 2 is not to be found in any 
documentation.

Until it is specified in a documentation I skip all these tests.


>
> [...]
>> +/**
>> + * check_tle:
>> + * @tc: pointer to first TLE
>> + *
>> + * Recursively check the containers TLEs until we
>> + * find a CPU TLE.
>> + */
>> +static uint8_t *check_tle(void *tc)
>> +{
> [...]
>> +       report(!cpus->d || (cpus->pp == 3 || cpus->pp == 0),
>> +              "Dedication versus entitlement");
> Maybe skip here if the CPU is not dedicated? With shared CPUs we really can't check much here.


OK


> [...]
>> +/*
>> + * The Maximum Nested level is given by SCLP READ_SCP_INFO if the MNEST facility
>> + * is available.
>> + * If the MNEST facility is not available, sclp_get_stsi_mnest  returns 0 and the
>> + * Maximum Nested level is 2
>> + */
>> +#define S390_DEFAULT_MNEST     2
>> +static int sclp_get_mnest(void)
>> +{
>> +       return sclp_get_stsi_mnest() ?: S390_DEFAULT_MNEST;
>> +}
>> +
>> +static int arch_max_cpus(void)
> If arch_topo_lvl is renamed, also rename this function accordingly.

OK


>
>>   static struct {
>>          const char *name;
>>          void (*func)(void);
>>   } tests[] = {
>>          { "PTF", test_ptf},
>> +       { "STSI", test_stsi},
> missing space                ^


Right, thanks,


regards,

Pierre