[kvm-unit-tests PATCH v6 0/2] S390x: CPU Topology Information

[kvm-unit-tests PATCH v6 0/2] S390x: CPU Topology Information

[Pierre Morel]

Hi,

new version of the kvm-unit-test s390x CPU topology series.

1. what is done
---------------

- First part is checking PTF errors, for KVM and LPAR

- Second part is checking PTF polarization change and STSI
  with the cpu topology including drawers and books.
  This tests are run for KVM only.

To run these tests under KVM successfully you need Linux 6.0
and the QEMU patches you find at:

https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg00081.html

To start the test in KVM just do:

# ./run_tests.sh topology

or

# ./s390x-run s390x/topology.elf \
	-smp 5,drawers=3,books=3,sockets=4,cores=4,maxcpus=144 \
	-append '-drawers 3 -books 3 -sockets 4 -cores 4'

Of course the declaration of the number of socket and core must be
coherent in -smp and -append arguments.


To start the tests in LPAR you do not need any argument.


Regards,
Pierre

Pierre Morel (2):
  s390x: topology: Check the Perform Topology Function
  s390x: topology: Checking Configuration Topology Information

 lib/s390x/sclp.h    |   3 +-
 lib/s390x/stsi.h    |  44 +++++
 s390x/Makefile      |   1 +
 s390x/topology.c    | 399 ++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |   4 +
 5 files changed, 450 insertions(+), 1 deletion(-)
 create mode 100644 s390x/topology.c

-- 
2.31.1

Changelog:

From v5:

- added drawers and books

- mnest parameter is not needed anymore, replaced
  by the use of sclp.

- Added several tests for polarity change and reset
  for PTF

From v4:

- Simplify the tests for socket and cores only.

[kvm-unit-tests PATCH v6 1/2] s390x: topology: Check the Perform Topology Function

[Pierre Morel]

We check that the PTF instruction is working correctly when
the cpu topology facility is available.

For KVM only, we test changing of the polarity between horizontal
and vertical and that a reset set the horizontal polarity.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 s390x/Makefile      |   1 +
 s390x/topology.c    | 155 ++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |   3 +
 3 files changed, 159 insertions(+)
 create mode 100644 s390x/topology.c

diff --git a/s390x/Makefile b/s390x/Makefile
index 52a9d82..b5fe8a3 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -39,6 +39,7 @@ tests += $(TEST_DIR)/panic-loop-extint.elf
 tests += $(TEST_DIR)/panic-loop-pgm.elf
 tests += $(TEST_DIR)/migration-sck.elf
 tests += $(TEST_DIR)/exittime.elf
+tests += $(TEST_DIR)/topology.elf
 
 pv-tests += $(TEST_DIR)/pv-diags.elf
 
diff --git a/s390x/topology.c b/s390x/topology.c
new file mode 100644
index 0000000..20f7ba2
--- /dev/null
+++ b/s390x/topology.c
@@ -0,0 +1,155 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * CPU Topology
+ *
+ * Copyright IBM Corp. 2022
+ *
+ * Authors:
+ *  Pierre Morel <pmorel@linux.ibm.com>
+ */
+
+#include <libcflat.h>
+#include <asm/page.h>
+#include <asm/asm-offsets.h>
+#include <asm/interrupt.h>
+#include <asm/facility.h>
+#include <smp.h>
+#include <sclp.h>
+#include <s390x/hardware.h>
+
+#define PTF_REQ_HORIZONTAL	0
+#define PTF_REQ_VERTICAL	1
+#define PTF_REQ_CHECK		2
+
+#define PTF_ERR_NO_REASON	0
+#define PTF_ERR_ALRDY_POLARIZED	1
+#define PTF_ERR_IN_PROGRESS	2
+
+extern int diag308_load_reset(u64);
+
+static int ptf(unsigned long fc, unsigned long *rc)
+{
+	int cc;
+
+	asm volatile(
+		"       .insn   rre,0xb9a20000,%1,0\n"
+		"       ipm     %0\n"
+		"       srl     %0,28\n"
+		: "=d" (cc), "+d" (fc)
+		:
+		: "cc");
+
+	*rc = fc >> 8;
+	return cc;
+}
+
+static void test_ptf(void)
+{
+	unsigned long rc;
+	int cc;
+
+	/* PTF is a privilege instruction */
+	report_prefix_push("Privilege");
+	enter_pstate();
+	expect_pgm_int();
+	ptf(PTF_REQ_CHECK, &rc);
+	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
+	report_prefix_pop();
+
+	report_prefix_push("Wrong fc");
+	expect_pgm_int();
+	ptf(0xff, &rc);
+	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+	report_prefix_pop();
+
+	report_prefix_push("Reserved bits");
+	expect_pgm_int();
+	ptf(0xffffffffffffff00UL, &rc);
+	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+	report_prefix_pop();
+
+	report_prefix_push("Topology Report pending");
+	/*
+	 * At this moment the topology may already have changed
+	 * since the VM has been started.
+	 * However, we can test if a second PTF instruction
+	 * reports that the topology did not change since the
+	 * preceding PFT instruction.
+	 */
+	ptf(PTF_REQ_CHECK, &rc);
+	cc = ptf(PTF_REQ_CHECK, &rc);
+	report(cc == 0, "PTF check should clear topology report");
+	report_prefix_pop();
+
+	report_prefix_push("Topology polarisation check");
+	/*
+	 * We can not assume the state of the polarization for
+	 * any Virtual Machine but KVM.
+	 * Let's skip the polarisation tests for other VMs.
+	 */
+	if (!host_is_kvm()) {
+		report_skip("Topology polarisation check is done for KVM only");
+		goto end;
+	}
+
+	/*
+	 * Set vertical polarization to verify that RESET sets
+	 * horizontal polarization back.
+	 */
+	cc = ptf(PTF_REQ_VERTICAL, &rc);
+	report(cc == 0, "Set vertical polarization.");
+
+	report(diag308_load_reset(1), "load normal reset done");
+
+	cc = ptf(PTF_REQ_CHECK, &rc);
+	report(cc == 0, "Reset should clear topology report");
+
+	cc = ptf(PTF_REQ_HORIZONTAL, &rc);
+	report(cc == 2 && rc == PTF_ERR_ALRDY_POLARIZED,
+	       "After RESET polarization is horizontal");
+
+	/* Flip between vertical and horizontal polarization */
+	cc = ptf(PTF_REQ_VERTICAL, &rc);
+	report(cc == 0, "Change to vertical polarization.");
+
+	cc = ptf(PTF_REQ_CHECK, &rc);
+	report(cc == 1, "Polarization change should set topology report");
+
+	cc = ptf(PTF_REQ_HORIZONTAL, &rc);
+	report(cc == 0, "Change to horizontal polarization.");
+
+end:
+	report_prefix_pop();
+}
+
+static struct {
+	const char *name;
+	void (*func)(void);
+} tests[] = {
+	{ "PTF", test_ptf},
+	{ NULL, NULL }
+};
+
+int main(int argc, char *argv[])
+{
+	int i;
+
+	report_prefix_push("CPU Topology");
+
+	if (!test_facility(11)) {
+		report_skip("Topology facility not present");
+		goto end;
+	}
+
+	report_info("Virtual machine level %ld", stsi_get_fc());
+
+	for (i = 0; tests[i].name; i++) {
+		report_prefix_push(tests[i].name);
+		tests[i].func();
+		report_prefix_pop();
+	}
+
+end:
+	report_prefix_pop();
+	return report_summary();
+}
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index 3caf81e..3530cc4 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -208,3 +208,6 @@ groups = migration
 [exittime]
 file = exittime.elf
 smp = 2
+
+[topology]
+file = topology.elf
-- 
2.31.1

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

[Pierre Morel]

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.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 lib/s390x/sclp.h    |   3 +-
 lib/s390x/stsi.h    |  44 ++++++++
 s390x/topology.c    | 244 ++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |   1 +
 4 files changed, 291 insertions(+), 1 deletion(-)

diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
index 853529b..6ecfb0a 100644
--- a/lib/s390x/sclp.h
+++ b/lib/s390x/sclp.h
@@ -150,7 +150,8 @@ typedef struct ReadInfo {
 	SCCBHeader h;
 	uint16_t rnmax;
 	uint8_t rnsize;
-	uint8_t  _reserved1[16 - 11];       /* 11-15 */
+	uint8_t  _reserved1[15 - 11];       /* 11-14 */
+	uint8_t stsi_parm;                  /* 15-15 */
 	uint16_t entries_cpu;               /* 16-17 */
 	uint16_t offset_cpu;                /* 18-19 */
 	uint8_t  _reserved2[24 - 20];       /* 20-23 */
diff --git a/lib/s390x/stsi.h b/lib/s390x/stsi.h
index bebc492..8dbbfc2 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 reserved0a;
+	uint8_t mnest;
+	uint8_t reserved0c[4];
+	union topology_entry tle[0];
+};
+
+static inline int cpus_in_tle_mask(uint64_t val)
+{
+	int i, n;
+
+	for (i = 0, n = 0; i < 64; i++, val >>= 1)
+		if (val & 0x01)
+			n++;
+	return n;
+}
+
 #endif  /* _S390X_STSI_H_ */
diff --git a/s390x/topology.c b/s390x/topology.c
index 20f7ba2..f21c653 100644
--- a/s390x/topology.c
+++ b/s390x/topology.c
@@ -16,6 +16,18 @@
 #include <smp.h>
 #include <sclp.h>
 #include <s390x/hardware.h>
+#include <s390x/stsi.h>
+
+static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
+
+static int max_nested_lvl;
+static int number_of_cpus;
+static int max_cpus = 1;
+
+/* Topology level as defined by architecture */
+static int arch_topo_lvl[CPU_TOPOLOGY_MAX_LEVEL];
+/* Topology nested level as reported in STSI */
+static int stsi_nested_lvl[CPU_TOPOLOGY_MAX_LEVEL];
 
 #define PTF_REQ_HORIZONTAL	0
 #define PTF_REQ_VERTICAL	1
@@ -122,11 +134,241 @@ end:
 	report_prefix_pop();
 }
 
