[kvm-unit-tests PATCH] s390x: Add specification exception test

[kvm-unit-tests PATCH] s390x: Add specification exception test

[Janis Schoetterl-Glausch]

Generate specification exceptions and check that they occur.
Also generate specification exceptions during a transaction,
which results in another interruption code.
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 +
 lib/s390x/asm/arch_def.h |   1 +
 s390x/spec_ex.c          | 344 +++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg      |   3 +
 4 files changed, 349 insertions(+)
 create mode 100644 s390x/spec_ex.c

diff --git a/s390x/Makefile b/s390x/Makefile
index 8820e99..be100d3 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -23,6 +23,7 @@ tests += $(TEST_DIR)/sie.elf
 tests += $(TEST_DIR)/mvpg.elf
 tests += $(TEST_DIR)/uv-host.elf
 tests += $(TEST_DIR)/edat.elf
+tests += $(TEST_DIR)/spec_ex.elf
 
 tests_binary = $(patsubst %.elf,%.bin,$(tests))
 ifneq ($(HOST_KEY_DOCUMENT),)
diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index 15cf7d4..7cb0b92 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -229,6 +229,7 @@ static inline uint64_t stctg(int cr)
 	return value;
 }
 
+#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
new file mode 100644
index 0000000..2e05bfb
--- /dev/null
+++ b/s390x/spec_ex.c
@@ -0,0 +1,344 @@
+#include <stdlib.h>
+#include <htmintrin.h>
+#include <libcflat.h>
+#include <asm/barrier.h>
+#include <asm/interrupt.h>
+#include <asm/facility.h>
+
+struct lowcore *lc = (struct lowcore *) 0;
+
+static bool expect_early;
+static struct psw expected_early_pgm_psw;
+static struct psw fixup_early_pgm_psw;
+
+static void fixup_early_pgm_ex(void)
+{
+	if (expect_early) {
+		report(expected_early_pgm_psw.mask == lc->pgm_old_psw.mask
+		       && expected_early_pgm_psw.addr == lc->pgm_old_psw.addr,
+		       "Early program new PSW as expected");
+		expect_early = false;
+	}
+	lc->pgm_old_psw = fixup_early_pgm_psw;
+}
+
+static void lpsw(uint64_t psw)
+{
+	uint32_t *high, *low;
+	uint64_t r0 = 0, r1 = 0;
+
+	high = (uint32_t *) &fixup_early_pgm_psw.mask;
+	low = high + 1;
+
+	asm volatile (
+		"	epsw	%0,%1\n"
+		"	st	%0,%[high]\n"
+		"	st	%1,%[low]\n"
+		"	larl	%0,nop%=\n"
+		"	stg	%0,%[addr]\n"
+		"	lpsw	%[psw]\n"
+		"nop%=:	nop\n"
+		: "+&r"(r0), "+&a"(r1), [high] "=&R"(*high), [low] "=&R"(*low)
+		, [addr] "=&R"(fixup_early_pgm_psw.addr)
+		: [psw] "Q"(psw)
+		: "cc", "memory"
+	);
+}
+
+static void psw_bit_31_32_are_1_0(void)
+{
+	uint64_t bad_psw = 0x000800015eadbeef;
+
+	//bit 12 gets inverted when extending to 128-bit PSW
+	expected_early_pgm_psw.mask = 0x0000000100000000;
+	expected_early_pgm_psw.addr = 0x000000005eadbeef;
+	expect_early = true;
+	lpsw(bad_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);
+	bool transactable;
+	void (*fixup)(void);
+};
+
+static const struct spec_ex_trigger spec_ex_triggers[] = {
+	{ "psw_bit_31_32_are_1_0", &psw_bit_31_32_are_1_0, false, &fixup_early_pgm_ex},
+	{ "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,
+			 const struct spec_ex_trigger *trigger)
+{
+	uint16_t expected_pgm = PGM_INT_CODE_SPECIFICATION;
+	uint16_t pgm;
+	unsigned int i;
+
+	register_pgm_cleanup_func(trigger->fixup);
+	for (i = 0; i < args->iterations; i++) {
+		expect_pgm_int();
+		trigger->func();
+		pgm = clear_pgm_int();
+		if (pgm != expected_pgm) {
+			report(0,
+			"Program interrupt: expected(%d) == received(%d)",
+			expected_pgm,
+			pgm);
+			return;
+		}
+	}
+	report(1,
+	"Program interrupt: always expected(%d) == received(%d)",
+	expected_pgm,
+	expected_pgm);
+}
+
+#define TRANSACTION_COMPLETED 4
+#define TRANSACTION_MAX_RETRIES 5
+
+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;
+}
+
+#define report_info_if(cond, fmt, ...)			\
+	do {						\
+		if (cond) {				\
+			report_info(fmt, ##__VA_ARGS__);\
+		}					\
+	} while (0)
+
+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 */
+
+	register_pgm_cleanup_func(trigger->fixup);
+	for (i = 0; i < args->iterations && failures <= args->max_failures; i++) {
+		trans_result = retry_transaction(trigger, args->max_retries, &diag.tdb, expected_pgm);
+		switch (trans_result) {
+		case 0:
+			continue;
+		case -_HTM_TBEGIN_INDETERMINATE:
+		case -_HTM_TBEGIN_PERSISTENT:
+			report_info_if(failures < args->suppress_info,
+				       "transaction failed with cc %d",
+				       -trans_result);
+			break;
+		case -_HTM_TBEGIN_TRANSIENT:
+			report(0,
+			       "Program interrupt: expected(%d) == received(%d)",
+			       expected_pgm,
+			       clear_pgm_int());
+			goto out;
+		case -TRANSACTION_COMPLETED:
+			report(0,
+			       "Transaction completed without exception");
+			goto out;
+		case -TRANSACTION_MAX_RETRIES:
+			report_info_if(failures < args->suppress_info,
+				       "Retried transaction %u times without exception",
+				       10);
+			break;
+		default:
+			report(0, "Invalid return transaction result");
+			goto out;
+		}
+
+		report_info_if(failures < args->suppress_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(1,
+		       "Program interrupt: always expected(%d) == received(%d), transaction failures: %u",
+		       expected_pgm,
+		       expected_pgm,
+		       failures);
+	} else {
+		report(0,
+		       "Too many transaction failures: %u", failures);
+	}
+	report_info_if(failures > args->suppress_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 parsed;
+	bool error, max_failures = false;
+	char *end;
+
+	for (i = 1; i < argc; i++) {
+		error = true;
+		if (i < argc - 1) {
+			error = *argv[i+1] == '\0';
+			parsed = strtol(argv[i+1], &end, 10);
+			error |= *end != '\0';
+			error |= parsed < 0;
+		}
+
+		if (!strcmp("--iterations", argv[i])) {
+			if (error)
+				report_abort("--iterations needs a positive parameter");
+			args.iterations = parsed;
+			++i;
+		} else if (!strcmp("--max-retries", argv[i])) {
+			if (error)
+				report_abort("--max-retries needs a positive parameter");
+			args.max_retries = parsed;
+			++i;
+		} else if (!strcmp("--suppress-info", argv[i])) {
+			if (error)
+				report_abort("--suppress-info needs a positive parameter");
+			args.suppress_info = parsed;
+			++i;
+		} else if (!strcmp("--max-failures", argv[i])) {
+			if (error)
+				report_abort("--max-failures needs a positive parameter");
+			args.max_failures = parsed;
+			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;
+}
+
+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();
+
+	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();
+}
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index a0ec886..b93aead 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -106,3 +106,6 @@ timeout = 10
 
 [edat]
 file = edat.elf
+
+[spec_ex]
+file = spec_ex.elf

base-commit: bc6f264386b4cb2cadc8b2492315f3e6e8a801a2
-- 
2.31.1

Re: [kvm-unit-tests PATCH] s390x: Add specification exception test

[Cornelia Huck]

On Tue, Jul 06 2021, Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

> Generate specification exceptions and check that they occur.
> Also generate specification exceptions during a transaction,
> which results in another interruption code.
> 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 +
>  lib/s390x/asm/arch_def.h |   1 +
>  s390x/spec_ex.c          | 344 +++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg      |   3 +
>  4 files changed, 349 insertions(+)
>  create mode 100644 s390x/spec_ex.c

(...)

> +static void lpsw(uint64_t psw)

Maybe call this load_psw(), as you do a bit more than a simple lpsw?

> +{
> +	uint32_t *high, *low;
> +	uint64_t r0 = 0, r1 = 0;
> +
> +	high = (uint32_t *) &fixup_early_pgm_psw.mask;
> +	low = high + 1;
> +
> +	asm volatile (
> +		"	epsw	%0,%1\n"
> +		"	st	%0,%[high]\n"
> +		"	st	%1,%[low]\n"
> +		"	larl	%0,nop%=\n"
> +		"	stg	%0,%[addr]\n"
> +		"	lpsw	%[psw]\n"
> +		"nop%=:	nop\n"
> +		: "+&r"(r0), "+&a"(r1), [high] "=&R"(*high), [low] "=&R"(*low)
> +		, [addr] "=&R"(fixup_early_pgm_psw.addr)
> +		: [psw] "Q"(psw)
> +		: "cc", "memory"
> +	);
> +}

(...)

> +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;
> +
> +	register_pgm_cleanup_func(trigger->fixup);
> +	for (i = 0; i < args->iterations; i++) {
> +		expect_pgm_int();
> +		trigger->func();
> +		pgm = clear_pgm_int();
> +		if (pgm != expected_pgm) {
> +			report(0,
> +			"Program interrupt: expected(%d) == received(%d)",
> +			expected_pgm,
> +			pgm);

The indentation looks a bit funny here.

> +			return;
> +		}
> +	}
> +	report(1,
> +	"Program interrupt: always expected(%d) == received(%d)",
> +	expected_pgm,
> +	expected_pgm);

Here as well.

> +}

(...)

> +#define report_info_if(cond, fmt, ...)			\
> +	do {						\
> +		if (cond) {				\
> +			report_info(fmt, ##__VA_ARGS__);\
> +		}					\
> +	} while (0)

I'm wondering whether such a wrapper function could be generally useful.

Re: [kvm-unit-tests PATCH] s390x: Add specification exception test

[Janis Schoetterl-Glausch]

On 7/9/21 11:22 AM, Cornelia Huck wrote:
> On Tue, Jul 06 2021, Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
> 
>> Generate specification exceptions and check that they occur.
>> Also generate specification exceptions during a transaction,
>> which results in another interruption code.
>> 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 +
>>  lib/s390x/asm/arch_def.h |   1 +
>>  s390x/spec_ex.c          | 344 +++++++++++++++++++++++++++++++++++++++
>>  s390x/unittests.cfg      |   3 +
>>  4 files changed, 349 insertions(+)
>>  create mode 100644 s390x/spec_ex.c
> 
> (...)
> 
>> +static void lpsw(uint64_t psw)
> 
> Maybe call this load_psw(), as you do a bit more than a simple lpsw?

[...]

> The indentation looks a bit funny here.

[...]

> Here as well.

Ok, will fix.
> 
>> +}
> 
> (...)
> 
>> +#define report_info_if(cond, fmt, ...)			\
>> +	do {						\
>> +		if (cond) {				\
>> +			report_info(fmt, ##__VA_ARGS__);\
>> +		}					\
>> +	} while (0)
> 
> I'm wondering whether such a wrapper function could be generally useful.
> 

I've found 9 occurrences with:
find . -type f \( -name "*.c" -o -name "*.h" \) -exec awk '/if\s*\(.*/{i=2;f=$0} /report_info/ && i>0{print FILENAME, NR-1 ":" f;r=4} r>1{print FILENAME, NR ":" $0;r--} r==1{print "--";r=0} {i--}' '{}' \;

./lib/s390x/css_lib.c 177:      if (cc) {
./lib/s390x/css_lib.c 178:              report_info("stsch: updating sch %08x failed with cc=%d",
./lib/s390x/css_lib.c 179:                          schid, cc);
./lib/s390x/css_lib.c 180:              return false;
--
./lib/s390x/css_lib.c 183:      if (!(pmcw->flags & PMCW_ENABLE)) {
./lib/s390x/css_lib.c 184:              report_info("stsch: sch %08x not enabled", schid);
./lib/s390x/css_lib.c 185:              return false;
./lib/s390x/css_lib.c 186:      }
--
./lib/s390x/css_lib.c 207:      if (cc) {
./lib/s390x/css_lib.c 208:              report_info("stsch: sch %08x failed with cc=%d", schid, cc);
./lib/s390x/css_lib.c 209:              return cc;
./lib/s390x/css_lib.c 210:      }
--
./lib/s390x/css_lib.c 213:      if ((pmcw->flags & (PMCW_ISC_MASK | PMCW_ENABLE)) == flags) {
./lib/s390x/css_lib.c 214:              report_info("stsch: sch %08x already enabled", schid);
./lib/s390x/css_lib.c 215:              return 0;
./lib/s390x/css_lib.c 216:      }
--
./lib/s390x/css_lib.c 269:      if (cc) {
./lib/s390x/css_lib.c 270:              report_info("stsch: sch %08x failed with cc=%d", schid, cc);
./lib/s390x/css_lib.c 271:              return false;
./lib/s390x/css_lib.c 272:      }
--
./lib/s390x/css_lib.c 305:      if (cc) {
./lib/s390x/css_lib.c 306:              report_info("stsch: updating sch %08x failed with cc=%d",
./lib/s390x/css_lib.c 307:                          schid, cc);
./lib/s390x/css_lib.c 308:              return false;
--
./lib/s390x/css_lib.c 466:      if (irb.scsw.sch_stat & ~SCSW_SCHS_IL) {
./lib/s390x/css_lib.c 467:              report_info("Unexpected Subch. status %02x", irb.scsw.sch_stat);
./lib/s390x/css_lib.c 468:              ret = -1;
./lib/s390x/css_lib.c 469:              goto end;
--
./s390x/sclp.c 80:      if (res) {
./s390x/sclp.c 81:              report_info("SCLP not ready (command %#x, address %p, cc %d)", cmd, addr, res);
./s390x/sclp.c 82:              return false;
./s390x/sclp.c 83:      }
--
./s390x/sclp.c 86:      if (!((1ULL << pgm) & exp_pgm)) {
./s390x/sclp.c 87:              report_info("First failure at addr %p, buf_len %d, cmd %#x, pgm code %d",
./s390x/sclp.c 88:                              addr, buf_len, cmd, pgm);
./s390x/sclp.c 89:              return false;
--
./s390x/sclp.c 92:      if (exp_rc && exp_rc != h->response_code) {
./s390x/sclp.c 93:              report_info("First failure at addr %p, buf_len %d, cmd %#x, resp code %#x",
./s390x/sclp.c 94:                              addr, buf_len, cmd, h->response_code);
./s390x/sclp.c 95:              return false;
--
./s390x/css.c 105:      if (ret < 0) {
./s390x/css.c 106:              report_info("no valid residual count");
./s390x/css.c 107:      } else if (ret != 0) {
./s390x/css.c 108:              len = sizeof(*senseid) - ret;
--
./s390x/css.c 112:              } else if (ret && len)
./s390x/css.c 113:                      report_info("transferred a shorter length: %d", len);
./s390x/css.c 114:      }
./s390x/css.c 115:
--
./s390x/css.c 153:      if (css_test_general_feature(CSSC_EXTENDED_MEASUREMENT_BLOCK))
./s390x/css.c 154:              report_info("Extended measurement block available");
./s390x/css.c 155:
./s390x/css.c 156:      /* bits 59-63 of MB address must be 0  if MBU is defined */
--
./arm/psci.c 119:               } else if (cpu_on_ret[cpu] != PSCI_RET_ALREADY_ON) {
./arm/psci.c 120:                       report_info("unexpected cpu_on return value: caller=CPU%d, ret=%d", cpu, cpu_on_ret[cpu]);
./arm/psci.c 121:                       failed = true;
./arm/psci.c 122:               }
--
./arm/psci.c 125:       if (ret_success != 1) {
./arm/psci.c 126:               report_info("got %d CPU_ON success", ret_success);
./arm/psci.c 127:               failed = true;
./arm/psci.c 128:       }
--
./arm/pmu.c 236:        if (!supported && warn)
./arm/pmu.c 237:                report_info("event 0x%x is not supported", n);
./arm/pmu.c 238:        return supported;
./arm/pmu.c 239:}
--
./arm/cache.c 115:      if (dcache_clean)
./arm/cache.c 116:              report_info("dcache clean to PoU required");
./arm/cache.c 117:      if (icache_inval)
./arm/cache.c 117:      if (icache_inval)
./arm/cache.c 118:              report_info("icache invalidation to PoU required");
./arm/cache.c 119:
./arm/cache.c 120:      check_code_generation(dcache_clean, icache_inval);
--
./arm/pl031.c 193:      if (!irq_triggered) {
./arm/pl031.c 194:              report_info("  RTC RIS: %"PRIx32, readl(&pl031->ris));
./arm/pl031.c 195:              report_info("  RTC MIS: %"PRIx32, readl(&pl031->mis));
./arm/pl031.c 196:              report_info("  RTC IMSC: %"PRIx32, readl(&pl031->imsc));
--
./arm/gic.c 84:                 if (i)
./arm/gic.c 85:                         report_info("interrupts took more than %d ms", i * 100);
./arm/gic.c 86:                 /* Wait for unexpected interrupts to fire */
./arm/gic.c 87:                 mdelay(100);
--
./arm/gic.c 115:                if (has_gicv2 && irq_sender[cpu] != sender) {
./arm/gic.c 116:                        report_info("cpu%d received IPI from wrong sender %d",
./arm/gic.c 117:                                        cpu, irq_sender[cpu]);
./arm/gic.c 118:                        pass = false;
--
./arm/gic.c 121:                if (irq_number[cpu] != irqnum) {
./arm/gic.c 122:                        report_info("cpu%d received wrong irq %d",
./arm/gic.c 123:                                        cpu, irq_number[cpu]);
./arm/gic.c 124:                        pass = false;
--
./arm/gic.c 128:        if (missing || extra || unexpected) {
./arm/gic.c 129:                report_info("ACKS: missing=%d extra=%d unexpected=%d",
./arm/gic.c 130:                                missing, extra, unexpected);
./arm/gic.c 131:                pass = false;
--
./arm/gic.c 142:                if (spurious[cpu])
./arm/gic.c 143:                        report_info("WARN: cpu%d got %d spurious interrupts",
./arm/gic.c 144:                                cpu, spurious[cpu]);
./arm/gic.c 145:        }
--
./arm/gic.c 194:                if (acked[i] != expected[i]) {
./arm/gic.c 195:                        report_info("expected %d LPIs on PE #%d, %d observed",
./arm/gic.c 196:                                    expected[i], i, acked[i]);
./arm/gic.c 197:                        pass = false;
--
./arm/gic.c 421:        if (!res)
./arm/gic.c 422:                report_info("byte 1 of 0x%08"PRIx32" => 0x%02"PRIx32, pattern & mask, reg);
./arm/gic.c 423:
./arm/gic.c 424:        pattern = REPLACE_BYTE(pattern, 2, 0x1f);
--
./arm/gic.c 429:        if (!res)
./arm/gic.c 430:                report_info("writing 0x%02"PRIx32" into bytes 2 => 0x%08"PRIx32,
./arm/gic.c 431:                            BYTE(pattern, 2), reg);
./arm/gic.c 432:}
--
./arm/gic.c 519:        if (reg != (pattern & cpu_mask))
./arm/gic.c 520:                report_info("writing %08"PRIx32" reads back as %08"PRIx32,
./arm/gic.c 521:                            pattern & cpu_mask, reg);
./arm/gic.c 522:
--
./x86/vmx_tests.c 4733: if (!(ctrl_cpu_rev[0].clr & CPU_NMI_WINDOW)) {
./x86/vmx_tests.c 4734:         report_info("NMI-window exiting is not supported, skipping...");
./x86/vmx_tests.c 4735:         goto done;
./x86/vmx_tests.c 4736: }
--
./x86/vmx_tests.c 4841:                 if (un_cache) {
./x86/vmx_tests.c 4842:                         report_info("EPT paging structure memory-type is Un-cacheable\n");
./x86/vmx_tests.c 4843:                         ctrl = true;
./x86/vmx_tests.c 4844:                 } else {
--
./x86/vmx_tests.c 4848:                 if (wr_bk) {
./x86/vmx_tests.c 4849:                         report_info("EPT paging structure memory-type is Write-back\n");
./x86/vmx_tests.c 4850:                         ctrl = true;
./x86/vmx_tests.c 4851:                 } else {
--
./x86/vmx_tests.c 4899: if (msr & EPT_CAP_AD_FLAG) {
./x86/vmx_tests.c 4900:         report_info("Processor supports accessed and dirty flag");
./x86/vmx_tests.c 4901:         eptp &= ~EPTP_AD_FLAG;
./x86/vmx_tests.c 4902:         test_eptp_ad_bit(eptp, true);
--

Re: [kvm-unit-tests PATCH] s390x: Add specification exception test

[Janosch Frank]

On 7/9/21 4:22 PM, Janis Schoetterl-Glausch wrote:
> On 7/9/21 11:22 AM, Cornelia Huck wrote:
>> On Tue, Jul 06 2021, Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
>>
>>> Generate specification exceptions and check that they occur.
>>> Also generate specification exceptions during a transaction,
>>> which results in another interruption code.
>>> 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 +
>>>  lib/s390x/asm/arch_def.h |   1 +
>>>  s390x/spec_ex.c          | 344 +++++++++++++++++++++++++++++++++++++++
>>>  s390x/unittests.cfg      |   3 +
>>>  4 files changed, 349 insertions(+)
>>>  create mode 100644 s390x/spec_ex.c
>>
>> (...)
>>
>>> +static void lpsw(uint64_t psw)
>>
>> Maybe call this load_psw(), as you do a bit more than a simple lpsw?
> 
> [...]
> 
>> The indentation looks a bit funny here.
> 
> [...]
> 
>> Here as well.
> 
> Ok, will fix.
>>
>>> +}
>>
>> (...)
>>
>>> +#define report_info_if(cond, fmt, ...)			\
>>> +	do {						\
>>> +		if (cond) {				\
>>> +			report_info(fmt, ##__VA_ARGS__);\
>>> +		}					\
>>> +	} while (0)
>>
>> I'm wondering whether such a wrapper function could be generally useful.
>>
> 
> I've found 9 occurrences with:
> find . -type f \( -name "*.c" -o -name "*.h" \) -exec awk '/if\s*\(.*/{i=2;f=$0} /report_info/ && i>0{print FILENAME, NR-1 ":" f;r=4} r>1{print FILENAME, NR ":" $0;r--} r==1{print "--";r=0} {i--}' '{}' \;
> 

Looking at the occurrences below most of those also do other things in
the conditional branches so for 90% we can't do a straight forward replace.

Nevertheless I see some value in it and the 6 lines won't hurt us, so
please create a separate patch for this and put the maintainers for the
other arches and Paolo on CC for that patch so they know the new wrapper
will be available.

Also I'd like having report_fail("Message"); and report_pass("Message");
functions instead of report(0, "Message"); and report(1, "Message"); ...
Those at least are straight forward replacements.


> ./lib/s390x/css_lib.c 177:      if (cc) {
> ./lib/s390x/css_lib.c 178:              report_info("stsch: updating sch %08x failed with cc=%d",
> ./lib/s390x/css_lib.c 179:                          schid, cc);
> ./lib/s390x/css_lib.c 180:              return false;
> --
> ./lib/s390x/css_lib.c 183:      if (!(pmcw->flags & PMCW_ENABLE)) {
> ./lib/s390x/css_lib.c 184:              report_info("stsch: sch %08x not enabled", schid);
> ./lib/s390x/css_lib.c 185:              return false;
> ./lib/s390x/css_lib.c 186:      }
> --
> ./lib/s390x/css_lib.c 207:      if (cc) {
> ./lib/s390x/css_lib.c 208:              report_info("stsch: sch %08x failed with cc=%d", schid, cc);
> ./lib/s390x/css_lib.c 209:              return cc;
> ./lib/s390x/css_lib.c 210:      }
> --
> ./lib/s390x/css_lib.c 213:      if ((pmcw->flags & (PMCW_ISC_MASK | PMCW_ENABLE)) == flags) {
> ./lib/s390x/css_lib.c 214:              report_info("stsch: sch %08x already enabled", schid);
> ./lib/s390x/css_lib.c 215:              return 0;
> ./lib/s390x/css_lib.c 216:      }
> --
> ./lib/s390x/css_lib.c 269:      if (cc) {
> ./lib/s390x/css_lib.c 270:              report_info("stsch: sch %08x failed with cc=%d", schid, cc);
> ./lib/s390x/css_lib.c 271:              return false;
> ./lib/s390x/css_lib.c 272:      }
> --
[...]

Re: [kvm-unit-tests PATCH] s390x: Add specification exception test

[Thomas Huth]

On 06/07/2021 13.54, Janis Schoetterl-Glausch wrote:
> Generate specification exceptions and check that they occur.
> Also generate specification exceptions during a transaction,
> which results in another interruption code.
> 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 +
>   lib/s390x/asm/arch_def.h |   1 +
>   s390x/spec_ex.c          | 344 +++++++++++++++++++++++++++++++++++++++
>   s390x/unittests.cfg      |   3 +
>   4 files changed, 349 insertions(+)
>   create mode 100644 s390x/spec_ex.c
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 8820e99..be100d3 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -23,6 +23,7 @@ tests += $(TEST_DIR)/sie.elf
>   tests += $(TEST_DIR)/mvpg.elf
>   tests += $(TEST_DIR)/uv-host.elf
>   tests += $(TEST_DIR)/edat.elf
> +tests += $(TEST_DIR)/spec_ex.elf
>   
>   tests_binary = $(patsubst %.elf,%.bin,$(tests))
>   ifneq ($(HOST_KEY_DOCUMENT),)
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 15cf7d4..7cb0b92 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -229,6 +229,7 @@ static inline uint64_t stctg(int cr)
>   	return value;
>   }
>   
> +#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
> new file mode 100644
> index 0000000..2e05bfb
> --- /dev/null
> +++ b/s390x/spec_ex.c
> @@ -0,0 +1,344 @@

Please add a short comment header at the top of the file with some 
information on what it is all about, and license information (e.g. a 
SPDX-License-Identifier)

> +#include <stdlib.h>
> +#include <htmintrin.h>
> +#include <libcflat.h>
> +#include <asm/barrier.h>
> +#include <asm/interrupt.h>
> +#include <asm/facility.h>
> +
> +struct lowcore *lc = (struct lowcore *) 0;
> +
> +static bool expect_early;
> +static struct psw expected_early_pgm_psw;
> +static struct psw fixup_early_pgm_psw;
> +
> +static void fixup_early_pgm_ex(void)

Could you please add a comment in front of this function with a description 
why this is required / good for?

> +{
> +	if (expect_early) {
> +		report(expected_early_pgm_psw.mask == lc->pgm_old_psw.mask
> +		       && expected_early_pgm_psw.addr == lc->pgm_old_psw.addr,
> +		       "Early program new PSW as expected");
> +		expect_early = false;
> +	}
> +	lc->pgm_old_psw = fixup_early_pgm_psw;
> +}
> +
> +static void lpsw(uint64_t psw)
> +{
> +	uint32_t *high, *low;
> +	uint64_t r0 = 0, r1 = 0;
> +
> +	high = (uint32_t *) &fixup_early_pgm_psw.mask;
> +	low = high + 1;
> +
> +	asm volatile (
> +		"	epsw	%0,%1\n"
> +		"	st	%0,%[high]\n"
> +		"	st	%1,%[low]\n"

What's all this magic with high and low good for? Looks like high and low 
are not used afterwards anymore?

> +		"	larl	%0,nop%=\n"
> +		"	stg	%0,%[addr]\n"
> +		"	lpsw	%[psw]\n"
> +		"nop%=:	nop\n"
> +		: "+&r"(r0), "+&a"(r1), [high] "=&R"(*high), [low] "=&R"(*low)

... also not sure why you need the "&" modifiers here?

> +		, [addr] "=&R"(fixup_early_pgm_psw.addr)
> +		: [psw] "Q"(psw)
> +		: "cc", "memory"
> +	);
> +}
> +
> +static void psw_bit_31_32_are_1_0(void)
> +{
> +	uint64_t bad_psw = 0x000800015eadbeef;
> +
> +	//bit 12 gets inverted when extending to 128-bit PSW

I'd prefer a space after the "//"

> +	expected_early_pgm_psw.mask = 0x0000000100000000;
> +	expected_early_pgm_psw.addr = 0x000000005eadbeef;
> +	expect_early = true;
> +	lpsw(bad_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);
> +	bool transactable;
> +	void (*fixup)(void);
> +};
> +
> +static const struct spec_ex_trigger spec_ex_triggers[] = {
> +	{ "psw_bit_31_32_are_1_0", &psw_bit_31_32_are_1_0, false, &fixup_early_pgm_ex},
> +	{ "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,
> +			 const struct spec_ex_trigger *trigger)
> +{
> +	uint16_t expected_pgm = PGM_INT_CODE_SPECIFICATION;
> +	uint16_t pgm;
> +	unsigned int i;
> +
> +	register_pgm_cleanup_func(trigger->fixup);
> +	for (i = 0; i < args->iterations; i++) {
> +		expect_pgm_int();
> +		trigger->func();
> +		pgm = clear_pgm_int();
> +		if (pgm != expected_pgm) {
> +			report(0,
> +			"Program interrupt: expected(%d) == received(%d)",
> +			expected_pgm,
> +			pgm);
> +			return;
> +		}
> +	}

Maybe it would be nice to "unregister" the cleanup function at the end with 
register_pgm_cleanup_func(NULL) ?

> +	report(1,
> +	"Program interrupt: always expected(%d) == received(%d)",
> +	expected_pgm,
> +	expected_pgm);
> +}
> +
> +#define TRANSACTION_COMPLETED 4
> +#define TRANSACTION_MAX_RETRIES 5
> +
> +static int __attribute__((nonnull))

Not sure whether that attribute makes much sense with a static function? ... 
the compiler has information about the implementation details here, so it 
should be able to see that e.g. trigger must be non-NULL anyway?

> +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;
> +	}
> +}
[...]

  Thomas

Re: [kvm-unit-tests PATCH] s390x: Add specification exception test

[Janis Schoetterl-Glausch]

On 7/21/21 3:26 PM, Thomas Huth wrote:
> On 06/07/2021 13.54, Janis Schoetterl-Glausch wrote:
>> Generate specification exceptions and check that they occur.
>> Also generate specification exceptions during a transaction,
>> which results in another interruption code.
>> 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 +
>>   lib/s390x/asm/arch_def.h |   1 +
>>   s390x/spec_ex.c          | 344 +++++++++++++++++++++++++++++++++++++++
>>   s390x/unittests.cfg      |   3 +
>>   4 files changed, 349 insertions(+)
>>   create mode 100644 s390x/spec_ex.c
>>
>> diff --git a/s390x/Makefile b/s390x/Makefile
>> index 8820e99..be100d3 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -23,6 +23,7 @@ tests += $(TEST_DIR)/sie.elf
>>   tests += $(TEST_DIR)/mvpg.elf
>>   tests += $(TEST_DIR)/uv-host.elf
>>   tests += $(TEST_DIR)/edat.elf
>> +tests += $(TEST_DIR)/spec_ex.elf
>>     tests_binary = $(patsubst %.elf,%.bin,$(tests))
>>   ifneq ($(HOST_KEY_DOCUMENT),)
>> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
>> index 15cf7d4..7cb0b92 100644
>> --- a/lib/s390x/asm/arch_def.h
>> +++ b/lib/s390x/asm/arch_def.h
>> @@ -229,6 +229,7 @@ static inline uint64_t stctg(int cr)
>>       return value;
>>   }
>>   +#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
>> new file mode 100644
>> index 0000000..2e05bfb
>> --- /dev/null
>> +++ b/s390x/spec_ex.c
>> @@ -0,0 +1,344 @@
> 
> Please add a short comment header at the top of the file with some information on what it is all about, and license information (e.g. a SPDX-License-Identifier)
> 
>> +#include <stdlib.h>
>> +#include <htmintrin.h>
>> +#include <libcflat.h>
>> +#include <asm/barrier.h>
>> +#include <asm/interrupt.h>
>> +#include <asm/facility.h>
>> +
>> +struct lowcore *lc = (struct lowcore *) 0;
>> +
>> +static bool expect_early;
>> +static struct psw expected_early_pgm_psw;
>> +static struct psw fixup_early_pgm_psw;
>> +
>> +static void fixup_early_pgm_ex(void)
> 
> Could you please add a comment in front of this function with a description why this is required / good for?

Sure, how about:

/* 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.
 */

I'll also change some names since something like this is necessary for all
exceptions caused by invalid PSWs, not just the early ones:

static void fixup_invalid_psw(void)
> 
>> +{
>> +    if (expect_early) {
>> +        report(expected_early_pgm_psw.mask == lc->pgm_old_psw.mask
>> +               && expected_early_pgm_psw.addr == lc->pgm_old_psw.addr,
>> +               "Early program new PSW as expected");
>> +        expect_early = false;
>> +    }
>> +    lc->pgm_old_psw = fixup_early_pgm_psw;
>> +}
>> +
>> +static void lpsw(uint64_t psw)
>> +{
>> +    uint32_t *high, *low;
>> +    uint64_t r0 = 0, r1 = 0;
>> +
>> +    high = (uint32_t *) &fixup_early_pgm_psw.mask;
>> +    low = high + 1;
>> +
>> +    asm volatile (
>> +        "    epsw    %0,%1\n"
>> +        "    st    %0,%[high]\n"
>> +        "    st    %1,%[low]\n"
> 
> What's all this magic with high and low good for? Looks like high and low are not used afterwards anymore?

Seems like the easiest way to store both halves of the current mask into the global fixup PSW.
> 
>> +        "    larl    %0,nop%=\n"
>> +        "    stg    %0,%[addr]\n"
>> +        "    lpsw    %[psw]\n"
>> +        "nop%=:    nop\n"
>> +        : "+&r"(r0), "+&a"(r1), [high] "=&R"(*high), [low] "=&R"(*low)
> 
> ... also not sure why you need the "&" modifiers here?

r0, r1 are stored into before reading psw, also there are implied input registers for the
memory output operands. To be honest, I didn't care to figure out the minimal '&' usage,
it's just test code after all.
> 
>> +        , [addr] "=&R"(fixup_early_pgm_psw.addr)
>> +        : [psw] "Q"(psw)
>> +        : "cc", "memory"
>> +    );
>> +}
>> +
>> +static void psw_bit_31_32_are_1_0(void)
>> +{
>> +    uint64_t bad_psw = 0x000800015eadbeef;
>> +
>> +    //bit 12 gets inverted when extending to 128-bit PSW
> 
> I'd prefer a space after the "//"
> 
>> +    expected_early_pgm_psw.mask = 0x0000000100000000;
>> +    expected_early_pgm_psw.addr = 0x000000005eadbeef;
>> +    expect_early = true;
>> +    lpsw(bad_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);
>> +    bool transactable;
>> +    void (*fixup)(void);
>> +};
>> +
>> +static const struct spec_ex_trigger spec_ex_triggers[] = {
>> +    { "psw_bit_31_32_are_1_0", &psw_bit_31_32_are_1_0, false, &fixup_early_pgm_ex},
>> +    { "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,
>> +             const struct spec_ex_trigger *trigger)
>> +{
>> +    uint16_t expected_pgm = PGM_INT_CODE_SPECIFICATION;
>> +    uint16_t pgm;
>> +    unsigned int i;
>> +
>> +    register_pgm_cleanup_func(trigger->fixup);
>> +    for (i = 0; i < args->iterations; i++) {
>> +        expect_pgm_int();
>> +        trigger->func();
>> +        pgm = clear_pgm_int();
>> +        if (pgm != expected_pgm) {
>> +            report(0,
>> +            "Program interrupt: expected(%d) == received(%d)",
>> +            expected_pgm,
>> +            pgm);
>> +            return;
>> +        }
>> +    }
> 
> Maybe it would be nice to "unregister" the cleanup function at the end with register_pgm_cleanup_func(NULL) ?

Yeah, I think I'll also move them just before and after the trigger->func().
> 
>> +    report(1,
>> +    "Program interrupt: always expected(%d) == received(%d)",
>> +    expected_pgm,
>> +    expected_pgm);
>> +}
>> +
>> +#define TRANSACTION_COMPLETED 4
>> +#define TRANSACTION_MAX_RETRIES 5
>> +
>> +static int __attribute__((nonnull))
> 
> Not sure whether that attribute makes much sense with a static function? ... the compiler has information about the implementation details here, so it should be able to see that e.g. trigger must be non-NULL anyway?

One isn't supposed to pass NULL to __builtin_tbegin via a variable, only via a constant.
I didn't want to deal with that constraint, so that's what the nonnull is there for.
Maybe I should add a comment?
> 
>> +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;
>> +    }
>> +}
> [...]
> 
>  Thomas
> 
Thank you for your comments.

Re: [kvm-unit-tests PATCH] s390x: Add specification exception test

[Thomas Huth]

On 21/07/2021 17.44, Janis Schoetterl-Glausch wrote:
> On 7/21/21 3:26 PM, Thomas Huth wrote:
>> On 06/07/2021 13.54, Janis Schoetterl-Glausch wrote:
>>> Generate specification exceptions and check that they occur.
>>> Also generate specification exceptions during a transaction,
>>> which results in another interruption code.
>>> 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 +
>>>    lib/s390x/asm/arch_def.h |   1 +
>>>    s390x/spec_ex.c          | 344 +++++++++++++++++++++++++++++++++++++++
>>>    s390x/unittests.cfg      |   3 +
>>>    4 files changed, 349 insertions(+)
>>>    create mode 100644 s390x/spec_ex.c
>>>
>>> diff --git a/s390x/Makefile b/s390x/Makefile
>>> index 8820e99..be100d3 100644
>>> --- a/s390x/Makefile
>>> +++ b/s390x/Makefile
>>> @@ -23,6 +23,7 @@ tests += $(TEST_DIR)/sie.elf
>>>    tests += $(TEST_DIR)/mvpg.elf
>>>    tests += $(TEST_DIR)/uv-host.elf
>>>    tests += $(TEST_DIR)/edat.elf
>>> +tests += $(TEST_DIR)/spec_ex.elf
>>>      tests_binary = $(patsubst %.elf,%.bin,$(tests))
>>>    ifneq ($(HOST_KEY_DOCUMENT),)
>>> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
>>> index 15cf7d4..7cb0b92 100644
>>> --- a/lib/s390x/asm/arch_def.h
>>> +++ b/lib/s390x/asm/arch_def.h
>>> @@ -229,6 +229,7 @@ static inline uint64_t stctg(int cr)
>>>        return value;
>>>    }
>>>    +#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
>>> new file mode 100644
>>> index 0000000..2e05bfb
>>> --- /dev/null
>>> +++ b/s390x/spec_ex.c
>>> @@ -0,0 +1,344 @@
>>
>> Please add a short comment header at the top of the file with some information on what it is all about, and license information (e.g. a SPDX-License-Identifier)
>>
>>> +#include <stdlib.h>
>>> +#include <htmintrin.h>
>>> +#include <libcflat.h>
>>> +#include <asm/barrier.h>
>>> +#include <asm/interrupt.h>
>>> +#include <asm/facility.h>
>>> +
>>> +struct lowcore *lc = (struct lowcore *) 0;
>>> +
>>> +static bool expect_early;
>>> +static struct psw expected_early_pgm_psw;
>>> +static struct psw fixup_early_pgm_psw;
>>> +
>>> +static void fixup_early_pgm_ex(void)
>>
>> Could you please add a comment in front of this function with a description why this is required / good for?
> 
> Sure, how about:
> 
> /* 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.
>   */
> 
> I'll also change some names since something like this is necessary for all
> exceptions caused by invalid PSWs, not just the early ones:
> 
> static void fixup_invalid_psw(void)
>>
>>> +{
>>> +    if (expect_early) {
>>> +        report(expected_early_pgm_psw.mask == lc->pgm_old_psw.mask
>>> +               && expected_early_pgm_psw.addr == lc->pgm_old_psw.addr,
>>> +               "Early program new PSW as expected");
>>> +        expect_early = false;
>>> +    }
>>> +    lc->pgm_old_psw = fixup_early_pgm_psw;
>>> +}
>>> +
>>> +static void lpsw(uint64_t psw)
>>> +{
>>> +    uint32_t *high, *low;
>>> +    uint64_t r0 = 0, r1 = 0;
>>> +
>>> +    high = (uint32_t *) &fixup_early_pgm_psw.mask;
>>> +    low = high + 1;
>>> +
>>> +    asm volatile (
>>> +        "    epsw    %0,%1\n"
>>> +        "    st    %0,%[high]\n"
>>> +        "    st    %1,%[low]\n"
>>
>> What's all this magic with high and low good for? Looks like high and low are not used afterwards anymore?
> 
> Seems like the easiest way to store both halves of the current mask into the global fixup PSW.

Ok, thanks, now I got it. But I think it would be easier to understand if 
you'd only pass the address of fixup_early_pgm_psw.mask to the assembly code 
and then do e.g. a "st %1,4(%[mask])" instead.

>>> +        "    larl    %0,nop%=\n"
>>> +        "    stg    %0,%[addr]\n"
>>> +        "    lpsw    %[psw]\n"
>>> +        "nop%=:    nop\n"
>>> +        : "+&r"(r0), "+&a"(r1), [high] "=&R"(*high), [low] "=&R"(*low)
>>
>> ... also not sure why you need the "&" modifiers here?
> 
> r0, r1 are stored into before reading psw, also there are implied input registers for the
> memory output operands. To be honest, I didn't care to figure out the minimal '&' usage,
> it's just test code after all.

Ok, fair point, makes sense now, too.

>>> +        , [addr] "=&R"(fixup_early_pgm_psw.addr)
>>> +        : [psw] "Q"(psw)
>>> +        : "cc", "memory"
>>> +    );
>>> +}
>>> +
>>> +static void psw_bit_31_32_are_1_0(void)
>>> +{
>>> +    uint64_t bad_psw = 0x000800015eadbeef;
>>> +
>>> +    //bit 12 gets inverted when extending to 128-bit PSW
>>
>> I'd prefer a space after the "//"
>>
>>> +    expected_early_pgm_psw.mask = 0x0000000100000000;
>>> +    expected_early_pgm_psw.addr = 0x000000005eadbeef;
>>> +    expect_early = true;
>>> +    lpsw(bad_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);
>>> +    bool transactable;
>>> +    void (*fixup)(void);
>>> +};
>>> +
>>> +static const struct spec_ex_trigger spec_ex_triggers[] = {
>>> +    { "psw_bit_31_32_are_1_0", &psw_bit_31_32_are_1_0, false, &fixup_early_pgm_ex},
>>> +    { "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,
>>> +             const struct spec_ex_trigger *trigger)
>>> +{
>>> +    uint16_t expected_pgm = PGM_INT_CODE_SPECIFICATION;
>>> +    uint16_t pgm;
>>> +    unsigned int i;
>>> +
>>> +    register_pgm_cleanup_func(trigger->fixup);
>>> +    for (i = 0; i < args->iterations; i++) {
>>> +        expect_pgm_int();
>>> +        trigger->func();
>>> +        pgm = clear_pgm_int();
>>> +        if (pgm != expected_pgm) {
>>> +            report(0,
>>> +            "Program interrupt: expected(%d) == received(%d)",
>>> +            expected_pgm,
>>> +            pgm);
>>> +            return;
>>> +        }
>>> +    }
>>
>> Maybe it would be nice to "unregister" the cleanup function at the end with register_pgm_cleanup_func(NULL) ?
> 
> Yeah, I think I'll also move them just before and after the trigger->func().
>>
>>> +    report(1,
>>> +    "Program interrupt: always expected(%d) == received(%d)",
>>> +    expected_pgm,
>>> +    expected_pgm);
>>> +}
>>> +
>>> +#define TRANSACTION_COMPLETED 4
>>> +#define TRANSACTION_MAX_RETRIES 5
>>> +
>>> +static int __attribute__((nonnull))
>>
>> Not sure whether that attribute makes much sense with a static function? ... the compiler has information about the implementation details here, so it should be able to see that e.g. trigger must be non-NULL anyway?
> 
> One isn't supposed to pass NULL to __builtin_tbegin via a variable, only via a constant.
> I didn't want to deal with that constraint, so that's what the nonnull is there for.
> Maybe I should add a comment?

Yes, that would be helpful.

  Thomas

Re: [kvm-unit-tests PATCH] s390x: Add specification exception test

[Janosch Frank]

On 7/6/21 1:54 PM, Janis Schoetterl-Glausch wrote:
> Generate specification exceptions and check that they occur.
> Also generate specification exceptions during a transaction,
> which results in another interruption code.
> 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>

Before you send out the next version could you please split up the
transactional and non-transactional code into separate patches?

350 non-trivial lines are a bit much for one patch :)

> ---
>  s390x/Makefile           |   1 +
>  lib/s390x/asm/arch_def.h |   1 +
>  s390x/spec_ex.c          | 344 +++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg      |   3 +
>  4 files changed, 349 insertions(+)
>  create mode 100644 s390x/spec_ex.c
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 8820e99..be100d3 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -23,6 +23,7 @@ tests += $(TEST_DIR)/sie.elf
>  tests += $(TEST_DIR)/mvpg.elf
>  tests += $(TEST_DIR)/uv-host.elf
>  tests += $(TEST_DIR)/edat.elf
> +tests += $(TEST_DIR)/spec_ex.elf
>  
>  tests_binary = $(patsubst %.elf,%.bin,$(tests))
>  ifneq ($(HOST_KEY_DOCUMENT),)
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 15cf7d4..7cb0b92 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -229,6 +229,7 @@ static inline uint64_t stctg(int cr)
>  	return value;
>  }
>  
> +#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
> new file mode 100644
> index 0000000..2e05bfb
> --- /dev/null
> +++ b/s390x/spec_ex.c
> @@ -0,0 +1,344 @@
> +#include <stdlib.h>
> +#include <htmintrin.h>
> +#include <libcflat.h>
> +#include <asm/barrier.h>
> +#include <asm/interrupt.h>
> +#include <asm/facility.h>
> +
> +struct lowcore *lc = (struct lowcore *) 0;
> +
> +static bool expect_early;
> +static struct psw expected_early_pgm_psw;
> +static struct psw fixup_early_pgm_psw;
> +
> +static void fixup_early_pgm_ex(void)
> +{
> +	if (expect_early) {
> +		report(expected_early_pgm_psw.mask == lc->pgm_old_psw.mask
> +		       && expected_early_pgm_psw.addr == lc->pgm_old_psw.addr,
> +		       "Early program new PSW as expected");
> +		expect_early = false;
> +	}
> +	lc->pgm_old_psw = fixup_early_pgm_psw;
> +}
> +
> +static void lpsw(uint64_t psw)
> +{
> +	uint32_t *high, *low;
> +	uint64_t r0 = 0, r1 = 0;
> +
> +	high = (uint32_t *) &fixup_early_pgm_psw.mask;
> +	low = high + 1;
> +
> +	asm volatile (
> +		"	epsw	%0,%1\n"
> +		"	st	%0,%[high]\n"
> +		"	st	%1,%[low]\n"
> +		"	larl	%0,nop%=\n"
> +		"	stg	%0,%[addr]\n"
> +		"	lpsw	%[psw]\n"
> +		"nop%=:	nop\n"
> +		: "+&r"(r0), "+&a"(r1), [high] "=&R"(*high), [low] "=&R"(*low)
> +		, [addr] "=&R"(fixup_early_pgm_psw.addr)
> +		: [psw] "Q"(psw)
> +		: "cc", "memory"
> +	);
> +}
> +
> +static void psw_bit_31_32_are_1_0(void)
> +{
> +	uint64_t bad_psw = 0x000800015eadbeef;
> +
> +	//bit 12 gets inverted when extending to 128-bit PSW
> +	expected_early_pgm_psw.mask = 0x0000000100000000;
> +	expected_early_pgm_psw.addr = 0x000000005eadbeef;
> +	expect_early = true;
> +	lpsw(bad_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);
> +	bool transactable;
> +	void (*fixup)(void);
> +};
> +
> +static const struct spec_ex_trigger spec_ex_triggers[] = {
> +	{ "psw_bit_31_32_are_1_0", &psw_bit_31_32_are_1_0, false, &fixup_early_pgm_ex},
> +	{ "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,
> +			 const struct spec_ex_trigger *trigger)
> +{
> +	uint16_t expected_pgm = PGM_INT_CODE_SPECIFICATION;
> +	uint16_t pgm;
> +	unsigned int i;
> +
> +	register_pgm_cleanup_func(trigger->fixup);
> +	for (i = 0; i < args->iterations; i++) {
> +		expect_pgm_int();
> +		trigger->func();
> +		pgm = clear_pgm_int();
> +		if (pgm != expected_pgm) {
> +			report(0,
> +			"Program interrupt: expected(%d) == received(%d)",
> +			expected_pgm,
> +			pgm);
> +			return;
> +		}
> +	}
> +	report(1,
> +	"Program interrupt: always expected(%d) == received(%d)",
> +	expected_pgm,
> +	expected_pgm);
> +}
> +
> +#define TRANSACTION_COMPLETED 4
> +#define TRANSACTION_MAX_RETRIES 5
> +
> +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;
> +}
> +
> +#define report_info_if(cond, fmt, ...)			\
> +	do {						\
> +		if (cond) {				\
> +			report_info(fmt, ##__VA_ARGS__);\
> +		}					\
> +	} while (0)
> +
> +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 */
> +
> +	register_pgm_cleanup_func(trigger->fixup);
> +	for (i = 0; i < args->iterations && failures <= args->max_failures; i++) {
> +		trans_result = retry_transaction(trigger, args->max_retries, &diag.tdb, expected_pgm);
> +		switch (trans_result) {
> +		case 0:
> +			continue;
> +		case -_HTM_TBEGIN_INDETERMINATE:
> +		case -_HTM_TBEGIN_PERSISTENT:
> +			report_info_if(failures < args->suppress_info,
> +				       "transaction failed with cc %d",
> +				       -trans_result);
> +			break;
> +		case -_HTM_TBEGIN_TRANSIENT:
> +			report(0,
> +			       "Program interrupt: expected(%d) == received(%d)",
> +			       expected_pgm,
> +			       clear_pgm_int());
> +			goto out;
> +		case -TRANSACTION_COMPLETED:
> +			report(0,
> +			       "Transaction completed without exception");
> +			goto out;
> +		case -TRANSACTION_MAX_RETRIES:
> +			report_info_if(failures < args->suppress_info,
> +				       "Retried transaction %u times without exception",
> +				       10);
> +			break;
> +		default:
> +			report(0, "Invalid return transaction result");
> +			goto out;
> +		}
> +
> +		report_info_if(failures < args->suppress_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(1,
> +		       "Program interrupt: always expected(%d) == received(%d), transaction failures: %u",
> +		       expected_pgm,
> +		       expected_pgm,
> +		       failures);
> +	} else {
> +		report(0,
> +		       "Too many transaction failures: %u", failures);
> +	}
> +	report_info_if(failures > args->suppress_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 parsed;
> +	bool error, max_failures = false;
> +	char *end;
> +
> +	for (i = 1; i < argc; i++) {
> +		error = true;
> +		if (i < argc - 1) {
> +			error = *argv[i+1] == '\0';
> +			parsed = strtol(argv[i+1], &end, 10);
> +			error |= *end != '\0';
> +			error |= parsed < 0;
> +		}
> +
> +		if (!strcmp("--iterations", argv[i])) {
> +			if (error)
> +				report_abort("--iterations needs a positive parameter");
> +			args.iterations = parsed;
> +			++i;
> +		} else if (!strcmp("--max-retries", argv[i])) {
> +			if (error)
> +				report_abort("--max-retries needs a positive parameter");
> +			args.max_retries = parsed;
> +			++i;
> +		} else if (!strcmp("--suppress-info", argv[i])) {
> +			if (error)
> +				report_abort("--suppress-info needs a positive parameter");
> +			args.suppress_info = parsed;
> +			++i;
> +		} else if (!strcmp("--max-failures", argv[i])) {
> +			if (error)
> +				report_abort("--max-failures needs a positive parameter");
> +			args.max_failures = parsed;
> +			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;
> +}
> +
> +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();
> +
> +	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();
> +}
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index a0ec886..b93aead 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -106,3 +106,6 @@ timeout = 10
>  
>  [edat]
>  file = edat.elf
> +
> +[spec_ex]
> +file = spec_ex.elf
> 
> base-commit: bc6f264386b4cb2cadc8b2492315f3e6e8a801a2
>