On 1/13/22 13:20, Claudio Imbrenda wrote:
> On Tue, 11 Jan 2022 17:39:01 +0100
> Janis Schoetterl-Glausch <scgl@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
>> ---
>> 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", ¬_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", ¬_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;
>> +}
>> +
[...]
