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

[Claudio Imbrenda <imbrenda@linux.ibm.com>]

On Tue, 11 Jan 2022 17:39:01 +0100
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

> Program interruptions during transactional execution cause other
> interruption codes.
> Check that we see the expected code for (some) specification exceptions.
> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
> I don't think we can use constraint transactions to guarantee successful
> execution of the transaction unless we implement it completely in asm,
> otherwise we cannot ensure that the constraints of the transaction are met.
> 
>  lib/s390x/asm/arch_def.h |   1 +
>  s390x/spec_ex.c          | 177 ++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 174 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 40626d7..f7fb467 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -55,6 +55,7 @@ struct psw {
>  #define PSW_MASK_BA			0x0000000080000000UL
>  #define PSW_MASK_64			(PSW_MASK_BA | PSW_MASK_EA)
>  
> +#define CTL0_TRANSACT_EX_CTL		(63 -  8)
>  #define CTL0_LOW_ADDR_PROT		(63 - 35)
>  #define CTL0_EDAT			(63 - 40)
>  #define CTL0_IEP			(63 - 43)
> diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
> index a9f9f31..e599994 100644
> --- a/s390x/spec_ex.c
> +++ b/s390x/spec_ex.c
> @@ -4,12 +4,18 @@
>   *
>   * 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>

where is this header ?

>  #include <libcflat.h>
> +#include <asm/barrier.h>
>  #include <asm/interrupt.h>
> +#include <asm/facility.h>
>  
>  static struct lowcore *lc = (struct lowcore *) 0;
>  
> @@ -106,19 +112,21 @@ 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)(void);
>  };
>  
>  /* 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 },
> -	{ "bad_alignment", &bad_alignment, NULL },
> -	{ "not_even", &not_even, NULL },
> -	{ NULL, NULL, NULL },
> +	{ "psw_bit_12_is_1", &psw_bit_12_is_1, false, &fixup_invalid_psw },
> +	{ "bad_alignment", &bad_alignment, true, NULL },
> +	{ "not_even", &not_even, true, NULL },
> +	{ NULL, NULL, false, NULL },
>  };
>  
>  static void test_spec_ex(const struct spec_ex_trigger *trigger)
> @@ -138,10 +146,161 @@ static void test_spec_ex(const struct spec_ex_trigger *trigger)
>  	       expected_pgm, pgm);
>  }
>  
> +#define TRANSACTION_COMPLETED 4
> +#define TRANSACTION_MAX_RETRIES 5
> +
> +/* NULL must be passed to __builtin_tbegin via constant, forbid diagnose from
> + * being NULL to keep things simple
> + */
> +static int __attribute__((nonnull))
> +with_transaction(int (*trigger)(void), struct __htm_tdb *diagnose)
> +{
> +	int cc;
> +
> +	cc = __builtin_tbegin(diagnose);

this is __really__ hard to understand if you don't know exactly how
transactions work. I would like to see some comments explaining what's
going on and why

> +	if (cc == _HTM_TBEGIN_STARTED) {
> +		/* return code is meaningless: transaction needs to complete

please, in a multi-line comment, leave the first line empty, like this:

/*
 * first line
 * another line
 * last line
 */

> +		 * 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 = lc->pgm_int_code;
> +			if (pgm == 0)

add a comment to explain why we try again with pgm == 0 .

> +				continue;
> +			else if (pgm == expected_pgm)
> +				return 0;
> +		}
> +		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_info("Retried transaction %lu times without exception",

I would word it differently, otherwise the difference between this
case and the one above is not clear. Maybe something like

"Transaction retried %lu times with transient failures, giving up"

Moreover, in this case the test is in practice skipped, I think you
should use report_skip

> +			    args->max_retries);
> +		break;
> +	default:
> +		report_fail("Invalid return transaction result");
> +		break;
> +	}
> +
> +	ctl_clear_bit(0, CTL0_TRANSACT_EX_CTL);
> +}
> +
> +static struct args parse_args(int argc, char **argv)
> +{

can you find a way to simplify this function, or at least to make it
more readable?

> +	struct args args = {
> +		.max_retries = 20,
> +		.diagnose = false
> +	};
> +	unsigned int i;
> +	long arg;
> +	bool no_arg;
> +	char *end;
> +	const char *flag;
> +	uint64_t *argp;
> +
> +	for (i = 1; i < argc; i++) {
> +		no_arg = true;
> +		if (i < argc - 1) {
> +			no_arg = *argv[i + 1] == '\0';
> +			arg = strtol(argv[i + 1], &end, 10);
> +			no_arg |= *end != '\0';
> +			no_arg |= arg < 0;
> +		}
> +
> +		flag = "--max-retries";
> +		argp = &args.max_retries;
> +		if (!strcmp(flag, argv[i])) {
> +			if (no_arg)
> +				report_abort("%s needs a positive parameter", flag);
> +			*argp = arg;
> +			++i;
> +			continue;
> +		}
> +		if (!strcmp("--diagnose", argv[i])) {
> +			args.diagnose = true;
> +			continue;
> +		}
> +		if (!strcmp("--no-diagnose", argv[i])) {
> +			args.diagnose = false;
> +			continue;
> +		}
> +		report_abort("Unsupported parameter '%s'",
> +			     argv[i]);
> +	}
> +
> +	return args;
> +}
> +
>  int main(int argc, char **argv)
>  {
>  	unsigned int i;
>  
> +	struct args args = parse_args(argc, argv);
> +
>  	report_prefix_push("specification exception");
>  	for (i = 0; spec_ex_triggers[i].name; i++) {
>  		report_prefix_push(spec_ex_triggers[i].name);
> @@ -150,5 +309,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();
>  }