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

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

[Janosch Frank]

The first patch allows for CECSIM booting via PSW restart.
The other ones add diag288 and STSI tests.

v2:

* Tested under TCG
* Split out stsi into library
* Addressed review

Janosch Frank (4):
  s390x: Support PSW restart boot
  s390x: Diag288 test
  s390x: Move stsi to library
  s390x: STSI tests

 lib/s390x/asm/arch_def.h |  17 +++++
 s390x/Makefile           |   2 +
 s390x/diag288.c          | 131 +++++++++++++++++++++++++++++++++++++++
 s390x/flat.lds           |  14 +++--
 s390x/skey.c             |  18 ------
 s390x/stsi.c             |  84 +++++++++++++++++++++++++
 s390x/unittests.cfg      |   7 +++
 7 files changed, 250 insertions(+), 23 deletions(-)
 create mode 100644 s390x/diag288.c
 create mode 100644 s390x/stsi.c

-- 
2.17.0

[kvm-unit-tests PATCH v2 1/4] 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>
Reviewed-by: Thomas Huth <thuth@redhat.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

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

[David Hildenbrand]

On 21.08.19 12:47, 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>
> Reviewed-by: Thomas Huth <thuth@redhat.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: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb

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

[David Hildenbrand]

On 21.08.19 12:47, 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>
> Reviewed-by: Thomas Huth <thuth@redhat.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 : {
> 

Seems you missed my

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb

[kvm-unit-tests PATCH v2 2/4] 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>
---
 lib/s390x/asm/arch_def.h |   1 +
 s390x/Makefile           |   1 +
 s390x/diag288.c          | 131 +++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg      |   4 ++
 4 files changed, 137 insertions(+)
 create mode 100644 s390x/diag288.c

diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index d2cd727..4bbb428 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -15,6 +15,7 @@ struct psw {
 	uint64_t	addr;
 };
 
+#define PSW_MASK_EXT			0x0100000000000000UL
 #define PSW_MASK_DAT			0x0400000000000000UL
 #define PSW_MASK_PSTATE			0x0001000000000000UL
 
diff --git a/s390x/Makefile b/s390x/Makefile
index 574a9a2..3453373 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)/cpumodel.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..c129b6a
--- /dev/null
+++ b/s390x/diag288.c
@@ -0,0 +1,131 @@
+/*
+ * 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 = (struct lowcore *)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 void test_specs(void)
+{
+	report_prefix_push("specification");
+
+	report_prefix_push("uneven");
+	expect_pgm_int();
+	asm volatile("diag %r1,%r2,0x288");
+	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+	report_prefix_pop();
+
+	report_prefix_push("unsupported action");
+	expect_pgm_int();
+	diag288(CODE_INIT, 15, 42);
+	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+	report_prefix_pop();
+
+	report_prefix_push("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(CODE_INIT, 15, ACTION_RESTART);
+	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
+	report_prefix_pop();
+}
+
+static inline void get_tod_clock_ext(char *clk)
+{
+	typedef struct { char _[16]; } addrtype;
+
+	asm volatile("stcke %0" : "=Q" (*(addrtype *) clk) : : "cc");
+}
+
+static inline unsigned long long get_tod_clock(void)
+{
+	char clk[16];
+
+	get_tod_clock_ext(clk);
+	return *((unsigned long long *)&clk[1]);
+}
+
+static void test_bite(void)
+{
+	unsigned long time;
+	uint64_t mask;
+
+	/* If watchdog doesn't bite, the cpu timer does */
+	time = get_tod_clock();
+	time += (uint64_t)(16000 * 1000) << 12;
+	asm volatile("sckc %0" : : "Q" (time));
+	ctl_set_bit(0, 11);
+	mask = extract_psw_mask();
+	mask |= PSW_MASK_EXT;
+	load_psw_mask(mask);
+
+	/* Arm watchdog */
+	lc->restart_new_psw.mask = extract_psw_mask() & ~PSW_MASK_EXT;
+	diag288(CODE_INIT, 15, ACTION_RESTART);
+	asm volatile("		larl	%r0, 1f\n"
+		     "		stg	%r0, 424\n"
+		     "0:	nop\n"
+		     "		j	0b\n"
+		     "1:");
+	report("restart", true);
+	return;
+}
+
+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 db58bad..9dd288a 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -64,3 +64,7 @@ file = iep.elf
 
 [cpumodel]
 file = cpumodel.elf
+
+[diag288]
+file = diag288.elf
+extra_params=-device diag288,id=watchdog0 --watchdog-action inject-nmi
-- 
2.17.0

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

[Thomas Huth]