+/*
+ * stsi_check_maxcpus
+ * @info: Pointer to the stsi information
+ *
+ * The product of the numbers of containers per level
+ * is the maximum number of CPU allowed by the machine.
+ */
+static void stsi_check_maxcpus(struct sysinfo_15_1_x *info)
+{
+	int n, i;
+
+	report_prefix_push("maximum cpus");
+
+	for (i = 0, n = 1; i < CPU_TOPOLOGY_MAX_LEVEL; i++) {
+		report_info("Mag%d: %d", CPU_TOPOLOGY_MAX_LEVEL - i, info->mag[i]);
+		n *= info->mag[i] ? info->mag[i] : 1;
+	}
+	report(n == max_cpus, "Maximum CPUs %d expected %d", n, max_cpus);
+
+	report_prefix_pop();
+}
+
+/*
+ * stsi_check_tle_coherency
+ * @info: Pointer to the stsi information
+ * @sel2: Topology level to check.
+ *
+ * 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, int sel2)
+{
+	struct topology_container *tc, *end;
+	struct topology_core *cpus;
+	int n = 0;
+	int i;
+
+	report_prefix_push("TLE coherency");
+
+	tc = &info->tle[0].container;
+	end = (struct topology_container *)((unsigned long)info + info->length);
+
+	for (i = 0; i < CPU_TOPOLOGY_MAX_LEVEL; i++)
+		stsi_nested_lvl[i] = 0;
+
+	while (tc < end) {
+		if (tc->nl > 5) {
+			report_abort("Unexpected TL Entry: tle->nl: %d", tc->nl);
+			return;
+		}
+		if (tc->nl == 0) {
+			cpus = (struct topology_core *)tc;
+			n += cpus_in_tle_mask(cpus->mask);
+			report_info("cpu type %02x  d: %d pp: %d", cpus->type, cpus->d, cpus->pp);
+			report_info("origin : %04x mask %016lx", cpus->origin, cpus->mask);
+		}
+
+		stsi_nested_lvl[tc->nl]++;
+		report_info("level %d: lvl: %d id: %d cnt: %d",
+			    tc->nl, tc->nl, tc->id, stsi_nested_lvl[tc->nl]);
+
+		/* trick: CPU TLEs are twice the size of containers TLE */
+		if (tc->nl == 0)
+			tc++;
+		tc++;
+	}
+	report(n == number_of_cpus, "Number of CPUs  : %d expect %d", n, number_of_cpus);
+	/*
+	 * For KVM we accept
+	 * - only 1 type of CPU
+	 * - only horizontal topology
+	 * - only dedicated CPUs
+	 * This leads to expect the number of entries of level 0 CPU
+	 * Topology Level Entry (TLE) to be:
+	 * 1 + (number_of_cpus - 1)  / arch_topo_lvl[0]
+	 *
+	 * For z/VM or LPAR this number can only be greater if different
+	 * polarity, CPU types because there may be a nested level 0 CPU TLE
+	 * for each of the CPU/polarity/sharing types in a level 1 container TLE.
+	 */
+	n =  (number_of_cpus - 1)  / arch_topo_lvl[0];
+	report(stsi_nested_lvl[0] >=  n + 1,
+	       "CPU Type TLE    : %d expect %d", stsi_nested_lvl[0], n + 1);
+
+	/* For each level found in STSI */
+	for (i = 1; i < CPU_TOPOLOGY_MAX_LEVEL; i++) {
+		/*
+		 * For non QEMU/KVM hypervisor the concatenation of the levels
+		 * above level 1 are architecture dependent.
+		 * Skip these checks.
+		 */
+		if (!host_is_kvm() && sel2 != 2)
+			continue;
+
+		/* For QEMU/KVM we expect a simple calculation */
+		if (sel2 > i) {
+			report(stsi_nested_lvl[i] ==  n + 1,
+			       "Container TLE  %d: %d expect %d", i, stsi_nested_lvl[i], n + 1);
+			n /= arch_topo_lvl[i];
+		}
+	}
+
+	report_prefix_pop();
+}
+
+/*
+ * 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;
+
+	report_prefix_pushf("mnested %d 15_1_%d", max_nested_lvl, sel2);
+
+	ret = stsi(pagebuf, 15, 1, sel2);
+	if (max_nested_lvl >= sel2) {
+		report(!ret, "Valid stsi instruction");
+	} else {
+		report(ret, "Invalid stsi instruction");
+		goto end;
+	}
+
+	stsi_check_maxcpus(info);
+	stsi_check_tle_coherency(info, sel2);
+
+end:
+	report_prefix_pop();
+}
+
+static int sclp_get_mnest(void)
+{
+	ReadInfo *sccb = (void *)_sccb;
+
+	sclp_mark_busy();
+	memset(_sccb, 0, PAGE_SIZE);
+	sccb->h.length = PAGE_SIZE;
+
+	sclp_service_call(SCLP_CMDW_READ_SCP_INFO, sccb);
+	assert(sccb->h.response_code == SCLP_RC_NORMAL_READ_COMPLETION);
+
+	return sccb->stsi_parm;
+}
+
+/*
+ * test_stsi
+ *
+ * Retrieves the maximum nested topology level supported by the architecture
+ * and the number of CPUs.
+ * Calls the checking for the STSI instruction in sel2 reverse level order
+ * from 6 (CPU_TOPOLOGY_MAX_LEVEL) to 2 to have the most interesting level,
+ * the one triggering a topology-change-report-pending condition, level 2,
+ * at the end of the report.
+ *
+ */
+static void test_stsi(void)
+{
+	int sel2;
+
+	max_nested_lvl = sclp_get_mnest();
+	report_info("SCLP maximum nested level : %d", max_nested_lvl);
+
+	number_of_cpus = sclp_get_cpu_num();
+	report_info("SCLP number of CPU: %d", number_of_cpus);
+
+	/* STSI selector 2 can takes values between 2 and 6 */
+	for (sel2 = 6; sel2 >= 2; sel2--)
+		check_sysinfo_15_1_x((struct sysinfo_15_1_x *)pagebuf, sel2);
+}
+
+/*
+ * parse_topology_args
+ * @argc: number of arguments
+ * @argv: argument array
+ *
+ * This function initialize the architecture topology levels
+ * which should be the same as the one provided by the hypervisor.
+ *
+ * We use the current names found in IBM/Z literature, Linux and QEMU:
+ * cores, sockets/packages, books, drawers and nodes to facilitate the
+ * human machine interface but store the result in a machine abstract
+ * array of architecture topology levels.
+ * Note that when QEMU uses socket as a name for the topology level 1
+ * Linux uses package or physical_package.
+ */
+static void parse_topology_args(int argc, char **argv)
+{
+	int i;
+
+	report_info("%d arguments", argc);
+	for (i = 1; i < argc; i++) {
+		if (!strcmp("-cores", argv[i])) {
+			i++;
+			if (i >= argc)
+				report_abort("-cores needs a parameter");
+			arch_topo_lvl[0] = atol(argv[i]);
+			report_info("cores: %d", arch_topo_lvl[0]);
+		} else if (!strcmp("-sockets", argv[i])) {
+			i++;
+			if (i >= argc)
+				report_abort("-sockets needs a parameter");
+			arch_topo_lvl[1] = atol(argv[i]);
+			report_info("sockets: %d", arch_topo_lvl[1]);
+		} else if (!strcmp("-books", argv[i])) {
+			i++;
+			if (i >= argc)
+				report_abort("-books needs a parameter");
+			arch_topo_lvl[2] = atol(argv[i]);
+			report_info("books: %d", arch_topo_lvl[2]);
+		} else if (!strcmp("-drawers", argv[i])) {
+			i++;
+			if (i >= argc)
+				report_abort("-drawers needs a parameter");
+			arch_topo_lvl[3] = atol(argv[i]);
+			report_info("drawers: %d", arch_topo_lvl[3]);
+		}
+	}
+
+	for (i = 0; i < CPU_TOPOLOGY_MAX_LEVEL; i++) {
+		if (!arch_topo_lvl[i])
+			arch_topo_lvl[i] = 1;
+		max_cpus *= arch_topo_lvl[i];
+	}
+}
+
 static struct {
 	const char *name;
 	void (*func)(void);
 } tests[] = {
 	{ "PTF", test_ptf},
+	{ "STSI", test_stsi},
 	{ NULL, NULL }
 };
 
@@ -136,6 +378,8 @@ int main(int argc, char *argv[])
 
 	report_prefix_push("CPU Topology");
 
+	parse_topology_args(argc, argv);
+
 	if (!test_facility(11)) {
 		report_skip("Topology facility not present");
 		goto end;
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index 3530cc4..b697aca 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -211,3 +211,4 @@ smp = 2
 
 [topology]
 file = topology.elf
+extra_params=-smp 5,drawers=3,books=3,sockets=4,cores=4,maxcpus=144 -append '-drawers 3 -books 3 -sockets 4 -cores 4'
-- 
2.31.1

Re: [kvm-unit-tests PATCH v6 1/2] s390x: topology: Check the Perform Topology Function

[Thomas Huth]

On 02/02/2023 10.28, Pierre Morel wrote:
> We check that the PTF instruction is working correctly when
> the cpu topology facility is available.
> 
> For KVM only, we test changing of the polarity between horizontal
> and vertical and that a reset set the horizontal polarity.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   s390x/Makefile      |   1 +
>   s390x/topology.c    | 155 ++++++++++++++++++++++++++++++++++++++++++++
>   s390x/unittests.cfg |   3 +
>   3 files changed, 159 insertions(+)
>   create mode 100644 s390x/topology.c
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 52a9d82..b5fe8a3 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -39,6 +39,7 @@ tests += $(TEST_DIR)/panic-loop-extint.elf
>   tests += $(TEST_DIR)/panic-loop-pgm.elf
>   tests += $(TEST_DIR)/migration-sck.elf
>   tests += $(TEST_DIR)/exittime.elf
> +tests += $(TEST_DIR)/topology.elf
>   
>   pv-tests += $(TEST_DIR)/pv-diags.elf
>   
> diff --git a/s390x/topology.c b/s390x/topology.c
> new file mode 100644
> index 0000000..20f7ba2
> --- /dev/null
> +++ b/s390x/topology.c
> @@ -0,0 +1,155 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * CPU Topology
> + *
> + * Copyright IBM Corp. 2022
> + *
> + * Authors:
> + *  Pierre Morel <pmorel@linux.ibm.com>
> + */
> +
> +#include <libcflat.h>
> +#include <asm/page.h>
> +#include <asm/asm-offsets.h>
> +#include <asm/interrupt.h>
> +#include <asm/facility.h>
> +#include <smp.h>
> +#include <sclp.h>
> +#include <s390x/hardware.h>
> +
> +#define PTF_REQ_HORIZONTAL	0
> +#define PTF_REQ_VERTICAL	1
> +#define PTF_REQ_CHECK		2
> +
> +#define PTF_ERR_NO_REASON	0
> +#define PTF_ERR_ALRDY_POLARIZED	1
> +#define PTF_ERR_IN_PROGRESS	2
> +
> +extern int diag308_load_reset(u64);
> +
> +static int ptf(unsigned long fc, unsigned long *rc)
> +{
> +	int cc;
> +
> +	asm volatile(
> +		"       .insn   rre,0xb9a20000,%1,0\n"

Why are you specifying the instruction manually? I think both, GCC and Clang 
should know the "ptf" mnemonic, shouldn't they?

> +		"       ipm     %0\n"
> +		"       srl     %0,28\n"
> +		: "=d" (cc), "+d" (fc)
> +		:
> +		: "cc");
> +
> +	*rc = fc >> 8;
> +	return cc;
> +}
> +
> +static void test_ptf(void)
> +{
> +	unsigned long rc;
> +	int cc;
> +
> +	/* PTF is a privilege instruction */

s/privilege/privileged/ ?

> +	report_prefix_push("Privilege");
> +	enter_pstate();
> +	expect_pgm_int();
> +	ptf(PTF_REQ_CHECK, &rc);
> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("Wrong fc");
> +	expect_pgm_int();
> +	ptf(0xff, &rc);
> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("Reserved bits");
> +	expect_pgm_int();
> +	ptf(0xffffffffffffff00UL, &rc);
> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();

This function is quite big ... I'd maybe group the above checks for error 
conditions into a separate function instead.

> +	report_prefix_push("Topology Report pending");
> +	/*
> +	 * At this moment the topology may already have changed
> +	 * since the VM has been started.
> +	 * However, we can test if a second PTF instruction
> +	 * reports that the topology did not change since the
> +	 * preceding PFT instruction.
> +	 */
> +	ptf(PTF_REQ_CHECK, &rc);
> +	cc = ptf(PTF_REQ_CHECK, &rc);
> +	report(cc == 0, "PTF check should clear topology report");
> +	report_prefix_pop();
> +
> +	report_prefix_push("Topology polarisation check");
> +	/*
> +	 * We can not assume the state of the polarization for

s/can not/cannot/ ?

Also, you sometimes write polarization with "z" and sometimes with "s". I'd 
suggest to standardize on "z" (as in "IBM Z" ;-))

> +	 * any Virtual Machine but KVM.
> +	 * Let's skip the polarisation tests for other VMs.
> +	 */
> +	if (!host_is_kvm()) {
> +		report_skip("Topology polarisation check is done for KVM only");
> +		goto end;
> +	}
> +
> +	/*
> +	 * Set vertical polarization to verify that RESET sets
> +	 * horizontal polarization back.
> +	 */
> +	cc = ptf(PTF_REQ_VERTICAL, &rc);
> +	report(cc == 0, "Set vertical polarization.");
> +
> +	report(diag308_load_reset(1), "load normal reset done");
> +
> +	cc = ptf(PTF_REQ_CHECK, &rc);
> +	report(cc == 0, "Reset should clear topology report");
> +
> +	cc = ptf(PTF_REQ_HORIZONTAL, &rc);
> +	report(cc == 2 && rc == PTF_ERR_ALRDY_POLARIZED,
> +	       "After RESET polarization is horizontal");
> +
> +	/* Flip between vertical and horizontal polarization */
> +	cc = ptf(PTF_REQ_VERTICAL, &rc);
> +	report(cc == 0, "Change to vertical polarization.");
> +
> +	cc = ptf(PTF_REQ_CHECK, &rc);
> +	report(cc == 1, "Polarization change should set topology report");
> +
> +	cc = ptf(PTF_REQ_HORIZONTAL, &rc);
> +	report(cc == 0, "Change to horizontal polarization.");
> +
> +end:
> +	report_prefix_pop();
> +}

Apart from the nits, the patch looks fine to me.

  Thomas

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

[Thomas Huth]

On 02/02/2023 10.28, Pierre Morel wrote:
> 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.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
...
> +static inline int cpus_in_tle_mask(uint64_t val)
> +{
> +	int i, n;
> +
> +	for (i = 0, n = 0; i < 64; i++, val >>= 1)
> +		if (val & 0x01)
> +			n++;
> +	return n;

I'd suggest to use __builtin_popcountl here instead of looping.

> +}
> +
>   #endif  /* _S390X_STSI_H_ */
> diff --git a/s390x/topology.c b/s390x/topology.c
> index 20f7ba2..f21c653 100644
> --- a/s390x/topology.c
> +++ b/s390x/topology.c
> @@ -16,6 +16,18 @@
>   #include <smp.h>
>   #include <sclp.h>
>   #include <s390x/hardware.h>
> +#include <s390x/stsi.h>
> +
> +static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));

