[kvm-unit-tests PATCH 0/3] s390x: More emulation tests

[kvm-unit-tests PATCH 0/3] s390x: More emulation tests

[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

 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

-- 
2.17.0

[kvm-unit-tests PATCH 1/3] s390x: Support PSW restart boot

[Janosch Frank]

Add a boot PSW to PSW restart new, so we can also boot via a PSW
restart.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/flat.lds | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/s390x/flat.lds b/s390x/flat.lds
index 403d967..86dffac 100644
--- a/s390x/flat.lds
+++ b/s390x/flat.lds
@@ -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 : {
-- 
2.17.0

[kvm-unit-tests PATCH 2/3] s390x: Diag288 test

[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 <frankja@linux.ibm.com>
---
 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@linux.ibm.com>
+ *
+ * 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;
+
+#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;
+	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");
+
+	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");
+	expect_pgm_int();
+	diag288(CODE_INIT, 15, 42);
+	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+	report_prefix_pop();
+
+	report_prefix_push("unsup fctn");
+	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);
+	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) {};
+}
+
+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
-- 
2.17.0

[kvm-unit-tests PATCH 3/3] s390x: STSI tests

[Janosch Frank]

For now let's concentrate on the error conditions.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/Makefile      |   1 +
 s390x/stsi.c        | 123 ++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |   5 +-
 3 files changed, 128 insertions(+), 1 deletion(-)
 create mode 100644 s390x/stsi.c