On 8/21/19 12:47 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>
> ---
>  lib/s390x/asm/arch_def.h |   1 +
>  s390x/Makefile           |   1 +
>  s390x/diag288.c          | 131 +++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg      |   4 ++
>  4 files changed, 137 insertions(+)
>  create mode 100644 s390x/diag288.c
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index d2cd727..4bbb428 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -15,6 +15,7 @@ struct psw {
>  	uint64_t	addr;
>  };
>  
> +#define PSW_MASK_EXT			0x0100000000000000UL
>  #define PSW_MASK_DAT			0x0400000000000000UL
>  #define PSW_MASK_PSTATE			0x0001000000000000UL
>  
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 574a9a2..3453373 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)/cpumodel.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..c129b6a
> --- /dev/null
> +++ b/s390x/diag288.c
[...]
> +static void test_specs(void)
> +{
> +	report_prefix_push("specification");
> +
> +	report_prefix_push("uneven");
> +	expect_pgm_int();
> +	asm volatile("diag %r1,%r2,0x288");

Don't you have to use "%%" in that case? ... well, if it also works
without, I don't mind, but in case you respin better play safe:

    asm volatile("diag %%r1,%%r2,0x288");

> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("unsupported action");
> +	expect_pgm_int();
> +	diag288(CODE_INIT, 15, 42);
> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("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(CODE_INIT, 15, ACTION_RESTART);
> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
> +	report_prefix_pop();
> +}
> +
> +static inline void get_tod_clock_ext(char *clk)
> +{
> +	typedef struct { char _[16]; } addrtype;
> +
> +	asm volatile("stcke %0" : "=Q" (*(addrtype *) clk) : : "cc");
> +}
> +
> +static inline unsigned long long get_tod_clock(void)
> +{
> +	char clk[16];
> +
> +	get_tod_clock_ext(clk);
> +	return *((unsigned long long *)&clk[1]);

You could use uint64_t instead of "unsigned long long".

> +}
> +
> +static void test_bite(void)
> +{
> +	unsigned long time;
> +	uint64_t mask;
> +
> +	/* If watchdog doesn't bite, the cpu timer does */
> +	time = get_tod_clock();
> +	time += (uint64_t)(16000 * 1000) << 12;
> +	asm volatile("sckc %0" : : "Q" (time));
> +	ctl_set_bit(0, 11);
> +	mask = extract_psw_mask();
> +	mask |= PSW_MASK_EXT;
> +	load_psw_mask(mask);
> +
> +	/* Arm watchdog */
> +	lc->restart_new_psw.mask = extract_psw_mask() & ~PSW_MASK_EXT;
> +	diag288(CODE_INIT, 15, ACTION_RESTART);
> +	asm volatile("		larl	%r0, 1f\n"
> +		     "		stg	%r0, 424\n"
> +		     "0:	nop\n"
> +		     "		j	0b\n"
> +		     "1:");
> +	report("restart", true);
> +	return;

Superfluous return statement.

> +}
> +
> +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 db58bad..9dd288a 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -64,3 +64,7 @@ file = iep.elf
>  
>  [cpumodel]
>  file = cpumodel.elf
> +
> +[diag288]
> +file = diag288.elf
> +extra_params=-device diag288,id=watchdog0 --watchdog-action inject-nmi
> 

Apart from the cosmetic nits, looks fine to me.

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

[kvm-unit-tests PATCH v2 3/4] s390x: Move stsi to library

[Janosch Frank]

It's needed in multiple tests now.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/asm/arch_def.h | 16 ++++++++++++++++
 s390x/skey.c             | 18 ------------------
 2 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index 4bbb428..5f8f45e 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -240,4 +240,20 @@ static inline void enter_pstate(void)
 	load_psw_mask(mask);
 }
 
+static inline int stsi(void *addr, int fc, int sel1, int sel2)
+{
+	register int r0 asm("0") = (fc << 28) | sel1;
+	register int 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;
+}
+
 #endif
diff --git a/s390x/skey.c b/s390x/skey.c
index b1e11af..fd4fcc7 100644
--- a/s390x/skey.c
+++ b/s390x/skey.c
@@ -70,24 +70,6 @@ static void test_set(void)
 	       skey.str.acc == ret.str.acc && skey.str.fp == ret.str.fp);
 }
 
