* [kvm-unit-tests PATCH v2 1/4] s390x: Support PSW restart boot
2019-08-21 10:47 [kvm-unit-tests PATCH v2 0/4] s390x: More emulation tests Janosch Frank
@ 2019-08-21 10:47 ` Janosch Frank
2019-08-21 13:23 ` David Hildenbrand
2019-08-23 10:31 ` David Hildenbrand
2019-08-21 10:47 ` [kvm-unit-tests PATCH v2 2/4] s390x: Diag288 test Janosch Frank
` (3 subsequent siblings)
4 siblings, 2 replies; 18+ messages in thread
From: Janosch Frank @ 2019-08-21 10:47 UTC (permalink / raw)
To: kvm; +Cc: linux-s390, david, thuth
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [kvm-unit-tests PATCH v2 1/4] s390x: Support PSW restart boot
2019-08-21 10:47 ` [kvm-unit-tests PATCH v2 1/4] s390x: Support PSW restart boot Janosch Frank
@ 2019-08-21 13:23 ` David Hildenbrand
2019-08-23 10:31 ` David Hildenbrand
1 sibling, 0 replies; 18+ messages in thread
From: David Hildenbrand @ 2019-08-21 13:23 UTC (permalink / raw)
To: Janosch Frank, kvm; +Cc: linux-s390, thuth
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [kvm-unit-tests PATCH v2 1/4] s390x: Support PSW restart boot
2019-08-21 10:47 ` [kvm-unit-tests PATCH v2 1/4] s390x: Support PSW restart boot Janosch Frank
2019-08-21 13:23 ` David Hildenbrand
@ 2019-08-23 10:31 ` David Hildenbrand
1 sibling, 0 replies; 18+ messages in thread
From: David Hildenbrand @ 2019-08-23 10:31 UTC (permalink / raw)
To: Janosch Frank, kvm; +Cc: linux-s390, thuth
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [kvm-unit-tests PATCH v2 2/4] s390x: Diag288 test
2019-08-21 10:47 [kvm-unit-tests PATCH v2 0/4] s390x: More emulation tests Janosch Frank
2019-08-21 10:47 ` [kvm-unit-tests PATCH v2 1/4] s390x: Support PSW restart boot Janosch Frank
@ 2019-08-21 10:47 ` Janosch Frank
2019-08-23 10:30 ` Thomas Huth
2019-08-21 10:47 ` [kvm-unit-tests PATCH v2 3/4] s390x: Move stsi to library Janosch Frank
` (2 subsequent siblings)
4 siblings, 1 reply; 18+ messages in thread
From: Janosch Frank @ 2019-08-21 10:47 UTC (permalink / raw)
To: kvm; +Cc: linux-s390, david, thuth
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [kvm-unit-tests PATCH v2 2/4] s390x: Diag288 test
2019-08-21 10:47 ` [kvm-unit-tests PATCH v2 2/4] s390x: Diag288 test Janosch Frank
@ 2019-08-23 10:30 ` Thomas Huth
0 siblings, 0 replies; 18+ messages in thread
From: Thomas Huth @ 2019-08-23 10:30 UTC (permalink / raw)
To: Janosch Frank, kvm; +Cc: linux-s390, david
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>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [kvm-unit-tests PATCH v2 3/4] s390x: Move stsi to library
2019-08-21 10:47 [kvm-unit-tests PATCH v2 0/4] s390x: More emulation tests Janosch Frank
2019-08-21 10:47 ` [kvm-unit-tests PATCH v2 1/4] s390x: Support PSW restart boot Janosch Frank
2019-08-21 10:47 ` [kvm-unit-tests PATCH v2 2/4] s390x: Diag288 test Janosch Frank
@ 2019-08-21 10:47 ` Janosch Frank
2019-08-21 13:27 ` David Hildenbrand
2019-08-23 10:34 ` Thomas Huth
2019-08-21 10:47 ` [kvm-unit-tests PATCH v2 4/4] s390x: STSI tests Janosch Frank
2019-08-22 11:11 ` [kvm-unit-tests PATCH] s390x: Add diag308 subcode 0 testing Janosch Frank
4 siblings, 2 replies; 18+ messages in thread
From: Janosch Frank @ 2019-08-21 10:47 UTC (permalink / raw)
To: kvm; +Cc: linux-s390, david, thuth
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [kvm-unit-tests PATCH v2 3/4] s390x: Move stsi to library
2019-08-21 10:47 ` [kvm-unit-tests PATCH v2 3/4] s390x: Move stsi to library Janosch Frank
@ 2019-08-21 13:27 ` David Hildenbrand
2019-08-23 10:34 ` Thomas Huth
1 sibling, 0 replies; 18+ messages in thread
From: David Hildenbrand @ 2019-08-21 13:27 UTC (permalink / raw)
To: Janosch Frank, kvm; +Cc: linux-s390, thuth
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [kvm-unit-tests PATCH v2 3/4] s390x: Move stsi to library
2019-08-21 10:47 ` [kvm-unit-tests PATCH v2 3/4] s390x: Move stsi to library Janosch Frank
2019-08-21 13:27 ` David Hildenbrand
@ 2019-08-23 10:34 ` Thomas Huth
1 sibling, 0 replies; 18+ messages in thread
From: Thomas Huth @ 2019-08-23 10:34 UTC (permalink / raw)
To: Janosch Frank, kvm; +Cc: linux-s390, david
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>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [kvm-unit-tests PATCH v2 4/4] s390x: STSI tests
2019-08-21 10:47 [kvm-unit-tests PATCH v2 0/4] s390x: More emulation tests Janosch Frank
` (2 preceding siblings ...)
2019-08-21 10:47 ` [kvm-unit-tests PATCH v2 3/4] s390x: Move stsi to library Janosch Frank
@ 2019-08-21 10:47 ` Janosch Frank
2019-08-23 10:57 ` Thomas Huth
2019-08-22 11:11 ` [kvm-unit-tests PATCH] s390x: Add diag308 subcode 0 testing Janosch Frank
4 siblings, 1 reply; 18+ messages in thread
From: Janosch Frank @ 2019-08-21 10:47 UTC (permalink / raw)
To: kvm; +Cc: linux-s390, david, thuth
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [kvm-unit-tests PATCH v2 4/4] s390x: STSI tests
2019-08-21 10:47 ` [kvm-unit-tests PATCH v2 4/4] s390x: STSI tests Janosch Frank
@ 2019-08-23 10:57 ` Thomas Huth
2019-08-23 11:16 ` Janosch Frank
0 siblings, 1 reply; 18+ messages in thread
From: Thomas Huth @ 2019-08-23 10:57 UTC (permalink / raw)
To: Janosch Frank, kvm; +Cc: linux-s390, david
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [kvm-unit-tests PATCH v2 4/4] s390x: STSI tests
2019-08-23 10:57 ` Thomas Huth
@ 2019-08-23 11:16 ` Janosch Frank
0 siblings, 0 replies; 18+ messages in thread
From: Janosch Frank @ 2019-08-23 11:16 UTC (permalink / raw)
To: Thomas Huth, kvm; +Cc: linux-s390, david
[-- 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 --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* [kvm-unit-tests PATCH] s390x: Add diag308 subcode 0 testing
2019-08-21 10:47 [kvm-unit-tests PATCH v2 0/4] s390x: More emulation tests Janosch Frank
` (3 preceding siblings ...)
2019-08-21 10:47 ` [kvm-unit-tests PATCH v2 4/4] s390x: STSI tests Janosch Frank
@ 2019-08-22 11:11 ` Janosch Frank
2019-08-23 11:00 ` David Hildenbrand
2019-08-23 14:12 ` Thomas Huth
4 siblings, 2 replies; 18+ messages in thread
From: Janosch Frank @ 2019-08-22 11:11 UTC (permalink / raw)
To: kvm; +Cc: linux-s390, david, thuth
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [kvm-unit-tests PATCH] s390x: Add diag308 subcode 0 testing
2019-08-22 11:11 ` [kvm-unit-tests PATCH] s390x: Add diag308 subcode 0 testing Janosch Frank
@ 2019-08-23 11:00 ` David Hildenbrand
2019-08-23 11:33 ` Janosch Frank
2019-08-23 14:12 ` Thomas Huth
1 sibling, 1 reply; 18+ messages in thread
From: David Hildenbrand @ 2019-08-23 11:00 UTC (permalink / raw)
To: Janosch Frank, kvm; +Cc: linux-s390, thuth
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [kvm-unit-tests PATCH] s390x: Add diag308 subcode 0 testing
2019-08-23 11:00 ` David Hildenbrand
@ 2019-08-23 11:33 ` Janosch Frank
2019-08-23 11:36 ` David Hildenbrand
0 siblings, 1 reply; 18+ messages in thread
From: Janosch Frank @ 2019-08-23 11:33 UTC (permalink / raw)
To: David Hildenbrand, kvm; +Cc: linux-s390, thuth
[-- 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 --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [kvm-unit-tests PATCH] s390x: Add diag308 subcode 0 testing
2019-08-23 11:33 ` Janosch Frank
@ 2019-08-23 11:36 ` David Hildenbrand
0 siblings, 0 replies; 18+ messages in thread
From: David Hildenbrand @ 2019-08-23 11:36 UTC (permalink / raw)
To: Janosch Frank, kvm; +Cc: linux-s390, thuth
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [kvm-unit-tests PATCH] s390x: Add diag308 subcode 0 testing
2019-08-22 11:11 ` [kvm-unit-tests PATCH] s390x: Add diag308 subcode 0 testing Janosch Frank
2019-08-23 11:00 ` David Hildenbrand
@ 2019-08-23 14:12 ` Thomas Huth
2019-08-23 14:41 ` Janosch Frank
1 sibling, 1 reply; 18+ messages in thread
From: Thomas Huth @ 2019-08-23 14:12 UTC (permalink / raw)
To: Janosch Frank, kvm; +Cc: linux-s390, david
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>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [kvm-unit-tests PATCH] s390x: Add diag308 subcode 0 testing
2019-08-23 14:12 ` Thomas Huth
@ 2019-08-23 14:41 ` Janosch Frank
0 siblings, 0 replies; 18+ messages in thread
From: Janosch Frank @ 2019-08-23 14:41 UTC (permalink / raw)
To: Thomas Huth, kvm; +Cc: linux-s390, david
[-- 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 --]
^ permalink raw reply [flat|nested] 18+ messages in thread