Isn't the SYSIB just one page only? Why reserve two pages here?

> +static int max_nested_lvl;
> +static int number_of_cpus;
> +static int max_cpus = 1;
> +
> +/* Topology level as defined by architecture */
> +static int arch_topo_lvl[CPU_TOPOLOGY_MAX_LEVEL];
> +/* Topology nested level as reported in STSI */
> +static int stsi_nested_lvl[CPU_TOPOLOGY_MAX_LEVEL];
>   
>   #define PTF_REQ_HORIZONTAL	0
>   #define PTF_REQ_VERTICAL	1
> @@ -122,11 +134,241 @@ end:
>   	report_prefix_pop();
>   }
>   
> +/*
> + * stsi_check_maxcpus
> + * @info: Pointer to the stsi information
> + *
> + * The product of the numbers of containers per level
> + * is the maximum number of CPU allowed by the machine.
> + */
> +static void stsi_check_maxcpus(struct sysinfo_15_1_x *info)
> +{
> +	int n, i;
> +
> +	report_prefix_push("maximum cpus");
> +
> +	for (i = 0, n = 1; i < CPU_TOPOLOGY_MAX_LEVEL; i++) {
> +		report_info("Mag%d: %d", CPU_TOPOLOGY_MAX_LEVEL - i, info->mag[i]);
> +		n *= info->mag[i] ? info->mag[i] : 1;

You could use the Elvis operator here instead.

> +	}
> +	report(n == max_cpus, "Maximum CPUs %d expected %d", n, max_cpus);
> +
> +	report_prefix_pop();
> +}
> +
> +/*
> + * stsi_check_tle_coherency
> + * @info: Pointer to the stsi information
> + * @sel2: Topology level to check.
> + *
> + * 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, int sel2)
> +{
> +	struct topology_container *tc, *end;
> +	struct topology_core *cpus;
> +	int n = 0;
> +	int i;
> +
> +	report_prefix_push("TLE coherency");
> +
> +	tc = &info->tle[0].container;
> +	end = (struct topology_container *)((unsigned long)info + info->length);

s/unsigned long/uintptr_t/ please!


> +
> +	for (i = 0; i < CPU_TOPOLOGY_MAX_LEVEL; i++)
> +		stsi_nested_lvl[i] = 0;

memset(stsi_nested_lvl, 0, sizeof(stsi_nested_lvl)) ?

> +	while (tc < end) {
> +		if (tc->nl > 5) {

Use ">= CPU_TOPOLOGY_MAX_LEVEL" instead of "> 5" ?

> +			report_abort("Unexpected TL Entry: tle->nl: %d", tc->nl);
> +			return;
> +		}
> +		if (tc->nl == 0) {
> +			cpus = (struct topology_core *)tc;
> +			n += cpus_in_tle_mask(cpus->mask);
> +			report_info("cpu type %02x  d: %d pp: %d", cpus->type, cpus->d, cpus->pp);
> +			report_info("origin : %04x mask %016lx", cpus->origin, cpus->mask);
> +		}
> +
> +		stsi_nested_lvl[tc->nl]++;
> +		report_info("level %d: lvl: %d id: %d cnt: %d",
> +			    tc->nl, tc->nl, tc->id, stsi_nested_lvl[tc->nl]);
> +
> +		/* trick: CPU TLEs are twice the size of containers TLE */
> +		if (tc->nl == 0)
> +			tc++;

IMHO it might be cleaner to have a "uint8_t *" or "void *" to the current 
position in the sysinfo block, and do the pointer arithmetic on that pointer 
instead... well, it's likely just a matter of taste.

> +		tc++;
> +	}
> +	report(n == number_of_cpus, "Number of CPUs  : %d expect %d", n, number_of_cpus);
> +	/*
> +	 * For KVM we accept
> +	 * - only 1 type of CPU
> +	 * - only horizontal topology
> +	 * - only dedicated CPUs
> +	 * This leads to expect the number of entries of level 0 CPU
> +	 * Topology Level Entry (TLE) to be:
> +	 * 1 + (number_of_cpus - 1)  / arch_topo_lvl[0]
> +	 *
> +	 * For z/VM or LPAR this number can only be greater if different
> +	 * polarity, CPU types because there may be a nested level 0 CPU TLE
> +	 * for each of the CPU/polarity/sharing types in a level 1 container TLE.
> +	 */
> +	n =  (number_of_cpus - 1)  / arch_topo_lvl[0];
> +	report(stsi_nested_lvl[0] >=  n + 1,
> +	       "CPU Type TLE    : %d expect %d", stsi_nested_lvl[0], n + 1);
> +
> +	/* For each level found in STSI */
> +	for (i = 1; i < CPU_TOPOLOGY_MAX_LEVEL; i++) {
> +		/*
> +		 * For non QEMU/KVM hypervisor the concatenation of the levels
> +		 * above level 1 are architecture dependent.
> +		 * Skip these checks.
> +		 */
> +		if (!host_is_kvm() && sel2 != 2)
> +			continue;
> +
> +		/* For QEMU/KVM we expect a simple calculation */
> +		if (sel2 > i) {
> +			report(stsi_nested_lvl[i] ==  n + 1,
> +			       "Container TLE  %d: %d expect %d", i, stsi_nested_lvl[i], n + 1);
> +			n /= arch_topo_lvl[i];
> +		}
> +	}
> +
> +	report_prefix_pop();
> +}
> +
> +/*
> + * 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;
> +
> +	report_prefix_pushf("mnested %d 15_1_%d", max_nested_lvl, sel2);
> +
> +	ret = stsi(pagebuf, 15, 1, sel2);
> +	if (max_nested_lvl >= sel2) {
> +		report(!ret, "Valid stsi instruction");
> +	} else {
> +		report(ret, "Invalid stsi instruction");
> +		goto end;
> +	}
> +
> +	stsi_check_maxcpus(info);
> +	stsi_check_tle_coherency(info, sel2);

You could also move the two stsi_check_* calls into the first part of the 
if-statement, then you could get rid of the goto in the second part.

> +end:
> +	report_prefix_pop();
> +}
> +
> +static int sclp_get_mnest(void)
> +{
> +	ReadInfo *sccb = (void *)_sccb;
> +
> +	sclp_mark_busy();
> +	memset(_sccb, 0, PAGE_SIZE);
> +	sccb->h.length = PAGE_SIZE;
> +
> +	sclp_service_call(SCLP_CMDW_READ_SCP_INFO, sccb);
> +	assert(sccb->h.response_code == SCLP_RC_NORMAL_READ_COMPLETION);
> +
> +	return sccb->stsi_parm;
> +}
> +
> +/*
> + * test_stsi
> + *
> + * Retrieves the maximum nested topology level supported by the architecture
> + * and the number of CPUs.
> + * Calls the checking for the STSI instruction in sel2 reverse level order
> + * from 6 (CPU_TOPOLOGY_MAX_LEVEL) to 2 to have the most interesting level,
> + * the one triggering a topology-change-report-pending condition, level 2,
> + * at the end of the report.
> + *
> + */
> +static void test_stsi(void)
> +{
> +	int sel2;
> +
> +	max_nested_lvl = sclp_get_mnest();
> +	report_info("SCLP maximum nested level : %d", max_nested_lvl);
> +
> +	number_of_cpus = sclp_get_cpu_num();
> +	report_info("SCLP number of CPU: %d", number_of_cpus);
> +
> +	/* STSI selector 2 can takes values between 2 and 6 */
> +	for (sel2 = 6; sel2 >= 2; sel2--)
> +		check_sysinfo_15_1_x((struct sysinfo_15_1_x *)pagebuf, sel2);
> +}
> +
> +/*
> + * parse_topology_args
> + * @argc: number of arguments
> + * @argv: argument array
> + *
> + * This function initialize the architecture topology levels
> + * which should be the same as the one provided by the hypervisor.
> + *
> + * We use the current names found in IBM/Z literature, Linux and QEMU:
> + * cores, sockets/packages, books, drawers and nodes to facilitate the
> + * human machine interface but store the result in a machine abstract
> + * array of architecture topology levels.
> + * Note that when QEMU uses socket as a name for the topology level 1
> + * Linux uses package or physical_package.
> + */
> +static void parse_topology_args(int argc, char **argv)
> +{
> +	int i;
> +
> +	report_info("%d arguments", argc);
> +	for (i = 1; i < argc; i++) {
> +		if (!strcmp("-cores", argv[i])) {
> +			i++;
> +			if (i >= argc)
> +				report_abort("-cores needs a parameter");
> +			arch_topo_lvl[0] = atol(argv[i]);
> +			report_info("cores: %d", arch_topo_lvl[0]);
> +		} else if (!strcmp("-sockets", argv[i])) {
> +			i++;
> +			if (i >= argc)
> +				report_abort("-sockets needs a parameter");
> +			arch_topo_lvl[1] = atol(argv[i]);
> +			report_info("sockets: %d", arch_topo_lvl[1]);
> +		} else if (!strcmp("-books", argv[i])) {
> +			i++;
> +			if (i >= argc)
> +				report_abort("-books needs a parameter");
> +			arch_topo_lvl[2] = atol(argv[i]);
> +			report_info("books: %d", arch_topo_lvl[2]);
> +		} else if (!strcmp("-drawers", argv[i])) {
> +			i++;
> +			if (i >= argc)
> +				report_abort("-drawers needs a parameter");
> +			arch_topo_lvl[3] = atol(argv[i]);
> +			report_info("drawers: %d", arch_topo_lvl[3]);
> +		}

Maybe abort on unkown parameters, to avoid that typos go unnoticed?

> +	}
> +
> +	for (i = 0; i < CPU_TOPOLOGY_MAX_LEVEL; i++) {
> +		if (!arch_topo_lvl[i])
> +			arch_topo_lvl[i] = 1;
> +		max_cpus *= arch_topo_lvl[i];
> +	}
> +}

  Thomas

Re: [kvm-unit-tests PATCH v6 1/2] s390x: topology: Check the Perform Topology Function

[Pierre Morel]

