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

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

[Pierre Morel]

Hi,

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

0. what is new in this new spin
-------------------------------

- Running the test on LPAR has been getestet on a46lp21
- splitting big functions in small ones
- better tests, reset before test
- Diverse rephrasing and functions reordering

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

Note that Fedora-35 already has the CPU Topology backport for Linux.

To start the test in KVM just do:

# ./run_tests.sh topology

or something like:

# ./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 need to define the loader entry.
For example, under RedHat:

	title topology
	version topology.bin
	linux /boot/topology.bin
	initrd /boot/initramfs-topology.bin.img
	options -drawers 4 -books 4 -sockets 2 -cores 8
	id rhel-0-topology.bin
	grub_users $grub_users
	grub_arg --unrestricted
	grub_class kernel

Output of the test is done on the SCLP console.

Regards,
Pierre

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

 lib/s390x/sclp.c    |   5 +
 lib/s390x/sclp.h    |   4 +-
 lib/s390x/stsi.h    |  36 ++++
 s390x/Makefile      |   1 +
 s390x/topology.c    | 471 ++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |  11 ++
 6 files changed, 527 insertions(+), 1 deletion(-)
 create mode 100644 s390x/topology.c

-- 
2.31.1

new in v7:

- better checks using device attributes on commandline
  (Pierre)
- use builtin to get the number of CPU in the TLE mask
  (Thomas)
- use Elvis (not dead)
  (Thomas)
- reset before tests
  (Nina)
- splitting test_ptf in small functions
  (Thomas)
- check every ptf function code for program check
  (Nina)
- Test made on LPAR
  (Janosch)
- use a single page for SYSIB
  (Thomas)
- abort on wrong parameter
  (Thomas)
- implement SYSIB check with a recursive funtion
  (Nina)
