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

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

On 4/27/23 10:47, Nico Boehr wrote:
> Quoting Pierre Morel (2023-04-26 10:34:26)
>> diff --git a/s390x/topology.c b/s390x/topology.c
>> index 07f1650..42a9cc9 100644
>> --- a/s390x/topology.c
>> +++ b/s390x/topology.c
>> @@ -17,6 +17,20 @@
> [...]
>> +/**
>> + * 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");
> Again, I would prefer something like this:
>
> if (!cpus->d)
>      report_skip("Not dedicated")
> else
>      report(cpus->pp == 3 || cpus->pp == 0, "Dedicated CPUs are either vertically polarized or have high entitlement")
>
> No?
>
> [...]

Yes looks better, thanks


>
>> +/**
>> + * check_sysinfo_15_1_x:
>> + * @info: pointer to the STSI info structure
>> + * @sel2: the selector giving the topology level to check
>> + *
>> + * Check if the validity of the STSI instruction and then
>> + * calls specific checks on the information buffer.
>> + */
>> +static void check_sysinfo_15_1_x(struct sysinfo_15_1_x *info, int sel2)
>> +{
>> +       int ret;
>> +       int cc;
>> +       unsigned long rc;
>> +
>> +       report_prefix_pushf("15_1_%d", sel2);
>> +
>> +       ret = stsi_get_sysib(info, sel2);
>> +       if (ret) {
>> +               report_skip("Selector 2 not supported by architecture");
>> +               goto end;
>> +       }
>> +
>> +       report_prefix_pushf("H");
>> +       cc = ptf(PTF_REQ_HORIZONTAL, &rc);
>> +       if (cc != 0 && rc != PTF_ERR_ALRDY_POLARIZED) {
>> +               report(0, "Unable to set horizontal polarization");
> report_fail() please

OK


>
>> +               goto vertical;
>> +       }
>> +
>> +       stsi_check_mag(info);
>> +       stsi_check_tle_coherency(info);
>> +
>> +vertical:
>> +       report_prefix_pop();
>> +       report_prefix_pushf("V");
>> +
>> +       cc = ptf(PTF_REQ_VERTICAL, &rc);
>> +       if (cc != 0 && rc != PTF_ERR_ALRDY_POLARIZED) {
>> +               report(0, "Unable to set vertical polarization");
> report_fail() please

OK


>
> [...]
>> +static int arch_max_cpus(void)
> Does the name arch_max_cpus() make sense? Maybe expected_num_cpus()?


Yes OK.


>
>> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
>> index fc3666b..375e6ce 100644
>> --- a/s390x/unittests.cfg
>> +++ b/s390x/unittests.cfg
>> @@ -221,3 +221,6 @@ file = ex.elf
>>   
>>   [topology]
>>   file = topology.elf
>> +# 3 CPUs on socket 0 with different CPU TLE (standard, dedicated, origin)
>> +# 1 CPU on socket 2
>> +extra_params = -smp 1,drawers=3,books=3,sockets=4,cores=4,maxcpus=144 -cpu z14,ctop=on -device z14-s390x-cpu,core-id=1,entitlement=low -device z14-s390x-cpu,core-id=2,dedicated=on -device z14-s390x-cpu,core-id=10 -device z14-s390x-cpu,core-id=20 -device z14-s390x-cpu,core-id=130,socket-id=0,book-id=0,drawer-id=0 -append '-drawers 3 -books 3 -sockets 4 -cores 4'
> If I got the command line right, all CPUs are on the same drawer with this command line, aren't they? If so, does it make sense to run with different combinations, i.e. CPUs on different drawers, books etc?

OK, I will add some CPU on different drawers and books.