On 2/8/23 12:06, Thomas Huth wrote:
> On 02/02/2023 10.28, Pierre Morel wrote:
>> We check that the PTF instruction is working correctly when
>> the cpu topology facility is available.
>>
>> For KVM only, we test changing of the polarity between horizontal
>> and vertical and that a reset set the horizontal polarity.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   s390x/Makefile      |   1 +
>>   s390x/topology.c    | 155 ++++++++++++++++++++++++++++++++++++++++++++
>>   s390x/unittests.cfg |   3 +
>>   3 files changed, 159 insertions(+)
>>   create mode 100644 s390x/topology.c
>>
>> diff --git a/s390x/Makefile b/s390x/Makefile
>> index 52a9d82..b5fe8a3 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -39,6 +39,7 @@ tests += $(TEST_DIR)/panic-loop-extint.elf
>>   tests += $(TEST_DIR)/panic-loop-pgm.elf
>>   tests += $(TEST_DIR)/migration-sck.elf
>>   tests += $(TEST_DIR)/exittime.elf
>> +tests += $(TEST_DIR)/topology.elf
>>   pv-tests += $(TEST_DIR)/pv-diags.elf
>> diff --git a/s390x/topology.c b/s390x/topology.c
>> new file mode 100644
>> index 0000000..20f7ba2
>> --- /dev/null
>> +++ b/s390x/topology.c
>> @@ -0,0 +1,155 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * CPU Topology
>> + *
>> + * Copyright IBM Corp. 2022
>> + *
>> + * Authors:
>> + *  Pierre Morel <pmorel@linux.ibm.com>
>> + */
>> +
>> +#include <libcflat.h>
>> +#include <asm/page.h>
>> +#include <asm/asm-offsets.h>
>> +#include <asm/interrupt.h>
>> +#include <asm/facility.h>
>> +#include <smp.h>
>> +#include <sclp.h>
>> +#include <s390x/hardware.h>
>> +
>> +#define PTF_REQ_HORIZONTAL    0
>> +#define PTF_REQ_VERTICAL    1
>> +#define PTF_REQ_CHECK        2
>> +
>> +#define PTF_ERR_NO_REASON    0
>> +#define PTF_ERR_ALRDY_POLARIZED    1
>> +#define PTF_ERR_IN_PROGRESS    2
>> +
>> +extern int diag308_load_reset(u64);
>> +
>> +static int ptf(unsigned long fc, unsigned long *rc)
>> +{
>> +    int cc;
>> +
>> +    asm volatile(
>> +        "       .insn   rre,0xb9a20000,%1,0\n"
> 
> Why are you specifying the instruction manually? I think both, GCC and 
> Clang should know the "ptf" mnemonic, shouldn't they?

:D right !

> 
>> +        "       ipm     %0\n"
>> +        "       srl     %0,28\n"
>> +        : "=d" (cc), "+d" (fc)
>> +        :
>> +        : "cc");
>> +
>> +    *rc = fc >> 8;
>> +    return cc;
>> +}
>> +
>> +static void test_ptf(void)
>> +{
>> +    unsigned long rc;
>> +    int cc;
>> +
>> +    /* PTF is a privilege instruction */
> 
> s/privilege/privileged/ ?

Yes, thanks

> 
>> +    report_prefix_push("Privilege");
>> +    enter_pstate();
>> +    expect_pgm_int();
>> +    ptf(PTF_REQ_CHECK, &rc);
>> +    check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
>> +    report_prefix_pop();
>> +
>> +    report_prefix_push("Wrong fc");
>> +    expect_pgm_int();
>> +    ptf(0xff, &rc);
>> +    check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>> +    report_prefix_pop();
>> +
>> +    report_prefix_push("Reserved bits");
>> +    expect_pgm_int();
>> +    ptf(0xffffffffffffff00UL, &rc);
>> +    check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>> +    report_prefix_pop();
> 
> This function is quite big ... I'd maybe group the above checks for 
> error conditions into a separate function instead.

OK

> 
>> +    report_prefix_push("Topology Report pending");
>> +    /*
>> +     * At this moment the topology may already have changed
>> +     * since the VM has been started.
>> +     * However, we can test if a second PTF instruction
>> +     * reports that the topology did not change since the
>> +     * preceding PFT instruction.
>> +     */
>> +    ptf(PTF_REQ_CHECK, &rc);
>> +    cc = ptf(PTF_REQ_CHECK, &rc);
>> +    report(cc == 0, "PTF check should clear topology report");
>> +    report_prefix_pop();
>> +
>> +    report_prefix_push("Topology polarisation check");
>> +    /*
>> +     * We can not assume the state of the polarization for
> 
> s/can not/cannot/ ?

OK

> 
> Also, you sometimes write polarization with "z" and sometimes with "s". 
> I'd suggest to standardize on "z" (as in "IBM Z" ;-))

OK

> 
>> +     * any Virtual Machine but KVM.
>> +     * Let's skip the polarisation tests for other VMs.
>> +     */
>> +    if (!host_is_kvm()) {
>> +        report_skip("Topology polarisation check is done for KVM only");
>> +        goto end;
>> +    }
>> +
>> +    /*
>> +     * Set vertical polarization to verify that RESET sets
>> +     * horizontal polarization back.
>> +     */
>> +    cc = ptf(PTF_REQ_VERTICAL, &rc);
>> +    report(cc == 0, "Set vertical polarization.");
>> +
>> +    report(diag308_load_reset(1), "load normal reset done");
>> +
>> +    cc = ptf(PTF_REQ_CHECK, &rc);
>> +    report(cc == 0, "Reset should clear topology report");
>> +
>> +    cc = ptf(PTF_REQ_HORIZONTAL, &rc);
>> +    report(cc == 2 && rc == PTF_ERR_ALRDY_POLARIZED,
>> +           "After RESET polarization is horizontal");
>> +
>> +    /* Flip between vertical and horizontal polarization */
>> +    cc = ptf(PTF_REQ_VERTICAL, &rc);
>> +    report(cc == 0, "Change to vertical polarization.");
>> +
>> +    cc = ptf(PTF_REQ_CHECK, &rc);
>> +    report(cc == 1, "Polarization change should set topology report");
>> +
>> +    cc = ptf(PTF_REQ_HORIZONTAL, &rc);
>> +    report(cc == 0, "Change to horizontal polarization.");
>> +
>> +end:
>> +    report_prefix_pop();
>> +}
> 
> Apart from the nits, the patch looks fine to me.
> 
>   Thomas
> 

Thanks,

Regards.
Pierre





-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 2/8/23 12:53, Thomas Huth wrote:
> On 02/02/2023 10.28, Pierre Morel wrote:
>> 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.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
> ...
>> +static inline int cpus_in_tle_mask(uint64_t val)
>> +{
>> +    int i, n;
>> +
>> +    for (i = 0, n = 0; i < 64; i++, val >>= 1)
>> +        if (val & 0x01)
>> +            n++;
>> +    return n;
> 
> I'd suggest to use __builtin_popcountl here instead of looping.

OK

> 
>> +}
>> +
>>   #endif  /* _S390X_STSI_H_ */
>> diff --git a/s390x/topology.c b/s390x/topology.c
>> index 20f7ba2..f21c653 100644
>> --- a/s390x/topology.c
>> +++ b/s390x/topology.c
>> @@ -16,6 +16,18 @@
>>   #include <smp.h>
>>   #include <sclp.h>
>>   #include <s390x/hardware.h>
>> +#include <s390x/stsi.h>
>> +
>> +static uint8_t pagebuf[PAGE_SIZE * 2] 
>> __attribute__((aligned(PAGE_SIZE * 2)));
> 
> Isn't the SYSIB just one page only? Why reserve two pages here?

Yes it is, I change it to a single page.

> 
>> +static int max_nested_lvl;
>> +static int number_of_cpus;
>> +static int max_cpus = 1;
>> +
>> +/* Topology level as defined by architecture */
>> +static int arch_topo_lvl[CPU_TOPOLOGY_MAX_LEVEL];
>> +/* Topology nested level as reported in STSI */
>> +static int stsi_nested_lvl[CPU_TOPOLOGY_MAX_LEVEL];
>>   #define PTF_REQ_HORIZONTAL    0
>>   #define PTF_REQ_VERTICAL    1
>> @@ -122,11 +134,241 @@ end:
>>       report_prefix_pop();
>>   }
>> +/*
>> + * stsi_check_maxcpus
>> + * @info: Pointer to the stsi information
>> + *
>> + * The product of the numbers of containers per level
>> + * is the maximum number of CPU allowed by the machine.
>> + */
>> +static void stsi_check_maxcpus(struct sysinfo_15_1_x *info)
>> +{
>> +    int n, i;
>> +
>> +    report_prefix_push("maximum cpus");
>> +
>> +    for (i = 0, n = 1; i < CPU_TOPOLOGY_MAX_LEVEL; i++) {
>> +        report_info("Mag%d: %d", CPU_TOPOLOGY_MAX_LEVEL - i, 
>> info->mag[i]);
>> +        n *= info->mag[i] ? info->mag[i] : 1;
> 
> You could use the Elvis operator here instead.

Right thanks.


> 
>> +    }
>> +    report(n == max_cpus, "Maximum CPUs %d expected %d", n, max_cpus);
>> +
>> +    report_prefix_pop();
>> +}
>> +
>> +/*
>> + * stsi_check_tle_coherency
>> + * @info: Pointer to the stsi information
>> + * @sel2: Topology level to check.
>> + *
>> + * 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, int 
>> sel2)
>> +{
>> +    struct topology_container *tc, *end;
>> +    struct topology_core *cpus;
>> +    int n = 0;
>> +    int i;
>> +
>> +    report_prefix_push("TLE coherency");
>> +
>> +    tc = &info->tle[0].container;
>> +    end = (struct topology_container *)((unsigned long)info + 
>> info->length);
> 
> s/unsigned long/uintptr_t/ please!

OK, thanks

> 
> 
>> +
>> +    for (i = 0; i < CPU_TOPOLOGY_MAX_LEVEL; i++)
>> +        stsi_nested_lvl[i] = 0;
> 
> memset(stsi_nested_lvl, 0, sizeof(stsi_nested_lvl)) ?

better, thanks

> 
>> +    while (tc < end) {
>> +        if (tc->nl > 5) {
> 
> Use ">= CPU_TOPOLOGY_MAX_LEVEL" instead of "> 5" ?

OK

> 
>> +            report_abort("Unexpected TL Entry: tle->nl: %d", tc->nl);
>> +            return;
>> +        }
>> +        if (tc->nl == 0) {
>> +            cpus = (struct topology_core *)tc;
>> +            n += cpus_in_tle_mask(cpus->mask);
>> +            report_info("cpu type %02x  d: %d pp: %d", cpus->type, 
>> cpus->d, cpus->pp);
>> +            report_info("origin : %04x mask %016lx", cpus->origin, 
>> cpus->mask);
>> +        }
>> +
>> +        stsi_nested_lvl[tc->nl]++;
>> +        report_info("level %d: lvl: %d id: %d cnt: %d",
>> +                tc->nl, tc->nl, tc->id, stsi_nested_lvl[tc->nl]);
>> +
>> +        /* trick: CPU TLEs are twice the size of containers TLE */
>> +        if (tc->nl == 0)
>> +            tc++;
> 
> IMHO it might be cleaner to have a "uint8_t *" or "void *" to the 
> current position in the sysinfo block, and do the pointer arithmetic on 
> that pointer instead... well, it's likely just a matter of taste.

OK

