[kvm-unit-tests PATCH 0/3] PV tests part 1

[kvm-unit-tests PATCH 0/3] PV tests part 1

[Janosch Frank]

Let's start bringing in some more PV related code.

Somehow I missed that we can also have a key in a exception new
PSW. The interesting bit when skrf is active is that if such a PSW is
loaded on an exception it will result in a specification exception and
not a special operation exception like on all other key related
actions.

The third patch adds a basic guest UV call API test. It has mostly
been used for firmware testing but I also think it's good to have a
building block like this for more PV tests.


GIT: https://github.com/frankjaa/kvm-unit-tests/tree/queue


Janosch Frank (3):
  s390x: Add custom pgm cleanup function
  s390x: skrf: Add exception new skey test and add test to unittests.cfg
  s390x: Ultavisor guest API test

 lib/s390x/asm/interrupt.h |   1 +
 lib/s390x/asm/uv.h        |  68 +++++++++++++++++
 lib/s390x/interrupt.c     |   9 +++
 s390x/Makefile            |   1 +
 s390x/skrf.c              |  81 ++++++++++++++++++++
 s390x/unittests.cfg       |   7 ++
 s390x/uv-guest.c          | 156 ++++++++++++++++++++++++++++++++++++++
 7 files changed, 323 insertions(+)
 create mode 100644 lib/s390x/asm/uv.h
 create mode 100644 s390x/uv-guest.c

-- 
2.25.1

[kvm-unit-tests PATCH 1/3] s390x: Add custom pgm cleanup function

[Janosch Frank]

Sometimes we need to do cleanup which we don't necessarily want to add
to interrupt.c, so lets add a way to register a cleanup function.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/asm/interrupt.h | 1 +
 lib/s390x/interrupt.c     | 9 +++++++++
 2 files changed, 10 insertions(+)

diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
index 4cfade9..b2a7c83 100644
--- a/lib/s390x/asm/interrupt.h
+++ b/lib/s390x/asm/interrupt.h
@@ -15,6 +15,7 @@
 #define EXT_IRQ_EXTERNAL_CALL	0x1202
 #define EXT_IRQ_SERVICE_SIG	0x2401
 
+void register_pgm_int_func(void (*f)(void));
 void handle_pgm_int(void);
 void handle_ext_int(void);
 void handle_mcck_int(void);
diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
index 243b9c2..36ba720 100644
--- a/lib/s390x/interrupt.c
+++ b/lib/s390x/interrupt.c
@@ -16,6 +16,7 @@
 
 static bool pgm_int_expected;
 static bool ext_int_expected;
+static void (*pgm_int_func)(void);
 static struct lowcore *lc;
 
 void expect_pgm_int(void)
@@ -51,8 +52,16 @@ void check_pgm_int_code(uint16_t code)
 	       lc->pgm_int_code);
 }
 