diff --git a/s390x/Makefile b/s390x/Makefile
index b654c56..311ab77 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -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
diff --git a/s390x/stsi.c b/s390x/stsi.c
new file mode 100644
index 0000000..005f337
--- /dev/null
+++ b/s390x/stsi.c
@@ -0,0 +1,123 @@
+/*
+ * Store System Information tests
+ *
+ * Copyright (c) 2019 IBM Corp
+ *
+ * Authors:
+ *  Janosch Frank <frankja@linux.ibm.com>
+ *
+ * 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/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)
+{
+	register unsigned long r0 asm("0") = (fc << 28) | sel1;
+	register unsigned long r1 asm("1") = sel2;
+	int cc;
+
+	asm volatile("stsi	0(%3)\n"
+		     "ipm	 %[cc]\n"
+		     "srl	 %[cc],28\n"
+		     : "+d" (r0), [cc] "=d" (cc)
+		     : "d" (r1), "a" (addr)
+		     : "cc", "memory");
+	return cc;
+}
+
+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;
+
+
+	asm volatile("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;
+
+
+	asm volatile("stsi	0(%2)"
+		     : "+d" (r0)
+		     : "d" (r1), "a" (addr)
+		     : "cc", "memory");
+}
+
+static inline unsigned long stsi_get_fc(unsigned long *addr)
+{
+	register unsigned long r0 asm("0") = 0;
+	register unsigned long r1 asm("1") = 0;
+
+
+	asm volatile("stsi	0(%2)"
+		     : "+d" (r0)
+		     : "d" (r1), "a" (addr)
+		     : "cc", "memory");
+	return r0 >> 28;
+}
+
+static void test_specs(void)
+{
+	report_prefix_push("spec ex");
+
+	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");
+	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));
+	report("r0 == 3", stsi_get_fc((void *)pagebuf));
+}
+
+int main(void)
+{
+	report_prefix_push("stsi");
+	test_priv();
+	test_specs();
+	test_fc();
+	return report_summary();
+}
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index ca10f38..c56258a 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -64,4 +64,7 @@ file = iep.elf
 
 [diag288]
 file = diag288.elf
-extra_params=-device diag288,id=watchdog0 --watchdog-action inject-nmi
\ No newline at end of file
+extra_params=-device diag288,id=watchdog0 --watchdog-action inject-nmi
+
+[stsi]
+file = stsi.elf
-- 
2.17.0

Re: [kvm-unit-tests PATCH 0/3] s390x: More emulation tests

[David Hildenbrand]

On 20.08.19 12:55, Janosch Frank wrote:
> 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
> 

Just wondering, did you try them with TCG as well? (or do I have to test)

-- 

Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH 1/3] s390x: Support PSW restart boot

[Thomas Huth]

On 8/20/19 12:55 PM, Janosch Frank wrote:
> Add a boot PSW to PSW restart new, so we can also boot via a PSW
> restart.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/flat.lds | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/s390x/flat.lds b/s390x/flat.lds
> index 403d967..86dffac 100644
> --- a/s390x/flat.lds
> +++ b/s390x/flat.lds
> @@ -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 : {
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [kvm-unit-tests PATCH 0/3] s390x: More emulation tests

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 1182 bytes --]

On 8/20/19 1:11 PM, David Hildenbrand wrote:
> On 20.08.19 12:55, Janosch Frank wrote:
>> 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
>>
> 
> 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.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [kvm-unit-tests PATCH 2/3] s390x: Diag288 test

[Thomas Huth]

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@linux.ibm.com>
> ---
>  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@linux.ibm.com>
> + *
> + * 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...

> +#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.

> +	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?

> +	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?

> +}
> +
> +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...)

 Thomas

Re: [kvm-unit-tests PATCH 2/3] s390x: Diag288 test

[Janosch Frank]

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@linux.ibm.com>
>> ---
>>  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@linux.ibm.com>
>> + *
>> + * 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
> 

Re: [kvm-unit-tests PATCH 2/3] s390x: Diag288 test

[Thomas Huth]

On 8/20/19 2:25 PM, Janosch Frank wrote:
> 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@linux.ibm.com>
>>> ---
>>>  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@linux.ibm.com>
>>> + *
>>> + * 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;

Fine for me, too.

>> ... 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 ?

>>> +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"

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...

>>> +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?

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 3/3] s390x: STSI tests

[Thomas Huth]

On 8/20/19 12:55 PM, Janosch Frank wrote:
> For now let's concentrate on the error conditions.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/Makefile      |   1 +
>  s390x/stsi.c        | 123 ++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg |   5 +-
>  3 files changed, 128 insertions(+), 1 deletion(-)
>  create mode 100644 s390x/stsi.c
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index b654c56..311ab77 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -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
> diff --git a/s390x/stsi.c b/s390x/stsi.c
> new file mode 100644
> index 0000000..005f337
> --- /dev/null
> +++ b/s390x/stsi.c
> @@ -0,0 +1,123 @@
> +/*
> + * Store System Information tests
> + *
> + * Copyright (c) 2019 IBM Corp
> + *
> + * Authors:
> + *  Janosch Frank <frankja@linux.ibm.com>
> + *
> + * 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/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.

> +{
> +	register unsigned long r0 asm("0") = (fc << 28) | sel1;
> +	register unsigned long r1 asm("1") = sel2;
> +	int cc;
> +
> +	asm volatile("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).

> +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.

> +	asm volatile("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

> +	asm volatile("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?

> +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.

> +	asm volatile("stsi	0(%2)"
> +		     : "+d" (r0)
> +		     : "d" (r1), "a" (addr)
> +		     : "cc", "memory");

Maybe assert that the CC is 0 after the call?

> +	return r0 >> 28;
> +}
> +
> +static void test_specs(void)
> +{
> +	report_prefix_push("spec ex");

s/spec ex/specification/ please

> +	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");
> +	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);

?

> +	report("r0 == 3", stsi_get_fc((void *)pagebuf));

    report("r0 >= 3", stsi_get_fc((void *)pagebuf) >= 3);

?

> +}
> +
> +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 2/3] s390x: Diag288 test

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 2091 bytes --]

On 8/20/19 2:55 PM, Thomas Huth wrote:
> On 8/20/19 2:25 PM, Janosch Frank wrote:
>> On 8/20/19 1:59 PM, Thomas Huth wrote:
>>> On 8/20/19 12:55 PM, Janosch Frank wrote:
[...]
>>> ... 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 ?
> 
>>>> +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"
> 
> 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...

> 
>>>> +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?
> 
> 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?

> 
>  Thomas
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [kvm-unit-tests PATCH 2/3] s390x: Diag288 test

[Thomas Huth]

[-- Attachment #1.1: Type: text/plain, Size: 2341 bytes --]

On 8/20/19 5:21 PM, Janosch Frank wrote:
> On 8/20/19 2:55 PM, Thomas Huth wrote:
>> On 8/20/19 2:25 PM, Janosch Frank wrote:
>>> On 8/20/19 1:59 PM, Thomas Huth wrote:
>>>> On 8/20/19 12:55 PM, Janosch Frank wrote:
> [...]
>>>> ... 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 ?
>>
>>>>> +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"
>>
>> 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.

>>>>> +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?
>>
>> 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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [kvm-unit-tests PATCH 0/3] s390x: More emulation tests

[David Hildenbrand]

On 20.08.19 12:55, Janosch Frank wrote:
> 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?

-- 

Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH 3/3] s390x: STSI tests

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 3395 bytes --]

On 8/20/19 3:21 PM, Thomas Huth wrote:
> On 8/20/19 12:55 PM, Janosch Frank wrote:
>> For now let's concentrate on the error conditions.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  s390x/Makefile      |   1 +
>>  s390x/stsi.c        | 123 ++++++++++++++++++++++++++++++++++++++++++++
>>  s390x/unittests.cfg |   5 +-
>>  3 files changed, 128 insertions(+), 1 deletion(-)
>>  create mode 100644 s390x/stsi.c
>>
>> diff --git a/s390x/Makefile b/s390x/Makefile
>> index b654c56..311ab77 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -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
>> diff --git a/s390x/stsi.c b/s390x/stsi.c
>> new file mode 100644
>> index 0000000..005f337
>> --- /dev/null
>> +++ b/s390x/stsi.c
>> @@ -0,0 +1,123 @@
>> +/*
>> + * Store System Information tests
>> + *
>> + * Copyright (c) 2019 IBM Corp
>> + *
>> + * Authors:
>> + *  Janosch Frank <frankja@linux.ibm.com>
>> + *
>> + * 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/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.

Ok

> 
>> +{
>> +	register unsigned long r0 asm("0") = (fc << 28) | sel1;
>> +	register unsigned long r1 asm("1") = sel2;
>> +	int cc;
>> +
>> +	asm volatile("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 ?

[...]

>> +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

> 
>> +	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...

> 
>> +}
>> +
>> +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

> 
>  Thomas
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [kvm-unit-tests PATCH 0/3] s390x: More emulation tests

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 745 bytes --]

On 8/20/19 9:04 PM, David Hildenbrand wrote:
> On 20.08.19 12:55, Janosch Frank wrote:
>> 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.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [kvm-unit-tests PATCH 0/3] s390x: More emulation tests

[David Hildenbrand]

On 21.08.19 10:48, Janosch Frank wrote:
> On 8/20/19 9:04 PM, David Hildenbrand wrote:
>> On 20.08.19 12:55, Janosch Frank wrote:
>>> 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.
> 

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?

-- 

Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH 0/3] s390x: More emulation tests

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 1837 bytes --]

On 8/21/19 10:53 AM, David Hildenbrand wrote:
> On 21.08.19 10:48, Janosch Frank wrote:
>> On 8/20/19 9:04 PM, David Hildenbrand wrote:
>>> On 20.08.19 12:55, Janosch Frank wrote:
>>>> 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.
>>
> 
> 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?
> 


The internal mirror did not pick up the changes of the last few days...


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]