[kvm-unit-tests PATCH v3 0/2] Add specification exception tests

[kvm-unit-tests PATCH v3 0/2] Add specification exception tests

[Janis Schoetterl-Glausch]

Test that specification exceptions cause the correct interruption code
during both normal and transactional execution.

v2 -> v3
	remove non-ascii symbol
	clean up load_psw
	fix nits

v1 -> v2
	Add license and test description
	Split test patch into normal test and transactional execution test
	Add comments to
		invalid PSW fixup function
		with_transaction
	Rename some variables/functions
	Pass mask as single parameter to asm
	Get rid of report_info_if macro
	Introduce report_pass/fail and use them
Janis Schoetterl-Glausch (2):
  s390x: Add specification exception test
  s390x: Test specification exceptions during transaction

 s390x/Makefile           |   1 +
 lib/s390x/asm/arch_def.h |   1 +
 s390x/spec_ex.c          | 343 +++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg      |   3 +
 4 files changed, 348 insertions(+)
 create mode 100644 s390x/spec_ex.c


base-commit: 3ac97f8fc847d05d0a5555aefd34e2cac26fdc0c
-- 
2.31.1

[kvm-unit-tests PATCH v3 1/2] s390x: Add specification exception test

[Janis Schoetterl-Glausch]

Generate specification exceptions and check that they occur.
With the iterations argument one can check if specification
exception interpretation occurs, e.g. by using a high value and
checking that the debugfs counters are substantially lower.
The argument is also useful for estimating the performance benefit
of interpretation.

Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
 s390x/Makefile      |   1 +
 s390x/spec_ex.c     | 181 ++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |   3 +
 3 files changed, 185 insertions(+)
 create mode 100644 s390x/spec_ex.c

diff --git a/s390x/Makefile b/s390x/Makefile
index d18b08b..3e42784 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -24,6 +24,7 @@ tests += $(TEST_DIR)/mvpg.elf
 tests += $(TEST_DIR)/uv-host.elf
 tests += $(TEST_DIR)/edat.elf
 tests += $(TEST_DIR)/mvpg-sie.elf
+tests += $(TEST_DIR)/spec_ex.elf
 
 tests_binary = $(patsubst %.elf,%.bin,$(tests))
 ifneq ($(HOST_KEY_DOCUMENT),)
diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
new file mode 100644
index 0000000..ec3322a
--- /dev/null
+++ b/s390x/spec_ex.c
@@ -0,0 +1,181 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright IBM Corp. 2021
+ *
+ * Specification exception test.
+ * Tests that specification exceptions occur when expected.
+ */
+#include <stdlib.h>
+#include <libcflat.h>
+#include <asm/interrupt.h>
+#include <asm/facility.h>
+
+static struct lowcore *lc = (struct lowcore *) 0;
+
+static bool expect_invalid_psw;
+static struct psw expected_psw;
+static struct psw fixup_psw;
+
+/* The standard program exception handler cannot deal with invalid old PSWs,
+ * especially not invalid instruction addresses, as in that case one cannot
+ * find the instruction following the faulting one from the old PSW.
+ * The PSW to return to is set by load_psw.
+ */
+static void fixup_invalid_psw(void)
+{
+	if (expect_invalid_psw) {
+		report(expected_psw.mask == lc->pgm_old_psw.mask
+		       && expected_psw.addr == lc->pgm_old_psw.addr,
+		       "Invalid program new PSW as expected");
+		expect_invalid_psw = false;
+	}
+	lc->pgm_old_psw = fixup_psw;
+}
+
+/* Load possibly invalid psw, but setup fixup_psw before,
+ * so that *fixup_invalid_psw() can bring us back onto the right track.
+ */
+static void load_psw(struct psw psw)
+{
+	uint64_t scratch;
+
+	fixup_psw.mask = extract_psw_mask();
+	asm volatile (
+		"	larl	%[scratch],nop%=\n"
+		"	stg	%[scratch],%[addr]\n"
+		"	lpswe	%[psw]\n"
+		"nop%=:	nop\n"
+		: [scratch] "=&r"(scratch),
+		  [addr] "=&T"(fixup_psw.addr)
+		: [psw] "Q"(psw)
+		: "cc", "memory"
+	);
+}
+
+static void psw_bit_12_is_1(void)
+{
+	expected_psw.mask = 0x0008000000000000;
+	expected_psw.addr = 0x00000000deadbeee;
+	expect_invalid_psw = true;
+	load_psw(expected_psw);
+}
+
+static void bad_alignment(void)
+{
+	uint32_t words[5] = {0, 0, 0};
+	uint32_t (*bad_aligned)[4];
+
+	register uint64_t r1 asm("6");
+	register uint64_t r2 asm("7");
+	if (((uintptr_t)&words[0]) & 0xf)
+		bad_aligned = (uint32_t (*)[4])&words[0];
+	else
+		bad_aligned = (uint32_t (*)[4])&words[1];
+	asm volatile ("lpq %0,%2"
+		      : "=r"(r1), "=r"(r2)
+		      : "T"(*bad_aligned)
+	);
+}
+
+static void not_even(void)
+{
+	uint64_t quad[2];
+
+	register uint64_t r1 asm("7");
+	register uint64_t r2 asm("8");
+	asm volatile (".insn	rxy,0xe3000000008f,%0,%2" //lpq %0,%2
+		      : "=r"(r1), "=r"(r2)
+		      : "T"(quad)
+	);
+}
+
+struct spec_ex_trigger {
+	const char *name;
+	void (*func)(void);
+	void (*fixup)(void);
+};
+
+static const struct spec_ex_trigger spec_ex_triggers[] = {
+	{ "psw_bit_12_is_1", &psw_bit_12_is_1, &fixup_invalid_psw},
+	{ "bad_alignment", &bad_alignment, NULL},
+	{ "not_even", &not_even, NULL},
+	{ NULL, NULL, NULL},
+};
+
+struct args {
+	uint64_t iterations;
+};
+
+static void test_spec_ex(struct args *args,
+			 const struct spec_ex_trigger *trigger)
+{
+	uint16_t expected_pgm = PGM_INT_CODE_SPECIFICATION;
+	uint16_t pgm;
+	unsigned int i;
+
+	for (i = 0; i < args->iterations; i++) {
+		expect_pgm_int();
+		register_pgm_cleanup_func(trigger->fixup);
+		trigger->func();
+		register_pgm_cleanup_func(NULL);
+		pgm = clear_pgm_int();
+		if (pgm != expected_pgm) {
+			report_fail("Program interrupt: expected(%d) == received(%d)",
+				    expected_pgm,
+				    pgm);
+			return;
+		}
+	}
+	report_pass("Program interrupt: always expected(%d) == received(%d)",
+		    expected_pgm,
+		    expected_pgm);
+}
+
+static struct args parse_args(int argc, char **argv)
+{
+	struct args args = {
+		.iterations = 1,
+	};
+	unsigned int i;
+	long arg;
+	bool no_arg;
+	char *end;
+
+	for (i = 1; i < argc; i++) {
+		no_arg = true;
+		if (i < argc - 1) {
+			no_arg = *argv[i + 1] == '\0';
+			arg = strtol(argv[i + 1], &end, 10);
+			no_arg |= *end != '\0';
+			no_arg |= arg < 0;
+		}
+
+		if (!strcmp("--iterations", argv[i])) {
+			if (no_arg)
+				report_abort("--iterations needs a positive parameter");
+			args.iterations = arg;
+			++i;
+		} else {
+			report_abort("Unsupported parameter '%s'",
+				     argv[i]);
+		}
+	}
+	return args;
+}
+
+int main(int argc, char **argv)
+{
+	unsigned int i;
+
+	struct args args = parse_args(argc, argv);
+
+	report_prefix_push("specification exception");
+	for (i = 0; spec_ex_triggers[i].name; i++) {
+		report_prefix_push(spec_ex_triggers[i].name);
+		test_spec_ex(&args, &spec_ex_triggers[i]);
+		report_prefix_pop();
+	}
+	report_prefix_pop();
+
+	return report_summary();
+}
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index 9e1802f..5f43d52 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -109,3 +109,6 @@ file = edat.elf
 
 [mvpg-sie]
 file = mvpg-sie.elf
+
+[spec_ex]
+file = spec_ex.elf
-- 
2.31.1

[kvm-unit-tests PATCH v3 2/2] s390x: Test specification exceptions during transaction

[Janis Schoetterl-Glausch]

Program interruptions during transactional execution cause other
interruption codes.
Check that we see the expected code for (some) specification exceptions.

Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
 lib/s390x/asm/arch_def.h |   1 +
 s390x/spec_ex.c          | 172 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 168 insertions(+), 5 deletions(-)

diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index 40626d7..f7fb467 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -55,6 +55,7 @@ struct psw {
 #define PSW_MASK_BA			0x0000000080000000UL
 #define PSW_MASK_64			(PSW_MASK_BA | PSW_MASK_EA)
 
+#define CTL0_TRANSACT_EX_CTL		(63 -  8)
 #define CTL0_LOW_ADDR_PROT		(63 - 35)
 #define CTL0_EDAT			(63 - 40)
 #define CTL0_IEP			(63 - 43)
diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
index ec3322a..f3628bd 100644
--- a/s390x/spec_ex.c
+++ b/s390x/spec_ex.c
@@ -4,9 +4,14 @@
  *
  * Specification exception test.
  * Tests that specification exceptions occur when expected.
+ * This includes specification exceptions occurring during transactional execution
+ * as these result in another interruption code (the transactional-execution-aborted
+ * bit is set).
  */
 #include <stdlib.h>
+#include <htmintrin.h>
 #include <libcflat.h>
+#include <asm/barrier.h>
 #include <asm/interrupt.h>
 #include <asm/facility.h>
 
@@ -92,18 +97,23 @@ static void not_even(void)
 struct spec_ex_trigger {
 	const char *name;
 	void (*func)(void);
+	bool transactable;
 	void (*fixup)(void);
 };
 
 static const struct spec_ex_trigger spec_ex_triggers[] = {
-	{ "psw_bit_12_is_1", &psw_bit_12_is_1, &fixup_invalid_psw},
-	{ "bad_alignment", &bad_alignment, NULL},
-	{ "not_even", &not_even, NULL},
-	{ NULL, NULL, NULL},
+	{ "psw_bit_12_is_1", &psw_bit_12_is_1, false, &fixup_invalid_psw},
+	{ "bad_alignment", &bad_alignment, true, NULL},
+	{ "not_even", &not_even, true, NULL},
+	{ NULL, NULL, true, NULL},
 };
 
 struct args {
 	uint64_t iterations;
+	uint64_t max_retries;
+	uint64_t suppress_info;
+	uint64_t max_failures;
+	bool diagnose;
 };
 
 static void test_spec_ex(struct args *args,
@@ -131,14 +141,132 @@ static void test_spec_ex(struct args *args,
 		    expected_pgm);
 }
 
+#define TRANSACTION_COMPLETED 4
+#define TRANSACTION_MAX_RETRIES 5
+
+/* NULL must be passed to __builtin_tbegin via constant, forbid diagnose from
+ * being NULL to keep things simple
+ */
+static int __attribute__((nonnull))
+with_transaction(void (*trigger)(void), struct __htm_tdb *diagnose)
+{
+	int cc;
+
+	cc = __builtin_tbegin(diagnose);
+	if (cc == _HTM_TBEGIN_STARTED) {
+		trigger();
+		__builtin_tend();
+		return -TRANSACTION_COMPLETED;
+	} else {
+		return -cc;
+	}
+}
+
+static int retry_transaction(const struct spec_ex_trigger *trigger, unsigned int max_retries,
+			     struct __htm_tdb *tdb, uint16_t expected_pgm)
+{
+	int trans_result, i;
+	uint16_t pgm;
+
+	for (i = 0; i < max_retries; i++) {
+		expect_pgm_int();
+		trans_result = with_transaction(trigger->func, tdb);
+		if (trans_result == -_HTM_TBEGIN_TRANSIENT) {
+			mb();
+			pgm = lc->pgm_int_code;
+			if (pgm == 0)
+				continue;
+			else if (pgm == expected_pgm)
+				return 0;
+		}
+		return trans_result;
+	}
+	return -TRANSACTION_MAX_RETRIES;
+}
+
+static void test_spec_ex_trans(struct args *args, const struct spec_ex_trigger *trigger)
+{
+	const uint16_t expected_pgm = PGM_INT_CODE_SPECIFICATION
+			      | PGM_INT_CODE_TX_ABORTED_EVENT;
+	union {
+		struct __htm_tdb tdb;
+		uint64_t dwords[sizeof(struct __htm_tdb) / sizeof(uint64_t)];
+	} diag;
+	unsigned int i, failures = 0;
+	int trans_result;
+
+	if (!test_facility(73)) {
+		report_skip("transactional-execution facility not installed");
+		return;
+	}
+	ctl_set_bit(0, CTL0_TRANSACT_EX_CTL); /* enable transactional-exec */
+
+	for (i = 0; i < args->iterations && failures <= args->max_failures; i++) {
+		register_pgm_cleanup_func(trigger->fixup);
+		trans_result = retry_transaction(trigger, args->max_retries, &diag.tdb, expected_pgm);
+		register_pgm_cleanup_func(NULL);
+		switch (trans_result) {
+		case 0:
+			continue;
+		case -_HTM_TBEGIN_INDETERMINATE:
+		case -_HTM_TBEGIN_PERSISTENT:
+			if (failures < args->suppress_info)
+				report_info("transaction failed with cc %d",
+					    -trans_result);
+			break;
+		case -_HTM_TBEGIN_TRANSIENT:
+			report_fail("Program interrupt: expected(%d) == received(%d)",
+				    expected_pgm,
+				    clear_pgm_int());
+			goto out;
+		case -TRANSACTION_COMPLETED:
+			report_fail("Transaction completed without exception");
+			goto out;
+		case -TRANSACTION_MAX_RETRIES:
+			if (failures < args->suppress_info)
+				report_info("Retried transaction %lu times without exception",
+					    args->max_retries);
+			break;
+		default:
+			report_fail("Invalid return transaction result");
+			goto out;
+		}
+
+		if (failures < args->suppress_info)
+			report_info("transaction abort code: %llu", diag.tdb.abort_code);
+		if (args->diagnose && failures < args->suppress_info) {
+			for (i = 0; i < 32; i++)
+				report_info("diag+%03d: %016lx", i*8, diag.dwords[i]);
+		}
+		++failures;
+	}
+	if (failures <= args->max_failures) {
+		report_pass(
+			"Program interrupt: always expected(%d) == received(%d), transaction failures: %u",
+			expected_pgm,
+			expected_pgm,
+			failures);
+	} else {
+		report_fail("Too many transaction failures: %u", failures);
+	}
+	if (failures > args->suppress_info)
+		report_info("Suppressed some transaction failure information messages");
+
+out:
+	ctl_clear_bit(0, CTL0_TRANSACT_EX_CTL);
+}
+
 static struct args parse_args(int argc, char **argv)
 {
 	struct args args = {
 		.iterations = 1,
+		.max_retries = 20,
+		.suppress_info = 20,
+		.diagnose = false
 	};
 	unsigned int i;
 	long arg;
-	bool no_arg;
+	bool no_arg, max_failures = false;
 	char *end;
 
 	for (i = 1; i < argc; i++) {
@@ -155,11 +283,35 @@ static struct args parse_args(int argc, char **argv)
 				report_abort("--iterations needs a positive parameter");
 			args.iterations = arg;
 			++i;
+		} else if (!strcmp("--max-retries", argv[i])) {
+			if (no_arg)
+				report_abort("--max-retries needs a positive parameter");
+			args.max_retries = arg;
+			++i;
+		} else if (!strcmp("--suppress-info", argv[i])) {
+			if (no_arg)
+				report_abort("--suppress-info needs a positive parameter");
+			args.suppress_info = arg;
+			++i;
+		} else if (!strcmp("--max-failures", argv[i])) {
+			if (no_arg)
+				report_abort("--max-failures needs a positive parameter");
+			args.max_failures = arg;
+			max_failures = true;
+			++i;
+		} else if (!strcmp("--diagnose", argv[i])) {
+			args.diagnose = true;
+		} else if (!strcmp("--no-diagnose", argv[i])) {
+			args.diagnose = false;
 		} else {
 			report_abort("Unsupported parameter '%s'",
 				     argv[i]);
 		}
 	}
+
+	if (!max_failures)
+		args.max_failures = args.iterations / 1000;
+
 	return args;
 }
 
@@ -177,5 +329,15 @@ int main(int argc, char **argv)
 	}
 	report_prefix_pop();
 
+	report_prefix_push("specification exception during transaction");
+	for (i = 0; spec_ex_triggers[i].name; i++) {
+		if (spec_ex_triggers[i].transactable) {
+			report_prefix_push(spec_ex_triggers[i].name);
+			test_spec_ex_trans(&args, &spec_ex_triggers[i]);
+			report_prefix_pop();
+		}
+	}
+	report_prefix_pop();
+
 	return report_summary();
 }
-- 
2.31.1

Re: [kvm-unit-tests PATCH v3 1/2] s390x: Add specification exception test

[Claudio Imbrenda]

On Fri, 22 Oct 2021 14:01:55 +0200
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

> Generate specification exceptions and check that they occur.
> With the iterations argument one can check if specification
> exception interpretation occurs, e.g. by using a high value and
> checking that the debugfs counters are substantially lower.
> The argument is also useful for estimating the performance benefit
> of interpretation.
> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
>  s390x/Makefile      |   1 +
>  s390x/spec_ex.c     | 181 ++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg |   3 +
>  3 files changed, 185 insertions(+)
>  create mode 100644 s390x/spec_ex.c
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index d18b08b..3e42784 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -24,6 +24,7 @@ tests += $(TEST_DIR)/mvpg.elf
>  tests += $(TEST_DIR)/uv-host.elf
>  tests += $(TEST_DIR)/edat.elf
>  tests += $(TEST_DIR)/mvpg-sie.elf
> +tests += $(TEST_DIR)/spec_ex.elf
>  
>  tests_binary = $(patsubst %.elf,%.bin,$(tests))
>  ifneq ($(HOST_KEY_DOCUMENT),)
> diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
> new file mode 100644
> index 0000000..ec3322a
> --- /dev/null
> +++ b/s390x/spec_ex.c
> @@ -0,0 +1,181 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright IBM Corp. 2021
> + *
> + * Specification exception test.
> + * Tests that specification exceptions occur when expected.
> + */
> +#include <stdlib.h>
> +#include <libcflat.h>
> +#include <asm/interrupt.h>
> +#include <asm/facility.h>
> +
> +static struct lowcore *lc = (struct lowcore *) 0;
> +
> +static bool expect_invalid_psw;
> +static struct psw expected_psw;
> +static struct psw fixup_psw;
> +
> +/* The standard program exception handler cannot deal with invalid old PSWs,
> + * especially not invalid instruction addresses, as in that case one cannot
> + * find the instruction following the faulting one from the old PSW.
> + * The PSW to return to is set by load_psw.
> + */
> +static void fixup_invalid_psw(void)
> +{
> +	if (expect_invalid_psw) {
> +		report(expected_psw.mask == lc->pgm_old_psw.mask
> +		       && expected_psw.addr == lc->pgm_old_psw.addr,
> +		       "Invalid program new PSW as expected");
> +		expect_invalid_psw = false;

can you find a way to call report() where the test is
triggered (psw_bit_12_is_1), instead of burying it here?

maybe instead of calling report you can set a flag like
"expected_psw_found" and then call report on it?

> +	}
> +	lc->pgm_old_psw = fixup_psw;
> +}
> +
> +/* Load possibly invalid psw, but setup fixup_psw before,
> + * so that *fixup_invalid_psw() can bring us back onto the right track.
> + */
> +static void load_psw(struct psw psw)
> +{
> +	uint64_t scratch;
> +

I understand why you are doing this, but I wonder if there is a "nicer"
way to do it. What happens if you chose a nicer and unique name for the
label and make it global?

> +	fixup_psw.mask = extract_psw_mask();

then you could add this here:
	fixup_psw.addr = after_lpswe;

> +	asm volatile (
> +		"	larl	%[scratch],nop%=\n"
> +		"	stg	%[scratch],%[addr]\n"
	^ those two lines are no longer needed ^
> +		"	lpswe	%[psw]\n"
> +		"nop%=:	nop\n"
	".global after_lpswe \n"
	"after_lpswe:	nop"
> +		: [scratch] "=&r"(scratch),
> +		  [addr] "=&T"(fixup_psw.addr)
> +		: [psw] "Q"(psw)
> +		: "cc", "memory"
> +	);
> +}
> +
> +static void psw_bit_12_is_1(void)
> +{
> +	expected_psw.mask = 0x0008000000000000;
> +	expected_psw.addr = 0x00000000deadbeee;
> +	expect_invalid_psw = true;
> +	load_psw(expected_psw);

and here something like
	report(expected_psw_found, "blah blah blah");

> +}
> +
> +static void bad_alignment(void)
> +{
> +	uint32_t words[5] = {0, 0, 0};
> +	uint32_t (*bad_aligned)[4];
> +
> +	register uint64_t r1 asm("6");
> +	register uint64_t r2 asm("7");
> +	if (((uintptr_t)&words[0]) & 0xf)
> +		bad_aligned = (uint32_t (*)[4])&words[0];
> +	else
> +		bad_aligned = (uint32_t (*)[4])&words[1];

this is a lot of work... can't you just declare it like:

	uint32_t words[5] __attribute__((aligned(16)));
and then just use
	(words + 1) ?

> +	asm volatile ("lpq %0,%2"
> +		      : "=r"(r1), "=r"(r2)

since you're ignoring the return value, can't you hardcode r6, and mark
it (and r7) as clobbered? like:
		"lpq 6, %[bad]"
		: : [bad] "T"(words[1])
		: "%r6", "%r7" 

> +		      : "T"(*bad_aligned)
> +	);
> +}
> +
> +static void not_even(void)
> +{
> +	uint64_t quad[2];
> +
> +	register uint64_t r1 asm("7");
> +	register uint64_t r2 asm("8");
> +	asm volatile (".insn	rxy,0xe3000000008f,%0,%2" //lpq
> %0,%2

this is even uglier. I guess you had already tried this?

		"lpq 7, %[good]"
			: : [good] "T"(quad)
			: "%r7", "%r8"

if that doesn't work, then the same but with .insn

> +		      : "=r"(r1), "=r"(r2)
> +		      : "T"(quad)
> +	);
> +}
> +
> +struct spec_ex_trigger {
> +	const char *name;
> +	void (*func)(void);
> +	void (*fixup)(void);
> +};
> +
> +static const struct spec_ex_trigger spec_ex_triggers[] = {
> +	{ "psw_bit_12_is_1", &psw_bit_12_is_1, &fixup_invalid_psw},
> +	{ "bad_alignment", &bad_alignment, NULL},
> +	{ "not_even", &not_even, NULL},
> +	{ NULL, NULL, NULL},
> +};
> +