+void register_pgm_int_func(void (*f)(void))
+{
+	pgm_int_func = f;
+}
+
 static void fixup_pgm_int(void)
 {
+	if (pgm_int_func)
+		return (*pgm_int_func)();
+
 	switch (lc->pgm_int_code) {
 	case PGM_INT_CODE_PRIVILEGED_OPERATION:
 		/* Normal operation is in supervisor state, so this exception
-- 
2.25.1

[kvm-unit-tests PATCH 2/3] s390x: skrf: Add exception new skey test and add test to unittests.cfg

[Janosch Frank]

If a exception new psw mask contains a key a specification exception
instead of a special operation exception is presented. Let's test
that.

Also let's add the test to unittests.cfg so it is run more often.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/skrf.c        | 81 +++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |  4 +++
 2 files changed, 85 insertions(+)

diff --git a/s390x/skrf.c b/s390x/skrf.c
index 9cae589..9733412 100644
--- a/s390x/skrf.c
+++ b/s390x/skrf.c
@@ -15,6 +15,8 @@
 #include <asm/page.h>
 #include <asm/facility.h>
 #include <asm/mem.h>
+#include <asm/sigp.h>
+#include <smp.h>
 
 static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
 
@@ -106,6 +108,84 @@ static void test_tprot(void)
 	report_prefix_pop();
 }
 
+#include <asm-generic/barrier.h>
+static int testflag = 0;
+
+static void wait_for_flag(void)
+{
+	while (!testflag)
+		mb();
+}
+
+static void set_flag(int val)
+{
+	mb();
+	testflag = val;
+	mb();
+}
+
+static void ecall_cleanup(void)
+{
+	struct lowcore *lc = (void *)0x0;
+
+	lc->ext_new_psw.mask = 0x0000000180000000UL;
+	lc->sw_int_crs[0] = 0x0000000000040000;
+
+	/*
+	 * PGM old contains the ext new PSW, we need to clean it up,
+	 * so we don't get a special oepration exception on the lpswe
+	 * of pgm old.
+	 */
+	lc->pgm_old_psw.mask = 0x0000000180000000UL;
+	lc->pgm_old_psw.addr = (unsigned long)wait_for_flag;
+
+	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+	set_flag(1);
+}
+
+/* Set a key into the external new psw mask and open external call masks */
+static void ecall_setup(void)
+{
+	struct lowcore *lc = (void *)0x0;
+	uint64_t mask;
+
+	register_pgm_int_func(ecall_cleanup);
+	expect_pgm_int();
+	/* Put a skey into the ext new psw */
+	lc->ext_new_psw.mask = 0x00F0000180000000UL;
+	/* Open up ext masks */
+	ctl_set_bit(0, 13);
+	mask = extract_psw_mask();
+	mask |= PSW_MASK_EXT;
+	load_psw_mask(mask);
+	/* Tell cpu 0 that we're ready */
+	set_flag(1);
+}
+
+static void test_exception_ext_new(void)
+{
+	struct psw psw = {
+		.mask = extract_psw_mask(),
+		.addr = (unsigned long)ecall_setup
+	};
+
+	report_prefix_push("exception external new");
+	if (smp_query_num_cpus() < 2) {
+		report_skip("Need second cpu for exception external new test.");
+		report_prefix_pop();
+		return;
+	}
+
+	smp_cpu_setup(1, psw);
+	wait_for_flag();
+	set_flag(0);
+
+	sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);
+	wait_for_flag();
+	smp_cpu_stop(1);
+	report_prefix_pop();
+}
+
 int main(void)
 {
 	report_prefix_push("skrf");
@@ -121,6 +201,7 @@ int main(void)
 	test_mvcos();
 	test_spka();
 	test_tprot();
+	test_exception_ext_new();
 
 done:
 	report_prefix_pop();
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index 0f156af..b35269b 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -88,3 +88,7 @@ extra_params = -m 3G
 [css]
 file = css.elf
 extra_params = -device virtio-net-ccw
+
+[skrf]
+file = skrf.elf
+smp = 2
-- 
2.25.1

[kvm-unit-tests PATCH 3/3] s390x: Ultavisor guest API test

[Janosch Frank]

Test the error conditions of guest 2 Ultravisor calls, namely:
     * Query Ultravisor information
     * Set shared access
     * Remove shared access

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/asm/uv.h  |  68 +++++++++++++++++++
 s390x/Makefile      |   1 +
 s390x/unittests.cfg |   3 +
 s390x/uv-guest.c    | 156 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 228 insertions(+)
 create mode 100644 lib/s390x/asm/uv.h
 create mode 100644 s390x/uv-guest.c

diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
new file mode 100644
index 0000000..14ab5cc
--- /dev/null
+++ b/lib/s390x/asm/uv.h
@@ -0,0 +1,68 @@
+/*
+ * s390x Ultravisor related definitions
+ *
+ * Copyright (c) 2020 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 General Public License version 2.
+ */
+#ifndef UV_H
+#define UV_H
+
+#define UVC_RC_EXECUTED		0x0001
+#define UVC_RC_INV_CMD		0x0002
+#define UVC_RC_INV_STATE	0x0003
+#define UVC_RC_INV_LEN		0x0005
+#define UVC_RC_NO_RESUME	0x0007
+
+#define UVC_CMD_QUI			0x0001
+#define UVC_CMD_SET_SHARED_ACCESS	0x1000
+#define UVC_CMD_REMOVE_SHARED_ACCESS	0x1001
+
+/* Bits in installed uv calls */
+enum uv_cmds_inst {
+	BIT_UVC_CMD_QUI = 0,
+	BIT_UVC_CMD_SET_SHARED_ACCESS = 8,
+	BIT_UVC_CMD_REMOVE_SHARED_ACCESS = 9,
+};
+
+struct uv_cb_header {
+	u16 len;
+	u16 cmd;	/* Command Code */
+	u16 rc;		/* Response Code */
+	u16 rrc;	/* Return Reason Code */
+} __attribute__((packed))  __attribute__((aligned(8)));
+
+struct uv_cb_qui {
+	struct uv_cb_header header;
+	u64 reserved08;
+	u64 inst_calls_list[4];
+	u64 reserved30[15];
+} __attribute__((packed))  __attribute__((aligned(8)));
+
+struct uv_cb_share {
+	struct uv_cb_header header;
+	u64 reserved08[3];
+	u64 paddr;
+	u64 reserved28;
+} __attribute__((packed))  __attribute__((aligned(8)));
+
+static inline int uv_call(unsigned long r1, unsigned long r2)
+{
+	int cc;
+
+	asm volatile(
+		"0:	.insn rrf,0xB9A40000,%[r1],%[r2],0,0\n"
+		"		brc	3,0b\n"
+		"		ipm	%[cc]\n"
+		"		srl	%[cc],28\n"
+		: [cc] "=d" (cc)
+		: [r1] "a" (r1), [r2] "a" (r2)
+		: "memory", "cc");
+	return cc;
+}
+
+#endif
diff --git a/s390x/Makefile b/s390x/Makefile
index 0f54bf4..c2213ad 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -18,6 +18,7 @@ tests += $(TEST_DIR)/skrf.elf
 tests += $(TEST_DIR)/smp.elf
 tests += $(TEST_DIR)/sclp.elf
 tests += $(TEST_DIR)/css.elf
+tests += $(TEST_DIR)/uv-guest.elf
 tests_binary = $(patsubst %.elf,%.bin,$(tests))
 
 all: directories test_cases test_cases_binary
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index b35269b..38c3257 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -92,3 +92,6 @@ extra_params = -device virtio-net-ccw
 [skrf]
 file = skrf.elf
 smp = 2
+
+[uv-guest]
+file = uv-guest.elf
\ No newline at end of file
diff --git a/s390x/uv-guest.c b/s390x/uv-guest.c
new file mode 100644
index 0000000..0cb5fae
--- /dev/null
+++ b/s390x/uv-guest.c
@@ -0,0 +1,156 @@
+/*
+ * Guest Ultravisor Call tests
+ *
+ * Copyright (c) 2020 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 General Public License version 2.
+ */
+
+#include <libcflat.h>
+#include <alloc_page.h>
+#include <asm/page.h>
+#include <asm/asm-offsets.h>
+#include <asm/interrupt.h>
+#include <asm/facility.h>
+#include <asm/uv.h>
+
+static inline int share(unsigned long addr, u16 cmd)
+{
+	struct uv_cb_share uvcb = {
+		.header.cmd = cmd,
+		.header.len = sizeof(uvcb),
+		.paddr = addr
+	};
+
+	uv_call(0, (u64)&uvcb);
+	return uvcb.header.rc;
+}
+
+static inline int uv_set_shared(unsigned long addr)
+{
+	return share(addr, UVC_CMD_SET_SHARED_ACCESS);
+}
+
+static inline int uv_remove_shared(unsigned long addr)
+{
+	return share(addr, UVC_CMD_REMOVE_SHARED_ACCESS);
+}
+
+static void test_priv(void)
+{
+	struct uv_cb_header uvcb = {};
+
+	report_prefix_push("privileged");
+
+	report_prefix_push("query");
+	expect_pgm_int();
+	uvcb.cmd = UVC_CMD_QUI;
+	uvcb.len = sizeof(struct uv_cb_qui);
+	enter_pstate();
+	uv_call(0, (u64)&uvcb);
+	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
+	report_prefix_pop();
+
+	report_prefix_push("share");
+	expect_pgm_int();
+	enter_pstate();
+	uv_set_shared(0x42000);
+	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
+	report_prefix_pop();
+
+	report_prefix_push("unshare");
+	expect_pgm_int();
+	enter_pstate();
+	uv_remove_shared(0x42000);
+	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
+	report_prefix_pop();
+
+	report_prefix_pop();
+}
+
+static void test_query(void)
+{
+	struct uv_cb_qui uvcb = {
+		.header.cmd = UVC_CMD_QUI,
+		.header.len = sizeof(uvcb) - 8,
+	};
+
+	report_prefix_push("query");
+	uv_call(0, (u64)&uvcb);
+	report(uvcb.header.rc == UVC_RC_INV_LEN, "length");
+
+	uvcb.header.len = sizeof(uvcb);
+	uv_call(0, (u64)&uvcb);
+	report(uvcb.header.rc == UVC_RC_EXECUTED, "successful query");
+
+	/*
+	 * These bits have been introduced with the very first
+	 * Ultravisor version and are expected to always be available
+	 * because they are basic building blocks.
+	 */
+	report(uvcb.inst_calls_list[0] & (1UL << (63 - BIT_UVC_CMD_QUI)),
+	       "query indicated");
+	report(uvcb.inst_calls_list[0] & (1UL << (63 - BIT_UVC_CMD_SET_SHARED_ACCESS)),
+	       "share indicated");
+	report(uvcb.inst_calls_list[0] & (1UL << (63 - BIT_UVC_CMD_REMOVE_SHARED_ACCESS)),
+	       "unshare indicated");
+	report_prefix_pop();
+}
+
+static void test_sharing(void)
+{
+	unsigned long mem = (unsigned long)alloc_page();
+	struct uv_cb_share uvcb = {
+		.header.cmd = UVC_CMD_SET_SHARED_ACCESS,
+		.header.len = sizeof(uvcb) - 8,
+	};
+
+	report_prefix_push("share");
+	uv_call(0, (u64)&uvcb);
+	report(uvcb.header.rc == UVC_RC_INV_LEN, "length");
+	report(uv_set_shared(mem) == UVC_RC_EXECUTED, "share");
+	report_prefix_pop();
+
+	report_prefix_push("unshare");
+	uvcb.header.cmd = UVC_CMD_REMOVE_SHARED_ACCESS;
+	uv_call(0, (u64)&uvcb);
+	report(uvcb.header.rc == UVC_RC_INV_LEN, "length");
+	report(uv_remove_shared(mem) == UVC_RC_EXECUTED, "unshare");
+	free_page((void *)mem);
+	report_prefix_pop();
+
+	report_prefix_pop();
+}
+
+static void test_invalid(void)
+{
+	struct uv_cb_header uvcb = {
+		.len = 16,
+		.cmd = 0x4242,
+	};
+
+	uv_call(0, (u64)&uvcb);
+	report(uvcb.rc == UVC_RC_INV_CMD, "invalid command");
+}
+
+int main(void)
+{
+	bool has_uvc = test_facility(158);
+
+	report_prefix_push("uvc");
+	if (!has_uvc) {
+		report_skip("Ultravisor call facility is not available");
+		goto done;
+	}
+
+	test_priv();
+	test_invalid();
+	test_query();
+	test_sharing();
+done:
+	return report_summary();
+}
-- 
2.25.1

Re: [kvm-unit-tests PATCH 1/3] s390x: Add custom pgm cleanup function

[Claudio Imbrenda]

On Fri, 17 Jul 2020 10:58:11 -0400
Janosch Frank <frankja@linux.ibm.com> wrote:

> Sometimes we need to do cleanup which we don't necessarily want to add
> to interrupt.c, so lets add a way to register a cleanup function.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/asm/interrupt.h | 1 +
>  lib/s390x/interrupt.c     | 9 +++++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
> index 4cfade9..b2a7c83 100644
> --- a/lib/s390x/asm/interrupt.h
> +++ b/lib/s390x/asm/interrupt.h
> @@ -15,6 +15,7 @@
>  #define EXT_IRQ_EXTERNAL_CALL	0x1202
>  #define EXT_IRQ_SERVICE_SIG	0x2401
>  
> +void register_pgm_int_func(void (*f)(void));
>  void handle_pgm_int(void);
>  void handle_ext_int(void);
>  void handle_mcck_int(void);
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index 243b9c2..36ba720 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -16,6 +16,7 @@
>  
>  static bool pgm_int_expected;
>  static bool ext_int_expected;
> +static void (*pgm_int_func)(void);
>  static struct lowcore *lc;
>  
>  void expect_pgm_int(void)
> @@ -51,8 +52,16 @@ void check_pgm_int_code(uint16_t code)
>  	       lc->pgm_int_code);
>  }
>  
> +void register_pgm_int_func(void (*f)(void))
> +{
> +	pgm_int_func = f;
> +}
> +
>  static void fixup_pgm_int(void)
>  {
> +	if (pgm_int_func)
> +		return (*pgm_int_func)();
> +
>  	switch (lc->pgm_int_code) {
>  	case PGM_INT_CODE_PRIVILEGED_OPERATION:
>  		/* Normal operation is in supervisor state, so this
> exception

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

Re: [kvm-unit-tests PATCH 2/3] s390x: skrf: Add exception new skey test and add test to unittests.cfg

[Claudio Imbrenda]

On Fri, 17 Jul 2020 10:58:12 -0400
Janosch Frank <frankja@linux.ibm.com> wrote:

> If a exception new psw mask contains a key a specification exception
> instead of a special operation exception is presented. Let's test
> that.
> 
> Also let's add the test to unittests.cfg so it is run more often.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/skrf.c        | 81
> +++++++++++++++++++++++++++++++++++++++++++++ s390x/unittests.cfg |
> 4 +++ 2 files changed, 85 insertions(+)
> 
> diff --git a/s390x/skrf.c b/s390x/skrf.c
> index 9cae589..9733412 100644
> --- a/s390x/skrf.c
> +++ b/s390x/skrf.c
> @@ -15,6 +15,8 @@
>  #include <asm/page.h>
>  #include <asm/facility.h>
>  #include <asm/mem.h>
> +#include <asm/sigp.h>
> +#include <smp.h>
>  
>  static uint8_t pagebuf[PAGE_SIZE * 2]
> __attribute__((aligned(PAGE_SIZE * 2))); 
> @@ -106,6 +108,84 @@ static void test_tprot(void)
>  	report_prefix_pop();
>  }
>  
> +#include <asm-generic/barrier.h>
> +static int testflag = 0;
> +
> +static void wait_for_flag(void)
> +{
> +	while (!testflag)
> +		mb();
> +}
> +
> +static void set_flag(int val)
> +{
> +	mb();
> +	testflag = val;
> +	mb();
> +}
> +
> +static void ecall_cleanup(void)
> +{
> +	struct lowcore *lc = (void *)0x0;
> +
> +	lc->ext_new_psw.mask = 0x0000000180000000UL;
> +	lc->sw_int_crs[0] = 0x0000000000040000;
> +
> +	/*
> +	 * PGM old contains the ext new PSW, we need to clean it up,
> +	 * so we don't get a special oepration exception on the lpswe
> +	 * of pgm old.
> +	 */
> +	lc->pgm_old_psw.mask = 0x0000000180000000UL;
> +	lc->pgm_old_psw.addr = (unsigned long)wait_for_flag;
> +
> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	set_flag(1);
> +}
> +
> +/* Set a key into the external new psw mask and open external call
> masks */ +static void ecall_setup(void)
> +{
> +	struct lowcore *lc = (void *)0x0;
> +	uint64_t mask;
> +
> +	register_pgm_int_func(ecall_cleanup);
> +	expect_pgm_int();
> +	/* Put a skey into the ext new psw */
> +	lc->ext_new_psw.mask = 0x00F0000180000000UL;
> +	/* Open up ext masks */
> +	ctl_set_bit(0, 13);
> +	mask = extract_psw_mask();
> +	mask |= PSW_MASK_EXT;
> +	load_psw_mask(mask);
> +	/* Tell cpu 0 that we're ready */
> +	set_flag(1);
> +}
> +
> +static void test_exception_ext_new(void)
> +{
> +	struct psw psw = {
> +		.mask = extract_psw_mask(),
> +		.addr = (unsigned long)ecall_setup
> +	};
> +
> +	report_prefix_push("exception external new");
> +	if (smp_query_num_cpus() < 2) {
> +		report_skip("Need second cpu for exception external
> new test.");
> +		report_prefix_pop();
> +		return;
> +	}
> +
> +	smp_cpu_setup(1, psw);
> +	wait_for_flag();
> +	set_flag(0);
> +
> +	sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);
> +	wait_for_flag();
> +	smp_cpu_stop(1);
> +	report_prefix_pop();
> +}
> +
>  int main(void)
>  {
>  	report_prefix_push("skrf");
> @@ -121,6 +201,7 @@ int main(void)
>  	test_mvcos();
>  	test_spka();
>  	test_tprot();
> +	test_exception_ext_new();
>  
>  done:
>  	report_prefix_pop();
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index 0f156af..b35269b 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -88,3 +88,7 @@ extra_params = -m 3G
>  [css]
>  file = css.elf
>  extra_params = -device virtio-net-ccw
> +
> +[skrf]
> +file = skrf.elf
> +smp = 2

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

Re: [kvm-unit-tests PATCH 3/3] s390x: Ultavisor guest API test

[Claudio Imbrenda]

On Fri, 17 Jul 2020 10:58:13 -0400
Janosch Frank <frankja@linux.ibm.com> wrote:

> Test the error conditions of guest 2 Ultravisor calls, namely:
>      * Query Ultravisor information
>      * Set shared access
>      * Remove shared access
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/asm/uv.h  |  68 +++++++++++++++++++
>  s390x/Makefile      |   1 +
>  s390x/unittests.cfg |   3 +
>  s390x/uv-guest.c    | 156
> ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 228
> insertions(+) create mode 100644 lib/s390x/asm/uv.h
>  create mode 100644 s390x/uv-guest.c
> 
> diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
> new file mode 100644
> index 0000000..14ab5cc
> --- /dev/null
> +++ b/lib/s390x/asm/uv.h
> @@ -0,0 +1,68 @@
> +/*
> + * s390x Ultravisor related definitions
> + *
> + * Copyright (c) 2020 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 General Public License version 2.
> + */
> +#ifndef UV_H
> +#define UV_H
> +
> +#define UVC_RC_EXECUTED		0x0001
> +#define UVC_RC_INV_CMD		0x0002
> +#define UVC_RC_INV_STATE	0x0003
> +#define UVC_RC_INV_LEN		0x0005
> +#define UVC_RC_NO_RESUME	0x0007
> +
> +#define UVC_CMD_QUI			0x0001
> +#define UVC_CMD_SET_SHARED_ACCESS	0x1000
> +#define UVC_CMD_REMOVE_SHARED_ACCESS	0x1001
> +
> +/* Bits in installed uv calls */
> +enum uv_cmds_inst {
> +	BIT_UVC_CMD_QUI = 0,
> +	BIT_UVC_CMD_SET_SHARED_ACCESS = 8,
> +	BIT_UVC_CMD_REMOVE_SHARED_ACCESS = 9,
> +};
> +
> +struct uv_cb_header {
> +	u16 len;
> +	u16 cmd;	/* Command Code */
> +	u16 rc;		/* Response Code */
> +	u16 rrc;	/* Return Reason Code */
> +} __attribute__((packed))  __attribute__((aligned(8)));
> +
> +struct uv_cb_qui {
> +	struct uv_cb_header header;
> +	u64 reserved08;
> +	u64 inst_calls_list[4];
> +	u64 reserved30[15];
> +} __attribute__((packed))  __attribute__((aligned(8)));
> +
> +struct uv_cb_share {
> +	struct uv_cb_header header;
> +	u64 reserved08[3];
> +	u64 paddr;
> +	u64 reserved28;
> +} __attribute__((packed))  __attribute__((aligned(8)));
> +
> +static inline int uv_call(unsigned long r1, unsigned long r2)
> +{
> +	int cc;
> +
> +	asm volatile(
> +		"0:	.insn rrf,0xB9A40000,%[r1],%[r2],0,0\n"
> +		"		brc	3,0b\n"
> +		"		ipm	%[cc]\n"
> +		"		srl	%[cc],28\n"
> +		: [cc] "=d" (cc)
> +		: [r1] "a" (r1), [r2] "a" (r2)
> +		: "memory", "cc");
> +	return cc;
> +}
> +
> +#endif
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 0f54bf4..c2213ad 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -18,6 +18,7 @@ tests += $(TEST_DIR)/skrf.elf
>  tests += $(TEST_DIR)/smp.elf
>  tests += $(TEST_DIR)/sclp.elf
>  tests += $(TEST_DIR)/css.elf
> +tests += $(TEST_DIR)/uv-guest.elf
>  tests_binary = $(patsubst %.elf,%.bin,$(tests))
>  
>  all: directories test_cases test_cases_binary
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index b35269b..38c3257 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -92,3 +92,6 @@ extra_params = -device virtio-net-ccw
>  [skrf]
>  file = skrf.elf
>  smp = 2
> +
> +[uv-guest]
> +file = uv-guest.elf
> \ No newline at end of file
> diff --git a/s390x/uv-guest.c b/s390x/uv-guest.c
> new file mode 100644
> index 0000000..0cb5fae
> --- /dev/null
> +++ b/s390x/uv-guest.c
> @@ -0,0 +1,156 @@
> +/*
> + * Guest Ultravisor Call tests
> + *
> + * Copyright (c) 2020 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 General Public License version 2.
> + */
> +
> +#include <libcflat.h>
> +#include <alloc_page.h>
> +#include <asm/page.h>
> +#include <asm/asm-offsets.h>
> +#include <asm/interrupt.h>
> +#include <asm/facility.h>
> +#include <asm/uv.h>
> +
> +static inline int share(unsigned long addr, u16 cmd)
> +{
> +	struct uv_cb_share uvcb = {
> +		.header.cmd = cmd,
> +		.header.len = sizeof(uvcb),
> +		.paddr = addr
> +	};
> +
> +	uv_call(0, (u64)&uvcb);
> +	return uvcb.header.rc;
> +}
> +
> +static inline int uv_set_shared(unsigned long addr)
> +{
> +	return share(addr, UVC_CMD_SET_SHARED_ACCESS);
> +}
> +
> +static inline int uv_remove_shared(unsigned long addr)
> +{
> +	return share(addr, UVC_CMD_REMOVE_SHARED_ACCESS);
> +}
> +
> +static void test_priv(void)
> +{
> +	struct uv_cb_header uvcb = {};
> +
> +	report_prefix_push("privileged");
> +
> +	report_prefix_push("query");
> +	expect_pgm_int();
> +	uvcb.cmd = UVC_CMD_QUI;
> +	uvcb.len = sizeof(struct uv_cb_qui);
> +	enter_pstate();
> +	uv_call(0, (u64)&uvcb);
> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("share");
> +	expect_pgm_int();
> +	enter_pstate();
> +	uv_set_shared(0x42000);
> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("unshare");
> +	expect_pgm_int();
> +	enter_pstate();
> +	uv_remove_shared(0x42000);

I don't like using a hardwired address here (and above). Things can get
messy if the address ends up outside memory or overlapping code.

I think the best approach would be to declare a page aligned buffer and
use that (or even allocate a page) 

> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
> +	report_prefix_pop();
> +
> +	report_prefix_pop();
> +}
> +
> +static void test_query(void)
> +{
> +	struct uv_cb_qui uvcb = {
> +		.header.cmd = UVC_CMD_QUI,
> +		.header.len = sizeof(uvcb) - 8,
> +	};
> +
> +	report_prefix_push("query");
> +	uv_call(0, (u64)&uvcb);
> +	report(uvcb.header.rc == UVC_RC_INV_LEN, "length");
> +
> +	uvcb.header.len = sizeof(uvcb);
> +	uv_call(0, (u64)&uvcb);
> +	report(uvcb.header.rc == UVC_RC_EXECUTED, "successful
> query"); +
> +	/*
> +	 * These bits have been introduced with the very first
> +	 * Ultravisor version and are expected to always be available
> +	 * because they are basic building blocks.
> +	 */
> +	report(uvcb.inst_calls_list[0] & (1UL << (63 -
> BIT_UVC_CMD_QUI)),
> +	       "query indicated");
> +	report(uvcb.inst_calls_list[0] & (1UL << (63 -
> BIT_UVC_CMD_SET_SHARED_ACCESS)),
> +	       "share indicated");
> +	report(uvcb.inst_calls_list[0] & (1UL << (63 -
> BIT_UVC_CMD_REMOVE_SHARED_ACCESS)),
> +	       "unshare indicated");
> +	report_prefix_pop();
> +}
> +
> +static void test_sharing(void)
> +{
> +	unsigned long mem = (unsigned long)alloc_page();
> +	struct uv_cb_share uvcb = {
> +		.header.cmd = UVC_CMD_SET_SHARED_ACCESS,
> +		.header.len = sizeof(uvcb) - 8,
> +	};
> +
> +	report_prefix_push("share");
> +	uv_call(0, (u64)&uvcb);
> +	report(uvcb.header.rc == UVC_RC_INV_LEN, "length");
> +	report(uv_set_shared(mem) == UVC_RC_EXECUTED, "share");
> +	report_prefix_pop();
> +
> +	report_prefix_push("unshare");
> +	uvcb.header.cmd = UVC_CMD_REMOVE_SHARED_ACCESS;
> +	uv_call(0, (u64)&uvcb);
> +	report(uvcb.header.rc == UVC_RC_INV_LEN, "length");
> +	report(uv_remove_shared(mem) == UVC_RC_EXECUTED, "unshare");
> +	free_page((void *)mem);
> +	report_prefix_pop();
> +
> +	report_prefix_pop();
> +}
> +
> +static void test_invalid(void)
> +{
> +	struct uv_cb_header uvcb = {
> +		.len = 16,
> +		.cmd = 0x4242,
> +	};
> +
> +	uv_call(0, (u64)&uvcb);
> +	report(uvcb.rc == UVC_RC_INV_CMD, "invalid command");
> +}
> +
> +int main(void)
> +{
> +	bool has_uvc = test_facility(158);
> +
> +	report_prefix_push("uvc");
> +	if (!has_uvc) {
> +		report_skip("Ultravisor call facility is not
> available");
> +		goto done;
> +	}
> +
> +	test_priv();
> +	test_invalid();
> +	test_query();
> +	test_sharing();
> +done:
> +	return report_summary();
> +}

once the above comment is addressed:
Claudio Imbrenda <imbrenda@linux.ibm.com>

Re: [kvm-unit-tests PATCH 3/3] s390x: Ultavisor guest API test

[Janosch Frank]

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

On 7/20/20 12:49 PM, Claudio Imbrenda wrote:
> On Fri, 17 Jul 2020 10:58:13 -0400
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> Test the error conditions of guest 2 Ultravisor calls, namely:
>>      * Query Ultravisor information
>>      * Set shared access
>>      * Remove shared access
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  lib/s390x/asm/uv.h  |  68 +++++++++++++++++++
>>  s390x/Makefile      |   1 +
>>  s390x/unittests.cfg |   3 +
>>  s390x/uv-guest.c    | 156
>> ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 228
>> insertions(+) create mode 100644 lib/s390x/asm/uv.h
>>  create mode 100644 s390x/uv-guest.c
>>
>> diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
>> new file mode 100644
>> index 0000000..14ab5cc
>> --- /dev/null
>> +++ b/lib/s390x/asm/uv.h
>> @@ -0,0 +1,68 @@
>> +/*
>> + * s390x Ultravisor related definitions
>> + *
>> + * Copyright (c) 2020 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 General Public License version 2.
>> + */
>> +#ifndef UV_H
>> +#define UV_H
>> +
>> +#define UVC_RC_EXECUTED		0x0001
>> +#define UVC_RC_INV_CMD		0x0002
>> +#define UVC_RC_INV_STATE	0x0003
>> +#define UVC_RC_INV_LEN		0x0005
>> +#define UVC_RC_NO_RESUME	0x0007
>> +
>> +#define UVC_CMD_QUI			0x0001
>> +#define UVC_CMD_SET_SHARED_ACCESS	0x1000
>> +#define UVC_CMD_REMOVE_SHARED_ACCESS	0x1001
>> +
>> +/* Bits in installed uv calls */
>> +enum uv_cmds_inst {
>> +	BIT_UVC_CMD_QUI = 0,
>> +	BIT_UVC_CMD_SET_SHARED_ACCESS = 8,
>> +	BIT_UVC_CMD_REMOVE_SHARED_ACCESS = 9,
>> +};
>> +
>> +struct uv_cb_header {
>> +	u16 len;
>> +	u16 cmd;	/* Command Code */
>> +	u16 rc;		/* Response Code */
>> +	u16 rrc;	/* Return Reason Code */
>> +} __attribute__((packed))  __attribute__((aligned(8)));
>> +
>> +struct uv_cb_qui {
>> +	struct uv_cb_header header;
>> +	u64 reserved08;
>> +	u64 inst_calls_list[4];
>> +	u64 reserved30[15];
>> +} __attribute__((packed))  __attribute__((aligned(8)));
>> +
>> +struct uv_cb_share {
>> +	struct uv_cb_header header;
>> +	u64 reserved08[3];
>> +	u64 paddr;
>> +	u64 reserved28;
>> +} __attribute__((packed))  __attribute__((aligned(8)));
>> +
>> +static inline int uv_call(unsigned long r1, unsigned long r2)
>> +{
>> +	int cc;
>> +
>> +	asm volatile(
>> +		"0:	.insn rrf,0xB9A40000,%[r1],%[r2],0,0\n"
>> +		"		brc	3,0b\n"
>> +		"		ipm	%[cc]\n"
>> +		"		srl	%[cc],28\n"
>> +		: [cc] "=d" (cc)
>> +		: [r1] "a" (r1), [r2] "a" (r2)
>> +		: "memory", "cc");
>> +	return cc;
>> +}
>> +
>> +#endif
>> diff --git a/s390x/Makefile b/s390x/Makefile
>> index 0f54bf4..c2213ad 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -18,6 +18,7 @@ tests += $(TEST_DIR)/skrf.elf
>>  tests += $(TEST_DIR)/smp.elf
>>  tests += $(TEST_DIR)/sclp.elf
>>  tests += $(TEST_DIR)/css.elf
>> +tests += $(TEST_DIR)/uv-guest.elf
>>  tests_binary = $(patsubst %.elf,%.bin,$(tests))
>>  
>>  all: directories test_cases test_cases_binary
>> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
>> index b35269b..38c3257 100644
>> --- a/s390x/unittests.cfg
>> +++ b/s390x/unittests.cfg
>> @@ -92,3 +92,6 @@ extra_params = -device virtio-net-ccw
>>  [skrf]
>>  file = skrf.elf
>>  smp = 2
>> +
>> +[uv-guest]
>> +file = uv-guest.elf
>> \ No newline at end of file
>> diff --git a/s390x/uv-guest.c b/s390x/uv-guest.c
>> new file mode 100644
>> index 0000000..0cb5fae
>> --- /dev/null
>> +++ b/s390x/uv-guest.c
>> @@ -0,0 +1,156 @@
>> +/*
>> + * Guest Ultravisor Call tests
>> + *
>> + * Copyright (c) 2020 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 General Public License version 2.
>> + */
>> +
>> +#include <libcflat.h>
>> +#include <alloc_page.h>
>> +#include <asm/page.h>
>> +#include <asm/asm-offsets.h>
>> +#include <asm/interrupt.h>
>> +#include <asm/facility.h>
>> +#include <asm/uv.h>
>> +
>> +static inline int share(unsigned long addr, u16 cmd)
>> +{
>> +	struct uv_cb_share uvcb = {
>> +		.header.cmd = cmd,
>> +		.header.len = sizeof(uvcb),
>> +		.paddr = addr
>> +	};
>> +
>> +	uv_call(0, (u64)&uvcb);
>> +	return uvcb.header.rc;
>> +}
>> +
>> +static inline int uv_set_shared(unsigned long addr)
>> +{
>> +	return share(addr, UVC_CMD_SET_SHARED_ACCESS);
>> +}
>> +
>> +static inline int uv_remove_shared(unsigned long addr)
>> +{
>> +	return share(addr, UVC_CMD_REMOVE_SHARED_ACCESS);
>> +}
>> +
>> +static void test_priv(void)
>> +{
>> +	struct uv_cb_header uvcb = {};
>> +
>> +	report_prefix_push("privileged");
>> +
>> +	report_prefix_push("query");
>> +	expect_pgm_int();
>> +	uvcb.cmd = UVC_CMD_QUI;
>> +	uvcb.len = sizeof(struct uv_cb_qui);
>> +	enter_pstate();
>> +	uv_call(0, (u64)&uvcb);
>> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
>> +	report_prefix_pop();
>> +
>> +	report_prefix_push("share");
>> +	expect_pgm_int();
>> +	enter_pstate();
>> +	uv_set_shared(0x42000);
>> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
>> +	report_prefix_pop();
>> +
>> +	report_prefix_push("unshare");
>> +	expect_pgm_int();
>> +	enter_pstate();
>> +	uv_remove_shared(0x42000);
> 
> I don't like using a hardwired address here (and above). Things can get
> messy if the address ends up outside memory or overlapping code.
> 
> I think the best approach would be to declare a page aligned buffer and
> use that (or even allocate a page) 

Sure, will do

> 
>> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
>> +	report_prefix_pop();
>> +
>> +	report_prefix_pop();
>> +}
>> +
>> +static void test_query(void)
>> +{
>> +	struct uv_cb_qui uvcb = {
>> +		.header.cmd = UVC_CMD_QUI,
>> +		.header.len = sizeof(uvcb) - 8,
>> +	};
>> +
>> +	report_prefix_push("query");
>> +	uv_call(0, (u64)&uvcb);
>> +	report(uvcb.header.rc == UVC_RC_INV_LEN, "length");
>> +
>> +	uvcb.header.len = sizeof(uvcb);
>> +	uv_call(0, (u64)&uvcb);
>> +	report(uvcb.header.rc == UVC_RC_EXECUTED, "successful
>> query"); +
>> +	/*
>> +	 * These bits have been introduced with the very first
>> +	 * Ultravisor version and are expected to always be available
>> +	 * because they are basic building blocks.
>> +	 */
>> +	report(uvcb.inst_calls_list[0] & (1UL << (63 -
>> BIT_UVC_CMD_QUI)),
>> +	       "query indicated");
>> +	report(uvcb.inst_calls_list[0] & (1UL << (63 -
>> BIT_UVC_CMD_SET_SHARED_ACCESS)),
>> +	       "share indicated");
>> +	report(uvcb.inst_calls_list[0] & (1UL << (63 -
>> BIT_UVC_CMD_REMOVE_SHARED_ACCESS)),
>> +	       "unshare indicated");
>> +	report_prefix_pop();
>> +}
>> +
>> +static void test_sharing(void)
>> +{
>> +	unsigned long mem = (unsigned long)alloc_page();
>> +	struct uv_cb_share uvcb = {
>> +		.header.cmd = UVC_CMD_SET_SHARED_ACCESS,
>> +		.header.len = sizeof(uvcb) - 8,
>> +	};
>> +
>> +	report_prefix_push("share");
>> +	uv_call(0, (u64)&uvcb);
>> +	report(uvcb.header.rc == UVC_RC_INV_LEN, "length");
>> +	report(uv_set_shared(mem) == UVC_RC_EXECUTED, "share");
>> +	report_prefix_pop();
>> +
>> +	report_prefix_push("unshare");
>> +	uvcb.header.cmd = UVC_CMD_REMOVE_SHARED_ACCESS;
>> +	uv_call(0, (u64)&uvcb);
>> +	report(uvcb.header.rc == UVC_RC_INV_LEN, "length");
>> +	report(uv_remove_shared(mem) == UVC_RC_EXECUTED, "unshare");
>> +	free_page((void *)mem);
>> +	report_prefix_pop();
>> +
>> +	report_prefix_pop();
>> +}
>> +
>> +static void test_invalid(void)
>> +{
>> +	struct uv_cb_header uvcb = {
>> +		.len = 16,
>> +		.cmd = 0x4242,
>> +	};
>> +
>> +	uv_call(0, (u64)&uvcb);
>> +	report(uvcb.rc == UVC_RC_INV_CMD, "invalid command");
>> +}
>> +
>> +int main(void)
>> +{
>> +	bool has_uvc = test_facility(158);
>> +
>> +	report_prefix_push("uvc");
>> +	if (!has_uvc) {
>> +		report_skip("Ultravisor call facility is not
>> available");
>> +		goto done;
>> +	}
>> +
>> +	test_priv();
>> +	test_invalid();
>> +	test_query();
>> +	test_sharing();
>> +done:
>> +	return report_summary();
>> +}
> 
> once the above comment is addressed:
> Claudio Imbrenda <imbrenda@linux.ibm.com>
> 
?


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

Re: [kvm-unit-tests PATCH 3/3] s390x: Ultavisor guest API test

[Claudio Imbrenda]

On Mon, 20 Jul 2020 13:42:35 +0200
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 7/20/20 12:49 PM, Claudio Imbrenda wrote:
> > On Fri, 17 Jul 2020 10:58:13 -0400
> > Janosch Frank <frankja@linux.ibm.com> wrote:
> >   
> >> Test the error conditions of guest 2 Ultravisor calls, namely:
> >>      * Query Ultravisor information
> >>      * Set shared access
> >>      * Remove shared access
> >>
> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> >> ---
> >>  lib/s390x/asm/uv.h  |  68 +++++++++++++++++++
> >>  s390x/Makefile      |   1 +
> >>  s390x/unittests.cfg |   3 +
> >>  s390x/uv-guest.c    | 156
> >> ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 228
> >> insertions(+) create mode 100644 lib/s390x/asm/uv.h
> >>  create mode 100644 s390x/uv-guest.c
> >>
> >> diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
> >> new file mode 100644
> >> index 0000000..14ab5cc
> >> --- /dev/null
> >> +++ b/lib/s390x/asm/uv.h
> >> @@ -0,0 +1,68 @@
> >> +/*
> >> + * s390x Ultravisor related definitions
> >> + *
> >> + * Copyright (c) 2020 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 General Public License version 2.
> >> + */
> >> +#ifndef UV_H
> >> +#define UV_H
> >> +
> >> +#define UVC_RC_EXECUTED		0x0001
> >> +#define UVC_RC_INV_CMD		0x0002
> >> +#define UVC_RC_INV_STATE	0x0003
> >> +#define UVC_RC_INV_LEN		0x0005
> >> +#define UVC_RC_NO_RESUME	0x0007
> >> +
> >> +#define UVC_CMD_QUI			0x0001
> >> +#define UVC_CMD_SET_SHARED_ACCESS	0x1000
> >> +#define UVC_CMD_REMOVE_SHARED_ACCESS	0x1001
> >> +
> >> +/* Bits in installed uv calls */
> >> +enum uv_cmds_inst {
> >> +	BIT_UVC_CMD_QUI = 0,
> >> +	BIT_UVC_CMD_SET_SHARED_ACCESS = 8,
> >> +	BIT_UVC_CMD_REMOVE_SHARED_ACCESS = 9,
> >> +};
> >> +
> >> +struct uv_cb_header {
> >> +	u16 len;
> >> +	u16 cmd;	/* Command Code */
> >> +	u16 rc;		/* Response Code */
> >> +	u16 rrc;	/* Return Reason Code */
> >> +} __attribute__((packed))  __attribute__((aligned(8)));
> >> +
> >> +struct uv_cb_qui {
> >> +	struct uv_cb_header header;
> >> +	u64 reserved08;
> >> +	u64 inst_calls_list[4];
> >> +	u64 reserved30[15];
> >> +} __attribute__((packed))  __attribute__((aligned(8)));
> >> +
> >> +struct uv_cb_share {
> >> +	struct uv_cb_header header;
> >> +	u64 reserved08[3];
> >> +	u64 paddr;
> >> +	u64 reserved28;
> >> +} __attribute__((packed))  __attribute__((aligned(8)));
> >> +
> >> +static inline int uv_call(unsigned long r1, unsigned long r2)
> >> +{
> >> +	int cc;
> >> +
> >> +	asm volatile(
> >> +		"0:	.insn rrf,0xB9A40000,%[r1],%[r2],0,0\n"
> >> +		"		brc	3,0b\n"
> >> +		"		ipm	%[cc]\n"
> >> +		"		srl	%[cc],28\n"
> >> +		: [cc] "=d" (cc)
> >> +		: [r1] "a" (r1), [r2] "a" (r2)
> >> +		: "memory", "cc");
> >> +	return cc;
> >> +}
> >> +
> >> +#endif
> >> diff --git a/s390x/Makefile b/s390x/Makefile
> >> index 0f54bf4..c2213ad 100644
> >> --- a/s390x/Makefile
> >> +++ b/s390x/Makefile
> >> @@ -18,6 +18,7 @@ tests += $(TEST_DIR)/skrf.elf
> >>  tests += $(TEST_DIR)/smp.elf
> >>  tests += $(TEST_DIR)/sclp.elf
> >>  tests += $(TEST_DIR)/css.elf
> >> +tests += $(TEST_DIR)/uv-guest.elf
> >>  tests_binary = $(patsubst %.elf,%.bin,$(tests))
> >>  
> >>  all: directories test_cases test_cases_binary
> >> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> >> index b35269b..38c3257 100644
> >> --- a/s390x/unittests.cfg
> >> +++ b/s390x/unittests.cfg
> >> @@ -92,3 +92,6 @@ extra_params = -device virtio-net-ccw
> >>  [skrf]
> >>  file = skrf.elf
> >>  smp = 2
> >> +
> >> +[uv-guest]
> >> +file = uv-guest.elf
> >> \ No newline at end of file
> >> diff --git a/s390x/uv-guest.c b/s390x/uv-guest.c
> >> new file mode 100644
> >> index 0000000..0cb5fae
> >> --- /dev/null
> >> +++ b/s390x/uv-guest.c
> >> @@ -0,0 +1,156 @@
> >> +/*
> >> + * Guest Ultravisor Call tests
> >> + *
> >> + * Copyright (c) 2020 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 General Public License version 2.
> >> + */
> >> +
> >> +#include <libcflat.h>
> >> +#include <alloc_page.h>
> >> +#include <asm/page.h>
> >> +#include <asm/asm-offsets.h>
> >> +#include <asm/interrupt.h>
> >> +#include <asm/facility.h>
> >> +#include <asm/uv.h>
> >> +
> >> +static inline int share(unsigned long addr, u16 cmd)
> >> +{
> >> +	struct uv_cb_share uvcb = {
> >> +		.header.cmd = cmd,
> >> +		.header.len = sizeof(uvcb),
> >> +		.paddr = addr
> >> +	};
> >> +
> >> +	uv_call(0, (u64)&uvcb);
> >> +	return uvcb.header.rc;
> >> +}
> >> +
> >> +static inline int uv_set_shared(unsigned long addr)
> >> +{
> >> +	return share(addr, UVC_CMD_SET_SHARED_ACCESS);
> >> +}
> >> +
> >> +static inline int uv_remove_shared(unsigned long addr)
> >> +{
> >> +	return share(addr, UVC_CMD_REMOVE_SHARED_ACCESS);
> >> +}
> >> +
> >> +static void test_priv(void)
> >> +{
> >> +	struct uv_cb_header uvcb = {};
> >> +
> >> +	report_prefix_push("privileged");
> >> +
> >> +	report_prefix_push("query");
> >> +	expect_pgm_int();
> >> +	uvcb.cmd = UVC_CMD_QUI;
> >> +	uvcb.len = sizeof(struct uv_cb_qui);
> >> +	enter_pstate();
> >> +	uv_call(0, (u64)&uvcb);
> >> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
> >> +	report_prefix_pop();
> >> +
> >> +	report_prefix_push("share");
> >> +	expect_pgm_int();
> >> +	enter_pstate();
> >> +	uv_set_shared(0x42000);
> >> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
> >> +	report_prefix_pop();
> >> +
> >> +	report_prefix_push("unshare");
> >> +	expect_pgm_int();
> >> +	enter_pstate();
> >> +	uv_remove_shared(0x42000);  
> > 
> > I don't like using a hardwired address here (and above). Things can
> > get messy if the address ends up outside memory or overlapping code.
> > 
> > I think the best approach would be to declare a page aligned buffer
> > and use that (or even allocate a page)   
> 
> Sure, will do
> 
> >   
> >> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
> >> +	report_prefix_pop();
> >> +
> >> +	report_prefix_pop();
> >> +}
> >> +
> >> +static void test_query(void)
> >> +{
> >> +	struct uv_cb_qui uvcb = {
> >> +		.header.cmd = UVC_CMD_QUI,
> >> +		.header.len = sizeof(uvcb) - 8,
> >> +	};
> >> +
> >> +	report_prefix_push("query");
> >> +	uv_call(0, (u64)&uvcb);
> >> +	report(uvcb.header.rc == UVC_RC_INV_LEN, "length");
> >> +
> >> +	uvcb.header.len = sizeof(uvcb);
> >> +	uv_call(0, (u64)&uvcb);
> >> +	report(uvcb.header.rc == UVC_RC_EXECUTED, "successful
> >> query"); +
> >> +	/*
> >> +	 * These bits have been introduced with the very first
> >> +	 * Ultravisor version and are expected to always be
> >> available
> >> +	 * because they are basic building blocks.
> >> +	 */
> >> +	report(uvcb.inst_calls_list[0] & (1UL << (63 -
> >> BIT_UVC_CMD_QUI)),
> >> +	       "query indicated");
> >> +	report(uvcb.inst_calls_list[0] & (1UL << (63 -
> >> BIT_UVC_CMD_SET_SHARED_ACCESS)),
> >> +	       "share indicated");
> >> +	report(uvcb.inst_calls_list[0] & (1UL << (63 -
> >> BIT_UVC_CMD_REMOVE_SHARED_ACCESS)),
> >> +	       "unshare indicated");
> >> +	report_prefix_pop();
> >> +}
> >> +
> >> +static void test_sharing(void)
> >> +{
> >> +	unsigned long mem = (unsigned long)alloc_page();
> >> +	struct uv_cb_share uvcb = {
> >> +		.header.cmd = UVC_CMD_SET_SHARED_ACCESS,
> >> +		.header.len = sizeof(uvcb) - 8,
> >> +	};
> >> +
> >> +	report_prefix_push("share");
> >> +	uv_call(0, (u64)&uvcb);
> >> +	report(uvcb.header.rc == UVC_RC_INV_LEN, "length");
> >> +	report(uv_set_shared(mem) == UVC_RC_EXECUTED, "share");
> >> +	report_prefix_pop();
> >> +
> >> +	report_prefix_push("unshare");
> >> +	uvcb.header.cmd = UVC_CMD_REMOVE_SHARED_ACCESS;
> >> +	uv_call(0, (u64)&uvcb);
> >> +	report(uvcb.header.rc == UVC_RC_INV_LEN, "length");
> >> +	report(uv_remove_shared(mem) == UVC_RC_EXECUTED,
> >> "unshare");
> >> +	free_page((void *)mem);
> >> +	report_prefix_pop();
> >> +
> >> +	report_prefix_pop();
> >> +}
> >> +
> >> +static void test_invalid(void)
> >> +{
> >> +	struct uv_cb_header uvcb = {
> >> +		.len = 16,
> >> +		.cmd = 0x4242,
> >> +	};
> >> +
> >> +	uv_call(0, (u64)&uvcb);
> >> +	report(uvcb.rc == UVC_RC_INV_CMD, "invalid command");
> >> +}
> >> +
> >> +int main(void)
> >> +{
> >> +	bool has_uvc = test_facility(158);
> >> +
> >> +	report_prefix_push("uvc");
> >> +	if (!has_uvc) {
> >> +		report_skip("Ultravisor call facility is not
> >> available");
> >> +		goto done;
> >> +	}
> >> +
> >> +	test_priv();
> >> +	test_invalid();
> >> +	test_query();
> >> +	test_sharing();
> >> +done:
> >> +	return report_summary();
> >> +}  
> > 
> > once the above comment is addressed:
> > Claudio Imbrenda <imbrenda@linux.ibm.com>
> >   
> ?
> 

copy-paste error.... I meant:

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

[kvm-unit-tests PATCH v2] s390x: Ultavisor guest API test

[Janosch Frank]

Test the error conditions of guest 2 Ultravisor calls, namely:
     * Query Ultravisor information
     * Set shared access
     * Remove shared access

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 lib/s390x/asm/uv.h  |  68 +++++++++++++++++++
 s390x/Makefile      |   1 +
 s390x/unittests.cfg |   3 +
 s390x/uv-guest.c    | 158 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 230 insertions(+)
 create mode 100644 lib/s390x/asm/uv.h
 create mode 100644 s390x/uv-guest.c

diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
new file mode 100644
index 0000000..14ab5cc
--- /dev/null
+++ b/lib/s390x/asm/uv.h
@@ -0,0 +1,68 @@
+/*
+ * s390x Ultravisor related definitions
+ *
+ * Copyright (c) 2020 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 General Public License version 2.
+ */
+#ifndef UV_H
+#define UV_H
+
+#define UVC_RC_EXECUTED		0x0001
+#define UVC_RC_INV_CMD		0x0002
+#define UVC_RC_INV_STATE	0x0003
+#define UVC_RC_INV_LEN		0x0005
+#define UVC_RC_NO_RESUME	0x0007
+
+#define UVC_CMD_QUI			0x0001
+#define UVC_CMD_SET_SHARED_ACCESS	0x1000
+#define UVC_CMD_REMOVE_SHARED_ACCESS	0x1001
+
+/* Bits in installed uv calls */
+enum uv_cmds_inst {
+	BIT_UVC_CMD_QUI = 0,
+	BIT_UVC_CMD_SET_SHARED_ACCESS = 8,
+	BIT_UVC_CMD_REMOVE_SHARED_ACCESS = 9,
+};
+
+struct uv_cb_header {
+	u16 len;
+	u16 cmd;	/* Command Code */
+	u16 rc;		/* Response Code */
+	u16 rrc;	/* Return Reason Code */
+} __attribute__((packed))  __attribute__((aligned(8)));
+
+struct uv_cb_qui {
+	struct uv_cb_header header;
+	u64 reserved08;
+	u64 inst_calls_list[4];
+	u64 reserved30[15];
+} __attribute__((packed))  __attribute__((aligned(8)));
+
+struct uv_cb_share {
+	struct uv_cb_header header;
+	u64 reserved08[3];
+	u64 paddr;
+	u64 reserved28;
+} __attribute__((packed))  __attribute__((aligned(8)));
+
+static inline int uv_call(unsigned long r1, unsigned long r2)
+{
+	int cc;
+
+	asm volatile(
+		"0:	.insn rrf,0xB9A40000,%[r1],%[r2],0,0\n"
+		"		brc	3,0b\n"
+		"		ipm	%[cc]\n"
+		"		srl	%[cc],28\n"
+		: [cc] "=d" (cc)
+		: [r1] "a" (r1), [r2] "a" (r2)
+		: "memory", "cc");
+	return cc;
+}
+
+#endif
diff --git a/s390x/Makefile b/s390x/Makefile
index 0f54bf4..c2213ad 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -18,6 +18,7 @@ tests += $(TEST_DIR)/skrf.elf
 tests += $(TEST_DIR)/smp.elf
 tests += $(TEST_DIR)/sclp.elf
 tests += $(TEST_DIR)/css.elf
+tests += $(TEST_DIR)/uv-guest.elf
 tests_binary = $(patsubst %.elf,%.bin,$(tests))
 
 all: directories test_cases test_cases_binary
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index b35269b..38c3257 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -92,3 +92,6 @@ extra_params = -device virtio-net-ccw
 [skrf]
 file = skrf.elf
 smp = 2
+
+[uv-guest]
+file = uv-guest.elf
\ No newline at end of file
diff --git a/s390x/uv-guest.c b/s390x/uv-guest.c
new file mode 100644
index 0000000..079e2a3
--- /dev/null
+++ b/s390x/uv-guest.c
@@ -0,0 +1,158 @@
+/*
+ * Guest Ultravisor Call tests
+ *
+ * Copyright (c) 2020 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 General Public License version 2.
+ */
+
+#include <libcflat.h>
+#include <alloc_page.h>
+#include <asm/page.h>
+#include <asm/asm-offsets.h>
+#include <asm/interrupt.h>
+#include <asm/facility.h>
+#include <asm/uv.h>
+
+static unsigned long page;
+
+static inline int share(unsigned long addr, u16 cmd)
+{
+	struct uv_cb_share uvcb = {
+		.header.cmd = cmd,
+		.header.len = sizeof(uvcb),
+		.paddr = addr
+	};
+
+	uv_call(0, (u64)&uvcb);
+	return uvcb.header.rc;
+}
+
+static inline int uv_set_shared(unsigned long addr)
+{
+	return share(addr, UVC_CMD_SET_SHARED_ACCESS);
+}
+
+static inline int uv_remove_shared(unsigned long addr)
+{
+	return share(addr, UVC_CMD_REMOVE_SHARED_ACCESS);
+}
+
+static void test_priv(void)
+{
+	struct uv_cb_header uvcb = {};
+
+	report_prefix_push("privileged");
+
+	report_prefix_push("query");
+	expect_pgm_int();
+	uvcb.cmd = UVC_CMD_QUI;
+	uvcb.len = sizeof(struct uv_cb_qui);
+	enter_pstate();
+	uv_call(0, (u64)&uvcb);
+	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
+	report_prefix_pop();
+
+	report_prefix_push("share");
+	expect_pgm_int();
+	enter_pstate();
+	uv_set_shared((unsigned long)page);
+	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
+	report_prefix_pop();
+
+	report_prefix_push("unshare");
+	expect_pgm_int();
+	enter_pstate();
+	uv_remove_shared((unsigned long)page);
+	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
+	report_prefix_pop();
+
+	report_prefix_pop();
+}
+
+static void test_query(void)
+{
+	struct uv_cb_qui uvcb = {
+		.header.cmd = UVC_CMD_QUI,
+		.header.len = sizeof(uvcb) - 8,
+	};
+
+	report_prefix_push("query");
+	uv_call(0, (u64)&uvcb);
+	report(uvcb.header.rc == UVC_RC_INV_LEN, "length");
+
+	uvcb.header.len = sizeof(uvcb);
+	uv_call(0, (u64)&uvcb);
+	report(uvcb.header.rc == UVC_RC_EXECUTED, "successful query");
+
+	/*
+	 * These bits have been introduced with the very first
+	 * Ultravisor version and are expected to always be available
+	 * because they are basic building blocks.
+	 */
+	report(uvcb.inst_calls_list[0] & (1UL << (63 - BIT_UVC_CMD_QUI)),
+	       "query indicated");
+	report(uvcb.inst_calls_list[0] & (1UL << (63 - BIT_UVC_CMD_SET_SHARED_ACCESS)),
+	       "share indicated");
+	report(uvcb.inst_calls_list[0] & (1UL << (63 - BIT_UVC_CMD_REMOVE_SHARED_ACCESS)),
+	       "unshare indicated");
+	report_prefix_pop();
+}
+
+static void test_sharing(void)
+{
+	struct uv_cb_share uvcb = {
+		.header.cmd = UVC_CMD_SET_SHARED_ACCESS,
+		.header.len = sizeof(uvcb) - 8,
+	};
+
+	report_prefix_push("share");
+	uv_call(0, (u64)&uvcb);
+	report(uvcb.header.rc == UVC_RC_INV_LEN, "length");
+	report(uv_set_shared(page) == UVC_RC_EXECUTED, "share");
+	report_prefix_pop();
+
+	report_prefix_push("unshare");
+	uvcb.header.cmd = UVC_CMD_REMOVE_SHARED_ACCESS;
+	uv_call(0, (u64)&uvcb);
+	report(uvcb.header.rc == UVC_RC_INV_LEN, "length");
+	report(uv_remove_shared(page) == UVC_RC_EXECUTED, "unshare");
+	report_prefix_pop();
+
+	report_prefix_pop();
+}
+
+static void test_invalid(void)
+{
+	struct uv_cb_header uvcb = {
+		.len = 16,
+		.cmd = 0x4242,
+	};
+
+	uv_call(0, (u64)&uvcb);
+	report(uvcb.rc == UVC_RC_INV_CMD, "invalid command");
+}
+
+int main(void)
+{
+	bool has_uvc = test_facility(158);
+
+	report_prefix_push("uvc");
+	if (!has_uvc) {
+		report_skip("Ultravisor call facility is not available");
+		goto done;
+	}
+
+	page = (unsigned long)alloc_page();
+	test_priv();
+	test_invalid();
+	test_query();
+	test_sharing();
+	free_page((void *)page);
+done:
+	return report_summary();
+}
-- 
2.25.1

Re: [kvm-unit-tests PATCH 1/3] s390x: Add custom pgm cleanup function

[Thomas Huth]

On 17/07/2020 16.58, Janosch Frank wrote:
> Sometimes we need to do cleanup which we don't necessarily want to add
> to interrupt.c, so lets add a way to register a cleanup function.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/asm/interrupt.h | 1 +
>  lib/s390x/interrupt.c     | 9 +++++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
> index 4cfade9..b2a7c83 100644
> --- a/lib/s390x/asm/interrupt.h
> +++ b/lib/s390x/asm/interrupt.h
> @@ -15,6 +15,7 @@
>  #define EXT_IRQ_EXTERNAL_CALL	0x1202
>  #define EXT_IRQ_SERVICE_SIG	0x2401
>  
> +void register_pgm_int_func(void (*f)(void));
>  void handle_pgm_int(void);
>  void handle_ext_int(void);
>  void handle_mcck_int(void);
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index 243b9c2..36ba720 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -16,6 +16,7 @@
>  
>  static bool pgm_int_expected;
>  static bool ext_int_expected;
> +static void (*pgm_int_func)(void);
>  static struct lowcore *lc;
>  
>  void expect_pgm_int(void)
> @@ -51,8 +52,16 @@ void check_pgm_int_code(uint16_t code)
>  	       lc->pgm_int_code);
>  }
>  
> +void register_pgm_int_func(void (*f)(void))
> +{
> +	pgm_int_func = f;
> +}
> +
>  static void fixup_pgm_int(void)
>  {
> +	if (pgm_int_func)
> +		return (*pgm_int_func)();
> +
>  	switch (lc->pgm_int_code) {
>  	case PGM_INT_CODE_PRIVILEGED_OPERATION:
>  		/* Normal operation is in supervisor state, so this exception
> 

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

Re: [kvm-unit-tests PATCH 2/3] s390x: skrf: Add exception new skey test and add test to unittests.cfg

[Thomas Huth]

On 17/07/2020 16.58, Janosch Frank wrote:
> If a exception new psw mask contains a key a specification exception
> instead of a special operation exception is presented.

I have troubles parsing that sentence... could you write that differently?
(and: "s/a exception/an exception/")

> Let's test
> that.
> 
> Also let's add the test to unittests.cfg so it is run more often.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/skrf.c        | 81 +++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg |  4 +++
>  2 files changed, 85 insertions(+)
> 
> diff --git a/s390x/skrf.c b/s390x/skrf.c
> index 9cae589..9733412 100644
> --- a/s390x/skrf.c
> +++ b/s390x/skrf.c
> @@ -15,6 +15,8 @@
>  #include <asm/page.h>
>  #include <asm/facility.h>
>  #include <asm/mem.h>
> +#include <asm/sigp.h>
> +#include <smp.h>
>  
>  static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
>  
> @@ -106,6 +108,84 @@ static void test_tprot(void)
>  	report_prefix_pop();
>  }
>  
> +#include <asm-generic/barrier.h>

Can we keep the #includes at the top of the file, please?

> +static int testflag = 0;
> +
> +static void wait_for_flag(void)
> +{
> +	while (!testflag)
> +		mb();
> +}
> +
> +static void set_flag(int val)
> +{
> +	mb();
> +	testflag = val;
> +	mb();
> +}
> +
> +static void ecall_cleanup(void)
> +{
> +	struct lowcore *lc = (void *)0x0;
> +
> +	lc->ext_new_psw.mask = 0x0000000180000000UL;

Don't we have defines for the PSW values yet?

> +	lc->sw_int_crs[0] = 0x0000000000040000;
> +
> +	/*
> +	 * PGM old contains the ext new PSW, we need to clean it up,
> +	 * so we don't get a special oepration exception on the lpswe

operation

> +	 * of pgm old.
> +	 */
> +	lc->pgm_old_psw.mask = 0x0000000180000000UL;
> +	lc->pgm_old_psw.addr = (unsigned long)wait_for_flag;
> +
> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	set_flag(1);
> +}

 Thomas

Re: [kvm-unit-tests PATCH 3/3] s390x: Ultavisor guest API test

[Thomas Huth]

On 17/07/2020 16.58, Janosch Frank wrote:
> Test the error conditions of guest 2 Ultravisor calls, namely:
>      * Query Ultravisor information
>      * Set shared access
>      * Remove shared access
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/asm/uv.h  |  68 +++++++++++++++++++
>  s390x/Makefile      |   1 +
>  s390x/unittests.cfg |   3 +
>  s390x/uv-guest.c    | 156 ++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 228 insertions(+)
>  create mode 100644 lib/s390x/asm/uv.h
>  create mode 100644 s390x/uv-guest.c
[...]
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index b35269b..38c3257 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -92,3 +92,6 @@ extra_params = -device virtio-net-ccw
>  [skrf]
>  file = skrf.elf
>  smp = 2
> +
> +[uv-guest]
> +file = uv-guest.elf
> \ No newline at end of file

Add the newline, please.

 Thomas

Re: [kvm-unit-tests PATCH v2] s390x: Ultavisor guest API test

[Thomas Huth]

On 20/07/2020 15.35, Janosch Frank wrote:
> Test the error conditions of guest 2 Ultravisor calls, namely:
>      * Query Ultravisor information
>      * Set shared access
>      * Remove shared access
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>  lib/s390x/asm/uv.h  |  68 +++++++++++++++++++
>  s390x/Makefile      |   1 +
>  s390x/unittests.cfg |   3 +
>  s390x/uv-guest.c    | 158 ++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 230 insertions(+)
>  create mode 100644 lib/s390x/asm/uv.h
>  create mode 100644 s390x/uv-guest.c
[...]
> +int main(void)
> +{
> +	bool has_uvc = test_facility(158);
> +
> +	report_prefix_push("uvc");
> +	if (!has_uvc) {
> +		report_skip("Ultravisor call facility is not available");
> +		goto done;
> +	}
> +
> +	page = (unsigned long)alloc_page();
> +	test_priv();
> +	test_invalid();
> +	test_query();
> +	test_sharing();
> +	free_page((void *)page);
> +done:

It likely does not matter much, but for cleanliness, please add a
report_prefix_pop() here.

> +	return report_summary();
> +}
> 

 Thomas

Re: [kvm-unit-tests PATCH 2/3] s390x: skrf: Add exception new skey test and add test to unittests.cfg

[Janosch Frank]

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

On 7/21/20 9:28 AM, Thomas Huth wrote:
> On 17/07/2020 16.58, Janosch Frank wrote:
>> If a exception new psw mask contains a key a specification exception
>> instead of a special operation exception is presented.
> 
> I have troubles parsing that sentence... could you write that differently?
> (and: "s/a exception/an exception/")

How about:

When an exception psw new with a storage key in its mask is loaded from
lowcore a specification exception is raised instead of the special
operation exception that is normally presented when skrf is active.

> 
>> Let's test
>> that.
>>
>> Also let's add the test to unittests.cfg so it is run more often.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  s390x/skrf.c        | 81 +++++++++++++++++++++++++++++++++++++++++++++
>>  s390x/unittests.cfg |  4 +++
>>  2 files changed, 85 insertions(+)
>>
>> diff --git a/s390x/skrf.c b/s390x/skrf.c
>> index 9cae589..9733412 100644
>> --- a/s390x/skrf.c
>> +++ b/s390x/skrf.c
>> @@ -15,6 +15,8 @@
>>  #include <asm/page.h>
>>  #include <asm/facility.h>
>>  #include <asm/mem.h>
>> +#include <asm/sigp.h>
>> +#include <smp.h>
>>  
>>  static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
>>  
>> @@ -106,6 +108,84 @@ static void test_tprot(void)
>>  	report_prefix_pop();
>>  }
>>  
>> +#include <asm-generic/barrier.h>
> 
> Can we keep the #includes at the top of the file, please?

Yes

> 
>> +static int testflag = 0;
>> +
>> +static void wait_for_flag(void)
>> +{
>> +	while (!testflag)
>> +		mb();
>> +}
>> +
>> +static void set_flag(int val)
>> +{
>> +	mb();
>> +	testflag = val;
>> +	mb();
>> +}
>> +
>> +static void ecall_cleanup(void)
>> +{
>> +	struct lowcore *lc = (void *)0x0;
>> +
>> +	lc->ext_new_psw.mask = 0x0000000180000000UL;
> 
> Don't we have defines for the PSW values yet?

Pierre dropped that patch because of your old binutils version...

> 
>> +	lc->sw_int_crs[0] = 0x0000000000040000;
>> +
>> +	/*
>> +	 * PGM old contains the ext new PSW, we need to clean it up,
>> +	 * so we don't get a special oepration exception on the lpswe
> 
> operation

fixed

> 
>> +	 * of pgm old.
>> +	 */
>> +	lc->pgm_old_psw.mask = 0x0000000180000000UL;
>> +	lc->pgm_old_psw.addr = (unsigned long)wait_for_flag;
>> +
>> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>> +	set_flag(1);
>> +}
> 
>  Thomas
> 



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

Re: [kvm-unit-tests PATCH 3/3] s390x: Ultavisor guest API test

[Claudio Imbrenda]

btw, s/Ultavisor/Ultravisor/ in the patch title (the R is missing!)

On Fri, 17 Jul 2020 10:58:13 -0400
Janosch Frank <frankja@linux.ibm.com> wrote:

> Test the error conditions of guest 2 Ultravisor calls, namely:
>      * Query Ultravisor information
>      * Set shared access
>      * Remove shared access
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/asm/uv.h  |  68 +++++++++++++++++++
>  s390x/Makefile      |   1 +
>  s390x/unittests.cfg |   3 +
>  s390x/uv-guest.c    | 156
> ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 228
> insertions(+) create mode 100644 lib/s390x/asm/uv.h
>  create mode 100644 s390x/uv-guest.c
> 
> diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
> new file mode 100644
> index 0000000..14ab5cc
> --- /dev/null
> +++ b/lib/s390x/asm/uv.h
> @@ -0,0 +1,68 @@
> +/*
> + * s390x Ultravisor related definitions
> + *
> + * Copyright (c) 2020 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 General Public License version 2.
> + */
> +#ifndef UV_H
> +#define UV_H
> +
> +#define UVC_RC_EXECUTED		0x0001
> +#define UVC_RC_INV_CMD		0x0002
> +#define UVC_RC_INV_STATE	0x0003
> +#define UVC_RC_INV_LEN		0x0005
> +#define UVC_RC_NO_RESUME	0x0007
> +
> +#define UVC_CMD_QUI			0x0001
> +#define UVC_CMD_SET_SHARED_ACCESS	0x1000
> +#define UVC_CMD_REMOVE_SHARED_ACCESS	0x1001
> +
> +/* Bits in installed uv calls */
> +enum uv_cmds_inst {
> +	BIT_UVC_CMD_QUI = 0,
> +	BIT_UVC_CMD_SET_SHARED_ACCESS = 8,
> +	BIT_UVC_CMD_REMOVE_SHARED_ACCESS = 9,
> +};
> +
> +struct uv_cb_header {
> +	u16 len;
> +	u16 cmd;	/* Command Code */
> +	u16 rc;		/* Response Code */
> +	u16 rrc;	/* Return Reason Code */
> +} __attribute__((packed))  __attribute__((aligned(8)));
> +
> +struct uv_cb_qui {
> +	struct uv_cb_header header;
> +	u64 reserved08;
> +	u64 inst_calls_list[4];
> +	u64 reserved30[15];
> +} __attribute__((packed))  __attribute__((aligned(8)));
> +
> +struct uv_cb_share {
> +	struct uv_cb_header header;
> +	u64 reserved08[3];
> +	u64 paddr;
> +	u64 reserved28;
> +} __attribute__((packed))  __attribute__((aligned(8)));
> +
> +static inline int uv_call(unsigned long r1, unsigned long r2)
> +{
> +	int cc;
> +
> +	asm volatile(
> +		"0:	.insn rrf,0xB9A40000,%[r1],%[r2],0,0\n"
> +		"		brc	3,0b\n"
> +		"		ipm	%[cc]\n"
> +		"		srl	%[cc],28\n"
> +		: [cc] "=d" (cc)
> +		: [r1] "a" (r1), [r2] "a" (r2)
> +		: "memory", "cc");
> +	return cc;
> +}
> +
> +#endif
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 0f54bf4..c2213ad 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -18,6 +18,7 @@ tests += $(TEST_DIR)/skrf.elf
>  tests += $(TEST_DIR)/smp.elf
>  tests += $(TEST_DIR)/sclp.elf
>  tests += $(TEST_DIR)/css.elf
> +tests += $(TEST_DIR)/uv-guest.elf
>  tests_binary = $(patsubst %.elf,%.bin,$(tests))
>  
>  all: directories test_cases test_cases_binary
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index b35269b..38c3257 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -92,3 +92,6 @@ extra_params = -device virtio-net-ccw
>  [skrf]
>  file = skrf.elf
>  smp = 2
> +
> +[uv-guest]
> +file = uv-guest.elf
> \ No newline at end of file
> diff --git a/s390x/uv-guest.c b/s390x/uv-guest.c
> new file mode 100644
> index 0000000..0cb5fae
> --- /dev/null
> +++ b/s390x/uv-guest.c
> @@ -0,0 +1,156 @@
> +/*
> + * Guest Ultravisor Call tests
> + *
> + * Copyright (c) 2020 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 General Public License version 2.
> + */
> +
> +#include <libcflat.h>
> +#include <alloc_page.h>
> +#include <asm/page.h>
> +#include <asm/asm-offsets.h>
> +#include <asm/interrupt.h>
> +#include <asm/facility.h>
> +#include <asm/uv.h>
> +
> +static inline int share(unsigned long addr, u16 cmd)
> +{
> +	struct uv_cb_share uvcb = {
> +		.header.cmd = cmd,
> +		.header.len = sizeof(uvcb),
> +		.paddr = addr
> +	};
> +
> +	uv_call(0, (u64)&uvcb);
> +	return uvcb.header.rc;
> +}
> +
> +static inline int uv_set_shared(unsigned long addr)
> +{
> +	return share(addr, UVC_CMD_SET_SHARED_ACCESS);
> +}
> +
> +static inline int uv_remove_shared(unsigned long addr)
> +{
> +	return share(addr, UVC_CMD_REMOVE_SHARED_ACCESS);
> +}
> +
> +static void test_priv(void)
> +{
> +	struct uv_cb_header uvcb = {};
> +
> +	report_prefix_push("privileged");
> +
> +	report_prefix_push("query");
> +	expect_pgm_int();
> +	uvcb.cmd = UVC_CMD_QUI;
> +	uvcb.len = sizeof(struct uv_cb_qui);
> +	enter_pstate();
> +	uv_call(0, (u64)&uvcb);
> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("share");
> +	expect_pgm_int();
> +	enter_pstate();
> +	uv_set_shared(0x42000);
> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("unshare");
> +	expect_pgm_int();
> +	enter_pstate();
> +	uv_remove_shared(0x42000);
> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
> +	report_prefix_pop();
> +
> +	report_prefix_pop();
> +}
> +
> +static void test_query(void)
> +{
> +	struct uv_cb_qui uvcb = {
> +		.header.cmd = UVC_CMD_QUI,
> +		.header.len = sizeof(uvcb) - 8,
> +	};
> +
> +	report_prefix_push("query");
> +	uv_call(0, (u64)&uvcb);
> +	report(uvcb.header.rc == UVC_RC_INV_LEN, "length");
> +
> +	uvcb.header.len = sizeof(uvcb);
> +	uv_call(0, (u64)&uvcb);
> +	report(uvcb.header.rc == UVC_RC_EXECUTED, "successful
> query"); +
> +	/*
> +	 * These bits have been introduced with the very first
> +	 * Ultravisor version and are expected to always be available
> +	 * because they are basic building blocks.
> +	 */
> +	report(uvcb.inst_calls_list[0] & (1UL << (63 -
> BIT_UVC_CMD_QUI)),
> +	       "query indicated");
> +	report(uvcb.inst_calls_list[0] & (1UL << (63 -
> BIT_UVC_CMD_SET_SHARED_ACCESS)),
> +	       "share indicated");
> +	report(uvcb.inst_calls_list[0] & (1UL << (63 -
> BIT_UVC_CMD_REMOVE_SHARED_ACCESS)),
> +	       "unshare indicated");
> +	report_prefix_pop();
> +}
> +
> +static void test_sharing(void)
> +{
> +	unsigned long mem = (unsigned long)alloc_page();
> +	struct uv_cb_share uvcb = {
> +		.header.cmd = UVC_CMD_SET_SHARED_ACCESS,
> +		.header.len = sizeof(uvcb) - 8,
> +	};
> +
> +	report_prefix_push("share");
> +	uv_call(0, (u64)&uvcb);
> +	report(uvcb.header.rc == UVC_RC_INV_LEN, "length");
> +	report(uv_set_shared(mem) == UVC_RC_EXECUTED, "share");
> +	report_prefix_pop();
> +
> +	report_prefix_push("unshare");
> +	uvcb.header.cmd = UVC_CMD_REMOVE_SHARED_ACCESS;
> +	uv_call(0, (u64)&uvcb);
> +	report(uvcb.header.rc == UVC_RC_INV_LEN, "length");
> +	report(uv_remove_shared(mem) == UVC_RC_EXECUTED, "unshare");
> +	free_page((void *)mem);
> +	report_prefix_pop();
> +
> +	report_prefix_pop();
> +}
> +
> +static void test_invalid(void)
> +{
> +	struct uv_cb_header uvcb = {
> +		.len = 16,
> +		.cmd = 0x4242,
> +	};
> +
> +	uv_call(0, (u64)&uvcb);
> +	report(uvcb.rc == UVC_RC_INV_CMD, "invalid command");
> +}
> +
> +int main(void)
> +{
> +	bool has_uvc = test_facility(158);
> +
> +	report_prefix_push("uvc");
> +	if (!has_uvc) {
> +		report_skip("Ultravisor call facility is not
> available");
> +		goto done;
> +	}
> +
> +	test_priv();
> +	test_invalid();
> +	test_query();
> +	test_sharing();
> +done:
> +	return report_summary();
> +}

Re: [kvm-unit-tests PATCH 2/3] s390x: skrf: Add exception new skey test and add test to unittests.cfg

[Thomas Huth]

On 21/07/2020 10.52, Janosch Frank wrote:
> On 7/21/20 9:28 AM, Thomas Huth wrote:
>> On 17/07/2020 16.58, Janosch Frank wrote:
>>> If a exception new psw mask contains a key a specification exception
>>> instead of a special operation exception is presented.
>>
>> I have troubles parsing that sentence... could you write that differently?
>> (and: "s/a exception/an exception/")
> 
> How about:
> 
> When an exception psw new with a storage key in its mask is loaded from
> lowcore a specification exception is raised instead of the special
> operation exception that is normally presented when skrf is active.

Still a huge beast of a sentence. Could you maybe make two sentences out
of it? For example:

" ... is raised. This differs from the normal case where ..."

?

 Thomas

Re: [kvm-unit-tests PATCH 2/3] s390x: skrf: Add exception new skey test and add test to unittests.cfg

[Janosch Frank]

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

On 7/21/20 4:28 PM, Thomas Huth wrote:
> On 21/07/2020 10.52, Janosch Frank wrote:
>> On 7/21/20 9:28 AM, Thomas Huth wrote:
>>> On 17/07/2020 16.58, Janosch Frank wrote:
>>>> If a exception new psw mask contains a key a specification exception
>>>> instead of a special operation exception is presented.
>>>
>>> I have troubles parsing that sentence... could you write that differently?
>>> (and: "s/a exception/an exception/")
>>
>> How about:
>>
>> When an exception psw new with a storage key in its mask is loaded from
>> lowcore a specification exception is raised instead of the special
>> operation exception that is normally presented when skrf is active.
> 
> Still a huge beast of a sentence. Could you maybe make two sentences out
> of it? For example:
> 
> " ... is raised. This differs from the normal case where ..."

When an exception psw new with a storage key in its mask is loaded from
lowcore a specification exception is raised. This behavior differs from
the one that is presented when trying to execute skey related
instructions which will raise special operation exceptions.

> 
> ?
> 
>  Thomas
> 



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

Re: [kvm-unit-tests PATCH 1/3] s390x: Add custom pgm cleanup function

[Cornelia Huck]

On Fri, 17 Jul 2020 10:58:11 -0400
Janosch Frank <frankja@linux.ibm.com> wrote:

> Sometimes we need to do cleanup which we don't necessarily want to add
> to interrupt.c, so lets add a way to register a cleanup function.

s/lets/let's/ :)

> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/asm/interrupt.h | 1 +
>  lib/s390x/interrupt.c     | 9 +++++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
> index 4cfade9..b2a7c83 100644
> --- a/lib/s390x/asm/interrupt.h
> +++ b/lib/s390x/asm/interrupt.h
> @@ -15,6 +15,7 @@
>  #define EXT_IRQ_EXTERNAL_CALL	0x1202
>  #define EXT_IRQ_SERVICE_SIG	0x2401
>  
> +void register_pgm_int_func(void (*f)(void));
>  void handle_pgm_int(void);
>  void handle_ext_int(void);
>  void handle_mcck_int(void);
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index 243b9c2..36ba720 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -16,6 +16,7 @@
>  
>  static bool pgm_int_expected;
>  static bool ext_int_expected;
> +static void (*pgm_int_func)(void);
>  static struct lowcore *lc;
>  
>  void expect_pgm_int(void)
> @@ -51,8 +52,16 @@ void check_pgm_int_code(uint16_t code)
>  	       lc->pgm_int_code);
>  }
>  
> +void register_pgm_int_func(void (*f)(void))
> +{
> +	pgm_int_func = f;
> +}
> +
>  static void fixup_pgm_int(void)
>  {
> +	if (pgm_int_func)
> +		return (*pgm_int_func)();
> +

Maybe rather call this function, if set, instead of fixup_pgm_int() in
handle_pgm_int()? Feels a bit cleaner to me.
		
>  	switch (lc->pgm_int_code) {
>  	case PGM_INT_CODE_PRIVILEGED_OPERATION:
>  		/* Normal operation is in supervisor state, so this exception

Re: [kvm-unit-tests PATCH 2/3] s390x: skrf: Add exception new skey test and add test to unittests.cfg

[Cornelia Huck]

[-- Attachment #1: Type: text/plain, Size: 1483 bytes --]

On Tue, 21 Jul 2020 17:03:10 +0200
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 7/21/20 4:28 PM, Thomas Huth wrote:
> > On 21/07/2020 10.52, Janosch Frank wrote:  
> >> On 7/21/20 9:28 AM, Thomas Huth wrote:  
> >>> On 17/07/2020 16.58, Janosch Frank wrote:  
> >>>> If a exception new psw mask contains a key a specification exception
> >>>> instead of a special operation exception is presented.  
> >>>
> >>> I have troubles parsing that sentence... could you write that differently?
> >>> (and: "s/a exception/an exception/")  
> >>
> >> How about:
> >>
> >> When an exception psw new with a storage key in its mask is loaded from
> >> lowcore a specification exception is raised instead of the special
> >> operation exception that is normally presented when skrf is active.  
> > 
> > Still a huge beast of a sentence. Could you maybe make two sentences out
> > of it? For example:
> > 
> > " ... is raised. This differs from the normal case where ..."  
> 
> When an exception psw new with a storage key in its mask is loaded from
> lowcore a specification exception is raised. This behavior differs from
> the one that is presented when trying to execute skey related
> instructions which will raise special operation exceptions.

s/psw new/new psw/ ?

(And probably a comma after 'lowcore'.)

"This differs from the behaviour when trying to execute skey related
instructions, which will result in special operation exceptions."

?

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

Re: [kvm-unit-tests PATCH 2/3] s390x: skrf: Add exception new skey test and add test to unittests.cfg

[Janosch Frank]

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

On 7/23/20 2:10 PM, Cornelia Huck wrote:
> On Tue, 21 Jul 2020 17:03:10 +0200
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> On 7/21/20 4:28 PM, Thomas Huth wrote:
>>> On 21/07/2020 10.52, Janosch Frank wrote:  
>>>> On 7/21/20 9:28 AM, Thomas Huth wrote:  
>>>>> On 17/07/2020 16.58, Janosch Frank wrote:  
>>>>>> If a exception new psw mask contains a key a specification exception
>>>>>> instead of a special operation exception is presented.  
>>>>>
>>>>> I have troubles parsing that sentence... could you write that differently?
>>>>> (and: "s/a exception/an exception/")  
>>>>
>>>> How about:
>>>>
>>>> When an exception psw new with a storage key in its mask is loaded from
>>>> lowcore a specification exception is raised instead of the special
>>>> operation exception that is normally presented when skrf is active.  
>>>
>>> Still a huge beast of a sentence. Could you maybe make two sentences out
>>> of it? For example:
>>>
>>> " ... is raised. This differs from the normal case where ..."  
>>
>> When an exception psw new with a storage key in its mask is loaded from
>> lowcore a specification exception is raised. This behavior differs from
>> the one that is presented when trying to execute skey related
>> instructions which will raise special operation exceptions.
> 
> s/psw new/new psw/ ?

Yeah that would align the naming with the pop one.

> 
> (And probably a comma after 'lowcore'.)
> 
> "This differs from the behaviour when trying to execute skey related
> instructions, which will result in special operation exceptions."
> 
> ?

Sure



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

Re: [kvm-unit-tests PATCH 1/3] s390x: Add custom pgm cleanup function

[Janosch Frank]

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

On 7/23/20 2:01 PM, Cornelia Huck wrote:
> On Fri, 17 Jul 2020 10:58:11 -0400
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> Sometimes we need to do cleanup which we don't necessarily want to add
>> to interrupt.c, so lets add a way to register a cleanup function.
> 
> s/lets/let's/ :)
> 
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  lib/s390x/asm/interrupt.h | 1 +
>>  lib/s390x/interrupt.c     | 9 +++++++++
>>  2 files changed, 10 insertions(+)
>>
>> diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
>> index 4cfade9..b2a7c83 100644
>> --- a/lib/s390x/asm/interrupt.h
>> +++ b/lib/s390x/asm/interrupt.h
>> @@ -15,6 +15,7 @@
>>  #define EXT_IRQ_EXTERNAL_CALL	0x1202
>>  #define EXT_IRQ_SERVICE_SIG	0x2401
>>  
>> +void register_pgm_int_func(void (*f)(void));
>>  void handle_pgm_int(void);
>>  void handle_ext_int(void);
>>  void handle_mcck_int(void);
>> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
>> index 243b9c2..36ba720 100644
>> --- a/lib/s390x/interrupt.c
>> +++ b/lib/s390x/interrupt.c
>> @@ -16,6 +16,7 @@
>>  
>>  static bool pgm_int_expected;
>>  static bool ext_int_expected;
>> +static void (*pgm_int_func)(void);
>>  static struct lowcore *lc;
>>  
>>  void expect_pgm_int(void)
>> @@ -51,8 +52,16 @@ void check_pgm_int_code(uint16_t code)
>>  	       lc->pgm_int_code);
>>  }
>>  
>> +void register_pgm_int_func(void (*f)(void))
>> +{
>> +	pgm_int_func = f;
>> +}
>> +
>>  static void fixup_pgm_int(void)
>>  {
>> +	if (pgm_int_func)
>> +		return (*pgm_int_func)();
>> +
> 
> Maybe rather call this function, if set, instead of fixup_pgm_int() in
> handle_pgm_int()? Feels a bit cleaner to me.

Well it's currently a cleanup function so it should be in
fixup_pgm_int() because it fixes up.

I don't need a handler here like Pierre with his IO changes.

So it might more sense to change the name of the function ptr and
registration function:

register_pgm_cleanup_func()
static void (*pgm_cleanup_func)(void);

> 		
>>  	switch (lc->pgm_int_code) {
>>  	case PGM_INT_CODE_PRIVILEGED_OPERATION:
>>  		/* Normal operation is in supervisor state, so this exception
> 



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

Re: [kvm-unit-tests PATCH 1/3] s390x: Add custom pgm cleanup function

[Cornelia Huck]

[-- Attachment #1: Type: text/plain, Size: 2638 bytes --]

On Thu, 23 Jul 2020 14:23:48 +0200
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 7/23/20 2:01 PM, Cornelia Huck wrote:
> > On Fri, 17 Jul 2020 10:58:11 -0400
> > Janosch Frank <frankja@linux.ibm.com> wrote:
> >   
> >> Sometimes we need to do cleanup which we don't necessarily want to add
> >> to interrupt.c, so lets add a way to register a cleanup function.  
> > 
> > s/lets/let's/ :)
> >   
> >>
> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> >> ---
> >>  lib/s390x/asm/interrupt.h | 1 +
> >>  lib/s390x/interrupt.c     | 9 +++++++++
> >>  2 files changed, 10 insertions(+)
> >>
> >> diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
> >> index 4cfade9..b2a7c83 100644
> >> --- a/lib/s390x/asm/interrupt.h
> >> +++ b/lib/s390x/asm/interrupt.h
> >> @@ -15,6 +15,7 @@
> >>  #define EXT_IRQ_EXTERNAL_CALL	0x1202
> >>  #define EXT_IRQ_SERVICE_SIG	0x2401
> >>  
> >> +void register_pgm_int_func(void (*f)(void));
> >>  void handle_pgm_int(void);
> >>  void handle_ext_int(void);
> >>  void handle_mcck_int(void);
> >> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> >> index 243b9c2..36ba720 100644
> >> --- a/lib/s390x/interrupt.c
> >> +++ b/lib/s390x/interrupt.c
> >> @@ -16,6 +16,7 @@
> >>  
> >>  static bool pgm_int_expected;
> >>  static bool ext_int_expected;
> >> +static void (*pgm_int_func)(void);
> >>  static struct lowcore *lc;
> >>  
> >>  void expect_pgm_int(void)
> >> @@ -51,8 +52,16 @@ void check_pgm_int_code(uint16_t code)
> >>  	       lc->pgm_int_code);
> >>  }
> >>  
> >> +void register_pgm_int_func(void (*f)(void))
> >> +{
> >> +	pgm_int_func = f;
> >> +}
> >> +
> >>  static void fixup_pgm_int(void)
> >>  {
> >> +	if (pgm_int_func)
> >> +		return (*pgm_int_func)();
> >> +  
> > 
> > Maybe rather call this function, if set, instead of fixup_pgm_int() in
> > handle_pgm_int()? Feels a bit cleaner to me.  
> 
> Well it's currently a cleanup function so it should be in
> fixup_pgm_int() because it fixes up.
> 
> I don't need a handler here like Pierre with his IO changes.
> 
> So it might more sense to change the name of the function ptr and
> registration function:
> 
> register_pgm_cleanup_func()
> static void (*pgm_cleanup_func)(void);

Sounds good.

But doesn't that cleanup func run instead of the 'normal' cleanup func?
I think making that distinction in handle_pgm_int() is clearer.

> 
> > 		  
> >>  	switch (lc->pgm_int_code) {
> >>  	case PGM_INT_CODE_PRIVILEGED_OPERATION:
> >>  		/* Normal operation is in supervisor state, so this exception  
> >   
> 
> 


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