Re: [kvm-unit-tests PATCH 00/10] s390x: topology: Fixes and extension

[Nina Schoetterl-Glausch <nsg@linux.ibm.com>]

On Wed, 2023-10-25 at 14:21 +0200, Nico Boehr wrote:
> Quoting Nina Schoetterl-Glausch (2023-10-20 16:48:50)
> > v1 -> v2:
> >  * patch 1, introducing enums (Janosch)
> >  * add comment explaining 8 alignment of stsi block length
> >  * unsigned cpu_in_masks, iteration (Nico)
> >  * fix copy paste error when checking ordering (thanks Nina)
> >  * don't escape newline when \\ at end of line in multiline string
> >  * change commit messages (thanks Janosch, thanks Nico)
> >  * pick up tags (thanks Janosch, thanks Nico)
> > 
> > Fix a number of issues as well as rewrite and extend the topology list
> > checking.
> > Add a test case with a complex topology configuration.
> > In order to keep the unittests.cfg file readable, implement multiline
> > strings for extra_params.
> 
> Thanks, I've pushed this to our CI for coverage.

And it found some problems.
Want me to resend the series or just fixup patches?
Preview (copy pasted):
--- a/s390x/topology.c
+++ b/s390x/topology.c
@@ -294,25 +294,38 @@ static union topology_container *check_child_cpus(struct sysinfo_15_1_x *info,
 {
        void *last = ((void *)info) + info->length;
        union topology_cpu *prev_cpu = NULL;
+       bool correct_ordering = true;
        unsigned int cpus = 0;
        int i;

-       for (i = 0; (void *)&child[i] < last && child[i].nl == 0; i++) {
+       for (i = 0; (void *)&child[i] < last && child[i].nl == 0; prev_cpu = &child[i++]) {
                cpus += check_cpu(&child[i], cont);
                if (prev_cpu) {
-                       report(prev_cpu->type <= child[i].type, "Correct ordering wrt type");
+                       if (prev_cpu->type > child[i].type) {
+                               report_info("Incorrect ordering wrt type for child %d", i);
+                               correct_ordering = false;
+                       }
                        if (prev_cpu->type < child[i].type)
                                continue;
-                       report(prev_cpu->pp >= child[i].pp, "Correct ordering wrt polarization");
+                       if (prev_cpu->pp < child[i].pp) {
+                               report_info("Incorrect ordering wrt polarization for child %d", i);
+                               correct_ordering = false;
+                       }
                        if (prev_cpu->pp > child[i].pp)
                                continue;
-                       report(prev_cpu->d || !child[i].d, "Correct ordering wrt dedication");
+                       if (!prev_cpu->d && child[i].d) {
+                               report_info("Incorrect ordering wrt dedication for child %d", i);
+                               correct_ordering = false;
+                       }
                        if (prev_cpu->d && !child[i].d)
                                continue;
-                       report(prev_cpu->origin <= child[i].origin, "Correct ordering wrt origin");
+                       if (prev_cpu->origin > child[i].origin) {
+                               report_info("Incorrect ordering wrt origin for child %d", i);
+                               correct_ordering = false;
+                       }
                }
-               prev_cpu = &child[i];
        }
+       report(correct_ordering, "children correctly ordered");
        report(cpus <= expected_topo_lvl[0], "%d children <= max of %d",
               cpus, expected_topo_lvl[0]);
        *cpus_in_masks += cpus;