this is a lot of infrastructure for 3 tests... (or even for 5 tests,
since you will add the transactions in the next patch)

are you planning to significantly extend this test in the future?

> +struct args {
> +	uint64_t iterations;
> +};
> +
> +static void test_spec_ex(struct args *args,
> +			 const struct spec_ex_trigger *trigger)
> +{
> +	uint16_t expected_pgm = PGM_INT_CODE_SPECIFICATION;
> +	uint16_t pgm;
> +	unsigned int i;
> +
> +	for (i = 0; i < args->iterations; i++) {
> +		expect_pgm_int();
> +		register_pgm_cleanup_func(trigger->fixup);
> +		trigger->func();
> +		register_pgm_cleanup_func(NULL);
> +		pgm = clear_pgm_int();
> +		if (pgm != expected_pgm) {
> +			report_fail("Program interrupt: expected(%d)
> == received(%d)",
> +				    expected_pgm,
> +				    pgm);
> +			return;
> +		}
> +	}
> +	report_pass("Program interrupt: always expected(%d) ==
> received(%d)",
> +		    expected_pgm,
> +		    expected_pgm);
> +}
> +
> +static struct args parse_args(int argc, char **argv)

do we _really_ need commandline arguments?

is it really so important to be able to control these parameters?

can you find some values for the parameters so that the test works (as
in, it actually tests what it's supposed to) and also so that the whole
unit test ends in less than 30 seconds?

> +{
> +	struct args args = {
> +		.iterations = 1,
> +	};
> +	unsigned int i;
> +	long arg;
> +	bool no_arg;
> +	char *end;
> +
> +	for (i = 1; i < argc; i++) {
> +		no_arg = true;
> +		if (i < argc - 1) {
> +			no_arg = *argv[i + 1] == '\0';
> +			arg = strtol(argv[i + 1], &end, 10);
> +			no_arg |= *end != '\0';
> +			no_arg |= arg < 0;
> +		}
> +
> +		if (!strcmp("--iterations", argv[i])) {
> +			if (no_arg)
> +				report_abort("--iterations needs a
> positive parameter");
> +			args.iterations = arg;
> +			++i;
> +		} else {
> +			report_abort("Unsupported parameter '%s'",
> +				     argv[i]);
> +		}
> +	}
> +	return args;
> +}
> +
> +int main(int argc, char **argv)
> +{
> +	unsigned int i;
> +
> +	struct args args = parse_args(argc, argv);
> +
> +	report_prefix_push("specification exception");
> +	for (i = 0; spec_ex_triggers[i].name; i++) {
> +		report_prefix_push(spec_ex_triggers[i].name);
> +		test_spec_ex(&args, &spec_ex_triggers[i]);
> +		report_prefix_pop();
> +	}
> +	report_prefix_pop();
> +
> +	return report_summary();
> +}
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index 9e1802f..5f43d52 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -109,3 +109,6 @@ file = edat.elf
>  
>  [mvpg-sie]
>  file = mvpg-sie.elf
> +
> +[spec_ex]
> +file = spec_ex.elf

Re: [kvm-unit-tests PATCH v3 2/2] s390x: Test specification exceptions during transaction

[Claudio Imbrenda]

On Fri, 22 Oct 2021 14:01:56 +0200
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

> Program interruptions during transactional execution cause other
> interruption codes.
> Check that we see the expected code for (some) specification exceptions.
> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
>  lib/s390x/asm/arch_def.h |   1 +
>  s390x/spec_ex.c          | 172 +++++++++++++++++++++++++++++++++++++--
>  2 files changed, 168 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 40626d7..f7fb467 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -55,6 +55,7 @@ struct psw {
>  #define PSW_MASK_BA			0x0000000080000000UL
>  #define PSW_MASK_64			(PSW_MASK_BA | PSW_MASK_EA)
>  
> +#define CTL0_TRANSACT_EX_CTL		(63 -  8)
>  #define CTL0_LOW_ADDR_PROT		(63 - 35)
>  #define CTL0_EDAT			(63 - 40)
>  #define CTL0_IEP			(63 - 43)
> diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
> index ec3322a..f3628bd 100644
> --- a/s390x/spec_ex.c
> +++ b/s390x/spec_ex.c
> @@ -4,9 +4,14 @@
>   *
>   * Specification exception test.
>   * Tests that specification exceptions occur when expected.
> + * This includes specification exceptions occurring during transactional execution
> + * as these result in another interruption code (the transactional-execution-aborted
> + * bit is set).
>   */
>  #include <stdlib.h>
> +#include <htmintrin.h>
>  #include <libcflat.h>
> +#include <asm/barrier.h>
>  #include <asm/interrupt.h>
>  #include <asm/facility.h>
>  
> @@ -92,18 +97,23 @@ static void not_even(void)
>  struct spec_ex_trigger {
>  	const char *name;
>  	void (*func)(void);
> +	bool transactable;
>  	void (*fixup)(void);
>  };
>  
>  static const struct spec_ex_trigger spec_ex_triggers[] = {
> -	{ "psw_bit_12_is_1", &psw_bit_12_is_1, &fixup_invalid_psw},
> -	{ "bad_alignment", &bad_alignment, NULL},
> -	{ "not_even", &not_even, NULL},
> -	{ NULL, NULL, NULL},
> +	{ "psw_bit_12_is_1", &psw_bit_12_is_1, false, &fixup_invalid_psw},
> +	{ "bad_alignment", &bad_alignment, true, NULL},
> +	{ "not_even", &not_even, true, NULL},
> +	{ NULL, NULL, true, NULL},
>  };
>  
>  struct args {
>  	uint64_t iterations;
> +	uint64_t max_retries;
> +	uint64_t suppress_info;
> +	uint64_t max_failures;
> +	bool diagnose;
>  };
>  
>  static void test_spec_ex(struct args *args,
> @@ -131,14 +141,132 @@ static void test_spec_ex(struct args *args,
>  		    expected_pgm);
>  }
>  
> +#define TRANSACTION_COMPLETED 4
> +#define TRANSACTION_MAX_RETRIES 5
> +
> +/* NULL must be passed to __builtin_tbegin via constant, forbid diagnose from
> + * being NULL to keep things simple
> + */
> +static int __attribute__((nonnull))
> +with_transaction(void (*trigger)(void), struct __htm_tdb *diagnose)
> +{
> +	int cc;
> +

if you want to be extra sure, put an assert here (although I'm not sure
how nonnull works, I have never seen it before)

> +	cc = __builtin_tbegin(diagnose);
> +	if (cc == _HTM_TBEGIN_STARTED) {
> +		trigger();
> +		__builtin_tend();
> +		return -TRANSACTION_COMPLETED;
> +	} else {
> +		return -cc;
> +	}
> +}
> +
> +static int retry_transaction(const struct spec_ex_trigger *trigger, unsigned int max_retries,
> +			     struct __htm_tdb *tdb, uint16_t expected_pgm)
> +{
> +	int trans_result, i;
> +	uint16_t pgm;
> +
> +	for (i = 0; i < max_retries; i++) {
> +		expect_pgm_int();
> +		trans_result = with_transaction(trigger->func, tdb);
> +		if (trans_result == -_HTM_TBEGIN_TRANSIENT) {
> +			mb();
> +			pgm = lc->pgm_int_code;
> +			if (pgm == 0)
> +				continue;
> +			else if (pgm == expected_pgm)
> +				return 0;
> +		}
> +		return trans_result;
> +	}
> +	return -TRANSACTION_MAX_RETRIES;

so this means that a test will be considered failed if the transaction
failed too many times?

this means that could fail if the test is run on busy system, even if
the host running the unit test is correct

also, do you really need to use negative values? it's probably easier
to read if you stick to positive values, and less prone to mistakes if
you accidentally forget a - somewhere.

> +}
> +
> +static void test_spec_ex_trans(struct args *args, const struct spec_ex_trigger *trigger)
> +{
> +	const uint16_t expected_pgm = PGM_INT_CODE_SPECIFICATION
> +			      | PGM_INT_CODE_TX_ABORTED_EVENT;
> +	union {
> +		struct __htm_tdb tdb;
> +		uint64_t dwords[sizeof(struct __htm_tdb) / sizeof(uint64_t)];
> +	} diag;
> +	unsigned int i, failures = 0;
> +	int trans_result;
> +
> +	if (!test_facility(73)) {
> +		report_skip("transactional-execution facility not installed");
> +		return;
> +	}
> +	ctl_set_bit(0, CTL0_TRANSACT_EX_CTL); /* enable transactional-exec */
> +
> +	for (i = 0; i < args->iterations && failures <= args->max_failures; i++) {
> +		register_pgm_cleanup_func(trigger->fixup);
> +		trans_result = retry_transaction(trigger, args->max_retries, &diag.tdb, expected_pgm);

so you retry each iteration up to args->max_retries times, and if a
transaction aborts too many times (maybe because the host system is
very busy), then you consider it a fail

> +		register_pgm_cleanup_func(NULL);
> +		switch (trans_result) {
> +		case 0:
> +			continue;
> +		case -_HTM_TBEGIN_INDETERMINATE:
> +		case -_HTM_TBEGIN_PERSISTENT:
> +			if (failures < args->suppress_info)
> +				report_info("transaction failed with cc %d",
> +					    -trans_result);
> +			break;
> +		case -_HTM_TBEGIN_TRANSIENT:
> +			report_fail("Program interrupt: expected(%d) == received(%d)",
> +				    expected_pgm,
> +				    clear_pgm_int());
> +			goto out;
> +		case -TRANSACTION_COMPLETED:
> +			report_fail("Transaction completed without exception");
> +			goto out;
> +		case -TRANSACTION_MAX_RETRIES:
> +			if (failures < args->suppress_info)
> +				report_info("Retried transaction %lu times without exception",
> +					    args->max_retries);
> +			break;
> +		default:
> +			report_fail("Invalid return transaction result");
> +			goto out;
> +		}
> +
> +		if (failures < args->suppress_info)
> +			report_info("transaction abort code: %llu", diag.tdb.abort_code);
> +		if (args->diagnose && failures < args->suppress_info) {
> +			for (i = 0; i < 32; i++)
> +				report_info("diag+%03d: %016lx", i*8, diag.dwords[i]);
> +		}
> +		++failures;
> +	}
> +	if (failures <= args->max_failures) {
> +		report_pass(
> +			"Program interrupt: always expected(%d) == received(%d), transaction failures: %u",
> +			expected_pgm,
> +			expected_pgm,
> +			failures);
> +	} else {
> +		report_fail("Too many transaction failures: %u", failures);
> +	}
> +	if (failures > args->suppress_info)
> +		report_info("Suppressed some transaction failure information messages");
> +
> +out:
> +	ctl_clear_bit(0, CTL0_TRANSACT_EX_CTL);
> +}
> +
>  static struct args parse_args(int argc, char **argv)
>  {
>  	struct args args = {
>  		.iterations = 1,
> +		.max_retries = 20,
> +		.suppress_info = 20,
> +		.diagnose = false
>  	};
>  	unsigned int i;
>  	long arg;
> -	bool no_arg;
> +	bool no_arg, max_failures = false;
>  	char *end;
>  
>  	for (i = 1; i < argc; i++) {
> @@ -155,11 +283,35 @@ static struct args parse_args(int argc, char **argv)

again, do we _really_ need all these parameters?

>  				report_abort("--iterations needs a positive parameter");
>  			args.iterations = arg;
>  			++i;
> +		} else if (!strcmp("--max-retries", argv[i])) {
> +			if (no_arg)
> +				report_abort("--max-retries needs a positive parameter");
> +			args.max_retries = arg;
> +			++i;
> +		} else if (!strcmp("--suppress-info", argv[i])) {
> +			if (no_arg)
> +				report_abort("--suppress-info needs a positive parameter");
> +			args.suppress_info = arg;
> +			++i;
> +		} else if (!strcmp("--max-failures", argv[i])) {
> +			if (no_arg)
> +				report_abort("--max-failures needs a positive parameter");
> +			args.max_failures = arg;
> +			max_failures = true;
> +			++i;
> +		} else if (!strcmp("--diagnose", argv[i])) {
> +			args.diagnose = true;
> +		} else if (!strcmp("--no-diagnose", argv[i])) {
> +			args.diagnose = false;
>  		} else {
>  			report_abort("Unsupported parameter '%s'",
>  				     argv[i]);
>  		}
>  	}
> +
> +	if (!max_failures)
> +		args.max_failures = args.iterations / 1000;
> +
>  	return args;
>  }
>  
> @@ -177,5 +329,15 @@ int main(int argc, char **argv)
>  	}
>  	report_prefix_pop();
>  
> +	report_prefix_push("specification exception during transaction");
> +	for (i = 0; spec_ex_triggers[i].name; i++) {
> +		if (spec_ex_triggers[i].transactable) {
> +			report_prefix_push(spec_ex_triggers[i].name);
> +			test_spec_ex_trans(&args, &spec_ex_triggers[i]);
> +			report_prefix_pop();
> +		}
> +	}
> +	report_prefix_pop();
> +
>  	return report_summary();
>  }

