[kvm-unit-tests PATCH 0/9] s390x: topology: Fixes and extension

[kvm-unit-tests PATCH 0/9] s390x: topology: Fixes and extension

[Nina Schoetterl-Glausch]

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.

Nina Schoetterl-Glausch (9):
  s390x: topology: Fix report message
  s390x: topology: Use parameter in stsi_get_sysib
  s390x: topology: Fix parsing loop
  s390x: topology: Don't use non unique message
  s390x: topology: Refine stsi header test
  s390x: topology: Rename topology_core to topology_cpu
  s390x: topology: Rewrite topology list test
  scripts: Implement multiline strings for extra_params
  s390x: topology: Add complex topology test

 scripts/common.bash  |  11 +++
 scripts/runtime.bash |   4 +-
 lib/s390x/stsi.h     |  36 ++++---
 s390x/topology.c     | 228 ++++++++++++++++++++++++++-----------------
 s390x/unittests.cfg  | 133 +++++++++++++++++++++++++
 5 files changed, 303 insertions(+), 109 deletions(-)


base-commit: 09e8c119b4cd7b615ea0ece16c92c79054dfb38d
-- 
2.41.0

[kvm-unit-tests PATCH 1/9] s390x: topology: Fix report message

[Nina Schoetterl-Glausch]

A polarization value of 0 means horizontal polarization.

Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
---
 s390x/topology.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/s390x/topology.c b/s390x/topology.c
index 69558236..53838ed1 100644
--- a/s390x/topology.c
+++ b/s390x/topology.c
@@ -275,7 +275,7 @@ static uint8_t *check_tle(void *tc)
 	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");
+		report(cpus->pp == 3 || cpus->pp == 0, "Dedicated CPUs are either horizontally polarized or have high entitlement");
 
 	return tc + sizeof(*cpus);
 }
-- 
2.41.0

[kvm-unit-tests PATCH 2/9] s390x: topology: Use parameter in stsi_get_sysib

[Nina Schoetterl-Glausch]

Instead of accessing global pagebuf.

Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
---
 s390x/topology.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/s390x/topology.c b/s390x/topology.c
index 53838ed1..e1bb6014 100644
--- a/s390x/topology.c
+++ b/s390x/topology.c
@@ -322,7 +322,7 @@ static int stsi_get_sysib(struct sysinfo_15_1_x *info, int sel2)
 
 	report_prefix_pushf("SYSIB");
 
