s390x: More emulation tests
cover s390x: More emulation tests
The first patch allows for CECSIM booting via PSW restart. The other ones add diag288 and STSI tests.
I chose to start with these since they are low controversy. My queue still contains the sclp patches and a simple smp library with tests. They will follow later.
Janosch Frank (3):
s390x: Support PSW restart boot
s390x: Diag288 test
s390x: STSI tests
s390x/Makefile | 2 + s390x/diag288.c | 111 +++++++++++++++++++++++++++++++++++++++ s390x/flat.lds | 14 +++-- s390x/stsi.c | 123 ++++++++++++++++++++++++++++++++++++++++++++ s390x/unittests.cfg | 7 +++ 5 files changed, 252 insertions(+), 5 deletions(-) create mode 100644 s390x/diag288.c create mode 100644 s390x/stsi.c
Signature · 1 line
2.17.0
PATCH 1/3 s390x: Support PSW restart boot
Add a boot PSW to PSW restart new, so we can also boot via a PSW restart.
--- s390x/flat.lds | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
s390x/flat.lds +9 -5
@@ -1,14 +1,18 @@ SECTIONS { - /* - * Initial short psw for disk boot, with 31 bit addressing for - * non z/Arch environment compatibility and the instruction - * address 0x10000 (cstart64.S .init). - */ .lowcore : { + /* + * Initial short psw for disk boot, with 31 bit addressing for + * non z/Arch environment compatibility and the instruction + * address 0x10000 (cstart64.S .init). + */ . = 0; LONG(0x00080000) LONG(0x80010000) + /* Restart new PSW for booting via PSW restart. */ + . = 0x1a0; + QUAD(0x0000000180000000) + QUAD(0x0000000000010000) } . = 0x10000; .text : {
Signature · 1 line
2.17.0
Re: [kvm-unit-tests PATCH 1/3] s390x: Support PSW restart boot
Quoted 27 lines · Janosch Frank
Add a boot PSW to PSW restart new, so we can also boot via a PSW restart.
Signed-off-by Janosch Frank <[email protected]>--- s390x/flat.lds | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)s390x/flat.lds +5
-5@@ -1,14 +1,18 @@ SECTIONS { - /* - * Initial short psw for disk boot, with 31 bit addressing for - * non z/Arch environment compatibility and the instruction - * address 0x10000 (cstart64.S .init). - */ .lowcore : { + /* + * Initial short psw for disk boot, with 31 bit addressing for + * non z/Arch environment compatibility and the instruction + * address 0x10000 (cstart64.S .init). + */
s390x/flat.lds line 18
Quoted 10 lines
s390x/flat.lds +4
-0. = 0; LONG(0x00080000) LONG(0x80010000) + /* Restart new PSW for booting via PSW restart. */ + . = 0x1a0; + QUAD(0x0000000180000000) + QUAD(0x0000000000010000) } . = 0x10000; .text : {
PATCH 2/3 s390x: Diag288 test
In reply to Janosch FrankA small test for the watchdog via diag288.
Minimum timer value is 15 (seconds) and the only supported action with QEMU is restart.
--- s390x/Makefile | 1 + s390x/diag288.c | 111 ++++++++++++++++++++++++++++++++++++++++++++ s390x/unittests.cfg | 4 ++ 3 files changed, 116 insertions(+) create mode 100644 s390x/diag288.c
s390x/Makefile +1 -0
@@ -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
s390x/diag288.c new file +111 -0
111 lines not on this page · raw message
s390x/unittests.cfg +4 -0
@@ -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
Signature · 1 line
2.17.0
Re: [kvm-unit-tests PATCH 2/3] s390x: Diag288 test
Quoted 38 lines · Janosch Frank
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 <[email protected]>--- s390x/Makefile | 1 + s390x/diag288.c | 111 ++++++++++++++++++++++++++++++++++++++++++++ s390x/unittests.cfg | 4 ++ 3 files changed, 116 insertions(+) create mode 100644 s390x/diag288.cs390x/Makefile +1
-0@@ -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_binarys390x/diag288.c new file +7
-0@@ -0,0 +1,111 @@ +/* +* Timer Event DIAG288 test +* +* Copyright (c) 2019 IBM Corp +* +* Authors: +* Janosch Frank <[email protected]>
s390x/diag288.c line 17
Quoted 10 lines
s390x/diag288.c new file +10
-0+* +* 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" ?
... 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...
Quoted 11 lines
s390x/diag288.c new file +11
-0+#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;
s390x/diag288.c line 39
Quoted 10 lines
s390x/diag288.c new file +10
-0+ register unsigned long ac asm("2") = action; + + asmvolatile ("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.
s390x/diag288.c line 48
Quoted 9 lines
s390x/diag288.c new file +9
-0+ register unsigned long action asm("2") = 0; + + asmvolatile ("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?
s390x/diag288.c line 56
Quoted 7 lines
s390x/diag288.c new file +7
-0+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?
s390x/diag288.c line 62
Quoted 6 lines
s390x/diag288.c new file +6
-0+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" ?
Quoted 16 lines
s390x/diag288.c new file +16
-0+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 ();
s390x/diag288.c line 88
Quoted 10 lines
s390x/diag288.c new file +10
-0+ +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) ?
s390x/diag288.c line 101
Quoted 13 lines
s390x/diag288.c new file +13
-0+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?
Quoted 15 lines
s390x/diag288.c new file +10
-0+} + +int main (void ) +{ +report_prefix_push ("diag288" ); +test_priv (); +test_specs (); +test_bite (); +return report_summary (); +}s390x/unittests.cfg +0
-0
s390x/unittests.cfg file = gs.elf · line 67
Quoted 8 lines
s390x/unittests.cfg +4
-0@@ -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
Nit: Add newline (well, it gets added by the next patch, but if you touch this patch again anyway...)
Thomas
Re: [kvm-unit-tests PATCH 2/3] s390x: Diag288 test
Quoted 43 lines · Thomas Huth
Quoted 42 lines · Janosch Frank
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 <[email protected]>--- s390x/Makefile | 1 + s390x/diag288.c | 111 ++++++++++++++++++++++++++++++++++++++++++++ s390x/unittests.cfg | 4 ++ 3 files changed, 116 insertions(+) create mode 100644 s390x/diag288.cs390x/Makefile +1
-0@@ -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_binarys390x/diag288.c new file +11
-0@@ -0,0 +1,111 @@ +/* +* Timer Event DIAG288 test +* +* Copyright (c) 2019 IBM Corp +* +* Authors: +* Janosch Frank <[email protected]> +* +* 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. +*/
Quoted 8 lines
s390x/diag288.c line 17
Quoted 6 lines
s390x/diag288.c new file +6
-0+ +#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.
Quoted 4 lines
... 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?
Quoted 18 lines
Quoted 18 lines
s390x/diag288.c new file +17
-0+#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; + + asmvolatile ("diag %0,%2,0x288" + : :"d" (fc),"d" (tm),"d" (ac)); +} +
Quoted 8 lines
s390x/diag288.c line 39
Quoted 4 lines
s390x/diag288.c new file +4
-0+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"
Quoted 14 lines
s390x/diag288.c line 48
Quoted 10 lines
s390x/diag288.c new file +9
-0+ register unsigned long action asm("2") = 0; + + asmvolatile ("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.
Quoted 48 lines
s390x/diag288.c line 56
Quoted 8 lines
s390x/diag288.c new file +7
-0+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?
s390x/diag288.c line 62
Quoted 6 lines
s390x/diag288.c new file +6
-0+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" ?
Quoted 16 lines
s390x/diag288.c new file +16
-0+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 ();s390x/diag288.c line 88
Quoted 10 lines
s390x/diag288.c new file +10
-0+ +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) ?
Quoted 15 lines
s390x/diag288.c line 101
Quoted 13 lines
s390x/diag288.c new file +13
-0+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?
Quoted 17 lines
Quoted 17 lines
s390x/diag288.c new file +10
-0+} + +int main (void ) +{ +report_prefix_push ("diag288" ); +test_priv (); +test_specs (); +test_bite (); +return report_summary (); +}s390x/unittests.cfg +0
-0@@ -61,3 +61,7 @@ file = gs.elf
Quoted 10 lines
s390x/unittests.cfg file = gs.elf · line 67
Quoted 7 lines
s390x/unittests.cfg +4
-0[iep] file = iep.elf + +[diag288] +file = diag288.elf +extra_params=-device diag288,id=watchdog0 --watchdog-action inject-nmiNit: Add newline (well, it gets added by the next patch, but if you touch this patch again anyway...)
Ok
Quoted 3 lines
Thomas
Re: [kvm-unit-tests PATCH 2/3] s390x: Diag288 test
Quoted 53 lines · Janosch Frank
Quoted 43 lines · Thomas Huth
Quoted 42 lines · Janosch Frank
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 <[email protected]>--- s390x/Makefile | 1 + s390x/diag288.c | 111 ++++++++++++++++++++++++++++++++++++++++++++ s390x/unittests.cfg | 4 ++ 3 files changed, 116 insertions(+) create mode 100644 s390x/diag288.cs390x/Makefile +1
-0@@ -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_binarys390x/diag288.c new file +11
-0@@ -0,0 +1,111 @@ +/* +* Timer Event DIAG288 test +* +* Copyright (c) 2019 IBM Corp +* +* Authors: +* Janosch Frank <[email protected]> +* +* 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. +*/ Quoted 8 lines
s390x/diag288.c line 17
Quoted 6 lines
s390x/diag288.c new file +6
-0+ +#include <libcflat.h> +#include <asm/asm-offsets.h> +#include <asm/interrupt.h> + +struct lowcore *lc = (void *)0x0 ;Maybe use "NULL" instead of "(void *)0x0" ?
Quoted 2 lines
Well I'd rather have:
struct lowcore *lc = (struct lowcore *)0x0;
Fine for me, too.
Quoted 5 lines
Quoted 3 lines
... 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?
No clue. Maybe lib/s390x/mmu.c ? Or a new file called lowcore.c ?
Quoted 9 lines
Quoted 8 lines
s390x/diag288.c line 39
Quoted 4 lines
s390x/diag288.c new file +4
-0+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.
Quoted 3 lines
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"
Yes, I think that's easier to understand.
BTW, is there another documentation of diag 288 beside the "CP programming services" manual? At least my version of that specification does not say that the fc register has to be even...
Quoted 12 lines
Quoted 11 lines
s390x/diag288.c line 101
Quoted 9 lines
s390x/diag288.c new file +9
-0+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?
Quoted 2 lines
Well
run_tests.shdoes timeout externally.
Do you need it backed into the test?
I sometimes also run the tests without the wrapper script, so in that case it would be convenient ... but I can also quit QEMU manually in that case, so it's not a big issue.
Thomas
Re: [kvm-unit-tests PATCH 2/3] s390x: Diag288 test
Quoted 3 lines · Thomas Huth
Quoted 2 lines · Janosch Frank
Quoted 1 line · Thomas Huth
On 8/20/19 12:55 PM, Janosch Frank wrote:
[...]
Quoted 19 lines
Quoted 5 lines
Quoted 3 lines
... 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?
No clue. Maybe
lib/s390x/mmu.c? Or a new file calledlowcore.c?Quoted 11 lines
Quoted 8 lines
s390x/diag288.c line 39
Quoted 4 lines
s390x/diag288.c new file +4
-0+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:
Quoted 7 lines
Quoted 1 line
"diag %r1,%r2,0x288"
Yes, I think that's easier to understand.
BTW, is there another documentation of diag 288 beside the "CP programming services" manual? At least my version of that specification does not say that the fc register has to be even...
I used the non-public lpar documentation...
Quoted 13 lines
Quoted 13 lines
Quoted 12 lines
s390x/diag288.c line 101
Quoted 10 lines
s390x/diag288.c new file +9
-0+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?
Quoted 6 lines
Quoted 2 lines
Well
run_tests.shdoes timeout externally.
Do you need it backed into the test?I sometimes also run the tests without the wrapper script, so in that case it would be convenient ... but I can also quit QEMU manually in that case, so it's not a big issue.
How about setting the clock comparator, that should trigger an unexpected external interrupt?
Quoted 3 lines
Thomas
Re: [kvm-unit-tests PATCH 2/3] s390x: Diag288 test
Quoted 26 lines · Janosch Frank
Quoted 3 lines · Thomas Huth
Quoted 2 lines · Janosch Frank
Quoted 1 line · Thomas Huth
On 8/20/19 12:55 PM, Janosch Frank wrote:
[...]
Quoted 21 lines
Quoted 5 lines
Quoted 3 lines
... 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?
No clue. Maybe
lib/s390x/mmu.c? Or a new file calledlowcore.c?Quoted 12 lines
Quoted 8 lines
s390x/diag288.c line 39
Quoted 4 lines
s390x/diag288.c new file +4
-0+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"
Quoted 7 lines
Quoted 5 lines
Yes, I think that's easier to understand.
BTW, is there another documentation of diag 288 beside the "CP programming services" manual? At least my version of that specification does not say that the fc register has to be even...
I used the non-public lpar documentation...
Ok, if it's specified there, then the check is fine with me.
Quoted 15 lines
Quoted 15 lines
Quoted 14 lines
Quoted 11 lines
s390x/diag288.c line 101
Quoted 9 lines
s390x/diag288.c new file +9
-0+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.shdoes timeout externally.
Do you need it backed into the test?
Quoted 6 lines
Quoted 3 lines
I sometimes also run the tests without the wrapper script, so in that case it would be convenient ... but I can also quit QEMU manually in that case, so it's not a big issue.
How about setting the clock comparator, that should trigger an unexpected external interrupt?
Sounds like an idea (if this is not getting too complicated... otherwise just leave it as it is).
Thomas
PATCH 3/3 s390x: STSI tests
In reply to Janosch FrankFor now let's concentrate on the error conditions.
--- s390x/Makefile | 1 + s390x/stsi.c | 123 ++++++++++++++++++++++++++++++++++++++++++++ s390x/unittests.cfg | 5 +- 3 files changed, 128 insertions(+), 1 deletion(-) create mode 100644 s390x/stsi.c
s390x/Makefile +1 -0
@@ -12,6 +12,7 @@ tests += $(TEST_DIR)/vector.elf tests += $(TEST_DIR)/gs.elf tests += $(TEST_DIR)/iep.elf tests += $(TEST_DIR)/diag288.elf +tests += $(TEST_DIR)/stsi.elf tests_binary = $(patsubst %.elf,%.bin,$(tests)) all: directories test_cases test_cases_binary
s390x/stsi.c new file +123 -0
123 lines not on this page · raw message
s390x/unittests.cfg +4 -1
@@ -64,4 +64,7 @@ file = iep.elf [diag288] file = diag288.elf -extra_params=-device diag288,id=watchdog0 --watchdog-action inject-nmi +extra_params=-device diag288,id=watchdog0 --watchdog-action inject-nmi + +[stsi] +file = stsi.elf
Signature · 1 line
2.17.0
Re: [kvm-unit-tests PATCH 3/3] s390x: STSI tests
Quoted 39 lines · Janosch Frank
For now let's concentrate on the error conditions.
Signed-off-by Janosch Frank <[email protected]>--- s390x/Makefile | 1 + s390x/stsi.c | 123 ++++++++++++++++++++++++++++++++++++++++++++ s390x/unittests.cfg | 5 +- 3 files changed, 128 insertions(+), 1 deletion(-) create mode 100644 s390x/stsi.cs390x/Makefile +1
-0@@ -12,6 +12,7 @@ tests += $(TEST_DIR)/vector.elf tests += $(TEST_DIR)/gs.elf tests += $(TEST_DIR)/iep.elf tests += $(TEST_DIR)/diag288.elf +tests += $(TEST_DIR)/stsi.elf tests_binary = $(patsubst %.elf,%.bin,$(tests)) all: directories test_cases test_cases_binarys390x/stsi.c new file +11
-0@@ -0,0 +1,123 @@ +/* +* Store System Information tests +* +* Copyright (c) 2019 IBM Corp +* +* Authors: +* Janosch Frank <[email protected]> +* +* 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. +*/
s390x/stsi.c line 21
Quoted 10 lines
s390x/stsi.c new file +10
-0+ +#include <libcflat.h> +#include <asm/page.h> +#include <asm/asm-offsets.h> +#include <asm/interrupt.h> + +static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2))); + +static inline unsigned long stsi (unsigned long *addr, +unsigned long fc,uint8_t sel1,uint8_t sel2)
Return code should be "int", not "long".
I'd also suggest to use "void *addr" instead of "unsigned long *addr", then you don't have to cast the pagebuf when you're calling this function.
s390x/stsi.c line 34
Quoted 13 lines
s390x/stsi.c new file +13
-0+{ + register unsigned long r0 asm("0") = (fc << 28) | sel1; + register unsigned long r1 asm("1") = sel2; +int cc; + + asmvolatile ("stsi 0(%3)\n" +"ipm %[cc]\n" +"srl %[cc],28\n" + :"+d" (r0), [cc]"=d" (cc) + :"d" (r1),"a" (addr) + :"cc" ,"memory" ); +return cc; +}
Bonus points for putting that function into a header and re-use it in skey.c (maybe in a separate patch, though).
s390x/stsi.c line 42
Quoted 7 lines
s390x/stsi.c new file +7
-0+static inline void stsi_zero_r0 (unsigned long *addr, +unsigned long fc,uint8_t sel1,uint8_t sel2) +{ + register unsigned long r0 asm("0") = (fc << 28) | (1 << 8) | sel1; + register unsigned long r1 asm("1") = sel2; + +
Please remove one empty line.
s390x/stsi.c line 55
Quoted 13 lines
s390x/stsi.c new file +13
-0+ asmvolatile ("stsi 0(%2)" + :"+d" (r0) + :"d" (r1),"a" (addr) + :"cc" ,"memory" ); +} + +static inline void stsi_zero_r1 (unsigned long *addr, +unsigned long fc,uint8_t sel1,uint8_t sel2) +{ + register unsigned long r0 asm("0") = (fc << 28) | sel1; + register unsigned long r1 asm("1") = (1 << 16) | sel2; + +
dito
s390x/stsi.c line 47
Quoted 5 lines
s390x/stsi.c new file +5
-0+ asmvolatile ("stsi 0(%2)" + :"+d" (r0) + :"d" (r1),"a" (addr) + :"cc" ,"memory" ); +}
Also not sure whether you need separate functions for these tests ... you could also change the type of sel1 and sel2 from "uint8_t" to "int" in the stsi() function and then pass the invalid types to that function instead?
s390x/stsi.c line 67
Quoted 6 lines
s390x/stsi.c new file +6
-0+static inline unsigned long stsi_get_fc (unsigned long *addr) +{ + register unsigned long r0 asm("0") = 0; + register unsigned long r1 asm("1") = 0; + +
Superfluous empty line again.
s390x/stsi.c line 46
Quoted 4 lines
s390x/stsi.c new file +4
-0+ asmvolatile ("stsi 0(%2)" + :"+d" (r0) + :"d" (r1),"a" (addr) + :"cc" ,"memory" );
Maybe assert that the CC is 0 after the call?
s390x/stsi.c line 77
Quoted 6 lines
s390x/stsi.c new file +6
-0+return r0 >>28 ; +} + +static void test_specs (void ) +{ +report_prefix_push ("spec ex" );
s/spec ex/specification/ please
Quoted 24 lines
s390x/stsi.c new file +24
-0+report_prefix_push ("inv r0" ); +expect_pgm_int (); +stsi_zero_r0 ((void *)pagebuf,1 ,0 ,0 ); +check_pgm_int_code (PGM_INT_CODE_SPECIFICATION ); +report_prefix_pop (); + +report_prefix_push ("inv r1" ); +expect_pgm_int (); +stsi_zero_r1 ((void *)pagebuf,1 ,0 ,0 ); +check_pgm_int_code (PGM_INT_CODE_SPECIFICATION ); +report_prefix_pop (); + +report_prefix_push ("unaligned" ); +expect_pgm_int (); +stsi ((void *)pagebuf +42 ,1 ,0 ,0 ); +check_pgm_int_code (PGM_INT_CODE_SPECIFICATION ); +report_prefix_pop (); + +report_prefix_pop (); +} + +static void test_priv (void ) +{ +report_prefix_push ("privileged" );
s390x/stsi.c line 112
Quoted 10 lines
s390x/stsi.c new file +10
-0+expect_pgm_int (); +enter_pstate (); +stsi ((void *)pagebuf,0 ,0 ,0 ); +check_pgm_int_code (PGM_INT_CODE_PRIVILEGED_OPERATION ); +report_prefix_pop (); +} + +static void test_fc (void ) +{ +report ("cc == 3" ,stsi ((void *)pagebuf,7 ,0 ,0 ));
Shouldn't that line look like this instead:
report("cc == 3", stsi((void *)pagebuf, 7, 0, 0) == 3);?
s390x/stsi.c line 113
Quoted 1 line
s390x/stsi.c new file +1
-0+report ("r0 == 3" ,stsi_get_fc ((void *)pagebuf));
report("r0 >= 3", stsi_get_fc((void *)pagebuf) >= 3);?
s390x/stsi.c line 123
Quoted 10 lines
s390x/stsi.c new file +10
-0+} + +int main (void ) +{ +report_prefix_push ("stsi" ); +test_priv (); +test_specs (); +test_fc (); +return report_summary (); +}
How about adding another test for access exceptions? Activate low address protection, then store to address 4096 ... and/or check "stsi((void *)-0xdeadadd, 1, 0, 0);" ?
Thomas
Re: [kvm-unit-tests PATCH 3/3] s390x: STSI tests
Quoted 53 lines · Thomas Huth
Quoted 39 lines · Janosch Frank
For now let's concentrate on the error conditions.
Signed-off-by Janosch Frank <[email protected]>--- s390x/Makefile | 1 + s390x/stsi.c | 123 ++++++++++++++++++++++++++++++++++++++++++++ s390x/unittests.cfg | 5 +- 3 files changed, 128 insertions(+), 1 deletion(-) create mode 100644 s390x/stsi.cs390x/Makefile +1
-0@@ -12,6 +12,7 @@ tests += $(TEST_DIR)/vector.elf tests += $(TEST_DIR)/gs.elf tests += $(TEST_DIR)/iep.elf tests += $(TEST_DIR)/diag288.elf +tests += $(TEST_DIR)/stsi.elf tests_binary = $(patsubst %.elf,%.bin,$(tests)) all: directories test_cases test_cases_binarys390x/stsi.c new file +11
-0@@ -0,0 +1,123 @@ +/* +* Store System Information tests +* +* Copyright (c) 2019 IBM Corp +* +* Authors: +* Janosch Frank <[email protected]> +* +* 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. +*/ s390x/stsi.c line 21
Quoted 10 lines
s390x/stsi.c new file +10
-0+ +#include <libcflat.h> +#include <asm/page.h> +#include <asm/asm-offsets.h> +#include <asm/interrupt.h> + +static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2))); + +static inline unsigned long stsi (unsigned long *addr, +unsigned long fc,uint8_t sel1,uint8_t sel2)Return code should be "int", not "long".
Quoted 2 lines
I'd also suggest to use "void *addr" instead of "unsigned long *addr", then you don't have to cast the pagebuf when you're calling this function.
Ok
Quoted 17 lines
Quoted 4 lines
s390x/stsi.c new file +3
-0+{ + register unsigned long r0 asm("0") = (fc << 28) | sel1; + register unsigned long r1 asm("1") = sel2;s390x/stsi.c line 34
Quoted 10 lines
s390x/stsi.c new file +10
-0+int cc; + + asmvolatile ("stsi 0(%3)\n" +"ipm %[cc]\n" +"srl %[cc],28\n" + :"+d" (r0), [cc]"=d" (cc) + :"d" (r1),"a" (addr) + :"cc" ,"memory" ); +return cc; +}Bonus points for putting that function into a header and re-use it in
skey.c(maybe in a separate patch, though).
I forgot that you added that...
How about moving it to lib/s390/asm/arch_def.h ?
[...]
Quoted 9 lines
s390x/stsi.c line 112
Quoted 3 lines
s390x/stsi.c new file +3
-0+static void test_fc (void ) +{ +report ("cc == 3" ,stsi ((void *)pagebuf,7 ,0 ,0 ));Shouldn't that line look like this instead:
report("cc == 3", stsi((void *)pagebuf, 7, 0, 0) == 3);?
Yes
Quoted 6 lines
s390x/stsi.c line 113
Quoted 2 lines
s390x/stsi.c new file +1
-0+report ("r0 == 3" ,stsi_get_fc ((void *)pagebuf));report("r0 >= 3", stsi_get_fc((void *)pagebuf) >= 3);?
Well rather >= 2 because we can also run on lpar with some additional patches applied. Time to test this under lpar...
Quoted 15 lines
s390x/stsi.c line 123
Quoted 11 lines
s390x/stsi.c new file +10
-0+} + +int main (void ) +{ +report_prefix_push ("stsi" ); +test_priv (); +test_specs (); +test_fc (); +return report_summary (); +}How about adding another test for access exceptions? Activate low address protection, then store to address 4096 ... and/or check "
stsi((void *)-0xdeadadd, 1, 0, 0);" ?
Sounds good
Quoted 3 lines
Thomas
Quoted 12 lines · Janosch Frank
The first patch allows for CECSIM booting via PSW restart. The other ones add diag288 and STSI tests.
I chose to start with these since they are low controversy. My queue still contains the sclp patches and a simple smp library with tests. They will follow later.
Janosch Frank (3):
s390x: Support PSW restart boot
s390x: Diag288 test
s390x: STSI tests
Quoted 9 lines
s390x/Makefile | 2 + s390x/diag288.c | 111 +++++++++++++++++++++++++++++++++++++++ s390x/flat.lds | 14 +++-- s390x/stsi.c | 123 ++++++++++++++++++++++++++++++++++++++++++++ s390x/unittests.cfg | 7 +++ 5 files changed, 252 insertions(+), 5 deletions(-) create mode 100644 s390x/diag288.c create mode 100644 s390x/stsi.c
Just wondering, did you try them with TCG as well? (or do I have to test)
Signature · 3 lines
Thanks, David / dhildenb
Quoted 23 lines · David Hildenbrand
Quoted 12 lines · Janosch Frank
The first patch allows for CECSIM booting via PSW restart. The other ones add diag288 and STSI tests.
I chose to start with these since they are low controversy. My queue still contains the sclp patches and a simple smp library with tests. They will follow later.
Janosch Frank (3):
s390x: Support PSW restart boot
s390x: Diag288 test
s390x: STSI testsQuoted 9 lines
s390x/Makefile | 2 + s390x/diag288.c | 111 +++++++++++++++++++++++++++++++++++++++ s390x/flat.lds | 14 +++-- s390x/stsi.c | 123 ++++++++++++++++++++++++++++++++++++++++++++ s390x/unittests.cfg | 7 +++ 5 files changed, 252 insertions(+), 5 deletions(-) create mode 100644 s390x/diag288.c create mode 100644 s390x/stsi.c
Quoted 2 lines
Just wondering, did you try them with TCG as well? (or do I have to test)
No, they are also pending for z/VM and LPAR testing (well at least for STSI).
They have been running with a fmt2 and a fmt4 SIE description though.
I'll speak with the CI people here to add a TCG test.
Quoted 6 lines · Janosch Frank
The first patch allows for CECSIM booting via PSW restart. The other ones add diag288 and STSI tests.
I chose to start with these since they are low controversy. My queue still contains the sclp patches and a simple smp library with tests. They will follow later.
On which branch do these patches apply? I fail to am 2+3 on master (well I didn't try too hard to resolve ;) ). Do you have a branch somewhere?
Signature · 3 lines
Thanks, David / dhildenb
Quoted 11 lines · David Hildenbrand
Quoted 6 lines · Janosch Frank
The first patch allows for CECSIM booting via PSW restart. The other ones add diag288 and STSI tests.
I chose to start with these since they are low controversy. My queue still contains the sclp patches and a simple smp library with tests. They will follow later.
On which branch do these patches apply? I fail to am 2+3 on master (well I didn't try too hard to resolve ;) ). Do you have a branch somewhere?
That is currently on top of master (24efc22), the only merge conflicts might be s390x/Makefile or unittests.conf if your branch is not clean. I'm trying to get a public github account for that and qemu.
Quoted 9 lines · Janosch Frank
Quoted 8 lines · David Hildenbrand
Quoted 6 lines · Janosch Frank
The first patch allows for CECSIM booting via PSW restart. The other ones add diag288 and STSI tests.
I chose to start with these since they are low controversy. My queue still contains the sclp patches and a simple smp library with tests. They will follow later.
Quoted 8 lines
Quoted 3 lines
On which branch do these patches apply? I fail to am 2+3 on master (well I didn't try too hard to resolve ;) ). Do you have a branch somewhere?
That is currently on top of master (24efc22), the only merge conflicts might be s390x/Makefile or unittests.conf if your branch is not clean. I'm trying to get a public github account for that and qemu.
t460s: ~/git/kvm-unit-tests master $ git fetch origin
t460s: ~/git/kvm-unit-tests master $ git reset --hard origin/master HEAD is now at 03b1e45 x86: Support environments without test-devices
t460s: ~/git/kvm-unit-tests master $ git am \[kvm-unit-tests\ PATCH\ *
Applying: s390x: Support PSW restart boot
Applying: s390x: Diag288 test
error: patch failed: s390x/Makefile:11
error: s390x/Makefile: patch does not apply
error: patch failed: s390x/unittests.cfg:61
error: s390x/unittests.cfg: patch does not apply
Patch failed at 0002 s390x: Diag288 test
hint: Use 'git am --show-current-patch' to see the failed patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort".
Are you sure?
Signature · 3 lines
Thanks, David / dhildenb
Quoted 35 lines · David Hildenbrand
Quoted 9 lines · Janosch Frank
Quoted 8 lines · David Hildenbrand
Quoted 6 lines · Janosch Frank
The first patch allows for CECSIM booting via PSW restart. The other ones add diag288 and STSI tests.
I chose to start with these since they are low controversy. My queue still contains the sclp patches and a simple smp library with tests. They will follow later.
Quoted 8 lines
Quoted 3 lines
On which branch do these patches apply? I fail to am 2+3 on master (well I didn't try too hard to resolve ;) ). Do you have a branch somewhere?
That is currently on top of master (24efc22), the only merge conflicts might be s390x/Makefile or unittests.conf if your branch is not clean. I'm trying to get a public github account for that and qemu.
t460s: ~/git/kvm-unit-tests master $ git fetch origin
t460s: ~/git/kvm-unit-tests master $ git reset --hard origin/master HEAD is now at 03b1e45 x86: Support environments without test-devices
t460s: ~/git/kvm-unit-tests master $ git am \[kvm-unit-tests\ PATCH\ *
Applying: s390x: Support PSW restart boot
Applying: s390x: Diag288 test
error: patch failed: s390x/Makefile:11
error: s390x/Makefile: patch does not apply
error: patch failed: s390x/unittests.cfg:61
error: s390x/unittests.cfg: patch does not apply
Patch failed at 0002 s390x: Diag288 test
hint: Use 'git am --show-current-patch' to see the failed patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort".
Quoted 2 lines
Are you sure?
The internal mirror did not pick up the changes of the last few days...