Re: [kvm-unit-tests PATCH v3 2/2] s390x: Test specification exceptions during transaction

[Christian Borntraeger]

Am 25.10.21 um 19:30 schrieb Claudio Imbrenda:
> On Fri, 22 Oct 2021 14:01:56 +0200
> Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
> 
>> Program interruptions during transactional execution cause other
>> interruption codes.
>> Check that we see the expected code for (some) specification exceptions.
>>
>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>> ---
>>   lib/s390x/asm/arch_def.h |   1 +
>>   s390x/spec_ex.c          | 172 +++++++++++++++++++++++++++++++++++++--
>>   2 files changed, 168 insertions(+), 5 deletions(-)
>>
>> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
>> index 40626d7..f7fb467 100644
>> --- a/lib/s390x/asm/arch_def.h
>> +++ b/lib/s390x/asm/arch_def.h
>> @@ -55,6 +55,7 @@ struct psw {
>>   #define PSW_MASK_BA			0x0000000080000000UL
>>   #define PSW_MASK_64			(PSW_MASK_BA | PSW_MASK_EA)
>>   
>> +#define CTL0_TRANSACT_EX_CTL		(63 -  8)
>>   #define CTL0_LOW_ADDR_PROT		(63 - 35)
>>   #define CTL0_EDAT			(63 - 40)
>>   #define CTL0_IEP			(63 - 43)
>> diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
>> index ec3322a..f3628bd 100644
>> --- a/s390x/spec_ex.c
>> +++ b/s390x/spec_ex.c
>> @@ -4,9 +4,14 @@
>>    *
>>    * Specification exception test.
>>    * Tests that specification exceptions occur when expected.
>> + * This includes specification exceptions occurring during transactional execution
>> + * as these result in another interruption code (the transactional-execution-aborted
>> + * bit is set).
>>    */
>>   #include <stdlib.h>
>> +#include <htmintrin.h>
>>   #include <libcflat.h>
>> +#include <asm/barrier.h>
>>   #include <asm/interrupt.h>
>>   #include <asm/facility.h>
>>   
>> @@ -92,18 +97,23 @@ static void not_even(void)
>>   struct spec_ex_trigger {
>>   	const char *name;
>>   	void (*func)(void);
>> +	bool transactable;
>>   	void (*fixup)(void);
>>   };
>>   
>>   static const struct spec_ex_trigger spec_ex_triggers[] = {
>> -	{ "psw_bit_12_is_1", &psw_bit_12_is_1, &fixup_invalid_psw},
>> -	{ "bad_alignment", &bad_alignment, NULL},
>> -	{ "not_even", &not_even, NULL},
>> -	{ NULL, NULL, NULL},
>> +	{ "psw_bit_12_is_1", &psw_bit_12_is_1, false, &fixup_invalid_psw},
>> +	{ "bad_alignment", &bad_alignment, true, NULL},
>> +	{ "not_even", &not_even, true, NULL},
>> +	{ NULL, NULL, true, NULL},
>>   };
>>   
>>   struct args {
>>   	uint64_t iterations;
>> +	uint64_t max_retries;
>> +	uint64_t suppress_info;
>> +	uint64_t max_failures;
>> +	bool diagnose;
>>   };
>>   
>>   static void test_spec_ex(struct args *args,
>> @@ -131,14 +141,132 @@ static void test_spec_ex(struct args *args,
>>   		    expected_pgm);
>>   }
>>   
>> +#define TRANSACTION_COMPLETED 4
>> +#define TRANSACTION_MAX_RETRIES 5
>> +
>> +/* NULL must be passed to __builtin_tbegin via constant, forbid diagnose from
>> + * being NULL to keep things simple
>> + */
>> +static int __attribute__((nonnull))
>> +with_transaction(void (*trigger)(void), struct __htm_tdb *diagnose)
>> +{
>> +	int cc;
>> +
> 
> if you want to be extra sure, put an assert here (although I'm not sure
> how nonnull works, I have never seen it before)
> 
>> +	cc = __builtin_tbegin(diagnose);
>> +	if (cc == _HTM_TBEGIN_STARTED) {
>> +		trigger();
>> +		__builtin_tend();
>> +		return -TRANSACTION_COMPLETED;
>> +	} else {
>> +		return -cc;
>> +	}
>> +}
>> +
>> +static int retry_transaction(const struct spec_ex_trigger *trigger, unsigned int max_retries,
>> +			     struct __htm_tdb *tdb, uint16_t expected_pgm)
>> +{
>> +	int trans_result, i;
>> +	uint16_t pgm;
>> +
>> +	for (i = 0; i < max_retries; i++) {
>> +		expect_pgm_int();
>> +		trans_result = with_transaction(trigger->func, tdb);
>> +		if (trans_result == -_HTM_TBEGIN_TRANSIENT) {
>> +			mb();
>> +			pgm = lc->pgm_int_code;
>> +			if (pgm == 0)
>> +				continue;
>> +			else if (pgm == expected_pgm)
>> +				return 0;
>> +		}
>> +		return trans_result;
>> +	}
>> +	return -TRANSACTION_MAX_RETRIES;
> 
> so this means that a test will be considered failed if the transaction
> failed too many times?
> this means that could fail if the test is run on busy system, even if
> the host running the unit test is correct

Can we use constrained transactions for this test? those will succeed.

Re: [kvm-unit-tests PATCH v3 1/2] s390x: Add specification exception test

[Janis Schoetterl-Glausch]

On 10/25/21 19:17, Claudio Imbrenda wrote:
> On Fri, 22 Oct 2021 14:01:55 +0200
> Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
> 
>> Generate specification exceptions and check that they occur.
>> With the iterations argument one can check if specification
>> exception interpretation occurs, e.g. by using a high value and
>> checking that the debugfs counters are substantially lower.
>> The argument is also useful for estimating the performance benefit
>> of interpretation.
>>
>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>> ---
>>  s390x/Makefile      |   1 +
>>  s390x/spec_ex.c     | 181 ++++++++++++++++++++++++++++++++++++++++++++
>>  s390x/unittests.cfg |   3 +
>>  3 files changed, 185 insertions(+)
>>  create mode 100644 s390x/spec_ex.c
>>
>> diff --git a/s390x/Makefile b/s390x/Makefile
>> index d18b08b..3e42784 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -24,6 +24,7 @@ tests += $(TEST_DIR)/mvpg.elf
>>  tests += $(TEST_DIR)/uv-host.elf
>>  tests += $(TEST_DIR)/edat.elf
>>  tests += $(TEST_DIR)/mvpg-sie.elf
>> +tests += $(TEST_DIR)/spec_ex.elf
>>  
>>  tests_binary = $(patsubst %.elf,%.bin,$(tests))
>>  ifneq ($(HOST_KEY_DOCUMENT),)
>> diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
>> new file mode 100644
>> index 0000000..ec3322a
>> --- /dev/null
>> +++ b/s390x/spec_ex.c
>> @@ -0,0 +1,181 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright IBM Corp. 2021
>> + *
>> + * Specification exception test.
>> + * Tests that specification exceptions occur when expected.
>> + */
>> +#include <stdlib.h>
>> +#include <libcflat.h>
>> +#include <asm/interrupt.h>
>> +#include <asm/facility.h>
>> +
>> +static struct lowcore *lc = (struct lowcore *) 0;
>> +
>> +static bool expect_invalid_psw;
>> +static struct psw expected_psw;
>> +static struct psw fixup_psw;
>> +
>> +/* The standard program exception handler cannot deal with invalid old PSWs,
>> + * especially not invalid instruction addresses, as in that case one cannot
>> + * find the instruction following the faulting one from the old PSW.
>> + * The PSW to return to is set by load_psw.
>> + */
>> +static void fixup_invalid_psw(void)
>> +{
>> +	if (expect_invalid_psw) {
>> +		report(expected_psw.mask == lc->pgm_old_psw.mask
>> +		       && expected_psw.addr == lc->pgm_old_psw.addr,
>> +		       "Invalid program new PSW as expected");
>> +		expect_invalid_psw = false;
> 
> can you find a way to call report() where the test is
> triggered (psw_bit_12_is_1), instead of burying it here?
> 
Yes, should be doable.

> maybe instead of calling report you can set a flag like
> "expected_psw_found" and then call report on it?
> 
>> +	}
>> +	lc->pgm_old_psw = fixup_psw;
>> +}
>> +
>> +/* Load possibly invalid psw, but setup fixup_psw before,
>> + * so that *fixup_invalid_psw() can bring us back onto the right track.
>> + */
>> +static void load_psw(struct psw psw)
>> +{
>> +	uint64_t scratch;
>> +
> 
> I understand why you are doing this, but I wonder if there is a "nicer"
> way to do it. What happens if you chose a nicer and unique name for the
> label and make it global?
> 
I don't think that would work, the compiler might inline the function,
duplicating the label.

I suppose I could replace the stg with an assignment in C, not sure if that's nicer.

>> +	fixup_psw.mask = extract_psw_mask();
> 
> then you could add this here:
> 	fixup_psw.addr = after_lpswe;
> 
>> +	asm volatile (
>> +		"	larl	%[scratch],nop%=\n"
>> +		"	stg	%[scratch],%[addr]\n"
> 	^ those two lines are no longer needed ^
>> +		"	lpswe	%[psw]\n"
>> +		"nop%=:	nop\n"
> 	".global after_lpswe \n"
> 	"after_lpswe:	nop"
>> +		: [scratch] "=&r"(scratch),
>> +		  [addr] "=&T"(fixup_psw.addr)
>> +		: [psw] "Q"(psw)
>> +		: "cc", "memory"
>> +	);
>> +}
>> +
>> +static void psw_bit_12_is_1(void)
>> +{
>> +	expected_psw.mask = 0x0008000000000000;
>> +	expected_psw.addr = 0x00000000deadbeee;
>> +	expect_invalid_psw = true;
>> +	load_psw(expected_psw);
> 
> and here something like
> 	report(expected_psw_found, "blah blah blah");
> 
>> +}
>> +
>> +static void bad_alignment(void)
>> +{
>> +	uint32_t words[5] = {0, 0, 0};
>> +	uint32_t (*bad_aligned)[4];
>> +
>> +	register uint64_t r1 asm("6");
>> +	register uint64_t r2 asm("7");
>> +	if (((uintptr_t)&words[0]) & 0xf)
>> +		bad_aligned = (uint32_t (*)[4])&words[0];
>> +	else
>> +		bad_aligned = (uint32_t (*)[4])&words[1];
> 
> this is a lot of work... can't you just declare it like:
> 
> 	uint32_t words[5] __attribute__((aligned(16)));
> and then just use
> 	(words + 1) ?

That's nicer indeed.
> 
>> +	asm volatile ("lpq %0,%2"
>> +		      : "=r"(r1), "=r"(r2)
> 
> since you're ignoring the return value, can't you hardcode r6, and mark
> it (and r7) as clobbered? like:
> 		"lpq 6, %[bad]"
> 		: : [bad] "T"(words[1])
> 		: "%r6", "%r7" 
> 
Ok, btw. is there a reason bare register numbers seem to be more common
compared to %%rN ?

>> +		      : "T"(*bad_aligned)
>> +	);
>> +}
>> +
>> +static void not_even(void)
>> +{
>> +	uint64_t quad[2];
>> +
>> +	register uint64_t r1 asm("7");
>> +	register uint64_t r2 asm("8");
>> +	asm volatile (".insn	rxy,0xe3000000008f,%0,%2" //lpq
>> %0,%2
> 
> this is even uglier. I guess you had already tried this?
> 
Yes, the assembler won't let you do that.

> 		"lpq 7, %[good]"
> 			: : [good] "T"(quad)
> 			: "%r7", "%r8"
> 
> if that doesn't work, then the same but with .insn
> 
>> +		      : "=r"(r1), "=r"(r2)
>> +		      : "T"(quad)
>> +	);
>> +}
>> +
>> +struct spec_ex_trigger {
>> +	const char *name;
>> +	void (*func)(void);
>> +	void (*fixup)(void);
>> +};
>> +
>> +static const struct spec_ex_trigger spec_ex_triggers[] = {
>> +	{ "psw_bit_12_is_1", &psw_bit_12_is_1, &fixup_invalid_psw},
>> +	{ "bad_alignment", &bad_alignment, NULL},
>> +	{ "not_even", &not_even, NULL},
>> +	{ NULL, NULL, NULL},
>> +};
>> +
> 
> this is a lot of infrastructure for 3 tests... (or even for 5 tests,
> since you will add the transactions in the next patch)

Is it? I think we'd want a test for a "normal" specification exception,
and one for an invalid PSW at least. Even for just those two, I don't
think it would be nice to duplicate the test_spec_ex harness.
> 
> are you planning to significantly extend this test in the future?

Not really, but I thought having it be easily extensible might be nice.
> 
>> +struct args {
>> +	uint64_t iterations;
>> +};
>> +
>> +static void test_spec_ex(struct args *args,
>> +			 const struct spec_ex_trigger *trigger)
>> +{
>> +	uint16_t expected_pgm = PGM_INT_CODE_SPECIFICATION;
>> +	uint16_t pgm;
>> +	unsigned int i;
>> +
>> +	for (i = 0; i < args->iterations; i++) {
>> +		expect_pgm_int();
>> +		register_pgm_cleanup_func(trigger->fixup);
>> +		trigger->func();
>> +		register_pgm_cleanup_func(NULL);
>> +		pgm = clear_pgm_int();
>> +		if (pgm != expected_pgm) {
>> +			report_fail("Program interrupt: expected(%d)
>> == received(%d)",
>> +				    expected_pgm,
>> +				    pgm);
>> +			return;
>> +		}
>> +	}
>> +	report_pass("Program interrupt: always expected(%d) ==
>> received(%d)",
>> +		    expected_pgm,
>> +		    expected_pgm);
>> +}
>> +
>> +static struct args parse_args(int argc, char **argv)
> 
> do we _really_ need commandline arguments?
> 
No, but they can be useful.
The iterations argument can be used to check if interpretation happens.
The transaction arguments can be useful while developing a test case.

> is it really so important to be able to control these parameters?
> 
> can you find some values for the parameters so that the test works (as
> in, it actually tests what it's supposed to) and also so that the whole
> unit test ends in less than 30 seconds?

I think the defaults are fine for that, no?
> 
>> +{
>> +	struct args args = {
>> +		.iterations = 1,
>> +	};
>> +	unsigned int i;
>> +	long arg;
>> +	bool no_arg;
>> +	char *end;
>> +
>> +	for (i = 1; i < argc; i++) {
>> +		no_arg = true;
>> +		if (i < argc - 1) {
>> +			no_arg = *argv[i + 1] == '\0';
>> +			arg = strtol(argv[i + 1], &end, 10);
>> +			no_arg |= *end != '\0';
>> +			no_arg |= arg < 0;
>> +		}
>> +
>> +		if (!strcmp("--iterations", argv[i])) {
>> +			if (no_arg)
>> +				report_abort("--iterations needs a
>> positive parameter");
>> +			args.iterations = arg;
>> +			++i;
>> +		} else {
>> +			report_abort("Unsupported parameter '%s'",
>> +				     argv[i]);
>> +		}
>> +	}
>> +	return args;
>> +}
>> +
>> +int main(int argc, char **argv)
>> +{
>> +	unsigned int i;
>> +
>> +	struct args args = parse_args(argc, argv);
>> +
>> +	report_prefix_push("specification exception");
>> +	for (i = 0; spec_ex_triggers[i].name; i++) {
>> +		report_prefix_push(spec_ex_triggers[i].name);
>> +		test_spec_ex(&args, &spec_ex_triggers[i]);
>> +		report_prefix_pop();
>> +	}
>> +	report_prefix_pop();
>> +
>> +	return report_summary();
>> +}
>> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
>> index 9e1802f..5f43d52 100644
>> --- a/s390x/unittests.cfg
>> +++ b/s390x/unittests.cfg
>> @@ -109,3 +109,6 @@ file = edat.elf
>>  
>>  [mvpg-sie]
>>  file = mvpg-sie.elf
>> +
>> +[spec_ex]
>> +file = spec_ex.elf
> 

Re: [kvm-unit-tests PATCH v3 1/2] s390x: Add specification exception test

[Claudio Imbrenda]

On Tue, 26 Oct 2021 14:00:31 +0200
Janis Schoetterl-Glausch <scgl@linux.vnet.ibm.com> wrote:

[...]

> I don't think that would work, the compiler might inline the function,
> duplicating the label.

__attribute__((noinline))

:)

> I suppose I could replace the stg with an assignment in C, not sure if that's nicer.
> 
> >> +	fixup_psw.mask = extract_psw_mask();  
> > 
> > then you could add this here:
> > 	fixup_psw.addr = after_lpswe;
> >   
> >> +	asm volatile (
> >> +		"	larl	%[scratch],nop%=\n"
> >> +		"	stg	%[scratch],%[addr]\n"  
> > 	^ those two lines are no longer needed ^  
> >> +		"	lpswe	%[psw]\n"
> >> +		"nop%=:	nop\n"  
> > 	".global after_lpswe \n"
> > 	"after_lpswe:	nop"  
> >> +		: [scratch] "=&r"(scratch),
> >> +		  [addr] "=&T"(fixup_psw.addr)
> >> +		: [psw] "Q"(psw)
> >> +		: "cc", "memory"
> >> +	);
> >> +}

[...]
 
> That's nicer indeed.
> >   
> >> +	asm volatile ("lpq %0,%2"
> >> +		      : "=r"(r1), "=r"(r2)  
> > 
> > since you're ignoring the return value, can't you hardcode r6, and mark
> > it (and r7) as clobbered? like:
> > 		"lpq 6, %[bad]"
> > 		: : [bad] "T"(words[1])
> > 		: "%r6", "%r7" 
> >   
> Ok, btw. is there a reason bare register numbers seem to be more common
> compared to %%rN ?

I don't know, I guess laziness?

> 
> >> +		      : "T"(*bad_aligned)
> >> +	);
> >> +}
> >> +
> >> +static void not_even(void)
> >> +{
> >> +	uint64_t quad[2];
> >> +
> >> +	register uint64_t r1 asm("7");
> >> +	register uint64_t r2 asm("8");
> >> +	asm volatile (".insn	rxy,0xe3000000008f,%0,%2" //lpq
> >> %0,%2  
> > 
> > this is even uglier. I guess you had already tried this?
> >   
> Yes, the assembler won't let you do that.

yeah I thought so

> 
> > 		"lpq 7, %[good]"
> > 			: : [good] "T"(quad)
> > 			: "%r7", "%r8"
> > 
> > if that doesn't work, then the same but with .insn

I guess you can still try this ^ ?

> >   
> >> +		      : "=r"(r1), "=r"(r2)
> >> +		      : "T"(quad)
> >> +	);
> >> +}
> >> +
> >> +struct spec_ex_trigger {
> >> +	const char *name;
> >> +	void (*func)(void);
> >> +	void (*fixup)(void);
> >> +};
> >> +
> >> +static const struct spec_ex_trigger spec_ex_triggers[] = {
> >> +	{ "psw_bit_12_is_1", &psw_bit_12_is_1, &fixup_invalid_psw},
> >> +	{ "bad_alignment", &bad_alignment, NULL},
> >> +	{ "not_even", &not_even, NULL},
> >> +	{ NULL, NULL, NULL},
> >> +};
> >> +  
> > 
> > this is a lot of infrastructure for 3 tests... (or even for 5 tests,
> > since you will add the transactions in the next patch)  
> 
> Is it? I think we'd want a test for a "normal" specification exception,
> and one for an invalid PSW at least. Even for just those two, I don't
> think it would be nice to duplicate the test_spec_ex harness.

usually we do duplicate code for simple tests, so that reviewers have
an easier time understanding what's going on, on the other hand..

> > 
> > are you planning to significantly extend this test in the future?  
> 
> Not really, but I thought having it be easily extensible might be nice.

..fair enough

this way it will be easier to extend this in the future, even though we
don't have any immediate plans to do so

maybe add some words in the patch description, and some comments, to
explain what's going on, to make it easier for others to understand
this code

> >   
> >> +struct args {
> >> +	uint64_t iterations;
> >> +};
> >> +
> >> +static void test_spec_ex(struct args *args,
> >> +			 const struct spec_ex_trigger *trigger)
> >> +{
> >> +	uint16_t expected_pgm = PGM_INT_CODE_SPECIFICATION;
> >> +	uint16_t pgm;
> >> +	unsigned int i;
> >> +
> >> +	for (i = 0; i < args->iterations; i++) {
> >> +		expect_pgm_int();
> >> +		register_pgm_cleanup_func(trigger->fixup);
> >> +		trigger->func();
> >> +		register_pgm_cleanup_func(NULL);
> >> +		pgm = clear_pgm_int();
> >> +		if (pgm != expected_pgm) {
> >> +			report_fail("Program interrupt: expected(%d)
> >> == received(%d)",
> >> +				    expected_pgm,
> >> +				    pgm);
> >> +			return;
> >> +		}
> >> +	}
> >> +	report_pass("Program interrupt: always expected(%d) ==
> >> received(%d)",
> >> +		    expected_pgm,
> >> +		    expected_pgm);
> >> +}
> >> +
> >> +static struct args parse_args(int argc, char **argv)  
> > 
> > do we _really_ need commandline arguments?
> >   
> No, but they can be useful.
> The iterations argument can be used to check if interpretation happens.
> The transaction arguments can be useful while developing a test case.
> 
> > is it really so important to be able to control these parameters?
> > 
> > can you find some values for the parameters so that the test works (as
> > in, it actually tests what it's supposed to) and also so that the whole
> > unit test ends in less than 30 seconds?  
> 
> I think the defaults are fine for that, no?

ok so they are only for convenience in case things go wrong?

> >   
> >> +{
> >> +	struct args args = {
> >> +		.iterations = 1,
> >> +	};
> >> +	unsigned int i;
> >> +	long arg;
> >> +	bool no_arg;
> >> +	char *end;
> >> +
> >> +	for (i = 1; i < argc; i++) {
> >> +		no_arg = true;
> >> +		if (i < argc - 1) {
> >> +			no_arg = *argv[i + 1] == '\0';
> >> +			arg = strtol(argv[i + 1], &end, 10);
> >> +			no_arg |= *end != '\0';
> >> +			no_arg |= arg < 0;
> >> +		}
> >> +
> >> +		if (!strcmp("--iterations", argv[i])) {
> >> +			if (no_arg)
> >> +				report_abort("--iterations needs a
> >> positive parameter");
> >> +			args.iterations = arg;
> >> +			++i;
> >> +		} else {
> >> +			report_abort("Unsupported parameter '%s'",
> >> +				     argv[i]);
> >> +		}
> >> +	}

I wonder if we can factor out the parameter parsing

> >> +	return args;
> >> +}
> >> +
> >> +int main(int argc, char **argv)
> >> +{
> >> +	unsigned int i;
> >> +
> >> +	struct args args = parse_args(argc, argv);
> >> +
> >> +	report_prefix_push("specification exception");
> >> +	for (i = 0; spec_ex_triggers[i].name; i++) {
> >> +		report_prefix_push(spec_ex_triggers[i].name);
> >> +		test_spec_ex(&args, &spec_ex_triggers[i]);
> >> +		report_prefix_pop();
> >> +	}
> >> +	report_prefix_pop();
> >> +
> >> +	return report_summary();
> >> +}
> >> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> >> index 9e1802f..5f43d52 100644
> >> --- a/s390x/unittests.cfg
> >> +++ b/s390x/unittests.cfg
> >> @@ -109,3 +109,6 @@ file = edat.elf
> >>  
> >>  [mvpg-sie]
> >>  file = mvpg-sie.elf
> >> +
> >> +[spec_ex]
> >> +file = spec_ex.elf  
> >   
> 

Re: [kvm-unit-tests PATCH v3 2/2] s390x: Test specification exceptions during transaction

[Janis Schoetterl-Glausch]

On 10/25/21 19:30, Claudio Imbrenda wrote:
> On Fri, 22 Oct 2021 14:01:56 +0200
> Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
> 
>> Program interruptions during transactional execution cause other
>> interruption codes.
>> Check that we see the expected code for (some) specification exceptions.
>>
>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>> ---

[...]

>> +#define TRANSACTION_MAX_RETRIES 5
>> +
>> +/* NULL must be passed to __builtin_tbegin via constant, forbid diagnose from
>> + * being NULL to keep things simple
>> + */
>> +static int __attribute__((nonnull))
>> +with_transaction(void (*trigger)(void), struct __htm_tdb *diagnose)
>> +{
>> +	int cc;
>> +
> 
> if you want to be extra sure, put an assert here (although I'm not sure
> how nonnull works, I have never seen it before)

Ok, with nonnull, the compiler might warn you if you pass NULL.
> 
>> +	cc = __builtin_tbegin(diagnose);
>> +	if (cc == _HTM_TBEGIN_STARTED) {
>> +		trigger();
>> +		__builtin_tend();
>> +		return -TRANSACTION_COMPLETED;
>> +	} else {
>> +		return -cc;
>> +	}
>> +}
>> +
>> +static int retry_transaction(const struct spec_ex_trigger *trigger, unsigned int max_retries,
>> +			     struct __htm_tdb *tdb, uint16_t expected_pgm)
>> +{
>> +	int trans_result, i;
>> +	uint16_t pgm;
>> +
>> +	for (i = 0; i < max_retries; i++) {
>> +		expect_pgm_int();
>> +		trans_result = with_transaction(trigger->func, tdb);
>> +		if (trans_result == -_HTM_TBEGIN_TRANSIENT) {
>> +			mb();
>> +			pgm = lc->pgm_int_code;
>> +			if (pgm == 0)
>> +				continue;
>> +			else if (pgm == expected_pgm)
>> +				return 0;
>> +		}
>> +		return trans_result;
>> +	}
>> +	return -TRANSACTION_MAX_RETRIES;
> 
> so this means that a test will be considered failed if the transaction
> failed too many times?

Yes.
> 
> this means that could fail if the test is run on busy system, even if
> the host running the unit test is correct

I suppose so, don't know how likely that is.
> 
> also, do you really need to use negative values? it's probably easier
> to read if you stick to positive values, and less prone to mistakes if
> you accidentally forget a - somewhere.

Ok.
> 
>> +}
>> +
>> +static void test_spec_ex_trans(struct args *args, const struct spec_ex_trigger *trigger)
>> +{
>> +	const uint16_t expected_pgm = PGM_INT_CODE_SPECIFICATION
>> +			      | PGM_INT_CODE_TX_ABORTED_EVENT;
>> +	union {
>> +		struct __htm_tdb tdb;
>> +		uint64_t dwords[sizeof(struct __htm_tdb) / sizeof(uint64_t)];
>> +	} diag;
>> +	unsigned int i, failures = 0;
>> +	int trans_result;
>> +
>> +	if (!test_facility(73)) {
>> +		report_skip("transactional-execution facility not installed");
>> +		return;
>> +	}
>> +	ctl_set_bit(0, CTL0_TRANSACT_EX_CTL); /* enable transactional-exec */
>> +
>> +	for (i = 0; i < args->iterations && failures <= args->max_failures; i++) {
>> +		register_pgm_cleanup_func(trigger->fixup);
>> +		trans_result = retry_transaction(trigger, args->max_retries, &diag.tdb, expected_pgm);
> 
> so you retry each iteration up to args->max_retries times, and if a
> transaction aborts too many times (maybe because the host system is
> very busy), then you consider it a fail
> 

[...]

Re: [kvm-unit-tests PATCH v3 2/2] s390x: Test specification exceptions during transaction

[Claudio Imbrenda]

On Tue, 26 Oct 2021 16:22:40 +0200
Janis Schoetterl-Glausch <scgl@linux.vnet.ibm.com> wrote:

> On 10/25/21 19:30, Claudio Imbrenda wrote:
> > On Fri, 22 Oct 2021 14:01:56 +0200
> > Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
> >   
> >> Program interruptions during transactional execution cause other
> >> interruption codes.
> >> Check that we see the expected code for (some) specification exceptions.
> >>
> >> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> >> ---  
> 
> [...]
> 
> >> +#define TRANSACTION_MAX_RETRIES 5
> >> +
> >> +/* NULL must be passed to __builtin_tbegin via constant, forbid diagnose from
> >> + * being NULL to keep things simple
> >> + */
> >> +static int __attribute__((nonnull))
> >> +with_transaction(void (*trigger)(void), struct __htm_tdb *diagnose)
> >> +{
> >> +	int cc;
> >> +  
> > 
> > if you want to be extra sure, put an assert here (although I'm not sure
> > how nonnull works, I have never seen it before)  
> 
> Ok, with nonnull, the compiler might warn you if you pass NULL.

fair enough

> >   
> >> +	cc = __builtin_tbegin(diagnose);
> >> +	if (cc == _HTM_TBEGIN_STARTED) {
> >> +		trigger();
> >> +		__builtin_tend();
> >> +		return -TRANSACTION_COMPLETED;
> >> +	} else {
> >> +		return -cc;
> >> +	}
> >> +}
> >> +
> >> +static int retry_transaction(const struct spec_ex_trigger *trigger, unsigned int max_retries,
> >> +			     struct __htm_tdb *tdb, uint16_t expected_pgm)
> >> +{
> >> +	int trans_result, i;
> >> +	uint16_t pgm;
> >> +
> >> +	for (i = 0; i < max_retries; i++) {
> >> +		expect_pgm_int();
> >> +		trans_result = with_transaction(trigger->func, tdb);
> >> +		if (trans_result == -_HTM_TBEGIN_TRANSIENT) {
> >> +			mb();
> >> +			pgm = lc->pgm_int_code;
> >> +			if (pgm == 0)
> >> +				continue;
> >> +			else if (pgm == expected_pgm)
> >> +				return 0;
> >> +		}
> >> +		return trans_result;
> >> +	}
> >> +	return -TRANSACTION_MAX_RETRIES;  
> > 
> > so this means that a test will be considered failed if the transaction
> > failed too many times?  
> 
> Yes.
> > 
> > this means that could fail if the test is run on busy system, even if
> > the host running the unit test is correct  
> 
> I suppose so, don't know how likely that is.

I don't like the idea of failing a test when the implementation is
correct, just because the system might be a little more busy than
expected.

if you can't find a way to refactor the test so that it doesn't fail if
there are too many retries, then at least make it a skip?

but I'd really like to see something that does not fail on a correctly
implemented system just because the test machine was too busy.

> > 
> > also, do you really need to use negative values? it's probably easier
> > to read if you stick to positive values, and less prone to mistakes if
> > you accidentally forget a - somewhere.  
> 
> Ok.
> >   
> >> +}
> >> +
> >> +static void test_spec_ex_trans(struct args *args, const struct spec_ex_trigger *trigger)
> >> +{
> >> +	const uint16_t expected_pgm = PGM_INT_CODE_SPECIFICATION
> >> +			      | PGM_INT_CODE_TX_ABORTED_EVENT;
> >> +	union {
> >> +		struct __htm_tdb tdb;
> >> +		uint64_t dwords[sizeof(struct __htm_tdb) / sizeof(uint64_t)];
> >> +	} diag;
> >> +	unsigned int i, failures = 0;
> >> +	int trans_result;
> >> +
> >> +	if (!test_facility(73)) {
> >> +		report_skip("transactional-execution facility not installed");
> >> +		return;
> >> +	}
> >> +	ctl_set_bit(0, CTL0_TRANSACT_EX_CTL); /* enable transactional-exec */
> >> +
> >> +	for (i = 0; i < args->iterations && failures <= args->max_failures; i++) {
> >> +		register_pgm_cleanup_func(trigger->fixup);
> >> +		trans_result = retry_transaction(trigger, args->max_retries, &diag.tdb, expected_pgm);  
> > 
> > so you retry each iteration up to args->max_retries times, and if a
> > transaction aborts too many times (maybe because the host system is
> > very busy), then you consider it a fail
> >   
> 
> [...]

Re: [kvm-unit-tests PATCH v3 1/2] s390x: Add specification exception test

[Janis Schoetterl-Glausch]

On 10/26/21 15:41, Claudio Imbrenda wrote:
> On Tue, 26 Oct 2021 14:00:31 +0200
> Janis Schoetterl-Glausch <scgl@linux.vnet.ibm.com> wrote:
> 
> [...]
> 
>> I don't think that would work, the compiler might inline the function,
>> duplicating the label.
> 
> __attribute__((noinline))
> 
> :)

+ a comment on why it's necessary and at that point I don't think it's worth it.
> 
>> I suppose I could replace the stg with an assignment in C, not sure if that's nicer.
>>
>>>> +	fixup_psw.mask = extract_psw_mask();  
>>>
>>> then you could add this here:
>>> 	fixup_psw.addr = after_lpswe;
>>>   
>>>> +	asm volatile (
>>>> +		"	larl	%[scratch],nop%=\n"
>>>> +		"	stg	%[scratch],%[addr]\n"  
>>> 	^ those two lines are no longer needed ^  
>>>> +		"	lpswe	%[psw]\n"
>>>> +		"nop%=:	nop\n"  
>>> 	".global after_lpswe \n"
>>> 	"after_lpswe:	nop"  
>>>> +		: [scratch] "=&r"(scratch),
>>>> +		  [addr] "=&T"(fixup_psw.addr)
>>>> +		: [psw] "Q"(psw)
>>>> +		: "cc", "memory"
>>>> +	);
>>>> +}
> 
> [...]
>  
>> That's nicer indeed.
>>>   
>>>> +	asm volatile ("lpq %0,%2"
>>>> +		      : "=r"(r1), "=r"(r2)  
>>>
>>> since you're ignoring the return value, can't you hardcode r6, and mark
>>> it (and r7) as clobbered? like:
>>> 		"lpq 6, %[bad]"
>>> 		: : [bad] "T"(words[1])
>>> 		: "%r6", "%r7" 
>>>   
>> Ok, btw. is there a reason bare register numbers seem to be more common
>> compared to %%rN ?
> 
> I don't know, I guess laziness?
> 
>>
>>>> +		      : "T"(*bad_aligned)
>>>> +	);
>>>> +}
>>>> +
>>>> +static void not_even(void)
>>>> +{
>>>> +	uint64_t quad[2];
>>>> +
>>>> +	register uint64_t r1 asm("7");
>>>> +	register uint64_t r2 asm("8");
>>>> +	asm volatile (".insn	rxy,0xe3000000008f,%0,%2" //lpq
>>>> %0,%2  
>>>
>>> this is even uglier. I guess you had already tried this?
>>>   
>> Yes, the assembler won't let you do that.
> 
> yeah I thought so
> 
>>
>>> 		"lpq 7, %[good]"
>>> 			: : [good] "T"(quad)
>>> 			: "%r7", "%r8"
>>>
>>> if that doesn't work, then the same but with .insn
> 
> I guess you can still try this ^ ?

Ok.
> 
>>>   
>>>> +		      : "=r"(r1), "=r"(r2)
>>>> +		      : "T"(quad)
>>>> +	);
>>>> +}
>>>> +
>>>> +struct spec_ex_trigger {
>>>> +	const char *name;
>>>> +	void (*func)(void);
>>>> +	void (*fixup)(void);
>>>> +};
>>>> +
>>>> +static const struct spec_ex_trigger spec_ex_triggers[] = {
>>>> +	{ "psw_bit_12_is_1", &psw_bit_12_is_1, &fixup_invalid_psw},
>>>> +	{ "bad_alignment", &bad_alignment, NULL},
>>>> +	{ "not_even", &not_even, NULL},
>>>> +	{ NULL, NULL, NULL},
>>>> +};
>>>> +  
>>>
>>> this is a lot of infrastructure for 3 tests... (or even for 5 tests,
>>> since you will add the transactions in the next patch)  
>>
>> Is it? I think we'd want a test for a "normal" specification exception,
>> and one for an invalid PSW at least. Even for just those two, I don't
>> think it would be nice to duplicate the test_spec_ex harness.
> 
> usually we do duplicate code for simple tests, so that reviewers have
> an easier time understanding what's going on, on the other hand..
> 
>>>
>>> are you planning to significantly extend this test in the future?  
>>
>> Not really, but I thought having it be easily extensible might be nice.
> 
> ..fair enough
> 
> this way it will be easier to extend this in the future, even though we
> don't have any immediate plans to do so
> 
> maybe add some words in the patch description, and some comments, to
> explain what's going on, to make it easier for others to understand
> this code

Ok.
> 
>>>   
>>>> +struct args {
>>>> +	uint64_t iterations;
>>>> +};
>>>> +
>>>> +static void test_spec_ex(struct args *args,
>>>> +			 const struct spec_ex_trigger *trigger)
>>>> +{
>>>> +	uint16_t expected_pgm = PGM_INT_CODE_SPECIFICATION;
>>>> +	uint16_t pgm;
>>>> +	unsigned int i;
>>>> +
>>>> +	for (i = 0; i < args->iterations; i++) {
>>>> +		expect_pgm_int();
>>>> +		register_pgm_cleanup_func(trigger->fixup);
>>>> +		trigger->func();
>>>> +		register_pgm_cleanup_func(NULL);
>>>> +		pgm = clear_pgm_int();
>>>> +		if (pgm != expected_pgm) {
>>>> +			report_fail("Program interrupt: expected(%d)
>>>> == received(%d)",
>>>> +				    expected_pgm,
>>>> +				    pgm);
>>>> +			return;
>>>> +		}
>>>> +	}
>>>> +	report_pass("Program interrupt: always expected(%d) ==
>>>> received(%d)",
>>>> +		    expected_pgm,
>>>> +		    expected_pgm);
>>>> +}
>>>> +
>>>> +static struct args parse_args(int argc, char **argv)  
>>>
>>> do we _really_ need commandline arguments?
>>>   
>> No, but they can be useful.
>> The iterations argument can be used to check if interpretation happens.
>> The transaction arguments can be useful while developing a test case.
>>
>>> is it really so important to be able to control these parameters?
>>>
>>> can you find some values for the parameters so that the test works (as
>>> in, it actually tests what it's supposed to) and also so that the whole
>>> unit test ends in less than 30 seconds?  
>>
>> I think the defaults are fine for that, no?
> 
> ok so they are only for convenience in case things go wrong?

Yes, for when you want to poke at it manually for whatever reason.
> 
>>>   
>>>> +{
>>>> +	struct args args = {
>>>> +		.iterations = 1,
>>>> +	};
>>>> +	unsigned int i;
>>>> +	long arg;
>>>> +	bool no_arg;
>>>> +	char *end;
>>>> +
>>>> +	for (i = 1; i < argc; i++) {
>>>> +		no_arg = true;
>>>> +		if (i < argc - 1) {
>>>> +			no_arg = *argv[i + 1] == '\0';
>>>> +			arg = strtol(argv[i + 1], &end, 10);
>>>> +			no_arg |= *end != '\0';
>>>> +			no_arg |= arg < 0;
>>>> +		}
>>>> +
>>>> +		if (!strcmp("--iterations", argv[i])) {
>>>> +			if (no_arg)
>>>> +				report_abort("--iterations needs a
>>>> positive parameter");
>>>> +			args.iterations = arg;
>>>> +			++i;
>>>> +		} else {
>>>> +			report_abort("Unsupported parameter '%s'",
>>>> +				     argv[i]);
>>>> +		}
>>>> +	}
> 
> I wonder if we can factor out the parameter parsing

I don't think it's worth it. Only three arguments are handled the same.
Doing this might be worthwhile tho:

        for (i = 1; i < argc; i++) {                                            
                no_arg = true;                                                  
                if (i < argc - 1) {                                             
                        no_arg = *argv[i + 1] == '\0';                          
                        arg = strtol(argv[i + 1], &end, 10);                    
                        no_arg |= *end != '\0';                                 
                        no_arg |= arg < 0;                                      
                }                                                               
                                                                                
                cmp = "--iterations";                                           
                argp = &args.iterations;                                        
                if (!strcmp(cmp, argv[i])) {                                    
                        if (no_arg)                                             
                                report_abort("%s needs a positive parameter", cmp);
                        *argp = arg;                                            
                        ++i;                                                    
                        continue;                                               
                }                                                               
                cmp = "--max-retries";                                          
                argp = &args.max_retries;                                       
                if (!strcmp(cmp, argv[i])) {                                    
                        if (no_arg)                                             
                                report_abort("%s needs a positive parameter", cmp);
                        *argp = arg;                                            
                        ++i;                                                    
                        continue;                                               
                }                                                               
                cmp = "--suppress-info";                                        
                argp = &args.suppress_info;                                     
                if (!strcmp(cmp, argv[i])) {                                    
                        if (no_arg)                                             
                                report_abort("%s needs a positive parameter", cmp);
                        *argp = arg;                                            
                        ++i;                                                    
                        continue;                                               
                }                                                               
                cmp = "--max-failures";                                         
                argp = &args.max_failures;                                      
                if (!strcmp(cmp, argv[i])) {                                    
                        max_failures = true;                                    
                                                                                
                        if (no_arg)                                             
                                report_abort("%s needs a positive parameter", cmp);
                        *argp = arg;                                            
                        ++i;                                                    
                        continue;                                               
                }                                                               
                if (!strcmp("--diagnose", argv[i])) {                           
                        args.diagnose = true;                                   
                        continue;
		}                                              
                if (!strcmp("--no-diagnose", argv[i])) {                        
                        args.diagnose = false;                                  
                        continue;                                               
                }                                                               
                report_abort("Unsupported parameter '%s'",                      
                             argv[i]);                                          
        }
> 
>>>> +	return args;
>>>> +}
>>>> +
>>>> +int main(int argc, char **argv)
>>>> +{
>>>> +	unsigned int i;
>>>> +
>>>> +	struct args args = parse_args(argc, argv);
>>>> +
>>>> +	report_prefix_push("specification exception");
>>>> +	for (i = 0; spec_ex_triggers[i].name; i++) {
>>>> +		report_prefix_push(spec_ex_triggers[i].name);
>>>> +		test_spec_ex(&args, &spec_ex_triggers[i]);
>>>> +		report_prefix_pop();
>>>> +	}
>>>> +	report_prefix_pop();
>>>> +
>>>> +	return report_summary();
>>>> +}
>>>> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
>>>> index 9e1802f..5f43d52 100644
>>>> --- a/s390x/unittests.cfg
>>>> +++ b/s390x/unittests.cfg
>>>> @@ -109,3 +109,6 @@ file = edat.elf
>>>>  
>>>>  [mvpg-sie]
>>>>  file = mvpg-sie.elf
>>>> +
>>>> +[spec_ex]
>>>> +file = spec_ex.elf  
>>>   
>>
> 

Re: [kvm-unit-tests PATCH v3 2/2] s390x: Test specification exceptions during transaction

[Janis Schoetterl-Glausch]

On 10/26/21 16:55, Claudio Imbrenda wrote:
> On Tue, 26 Oct 2021 16:22:40 +0200
> Janis Schoetterl-Glausch <scgl@linux.vnet.ibm.com> wrote:
> 
>> On 10/25/21 19:30, Claudio Imbrenda wrote:
>>> On Fri, 22 Oct 2021 14:01:56 +0200
>>> Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
>>>   
>>>> Program interruptions during transactional execution cause other
>>>> interruption codes.
>>>> Check that we see the expected code for (some) specification exceptions.
>>>>
>>>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>>>> ---  
>>
>> [...]
>>
>>>> +#define TRANSACTION_MAX_RETRIES 5
>>>> +
>>>> +/* NULL must be passed to __builtin_tbegin via constant, forbid diagnose from
>>>> + * being NULL to keep things simple
>>>> + */
>>>> +static int __attribute__((nonnull))
>>>> +with_transaction(void (*trigger)(void), struct __htm_tdb *diagnose)
>>>> +{
>>>> +	int cc;
>>>> +  
>>>
>>> if you want to be extra sure, put an assert here (although I'm not sure
>>> how nonnull works, I have never seen it before)  
>>
>> Ok, with nonnull, the compiler might warn you if you pass NULL.
> 
> fair enough
> 
>>>   
>>>> +	cc = __builtin_tbegin(diagnose);
>>>> +	if (cc == _HTM_TBEGIN_STARTED) {
>>>> +		trigger();
>>>> +		__builtin_tend();
>>>> +		return -TRANSACTION_COMPLETED;
>>>> +	} else {
>>>> +		return -cc;
>>>> +	}
>>>> +}
>>>> +
>>>> +static int retry_transaction(const struct spec_ex_trigger *trigger, unsigned int max_retries,
>>>> +			     struct __htm_tdb *tdb, uint16_t expected_pgm)
>>>> +{
>>>> +	int trans_result, i;
>>>> +	uint16_t pgm;
>>>> +
>>>> +	for (i = 0; i < max_retries; i++) {
>>>> +		expect_pgm_int();
>>>> +		trans_result = with_transaction(trigger->func, tdb);
>>>> +		if (trans_result == -_HTM_TBEGIN_TRANSIENT) {
>>>> +			mb();
>>>> +			pgm = lc->pgm_int_code;
>>>> +			if (pgm == 0)
>>>> +				continue;
>>>> +			else if (pgm == expected_pgm)
>>>> +				return 0;
>>>> +		}
>>>> +		return trans_result;
>>>> +	}
>>>> +	return -TRANSACTION_MAX_RETRIES;  
>>>
>>> so this means that a test will be considered failed if the transaction
>>> failed too many times?  
>>
>> Yes.
>>>
>>> this means that could fail if the test is run on busy system, even if
>>> the host running the unit test is correct  
>>
>> I suppose so, don't know how likely that is.
> 
> I don't like the idea of failing a test when the implementation is
> correct, just because the system might be a little more busy than
> expected.

Fair enough, I'll see what I can do.
> 
> if you can't find a way to refactor the test so that it doesn't fail if
> there are too many retries, then at least make it a skip?
> 
> but I'd really like to see something that does not fail on a correctly
> implemented system just because the test machine was too busy.
> 
>>>
>>> also, do you really need to use negative values? it's probably easier
>>> to read if you stick to positive values, and less prone to mistakes if
>>> you accidentally forget a - somewhere.  
>>
>> Ok.
>>>   
>>>> +}
>>>> +
>>>> +static void test_spec_ex_trans(struct args *args, const struct spec_ex_trigger *trigger)
>>>> +{
>>>> +	const uint16_t expected_pgm = PGM_INT_CODE_SPECIFICATION
>>>> +			      | PGM_INT_CODE_TX_ABORTED_EVENT;
>>>> +	union {
>>>> +		struct __htm_tdb tdb;
>>>> +		uint64_t dwords[sizeof(struct __htm_tdb) / sizeof(uint64_t)];
>>>> +	} diag;
>>>> +	unsigned int i, failures = 0;
>>>> +	int trans_result;
>>>> +
>>>> +	if (!test_facility(73)) {
>>>> +		report_skip("transactional-execution facility not installed");
>>>> +		return;
>>>> +	}
>>>> +	ctl_set_bit(0, CTL0_TRANSACT_EX_CTL); /* enable transactional-exec */
>>>> +
>>>> +	for (i = 0; i < args->iterations && failures <= args->max_failures; i++) {
>>>> +		register_pgm_cleanup_func(trigger->fixup);
>>>> +		trans_result = retry_transaction(trigger, args->max_retries, &diag.tdb, expected_pgm);  
>>>
>>> so you retry each iteration up to args->max_retries times, and if a
>>> transaction aborts too many times (maybe because the host system is
>>> very busy), then you consider it a fail
>>>   
>>
>> [...]
> 

Re: [kvm-unit-tests PATCH v3 1/2] s390x: Add specification exception test

[Thomas Huth]

On 26/10/2021 15.41, Claudio Imbrenda wrote:
> On Tue, 26 Oct 2021 14:00:31 +0200
> Janis Schoetterl-Glausch <scgl@linux.vnet.ibm.com> wrote:
[...]
>>> since you're ignoring the return value, can't you hardcode r6, and mark
>>> it (and r7) as clobbered? like:
>>> 		"lpq 6, %[bad]"
>>> 		: : [bad] "T"(words[1])
>>> 		: "%r6", "%r7"
>>>    
>> Ok, btw. is there a reason bare register numbers seem to be more common
>> compared to %%rN ?
> 
> I don't know, I guess laziness?

FWIW, older versions of Clang do not support bare register numbers on s390x, 
so it's better to use %%rN, AFAIK...
OTOH, we cannot compile the kvm-unit-tests with older versions of Clang 
anyway, so it likely doesn't matter here.

  Thomas