-static inline int stsi(void *addr, int fc, int sel1, int sel2)
-{
-	register int r0 asm("0") = (fc << 28) | sel1;
-	register int r1 asm("1") = sel2;
-	int rc = 0;
-
-	asm volatile(
-		"	stsi	0(%3)\n"
-		"	jz	0f\n"
-		"	lhi	%1,-1\n"
-		"0:\n"
-		: "+d" (r0), "+d" (rc)
-		: "d" (r1), "a" (addr)
-		: "cc", "memory");
-
-	return rc;
-}
-
 /* Returns true if we are running under z/VM 6.x */
 static bool check_for_zvm6(void)
 {
-- 
2.17.0

Re: [kvm-unit-tests PATCH v2 3/4] s390x: Move stsi to library

[David Hildenbrand]

On 21.08.19 12:47, Janosch Frank wrote:
> It's needed in multiple tests now.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/asm/arch_def.h | 16 ++++++++++++++++
>  s390x/skey.c             | 18 ------------------
>  2 files changed, 16 insertions(+), 18 deletions(-)
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 4bbb428..5f8f45e 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -240,4 +240,20 @@ static inline void enter_pstate(void)
>  	load_psw_mask(mask);
>  }
>  
> +static inline int stsi(void *addr, int fc, int sel1, int sel2)
> +{
> +	register int r0 asm("0") = (fc << 28) | sel1;
> +	register int 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;
> +}
> +
>  #endif
> diff --git a/s390x/skey.c b/s390x/skey.c
> index b1e11af..fd4fcc7 100644
> --- a/s390x/skey.c
> +++ b/s390x/skey.c
> @@ -70,24 +70,6 @@ static void test_set(void)
>  	       skey.str.acc == ret.str.acc && skey.str.fp == ret.str.fp);
>  }
>  
> -static inline int stsi(void *addr, int fc, int sel1, int sel2)
> -{
> -	register int r0 asm("0") = (fc << 28) | sel1;
> -	register int r1 asm("1") = sel2;
> -	int rc = 0;
> -
> -	asm volatile(
> -		"	stsi	0(%3)\n"
> -		"	jz	0f\n"
> -		"	lhi	%1,-1\n"
> -		"0:\n"
> -		: "+d" (r0), "+d" (rc)
> -		: "d" (r1), "a" (addr)
> -		: "cc", "memory");
> -
> -	return rc;
> -}
> -
>  /* Returns true if we are running under z/VM 6.x */
>  static bool check_for_zvm6(void)
>  {
> 

You don't simply move, you also modify and change the return value from
o/-1 to cc. AFAIKs, this should be fine.

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH v2 3/4] s390x: Move stsi to library

[Thomas Huth]

