On 8/20/19 1:59 PM, Thomas Huth wrote:
> On 8/20/19 12:55 PM, Janosch Frank wrote:
>> A small test for the watchdog via diag288.
>>
>> Minimum timer value is 15 (seconds) and the only supported action with
>> QEMU is restart.
>>
>> Signed-off-by: Janosch Frank <frankja@xxxxxxxxxxxxx>
>> ---
>> s390x/Makefile | 1 +
>> s390x/diag288.c | 111 ++++++++++++++++++++++++++++++++++++++++++++
>> s390x/unittests.cfg | 4 ++
>> 3 files changed, 116 insertions(+)
>> create mode 100644 s390x/diag288.c
>>
>> diff --git a/s390x/Makefile b/s390x/Makefile
>> index 1f21ddb..b654c56 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -11,6 +11,7 @@ tests += $(TEST_DIR)/cmm.elf
>> tests += $(TEST_DIR)/vector.elf
>> tests += $(TEST_DIR)/gs.elf
>> tests += $(TEST_DIR)/iep.elf
>> +tests += $(TEST_DIR)/diag288.elf
>> tests_binary = $(patsubst %.elf,%.bin,$(tests))
>>
>> all: directories test_cases test_cases_binary
>> diff --git a/s390x/diag288.c b/s390x/diag288.c
>> new file mode 100644
>> index 0000000..5abcec4
>> --- /dev/null
>> +++ b/s390x/diag288.c
>> @@ -0,0 +1,111 @@
>> +/*
>> + * Timer Event DIAG288 test
>> + *
>> + * Copyright (c) 2019 IBM Corp
>> + *
>> + * Authors:
>> + * Janosch Frank <frankja@xxxxxxxxxxxxx>
>> + *
>> + * This code is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU Library General Public License version 2.
>> + */
>> +
>> +#include <libcflat.h>
>> +#include <asm/asm-offsets.h>
>> +#include <asm/interrupt.h>
>> +
>> +struct lowcore *lc = (void *)0x0;
>
> Maybe use "NULL" instead of "(void *)0x0" ?
Well I'd rather have:
struct lowcore *lc = (struct lowcore *)0x0;
Than using NULL.
>
> ... maybe we could also introduce such a variable as a global variable
> in lib/s390x/ since this is already the third or fourth time that we use
> it in the kvm-unit-tests...
Sure I also thought about that, any particular place?
>
>> +#define CODE_INIT 0
>> +#define CODE_CHANGE 1
>> +#define CODE_CANCEL 2
>> +
>> +#define ACTION_RESTART 0
>> +
>> +static inline void diag288(unsigned long code, unsigned long time,
>> + unsigned long action)
>> +{
>> + register unsigned long fc asm("0") = code;
>> + register unsigned long tm asm("1") = time;
>> + register unsigned long ac asm("2") = action;
>> +
>> + asm volatile("diag %0,%2,0x288"
>> + : : "d" (fc), "d" (tm), "d" (ac));
>> +}
>> +
>> +static inline void diag288_uneven(void)
>> +{
>> + register unsigned long fc asm("1") = 0;
>> + register unsigned long time asm("1") = 15;
>
> So you're setting register 1 twice? And "time" is not really used in the
> inline assembly below? How's that supposed to work? Looks like a bug to
> me... if not, please explain with a comment in the code here.
Well I'm waiting for a spec exception here, so it doesn't have to work.
I'll probably just remove the register variables and do a:
"diag %r1,%r2,0x288"
>
>> + register unsigned long action asm("2") = 0;
>> +
>> + asm volatile("diag %0,%2,0x288"
>> + : : "d" (fc), "d" (time), "d" (action));
>> +}
>> +
>> +static void test_specs(void)
>> +{
>> + report_prefix_push("spec ex");
>
> After all those Spectre bugs in the last year, "spec ex" makes me think
> of speculative execution first... maybe better use "specification" as
> prefix?
Sure, I'll take the review for the prefixes.
I thought a short prefix makes that more readable, but if it only
confuses, let's use a longer one.
>
>> + report_prefix_push("uneven");
>> + expect_pgm_int();
>> + diag288_uneven();
>> + check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>> + report_prefix_pop();
>> +
>> + report_prefix_push("unsup act");
>
> "unsupported action" ? ... it's not that long, is it?
>
>> + expect_pgm_int();
>> + diag288(CODE_INIT, 15, 42);
>> + check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>> + report_prefix_pop();
>> +
>> + report_prefix_push("unsup fctn");
>
> "unsupported function" ?
>
>> + expect_pgm_int();
>> + diag288(42, 15, ACTION_RESTART);
>> + check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>> + report_prefix_pop();
>> +
>> + report_prefix_push("no init");
>> + expect_pgm_int();
>> + diag288(CODE_CANCEL, 15, ACTION_RESTART);
>> + check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>> + report_prefix_pop();
>> +
>> + report_prefix_push("min timer");
>> + expect_pgm_int();
>> + diag288(CODE_INIT, 14, ACTION_RESTART);
>> + check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>> + report_prefix_pop();
>> +
>> + report_prefix_pop();
>> +}
>> +
>> +static void test_priv(void)
>> +{
>> + report_prefix_push("privileged");
>> + expect_pgm_int();
>> + enter_pstate();
>> + diag288(0, 15, 0);
> diag288(CODE_INIT, 0, ACTION_RESTART) ?
>
>> + check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
>> + report_prefix_pop();
>> +}
>> +
>> +static void test_bite(void)
>> +{
>> + if (lc->restart_old_psw.addr) {
>> + report("restart", true);
>> + return;
>> + }
>> + lc->restart_new_psw.addr = (uint64_t)test_bite;
>> + diag288(CODE_INIT, 15, ACTION_RESTART);
>> + while(1) {};
>
> Should this maybe timeout after a minute or so?
Well run_tests.sh does timeout externally.
Do you need it backed into the test?
>
>> +}
>> +
>> +int main(void)
>> +{
>> + report_prefix_push("diag288");
>> + test_priv();
>> + test_specs();
>> + test_bite();
>> + return report_summary();
>> +}
>> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
>> index 546b1f2..ca10f38 100644
>> --- a/s390x/unittests.cfg
>> +++ b/s390x/unittests.cfg
>> @@ -61,3 +61,7 @@ file = gs.elf
>>
>> [iep]
>> file = iep.elf
>> +
>> +[diag288]
>> +file = diag288.elf
>> +extra_params=-device diag288,id=watchdog0 --watchdog-action inject-nmi
>> \ No newline at end of file
>
> Nit: Add newline (well, it gets added by the next patch, but if you
> touch this patch again anyway...)
Ok
>
> Thomas
>