- diverse little changes (naming, clearer checks
  (Nina, Thomas)

[kvm-unit-tests PATCH v7 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    | 180 ++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |   3 +
 3 files changed, 184 insertions(+)
 create mode 100644 s390x/topology.c

diff --git a/s390x/Makefile b/s390x/Makefile
index e94b720..05dac04 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -40,6 +40,7 @@ tests += $(TEST_DIR)/panic-loop-pgm.elf
 tests += $(TEST_DIR)/migration-sck.elf
 tests += $(TEST_DIR)/exittime.elf
 tests += $(TEST_DIR)/ex.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..ce248f1
--- /dev/null
+++ b/s390x/topology.c
@@ -0,0 +1,180 @@
+/* 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(
+		"	ptf	%1	\n"
+		"       ipm     %0	\n"
+		"       srl     %0,28	\n"
+		: "=d" (cc), "+d" (fc)
+		:
+		: "cc");
+
+	*rc = fc >> 8;
+	return cc;
+}
+
+static void check_privilege(int fc)
+{
+	unsigned long rc;
+
+	report_prefix_push("Privilege");
+	report_info("function code %d", fc);
+	enter_pstate();
+	expect_pgm_int();
+	ptf(fc, &rc);
+	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
+	report_prefix_pop();
+}
+
+static void check_function_code(void)
+{
+	unsigned long rc;
+
+	report_prefix_push("Undefined fc");
+	expect_pgm_int();
+	ptf(0xff, &rc);
+	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+	report_prefix_pop();
+}
+
+static void check_reserved_bits(void)
+{
+	unsigned long rc;
+
+	report_prefix_push("Reserved bits");
+	expect_pgm_int();
+	ptf(0xffffffffffffff00UL, &rc);
+	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+	report_prefix_pop();
+}
+
+static void check_mtcr_pending(void)
+{
+	unsigned long rc;
+	int cc;
+
+	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();
+}
+
+static void check_polarization_change(void)
+{
+	unsigned long rc;
+	int cc;
+
+	report_prefix_push("Topology polarization check");
+
+	/* We expect a clean state through reset */
+	report(diag308_load_reset(1), "load normal reset done");
+
+	/*
+	 * 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.");
+
+	report_prefix_pop();
+}
+
+static void test_ptf(void)
+{
+	check_privilege(PTF_REQ_HORIZONTAL);
+	check_privilege(PTF_REQ_VERTICAL);
+	check_privilege(PTF_REQ_CHECK);
+	check_function_code();
+	check_reserved_bits();
+	check_mtcr_pending();
+	check_polarization_change();
+}
+
+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 453ee9c..d0ac683 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -233,3 +233,6 @@ extra_params = -append '--parallel'
 
 [execute]
 file = ex.elf
+
+[topology]
+file = topology.elf
-- 
2.31.1

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

[Claudio Imbrenda]

On Mon, 20 Mar 2023 09:56:41 +0100
Pierre Morel <pmorel@linux.ibm.com> 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    | 180 ++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg |   3 +
>  3 files changed, 184 insertions(+)
>  create mode 100644 s390x/topology.c
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index e94b720..05dac04 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -40,6 +40,7 @@ tests += $(TEST_DIR)/panic-loop-pgm.elf
>  tests += $(TEST_DIR)/migration-sck.elf
>  tests += $(TEST_DIR)/exittime.elf
>  tests += $(TEST_DIR)/ex.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..ce248f1
> --- /dev/null
> +++ b/s390x/topology.c
> @@ -0,0 +1,180 @@
> +/* 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(
> +		"	ptf	%1	\n"
> +		"       ipm     %0	\n"
> +		"       srl     %0,28	\n"
> +		: "=d" (cc), "+d" (fc)
> +		:
> +		: "cc");
> +
> +	*rc = fc >> 8;
> +	return cc;
> +}
> +
> +static void check_privilege(int fc)
> +{
> +	unsigned long rc;
> +
> +	report_prefix_push("Privilege");
> +	report_info("function code %d", fc);
> +	enter_pstate();
> +	expect_pgm_int();
> +	ptf(fc, &rc);
> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
> +	report_prefix_pop();
> +}
> +
> +static void check_function_code(void)
> +{
> +	unsigned long rc;
> +
> +	report_prefix_push("Undefined fc");
> +	expect_pgm_int();
> +	ptf(0xff, &rc);

please don't use magic numbers, add a new macro PTF_INVALID_FUNCTION
(or something like that)

> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +}
> +
> +static void check_reserved_bits(void)
> +{
> +	unsigned long rc;
> +
> +	report_prefix_push("Reserved bits");
> +	expect_pgm_int();
> +	ptf(0xffffffffffffff00UL, &rc);

I would like every single bit to be tested, since all of them are
required to be zero.

make a loop and test each, but please report success of failure only
once at the end. 
use a report_info in case of failure to indicate which bit failed

> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +}
> +
> +static void check_mtcr_pending(void)
> +{
> +	unsigned long rc;
> +	int cc;
> +
> +	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();
> +}
> +
> +static void check_polarization_change(void)
> +{
> +	unsigned long rc;
> +	int cc;
> +
> +	report_prefix_push("Topology polarization check");
> +
> +	/* We expect a clean state through reset */
> +	report(diag308_load_reset(1), "load normal reset done");
> +
> +	/*
> +	 * 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.");

either here or in a new block, test that setting vertical twice in
a row will also result in a cc == 2 && rc == PTF_ERR_ALRDY_POLARIZED

> +
> +	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.");

it cannot hurt to add here another check for pending reports

> +
> +	report_prefix_pop();
> +}
> +
> +static void test_ptf(void)
> +{
> +	check_privilege(PTF_REQ_HORIZONTAL);
> +	check_privilege(PTF_REQ_VERTICAL);
> +	check_privilege(PTF_REQ_CHECK);
> +	check_function_code();
> +	check_reserved_bits();
> +	check_mtcr_pending();
> +	check_polarization_change();
> +}
> +
> +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 453ee9c..d0ac683 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -233,3 +233,6 @@ extra_params = -append '--parallel'
>  
>  [execute]
>  file = ex.elf
> +
> +[topology]
> +file = topology.elf

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

[Pierre Morel]

On 3/23/23 16:45, Claudio Imbrenda wrote:
> On Mon, 20 Mar 2023 09:56:41 +0100
> Pierre Morel <pmorel@linux.ibm.com> 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    | 180 ++++++++++++++++++++++++++++++++++++++++++++
>>   s390x/unittests.cfg |   3 +
>>   3 files changed, 184 insertions(+)
>>   create mode 100644 s390x/topology.c
>>
>> diff --git a/s390x/Makefile b/s390x/Makefile
>> index e94b720..05dac04 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -40,6 +40,7 @@ tests += $(TEST_DIR)/panic-loop-pgm.elf
>>   tests += $(TEST_DIR)/migration-sck.elf
>>   tests += $(TEST_DIR)/exittime.elf
>>   tests += $(TEST_DIR)/ex.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..ce248f1
>> --- /dev/null
>> +++ b/s390x/topology.c
>> @@ -0,0 +1,180 @@
>> +/* 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(
>> +		"	ptf	%1	\n"
>> +		"       ipm     %0	\n"
>> +		"       srl     %0,28	\n"
>> +		: "=d" (cc), "+d" (fc)
>> +		:
>> +		: "cc");
>> +
>> +	*rc = fc >> 8;
>> +	return cc;
>> +}
>> +
>> +static void check_privilege(int fc)
>> +{
>> +	unsigned long rc;
>> +
>> +	report_prefix_push("Privilege");
>> +	report_info("function code %d", fc);
>> +	enter_pstate();
>> +	expect_pgm_int();
>> +	ptf(fc, &rc);
>> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
>> +	report_prefix_pop();
>> +}
>> +
>> +static void check_function_code(void)
>> +{
>> +	unsigned long rc;
>> +
>> +	report_prefix_push("Undefined fc");
>> +	expect_pgm_int();
>> +	ptf(0xff, &rc);
> please don't use magic numbers, add a new macro PTF_INVALID_FUNCTION
> (or something like that)

OK


>
>> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>> +	report_prefix_pop();
>> +}
>> +
>> +static void check_reserved_bits(void)
>> +{
>> +	unsigned long rc;
>> +
>> +	report_prefix_push("Reserved bits");
>> +	expect_pgm_int();
>> +	ptf(0xffffffffffffff00UL, &rc);
> I would like every single bit to be tested, since all of them are
> required to be zero.
>
> make a loop and test each, but please report success of failure only
> once at the end.
> use a report_info in case of failure to indicate which bit failed


OK


>
>> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>> +	report_prefix_pop();
>> +}
>> +
>> +static void check_mtcr_pending(void)
>> +{
>> +	unsigned long rc;
>> +	int cc;
>> +
>> +	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();
>> +}
>> +
>> +static void check_polarization_change(void)
>> +{
>> +	unsigned long rc;
>> +	int cc;
>> +
>> +	report_prefix_push("Topology polarization check");
>> +
>> +	/* We expect a clean state through reset */
>> +	report(diag308_load_reset(1), "load normal reset done");
>> +
>> +	/*
>> +	 * 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.");
> either here or in a new block, test that setting vertical twice in
> a row will also result in a cc == 2 && rc == PTF_ERR_ALRDY_POLARIZED


OK


>
>> +
>> +	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.");
> it cannot hurt to add here another check for pending reports


OK


Thanks for the comments,


Regards,

Pierre

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

[Nico Boehr]

Quoting Pierre Morel (2023-03-20 09:56:41)
[...]
> diff --git a/s390x/topology.c b/s390x/topology.c
> new file mode 100644
> index 0000000..ce248f1
> --- /dev/null
> +++ b/s390x/topology.c
> @@ -0,0 +1,180 @@
> +/* 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

These are all function codes, so how about we name these defines PTF_FC_...

and since PTF_REQ_CHECK doesn't request anything we should rename to PTF_FC_CHECK

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

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

[Pierre Morel]

On 3/24/23 11:11, Nico Boehr wrote:
> Quoting Pierre Morel (2023-03-20 09:56:41)
> [...]
>> diff --git a/s390x/topology.c b/s390x/topology.c
>> new file mode 100644
>> index 0000000..ce248f1
>> --- /dev/null
>> +++ b/s390x/topology.c
>> @@ -0,0 +1,180 @@
>> +/* 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
> These are all function codes, so how about we name these defines PTF_FC_...
>
> and since PTF_REQ_CHECK doesn't request anything we should rename to PTF_FC_CHECK

OK


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


Yes, thanks.

Regards,

Pierre

[kvm-unit-tests PATCH v7 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.c    |   5 +
 lib/s390x/sclp.h    |   4 +-
 lib/s390x/stsi.h    |  36 ++++++
 s390x/topology.c    | 291 ++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |   8 ++
 5 files changed, 343 insertions(+), 1 deletion(-)

diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
index 390fde7..9679332 100644
--- a/lib/s390x/sclp.c
+++ b/lib/s390x/sclp.c
@@ -238,3 +238,8 @@ uint64_t get_max_ram_size(void)
 {
 	return max_ram_size;
 }
+
+uint64_t sclp_get_stsi_mnest(void)
+{
+	return read_info->stsi_parm;
+}
diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
index 853529b..6a611bc 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 */
@@ -341,5 +342,6 @@ int sclp_service_call(unsigned int command, void *sccb);
 void sclp_memory_setup(void);
 uint64_t get_ram_size(void);
 uint64_t get_max_ram_size(void);
+uint64_t sclp_get_stsi_mnest(void);
 
 #endif /* _S390X_SCLP_H_ */
diff --git a/lib/s390x/stsi.h b/lib/s390x/stsi.h
index bebc492..1351a6f 100644
--- a/lib/s390x/stsi.h
+++ b/lib/s390x/stsi.h
@@ -29,4 +29,40 @@ struct sysinfo_3_2_2 {
 	uint8_t ext_names[8][256];
 };
 
+#define CPUS_TLE_RES_BITS 0x00fffffff8000000UL
+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;
+};
+
+#define CONTAINER_TLE_RES_BITS 0x00ffffffffffff00UL
+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[];
+};
+
 #endif  /* _S390X_STSI_H_ */