-	ret = stsi(pagebuf, 15, 1, sel2);
+	ret = stsi(info, 15, 1, sel2);
 
 	if (max_nested_lvl >= sel2) {
 		report(!ret, "Valid instruction");
-- 
2.41.0

[kvm-unit-tests PATCH 3/9] s390x: topology: Fix parsing loop

[Nina Schoetterl-Glausch]

Without a comparison the loop is infinite.

Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
---
 s390x/topology.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/s390x/topology.c b/s390x/topology.c
index e1bb6014..49d6dfeb 100644
--- a/s390x/topology.c
+++ b/s390x/topology.c
@@ -466,7 +466,7 @@ static void parse_topology_args(int argc, char **argv)
 		if (flag[0] != '-')
 			report_abort("Argument is expected to begin with '-'");
 		flag++;
-		for (level = 0; ARRAY_SIZE(levels); level++) {
+		for (level = 0; level < ARRAY_SIZE(levels); level++) {
 			if (!strcmp(levels[level], flag))
 				break;
 		}
-- 
2.41.0

[kvm-unit-tests PATCH 4/9] s390x: topology: Don't use non unique message

[Nina Schoetterl-Glausch]

When we test something, i.e. do a report() we want unique messages,
otherwise, from the test output, it will appear as if the same test was
run multiple times, possible with different PASS/FAIL values.

Convert some reports that don't actually test anything topology specific
into asserts.
Refine the report message for others.

Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
---
 s390x/topology.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/s390x/topology.c b/s390x/topology.c
index 49d6dfeb..5374582f 100644
--- a/s390x/topology.c
+++ b/s390x/topology.c
@@ -114,7 +114,7 @@ static void check_polarization_change(void)
 	report_prefix_push("Polarization change");
 
 	/* We expect a clean state through reset */
-	report(diag308_load_reset(1), "load normal reset done");
+	assert(diag308_load_reset(1));
 
 	/*
 	 * Set vertical polarization to verify that RESET sets
@@ -123,7 +123,7 @@ static void check_polarization_change(void)
 	cc = ptf(PTF_REQ_VERTICAL, &rc);
 	report(cc == 0, "Set vertical polarization.");
 
-	report(diag308_load_reset(1), "load normal reset done");
+	assert(diag308_load_reset(1));
 
 	cc = ptf(PTF_CHECK, &rc);
 	report(cc == 0, "Reset should clear topology report");
@@ -137,25 +137,25 @@ static void check_polarization_change(void)
 	report(cc == 0, "Change to vertical");
 
 	cc = ptf(PTF_CHECK, &rc);
-	report(cc == 1, "Should report");
+	report(cc == 1, "Should report change after horizontal -> vertical");
 
 	cc = ptf(PTF_REQ_VERTICAL, &rc);
 	report(cc == 2 && rc == PTF_ERR_ALRDY_POLARIZED, "Double change to vertical");
 
 	cc = ptf(PTF_CHECK, &rc);
-	report(cc == 0, "Should not report");
+	report(cc == 0, "Should not report change after vertical -> vertical");
 
 	cc = ptf(PTF_REQ_HORIZONTAL, &rc);
 	report(cc == 0, "Change to horizontal");
 
 	cc = ptf(PTF_CHECK, &rc);
-	report(cc == 1, "Should Report");
+	report(cc == 1, "Should report change after vertical -> horizontal");
 
 	cc = ptf(PTF_REQ_HORIZONTAL, &rc);
 	report(cc == 2 && rc == PTF_ERR_ALRDY_POLARIZED, "Double change to horizontal");
 
 	cc = ptf(PTF_CHECK, &rc);
-	report(cc == 0, "Should not report");
+	report(cc == 0, "Should not report change after horizontal -> horizontal");
 
 	report_prefix_pop();
 }
-- 
2.41.0

[kvm-unit-tests PATCH 5/9] s390x: topology: Refine stsi header test

[Nina Schoetterl-Glausch]

Add checks for length field.
Also minor refactor.

Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
---
 s390x/topology.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/s390x/topology.c b/s390x/topology.c
index 5374582f..0ba57986 100644
--- a/s390x/topology.c
+++ b/s390x/topology.c
@@ -187,18 +187,22 @@ static void stsi_check_maxcpus(struct sysinfo_15_1_x *info)
 }
 
 /*
- * stsi_check_mag
+ * stsi_check_header
  * @info: Pointer to the stsi information
+ * @sel2: stsi selector 2 value
  *
  * 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)
+static void stsi_check_header(struct sysinfo_15_1_x *info, int sel2)
 {
 	int i;
 
-	report_prefix_push("MAG");
+	report_prefix_push("Header");
 
+	report(IS_ALIGNED(info->length, 8), "Length %d multiple of 8", info->length);
+	report(info->length < PAGE_SIZE, "Length %d in bounds", info->length);
+	report(sel2 == info->mnest, "Valid mnest");
 	stsi_check_maxcpus(info);
 
 	/*
@@ -326,7 +330,6 @@ static int stsi_get_sysib(struct sysinfo_15_1_x *info, int sel2)
 
 	if (max_nested_lvl >= sel2) {
 		report(!ret, "Valid instruction");
-		report(sel2 == info->mnest, "Valid mnest");
 	} else {
 		report(ret, "Invalid instruction");
 	}
@@ -365,7 +368,7 @@ static void check_sysinfo_15_1_x(struct sysinfo_15_1_x *info, int sel2)
 		goto vertical;
 	}
 
-	stsi_check_mag(info);
+	stsi_check_header(info, sel2);
 	stsi_check_tle_coherency(info);
 
 vertical:
@@ -378,7 +381,7 @@ vertical:
 		goto end;
 	}
 
-	stsi_check_mag(info);
+	stsi_check_header(info, sel2);
 	stsi_check_tle_coherency(info);
 	report_prefix_pop();
 
-- 
2.41.0

[kvm-unit-tests PATCH 6/9] s390x: topology: Rename topology_core to topology_cpu

[Nina Schoetterl-Glausch]

This is more in line with the nomenclature in the PoP.

Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
---
 lib/s390x/stsi.h | 4 ++--
 s390x/topology.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/s390x/stsi.h b/lib/s390x/stsi.h
index 1351a6f3..8a97f44e 100644
--- a/lib/s390x/stsi.h
+++ b/lib/s390x/stsi.h
@@ -30,7 +30,7 @@ struct sysinfo_3_2_2 {
 };
 
 #define CPUS_TLE_RES_BITS 0x00fffffff8000000UL
-struct topology_core {
+struct topology_cpu {
 	uint8_t nl;
 	uint8_t reserved1[3];
 	uint8_t reserved4:5;
@@ -50,7 +50,7 @@ struct topology_container {
 
 union topology_entry {
 	uint8_t nl;
-	struct topology_core cpu;
+	struct topology_cpu cpu;
 	struct topology_container container;
 };
 
diff --git a/s390x/topology.c b/s390x/topology.c
index 0ba57986..c1f6520f 100644
--- a/s390x/topology.c
+++ b/s390x/topology.c
@@ -246,7 +246,7 @@ done:
 static uint8_t *check_tle(void *tc)
 {
 	struct topology_container *container = tc;
-	struct topology_core *cpus;
+	struct topology_cpu *cpus;
 	int n;
 
 	if (container->nl) {
-- 
2.41.0

[kvm-unit-tests PATCH 7/9] s390x: topology: Rewrite topology list test

[Nina Schoetterl-Glausch]

Rewrite recursion with separate functions for checking containers,
containers containing CPUs and CPUs.
This improves comprehension and allows for more tests.
We now also test for ordering of CPU TLEs and number of child entries.

Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
---
 lib/s390x/stsi.h |  36 +++++----
 s390x/topology.c | 197 ++++++++++++++++++++++++++++-------------------
 2 files changed, 140 insertions(+), 93 deletions(-)

diff --git a/lib/s390x/stsi.h b/lib/s390x/stsi.h
index 8a97f44e..fdb38e2c 100644
--- a/lib/s390x/stsi.h
+++ b/lib/s390x/stsi.h
@@ -30,28 +30,34 @@ struct sysinfo_3_2_2 {
 };
 
 #define CPUS_TLE_RES_BITS 0x00fffffff8000000UL
-struct topology_cpu {
-	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;
+union topology_cpu {
+	uint64_t raw[2];
+	struct {
+		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;
+	};
 };
 
 #define CONTAINER_TLE_RES_BITS 0x00ffffffffffff00UL
-struct topology_container {
-	uint8_t nl;
-	uint8_t reserved[6];
-	uint8_t id;
+union topology_container {
+	uint64_t raw;
+	struct {
+		uint8_t nl;
+		uint8_t reserved[6];
+		uint8_t id;
+	};
 };
 
 union topology_entry {
 	uint8_t nl;
-	struct topology_cpu cpu;
-	struct topology_container container;
+	union topology_cpu cpu;
+	union topology_container container;
 };
 
 #define CPU_TOPOLOGY_MAX_LEVEL 6
diff --git a/s390x/topology.c b/s390x/topology.c
index c1f6520f..9838434c 100644
--- a/s390x/topology.c
+++ b/s390x/topology.c
@@ -2,7 +2,7 @@
 /*
  * CPU Topology
  *
- * Copyright IBM Corp. 2022
+ * Copyright IBM Corp. 2022, 2023
  *
  * Authors:
  *  Pierre Morel <pmorel@linux.ibm.com>
@@ -23,7 +23,6 @@ static uint8_t pagebuf[PAGE_SIZE] __attribute__((aligned(PAGE_SIZE)));
 
 static int max_nested_lvl;
 static int number_of_cpus;
-static int cpus_in_masks;
 static int max_cpus;
 
 /*
@@ -236,80 +235,6 @@ done:
 	report_prefix_pop();
 }
 
-/**
- * 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)
-{
-	struct topology_container *container = tc;
-	struct topology_cpu *cpus;
-	int n;
-
-	if (container->nl) {
-		report_info("NL: %d id: %d", container->nl, container->id);
-
-		report(!(*(uint64_t *)tc & CONTAINER_TLE_RES_BITS),
-		       "reserved bits %016lx",
-		       *(uint64_t *)tc & CONTAINER_TLE_RES_BITS);
-
-		return check_tle(tc + sizeof(*container));
-	}
-
-	report_info("NL: %d", container->nl);
-	cpus = tc;
-
-	report(!(*(uint64_t *)tc & CPUS_TLE_RES_BITS), "reserved bits %016lx",
-	       *(uint64_t *)tc & CPUS_TLE_RES_BITS);
-
-	report(cpus->type == 0x03, "type IFL");
-
-	report_info("origin: %d", cpus->origin);
-	report_info("mask: %016lx", cpus->mask);
-	report_info("dedicated: %d entitlement: %d", cpus->d, cpus->pp);
-
-	n = __builtin_popcountl(cpus->mask);
-	report(n <= expected_topo_lvl[0], "CPUs per mask: %d out of max %d",
-	       n, expected_topo_lvl[0]);
-	cpus_in_masks += n;
-
-	if (!cpus->d)
-		report_skip("Not dedicated");
-	else
-		report(cpus->pp == 3 || cpus->pp == 0, "Dedicated CPUs are either horizontally polarized or have high entitlement");
-
-	return tc + sizeof(*cpus);
-}
-
-/**
- * stsi_check_tle_coherency:
- * @info: Pointer to the stsi information
- *
- * We verify that we get the expected number of Topology List Entry
- * containers for a specific level.
- */
-static void stsi_check_tle_coherency(struct sysinfo_15_1_x *info)
-{
-	void *tc, *end;
-
-	report_prefix_push("TLE");
-	cpus_in_masks = 0;
-
-	tc = info->tle;
-	end = (void *)info + info->length;
-
-	while (tc < end)
-		tc = check_tle(tc);
-
-	report(cpus_in_masks == number_of_cpus, "CPUs in mask %d",
-	       cpus_in_masks);
-
-	report_prefix_pop();
-}
-
 /**
  * stsi_get_sysib:
  * @info: pointer to the STSI info structure
@@ -339,6 +264,122 @@ static int stsi_get_sysib(struct sysinfo_15_1_x *info, int sel2)
 	return ret;
 }
 
+static int check_cpu(union topology_cpu *cpu,
+		     union topology_container *parent)
+{
+	report_prefix_pushf("%d:%d:%d:%d", cpu->d, cpu->pp, cpu->type, cpu->origin);
+
+	report(!(cpu->raw[0] & CPUS_TLE_RES_BITS), "reserved bits %016lx",
+	       cpu->raw[0] & CPUS_TLE_RES_BITS);
+
+	report(cpu->type == 0x03, "type IFL");
+
+	if (cpu->d)
+		report(cpu->pp == 3 || cpu->pp == 0,
+		       "Dedicated CPUs are either horizontally polarized or have high entitlement");
+	else
+		report_skip("Not dedicated");
+
+	report_prefix_pop();
+
+	return __builtin_popcountl(cpu->mask);
+}
+
+static union topology_container *check_child_cpus(struct sysinfo_15_1_x *info,
+						  union topology_container *cont,
+						  union topology_cpu *child,
+						  int *cpus_in_masks)
+{
+	void *last = ((void *)info) + info->length;
+	union topology_cpu *prev_cpu = NULL;
+	int cpus = 0;
+
+	for (; (void *)child < last && child->nl == 0; child++) {
+		cpus += check_cpu(child, cont);
+		if (prev_cpu) {
+			report(prev_cpu->type <= child->type, "Correct ordering wrt type");
+			if (prev_cpu->type < child->type)
+				continue;
+			report(prev_cpu->pp >= child->pp, "Correct ordering wrt polarization");
+			if (prev_cpu->type > child->type)
+				continue;
+			report(prev_cpu->d || !child->d, "Correct ordering wrt dedication");
+			if (prev_cpu->d && !child->d)
+				continue;
+			report(prev_cpu->origin <= child->origin, "Correct ordering wrt origin");
+		}
+		prev_cpu = child;
+	}
+	report(cpus <= expected_topo_lvl[0], "%d children <= max of %d",
+	       cpus, expected_topo_lvl[0]);
+	*cpus_in_masks += cpus;
+
+	return (union topology_container *)child;
+}
+
+static union topology_container *check_container(struct sysinfo_15_1_x *info,
+						 union topology_container *cont,
+						 union topology_entry *child,
+						 int *cpus_in_masks);
+
+static union topology_container *check_child_containers(struct sysinfo_15_1_x *info,
+							union topology_container *cont,
+							union topology_container *child,
+							int *cpus_in_masks)
+{
+	void *last = ((void *)info) + info->length;
+	union topology_container *entry;
+	int i;
+
+	for (i = 0, entry = child; (void *)entry < last && entry->nl == cont->nl - 1; i++) {
+		entry = check_container(info, entry, (union topology_entry *)(entry + 1),
+					cpus_in_masks);
+	}
+	if (max_nested_lvl == info->mnest)
+		report(i <= expected_topo_lvl[cont->nl - 1], "%d children <= max of %d",
+		       i, expected_topo_lvl[cont->nl - 1]);
+
+	return entry;
+}
+
+static union topology_container *check_container(struct sysinfo_15_1_x *info,
+						 union topology_container *cont,
+						 union topology_entry *child,
+						 int *cpus_in_masks)
+{
+	union topology_container *entry;
+
+	report_prefix_pushf("%d", cont->id);
+
+	report(cont->nl - 1 == child->nl, "Level %d one above child level %d",
+	       cont->nl, child->nl);
+	report(!(cont->raw & CONTAINER_TLE_RES_BITS), "reserved bits %016lx",
+	       cont->raw & CONTAINER_TLE_RES_BITS);
+
+	if (cont->nl > 1)
+		entry = check_child_containers(info, cont, &child->container, cpus_in_masks);
+	else
+		entry = check_child_cpus(info, cont, &child->cpu, cpus_in_masks);
+
+	report_prefix_pop();
+	return entry;
+}
+
+static void check_topology_list(struct sysinfo_15_1_x *info, int sel2)
+{
+	union topology_container dummy = { .nl = sel2, .id = 0 };
+	int cpus_in_masks = 0;
+
+	report_prefix_push("TLE");
+
+	check_container(info, &dummy, info->tle, &cpus_in_masks);
+	report(cpus_in_masks == number_of_cpus,
+	       "Number of CPUs %d equals  %d CPUs in masks",
+	       number_of_cpus, cpus_in_masks);
+
+	report_prefix_pop();
+}
+
 /**
  * check_sysinfo_15_1_x:
  * @info: pointer to the STSI info structure
@@ -369,7 +410,7 @@ static void check_sysinfo_15_1_x(struct sysinfo_15_1_x *info, int sel2)
 	}
 
 	stsi_check_header(info, sel2);
-	stsi_check_tle_coherency(info);
+	check_topology_list(info, sel2);
 
 vertical:
 	report_prefix_pop();
@@ -382,7 +423,7 @@ vertical:
 	}
 
 	stsi_check_header(info, sel2);
-	stsi_check_tle_coherency(info);
+	check_topology_list(info, sel2);
 	report_prefix_pop();
 
 end:
-- 
2.41.0

[kvm-unit-tests PATCH 8/9] scripts: Implement multiline strings for extra_params

[Nina Schoetterl-Glausch]

Implement a rudimentary form only.
extra_params can get long when passing a lot of arguments to qemu.
Multiline strings help with readability of the .cfg file.
Multiline strings begin and end with """, which must occur on separate
lines.

For example:
extra_params = """-cpu max,ctop=on -smp cpus=1,cores=16,maxcpus=128 \
-append '-drawers 2 -books 2 -sockets 2 -cores 16' \
-device max-s390x-cpu,core-id=31,drawer-id=0,book-id=0,socket-id=0"""

The command string built with extra_params is eval'ed by the runtime
script, so the newlines need to be escaped with \.

Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
---


This could certainly be done differently, suggestions welcome.


 scripts/common.bash  | 11 +++++++++++
 scripts/runtime.bash |  4 ++--
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/scripts/common.bash b/scripts/common.bash
index 7b983f7d..738e64af 100644
--- a/scripts/common.bash
+++ b/scripts/common.bash
@@ -36,6 +36,17 @@ function for_each_unittest()
 			kernel=$TEST_DIR/${BASH_REMATCH[1]}
 		elif [[ $line =~ ^smp\ *=\ *(.*)$ ]]; then
 			smp=${BASH_REMATCH[1]}
+		elif [[ $line =~ ^extra_params\ *=\ *'"""'(.*)$ ]]; then
+			opts=${BASH_REMATCH[1]}$'\n'
+			while read -r -u $fd; do
+				opts=${opts%\\$'\n'}
+				if [[ "$REPLY" =~ ^(.*)'"""'[:blank:]*$ ]]; then
+					opts+=${BASH_REMATCH[1]}
+					break
+				else
+					opts+=$REPLY$'\n'
+				fi
+			done
 		elif [[ $line =~ ^extra_params\ *=\ *(.*)$ ]]; then
 			opts=${BASH_REMATCH[1]}
 		elif [[ $line =~ ^groups\ *=\ *(.*)$ ]]; then
diff --git a/scripts/runtime.bash b/scripts/runtime.bash
index ada8ffd7..fc156f2f 100644
--- a/scripts/runtime.bash
+++ b/scripts/runtime.bash
@@ -15,7 +15,7 @@ extract_summary()
 # We assume that QEMU is going to work if it tried to load the kernel
 premature_failure()
 {
-    local log="$(eval $(get_cmdline _NO_FILE_4Uhere_) 2>&1)"
+    local log="$(eval "$(get_cmdline _NO_FILE_4Uhere_)" 2>&1)"
 
     echo "$log" | grep "_NO_FILE_4Uhere_" |
         grep -q -e "could not \(load\|open\) kernel" -e "error loading" &&
@@ -168,7 +168,7 @@ function run()
     # extra_params in the config file may contain backticks that need to be
     # expanded, so use eval to start qemu.  Use "> >(foo)" instead of a pipe to
     # preserve the exit status.
-    summary=$(eval $cmdline 2> >(RUNTIME_log_stderr $testname) \
+    summary=$(eval "$cmdline" 2> >(RUNTIME_log_stderr $testname) \
                              > >(tee >(RUNTIME_log_stdout $testname $kernel) | extract_summary))
     ret=$?
     [ "$KUT_STANDALONE" != "yes" ] && echo > >(RUNTIME_log_stdout $testname $kernel)
-- 
2.41.0

[kvm-unit-tests PATCH 9/9] s390x: topology: Add complex topology test

[Nina Schoetterl-Glausch]

Run the topology test case with a complex topology configuration.

Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
---


We could also embed a script that generates a random configuration
(in fact this configuration is generated), but that seemed a bit too
yucky.


 s390x/unittests.cfg | 133 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 133 insertions(+)

diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index 68e119e4..b08b0fb1 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -247,3 +247,136 @@ file = topology.elf
 [topology-2]
 file = topology.elf
 extra_params = -cpu max,ctop=on -smp sockets=31,cores=8,maxcpus=248  -append '-sockets 31 -cores 8'
+
+[topology-3]
+file = topology.elf
+extra_params = """-cpu max,ctop=on -smp cpus=1,drawers=2,books=2,sockets=2,cores=16,maxcpus=128 \
+-append '-drawers 2 -books 2 -sockets 2 -cores 16' \
+-device max-s390x-cpu,core-id=31,drawer-id=0,book-id=0,socket-id=0,entitlement=medium,dedicated=false \
+-device max-s390x-cpu,core-id=11,drawer-id=0,book-id=0,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=95,drawer-id=0,book-id=0,socket-id=0,entitlement=medium,dedicated=false \
+-device max-s390x-cpu,core-id=73,drawer-id=0,book-id=0,socket-id=0,entitlement=high,dedicated=false \
+-device max-s390x-cpu,core-id=78,drawer-id=0,book-id=0,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=13,drawer-id=0,book-id=0,socket-id=0,entitlement=medium,dedicated=false \
+-device max-s390x-cpu,core-id=40,drawer-id=0,book-id=0,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=101,drawer-id=0,book-id=0,socket-id=0,entitlement=medium,dedicated=false \
+-device max-s390x-cpu,core-id=29,drawer-id=0,book-id=0,socket-id=0,entitlement=medium,dedicated=false \
+-device max-s390x-cpu,core-id=56,drawer-id=0,book-id=0,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=92,drawer-id=0,book-id=0,socket-id=0,entitlement=medium,dedicated=false \
+-device max-s390x-cpu,core-id=30,drawer-id=0,book-id=0,socket-id=0,entitlement=low,dedicated=false \
+-device max-s390x-cpu,core-id=118,drawer-id=0,book-id=0,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=71,drawer-id=0,book-id=0,socket-id=0,entitlement=medium,dedicated=false \
+-device max-s390x-cpu,core-id=93,drawer-id=0,book-id=0,socket-id=0,entitlement=high,dedicated=false \
+-device max-s390x-cpu,core-id=16,drawer-id=0,book-id=0,socket-id=1,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=5,drawer-id=0,book-id=0,socket-id=1,entitlement=medium,dedicated=false \
+-device max-s390x-cpu,core-id=42,drawer-id=0,book-id=0,socket-id=1,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=98,drawer-id=0,book-id=0,socket-id=1,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=44,drawer-id=0,book-id=0,socket-id=1,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=23,drawer-id=0,book-id=0,socket-id=1,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=65,drawer-id=0,book-id=0,socket-id=1,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=102,drawer-id=0,book-id=0,socket-id=1,entitlement=low,dedicated=false \
+-device max-s390x-cpu,core-id=57,drawer-id=0,book-id=0,socket-id=1,entitlement=medium,dedicated=false \
+-device max-s390x-cpu,core-id=125,drawer-id=0,book-id=0,socket-id=1,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=127,drawer-id=0,book-id=0,socket-id=1,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=82,drawer-id=0,book-id=0,socket-id=1,entitlement=high,dedicated=false \
+-device max-s390x-cpu,core-id=14,drawer-id=0,book-id=0,socket-id=1,entitlement=low,dedicated=false \
+-device max-s390x-cpu,core-id=91,drawer-id=0,book-id=0,socket-id=1,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=12,drawer-id=0,book-id=0,socket-id=1,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=8,drawer-id=0,book-id=0,socket-id=1,entitlement=high,dedicated=false \
+-device max-s390x-cpu,core-id=112,drawer-id=0,book-id=1,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=109,drawer-id=0,book-id=1,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=19,drawer-id=0,book-id=1,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=96,drawer-id=0,book-id=1,socket-id=0,entitlement=low,dedicated=false \
+-device max-s390x-cpu,core-id=67,drawer-id=0,book-id=1,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=80,drawer-id=0,book-id=1,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=108,drawer-id=0,book-id=1,socket-id=0,entitlement=medium,dedicated=false \
+-device max-s390x-cpu,core-id=34,drawer-id=0,book-id=1,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=18,drawer-id=0,book-id=1,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=39,drawer-id=0,book-id=1,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=53,drawer-id=0,book-id=1,socket-id=0,entitlement=low,dedicated=false \
+-device max-s390x-cpu,core-id=46,drawer-id=0,book-id=1,socket-id=0,entitlement=low,dedicated=false \
+-device max-s390x-cpu,core-id=3,drawer-id=0,book-id=1,socket-id=0,entitlement=low,dedicated=false \
+-device max-s390x-cpu,core-id=76,drawer-id=0,book-id=1,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=15,drawer-id=0,book-id=1,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=7,drawer-id=0,book-id=1,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=81,drawer-id=0,book-id=1,socket-id=1,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=1,drawer-id=0,book-id=1,socket-id=1,entitlement=low,dedicated=false \
+-device max-s390x-cpu,core-id=113,drawer-id=0,book-id=1,socket-id=1,entitlement=low,dedicated=false \
+-device max-s390x-cpu,core-id=38,drawer-id=0,book-id=1,socket-id=1,entitlement=low,dedicated=false \
+-device max-s390x-cpu,core-id=90,drawer-id=0,book-id=1,socket-id=1,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=117,drawer-id=0,book-id=1,socket-id=1,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=62,drawer-id=0,book-id=1,socket-id=1,entitlement=medium,dedicated=false \
+-device max-s390x-cpu,core-id=85,drawer-id=0,book-id=1,socket-id=1,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=49,drawer-id=0,book-id=1,socket-id=1,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=24,drawer-id=0,book-id=1,socket-id=1,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=107,drawer-id=0,book-id=1,socket-id=1,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=103,drawer-id=0,book-id=1,socket-id=1,entitlement=medium,dedicated=false \
+-device max-s390x-cpu,core-id=33,drawer-id=0,book-id=1,socket-id=1,entitlement=low,dedicated=false \
+-device max-s390x-cpu,core-id=51,drawer-id=0,book-id=1,socket-id=1,entitlement=medium,dedicated=false \
+-device max-s390x-cpu,core-id=21,drawer-id=0,book-id=1,socket-id=1,entitlement=high,dedicated=false \
+-device max-s390x-cpu,core-id=72,drawer-id=0,book-id=1,socket-id=1,entitlement=low,dedicated=false \
+-device max-s390x-cpu,core-id=63,drawer-id=1,book-id=0,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=105,drawer-id=1,book-id=0,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=74,drawer-id=1,book-id=0,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=50,drawer-id=1,book-id=0,socket-id=0,entitlement=low,dedicated=false \
+-device max-s390x-cpu,core-id=60,drawer-id=1,book-id=0,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=22,drawer-id=1,book-id=0,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=43,drawer-id=1,book-id=0,socket-id=0,entitlement=low,dedicated=false \
+-device max-s390x-cpu,core-id=48,drawer-id=1,book-id=0,socket-id=0,entitlement=low,dedicated=false \
+-device max-s390x-cpu,core-id=35,drawer-id=1,book-id=0,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=58,drawer-id=1,book-id=0,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=106,drawer-id=1,book-id=0,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=123,drawer-id=1,book-id=0,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=122,drawer-id=1,book-id=0,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=9,drawer-id=1,book-id=0,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=10,drawer-id=1,book-id=0,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=25,drawer-id=1,book-id=0,socket-id=0,entitlement=low,dedicated=false \
+-device max-s390x-cpu,core-id=116,drawer-id=1,book-id=0,socket-id=1,entitlement=high,dedicated=false \
+-device max-s390x-cpu,core-id=26,drawer-id=1,book-id=0,socket-id=1,entitlement=high,dedicated=false \
+-device max-s390x-cpu,core-id=17,drawer-id=1,book-id=0,socket-id=1,entitlement=low,dedicated=false \
+-device max-s390x-cpu,core-id=20,drawer-id=1,book-id=0,socket-id=1,entitlement=high,dedicated=false \
+-device max-s390x-cpu,core-id=59,drawer-id=1,book-id=0,socket-id=1,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=54,drawer-id=1,book-id=0,socket-id=1,entitlement=high,dedicated=false \
+-device max-s390x-cpu,core-id=70,drawer-id=1,book-id=0,socket-id=1,entitlement=medium,dedicated=false \
+-device max-s390x-cpu,core-id=88,drawer-id=1,book-id=0,socket-id=1,entitlement=high,dedicated=false \
+-device max-s390x-cpu,core-id=6,drawer-id=1,book-id=0,socket-id=1,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=52,drawer-id=1,book-id=0,socket-id=1,entitlement=low,dedicated=false \
+-device max-s390x-cpu,core-id=55,drawer-id=1,book-id=0,socket-id=1,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=124,drawer-id=1,book-id=0,socket-id=1,entitlement=medium,dedicated=false \
+-device max-s390x-cpu,core-id=61,drawer-id=1,book-id=0,socket-id=1,entitlement=low,dedicated=false \
+-device max-s390x-cpu,core-id=84,drawer-id=1,book-id=0,socket-id=1,entitlement=high,dedicated=false \
+-device max-s390x-cpu,core-id=68,drawer-id=1,book-id=0,socket-id=1,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=86,drawer-id=1,book-id=0,socket-id=1,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=4,drawer-id=1,book-id=1,socket-id=0,entitlement=high,dedicated=false \
+-device max-s390x-cpu,core-id=75,drawer-id=1,book-id=1,socket-id=0,entitlement=low,dedicated=false \
+-device max-s390x-cpu,core-id=115,drawer-id=1,book-id=1,socket-id=0,entitlement=low,dedicated=false \
+-device max-s390x-cpu,core-id=28,drawer-id=1,book-id=1,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=120,drawer-id=1,book-id=1,socket-id=0,entitlement=low,dedicated=false \
+-device max-s390x-cpu,core-id=41,drawer-id=1,book-id=1,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=87,drawer-id=1,book-id=1,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=119,drawer-id=1,book-id=1,socket-id=0,entitlement=low,dedicated=false \
+-device max-s390x-cpu,core-id=114,drawer-id=1,book-id=1,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=104,drawer-id=1,book-id=1,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=27,drawer-id=1,book-id=1,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=121,drawer-id=1,book-id=1,socket-id=0,entitlement=high,dedicated=false \
+-device max-s390x-cpu,core-id=126,drawer-id=1,book-id=1,socket-id=0,entitlement=low,dedicated=false \
+-device max-s390x-cpu,core-id=37,drawer-id=1,book-id=1,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=32,drawer-id=1,book-id=1,socket-id=0,entitlement=high,dedicated=false \
+-device max-s390x-cpu,core-id=94,drawer-id=1,book-id=1,socket-id=0,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=110,drawer-id=1,book-id=1,socket-id=1,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=77,drawer-id=1,book-id=1,socket-id=1,entitlement=medium,dedicated=false \
+-device max-s390x-cpu,core-id=36,drawer-id=1,book-id=1,socket-id=1,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=66,drawer-id=1,book-id=1,socket-id=1,entitlement=medium,dedicated=false \
+-device max-s390x-cpu,core-id=83,drawer-id=1,book-id=1,socket-id=1,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=47,drawer-id=1,book-id=1,socket-id=1,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=99,drawer-id=1,book-id=1,socket-id=1,entitlement=low,dedicated=false \
+-device max-s390x-cpu,core-id=79,drawer-id=1,book-id=1,socket-id=1,entitlement=low,dedicated=false \
+-device max-s390x-cpu,core-id=100,drawer-id=1,book-id=1,socket-id=1,entitlement=medium,dedicated=false \
+-device max-s390x-cpu,core-id=89,drawer-id=1,book-id=1,socket-id=1,entitlement=high,dedicated=false \
+-device max-s390x-cpu,core-id=2,drawer-id=1,book-id=1,socket-id=1,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=45,drawer-id=1,book-id=1,socket-id=1,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=69,drawer-id=1,book-id=1,socket-id=1,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=64,drawer-id=1,book-id=1,socket-id=1,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=97,drawer-id=1,book-id=1,socket-id=1,entitlement=high,dedicated=true \
+-device max-s390x-cpu,core-id=111,drawer-id=1,book-id=1,socket-id=1,entitlement=medium,dedicated=false \
+"""
-- 
2.41.0

Re: [kvm-unit-tests PATCH 1/9] s390x: topology: Fix report message

[Janosch Frank]

On 10/11/23 10:56, Nina Schoetterl-Glausch wrote:
> A polarization value of 0 means horizontal polarization.
> 
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>

Don't we need to remove the entitlement part?
Entitlement is defined as the degree of vertical polarization.

> ---
>   s390x/topology.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/s390x/topology.c b/s390x/topology.c
> index 69558236..53838ed1 100644
> --- a/s390x/topology.c
> +++ b/s390x/topology.c
> @@ -275,7 +275,7 @@ static uint8_t *check_tle(void *tc)
>   	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");
> +		report(cpus->pp == 3 || cpus->pp == 0, "Dedicated CPUs are either horizontally polarized or have high entitlement");
>   
>   	return tc + sizeof(*cpus);
>   }

Re: [kvm-unit-tests PATCH 2/9] s390x: topology: Use parameter in stsi_get_sysib

[Janosch Frank]

On 10/11/23 10:56, Nina Schoetterl-Glausch wrote:
> Instead of accessing global pagebuf.

The fact that you mentioned global pagebuf made me look for a exported 
pagebuf variable in the lib.

Maybe this might be better:

s390x: topology: replace static address with function parameter in 
stsi_get_sysib

If we're given a parameter we should actually use it and not use 
hardcoded values. Let's replace the static pagebuf variable with the 
info function parameter.


Otherwise:
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

> 
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> ---
>   s390x/topology.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/s390x/topology.c b/s390x/topology.c
> index 53838ed1..e1bb6014 100644
> --- a/s390x/topology.c
> +++ b/s390x/topology.c
> @@ -322,7 +322,7 @@ static int stsi_get_sysib(struct sysinfo_15_1_x *info, int sel2)
>   
>   	report_prefix_pushf("SYSIB");
>   
> -	ret = stsi(pagebuf, 15, 1, sel2);
> +	ret = stsi(info, 15, 1, sel2);
>   
>   	if (max_nested_lvl >= sel2) {
>   		report(!ret, "Valid instruction");

Re: [kvm-unit-tests PATCH 3/9] s390x: topology: Fix parsing loop

[Janosch Frank]

On 10/11/23 10:56, Nina Schoetterl-Glausch wrote:
> Without a comparison the loop is infinite.
> 
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>

Wait, how did this work before this change?
Did the strcmp effectively end the loop?

> ---
>   s390x/topology.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/s390x/topology.c b/s390x/topology.c
> index e1bb6014..49d6dfeb 100644
> --- a/s390x/topology.c
> +++ b/s390x/topology.c
> @@ -466,7 +466,7 @@ static void parse_topology_args(int argc, char **argv)
>   		if (flag[0] != '-')
>   			report_abort("Argument is expected to begin with '-'");
>   		flag++;
> -		for (level = 0; ARRAY_SIZE(levels); level++) {
> +		for (level = 0; level < ARRAY_SIZE(levels); level++) {
>   			if (!strcmp(levels[level], flag))
>   				break;
>   		}

Re: [kvm-unit-tests PATCH 1/9] s390x: topology: Fix report message

[Nina Schoetterl-Glausch]

On Wed, 2023-10-11 at 12:56 +0200, Janosch Frank wrote:
> On 10/11/23 10:56, Nina Schoetterl-Glausch wrote:
> > A polarization value of 0 means horizontal polarization.
> > 
> > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> 
> Don't we need to remove the entitlement part?
> Entitlement is defined as the degree of vertical polarization.

I don't follow.
We're checking this from the PoP:
A dedicated CPU is either horizontally or vertically
polarized. When a dedicated CPU is vertically polar-
ized, entitlement is always high. Thus, when D is one,
PP is either 00 binary or 11 binary. 

> > ---
> >   s390x/topology.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/s390x/topology.c b/s390x/topology.c
> > index 69558236..53838ed1 100644
> > --- a/s390x/topology.c
> > +++ b/s390x/topology.c
> > @@ -275,7 +275,7 @@ static uint8_t *check_tle(void *tc)
> >   	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");
> > +		report(cpus->pp == 3 || cpus->pp == 0, "Dedicated CPUs are either horizontally polarized or have high entitlement");
> >   
> >   	return tc + sizeof(*cpus);
> >   }
> 

Re: [kvm-unit-tests PATCH 4/9] s390x: topology: Don't use non unique message

[Janosch Frank]

On 10/11/23 10:56, Nina Schoetterl-Glausch wrote:
> When we test something, i.e. do a report() we want unique messages,
> otherwise, from the test output, it will appear as if the same test was
> run multiple times, possible with different PASS/FAIL values.
> 
> Convert some reports that don't actually test anything topology specific
> into asserts.
> Refine the report message for others.
> 
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>

Please change to:
s390x: topology: make report messages unique

Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

> ---
>   s390x/topology.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/s390x/topology.c b/s390x/topology.c
> index 49d6dfeb..5374582f 100644
> --- a/s390x/topology.c
> +++ b/s390x/topology.c
> @@ -114,7 +114,7 @@ static void check_polarization_change(void)
>   	report_prefix_push("Polarization change");
>   
>   	/* We expect a clean state through reset */
> -	report(diag308_load_reset(1), "load normal reset done");
> +	assert(diag308_load_reset(1));
>   
>   	/*
>   	 * Set vertical polarization to verify that RESET sets
> @@ -123,7 +123,7 @@ static void check_polarization_change(void)
>   	cc = ptf(PTF_REQ_VERTICAL, &rc);
>   	report(cc == 0, "Set vertical polarization.");
>   
> -	report(diag308_load_reset(1), "load normal reset done");
> +	assert(diag308_load_reset(1));
>   
>   	cc = ptf(PTF_CHECK, &rc);
>   	report(cc == 0, "Reset should clear topology report");
> @@ -137,25 +137,25 @@ static void check_polarization_change(void)
>   	report(cc == 0, "Change to vertical");
>   
>   	cc = ptf(PTF_CHECK, &rc);
> -	report(cc == 1, "Should report");
> +	report(cc == 1, "Should report change after horizontal -> vertical");
>   
>   	cc = ptf(PTF_REQ_VERTICAL, &rc);
>   	report(cc == 2 && rc == PTF_ERR_ALRDY_POLARIZED, "Double change to vertical");
>   
>   	cc = ptf(PTF_CHECK, &rc);
> -	report(cc == 0, "Should not report");
> +	report(cc == 0, "Should not report change after vertical -> vertical");
>   
>   	cc = ptf(PTF_REQ_HORIZONTAL, &rc);
>   	report(cc == 0, "Change to horizontal");
>   
>   	cc = ptf(PTF_CHECK, &rc);
> -	report(cc == 1, "Should Report");
> +	report(cc == 1, "Should report change after vertical -> horizontal");
>   
>   	cc = ptf(PTF_REQ_HORIZONTAL, &rc);
>   	report(cc == 2 && rc == PTF_ERR_ALRDY_POLARIZED, "Double change to horizontal");
>   
>   	cc = ptf(PTF_CHECK, &rc);
> -	report(cc == 0, "Should not report");
> +	report(cc == 0, "Should not report change after horizontal -> horizontal");
>   
>   	report_prefix_pop();
>   }

Re: [kvm-unit-tests PATCH 3/9] s390x: topology: Fix parsing loop

[Nina Schoetterl-Glausch]

On Wed, 2023-10-11 at 13:07 +0200, Janosch Frank wrote:
> On 10/11/23 10:56, Nina Schoetterl-Glausch wrote:
> > Without a comparison the loop is infinite.
> > 
> > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> 
> Wait, how did this work before this change?
> Did the strcmp effectively end the loop?

Yes, it only worked because the test was passed correct arguments.
> 
> > ---
> >   s390x/topology.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/s390x/topology.c b/s390x/topology.c
> > index e1bb6014..49d6dfeb 100644
> > --- a/s390x/topology.c
> > +++ b/s390x/topology.c
> > @@ -466,7 +466,7 @@ static void parse_topology_args(int argc, char **argv)
> >   		if (flag[0] != '-')
> >   			report_abort("Argument is expected to begin with '-'");
> >   		flag++;
> > -		for (level = 0; ARRAY_SIZE(levels); level++) {
> > +		for (level = 0; level < ARRAY_SIZE(levels); level++) {
> >   			if (!strcmp(levels[level], flag))
> >   				break;
> >   		}
> 

Re: [kvm-unit-tests PATCH 5/9] s390x: topology: Refine stsi header test

[Janosch Frank]

On 10/11/23 10:56, Nina Schoetterl-Glausch wrote:
> Add checks for length field.
> Also minor refactor.
> 
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> ---
>   s390x/topology.c | 15 +++++++++------
>   1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/s390x/topology.c b/s390x/topology.c
> index 5374582f..0ba57986 100644
> --- a/s390x/topology.c
> +++ b/s390x/topology.c
> @@ -187,18 +187,22 @@ static void stsi_check_maxcpus(struct sysinfo_15_1_x *info)
>   }
>   
>   /*
> - * stsi_check_mag
> + * stsi_check_header
>    * @info: Pointer to the stsi information
> + * @sel2: stsi selector 2 value
>    *
>    * 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)
> +static void stsi_check_header(struct sysinfo_15_1_x *info, int sel2)
>   {
>   	int i;
>   
> -	report_prefix_push("MAG");
> +	report_prefix_push("Header");
>   
> +	report(IS_ALIGNED(info->length, 8), "Length %d multiple of 8", info->length);

STSI 15 works on Words, not DWords, no?
So we need to check length against 4, not 8.

> +	report(info->length < PAGE_SIZE, "Length %d in bounds", info->length);
> +	report(sel2 == info->mnest, "Valid mnest");
>   	stsi_check_maxcpus(info);
>   
>   	/*
> @@ -326,7 +330,6 @@ static int stsi_get_sysib(struct sysinfo_15_1_x *info, int sel2)
>   
>   	if (max_nested_lvl >= sel2) {
>   		report(!ret, "Valid instruction");
> -		report(sel2 == info->mnest, "Valid mnest");
>   	} else {
>   		report(ret, "Invalid instruction");
>   	}
> @@ -365,7 +368,7 @@ static void check_sysinfo_15_1_x(struct sysinfo_15_1_x *info, int sel2)
>   		goto vertical;
>   	}
>   
> -	stsi_check_mag(info);
> +	stsi_check_header(info, sel2);
>   	stsi_check_tle_coherency(info);
>   
>   vertical:
> @@ -378,7 +381,7 @@ vertical:
>   		goto end;
>   	}
>   
> -	stsi_check_mag(info);
> +	stsi_check_header(info, sel2);
>   	stsi_check_tle_coherency(info);
>   	report_prefix_pop();
>   

Re: [kvm-unit-tests PATCH 5/9] s390x: topology: Refine stsi header test

[Nina Schoetterl-Glausch]

On Wed, 2023-10-11 at 13:16 +0200, Janosch Frank wrote:
> On 10/11/23 10:56, Nina Schoetterl-Glausch wrote:
> > Add checks for length field.
> > Also minor refactor.
> > 
> > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> > ---
> >   s390x/topology.c | 15 +++++++++------
> >   1 file changed, 9 insertions(+), 6 deletions(-)
> > 
> > diff --git a/s390x/topology.c b/s390x/topology.c
> > index 5374582f..0ba57986 100644
> > --- a/s390x/topology.c
> > +++ b/s390x/topology.c
> > @@ -187,18 +187,22 @@ static void stsi_check_maxcpus(struct sysinfo_15_1_x *info)
> >   }
> >   
> >   /*
> > - * stsi_check_mag
> > + * stsi_check_header
> >    * @info: Pointer to the stsi information
> > + * @sel2: stsi selector 2 value
> >    *
> >    * 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)
> > +static void stsi_check_header(struct sysinfo_15_1_x *info, int sel2)
> >   {
> >   	int i;
> >   
> > -	report_prefix_push("MAG");
> > +	report_prefix_push("Header");
> >   
> > +	report(IS_ALIGNED(info->length, 8), "Length %d multiple of 8", info->length);
> 
> STSI 15 works on Words, not DWords, no?
> So we need to check length against 4, not 8.

The header is 16 bytes.
Topology list entries are 8 or 16, so it must be a multiple of 8 at least.

> 
> > +	report(info->length < PAGE_SIZE, "Length %d in bounds", info->length);
> > +	report(sel2 == info->mnest, "Valid mnest");
> >   	stsi_check_maxcpus(info);
> >   
> >   	/*
> > @@ -326,7 +330,6 @@ static int stsi_get_sysib(struct sysinfo_15_1_x *info, int sel2)
> >   
> >   	if (max_nested_lvl >= sel2) {
> >   		report(!ret, "Valid instruction");
> > -		report(sel2 == info->mnest, "Valid mnest");
> >   	} else {
> >   		report(ret, "Invalid instruction");
> >   	}
> > @@ -365,7 +368,7 @@ static void check_sysinfo_15_1_x(struct sysinfo_15_1_x *info, int sel2)
> >   		goto vertical;
> >   	}
> >   
> > -	stsi_check_mag(info);
> > +	stsi_check_header(info, sel2);
> >   	stsi_check_tle_coherency(info);
> >   
> >   vertical:
> > @@ -378,7 +381,7 @@ vertical:
> >   		goto end;
> >   	}
> >   
> > -	stsi_check_mag(info);
> > +	stsi_check_header(info, sel2);
> >   	stsi_check_tle_coherency(info);
> >   	report_prefix_pop();
> >   
> 

Re: [kvm-unit-tests PATCH 5/9] s390x: topology: Refine stsi header test

[Janosch Frank]

On 10/11/23 13:19, Nina Schoetterl-Glausch wrote:
> On Wed, 2023-10-11 at 13:16 +0200, Janosch Frank wrote:
>> On 10/11/23 10:56, Nina Schoetterl-Glausch wrote:
>>> Add checks for length field.
>>> Also minor refactor.
>>>
>>> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
>>> ---
>>>    s390x/topology.c | 15 +++++++++------
>>>    1 file changed, 9 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/s390x/topology.c b/s390x/topology.c
>>> index 5374582f..0ba57986 100644
>>> --- a/s390x/topology.c
>>> +++ b/s390x/topology.c
>>> @@ -187,18 +187,22 @@ static void stsi_check_maxcpus(struct sysinfo_15_1_x *info)
>>>    }
>>>    
>>>    /*
>>> - * stsi_check_mag
>>> + * stsi_check_header
>>>     * @info: Pointer to the stsi information
>>> + * @sel2: stsi selector 2 value
>>>     *
>>>     * 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)
>>> +static void stsi_check_header(struct sysinfo_15_1_x *info, int sel2)
>>>    {
>>>    	int i;
>>>    
>>> -	report_prefix_push("MAG");
>>> +	report_prefix_push("Header");
>>>    
>>> +	report(IS_ALIGNED(info->length, 8), "Length %d multiple of 8", info->length);
>>
>> STSI 15 works on Words, not DWords, no?
>> So we need to check length against 4, not 8.
> 
> The header is 16 bytes.
> Topology list entries are 8 or 16, so it must be a multiple of 8 at least.

Fair enough

> 
>>
>>> +	report(info->length < PAGE_SIZE, "Length %d in bounds", info->length);
>>> +	report(sel2 == info->mnest, "Valid mnest");
>>>    	stsi_check_maxcpus(info);
>>>    
>>>    	/*
>>> @@ -326,7 +330,6 @@ static int stsi_get_sysib(struct sysinfo_15_1_x *info, int sel2)
>>>    
>>>    	if (max_nested_lvl >= sel2) {
>>>    		report(!ret, "Valid instruction");
>>> -		report(sel2 == info->mnest, "Valid mnest");
>>>    	} else {
>>>    		report(ret, "Invalid instruction");
>>>    	}
>>> @@ -365,7 +368,7 @@ static void check_sysinfo_15_1_x(struct sysinfo_15_1_x *info, int sel2)
>>>    		goto vertical;
>>>    	}
>>>    
>>> -	stsi_check_mag(info);
>>> +	stsi_check_header(info, sel2);
>>>    	stsi_check_tle_coherency(info);
>>>    
>>>    vertical:
>>> @@ -378,7 +381,7 @@ vertical:
>>>    		goto end;
>>>    	}
>>>    
>>> -	stsi_check_mag(info);
>>> +	stsi_check_header(info, sel2);
>>>    	stsi_check_tle_coherency(info);
>>>    	report_prefix_pop();
>>>    
>>
> 

Re: [kvm-unit-tests PATCH 1/9] s390x: topology: Fix report message

[Janosch Frank]

On 10/11/23 13:10, Nina Schoetterl-Glausch wrote:
> On Wed, 2023-10-11 at 12:56 +0200, Janosch Frank wrote:
>> On 10/11/23 10:56, Nina Schoetterl-Glausch wrote:
>>> A polarization value of 0 means horizontal polarization.
>>>
>>> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
>>
>> Don't we need to remove the entitlement part?
>> Entitlement is defined as the degree of vertical polarization.
> 
> I don't follow.
> We're checking this from the PoP:
> A dedicated CPU is either horizontally or vertically
> polarized. When a dedicated CPU is vertically polar-
> ized, entitlement is always high. Thus, when D is one,
> PP is either 00 binary or 11 binary.

Ahhhh, I see what's the issue for my brain: Magic values

Could you please add a patch that introduces an enum for the pp values 
so the report below doesn't need a look into the POP to understand it?

> 
>>> ---
>>>    s390x/topology.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/s390x/topology.c b/s390x/topology.c
>>> index 69558236..53838ed1 100644
>>> --- a/s390x/topology.c
>>> +++ b/s390x/topology.c
>>> @@ -275,7 +275,7 @@ static uint8_t *check_tle(void *tc)
>>>    	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");
>>> +		report(cpus->pp == 3 || cpus->pp == 0, "Dedicated CPUs are either horizontally polarized or have high entitlement");
>>>    
>>>    	return tc + sizeof(*cpus);
>>>    }
>>
> 

Re: [kvm-unit-tests PATCH 6/9] s390x: topology: Rename topology_core to topology_cpu

[Janosch Frank]

On 10/11/23 10:56, Nina Schoetterl-Glausch wrote:
> This is more in line with the nomenclature in the PoP.
> 
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>

Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

Re: [kvm-unit-tests PATCH 1/9] s390x: topology: Fix report message

[Nina Schoetterl-Glausch]

On Wed, 2023-10-11 at 13:30 +0200, Janosch Frank wrote:
> On 10/11/23 13:10, Nina Schoetterl-Glausch wrote:
> > On Wed, 2023-10-11 at 12:56 +0200, Janosch Frank wrote:
> > > On 10/11/23 10:56, Nina Schoetterl-Glausch wrote:
> > > > A polarization value of 0 means horizontal polarization.
> > > > 
> > > > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> > > 
> > > Don't we need to remove the entitlement part?
> > > Entitlement is defined as the degree of vertical polarization.
> > 
> > I don't follow.
> > We're checking this from the PoP:
> > A dedicated CPU is either horizontally or vertically
> > polarized. When a dedicated CPU is vertically polar-
> > ized, entitlement is always high. Thus, when D is one,
> > PP is either 00 binary or 11 binary.
> 
> Ahhhh, I see what's the issue for my brain: Magic values
> 
> Could you please add a patch that introduces an enum for the pp values 
> so the report below doesn't need a look into the POP to understand it?

Sure can do, probably should also add an enum for type,
even if it will only have the one value (IFL) we check for.

> 
> > 
> > > > ---
> > > >    s390x/topology.c | 2 +-
> > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/s390x/topology.c b/s390x/topology.c
> > > > index 69558236..53838ed1 100644
> > > > --- a/s390x/topology.c
> > > > +++ b/s390x/topology.c
> > > > @@ -275,7 +275,7 @@ static uint8_t *check_tle(void *tc)
> > > >    	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");
> > > > +		report(cpus->pp == 3 || cpus->pp == 0, "Dedicated CPUs are either horizontally polarized or have high entitlement");
> > > >    
> > > >    	return tc + sizeof(*cpus);
> > > >    }
> > > 
> > 
> 

Re: [kvm-unit-tests PATCH 1/9] s390x: topology: Fix report message

[Nico Boehr]

Quoting Nina Schoetterl-Glausch (2023-10-11 10:56:24)
> A polarization value of 0 means horizontal polarization.
> 
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

Re: [kvm-unit-tests PATCH 2/9] s390x: topology: Use parameter in stsi_get_sysib

[Nico Boehr]

Quoting Nina Schoetterl-Glausch (2023-10-11 10:56:25)
> Instead of accessing global pagebuf.
> 
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

Re: [kvm-unit-tests PATCH 3/9] s390x: topology: Fix parsing loop

[Nico Boehr]

Quoting Nina Schoetterl-Glausch (2023-10-11 10:56:26)
> Without a comparison the loop is infinite.
> 
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

Re: [kvm-unit-tests PATCH 4/9] s390x: topology: Don't use non unique message

[Nico Boehr]

Quoting Nina Schoetterl-Glausch (2023-10-11 10:56:27)
> When we test something, i.e. do a report() we want unique messages,
> otherwise, from the test output, it will appear as if the same test was
> run multiple times, possible with different PASS/FAIL values.
> 
> Convert some reports that don't actually test anything topology specific
> into asserts.
> Refine the report message for others.
> 
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>

There is still the "TLE: reserved bits 0000000000000000" message which may
be duplicate, but I think you fix that in a later patch.

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

Re: [kvm-unit-tests PATCH 4/9] s390x: topology: Don't use non unique message

[Nina Schoetterl-Glausch]

On Fri, 2023-10-13 at 10:16 +0200, Nico Boehr wrote:
> Quoting Nina Schoetterl-Glausch (2023-10-11 10:56:27)
> > When we test something, i.e. do a report() we want unique messages,
> > otherwise, from the test output, it will appear as if the same test was
> > run multiple times, possible with different PASS/FAIL values.
> > 
> > Convert some reports that don't actually test anything topology specific
> > into asserts.
> > Refine the report message for others.
> > 
> > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> 
> There is still the "TLE: reserved bits 0000000000000000" message which may
> be duplicate, but I think you fix that in a later patch.
> 
> Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

Yes, this isn't comprehensive, the rewrite takes care of the rest.

Re: [kvm-unit-tests PATCH 5/9] s390x: topology: Refine stsi header test

[Nico Boehr]

Quoting Janosch Frank (2023-10-11 13:22:03)
> On 10/11/23 13:19, Nina Schoetterl-Glausch wrote:
> > On Wed, 2023-10-11 at 13:16 +0200, Janosch Frank wrote:
> >> On 10/11/23 10:56, Nina Schoetterl-Glausch wrote:
> >>> Add checks for length field.
> >>> Also minor refactor.
> >>>
> >>> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> >>> ---
> >>>    s390x/topology.c | 15 +++++++++------
> >>>    1 file changed, 9 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/s390x/topology.c b/s390x/topology.c
> >>> index 5374582f..0ba57986 100644
> >>> --- a/s390x/topology.c
> >>> +++ b/s390x/topology.c
> >>> @@ -187,18 +187,22 @@ static void stsi_check_maxcpus(struct sysinfo_15_1_x *info)
> >>>    }
> >>>    
> >>>    /*
> >>> - * stsi_check_mag
> >>> + * stsi_check_header
> >>>     * @info: Pointer to the stsi information
> >>> + * @sel2: stsi selector 2 value
> >>>     *
> >>>     * 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)
> >>> +static void stsi_check_header(struct sysinfo_15_1_x *info, int sel2)
> >>>    {
> >>>     int i;
> >>>    
> >>> -   report_prefix_push("MAG");
> >>> +   report_prefix_push("Header");
> >>>    
> >>> +   report(IS_ALIGNED(info->length, 8), "Length %d multiple of 8", info->length);
> >>
> >> STSI 15 works on Words, not DWords, no?
> >> So we need to check length against 4, not 8.
> > 
> > The header is 16 bytes.
> > Topology list entries are 8 or 16, so it must be a multiple of 8 at least.
> 
> Fair enough

Since I had the same question, can we have a comment here?

This isn't perfect but good enough IMO:

/* PoP mandates 4-byte alignment, but header is 16 bytes, TLEs are 8 or 16 bytes, so check for 8 byte align */

Otherwise:

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

Re: [kvm-unit-tests PATCH 6/9] s390x: topology: Rename topology_core to topology_cpu

[Nico Boehr]

Quoting Nina Schoetterl-Glausch (2023-10-11 10:56:29)
> This is more in line with the nomenclature in the PoP.
> 
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

Re: [kvm-unit-tests PATCH 7/9] s390x: topology: Rewrite topology list test

[Nico Boehr]

Quoting Nina Schoetterl-Glausch (2023-10-11 10:56:30)
> Rewrite recursion with separate functions for checking containers,
> containers containing CPUs and CPUs.
> This improves comprehension and allows for more tests.
> We now also test for ordering of CPU TLEs and number of child entries.
> 
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
[...]
> diff --git a/s390x/topology.c b/s390x/topology.c
> index c1f6520f..9838434c 100644
> --- a/s390x/topology.c
> +++ b/s390x/topology.c
[...]
> +static union topology_container *check_child_cpus(struct sysinfo_15_1_x *info,
> +                                                 union topology_container *cont,
> +                                                 union topology_cpu *child,
> +                                                 int *cpus_in_masks)
> +{
> +       void *last = ((void *)info) + info->length;
> +       union topology_cpu *prev_cpu = NULL;
> +       int cpus = 0;

I know __builtin_popcountl returns int, but maybe it makes sense to make
this and cpus_in_masks an unsigned type?

> +       for (; (void *)child < last && child->nl == 0; child++) {

Personal preference, I prefer simply iterating over a counter, but its up
to you.

Re: [kvm-unit-tests PATCH 8/9] scripts: Implement multiline strings for extra_params

[Nico Boehr]

Quoting Nina Schoetterl-Glausch (2023-10-11 10:56:31)
> Implement a rudimentary form only.
> extra_params can get long when passing a lot of arguments to qemu.
> Multiline strings help with readability of the .cfg file.
> Multiline strings begin and end with """, which must occur on separate
> lines.
> 
> For example:
> extra_params = """-cpu max,ctop=on -smp cpus=1,cores=16,maxcpus=128 \
> -append '-drawers 2 -books 2 -sockets 2 -cores 16' \
> -device max-s390x-cpu,core-id=31,drawer-id=0,book-id=0,socket-id=0"""
> 
> The command string built with extra_params is eval'ed by the runtime
> script, so the newlines need to be escaped with \.
> 
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> ---
> 
> 
> This could certainly be done differently, suggestions welcome.

I honestly do not have a better idea. If someone has, please bring it up!

[...]
> diff --git a/scripts/common.bash b/scripts/common.bash
> index 7b983f7d..738e64af 100644
> --- a/scripts/common.bash
> +++ b/scripts/common.bash
> @@ -36,6 +36,17 @@ function for_each_unittest()
>                         kernel=$TEST_DIR/${BASH_REMATCH[1]}
>                 elif [[ $line =~ ^smp\ *=\ *(.*)$ ]]; then
>                         smp=${BASH_REMATCH[1]}
> +               elif [[ $line =~ ^extra_params\ *=\ *'"""'(.*)$ ]]; then
> +                       opts=${BASH_REMATCH[1]}$'\n'
> +                       while read -r -u $fd; do

I was actually unaware that read saves into REPLY by default and went
looking for the REPLY variable to no avail. Can we make this more explicit
like so:

while read -r -u $fd LINE; do

and replace REPLY with LINE below?

> +                               opts=${opts%\\$'\n'}

So we strip the \ at the end of the line, but if it's not there we preserve
the line break? Is there a reason to do it this way? Why do we need the \
at all, as far as I can see """ terminates the multi line string, so what's
the purpose?

Did you test this in standalone mode?

Re: [kvm-unit-tests PATCH 9/9] s390x: topology: Add complex topology test

[Nico Boehr]

Quoting Nina Schoetterl-Glausch (2023-10-11 10:56:32)
> Run the topology test case with a complex topology configuration.

Can you add a TL;DR comment which summarizes the topology? And/or put the
script you used to generate the config in the commit message.

Re: [kvm-unit-tests PATCH 8/9] scripts: Implement multiline strings for extra_params

[Nina Schoetterl-Glausch]

On Thu, 2023-10-19 at 12:50 +0200, Nico Boehr wrote:
> Quoting Nina Schoetterl-Glausch (2023-10-11 10:56:31)
> > Implement a rudimentary form only.
> > extra_params can get long when passing a lot of arguments to qemu.
> > Multiline strings help with readability of the .cfg file.
> > Multiline strings begin and end with """, which must occur on separate
> > lines.
> > 
> > For example:
> > extra_params = """-cpu max,ctop=on -smp cpus=1,cores=16,maxcpus=128 \
> > -append '-drawers 2 -books 2 -sockets 2 -cores 16' \
> > -device max-s390x-cpu,core-id=31,drawer-id=0,book-id=0,socket-id=0"""
> > 
> > The command string built with extra_params is eval'ed by the runtime
> > script, so the newlines need to be escaped with \.
> > 
> > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> > ---
> > 
> > 
> > This could certainly be done differently, suggestions welcome.
> 
> I honestly do not have a better idea. If someone has, please bring it up!
> 
> [...]
> > diff --git a/scripts/common.bash b/scripts/common.bash
> > index 7b983f7d..738e64af 100644
> > --- a/scripts/common.bash
> > +++ b/scripts/common.bash
> > @@ -36,6 +36,17 @@ function for_each_unittest()
> >                         kernel=$TEST_DIR/${BASH_REMATCH[1]}
> >                 elif [[ $line =~ ^smp\ *=\ *(.*)$ ]]; then
> >                         smp=${BASH_REMATCH[1]}
> > +               elif [[ $line =~ ^extra_params\ *=\ *'"""'(.*)$ ]]; then
> > +                       opts=${BASH_REMATCH[1]}$'\n'
> > +                       while read -r -u $fd; do
> 
> I was actually unaware that read saves into REPLY by default and went
> looking for the REPLY variable to no avail. Can we make this more explicit
> like so:
> 
> while read -r -u $fd LINE; do
> 
> and replace REPLY with LINE below?

No, at least it's not so simple. man bash says:
If no names are supplied, the line read, *without the ending delimiter but otherwise unmodified*,
is assigned to the variable REPLY.
We don't want word splitting and white space being removed.
> 
> > +                               opts=${opts%\\$'\n'}
> 
> So we strip the \ at the end of the line, but if it's not there we preserve

We strip backslash newline, yes.
This way it's up to you if you want newlines in the string or not.
A bit of future proofing.
That is the only escaping I implemented, so right now you cannot have an
actual backslash at the end of the line.
The string is later eval'ed by bash which also does escaping,
but we cannot just rely on that, because by that time the last \n has
been dropped. If you had a backslash before you'll run into trouble.

> the line break? Is there a reason to do it this way? Why do we need the \
> at all, as far as I can see """ terminates the multi line string, so what's
> the purpose?
> 
> Did you test this in standalone mode?

I did not.