> 
>> +        tc++;
>> +    }
>> +    report(n == number_of_cpus, "Number of CPUs  : %d expect %d", n, 
>> number_of_cpus);
>> +    /*
>> +     * For KVM we accept
>> +     * - only 1 type of CPU
>> +     * - only horizontal topology
>> +     * - only dedicated CPUs
>> +     * This leads to expect the number of entries of level 0 CPU
>> +     * Topology Level Entry (TLE) to be:
>> +     * 1 + (number_of_cpus - 1)  / arch_topo_lvl[0]
>> +     *
>> +     * For z/VM or LPAR this number can only be greater if different
>> +     * polarity, CPU types because there may be a nested level 0 CPU TLE
>> +     * for each of the CPU/polarity/sharing types in a level 1 
>> container TLE.
>> +     */
>> +    n =  (number_of_cpus - 1)  / arch_topo_lvl[0];
>> +    report(stsi_nested_lvl[0] >=  n + 1,
>> +           "CPU Type TLE    : %d expect %d", stsi_nested_lvl[0], n + 1);
>> +
>> +    /* For each level found in STSI */
>> +    for (i = 1; i < CPU_TOPOLOGY_MAX_LEVEL; i++) {
>> +        /*
>> +         * For non QEMU/KVM hypervisor the concatenation of the levels
>> +         * above level 1 are architecture dependent.
>> +         * Skip these checks.
>> +         */
>> +        if (!host_is_kvm() && sel2 != 2)
>> +            continue;
>> +
>> +        /* For QEMU/KVM we expect a simple calculation */
>> +        if (sel2 > i) {
>> +            report(stsi_nested_lvl[i] ==  n + 1,
>> +                   "Container TLE  %d: %d expect %d", i, 
>> stsi_nested_lvl[i], n + 1);
>> +            n /= arch_topo_lvl[i];
>> +        }
>> +    }
>> +
>> +    report_prefix_pop();
>> +}
>> +
>> +/*
>> + * 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;
>> +
>> +    report_prefix_pushf("mnested %d 15_1_%d", max_nested_lvl, sel2);
>> +
>> +    ret = stsi(pagebuf, 15, 1, sel2);
>> +    if (max_nested_lvl >= sel2) {
>> +        report(!ret, "Valid stsi instruction");
>> +    } else {
>> +        report(ret, "Invalid stsi instruction");
>> +        goto end;
>> +    }
>> +
>> +    stsi_check_maxcpus(info);
>> +    stsi_check_tle_coherency(info, sel2);
> 
> You could also move the two stsi_check_* calls into the first part of 
> the if-statement, then you could get rid of the goto in the second part.

Thanks, yes.

> 
>> +end:
>> +    report_prefix_pop();
>> +}
>> +
>> +static int sclp_get_mnest(void)
>> +{
>> +    ReadInfo *sccb = (void *)_sccb;
>> +
>> +    sclp_mark_busy();
>> +    memset(_sccb, 0, PAGE_SIZE);
>> +    sccb->h.length = PAGE_SIZE;
>> +
>> +    sclp_service_call(SCLP_CMDW_READ_SCP_INFO, sccb);
>> +    assert(sccb->h.response_code == SCLP_RC_NORMAL_READ_COMPLETION);
>> +
>> +    return sccb->stsi_parm;
>> +}
>> +
>> +/*
>> + * test_stsi
>> + *
>> + * Retrieves the maximum nested topology level supported by the 
>> architecture
>> + * and the number of CPUs.
>> + * Calls the checking for the STSI instruction in sel2 reverse level 
>> order
>> + * from 6 (CPU_TOPOLOGY_MAX_LEVEL) to 2 to have the most interesting 
>> level,
>> + * the one triggering a topology-change-report-pending condition, 
>> level 2,
>> + * at the end of the report.
>> + *
>> + */
>> +static void test_stsi(void)
>> +{
>> +    int sel2;
>> +
>> +    max_nested_lvl = sclp_get_mnest();
>> +    report_info("SCLP maximum nested level : %d", max_nested_lvl);
>> +
>> +    number_of_cpus = sclp_get_cpu_num();
>> +    report_info("SCLP number of CPU: %d", number_of_cpus);
>> +
>> +    /* STSI selector 2 can takes values between 2 and 6 */
>> +    for (sel2 = 6; sel2 >= 2; sel2--)
>> +        check_sysinfo_15_1_x((struct sysinfo_15_1_x *)pagebuf, sel2);
>> +}
>> +
>> +/*
>> + * parse_topology_args
>> + * @argc: number of arguments
>> + * @argv: argument array
>> + *
>> + * This function initialize the architecture topology levels
>> + * which should be the same as the one provided by the hypervisor.
>> + *
>> + * We use the current names found in IBM/Z literature, Linux and QEMU:
>> + * cores, sockets/packages, books, drawers and nodes to facilitate the
>> + * human machine interface but store the result in a machine abstract
>> + * array of architecture topology levels.
>> + * Note that when QEMU uses socket as a name for the topology level 1
>> + * Linux uses package or physical_package.
>> + */
>> +static void parse_topology_args(int argc, char **argv)
>> +{
>> +    int i;
>> +
>> +    report_info("%d arguments", argc);
>> +    for (i = 1; i < argc; i++) {
>> +        if (!strcmp("-cores", argv[i])) {
>> +            i++;
>> +            if (i >= argc)
>> +                report_abort("-cores needs a parameter");
>> +            arch_topo_lvl[0] = atol(argv[i]);
>> +            report_info("cores: %d", arch_topo_lvl[0]);
>> +        } else if (!strcmp("-sockets", argv[i])) {
>> +            i++;
>> +            if (i >= argc)
>> +                report_abort("-sockets needs a parameter");
>> +            arch_topo_lvl[1] = atol(argv[i]);
>> +            report_info("sockets: %d", arch_topo_lvl[1]);
>> +        } else if (!strcmp("-books", argv[i])) {
>> +            i++;
>> +            if (i >= argc)
>> +                report_abort("-books needs a parameter");
>> +            arch_topo_lvl[2] = atol(argv[i]);
>> +            report_info("books: %d", arch_topo_lvl[2]);
>> +        } else if (!strcmp("-drawers", argv[i])) {
>> +            i++;
>> +            if (i >= argc)
>> +                report_abort("-drawers needs a parameter");
>> +            arch_topo_lvl[3] = atol(argv[i]);
>> +            report_info("drawers: %d", arch_topo_lvl[3]);
>> +        }
> 
> Maybe abort on unkown parameters, to avoid that typos go unnoticed?

Yes, better.
Thanks,

Regards.
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v6 1/2] s390x: topology: Check the Perform Topology Function

[Nina Schoetterl-Glausch]

On Thu, 2023-02-02 at 10:28 +0100, Pierre Morel wrote:
> We check that the PTF instruction is working correctly when
> the cpu topology facility is available.
> 
> For KVM only, we test changing of the polarity between horizontal
> and vertical and that a reset set the horizontal polarity.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  s390x/Makefile      |   1 +
>  s390x/topology.c    | 155 ++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg |   3 +
>  3 files changed, 159 insertions(+)
>  create mode 100644 s390x/topology.c
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 52a9d82..b5fe8a3 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -39,6 +39,7 @@ tests += $(TEST_DIR)/panic-loop-extint.elf
>  tests += $(TEST_DIR)/panic-loop-pgm.elf
>  tests += $(TEST_DIR)/migration-sck.elf
>  tests += $(TEST_DIR)/exittime.elf
> +tests += $(TEST_DIR)/topology.elf
>  
>  pv-tests += $(TEST_DIR)/pv-diags.elf
>  
> diff --git a/s390x/topology.c b/s390x/topology.c
> new file mode 100644
> index 0000000..20f7ba2
> --- /dev/null
> +++ b/s390x/topology.c
> @@ -0,0 +1,155 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * CPU Topology
> + *
> + * Copyright IBM Corp. 2022
> + *
> + * Authors:
> + *  Pierre Morel <pmorel@linux.ibm.com>
> + */
> +
> +#include <libcflat.h>
> +#include <asm/page.h>
> +#include <asm/asm-offsets.h>
> +#include <asm/interrupt.h>
> +#include <asm/facility.h>
> +#include <smp.h>
> +#include <sclp.h>
> +#include <s390x/hardware.h>
> +
> +#define PTF_REQ_HORIZONTAL	0
> +#define PTF_REQ_VERTICAL	1
> +#define PTF_REQ_CHECK		2
> +
> +#define PTF_ERR_NO_REASON	0
> +#define PTF_ERR_ALRDY_POLARIZED	1
> +#define PTF_ERR_IN_PROGRESS	2

Maybe also give the CC codes names for improved readability.

> +
> +extern int diag308_load_reset(u64);
> +
> +static int ptf(unsigned long fc, unsigned long *rc)
> +{
> +	int cc;
> +
> +	asm volatile(
> +		"       .insn   rre,0xb9a20000,%1,0\n"
> +		"       ipm     %0\n"
> +		"       srl     %0,28\n"
> +		: "=d" (cc), "+d" (fc)
> +		:
> +		: "cc");

Personally I always name asm arguments, but it is a very short snippet,
so still very readable. Could also pull the shift into C code,
but again, small difference.

> +
> +	*rc = fc >> 8;
> +	return cc;
> +}
> +
> +static void test_ptf(void)
> +{
> +	unsigned long rc;
> +	int cc;
> +
> +	/* PTF is a privilege instruction */
> +	report_prefix_push("Privilege");
> +	enter_pstate();
> +	expect_pgm_int();
> +	ptf(PTF_REQ_CHECK, &rc);
> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
> +	report_prefix_pop();

IMO, you should repeat this test for all FCs, since some are emulated,
others interpreted by SIE.

> +
> +	report_prefix_push("Wrong fc");

"Undefined fc" is more informative IMO.

> +	expect_pgm_int();
> +	ptf(0xff, &rc);
> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("Reserved bits");
> +	expect_pgm_int();
> +	ptf(0xffffffffffffff00UL, &rc);
> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("Topology Report pending");
> +	/*
> +	 * At this moment the topology may already have changed
> +	 * since the VM has been started.
> +	 * However, we can test if a second PTF instruction
> +	 * reports that the topology did not change since the
> +	 * preceding PFT instruction.
> +	 */
> +	ptf(PTF_REQ_CHECK, &rc);
> +	cc = ptf(PTF_REQ_CHECK, &rc);
> +	report(cc == 0, "PTF check should clear topology report");
> +	report_prefix_pop();
> +
> +	report_prefix_push("Topology polarisation check");
> +	/*
> +	 * We can not assume the state of the polarization for
> +	 * any Virtual Machine but KVM.

Random Capitalization :)
Why can you not test the same thing for other hypervisors/LPAR?

> +	 * Let's skip the polarisation tests for other VMs.
> +	 */
> +	if (!host_is_kvm()) {
> +		report_skip("Topology polarisation check is done for KVM only");
> +		goto end;
> +	}
> +
> +	/*
> +	 * Set vertical polarization to verify that RESET sets
> +	 * horizontal polarization back.
> +	 */

You might want to do a reset here also, since there could be some other
test case that could have run before and modified the polarization.
There isn't right now of course, but doing a reset improves separation of tests.

> +	cc = ptf(PTF_REQ_VERTICAL, &rc);
> +	report(cc == 0, "Set vertical polarization.");
> +
> +	report(diag308_load_reset(1), "load normal reset done");
> +
> +	cc = ptf(PTF_REQ_CHECK, &rc);
> +	report(cc == 0, "Reset should clear topology report");
> +
> +	cc = ptf(PTF_REQ_HORIZONTAL, &rc);
> +	report(cc == 2 && rc == PTF_ERR_ALRDY_POLARIZED,
> +	       "After RESET polarization is horizontal");
> +
> +	/* Flip between vertical and horizontal polarization */
> +	cc = ptf(PTF_REQ_VERTICAL, &rc);
> +	report(cc == 0, "Change to vertical polarization.");
> +
> +	cc = ptf(PTF_REQ_CHECK, &rc);
> +	report(cc == 1, "Polarization change should set topology report");
> +
> +	cc = ptf(PTF_REQ_HORIZONTAL, &rc);
> +	report(cc == 0, "Change to horizontal polarization.");
> +
> +end:
> +	report_prefix_pop();
> +}
> +
> +static struct {
> +	const char *name;
> +	void (*func)(void);
> +} tests[] = {
> +	{ "PTF", test_ptf},
> +	{ NULL, NULL }
> +};
> +
> +int main(int argc, char *argv[])
> +{
> +	int i;
> +
> +	report_prefix_push("CPU Topology");
> +
> +	if (!test_facility(11)) {
> +		report_skip("Topology facility not present");
> +		goto end;
> +	}
> +
> +	report_info("Virtual machine level %ld", stsi_get_fc());
> +
> +	for (i = 0; tests[i].name; i++) {
> +		report_prefix_push(tests[i].name);
> +		tests[i].func();
> +		report_prefix_pop();
> +	}
> +
> +end:
> +	report_prefix_pop();
> +	return report_summary();
> +}
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index 3caf81e..3530cc4 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -208,3 +208,6 @@ groups = migration
>  [exittime]
>  file = exittime.elf
>  smp = 2
> +
> +[topology]
> +file = topology.elf

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

[Nina Schoetterl-Glausch]

On Thu, 2023-02-02 at 10:28 +0100, Pierre Morel wrote:
> 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.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/sclp.h    |   3 +-
>  lib/s390x/stsi.h    |  44 ++++++++
>  s390x/topology.c    | 244 ++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg |   1 +
>  4 files changed, 291 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
> index 853529b..6ecfb0a 100644
> --- a/lib/s390x/sclp.h
> +++ b/lib/s390x/sclp.h
> @@ -150,7 +150,8 @@ typedef struct ReadInfo {
>  	SCCBHeader h;
>  	uint16_t rnmax;
>  	uint8_t rnsize;
> -	uint8_t  _reserved1[16 - 11];       /* 11-15 */
> +	uint8_t  _reserved1[15 - 11];       /* 11-14 */
> +	uint8_t stsi_parm;                  /* 15-15 */
>  	uint16_t entries_cpu;               /* 16-17 */
>  	uint16_t offset_cpu;                /* 18-19 */
>  	uint8_t  _reserved2[24 - 20];       /* 20-23 */
> diff --git a/lib/s390x/stsi.h b/lib/s390x/stsi.h
> index bebc492..8dbbfc2 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 reserved0a;
> +	uint8_t mnest;
> +	uint8_t reserved0c[4];
> +	union topology_entry tle[0];

I'd use [] since it's C99.

> +};
> +
> +static inline int cpus_in_tle_mask(uint64_t val)
> +{
> +	int i, n;
> +
> +	for (i = 0, n = 0; i < 64; i++, val >>= 1)
> +		if (val & 0x01)
> +			n++;
> +	return n;
> +}
> +
>  #endif  /* _S390X_STSI_H_ */
> diff --git a/s390x/topology.c b/s390x/topology.c
> index 20f7ba2..f21c653 100644
> --- a/s390x/topology.c
> +++ b/s390x/topology.c
> @@ -16,6 +16,18 @@
>  #include <smp.h>
>  #include <sclp.h>
>  #include <s390x/hardware.h>
> +#include <s390x/stsi.h>
> +
> +static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
> +
> +static int max_nested_lvl;
> +static int number_of_cpus;
> +static int max_cpus = 1;

IMO you shouldn't initialize this, it's a bit confusing because it gets modified later.
I think it's good to initialize globals where one would expect it, so in main, or
at the very top of the individual test case.

> +
> +/* Topology level as defined by architecture */
> +static int arch_topo_lvl[CPU_TOPOLOGY_MAX_LEVEL];
> +/* Topology nested level as reported in STSI */
> +static int stsi_nested_lvl[CPU_TOPOLOGY_MAX_LEVEL];
>  
>  #define PTF_REQ_HORIZONTAL	0
>  #define PTF_REQ_VERTICAL	1
> @@ -122,11 +134,241 @@ end:
>  	report_prefix_pop();
>  }
>  
> +/*
> + * stsi_check_maxcpus
> + * @info: Pointer to the stsi information
> + *
> + * The product of the numbers of containers per level
> + * is the maximum number of CPU allowed by the machine.
> + */
> +static void stsi_check_maxcpus(struct sysinfo_15_1_x *info)
> +{
> +	int n, i;
> +
> +	report_prefix_push("maximum cpus");
> +
> +	for (i = 0, n = 1; i < CPU_TOPOLOGY_MAX_LEVEL; i++) {
> +		report_info("Mag%d: %d", CPU_TOPOLOGY_MAX_LEVEL - i, info->mag[i]);
> +		n *= info->mag[i] ? info->mag[i] : 1;
> +	}
> +	report(n == max_cpus, "Maximum CPUs %d expected %d", n, max_cpus);
> +
> +	report_prefix_pop();
> +}
> +
> +/*
> + * stsi_check_tle_coherency
> + * @info: Pointer to the stsi information
> + * @sel2: Topology level to check.
> + *
> + * 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, int sel2)
> +{
> +	struct topology_container *tc, *end;
> +	struct topology_core *cpus;
> +	int n = 0;
> +	int i;
> +
> +	report_prefix_push("TLE coherency");
> +
> +	tc = &info->tle[0].container;
> +	end = (struct topology_container *)((unsigned long)info + info->length);
> +
> +	for (i = 0; i < CPU_TOPOLOGY_MAX_LEVEL; i++)
> +		stsi_nested_lvl[i] = 0;
> +
> +	while (tc < end) {
> +		if (tc->nl > 5) {
> +			report_abort("Unexpected TL Entry: tle->nl: %d", tc->nl);
> +			return;
> +		}
> +		if (tc->nl == 0) {
> +			cpus = (struct topology_core *)tc;
> +			n += cpus_in_tle_mask(cpus->mask);
> +			report_info("cpu type %02x  d: %d pp: %d", cpus->type, cpus->d, cpus->pp);
> +			report_info("origin : %04x mask %016lx", cpus->origin, cpus->mask);
> +		}
> +
> +		stsi_nested_lvl[tc->nl]++;
> +		report_info("level %d: lvl: %d id: %d cnt: %d",
> +			    tc->nl, tc->nl, tc->id, stsi_nested_lvl[tc->nl]);
> +
> +		/* trick: CPU TLEs are twice the size of containers TLE */
> +		if (tc->nl == 0)
> +			tc++;
> +		tc++;
> +	}
> +	report(n == number_of_cpus, "Number of CPUs  : %d expect %d", n, number_of_cpus);
> +	/*
> +	 * For KVM we accept
> +	 * - only 1 type of CPU
> +	 * - only horizontal topology
> +	 * - only dedicated CPUs
> +	 * This leads to expect the number of entries of level 0 CPU
> +	 * Topology Level Entry (TLE) to be:
> +	 * 1 + (number_of_cpus - 1)  / arch_topo_lvl[0]
> +	 *
> +	 * For z/VM or LPAR this number can only be greater if different
> +	 * polarity, CPU types because there may be a nested level 0 CPU TLE
> +	 * for each of the CPU/polarity/sharing types in a level 1 container TLE.
> +	 */