diff --git a/s390x/topology.c b/s390x/topology.c
index ce248f1..11ce931 100644
--- a/s390x/topology.c
+++ b/s390x/topology.c
@@ -16,6 +16,20 @@
 #include <smp.h>
 #include <sclp.h>
 #include <s390x/hardware.h>
+#include <s390x/stsi.h>
+
+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;
+
+/*
+ * Topology level as defined by architecture, all levels exists with
+ * a single container unless overwritten by the QEMU -smp parameter.
+ */
+static int arch_topo_lvl[CPU_TOPOLOGY_MAX_LEVEL]; // = {1, 1, 1, 1, 1, 1};
 
 #define PTF_REQ_HORIZONTAL	0
 #define PTF_REQ_VERTICAL	1
@@ -147,11 +161,286 @@ static void test_ptf(void)
 	check_polarization_change();
 }
 
+/*
+ * 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;
+
+	for (i = 0, n = 1; i < CPU_TOPOLOGY_MAX_LEVEL; i++)
+		n *= info->mag[i] ?: 1;
+
+	report(n == max_cpus, "Calculated max CPUs: %d", n);
+}
+
+/*
+ * stsi_check_mag
+ * @info: Pointer to the stsi information
+ *
+ * MAG field should match the architecture defined containers
+ * when MNEST as returned by SCLP matches MNEST of the SYSIB.
+ */
+static void stsi_check_mag(struct sysinfo_15_1_x *info)
+{
+	int i;
+
+	report_prefix_push("MAG");
+
+	stsi_check_maxcpus(info);
+
+	/* Explicitly skip the test if both mnest do not match */
+	if (max_nested_lvl != info->mnest)
+		goto done;
+
+	/*
+	 * MAG up to max_nested_lvl must match the architecture
+	 * defined containers.
+	 */
+	for (i = 0; i < max_nested_lvl; i++)
+		report(info->mag[CPU_TOPOLOGY_MAX_LEVEL - i - 1] == arch_topo_lvl[i],
+		       "MAG %d field match %d == %d",
+		       i + 1,
+		       info->mag[CPU_TOPOLOGY_MAX_LEVEL - i - 1],
+		       arch_topo_lvl[i]);
+
+	/* Above max_nested_lvl the MAG field must be null */
+	for (; i < CPU_TOPOLOGY_MAX_LEVEL; i++)
+		report(info->mag[CPU_TOPOLOGY_MAX_LEVEL - i - 1] == 0,
+		       "MAG %d field match %d == %d", i + 1,
+		       info->mag[CPU_TOPOLOGY_MAX_LEVEL - i - 1], 0);
+
+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_core *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 <= arch_topo_lvl[0], "CPUs per mask: %d out of max %d",
+	       n, arch_topo_lvl[0]);
+	cpus_in_masks += n;
+
+	report(!cpus->d || (cpus->pp == 3 || cpus->pp == 0),
+	       "Dedication versus 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
+ * @sel2: the selector giving the topology level to check
+ *
+ * Fill the sysinfo_15_1_x info structure and check the
+ * SYSIB header.
+ *
+ * Returns instruction validity.
+ */
+static int stsi_get_sysib(struct sysinfo_15_1_x *info, int sel2)
+{
+	int ret;
+
+	report_prefix_pushf("SYSIB");
+
+	ret = stsi(pagebuf, 15, 1, sel2);
+
+	if (max_nested_lvl >= sel2) {
+		report(!ret, "Valid instruction");
+		report(sel2 == info->mnest, "Valid mnest");
+	} else {
+		report(ret, "Invalid instruction");
+	}
+
+	report_prefix_pop();
+
+	return ret;
+}
+
+/**
+ * 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("15_1_%d", sel2);
+
+	ret = stsi_get_sysib(info, sel2);
+	if (!ret) {
+		stsi_check_mag(info);
+		stsi_check_tle_coherency(info);
+	}
+
+	report_prefix_pop();
+}
+
+/*
+ * The Maximum Nested level is given by SCLP READ_SCP_INFO if the MNEST facility
+ * is available.
+ * If the MNEST facility is not available, sclp_get_stsi_mnest  returns 0 and the
+ * Maximum Nested level is 2
+ */
+#define S390_DEFAULT_MNEST	2
+static int sclp_get_mnest(void)
+{
+	return sclp_get_stsi_mnest() ?: S390_DEFAULT_MNEST;
+}
+
+static int arch_max_cpus(void)
+{
+	int i;
+	int ncpus = 1;
+
+	for (i = 0; i < CPU_TOPOLOGY_MAX_LEVEL; i++)
+		ncpus *= arch_topo_lvl[i] ?: 1;
+
+	return ncpus;
+}
+
+/**
+ * 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_cpus = arch_max_cpus();
+	report_info("Architecture max CPUs: %d", max_cpus);
+
+	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;
+	static const char * const levels[] = { "cores", "sockets",
+					       "books", "drawers" };
+
+	for (i = 1; i < argc; i++) {
+		char *flag = argv[i];
+		int level;
+
+		if (flag[0] != '-')
+			report_abort("Argument is expected to begin with '-'");
+		flag++;
+		for (level = 0; ARRAY_SIZE(levels); level++) {
+			if (!strcmp(levels[level], flag))
+				break;
+		}
+		if (level == ARRAY_SIZE(levels))
+			report_abort("Unknown parameter %s", flag);
+
+		arch_topo_lvl[level] = atol(argv[++i]);
+		report_info("%s: %d", levels[level], arch_topo_lvl[level]);
+	}
+}
+
 static struct {
 	const char *name;
 	void (*func)(void);
 } tests[] = {
 	{ "PTF", test_ptf},
+	{ "STSI", test_stsi},
 	{ NULL, NULL }
 };
 
@@ -161,6 +450,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 d0ac683..7285f60 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -236,3 +236,11 @@ file = ex.elf
 
 [topology]
 file = topology.elf
+# 3 CPUs on socket 0 with different CPU TLE (standard, dedicated, origin)
+# 1 CPU on socket 2
+extra_params = -smp 1,drawers=3,books=3,sockets=4,cores=4,maxcpus=144 -cpu z14,ctop=on -device z14-s390x-cpu,core-id=1 -device z14-s390x-cpu,core-id=2,dedicated=on -device z14-s390x-cpu,core-id=10 -device z14-s390x-cpu,core-id=20 -device z14-s390x-cpu,core-id=130,socket-id=0,book-id=0,drawer-id=0 -append '-drawers 3 -books 3 -sockets 4 -cores 4'
+
+[topology_max]
+# define 240 CPUs
+file = topology.elf
+extra_params = -smp drawers=3,books=4,sockets=5,cores=4,maxcpus=240 -cpu z14,ctop=on -append '-drawers 3 -books 4 -sockets 5 -cores 4'
-- 
2.31.1

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

[Nico Boehr]

Quoting Pierre Morel (2023-03-20 09:56:42)
> STSI with function code 15 is used to store the CPU configuration
> topology.
> 
> We retrieve the maximum nested level with SCLP and use the
> topology tree provided by the drawers, books, sockets, cores
> arguments.
> 
> We check :
> - if the topology stored is coherent between the QEMU -smp
>   parameters and kernel parameters.
> - the number of CPUs
> - the maximum number of CPUs
> - the number of containers of each levels for every STSI(15.1.x)
>   instruction allowed by the machine.
[...]
> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
> index 390fde7..9679332 100644
> --- a/lib/s390x/sclp.c
> +++ b/lib/s390x/sclp.c
> @@ -238,3 +238,8 @@ uint64_t get_max_ram_size(void)
>  {
>         return max_ram_size;
>  }
> +
> +uint64_t sclp_get_stsi_mnest(void)
> +{

maybe add:
assert(read_info);

[...]
> diff --git a/s390x/topology.c b/s390x/topology.c
> index ce248f1..11ce931 100644
> --- a/s390x/topology.c
> +++ b/s390x/topology.c
[...]
> +/*
> + * Topology level as defined by architecture, all levels exists with
> + * a single container unless overwritten by the QEMU -smp parameter.
> + */
> +static int arch_topo_lvl[CPU_TOPOLOGY_MAX_LEVEL]; // = {1, 1, 1, 1, 1, 1};

So that's what is being provided to the test on the command line, right?

How about renaming this to expected_topo_lvl?

What do you mean by 'defined by architecture'?

[...]
> +/*
> + * stsi_check_mag
> + * @info: Pointer to the stsi information
> + *
> + * MAG field should match the architecture defined containers
> + * when MNEST as returned by SCLP matches MNEST of the SYSIB.
> + */
> +static void stsi_check_mag(struct sysinfo_15_1_x *info)
> +{
> +       int i;
> +
> +       report_prefix_push("MAG");
> +
> +       stsi_check_maxcpus(info);
> +
> +       /* Explicitly skip the test if both mnest do not match */
> +       if (max_nested_lvl != info->mnest)
> +               goto done;

What does it mean if the two don't match, i.e. is this an error? Or a skip? Or is it just expected?

[...]
> +/**
> + * check_tle:
> + * @tc: pointer to first TLE
> + *
> + * Recursively check the containers TLEs until we
> + * find a CPU TLE.
> + */
> +static uint8_t *check_tle(void *tc)
> +{
[...]
> +       report(!cpus->d || (cpus->pp == 3 || cpus->pp == 0),
> +              "Dedication versus entitlement");

Maybe skip here if the CPU is not dedicated? With shared CPUs we really can't check much here.

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

If arch_topo_lvl is renamed, also rename this function accordingly.

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

missing space                ^

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

[Pierre Morel]

On 3/24/23 11:59, Nico Boehr wrote:
> Quoting Pierre Morel (2023-03-20 09:56:42)
>> STSI with function code 15 is used to store the CPU configuration
>> topology.
>>
>> We retrieve the maximum nested level with SCLP and use the
>> topology tree provided by the drawers, books, sockets, cores
>> arguments.
>>
>> We check :
>> - if the topology stored is coherent between the QEMU -smp
>>    parameters and kernel parameters.
>> - the number of CPUs
>> - the maximum number of CPUs
>> - the number of containers of each levels for every STSI(15.1.x)
>>    instruction allowed by the machine.
> [...]
>> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
>> index 390fde7..9679332 100644
>> --- a/lib/s390x/sclp.c
>> +++ b/lib/s390x/sclp.c
>> @@ -238,3 +238,8 @@ uint64_t get_max_ram_size(void)
>>   {
>>          return max_ram_size;
>>   }
>> +
>> +uint64_t sclp_get_stsi_mnest(void)
>> +{
> maybe add:
> assert(read_info);

right


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

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

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

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

OK for expected_topo_lvl if you prefer.


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

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

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

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

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


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


OK


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

OK


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


Right, thanks,


regards,

Pierre

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

[Nico Boehr]

Quoting Pierre Morel (2023-03-27 14:38:35)
> > [...]
> >> diff --git a/s390x/topology.c b/s390x/topology.c
> >> index ce248f1..11ce931 100644
> >> --- a/s390x/topology.c
> >> +++ b/s390x/topology.c
> > [...]
> >> +/*
> >> + * Topology level as defined by architecture, all levels exists with
> >> + * a single container unless overwritten by the QEMU -smp parameter.
> >> + */
> >> +static int arch_topo_lvl[CPU_TOPOLOGY_MAX_LEVEL]; // = {1, 1, 1, 1, 1, 1};
> > So that's what is being provided to the test on the command line, right?
> >
> > How about renaming this to expected_topo_lvl?
> >
> > What do you mean by 'defined by architecture'?
> 
> This is what is provided by the boot arguments and should correspond to 
> the physical topology.
> 
> The test checks that this is corresponding to what LPAR or QEMU shows in 
> the SYSIB.

Yep, OK. Makes sense.

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

What do you mean by 'implicitly unique'?

> OK for expected_topo_lvl if you prefer.

Yes, please.

> > [...]
> >> +/*
> >> + * stsi_check_mag
> >> + * @info: Pointer to the stsi information
> >> + *
> >> + * MAG field should match the architecture defined containers
> >> + * when MNEST as returned by SCLP matches MNEST of the SYSIB.
> >> + */
> >> +static void stsi_check_mag(struct sysinfo_15_1_x *info)
> >> +{
> >> +       int i;
> >> +
> >> +       report_prefix_push("MAG");
> >> +
> >> +       stsi_check_maxcpus(info);
> >> +
> >> +       /* Explicitly skip the test if both mnest do not match */
> >> +       if (max_nested_lvl != info->mnest)
> >> +               goto done;
> > What does it mean if the two don't match, i.e. is this an error? Or a skip? Or is it just expected?
> 
> I have no information on the representation of the MAG fields for a 
> SYSIB with a nested level different than the maximum nested level.
> 
> There are examples in the documentation but I did not find, and did not 
> get a clear answer, on how the MAG field are calculated.
> 
> The examples seems clear for info->mnest between MNEST -1 and 3 but the 
> explication I had on info->mnest = 2 is not to be found in any 
> documentation.
> 
> Until it is specified in a documentation I skip all these tests.

Alright - then please:
- update the comment to say:
  "It is not clear how the MAG fields are calculated when mnest in the SYSIB 15.x is different from the maximum nested level in the SCLP info, so we skip here for now."
- when this is the case, do a report_skip() and show info->mnest and max_nested_lvl in the message.

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

[Pierre Morel]

On 3/28/23 08:25, Nico Boehr wrote:
> Quoting Pierre Morel (2023-03-27 14:38:35)
>>> [...]


[...]


>> If a topology level always exist physically and if it is not specified
>> on the QEMU command line it is implicitly unique.
> What do you mean by 'implicitly unique'?

I mean that if the topology level is not explicitly specified on the 
command line, it exists a single entity of this topology level.


>
>> OK for expected_topo_lvl if you prefer.
> Yes, please.
>
>>> [...]
>>>> +/*
>>>> + * stsi_check_mag
>>>> + * @info: Pointer to the stsi information
>>>> + *
>>>> + * MAG field should match the architecture defined containers
>>>> + * when MNEST as returned by SCLP matches MNEST of the SYSIB.
>>>> + */
>>>> +static void stsi_check_mag(struct sysinfo_15_1_x *info)
>>>> +{
>>>> +       int i;
>>>> +
>>>> +       report_prefix_push("MAG");
>>>> +
>>>> +       stsi_check_maxcpus(info);
>>>> +
>>>> +       /* Explicitly skip the test if both mnest do not match */
>>>> +       if (max_nested_lvl != info->mnest)
>>>> +               goto done;
>>> What does it mean if the two don't match, i.e. is this an error? Or a skip? Or is it just expected?
>> I have no information on the representation of the MAG fields for a
>> SYSIB with a nested level different than the maximum nested level.
>>
>> There are examples in the documentation but I did not find, and did not
>> get a clear answer, on how the MAG field are calculated.
>>
>> The examples seems clear for info->mnest between MNEST -1 and 3 but the
>> explication I had on info->mnest = 2 is not to be found in any
>> documentation.
>>
>> Until it is specified in a documentation I skip all these tests.
> Alright - then please:
> - update the comment to say:
>    "It is not clear how the MAG fields are calculated when mnest in the SYSIB 15.x is different from the maximum nested level in the SCLP info, so we skip here for now."
> - when this is the case, do a report_skip() and show info->mnest and max_nested_lvl in the message.


OK, thanks.

Regards,

Pierre

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

[Nico Boehr]

Quoting Pierre Morel (2023-03-28 13:37:59)
> 
> On 3/28/23 08:25, Nico Boehr wrote:
> > Quoting Pierre Morel (2023-03-27 14:38:35)
> >>> [...]
> 
> 
> [...]
> 
> 
> >> If a topology level always exist physically and if it is not specified
> >> on the QEMU command line it is implicitly unique.
> > What do you mean by 'implicitly unique'?
> 
> I mean that if the topology level is not explicitly specified on the 
> command line, it exists a single entity of this topology level.

OK, makes sense! Thanks!

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

[Pierre Morel]

On 3/24/23 11:59, Nico Boehr wrote:
> Quoting Pierre Morel (2023-03-20 09:56:42)

[...]


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

Thinking more about this I think I will not change this but I better will
make two check one for horizontal and once for vertical polarization.