diff mbox series

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

Message ID 20220826161112.3786131-3-scgl@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series Add specification exception tests | expand

Commit Message

Janis Schoetterl-Glausch Aug. 26, 2022, 4:11 p.m. UTC
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(-)

Comments

Nico Boehr Sept. 1, 2022, 2:59 p.m. UTC | #1
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?
Janis Schoetterl-Glausch Sept. 1, 2022, 4:08 p.m. UTC | #2
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.
Nico Boehr Sept. 5, 2022, 7:10 a.m. UTC | #3
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>
Janosch Frank Sept. 26, 2022, 1:18 p.m. UTC | #4
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;
> +
diff mbox series

Patch

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();
 }