IMO you should only test what is defined by the architecture. This behavior isn't
even dependent on KVM, but on user space, ok effectively this is QEMU but you never know.
And that behavior could change in the future.

> +	n =  (number_of_cpus - 1)  / arch_topo_lvl[0];
> +	report(stsi_nested_lvl[0] >=  n + 1,
> +	       "CPU Type TLE    : %d expect %d", stsi_nested_lvl[0], n + 1);
> +
> +	/* For each level found in STSI */
> +	for (i = 1; i < CPU_TOPOLOGY_MAX_LEVEL; i++) {
> +		/*
> +		 * For non QEMU/KVM hypervisor the concatenation of the levels
> +		 * above level 1 are architecture dependent.
> +		 * Skip these checks.
> +		 */
> +		if (!host_is_kvm() && sel2 != 2)
> +			continue;
> +
> +		/* For QEMU/KVM we expect a simple calculation */
> +		if (sel2 > i) {
> +			report(stsi_nested_lvl[i] ==  n + 1,
> +			       "Container TLE  %d: %d expect %d", i, stsi_nested_lvl[i], n + 1);
> +			n /= arch_topo_lvl[i];
> +		}
> +	}
> +
> +	report_prefix_pop();
> +}

I think you could make this test stricter and more readable by using recursion.
You have a function check_container which:
* checks that each child has nesting level one level lower
* calls check_container on the child
	* gets back the number of cpus in the child
	* gets back the next child
* if the current "child" isn't actually a child but has one higher nesting level we're done
	* check if sum of child cpus makes sense
* error out on any anomaly (e.g required 0 bits)
* return "child" and sum of cpus

For CPU entries you have a special checking function:
* check as much as you can
* calculate number of cpus in mask
* you can also test if the actual cpus exist by:
	* building a bitmap of all cpus first before the test
	* checking if the bits in the mask are a subset of existing cpus