On 8/21/19 12:47 PM, Janosch Frank wrote:
> It's needed in multiple tests now.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/asm/arch_def.h | 16 ++++++++++++++++
>  s390x/skey.c             | 18 ------------------
>  2 files changed, 16 insertions(+), 18 deletions(-)
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 4bbb428..5f8f45e 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -240,4 +240,20 @@ static inline void enter_pstate(void)
>  	load_psw_mask(mask);
>  }
>  
> +static inline int stsi(void *addr, int fc, int sel1, int sel2)
> +{
> +	register int r0 asm("0") = (fc << 28) | sel1;
> +	register int 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;

Maybe mention the changed return value in the patch description.

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

[kvm-unit-tests PATCH v2 4/4] 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        | 84 +++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |  3 ++
 3 files changed, 88 insertions(+)
 create mode 100644 s390x/stsi.c

diff --git a/s390x/Makefile b/s390x/Makefile
index 3453373..76db0bb 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -13,6 +13,7 @@ tests += $(TEST_DIR)/gs.elf
 tests += $(TEST_DIR)/iep.elf
 tests += $(TEST_DIR)/cpumodel.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..0f90c9a
--- /dev/null
+++ b/s390x/stsi.c
@@ -0,0 +1,84 @@
+/*
+ * 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 void test_specs(void)
+{
+	report_prefix_push("specification");
+
+	report_prefix_push("inv r0");
+	expect_pgm_int();
+	stsi(pagebuf, 0, 1 << 8, 0);
+	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+	report_prefix_pop();
+
+	report_prefix_push("inv r1");
+	expect_pgm_int();
+	stsi(pagebuf, 1, 0, 1 << 16);
+	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+	report_prefix_pop();
+
+	report_prefix_push("unaligned");
+	expect_pgm_int();
+	stsi(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(pagebuf, 0, 0, 0);
+	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
+	report_prefix_pop();
+}
+
+static inline unsigned long stsi_get_fc(void *addr)
+{
+	register unsigned long r0 asm("0") = 0;
+	register unsigned long r1 asm("1") = 0;
+	int cc;
+
+	asm volatile("stsi	0(%2)\n"
+		     "ipm	%[cc]\n"
+		     "srl	%[cc],28\n"
+		     : "+d" (r0), [cc] "=d" (cc)
+		     : "d" (r1), "a" (addr)
+		     : "cc", "memory");
+	assert(!cc);
+	return r0 >> 28;
+}
+
+static void test_fc(void)
+{
+	report("cc == 3",  stsi(pagebuf, 7, 0, 0) == 3);
+	report("r0 == 3",  stsi_get_fc(pagebuf) >= 2);
+}
+
+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 9dd288a..cc79a4e 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -68,3 +68,6 @@ file = cpumodel.elf
 [diag288]
 file = diag288.elf
 extra_params=-device diag288,id=watchdog0 --watchdog-action inject-nmi
+
+[stsi]
+file = stsi.elf
-- 
2.17.0

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

[Thomas Huth]

On 8/21/19 12:47 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        | 84 +++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg |  3 ++
>  3 files changed, 88 insertions(+)
>  create mode 100644 s390x/stsi.c
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 3453373..76db0bb 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -13,6 +13,7 @@ tests += $(TEST_DIR)/gs.elf
>  tests += $(TEST_DIR)/iep.elf
>  tests += $(TEST_DIR)/cpumodel.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..0f90c9a
> --- /dev/null
> +++ b/s390x/stsi.c
> @@ -0,0 +1,84 @@
[...]
> +static inline unsigned long stsi_get_fc(void *addr)
> +{
> +	register unsigned long r0 asm("0") = 0;
> +	register unsigned long r1 asm("1") = 0;
> +	int cc;
> +
> +	asm volatile("stsi	0(%2)\n"

Shouldn't that be %3 instead?

> +		     "ipm	%[cc]\n"
> +		     "srl	%[cc],28\n"
> +		     : "+d" (r0), [cc] "=d" (cc)
> +		     : "d" (r1), "a" (addr)
> +		     : "cc", "memory");
> +	assert(!cc);
> +	return r0 >> 28;
> +}
> +
> +static void test_fc(void)
> +{
> +	report("cc == 3",  stsi(pagebuf, 7, 0, 0) == 3);
> +	report("r0 == 3",  stsi_get_fc(pagebuf) >= 2);

I'd like to suggest to change the string to "r0 >= 2", too.

 Thomas

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

[Janosch Frank]

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

On 8/23/19 12:57 PM, Thomas Huth wrote:
> On 8/21/19 12:47 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        | 84 +++++++++++++++++++++++++++++++++++++++++++++
>>  s390x/unittests.cfg |  3 ++
>>  3 files changed, 88 insertions(+)
>>  create mode 100644 s390x/stsi.c
>>
>> diff --git a/s390x/Makefile b/s390x/Makefile
>> index 3453373..76db0bb 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -13,6 +13,7 @@ tests += $(TEST_DIR)/gs.elf
>>  tests += $(TEST_DIR)/iep.elf
>>  tests += $(TEST_DIR)/cpumodel.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..0f90c9a
>> --- /dev/null
>> +++ b/s390x/stsi.c
>> @@ -0,0 +1,84 @@
> [...]
>> +static inline unsigned long stsi_get_fc(void *addr)
>> +{
>> +	register unsigned long r0 asm("0") = 0;
>> +	register unsigned long r1 asm("1") = 0;
>> +	int cc;
>> +
>> +	asm volatile("stsi	0(%2)\n"
> 
> Shouldn't that be %3 instead?

Yup, although it doesn't really matter, as the address is ignored.

> 
>> +		     "ipm	%[cc]\n"
>> +		     "srl	%[cc],28\n"
>> +		     : "+d" (r0), [cc] "=d" (cc)
>> +		     : "d" (r1), "a" (addr)
>> +		     : "cc", "memory");
>> +	assert(!cc);
>> +	return r0 >> 28;
>> +}
>> +
>> +static void test_fc(void)
>> +{
>> +	report("cc == 3",  stsi(pagebuf, 7, 0, 0) == 3);
>> +	report("r0 == 3",  stsi_get_fc(pagebuf) >= 2);

Just changed it to:
"invalid fc"
"query fc >= 2

> 
> I'd like to suggest to change the string to "r0 >= 2", too.
> 
>  Thomas
> 



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

[kvm-unit-tests PATCH] s390x: Add diag308 subcode 0 testing

[Janosch Frank]

By adding a load reset routine to cstart.S we can also test the clear
reset done by subcode 0, as we now can restore our registers again.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
I managed to extract this from another bigger test, so let's add it to the bunch.
I'd be very happy about assembly review :-)
---
 s390x/cstart64.S | 27 +++++++++++++++++++++++++++
 s390x/diag308.c  | 31 ++++++++++---------------------
 2 files changed, 37 insertions(+), 21 deletions(-)

diff --git a/s390x/cstart64.S b/s390x/cstart64.S
index dedfe80..47045e1 100644
--- a/s390x/cstart64.S
+++ b/s390x/cstart64.S
@@ -145,6 +145,33 @@ memsetxc:
 	.endm
 
 .section .text
+/*
+ * load_reset calling convention:
+ * %r2 subcode (0 or 1)
+ */
+.globl load_reset
+load_reset:
+	SAVE_REGS
+	/* Save the first PSW word to the IPL PSW */
+	epsw	%r0, %r1
+	st	%r0, 0
+	/* Store the address and the bit for 31 bit addressing */
+	larl    %r0, 0f
+	oilh    %r0, 0x8000
+	st      %r0, 0x4
+	/* Do the reset */
+	diag    %r0,%r2,0x308
+	/* Failure path */
+	xgr	%r2, %r2
+	br	%r14
+	/* Success path */
+	/* We lost cr0 due to the reset */
+0:	larl	%r1, initial_cr0
+	lctlg	%c0, %c0, 0(%r1)
+	RESTORE_REGS
+	lhi	%r2, 1
+	br	%r14
+
 pgm_int:
 	SAVE_REGS
 	brasl	%r14, handle_pgm_int
diff --git a/s390x/diag308.c b/s390x/diag308.c
index f085b1a..baf9fd3 100644
--- a/s390x/diag308.c
+++ b/s390x/diag308.c
@@ -21,32 +21,20 @@ static void test_priv(void)
 	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
 }
 
