On 10/25/21 19:17, Claudio Imbrenda wrote:
> On Fri, 22 Oct 2021 14:01:55 +0200
> Janis Schoetterl-Glausch <scgl@xxxxxxxxxxxxx> wrote:
>
>> Generate specification exceptions and check that they occur.
>> With the iterations argument one can check if specification
>> exception interpretation occurs, e.g. by using a high value and
>> checking that the debugfs counters are substantially lower.
>> The argument is also useful for estimating the performance benefit
>> of interpretation.
>>
>> Signed-off-by: Janis Schoetterl-Glausch <scgl@xxxxxxxxxxxxx>
>> ---
>> s390x/Makefile | 1 +
>> s390x/spec_ex.c | 181 ++++++++++++++++++++++++++++++++++++++++++++
>> s390x/unittests.cfg | 3 +
>> 3 files changed, 185 insertions(+)
>> create mode 100644 s390x/spec_ex.c
>>
>> diff --git a/s390x/Makefile b/s390x/Makefile
>> index d18b08b..3e42784 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -24,6 +24,7 @@ tests += $(TEST_DIR)/mvpg.elf
>> tests += $(TEST_DIR)/uv-host.elf
>> tests += $(TEST_DIR)/edat.elf
>> tests += $(TEST_DIR)/mvpg-sie.elf
>> +tests += $(TEST_DIR)/spec_ex.elf
>>
>> tests_binary = $(patsubst %.elf,%.bin,$(tests))
>> ifneq ($(HOST_KEY_DOCUMENT),)
>> diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
>> new file mode 100644
>> index 0000000..ec3322a
>> --- /dev/null
>> +++ b/s390x/spec_ex.c
>> @@ -0,0 +1,181 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright IBM Corp. 2021
>> + *
>> + * Specification exception test.
>> + * Tests that specification exceptions occur when expected.
>> + */
>> +#include <stdlib.h>
>> +#include <libcflat.h>
>> +#include <asm/interrupt.h>
>> +#include <asm/facility.h>
>> +
>> +static struct lowcore *lc = (struct lowcore *) 0;
>> +
>> +static bool expect_invalid_psw;
>> +static struct psw expected_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(void)
>> +{
>> + if (expect_invalid_psw) {
>> + report(expected_psw.mask == lc->pgm_old_psw.mask
>> + && expected_psw.addr == lc->pgm_old_psw.addr,
>> + "Invalid program new PSW as expected");
>> + expect_invalid_psw = false;
>
> can you find a way to call report() where the test is
> triggered (psw_bit_12_is_1), instead of burying it here?
>
Yes, should be doable.
> maybe instead of calling report you can set a flag like
> "expected_psw_found" and then call report on it?
>
>> + }
>> + lc->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.
>> + */
>> +static void load_psw(struct psw psw)
>> +{
>> + uint64_t scratch;
>> +
>
> I understand why you are doing this, but I wonder if there is a "nicer"
> way to do it. What happens if you chose a nicer and unique name for the
> label and make it global?
>
I don't think that would work, the compiler might inline the function,
duplicating the label.
I suppose I could replace the stg with an assignment in C, not sure if that's nicer.
>> + fixup_psw.mask = extract_psw_mask();
>
> then you could add this here:
> fixup_psw.addr = after_lpswe;
>
>> + asm volatile (
>> + " larl %[scratch],nop%=\n"
>> + " stg %[scratch],%[addr]\n"
> ^ those two lines are no longer needed ^
>> + " lpswe %[psw]\n"
>> + "nop%=: nop\n"
> ".global after_lpswe \n"
> "after_lpswe: nop"
>> + : [scratch] "=&r"(scratch),
>> + [addr] "=&T"(fixup_psw.addr)
>> + : [psw] "Q"(psw)
>> + : "cc", "memory"
>> + );
>> +}
>> +
>> +static void psw_bit_12_is_1(void)
>> +{
>> + expected_psw.mask = 0x0008000000000000;
>> + expected_psw.addr = 0x00000000deadbeee;
>> + expect_invalid_psw = true;
>> + load_psw(expected_psw);
>
> and here something like
> report(expected_psw_found, "blah blah blah");
>
>> +}
>> +
>> +static void bad_alignment(void)
>> +{
>> + uint32_t words[5] = {0, 0, 0};
>> + uint32_t (*bad_aligned)[4];
>> +
>> + register uint64_t r1 asm("6");
>> + register uint64_t r2 asm("7");
>> + if (((uintptr_t)&words[0]) & 0xf)
>> + bad_aligned = (uint32_t (*)[4])&words[0];
>> + else
>> + bad_aligned = (uint32_t (*)[4])&words[1];
>
> this is a lot of work... can't you just declare it like:
>
> uint32_t words[5] __attribute__((aligned(16)));
> and then just use
> (words + 1) ?
That's nicer indeed.
>
>> + asm volatile ("lpq %0,%2"
>> + : "=r"(r1), "=r"(r2)
>
> since you're ignoring the return value, can't you hardcode r6, and mark
> it (and r7) as clobbered? like:
> "lpq 6, %[bad]"
> : : [bad] "T"(words[1])
> : "%r6", "%r7"
>
Ok, btw. is there a reason bare register numbers seem to be more common
compared to %%rN ?
>> + : "T"(*bad_aligned)
>> + );
>> +}
>> +
>> +static void not_even(void)
>> +{
>> + uint64_t quad[2];
>> +
>> + register uint64_t r1 asm("7");
>> + register uint64_t r2 asm("8");
>> + asm volatile (".insn rxy,0xe3000000008f,%0,%2" //lpq
>> %0,%2
>
> this is even uglier. I guess you had already tried this?
>
Yes, the assembler won't let you do that.
> "lpq 7, %[good]"
> : : [good] "T"(quad)
> : "%r7", "%r8"
>
> if that doesn't work, then the same but with .insn
>
>> + : "=r"(r1), "=r"(r2)
>> + : "T"(quad)
>> + );
>> +}
>> +
>> +struct spec_ex_trigger {
>> + const char *name;
>> + void (*func)(void);
>> + void (*fixup)(void);
>> +};
>> +
>> +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},
>> +};
>> +
>
> this is a lot of infrastructure for 3 tests... (or even for 5 tests,
> since you will add the transactions in the next patch)
Is it? I think we'd want a test for a "normal" specification exception,
and one for an invalid PSW at least. Even for just those two, I don't
think it would be nice to duplicate the test_spec_ex harness.
>
> are you planning to significantly extend this test in the future?
Not really, but I thought having it be easily extensible might be nice.
>
>> +struct args {
>> + uint64_t iterations;
>> +};
>> +
>> +static void test_spec_ex(struct args *args,
>> + const struct spec_ex_trigger *trigger)
>> +{
>> + uint16_t expected_pgm = PGM_INT_CODE_SPECIFICATION;
>> + uint16_t pgm;
>> + unsigned int i;
>> +
>> + for (i = 0; i < args->iterations; i++) {
>> + expect_pgm_int();
>> + register_pgm_cleanup_func(trigger->fixup);
>> + trigger->func();
>> + register_pgm_cleanup_func(NULL);
>> + pgm = clear_pgm_int();
>> + if (pgm != expected_pgm) {
>> + report_fail("Program interrupt: expected(%d)
>> == received(%d)",
>> + expected_pgm,
>> + pgm);
>> + return;
>> + }
>> + }
>> + report_pass("Program interrupt: always expected(%d) ==
>> received(%d)",
>> + expected_pgm,
>> + expected_pgm);
>> +}
>> +
>> +static struct args parse_args(int argc, char **argv)
>
> do we _really_ need commandline arguments?
>
No, but they can be useful.
The iterations argument can be used to check if interpretation happens.
The transaction arguments can be useful while developing a test case.
> is it really so important to be able to control these parameters?
>
> can you find some values for the parameters so that the test works (as
> in, it actually tests what it's supposed to) and also so that the whole
> unit test ends in less than 30 seconds?
I think the defaults are fine for that, no?
>
>> +{
>> + struct args args = {
>> + .iterations = 1,
>> + };
>> + unsigned int i;
>> + long arg;
>> + bool no_arg;
>> + char *end;
>> +
>> + 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;
>> + }
>> +
>> + if (!strcmp("--iterations", argv[i])) {
>> + if (no_arg)
>> + report_abort("--iterations needs a
>> positive parameter");
>> + args.iterations = arg;
>> + ++i;
>> + } else {
>> + 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);
>> + test_spec_ex(&args, &spec_ex_triggers[i]);
>> + report_prefix_pop();
>> + }
>> + report_prefix_pop();
>> +
>> + return report_summary();
>> +}
>> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
>> index 9e1802f..5f43d52 100644
>> --- a/s390x/unittests.cfg
>> +++ b/s390x/unittests.cfg
>> @@ -109,3 +109,6 @@ file = edat.elf
>>
>> [mvpg-sie]
>> file = mvpg-sie.elf
>> +
>> +[spec_ex]
>> +file = spec_ex.elf
>