> +
> +/*
> + * 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;
> +
> +	report_prefix_pushf("mnested %d 15_1_%d", max_nested_lvl, sel2);
> +
> +	ret = stsi(pagebuf, 15, 1, sel2);
> +	if (max_nested_lvl >= sel2) {
> +		report(!ret, "Valid stsi instruction");
> +	} else {
> +		report(ret, "Invalid stsi instruction");
> +		goto end;
> +	}
> +
> +	stsi_check_maxcpus(info);
> +	stsi_check_tle_coherency(info, sel2);
> +
> +end:
> +	report_prefix_pop();
> +}
> +
> +static int sclp_get_mnest(void)
> +{
> +	ReadInfo *sccb = (void *)_sccb;
> +
> +	sclp_mark_busy();
> +	memset(_sccb, 0, PAGE_SIZE);
> +	sccb->h.length = PAGE_SIZE;
> +
> +	sclp_service_call(SCLP_CMDW_READ_SCP_INFO, sccb);
> +	assert(sccb->h.response_code == SCLP_RC_NORMAL_READ_COMPLETION);
> +
> +	return sccb->stsi_parm;
> +}

I think it would be better if you just add a function sclp_get_stsi_max_sel
to lib/s390x/sclp.[ch] that reads the value from read_info and calculates
the maximum selector value.

> +
> +/*
> + * test_stsi
> + *
> + * Retrieves the maximum nested topology level supported by the architecture
> + * and the number of CPUs.
> + * Calls the checking for the STSI instruction in sel2 reverse level order
> + * from 6 (CPU_TOPOLOGY_MAX_LEVEL) to 2 to have the most interesting level,
> + * the one triggering a topology-change-report-pending condition, level 2,
> + * at the end of the report.
> + *
> + */
> +static void test_stsi(void)
> +{
> +	int sel2;
> +
> +	max_nested_lvl = sclp_get_mnest();
> +	report_info("SCLP maximum nested level : %d", max_nested_lvl);
> +
> +	number_of_cpus = sclp_get_cpu_num();
> +	report_info("SCLP number of CPU: %d", number_of_cpus);
> +
> +	/* STSI selector 2 can takes values between 2 and 6 */
> +	for (sel2 = 6; sel2 >= 2; sel2--)
> +		check_sysinfo_15_1_x((struct sysinfo_15_1_x *)pagebuf, sel2);
> +}
> +
> +/*
> + * parse_topology_args
> + * @argc: number of arguments
> + * @argv: argument array
> + *
> + * This function initialize the architecture topology levels
> + * which should be the same as the one provided by the hypervisor.
> + *
> + * We use the current names found in IBM/Z literature, Linux and QEMU:
> + * cores, sockets/packages, books, drawers and nodes to facilitate the
> + * human machine interface but store the result in a machine abstract
> + * array of architecture topology levels.
> + * Note that when QEMU uses socket as a name for the topology level 1
> + * Linux uses package or physical_package.
> + */
> +static void parse_topology_args(int argc, char **argv)
> +{
> +	int i;
> +
> +	report_info("%d arguments", argc);
> +	for (i = 1; i < argc; i++) {
> +		if (!strcmp("-cores", argv[i])) {
> +			i++;
> +			if (i >= argc)
> +				report_abort("-cores needs a parameter");
> +			arch_topo_lvl[0] = atol(argv[i]);

You don't do any error checking of the number parsing, and don't check the sign.
You could use parse_unsigned in spec_ex.c, should maybe be in a lib somewhere instead.

> +			report_info("cores: %d", arch_topo_lvl[0]);
> +		} else if (!strcmp("-sockets", argv[i])) {
> +			i++;
> +			if (i >= argc)
> +				report_abort("-sockets needs a parameter");
> +			arch_topo_lvl[1] = atol(argv[i]);
> +			report_info("sockets: %d", arch_topo_lvl[1]);
> +		} else if (!strcmp("-books", argv[i])) {
> +			i++;
> +			if (i >= argc)
> +				report_abort("-books needs a parameter");
> +			arch_topo_lvl[2] = atol(argv[i]);
> +			report_info("books: %d", arch_topo_lvl[2]);
> +		} else if (!strcmp("-drawers", argv[i])) {
> +			i++;
> +			if (i >= argc)
> +				report_abort("-drawers needs a parameter");
> +			arch_topo_lvl[3] = atol(argv[i]);
> +			report_info("drawers: %d", arch_topo_lvl[3]);
> +		}

Might be nice to error out if no flags match.
I'm guessing, currently, -sockets -cores 1 would "parse", but the result is nonsense

> +	}

You can also get rid of some code redundancy:
        char *levels[] = { "cores", "sockets", "books", "drawers" };
        for (...) {
                char *flag = argv[i];
                int level;

                if (flag[0] != '-')
                        report_abort("Expected ...");
                flag++;
                for (level = 0; ARRAY_SIZE(levels); level++) {
                        if (!strcmp(levels[level]), flag)
                                break;
                }
                if (level == ARRAY_SIZE(levels))
                        report_abort("Unknown ...");

                arch_topo_lvl[level] = atol(argv[++i]);
                report_info("%s: %d", levels[level], arch_topo_lvl[level]);
        }
> +
> +	for (i = 0; i < CPU_TOPOLOGY_MAX_LEVEL; i++) {
> +		if (!arch_topo_lvl[i])
> +			arch_topo_lvl[i] = 1;
> +		max_cpus *= arch_topo_lvl[i];
> +	}

Might be nice to split this function up into two, one that parses the values
into an array and one that calculates max_cpus.
Then you can do max_cpus = calculate_max_cpus or whatever and when someone is
looking for where max_cpus is defined searching for "max_cpus =" works.

> +}
> +
>  static struct {
>  	const char *name;
>  	void (*func)(void);
>  } tests[] = {
>  	{ "PTF", test_ptf},
> +	{ "STSI", test_stsi},
>  	{ NULL, NULL }
>  };
>  
> @@ -136,6 +378,8 @@ int main(int argc, char *argv[])
>  
>  	report_prefix_push("CPU Topology");
>  
> +	parse_topology_args(argc, argv);
> +
>  	if (!test_facility(11)) {
>  		report_skip("Topology facility not present");
>  		goto end;
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index 3530cc4..b697aca 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -211,3 +211,4 @@ smp = 2
>  
>  [topology]
>  file = topology.elf
> +extra_params=-smp 5,drawers=3,books=3,sockets=4,cores=4,maxcpus=144 -append '-drawers 3 -books 3 -sockets 4 -cores 4'

Re: [kvm-unit-tests PATCH v6 1/2] s390x: topology: Check the Perform Topology Function

[Pierre Morel]

On 2/10/23 15:51, Nina Schoetterl-Glausch wrote:
> On Thu, 2023-02-02 at 10:28 +0100, Pierre Morel wrote:
>> We check that the PTF instruction is working correctly when
>> the cpu topology facility is available.
>>
>> For KVM only, we test changing of the polarity between horizontal
>> and vertical and that a reset set the horizontal polarity.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   s390x/Makefile      |   1 +
>>   s390x/topology.c    | 155 ++++++++++++++++++++++++++++++++++++++++++++
>>   s390x/unittests.cfg |   3 +
>>   3 files changed, 159 insertions(+)
>>   create mode 100644 s390x/topology.c
>>
>> diff --git a/s390x/Makefile b/s390x/Makefile
>> index 52a9d82..b5fe8a3 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -39,6 +39,7 @@ tests += $(TEST_DIR)/panic-loop-extint.elf
>>   tests += $(TEST_DIR)/panic-loop-pgm.elf
>>   tests += $(TEST_DIR)/migration-sck.elf
>>   tests += $(TEST_DIR)/exittime.elf
>> +tests += $(TEST_DIR)/topology.elf
>>   
>>   pv-tests += $(TEST_DIR)/pv-diags.elf
>>   
>> diff --git a/s390x/topology.c b/s390x/topology.c
>> new file mode 100644
>> index 0000000..20f7ba2
>> --- /dev/null
>> +++ b/s390x/topology.c
>> @@ -0,0 +1,155 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * CPU Topology
>> + *
>> + * Copyright IBM Corp. 2022
>> + *
>> + * Authors:
>> + *  Pierre Morel <pmorel@linux.ibm.com>
>> + */
>> +
>> +#include <libcflat.h>
>> +#include <asm/page.h>
>> +#include <asm/asm-offsets.h>
>> +#include <asm/interrupt.h>
>> +#include <asm/facility.h>
>> +#include <smp.h>
>> +#include <sclp.h>
>> +#include <s390x/hardware.h>
>> +
>> +#define PTF_REQ_HORIZONTAL	0
>> +#define PTF_REQ_VERTICAL	1
>> +#define PTF_REQ_CHECK		2
>> +
>> +#define PTF_ERR_NO_REASON	0
>> +#define PTF_ERR_ALRDY_POLARIZED	1
>> +#define PTF_ERR_IN_PROGRESS	2
> 
> Maybe also give the CC codes names for improved readability.

OK

> 
>> +
>> +extern int diag308_load_reset(u64);
>> +
>> +static int ptf(unsigned long fc, unsigned long *rc)
>> +{
>> +	int cc;
>> +
>> +	asm volatile(
>> +		"       .insn   rre,0xb9a20000,%1,0\n"
>> +		"       ipm     %0\n"
>> +		"       srl     %0,28\n"
>> +		: "=d" (cc), "+d" (fc)
>> +		:
>> +		: "cc");
> 
> Personally I always name asm arguments, but it is a very short snippet,
> so still very readable. Could also pull the shift into C code,
> but again, small difference.
> 
>> +
>> +	*rc = fc >> 8;
>> +	return cc;
>> +}
>> +
>> +static void test_ptf(void)
>> +{
>> +	unsigned long rc;
>> +	int cc;
>> +
>> +	/* PTF is a privilege instruction */
>> +	report_prefix_push("Privilege");
>> +	enter_pstate();
>> +	expect_pgm_int();
>> +	ptf(PTF_REQ_CHECK, &rc);
>> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
>> +	report_prefix_pop();
> 
> IMO, you should repeat this test for all FCs, since some are emulated,
> others interpreted by SIE.

right

> 
>> +
>> +	report_prefix_push("Wrong fc");
> 
> "Undefined fc" is more informative IMO.

OK

> 
>> +	expect_pgm_int();
>> +	ptf(0xff, &rc);
>> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>> +	report_prefix_pop();
>> +
>> +	report_prefix_push("Reserved bits");
>> +	expect_pgm_int();
>> +	ptf(0xffffffffffffff00UL, &rc);
>> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>> +	report_prefix_pop();
>> +
>> +	report_prefix_push("Topology Report pending");
>> +	/*
>> +	 * At this moment the topology may already have changed
>> +	 * since the VM has been started.
>> +	 * However, we can test if a second PTF instruction
>> +	 * reports that the topology did not change since the
>> +	 * preceding PFT instruction.
>> +	 */
>> +	ptf(PTF_REQ_CHECK, &rc);
>> +	cc = ptf(PTF_REQ_CHECK, &rc);
>> +	report(cc == 0, "PTF check should clear topology report");
>> +	report_prefix_pop();
>> +
>> +	report_prefix_push("Topology polarisation check");
>> +	/*
>> +	 * We can not assume the state of the polarization for
>> +	 * any Virtual Machine but KVM.
> 
> Random Capitalization :)

OK

> Why can you not test the same thing for other hypervisors/LPAR?

At first QEMU did not support vertical polarization so my tests would 
have get a false negative on LPAR.
I could have done different tests but did not.

I think that now it is alright to do the checks on LPAR too.


> 
>> +	 * Let's skip the polarisation tests for other VMs.
>> +	 */
>> +	if (!host_is_kvm()) {
>> +		report_skip("Topology polarisation check is done for KVM only");
>> +		goto end;
>> +	}
>> +
>> +	/*
>> +	 * Set vertical polarization to verify that RESET sets
>> +	 * horizontal polarization back.
>> +	 */
> 
> You might want to do a reset here also, since there could be some other
> test case that could have run before and modified the polarization.
> There isn't right now of course, but doing a reset improves separation of tests.

Not sure about this but it does not arm so why not.

Thanks.

regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 2/10/23 16:39, Nina Schoetterl-Glausch wrote:
> On Thu, 2023-02-02 at 10:28 +0100, Pierre Morel wrote:
>> 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.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/sclp.h    |   3 +-
>>   lib/s390x/stsi.h    |  44 ++++++++
>>   s390x/topology.c    | 244 ++++++++++++++++++++++++++++++++++++++++++++
>>   s390x/unittests.cfg |   1 +
>>   4 files changed, 291 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
>> index 853529b..6ecfb0a 100644
>> --- a/lib/s390x/sclp.h
>> +++ b/lib/s390x/sclp.h
>> @@ -150,7 +150,8 @@ typedef struct ReadInfo {
>>   	SCCBHeader h;
>>   	uint16_t rnmax;
>>   	uint8_t rnsize;
>> -	uint8_t  _reserved1[16 - 11];       /* 11-15 */
>> +	uint8_t  _reserved1[15 - 11];       /* 11-14 */
>> +	uint8_t stsi_parm;                  /* 15-15 */
>>   	uint16_t entries_cpu;               /* 16-17 */
>>   	uint16_t offset_cpu;                /* 18-19 */
>>   	uint8_t  _reserved2[24 - 20];       /* 20-23 */
>> diff --git a/lib/s390x/stsi.h b/lib/s390x/stsi.h
>> index bebc492..8dbbfc2 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 reserved0a;
>> +	uint8_t mnest;
>> +	uint8_t reserved0c[4];
>> +	union topology_entry tle[0];
> 
> I'd use [] since it's C99.

OK

> 
>> +};
>> +
>> +static inline int cpus_in_tle_mask(uint64_t val)
>> +{
>> +	int i, n;
>> +
>> +	for (i = 0, n = 0; i < 64; i++, val >>= 1)
>> +		if (val & 0x01)
>> +			n++;
>> +	return n;
>> +}
>> +
>>   #endif  /* _S390X_STSI_H_ */
>> diff --git a/s390x/topology.c b/s390x/topology.c
>> index 20f7ba2..f21c653 100644
>> --- a/s390x/topology.c
>> +++ b/s390x/topology.c
>> @@ -16,6 +16,18 @@
>>   #include <smp.h>
>>   #include <sclp.h>
>>   #include <s390x/hardware.h>
>> +#include <s390x/stsi.h>
>> +
>> +static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
>> +
>> +static int max_nested_lvl;
>> +static int number_of_cpus;
>> +static int max_cpus = 1;
> 
> IMO you shouldn't initialize this, it's a bit confusing because it gets modified later.
> I think it's good to initialize globals where one would expect it, so in main, or
> at the very top of the individual test case.

OK

> 
>> +
>> +/* Topology level as defined by architecture */
>> +static int arch_topo_lvl[CPU_TOPOLOGY_MAX_LEVEL];
>> +/* Topology nested level as reported in STSI */
>> +static int stsi_nested_lvl[CPU_TOPOLOGY_MAX_LEVEL];
>>   
>>   #define PTF_REQ_HORIZONTAL	0
>>   #define PTF_REQ_VERTICAL	1
>> @@ -122,11 +134,241 @@ end:
>>   	report_prefix_pop();
>>   }
>>   
>> +/*
>> + * stsi_check_maxcpus
>> + * @info: Pointer to the stsi information
>> + *
>> + * The product of the numbers of containers per level
>> + * is the maximum number of CPU allowed by the machine.
>> + */
>> +static void stsi_check_maxcpus(struct sysinfo_15_1_x *info)
>> +{
>> +	int n, i;
>> +
>> +	report_prefix_push("maximum cpus");
>> +
>> +	for (i = 0, n = 1; i < CPU_TOPOLOGY_MAX_LEVEL; i++) {
>> +		report_info("Mag%d: %d", CPU_TOPOLOGY_MAX_LEVEL - i, info->mag[i]);
>> +		n *= info->mag[i] ? info->mag[i] : 1;
>> +	}
>> +	report(n == max_cpus, "Maximum CPUs %d expected %d", n, max_cpus);
>> +
>> +	report_prefix_pop();
>> +}
>> +
>> +/*
>> + * stsi_check_tle_coherency
>> + * @info: Pointer to the stsi information
>> + * @sel2: Topology level to check.
>> + *
>> + * 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, int sel2)
>> +{
>> +	struct topology_container *tc, *end;
>> +	struct topology_core *cpus;
>> +	int n = 0;
>> +	int i;
>> +
>> +	report_prefix_push("TLE coherency");
>> +
>> +	tc = &info->tle[0].container;
>> +	end = (struct topology_container *)((unsigned long)info + info->length);
>> +
>> +	for (i = 0; i < CPU_TOPOLOGY_MAX_LEVEL; i++)
>> +		stsi_nested_lvl[i] = 0;
>> +
>> +	while (tc < end) {
>> +		if (tc->nl > 5) {
>> +			report_abort("Unexpected TL Entry: tle->nl: %d", tc->nl);
>> +			return;
>> +		}
>> +		if (tc->nl == 0) {
>> +			cpus = (struct topology_core *)tc;
>> +			n += cpus_in_tle_mask(cpus->mask);
>> +			report_info("cpu type %02x  d: %d pp: %d", cpus->type, cpus->d, cpus->pp);
>> +			report_info("origin : %04x mask %016lx", cpus->origin, cpus->mask);
>> +		}
>> +
>> +		stsi_nested_lvl[tc->nl]++;
>> +		report_info("level %d: lvl: %d id: %d cnt: %d",
>> +			    tc->nl, tc->nl, tc->id, stsi_nested_lvl[tc->nl]);
>> +
>> +		/* trick: CPU TLEs are twice the size of containers TLE */
>> +		if (tc->nl == 0)
>> +			tc++;
>> +		tc++;
>> +	}
>> +	report(n == number_of_cpus, "Number of CPUs  : %d expect %d", n, number_of_cpus);
>> +	/*
>> +	 * For KVM we accept
>> +	 * - only 1 type of CPU
>> +	 * - only horizontal topology
>> +	 * - only dedicated CPUs
>> +	 * This leads to expect the number of entries of level 0 CPU
>> +	 * Topology Level Entry (TLE) to be:
>> +	 * 1 + (number_of_cpus - 1)  / arch_topo_lvl[0]
>> +	 *
>> +	 * For z/VM or LPAR this number can only be greater if different
>> +	 * polarity, CPU types because there may be a nested level 0 CPU TLE
>> +	 * for each of the CPU/polarity/sharing types in a level 1 container TLE.
>> +	 */
> 
> IMO you should only test what is defined by the architecture. This behavior isn't
> even dependent on KVM, but on user space, ok effectively this is QEMU but you never know.
> And that behavior could change in the future.

