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

[Janis Schoetterl-Glausch <scgl@linux.ibm.com>]

On 1/13/22 13:20, Claudio Imbrenda wrote:
> 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 ?

Not sure what you're asking exactly. The path is /usr/lib/gcc/s390x-redhat-linux/11/include/htmintrin.h
on my machine.
> 
>>  #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

True.

[...]

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

Yes to both.

[...]

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

I'll try to make that ^ more readable, the stuff below seems fine to me.

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

[...]