Re: [kvm-unit-tests PATCH v4 1/2] s390x: Add specification exception test

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

On 1/13/22 08:56, Claudio Imbrenda wrote:
> On Tue, 11 Jan 2022 17:39:00 +0100
> Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
> 
>> Generate specification exceptions and check that they occur.
>>
>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>> ---
>>  s390x/Makefile      |   1 +
>>  s390x/spec_ex.c     | 154 ++++++++++++++++++++++++++++++++++++++++++++
>>  s390x/unittests.cfg |   3 +
>>  3 files changed, 158 insertions(+)
>>  create mode 100644 s390x/spec_ex.c
>>
>> diff --git a/s390x/Makefile b/s390x/Makefile
>> index 1e567c1..5635c08 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -25,6 +25,7 @@ tests += $(TEST_DIR)/uv-host.elf
>>  tests += $(TEST_DIR)/edat.elf
>>  tests += $(TEST_DIR)/mvpg-sie.elf
>>  tests += $(TEST_DIR)/spec_ex-sie.elf
>> +tests += $(TEST_DIR)/spec_ex.elf
>>  tests += $(TEST_DIR)/firq.elf
>>  
>>  tests_binary = $(patsubst %.elf,%.bin,$(tests))
>> diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
>> new file mode 100644
>> index 0000000..a9f9f31
>> --- /dev/null
>> +++ b/s390x/spec_ex.c
>> @@ -0,0 +1,154 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright IBM Corp. 2021
>> + *
>> + * Specification exception test.
>> + * Tests that specification exceptions occur when expected.
>> + *
>> + * Can be extended by adding triggers to spec_ex_triggers, see comments below.
>> + */
>> +#include <stdlib.h>
>> +#include <libcflat.h>
>> +#include <asm/interrupt.h>
>> +
>> +static struct lowcore *lc = (struct lowcore *) 0;
>> +
>> +static bool invalid_psw_expected;
>> +static struct psw expected_psw;
>> +static struct psw invalid_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)
>> +{
>> +	// signal occurrence of invalid psw fixup
>> +	invalid_psw_expected = false;
>> +	invalid_psw = lc->pgm_old_psw;
>> +	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.
> 
> is the * just a typo?

Must be :)

[...]

>> +static void test_spec_ex(const struct spec_ex_trigger *trigger)
>> +{
>> +	uint16_t expected_pgm = PGM_INT_CODE_SPECIFICATION;
>> +	uint16_t pgm;
>> +	int rc;
>> +
>> +	expect_pgm_int();
>> +	register_pgm_cleanup_func(trigger->fixup);
>> +	rc = trigger->func();
>> +	register_pgm_cleanup_func(NULL);
>> +	if (rc)
>> +		return;
> 
> why do you exit early in case of failure? (moreover, your are not even
> reporting the failure)

In that case it's the triggers responsibility to report. That is, we're testing
for additional conditions specific to the trigger and one of those failed.
Might not make sense to check if the expected interrupt occurred, so we don't.
Basically it's because check_invalid_psw might fail in psw_bit_12_is_1.

> 
>> +	pgm = clear_pgm_int();
>> +	report(pgm == expected_pgm, "Program interrupt: expected(%d) == received(%d)",
>> +	       expected_pgm, pgm);
>> +}
>> +
>> +int main(int argc, char **argv)
>> +{
>> +	unsigned int i;
>> +
>> +	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(&spec_ex_triggers[i]);
>> +		report_prefix_pop();
>> +	}
>> +	report_prefix_pop();
>> +
>> +	return report_summary();
>> +}
>> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
>> index 054560c..26510cf 100644
>> --- a/s390x/unittests.cfg
>> +++ b/s390x/unittests.cfg
>> @@ -113,6 +113,9 @@ file = mvpg-sie.elf
>>  [spec_ex-sie]
>>  file = spec_ex-sie.elf
>>  
>> +[spec_ex]
>> +file = spec_ex.elf
>> +
>>  [firq-linear-cpu-ids]
>>  file = firq.elf
>>  timeout = 20
>