Right, I will let fall this check as I did not find any specification 
about how the levels collapse in the documentation.
And the measures on LPAR gives unexpected results.


> 
>> +	n =  (number_of_cpus - 1)  / arch_topo_lvl[0];
>> +	report(stsi_nested_lvl[0] >=  n + 1,
>> +	       "CPU Type TLE    : %d expect %d", stsi_nested_lvl[0], n + 1);
>> +
>> +	/* For each level found in STSI */
>> +	for (i = 1; i < CPU_TOPOLOGY_MAX_LEVEL; i++) {
>> +		/*
>> +		 * For non QEMU/KVM hypervisor the concatenation of the levels
>> +		 * above level 1 are architecture dependent.
>> +		 * Skip these checks.
>> +		 */
>> +		if (!host_is_kvm() && sel2 != 2)
>> +			continue;
>> +
>> +		/* For QEMU/KVM we expect a simple calculation */
>> +		if (sel2 > i) {
>> +			report(stsi_nested_lvl[i] ==  n + 1,
>> +			       "Container TLE  %d: %d expect %d", i, stsi_nested_lvl[i], n + 1);
>> +			n /= arch_topo_lvl[i];
>> +		}
>> +	}
>> +
>> +	report_prefix_pop();
>> +}
> 
> I think you could make this test stricter and more readable by using recursion.
> You have a function check_container which:
> * checks that each child has nesting level one level lower
> * calls check_container on the child
> 	* gets back the number of cpus in the child
> 	* gets back the next child
> * if the current "child" isn't actually a child but has one higher nesting level we're done
> 	* check if sum of child cpus makes sense
> * error out on any anomaly (e.g required 0 bits)
> * return "child" and sum of cpus
> 
> For CPU entries you have a special checking function:
> * check as much as you can
> * calculate number of cpus in mask
> * you can also test if the actual cpus exist by:
> 	* building a bitmap of all cpus first before the test
> 	* checking if the bits in the mask are a subset of existing cpus

That is very interesting however I wonder if we should not propose the 
test without these checks first, because it is quite finish then, and 
expand the test in a second series.

IMO having a test now even not complete and get more intense checking 
later is better than to have nothing until we finally have all tests we 
can think about in the framework.


>> +
>> +/*
>> + * 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;
>> +
>> +	report_prefix_pushf("mnested %d 15_1_%d", max_nested_lvl, sel2);
>> +
>> +	ret = stsi(pagebuf, 15, 1, sel2);
>> +	if (max_nested_lvl >= sel2) {
>> +		report(!ret, "Valid stsi instruction");
>> +	} else {
>> +		report(ret, "Invalid stsi instruction");
>> +		goto end;
>> +	}
>> +
>> +	stsi_check_maxcpus(info);
>> +	stsi_check_tle_coherency(info, sel2);
>> +
>> +end:
>> +	report_prefix_pop();
>> +}
>> +
>> +static int sclp_get_mnest(void)
>> +{
>> +	ReadInfo *sccb = (void *)_sccb;
>> +
>> +	sclp_mark_busy();
>> +	memset(_sccb, 0, PAGE_SIZE);
>> +	sccb->h.length = PAGE_SIZE;
>> +
>> +	sclp_service_call(SCLP_CMDW_READ_SCP_INFO, sccb);
>> +	assert(sccb->h.response_code == SCLP_RC_NORMAL_READ_COMPLETION);
>> +
>> +	return sccb->stsi_parm;
>> +}
> 
> I think it would be better if you just add a function sclp_get_stsi_max_sel
> to lib/s390x/sclp.[ch] that reads the value from read_info and calculates
> the maximum selector value.

OK

> 
>> +
>> +/*
>> + * test_stsi
>> + *
>> + * Retrieves the maximum nested topology level supported by the architecture
>> + * and the number of CPUs.
>> + * Calls the checking for the STSI instruction in sel2 reverse level order
>> + * from 6 (CPU_TOPOLOGY_MAX_LEVEL) to 2 to have the most interesting level,
>> + * the one triggering a topology-change-report-pending condition, level 2,
>> + * at the end of the report.
>> + *
>> + */
>> +static void test_stsi(void)
>> +{
>> +	int sel2;
>> +
>> +	max_nested_lvl = sclp_get_mnest();
>> +	report_info("SCLP maximum nested level : %d", max_nested_lvl);
>> +
>> +	number_of_cpus = sclp_get_cpu_num();
>> +	report_info("SCLP number of CPU: %d", number_of_cpus);
>> +
>> +	/* STSI selector 2 can takes values between 2 and 6 */
>> +	for (sel2 = 6; sel2 >= 2; sel2--)
>> +		check_sysinfo_15_1_x((struct sysinfo_15_1_x *)pagebuf, sel2);
>> +}
>> +
>> +/*
>> + * parse_topology_args
>> + * @argc: number of arguments
>> + * @argv: argument array
>> + *
>> + * This function initialize the architecture topology levels
>> + * which should be the same as the one provided by the hypervisor.
>> + *
>> + * We use the current names found in IBM/Z literature, Linux and QEMU:
>> + * cores, sockets/packages, books, drawers and nodes to facilitate the
>> + * human machine interface but store the result in a machine abstract
>> + * array of architecture topology levels.
>> + * Note that when QEMU uses socket as a name for the topology level 1
>> + * Linux uses package or physical_package.
>> + */
>> +static void parse_topology_args(int argc, char **argv)
>> +{
>> +	int i;
>> +
>> +	report_info("%d arguments", argc);
>> +	for (i = 1; i < argc; i++) {
>> +		if (!strcmp("-cores", argv[i])) {
>> +			i++;
>> +			if (i >= argc)
>> +				report_abort("-cores needs a parameter");
>> +			arch_topo_lvl[0] = atol(argv[i]);
> 
> You don't do any error checking of the number parsing, and don't check the sign.
> You could use parse_unsigned in spec_ex.c, should maybe be in a lib somewhere instead.

OK

> 
>> +			report_info("cores: %d", arch_topo_lvl[0]);
>> +		} else if (!strcmp("-sockets", argv[i])) {
>> +			i++;
>> +			if (i >= argc)
>> +				report_abort("-sockets needs a parameter");
>> +			arch_topo_lvl[1] = atol(argv[i]);
>> +			report_info("sockets: %d", arch_topo_lvl[1]);
>> +		} else if (!strcmp("-books", argv[i])) {
>> +			i++;
>> +			if (i >= argc)
>> +				report_abort("-books needs a parameter");
>> +			arch_topo_lvl[2] = atol(argv[i]);
>> +			report_info("books: %d", arch_topo_lvl[2]);
>> +		} else if (!strcmp("-drawers", argv[i])) {
>> +			i++;
>> +			if (i >= argc)
>> +				report_abort("-drawers needs a parameter");
>> +			arch_topo_lvl[3] = atol(argv[i]);
>> +			report_info("drawers: %d", arch_topo_lvl[3]);
>> +		}
> 
> Might be nice to error out if no flags match.
> I'm guessing, currently, -sockets -cores 1 would "parse", but the result is nonsense

OK

> 
>> +	}
> 
> You can also get rid of some code redundancy:
>          char *levels[] = { "cores", "sockets", "books", "drawers" };
>          for (...) {
>                  char *flag = argv[i];
>                  int level;
> 
>                  if (flag[0] != '-')
>                          report_abort("Expected ...");
>                  flag++;
>                  for (level = 0; ARRAY_SIZE(levels); level++) {
>                          if (!strcmp(levels[level]), flag)
>                                  break;
>                  }
>                  if (level == ARRAY_SIZE(levels))
>                          report_abort("Unknown ...");
> 
>                  arch_topo_lvl[level] = atol(argv[++i]);
>                  report_info("%s: %d", levels[level], arch_topo_lvl[level]);
>          }

OK, I take it.

>> +
>> +	for (i = 0; i < CPU_TOPOLOGY_MAX_LEVEL; i++) {
>> +		if (!arch_topo_lvl[i])
>> +			arch_topo_lvl[i] = 1;
>> +		max_cpus *= arch_topo_lvl[i];
>> +	}
> 
> Might be nice to split this function up into two, one that parses the values
> into an array and one that calculates max_cpus.
> Then you can do max_cpus = calculate_max_cpus or whatever and when someone is
> looking for where max_cpus is defined searching for "max_cpus =" works.

OK

Regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen