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

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

[Janis Schoetterl-Glausch]

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

TCG fails the tests setting an invalid PSW bit.
I had a look at how best to fix it, but where best to check for early
PSW exceptions was not immediately clear to me. Ideas welcome.

v5 -> b6
	rebased onto master
	comments and style changes

v4 -> v5
	add lpsw with invalid bit 12 test
		TCG fails as with lpswe but must also invert bit 12
	update copyright statement
	add comments
	cleanups and style fixes

v3 -> v4
	remove iterations argument in order to simplify the code
		for manual performance testing adding a for loop is easy
	move report out of fixup_invalid_psw
	simplify/improve readability of triggers
	use positive error values

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 |   6 +
 s390x/spec_ex.c          | 383 +++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg      |   3 +
 4 files changed, 393 insertions(+)
 create mode 100644 s390x/spec_ex.c

Range-diff against v5:
1:  fd9780d8 ! 1:  bbfb5d40 s390x: Add specification exception test
    @@ lib/s390x/asm/arch_def.h: struct psw {
     +	uint32_t	addr;
     +};
     +
    - #define AS_PRIM				0
    - #define AS_ACCR				1
    - #define AS_SECN				2
    + struct cpu {
    + 	struct lowcore *lowcore;
    + 	uint64_t *stack;
     
      ## s390x/spec_ex.c (new) ##
     @@
    @@ s390x/spec_ex.c (new)
     + */
     +#include <stdlib.h>
     +#include <libcflat.h>
    ++#include <bitops.h>
     +#include <asm/interrupt.h>
     +
    ++/* toggled to signal occurrence of invalid psw fixup */
     +static bool invalid_psw_expected;
     +static struct psw expected_psw;
     +static struct psw invalid_psw;
    @@ s390x/spec_ex.c (new)
     + * 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)
    ++static void fixup_invalid_psw(struct stack_frame_int *stack)
     +{
     +	/* signal occurrence of invalid psw fixup */
     +	invalid_psw_expected = false;
    @@ s390x/spec_ex.c (new)
     +{
     +	uint64_t scratch;
     +
    ++	/*
    ++	 * The fixup psw is current psw with the instruction address replaced by
    ++	 * the address of the nop following the instruction loading the new psw.
    ++	 */
     +	fixup_psw.mask = extract_psw_mask();
     +	asm volatile ( "larl	%[scratch],0f\n"
    -+		"	stg	%[scratch],%[addr]\n"
    ++		"	stg	%[scratch],%[fixup_addr]\n"
     +		"	lpswe	%[psw]\n"
     +		"0:	nop\n"
    -+		: [scratch] "=&d"(scratch),
    -+		  [addr] "=&T"(fixup_psw.addr)
    -+		: [psw] "Q"(psw)
    ++		: [scratch] "=&d" (scratch),
    ++		  [fixup_addr] "=&T" (fixup_psw.addr)
    ++		: [psw] "Q" (psw)
     +		: "cc", "memory"
     +	);
     +}
    @@ s390x/spec_ex.c (new)
     +
     +	fixup_psw.mask = extract_psw_mask();
     +	asm volatile ( "larl	%[scratch],0f\n"
    -+		"	stg	%[scratch],%[addr]\n"
    ++		"	stg	%[scratch],%[fixup_addr]\n"
     +		"	lpsw	%[psw]\n"
     +		"0:	nop\n"
    -+		: [scratch] "=&d"(scratch),
    -+		  [addr] "=&T"(fixup_psw.addr)
    -+		: [psw] "Q"(psw)
    ++		: [scratch] "=&d" (scratch),
    ++		  [fixup_addr] "=&T" (fixup_psw.addr)
    ++		: [psw] "Q" (psw)
     +		: "cc", "memory"
     +	);
     +}
    @@ s390x/spec_ex.c (new)
     +
     +static int check_invalid_psw(void)
     +{
    -+	/* toggled to signal occurrence of invalid psw fixup */
     +	if (!invalid_psw_expected) {
     +		if (expected_psw.mask == invalid_psw.mask &&
     +		    expected_psw.addr == invalid_psw.addr)
    @@ s390x/spec_ex.c (new)
     +	return 1;
     +}
     +
    ++/* For normal PSWs bit 12 has to be 0 to be a valid PSW*/
     +static int psw_bit_12_is_1(void)
     +{
    -+	struct psw invalid = { .mask = 0x0008000000000000, .addr = 0x00000000deadbeee};
    ++	struct psw invalid = {
    ++		.mask = BIT(63 - 12),
    ++		.addr = 0x00000000deadbeee
    ++	};
     +
     +	expect_invalid_psw(invalid);
     +	load_psw(invalid);
     +	return check_invalid_psw();
     +}
     +
    ++/* A short PSW needs to have bit 12 set to be valid. */
     +static int short_psw_bit_12_is_0(void)
     +{
    -+	struct short_psw short_invalid = { .mask = 0x00000000, .addr = 0xdeadbeee};
    ++	struct short_psw short_invalid = {
    ++		.mask = 0x0,
    ++		.addr = 0xdeadbeee
    ++	};
     +
     +	/*
     +	 * lpsw may optionally check bit 12 before loading the new psw
    @@ s390x/spec_ex.c (new)
     +	uint32_t words[5] __attribute__((aligned(16)));
     +	uint32_t (*bad_aligned)[4] = (uint32_t (*)[4])&words[1];
     +
    ++	/* LOAD PAIR FROM QUADWORD (LPQ) requires quadword alignment */
     +	asm volatile ("lpq %%r6,%[bad]"
    -+		      : : [bad] "T"(*bad_aligned)
    ++		      : : [bad] "T" (*bad_aligned)
     +		      : "%r6", "%r7"
     +	);
     +	return 0;
    @@ s390x/spec_ex.c (new)
     +	uint64_t quad[2] __attribute__((aligned(16))) = {0};
     +
     +	asm volatile (".insn	rxy,0xe3000000008f,%%r7,%[quad]" /* lpq %%r7,%[quad] */
    -+		      : : [quad] "T"(quad)
    ++		      : : [quad] "T" (quad)
     +		      : "%r7", "%r8"
     +	);
     +	return 0;
    @@ s390x/spec_ex.c (new)
     +struct spec_ex_trigger {
     +	const char *name;
     +	int (*func)(void);
    -+	void (*fixup)(void);
    ++	void (*fixup)(struct stack_frame_int *stack);
     +};
     +
     +/* List of all tests to execute */
2:  c14092a3 ! 2:  0f19be7d s390x: Test specification exceptions during transaction
    @@ Commit message
         Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
     
      ## lib/s390x/asm/arch_def.h ##
    -@@ lib/s390x/asm/arch_def.h: struct short_psw {
    +@@ lib/s390x/asm/arch_def.h: struct cpu {
      #define PSW_MASK_BA			0x0000000080000000UL
      #define PSW_MASK_64			(PSW_MASK_BA | PSW_MASK_EA)
      
    @@ s390x/spec_ex.c
      #include <stdlib.h>
     +#include <htmintrin.h>
      #include <libcflat.h>
    + #include <bitops.h>
     +#include <asm/barrier.h>
      #include <asm/interrupt.h>
     +#include <asm/facility.h>
      
    + /* toggled to signal occurrence of invalid psw fixup */
      static bool invalid_psw_expected;
    - static struct psw expected_psw;
     @@ s390x/spec_ex.c: static int not_even(void)
      /*
       * Harness for specification exception testing.
    @@ s390x/spec_ex.c: static int not_even(void)
      	const char *name;
      	int (*func)(void);
     +	bool transactable;
    - 	void (*fixup)(void);
    + 	void (*fixup)(struct stack_frame_int *stack);
      };
      
      /* List of all tests to execute */

base-commit: 7362976db651a2142ec64b5cea2029ab77a5b157
-- 
2.36.1

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

[Janis Schoetterl-Glausch]

Generate specification exceptions and check that they occur.

Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
 s390x/Makefile           |   1 +
 lib/s390x/asm/arch_def.h |   5 +
 s390x/spec_ex.c          | 194 +++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg      |   3 +
 4 files changed, 203 insertions(+)
 create mode 100644 s390x/spec_ex.c

diff --git a/s390x/Makefile b/s390x/Makefile
index efd5e0c1..58b1bf54 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -27,6 +27,7 @@ tests += $(TEST_DIR)/uv-host.elf
 tests += $(TEST_DIR)/edat.elf
 tests += $(TEST_DIR)/mvpg-sie.elf
 tests += $(TEST_DIR)/spec_ex-sie.elf
+tests += $(TEST_DIR)/spec_ex.elf
 tests += $(TEST_DIR)/firq.elf
 tests += $(TEST_DIR)/epsw.elf
 tests += $(TEST_DIR)/adtl-status.elf
diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index e7ae454b..b6e60fb0 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -41,6 +41,11 @@ struct psw {
 	uint64_t	addr;
 };
 
+struct short_psw {
+	uint32_t	mask;
+	uint32_t	addr;
+};
+
 struct cpu {
 	struct lowcore *lowcore;
 	uint64_t *stack;
diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
new file mode 100644
index 00000000..68469e4b
--- /dev/null
+++ b/s390x/spec_ex.c
@@ -0,0 +1,194 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright IBM Corp. 2021, 2022
+ *
+ * Specification exception test.
+ * Tests that specification exceptions occur when expected.
+ *
+ * Can be extended by adding triggers to spec_ex_triggers, see comments below.
+ */
+#include <stdlib.h>
+#include <libcflat.h>
+#include <bitops.h>
+#include <asm/interrupt.h>
+
+/* toggled to signal occurrence of invalid psw fixup */
+static bool invalid_psw_expected;
+static struct psw expected_psw;
+static struct psw invalid_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(struct stack_frame_int *stack)
+{
+	/* signal occurrence of invalid psw fixup */
+	invalid_psw_expected = false;
+	invalid_psw = lowcore.pgm_old_psw;
+	lowcore.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.
+ * Also acts as compiler barrier, -> none required in expect/check_invalid_psw
+ */
+static void load_psw(struct psw psw)
+{
+	uint64_t scratch;
+
+	/*
+	 * The fixup psw is current psw with the instruction address replaced by
+	 * the address of the nop following the instruction loading the new psw.
+	 */
+	fixup_psw.mask = extract_psw_mask();
+	asm volatile ( "larl	%[scratch],0f\n"
+		"	stg	%[scratch],%[fixup_addr]\n"
+		"	lpswe	%[psw]\n"
+		"0:	nop\n"
+		: [scratch] "=&d" (scratch),
+		  [fixup_addr] "=&T" (fixup_psw.addr)
+		: [psw] "Q" (psw)
+		: "cc", "memory"
+	);
+}
+
+static void load_short_psw(struct short_psw psw)
+{
+	uint64_t scratch;
+
+	fixup_psw.mask = extract_psw_mask();
+	asm volatile ( "larl	%[scratch],0f\n"
+		"	stg	%[scratch],%[fixup_addr]\n"
+		"	lpsw	%[psw]\n"
+		"0:	nop\n"
+		: [scratch] "=&d" (scratch),
+		  [fixup_addr] "=&T" (fixup_psw.addr)
+		: [psw] "Q" (psw)
+		: "cc", "memory"
+	);
+}
+
+static void expect_invalid_psw(struct psw psw)
+{
+	expected_psw = psw;
+	invalid_psw_expected = true;
+}
+
+static int check_invalid_psw(void)
+{
+	if (!invalid_psw_expected) {
+		if (expected_psw.mask == invalid_psw.mask &&
+		    expected_psw.addr == invalid_psw.addr)
+			return 0;
+		report_fail("Wrong invalid PSW");
+	} else {
+		report_fail("Expected exception due to invalid PSW");
+	}
+	return 1;
+}
+
+/* For normal PSWs bit 12 has to be 0 to be a valid PSW*/
+static int psw_bit_12_is_1(void)
+{
+	struct psw invalid = {
+		.mask = BIT(63 - 12),
+		.addr = 0x00000000deadbeee
+	};
+
+	expect_invalid_psw(invalid);
+	load_psw(invalid);
+	return check_invalid_psw();
+}
+
+/* A short PSW needs to have bit 12 set to be valid. */
+static int short_psw_bit_12_is_0(void)
+{
+	struct short_psw short_invalid = {
+		.mask = 0x0,
+		.addr = 0xdeadbeee
+	};
+
+	/*
+	 * lpsw may optionally check bit 12 before loading the new psw
+	 * -> cannot check the expected invalid psw like with lpswe
+	 */
+	load_short_psw(short_invalid);
+	return 0;
+}
+
+static int bad_alignment(void)
+{
+	uint32_t words[5] __attribute__((aligned(16)));
+	uint32_t (*bad_aligned)[4] = (uint32_t (*)[4])&words[1];
+
+	/* LOAD PAIR FROM QUADWORD (LPQ) requires quadword alignment */
+	asm volatile ("lpq %%r6,%[bad]"
+		      : : [bad] "T" (*bad_aligned)
+		      : "%r6", "%r7"
+	);
+	return 0;
+}
+
+static int not_even(void)
+{
+	uint64_t quad[2] __attribute__((aligned(16))) = {0};
+
+	asm volatile (".insn	rxy,0xe3000000008f,%%r7,%[quad]" /* lpq %%r7,%[quad] */
+		      : : [quad] "T" (quad)
+		      : "%r7", "%r8"
+	);
+	return 0;
+}
+
+/*
+ * Harness for specification exception testing.
+ * func only triggers exception, reporting is taken care of automatically.
+ */
+struct spec_ex_trigger {
+	const char *name;
+	int (*func)(void);
+	void (*fixup)(struct stack_frame_int *stack);
+};
+
+/* List of all tests to execute */
+static const struct spec_ex_trigger spec_ex_triggers[] = {
+	{ "psw_bit_12_is_1", &psw_bit_12_is_1, &fixup_invalid_psw },
+	{ "short_psw_bit_12_is_0", &short_psw_bit_12_is_0, &fixup_invalid_psw },
+	{ "bad_alignment", &bad_alignment, NULL },
+	{ "not_even", &not_even, NULL },
+	{ NULL, NULL, NULL },
+};
+
+static void test_spec_ex(const struct spec_ex_trigger *trigger)
+{
+	int rc;
+
+	expect_pgm_int();
+	register_pgm_cleanup_func(trigger->fixup);
+	rc = trigger->func();
+	register_pgm_cleanup_func(NULL);
+	/* test failed, nothing to be done, reporting responsibility of trigger */
+	if (rc)
+		return;
+	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+}
+
+int main(int argc, char **argv)
+{
+	unsigned int i;
+
+	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(&spec_ex_triggers[i]);
+		report_prefix_pop();
+	}
+	report_prefix_pop();
+
+	return report_summary();
+}
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index f7b1fc3d..2ecaee1f 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -114,6 +114,9 @@ file = mvpg-sie.elf
 [spec_ex-sie]
 file = spec_ex-sie.elf
 
+[spec_ex]
+file = spec_ex.elf
+
 [firq-linear-cpu-ids-kvm]
 file = firq.elf
 timeout = 20
-- 
2.36.1

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

[Nico Boehr]

Quoting Janis Schoetterl-Glausch (2022-08-26 18:11:11)
> Generate specification exceptions and check that they occur.
> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>

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

with minor nits below you may want to consider

> diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
> new file mode 100644
[...]
> +static int bad_alignment(void)
> +{
> +       uint32_t words[5] __attribute__((aligned(16)));
> +       uint32_t (*bad_aligned)[4] = (uint32_t (*)[4])&words[1];

Why not simply:

uint32_t *bad_aligned = &words[1];

> +
> +       /* LOAD PAIR FROM QUADWORD (LPQ) requires quadword alignment */
> +       asm volatile ("lpq %%r6,%[bad]"
> +                     : : [bad] "T" (*bad_aligned)
> +                     : "%r6", "%r7"
> +       );
> +       return 0;
> +}
> +
> +static int not_even(void)
> +{
> +       uint64_t quad[2] __attribute__((aligned(16))) = {0};
> +
> +       asm volatile (".insn    rxy,0xe3000000008f,%%r7,%[quad]" /* lpq %%r7,%[quad] */

Here you use .insn above you use lpq - why?

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

[Janis Schoetterl-Glausch]

On Tue, 2022-08-30 at 16:34 +0200, Nico Boehr wrote:
> Quoting Janis Schoetterl-Glausch (2022-08-26 18:11:11)
> > Generate specification exceptions and check that they occur.
> > 
> > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> 
> Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

Thanks
> 
> with minor nits below you may want to consider
> 
> > diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
> > new file mode 100644
> [...]
> > +static int bad_alignment(void)
> > +{
> > +       uint32_t words[5] __attribute__((aligned(16)));
> > +       uint32_t (*bad_aligned)[4] = (uint32_t (*)[4])&words[1];
> 
> Why not simply:
> 
> uint32_t *bad_aligned = &words[1];

This is a pointer to a word, the argument to lpq is a quadword.
Your way would probably work, especially since we don't actually want
the asm to do anything, but no harm in doing it the correct way.
> 
> > +
> > +       /* LOAD PAIR FROM QUADWORD (LPQ) requires quadword alignment */
> > +       asm volatile ("lpq %%r6,%[bad]"
> > +                     : : [bad] "T" (*bad_aligned)
> > +                     : "%r6", "%r7"
> > +       );
> > +       return 0;
> > +}
> > +
> > +static int not_even(void)
> > +{
> > +       uint64_t quad[2] __attribute__((aligned(16))) = {0};
> > +
> > +       asm volatile (".insn    rxy,0xe3000000008f,%%r7,%[quad]" /* lpq %%r7,%[quad] */
> 
> Here you use .insn above you use lpq - why?

The assembler will complain about the odd register number, but that is
intentional.

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

[Janosch Frank]

On 8/26/22 18:11, Janis Schoetterl-Glausch wrote:
> Generate specification exceptions and check that they occur.
> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>

Minor issues below, apart from that:
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

> ---
>   s390x/Makefile           |   1 +
>   lib/s390x/asm/arch_def.h |   5 +
>   s390x/spec_ex.c          | 194 +++++++++++++++++++++++++++++++++++++++
>   s390x/unittests.cfg      |   3 +
>   4 files changed, 203 insertions(+)
>   create mode 100644 s390x/spec_ex.c
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index efd5e0c1..58b1bf54 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -27,6 +27,7 @@ tests += $(TEST_DIR)/uv-host.elf
>   tests += $(TEST_DIR)/edat.elf
>   tests += $(TEST_DIR)/mvpg-sie.elf
>   tests += $(TEST_DIR)/spec_ex-sie.elf
> +tests += $(TEST_DIR)/spec_ex.elf
>   tests += $(TEST_DIR)/firq.elf
>   tests += $(TEST_DIR)/epsw.elf
>   tests += $(TEST_DIR)/adtl-status.elf
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index e7ae454b..b6e60fb0 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -41,6 +41,11 @@ struct psw {
>   	uint64_t	addr;
>   };
>   
> +struct short_psw {
> +	uint32_t	mask;
> +	uint32_t	addr;
> +};
> +
>   struct cpu {
>   	struct lowcore *lowcore;
>   	uint64_t *stack;
> diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
> new file mode 100644
> index 00000000..68469e4b
> --- /dev/null
> +++ b/s390x/spec_ex.c
> @@ -0,0 +1,194 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright IBM Corp. 2021, 2022
> + *
> + * Specification exception test.
> + * Tests that specification exceptions occur when expected.
> + *
> + * Can be extended by adding triggers to spec_ex_triggers, see comments below.
> + */
> +#include <stdlib.h>
> +#include <libcflat.h>
> +#include <bitops.h>
> +#include <asm/interrupt.h>
> +
> +/* toggled to signal occurrence of invalid psw fixup */
> +static bool invalid_psw_expected;
> +static struct psw expected_psw;
> +static struct psw invalid_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(struct stack_frame_int *stack)
> +{
> +	/* signal occurrence of invalid psw fixup */
> +	invalid_psw_expected = false;

Hmmmm (TM), assert(invalid_psw_expected) ?

> +	invalid_psw = lowcore.pgm_old_psw;
> +	lowcore.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.
> + * Also acts as compiler barrier, -> none required in expect/check_invalid_psw
> + */
> +static void load_psw(struct psw psw)
> +{
> +	uint64_t scratch;
> +
> +	/*
> +	 * The fixup psw is current psw with the instruction address replaced by

is the current psw

> +	 * the address of the nop following the instruction loading the new psw. > +	 */
> +	fixup_psw.mask = extract_psw_mask();
> +	asm volatile ( "larl	%[scratch],0f\n"
> +		"	stg	%[scratch],%[fixup_addr]\n"
> +		"	lpswe	%[psw]\n"
> +		"0:	nop\n"
> +		: [scratch] "=&d" (scratch),
> +		  [fixup_addr] "=&T" (fixup_psw.addr)
> +		: [psw] "Q" (psw)
> +		: "cc", "memory"
> +	);
> +}
> +
> +static void load_short_psw(struct short_psw psw)
> +{
> +	uint64_t scratch;
> +
> +	fixup_psw.mask = extract_psw_mask();
> +	asm volatile ( "larl	%[scratch],0f\n"
> +		"	stg	%[scratch],%[fixup_addr]\n"
> +		"	lpsw	%[psw]\n"
> +		"0:	nop\n"
> +		: [scratch] "=&d" (scratch),
> +		  [fixup_addr] "=&T" (fixup_psw.addr)
> +		: [psw] "Q" (psw)
> +		: "cc", "memory"
> +	);
> +}
> +
> +static void expect_invalid_psw(struct psw psw)
> +{
> +	expected_psw = psw;
> +	invalid_psw_expected = true;
> +}
> +
> +static int check_invalid_psw(void)
> +{

/* Since the fixup sets this to false we check for false here. */

> +	if (!invalid_psw_expected) {
> +		if (expected_psw.mask == invalid_psw.mask &&
> +		    expected_psw.addr == invalid_psw.addr)
> +			return 0;
> +		report_fail("Wrong invalid PSW");
> +	} else {
> +		report_fail("Expected exception due to invalid PSW");
> +	}
> +	return 1;
> +}
> +

[kvm-unit-tests PATCH v6 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          | 199 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 195 insertions(+), 5 deletions(-)

diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index b6e60fb0..c841871c 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -73,6 +73,7 @@ struct cpu {
 #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_FETCH_PROTECTION_OVERRIDE		(63 - 38)
diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
index 68469e4b..56f26564 100644
--- a/s390x/spec_ex.c
+++ b/s390x/spec_ex.c
@@ -4,13 +4,19 @@
  *
  * 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).
  *
  * Can be extended by adding triggers to spec_ex_triggers, see comments below.
  */
 #include <stdlib.h>
+#include <htmintrin.h>
 #include <libcflat.h>
 #include <bitops.h>
+#include <asm/barrier.h>
 #include <asm/interrupt.h>
+#include <asm/facility.h>
 
 /* toggled to signal occurrence of invalid psw fixup */
 static bool invalid_psw_expected;
@@ -148,20 +154,22 @@ static int not_even(void)
 /*
  * Harness for specification exception testing.
  * func only triggers exception, reporting is taken care of automatically.
+ * If a trigger is transactable it will also  be executed during a transaction.
  */
 struct spec_ex_trigger {
 	const char *name;
 	int (*func)(void);
+	bool transactable;
 	void (*fixup)(struct stack_frame_int *stack);
 };
 
 /* List of all tests to execute */
 static const struct spec_ex_trigger spec_ex_triggers[] = {
-	{ "psw_bit_12_is_1", &psw_bit_12_is_1, &fixup_invalid_psw },
-	{ "short_psw_bit_12_is_0", &short_psw_bit_12_is_0, &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 },
+	{ "short_psw_bit_12_is_0", &short_psw_bit_12_is_0, false, &fixup_invalid_psw },
+	{ "bad_alignment", &bad_alignment, true, NULL },
+	{ "not_even", &not_even, true, NULL },
+	{ NULL, NULL, false, NULL },
 };
 
 static void test_spec_ex(const struct spec_ex_trigger *trigger)
@@ -178,10 +186,181 @@ static void test_spec_ex(const struct spec_ex_trigger *trigger)
 	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
 }
 
+#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(int (*trigger)(void), struct __htm_tdb *diagnose)
+{
+	int cc;
+
+	cc = __builtin_tbegin(diagnose);
+	/*
+	 * Everything between tbegin and tend is part of the transaction,
+	 * which either completes in its entirety or does not have any effect.
+	 * If the transaction fails, execution is reset to this point with another
+	 * condition code indicating why the transaction failed.
+	 */
+	if (cc == _HTM_TBEGIN_STARTED) {
+		/*
+		 * return code is meaningless: transaction needs to complete
+		 * in order to return and completion indicates a test failure
+		 */
+		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 = lowcore.pgm_int_code;
+			if (pgm == expected_pgm)
+				return 0;
+			else if (pgm == 0)
+				/*
+				 * Transaction failed for unknown reason but not because
+				 * of an unexpected program exception. Give it another
+				 * go so that hopefully it reaches the triggering instruction.
+				 */
+				continue;
+		}
+		return trans_result;
+	}
+	return TRANSACTION_MAX_RETRIES;
+}
+
+struct args {
+	uint64_t max_retries;
+	bool diagnose;
+};
+
+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;
+	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);
+	trans_result = retry_transaction(trigger, args->max_retries, &diag.tdb, expected_pgm);
+	register_pgm_cleanup_func(NULL);
+	switch (trans_result) {
+	case 0:
+		report_pass("Program interrupt: expected(%d) == received(%d)",
+			    expected_pgm, expected_pgm);
+		break;
+	case _HTM_TBEGIN_INDETERMINATE:
+	case _HTM_TBEGIN_PERSISTENT:
+		report_info("transaction failed with cc %d", trans_result);
+		report_info("transaction abort code: %llu", diag.tdb.abort_code);
+		if (args->diagnose)
+			for (i = 0; i < 32; i++)
+				report_info("diag+%03d: %016lx", i * 8, diag.dwords[i]);
+		break;
+	case _HTM_TBEGIN_TRANSIENT:
+		report_fail("Program interrupt: expected(%d) == received(%d)",
+			    expected_pgm, clear_pgm_int());
+		break;
+	case TRANSACTION_COMPLETED:
+		report_fail("Transaction completed without exception");
+		break;
+	case TRANSACTION_MAX_RETRIES:
+		report_skip("Transaction retried %lu times with transient failures, giving up",
+			    args->max_retries);
+		break;
+	default:
+		report_fail("Invalid transaction result");
+		break;
+	}
+
+	ctl_clear_bit(0, CTL0_TRANSACT_EX_CTL);
+}
+
+static bool parse_unsigned(const char *arg, unsigned int *out)
+{
+	char *end;
+	long num;
+
+	if (arg[0] == '\0')
+		return false;
+	num = strtol(arg, &end, 10);
+	if (end[0] != '\0' || num < 0)
+		return false;
+	*out = num;
+	return true;
+}
+
+static struct args parse_args(int argc, char **argv)
+{
+	struct args args = {
+		.max_retries = 20,
+		.diagnose = false
+	};
+	unsigned int i, arg;
+	bool has_arg;
+	const char *flag;
+
+	for (i = 1; i < argc; i++) {
+		if (i + 1 < argc)
+			has_arg = parse_unsigned(argv[i + 1], &arg);
+		else
+			has_arg = false;
+
+		flag = "--max-retries";
+		if (!strcmp(flag, argv[i])) {
+			if (!has_arg)
+				report_abort("%s needs a positive parameter", flag);
+			args.max_retries = 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);
@@ -190,5 +369,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.36.1

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

[Nico Boehr]

Quoting Janis Schoetterl-Glausch (2022-08-26 18:11:12)
> diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
> index 68469e4b..56f26564 100644
[...]
> +#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
> + */

For some reason, it took me a while to get this, because the context was not clear to me. Maybe rephrase it a tiny bit:

If diagnose should be NULL, it must be passed to __builtin_tbegin via constant, so forbid NULL to keep things simple

[...]
> +static void test_spec_ex_trans(struct args *args, const struct spec_ex_trigger *trigger)
> +{
[...]
> +       case TRANSACTION_MAX_RETRIES:
> +               report_skip("Transaction retried %lu times with transient failures, giving up",
> +                           args->max_retries);

Hmhm, I am unsure whether a skip is the right thing here. On one hand, it might hide bugs, on the other hand, it might cause spurious failures. Why did you decide for the skip?

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

[Janis Schoetterl-Glausch]

On Thu, 2022-09-01 at 16:59 +0200, Nico Boehr wrote:
> Quoting Janis Schoetterl-Glausch (2022-08-26 18:11:12)
> > diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
> > index 68469e4b..56f26564 100644
> [...]
> > +#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
> > + */
> 
> For some reason, it took me a while to get this, because the context was not clear to me. Maybe rephrase it a tiny bit:
> 
> If diagnose should be NULL, it must be passed to __builtin_tbegin via constant, so forbid NULL to keep things simple
> 
> [...]
> > +static void test_spec_ex_trans(struct args *args, const struct spec_ex_trigger *trigger)
> > +{
> [...]
> > +       case TRANSACTION_MAX_RETRIES:
> > +               report_skip("Transaction retried %lu times with transient failures, giving up",
> > +                           args->max_retries);
> 
> Hmhm, I am unsure whether a skip is the right thing here. On one hand, it might hide bugs, on the other hand, it might cause spurious failures. Why did you decide for the skip?

Yeah, it's a toss-up. Claudio asked me to change it in v4.

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

[Nico Boehr]

Quoting Janis Schoetterl-Glausch (2022-09-01 18:08:17)
[...]
> > Hmhm, I am unsure whether a skip is the right thing here. On one hand, it might hide bugs, on the other hand, it might cause spurious failures. Why did you decide for the skip?
> 
> Yeah, it's a toss-up. Claudio asked me to change it in v4.

Allright, then:

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

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

[Janosch Frank]

On 8/26/22 18:11, Janis Schoetterl-Glausch 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>

First off a disclaimer stating that I don't know anything about our TB 
facility and I'm currently lacking the time to read the documentation.

But the code looks good to me and I don't see a reason that keeps me 
from picking this.

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

Minor nits below

> ---
>   lib/s390x/asm/arch_def.h |   1 +
>   s390x/spec_ex.c          | 199 ++++++++++++++++++++++++++++++++++++++-
>   2 files changed, 195 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index b6e60fb0..c841871c 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -73,6 +73,7 @@ struct cpu {
>   #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_FETCH_PROTECTION_OVERRIDE		(63 - 38)
> diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
> index 68469e4b..56f26564 100644
> --- a/s390x/spec_ex.c
> +++ b/s390x/spec_ex.c
> @@ -4,13 +4,19 @@
>    *
>    * 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).
>    *
>    * Can be extended by adding triggers to spec_ex_triggers, see comments below.
>    */
>   #include <stdlib.h>
> +#include <htmintrin.h>
>   #include <libcflat.h>
>   #include <bitops.h>
> +#include <asm/barrier.h>
>   #include <asm/interrupt.h>
> +#include <asm/facility.h>
>   
>   /* toggled to signal occurrence of invalid psw fixup */
>   static bool invalid_psw_expected;
> @@ -148,20 +154,22 @@ static int not_even(void)
>   /*
>    * Harness for specification exception testing.
>    * func only triggers exception, reporting is taken care of automatically.
> + * If a trigger is transactable it will also  be executed during a transaction.

Double space

> +
> +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;

I usually prefer having | and & at the end so it's easier to read.

> +	union {
> +		struct __htm_tdb tdb;
> +		uint64_t dwords[sizeof(struct __htm_tdb) / sizeof(uint64_t)];
> +	} diag;
> +	unsigned int i;
> +	int trans_result;
> +