+
 /*
- * Check that diag308 with subcode 1 loads the PSW at address 0, i.e.
+ * Check that diag308 with subcode 0 and 1 loads the PSW at address 0, i.e.
  * that we can put a pointer into address 4 which then gets executed.
  */
+extern int load_reset(u64);
+static void test_subcode0(void)
+{
+	report("load modified clear done", load_reset(0));
+}
+
 static void test_subcode1(void)
 {
-	uint64_t saved_psw = *(uint64_t *)0;
-	long subcode = 1;
-	long ret, tmp;
-
-	asm volatile (
-		"	epsw	%0,%1\n"
-		"	st	%0,0\n"
-		"	larl	%0,0f\n"
-		"	oilh	%0,0x8000\n"
-		"	st	%0,4\n"
-		"	diag	0,%2,0x308\n"
-		"	lghi	%0,0\n"
-		"	j	1f\n"
-		"0:	lghi	%0,1\n"
-		"1:"
-		: "=&d"(ret), "=&d"(tmp) : "d"(subcode) : "memory");
-
-	*(uint64_t *)0 = saved_psw;
-
-	report("load normal reset done", ret == 1);
+	report("load normal reset done", load_reset(1));
 }
 
 /* Expect a specification exception when using an uneven register */
@@ -107,6 +95,7 @@ static struct {
 	void (*func)(void);
 } tests[] = {
 	{ "privileged", test_priv },
+	{ "subcode 0", test_subcode0 },
 	{ "subcode 1", test_subcode1 },
 	{ "subcode 5", test_subcode5 },
 	{ "subcode 6", test_subcode6 },
-- 
2.17.0

Re: [kvm-unit-tests PATCH] s390x: Add diag308 subcode 0 testing

[David Hildenbrand]

On 22.08.19 13:11, Janosch Frank wrote:
> By adding a load reset routine to cstart.S we can also test the clear
> reset done by subcode 0, as we now can restore our registers again.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> I managed to extract this from another bigger test, so let's add it to the bunch.
> I'd be very happy about assembly review :-)
> ---
>  s390x/cstart64.S | 27 +++++++++++++++++++++++++++
>  s390x/diag308.c  | 31 ++++++++++---------------------
>  2 files changed, 37 insertions(+), 21 deletions(-)
> 
> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
> index dedfe80..47045e1 100644
> --- a/s390x/cstart64.S
> +++ b/s390x/cstart64.S
> @@ -145,6 +145,33 @@ memsetxc:
>  	.endm
>  
>  .section .text
> +/*
> + * load_reset calling convention:
> + * %r2 subcode (0 or 1)
> + */
> +.globl load_reset
> +load_reset:
> +	SAVE_REGS
> +	/* Save the first PSW word to the IPL PSW */
> +	epsw	%r0, %r1
> +	st	%r0, 0
> +	/* Store the address and the bit for 31 bit addressing */
> +	larl    %r0, 0f
> +	oilh    %r0, 0x8000
> +	st      %r0, 0x4
> +	/* Do the reset */
> +	diag    %r0,%r2,0x308
> +	/* Failure path */
> +	xgr	%r2, %r2
> +	br	%r14
> +	/* Success path */
> +	/* We lost cr0 due to the reset */
> +0:	larl	%r1, initial_cr0
> +	lctlg	%c0, %c0, 0(%r1)
> +	RESTORE_REGS
> +	lhi	%r2, 1
> +	br	%r14
> +
>  pgm_int:
>  	SAVE_REGS
>  	brasl	%r14, handle_pgm_int
> diff --git a/s390x/diag308.c b/s390x/diag308.c
> index f085b1a..baf9fd3 100644
> --- a/s390x/diag308.c
> +++ b/s390x/diag308.c
> @@ -21,32 +21,20 @@ static void test_priv(void)
>  	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
>  }
>  
> +
>  /*
> - * Check that diag308 with subcode 1 loads the PSW at address 0, i.e.
> + * Check that diag308 with subcode 0 and 1 loads the PSW at address 0, i.e.
>   * that we can put a pointer into address 4 which then gets executed.
>   */
> +extern int load_reset(u64);
> +static void test_subcode0(void)
> +{
> +	report("load modified clear done", load_reset(0));
> +}
> +
>  static void test_subcode1(void)
>  {
> -	uint64_t saved_psw = *(uint64_t *)0;
> -	long subcode = 1;
> -	long ret, tmp;
> -
> -	asm volatile (
> -		"	epsw	%0,%1\n"
> -		"	st	%0,0\n"
> -		"	larl	%0,0f\n"
> -		"	oilh	%0,0x8000\n"
> -		"	st	%0,4\n"
> -		"	diag	0,%2,0x308\n"
> -		"	lghi	%0,0\n"
> -		"	j	1f\n"
> -		"0:	lghi	%0,1\n"
> -		"1:"
> -		: "=&d"(ret), "=&d"(tmp) : "d"(subcode) : "memory");
> -
> -	*(uint64_t *)0 = saved_psw;
> -
> -	report("load normal reset done", ret == 1);
> +	report("load normal reset done", load_reset(1));
>  }
>  
>  /* Expect a specification exception when using an uneven register */
> @@ -107,6 +95,7 @@ static struct {
>  	void (*func)(void);
>  } tests[] = {
>  	{ "privileged", test_priv },
> +	{ "subcode 0", test_subcode0 },
>  	{ "subcode 1", test_subcode1 },
>  	{ "subcode 5", test_subcode5 },
>  	{ "subcode 6", test_subcode6 },
> 

So, in general I am wondering if we should restore the original IPL_PSW
after we used it - is there any chance we might require the old value
again (I guess we're fine with cpu resets)?

-- 

Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH] s390x: Add diag308 subcode 0 testing

[Janosch Frank]

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

On 8/23/19 1:00 PM, David Hildenbrand wrote:
> On 22.08.19 13:11, Janosch Frank wrote:
>> By adding a load reset routine to cstart.S we can also test the clear
>> reset done by subcode 0, as we now can restore our registers again.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>> I managed to extract this from another bigger test, so let's add it to the bunch.
>> I'd be very happy about assembly review :-)
>> ---
>>  s390x/cstart64.S | 27 +++++++++++++++++++++++++++
>>  s390x/diag308.c  | 31 ++++++++++---------------------
>>  2 files changed, 37 insertions(+), 21 deletions(-)
>>
>> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
>> index dedfe80..47045e1 100644
>> --- a/s390x/cstart64.S
>> +++ b/s390x/cstart64.S
>> @@ -145,6 +145,33 @@ memsetxc:
>>  	.endm
>>  
>>  .section .text
>> +/*
>> + * load_reset calling convention:
>> + * %r2 subcode (0 or 1)
>> + */
>> +.globl load_reset
>> +load_reset:
>> +	SAVE_REGS
>> +	/* Save the first PSW word to the IPL PSW */
>> +	epsw	%r0, %r1
>> +	st	%r0, 0
>> +	/* Store the address and the bit for 31 bit addressing */
>> +	larl    %r0, 0f
>> +	oilh    %r0, 0x8000
>> +	st      %r0, 0x4
>> +	/* Do the reset */
>> +	diag    %r0,%r2,0x308
>> +	/* Failure path */
>> +	xgr	%r2, %r2
>> +	br	%r14
>> +	/* Success path */
>> +	/* We lost cr0 due to the reset */
>> +0:	larl	%r1, initial_cr0
>> +	lctlg	%c0, %c0, 0(%r1)
>> +	RESTORE_REGS
>> +	lhi	%r2, 1
>> +	br	%r14
>> +
>>  pgm_int:
>>  	SAVE_REGS
>>  	brasl	%r14, handle_pgm_int
>> diff --git a/s390x/diag308.c b/s390x/diag308.c
>> index f085b1a..baf9fd3 100644
>> --- a/s390x/diag308.c
>> +++ b/s390x/diag308.c
>> @@ -21,32 +21,20 @@ static void test_priv(void)
>>  	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
>>  }
>>  
>> +
>>  /*
>> - * Check that diag308 with subcode 1 loads the PSW at address 0, i.e.
>> + * Check that diag308 with subcode 0 and 1 loads the PSW at address 0, i.e.
>>   * that we can put a pointer into address 4 which then gets executed.
>>   */
>> +extern int load_reset(u64);
>> +static void test_subcode0(void)
>> +{
>> +	report("load modified clear done", load_reset(0));
>> +}
>> +
>>  static void test_subcode1(void)
>>  {
>> -	uint64_t saved_psw = *(uint64_t *)0;
>> -	long subcode = 1;
>> -	long ret, tmp;
>> -
>> -	asm volatile (
>> -		"	epsw	%0,%1\n"
>> -		"	st	%0,0\n"
>> -		"	larl	%0,0f\n"
>> -		"	oilh	%0,0x8000\n"
>> -		"	st	%0,4\n"
>> -		"	diag	0,%2,0x308\n"
>> -		"	lghi	%0,0\n"
>> -		"	j	1f\n"
>> -		"0:	lghi	%0,1\n"
>> -		"1:"
>> -		: "=&d"(ret), "=&d"(tmp) : "d"(subcode) : "memory");
>> -
>> -	*(uint64_t *)0 = saved_psw;
>> -
>> -	report("load normal reset done", ret == 1);
>> +	report("load normal reset done", load_reset(1));
>>  }
>>  
>>  /* Expect a specification exception when using an uneven register */
>> @@ -107,6 +95,7 @@ static struct {
>>  	void (*func)(void);
>>  } tests[] = {
>>  	{ "privileged", test_priv },
>> +	{ "subcode 0", test_subcode0 },
>>  	{ "subcode 1", test_subcode1 },
>>  	{ "subcode 5", test_subcode5 },
>>  	{ "subcode 6", test_subcode6 },
>>
> 
> So, in general I am wondering if we should restore the original IPL_PSW
> after we used it - is there any chance we might require the old value
> again (I guess we're fine with cpu resets)?

I currently don't see a need, but we could cache it in the restart old
psw address. Or we just store back the two word constant.



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

Re: [kvm-unit-tests PATCH] s390x: Add diag308 subcode 0 testing

[David Hildenbrand]

On 23.08.19 13:33, Janosch Frank wrote:
> On 8/23/19 1:00 PM, David Hildenbrand wrote:
>> On 22.08.19 13:11, Janosch Frank wrote:
>>> By adding a load reset routine to cstart.S we can also test the clear
>>> reset done by subcode 0, as we now can restore our registers again.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>> I managed to extract this from another bigger test, so let's add it to the bunch.
>>> I'd be very happy about assembly review :-)
>>> ---
>>>  s390x/cstart64.S | 27 +++++++++++++++++++++++++++
>>>  s390x/diag308.c  | 31 ++++++++++---------------------
>>>  2 files changed, 37 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
>>> index dedfe80..47045e1 100644
>>> --- a/s390x/cstart64.S
>>> +++ b/s390x/cstart64.S
>>> @@ -145,6 +145,33 @@ memsetxc:
>>>  	.endm
>>>  
>>>  .section .text
>>> +/*
>>> + * load_reset calling convention:
>>> + * %r2 subcode (0 or 1)
>>> + */
>>> +.globl load_reset
>>> +load_reset:
>>> +	SAVE_REGS
>>> +	/* Save the first PSW word to the IPL PSW */
>>> +	epsw	%r0, %r1
>>> +	st	%r0, 0
>>> +	/* Store the address and the bit for 31 bit addressing */
>>> +	larl    %r0, 0f
>>> +	oilh    %r0, 0x8000
>>> +	st      %r0, 0x4
>>> +	/* Do the reset */
>>> +	diag    %r0,%r2,0x308
>>> +	/* Failure path */
>>> +	xgr	%r2, %r2
>>> +	br	%r14
>>> +	/* Success path */
>>> +	/* We lost cr0 due to the reset */
>>> +0:	larl	%r1, initial_cr0
>>> +	lctlg	%c0, %c0, 0(%r1)
>>> +	RESTORE_REGS
>>> +	lhi	%r2, 1
>>> +	br	%r14
>>> +
>>>  pgm_int:
>>>  	SAVE_REGS
>>>  	brasl	%r14, handle_pgm_int
>>> diff --git a/s390x/diag308.c b/s390x/diag308.c
>>> index f085b1a..baf9fd3 100644
>>> --- a/s390x/diag308.c
>>> +++ b/s390x/diag308.c
>>> @@ -21,32 +21,20 @@ static void test_priv(void)
>>>  	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
>>>  }
>>>  
>>> +
>>>  /*
>>> - * Check that diag308 with subcode 1 loads the PSW at address 0, i.e.
>>> + * Check that diag308 with subcode 0 and 1 loads the PSW at address 0, i.e.
>>>   * that we can put a pointer into address 4 which then gets executed.
>>>   */
>>> +extern int load_reset(u64);
>>> +static void test_subcode0(void)
>>> +{
>>> +	report("load modified clear done", load_reset(0));
>>> +}
>>> +
>>>  static void test_subcode1(void)
>>>  {
>>> -	uint64_t saved_psw = *(uint64_t *)0;
>>> -	long subcode = 1;
>>> -	long ret, tmp;
>>> -
>>> -	asm volatile (
>>> -		"	epsw	%0,%1\n"
>>> -		"	st	%0,0\n"
>>> -		"	larl	%0,0f\n"
>>> -		"	oilh	%0,0x8000\n"
>>> -		"	st	%0,4\n"
>>> -		"	diag	0,%2,0x308\n"
>>> -		"	lghi	%0,0\n"
>>> -		"	j	1f\n"
>>> -		"0:	lghi	%0,1\n"
>>> -		"1:"
>>> -		: "=&d"(ret), "=&d"(tmp) : "d"(subcode) : "memory");
>>> -
>>> -	*(uint64_t *)0 = saved_psw;
>>> -
>>> -	report("load normal reset done", ret == 1);
>>> +	report("load normal reset done", load_reset(1));
>>>  }
>>>  
>>>  /* Expect a specification exception when using an uneven register */
>>> @@ -107,6 +95,7 @@ static struct {
>>>  	void (*func)(void);
>>>  } tests[] = {
>>>  	{ "privileged", test_priv },
>>> +	{ "subcode 0", test_subcode0 },
>>>  	{ "subcode 1", test_subcode1 },
>>>  	{ "subcode 5", test_subcode5 },
>>>  	{ "subcode 6", test_subcode6 },
>>>
>>
>> So, in general I am wondering if we should restore the original IPL_PSW
>> after we used it - is there any chance we might require the old value
>> again (I guess we're fine with cpu resets)?
> 
> I currently don't see a need, but we could cache it in the restart old
> psw address. Or we just store back the two word constant.
> 

If there's no need right no, I guess we can skip that. Was just wondering.


-- 

Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH] s390x: Add diag308 subcode 0 testing

[Thomas Huth]

On 8/22/19 1:11 PM, Janosch Frank wrote:
> By adding a load reset routine to cstart.S we can also test the clear
> reset done by subcode 0, as we now can restore our registers again.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> I managed to extract this from another bigger test, so let's add it to the bunch.
> I'd be very happy about assembly review :-)

FWIW, the assembly code looks fine to me.

> ---
>  s390x/cstart64.S | 27 +++++++++++++++++++++++++++
>  s390x/diag308.c  | 31 ++++++++++---------------------
>  2 files changed, 37 insertions(+), 21 deletions(-)
> 
> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
> index dedfe80..47045e1 100644
> --- a/s390x/cstart64.S
> +++ b/s390x/cstart64.S
> @@ -145,6 +145,33 @@ memsetxc:
>  	.endm
>  
>  .section .text
> +/*
> + * load_reset calling convention:
> + * %r2 subcode (0 or 1)
> + */
> +.globl load_reset
> +load_reset:

Maybe rather name the function diag308_load_reset so that it is clear
that it belongs to the diag308 test?
Or are you going to re-use this function in other tests later?

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

Re: [kvm-unit-tests PATCH] s390x: Add diag308 subcode 0 testing

[Janosch Frank]

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

On 8/23/19 4:12 PM, Thomas Huth wrote:
> On 8/22/19 1:11 PM, Janosch Frank wrote:
>> By adding a load reset routine to cstart.S we can also test the clear
>> reset done by subcode 0, as we now can restore our registers again.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>> I managed to extract this from another bigger test, so let's add it to the bunch.
>> I'd be very happy about assembly review :-)
> 
> FWIW, the assembly code looks fine to me.
> 
>> ---
>>  s390x/cstart64.S | 27 +++++++++++++++++++++++++++
>>  s390x/diag308.c  | 31 ++++++++++---------------------
>>  2 files changed, 37 insertions(+), 21 deletions(-)
>>
>> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
>> index dedfe80..47045e1 100644
>> --- a/s390x/cstart64.S
>> +++ b/s390x/cstart64.S
>> @@ -145,6 +145,33 @@ memsetxc:
>>  	.endm
>>  
>>  .section .text
>> +/*
>> + * load_reset calling convention:
>> + * %r2 subcode (0 or 1)
>> + */
>> +.globl load_reset
>> +load_reset:
> 
> Maybe rather name the function diag308_load_reset so that it is clear
> that it belongs to the diag308 test?

Sure

> Or are you going to re-use this function in other tests later?

I currently have no such plans
But I'm thinking about a way to check the CPU registers in combination
with smp. So it might be extended.

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

Thanks!



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