kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [kvm-unit-tests PATCH v2 0/3] PV tests part 1
@ 2020-08-07 11:15 Janosch Frank
  2020-08-07 11:15 ` [kvm-unit-tests PATCH v2 1/3] s390x: Add custom pgm cleanup function Janosch Frank
                   ` (3 more replies)
  0 siblings, 4 replies; 22+ messages in thread
From: Janosch Frank @ 2020-08-07 11:15 UTC (permalink / raw)
  To: kvm; +Cc: thuth, linux-s390, david, borntraeger, cohuck, imbrenda

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 is that if such a PSW is loaded on an
exception it will result in a specification exception and not a
special operation exception.

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://gitlab.com/frankja/kvm-unit-tests/-/tree/queue


v2:
	* Renamed pgm_int_func to pgm_cleanup_func() and moved the call to handle_pgm_int()
	* Added page allocation to UV test
	* Cleanups

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

 lib/s390x/asm/interrupt.h |   1 +
 lib/s390x/asm/uv.h        |  74 +++++++++++++++++
 lib/s390x/interrupt.c     |  12 ++-
 s390x/Makefile            |   1 +
 s390x/skrf.c              |  79 +++++++++++++++++++
 s390x/unittests.cfg       |   7 ++
 s390x/uv-guest.c          | 162 ++++++++++++++++++++++++++++++++++++++
 7 files changed, 335 insertions(+), 1 deletion(-)
 create mode 100644 lib/s390x/asm/uv.h
 create mode 100644 s390x/uv-guest.c

-- 
2.25.1


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [kvm-unit-tests PATCH v2 1/3] s390x: Add custom pgm cleanup function
  2020-08-07 11:15 [kvm-unit-tests PATCH v2 0/3] PV tests part 1 Janosch Frank
@ 2020-08-07 11:15 ` Janosch Frank
  2020-08-10 12:21   ` Cornelia Huck
  2020-08-07 11:15 ` [kvm-unit-tests PATCH v2 2/3] s390x: skrf: Add exception new skey test and add test to unittests.cfg Janosch Frank
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 22+ messages in thread
From: Janosch Frank @ 2020-08-07 11:15 UTC (permalink / raw)
  To: kvm; +Cc: thuth, linux-s390, david, borntraeger, cohuck, imbrenda

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

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 lib/s390x/asm/interrupt.h |  1 +
 lib/s390x/interrupt.c     | 12 +++++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
index 4cfade9..2772e6b 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_cleanup_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..a074505 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_cleanup_func)(void);
 static struct lowcore *lc;
 
 void expect_pgm_int(void)
@@ -51,6 +52,11 @@ void check_pgm_int_code(uint16_t code)
 	       lc->pgm_int_code);
 }
 
+void register_pgm_cleanup_func(void (*f)(void))
+{
+	pgm_cleanup_func = f;
+}
+
 static void fixup_pgm_int(void)
 {
 	switch (lc->pgm_int_code) {
@@ -115,7 +121,11 @@ void handle_pgm_int(void)
 	}
 
 	pgm_int_expected = false;
-	fixup_pgm_int();
+
+	if (pgm_cleanup_func)
+		(*pgm_cleanup_func)();
+	else
+		fixup_pgm_int();
 }
 
 void handle_ext_int(void)
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [kvm-unit-tests PATCH v2 2/3] s390x: skrf: Add exception new skey test and add test to unittests.cfg
  2020-08-07 11:15 [kvm-unit-tests PATCH v2 0/3] PV tests part 1 Janosch Frank
  2020-08-07 11:15 ` [kvm-unit-tests PATCH v2 1/3] s390x: Add custom pgm cleanup function Janosch Frank
@ 2020-08-07 11:15 ` Janosch Frank
  2020-08-07 12:12   ` Thomas Huth
  2020-08-10 14:38   ` Cornelia Huck
  2020-08-07 11:15 ` [kvm-unit-tests PATCH v2 3/3] s390x: Ultravisor guest API test Janosch Frank
  2020-08-11  7:29 ` [kvm-unit-tests PATCH v2 0/3] PV tests part 1 Janosch Frank
  3 siblings, 2 replies; 22+ messages in thread
From: Janosch Frank @ 2020-08-07 11:15 UTC (permalink / raw)
  To: kvm; +Cc: thuth, linux-s390, david, borntraeger, cohuck, imbrenda

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

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

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 s390x/skrf.c        | 79 +++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |  4 +++
 2 files changed, 83 insertions(+)

diff --git a/s390x/skrf.c b/s390x/skrf.c
index 9cae589..b19d0f4 100644
--- a/s390x/skrf.c
+++ b/s390x/skrf.c
@@ -11,12 +11,16 @@
  */
 #include <libcflat.h>
 #include <asm/asm-offsets.h>
+#include <asm-generic/barrier.h>
 #include <asm/interrupt.h>
 #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)));
+static int testflag = 0;
 
 static void test_facilities(void)
 {
@@ -106,6 +110,80 @@ static void test_tprot(void)
 	report_prefix_pop();
 }
 
+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 operation exception on the lpswe
+	 * of pgm old.
+	 */
+	lc->pgm_old_psw.mask = 0x0000000180000000UL;
+
+	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_cleanup_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 +199,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


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [kvm-unit-tests PATCH v2 3/3] s390x: Ultravisor guest API test
  2020-08-07 11:15 [kvm-unit-tests PATCH v2 0/3] PV tests part 1 Janosch Frank
  2020-08-07 11:15 ` [kvm-unit-tests PATCH v2 1/3] s390x: Add custom pgm cleanup function Janosch Frank
  2020-08-07 11:15 ` [kvm-unit-tests PATCH v2 2/3] s390x: skrf: Add exception new skey test and add test to unittests.cfg Janosch Frank
@ 2020-08-07 11:15 ` Janosch Frank
  2020-08-07 12:14   ` Thomas Huth
  2020-08-10 14:50   ` Cornelia Huck
  2020-08-11  7:29 ` [kvm-unit-tests PATCH v2 0/3] PV tests part 1 Janosch Frank
  3 siblings, 2 replies; 22+ messages in thread
From: Janosch Frank @ 2020-08-07 11:15 UTC (permalink / raw)
  To: kvm; +Cc: thuth, linux-s390, david, borntraeger, cohuck, imbrenda

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  |  74 ++++++++++++++++++++
 s390x/Makefile      |   1 +
 s390x/unittests.cfg |   3 +
 s390x/uv-guest.c    | 162 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 240 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..4c2fc48
--- /dev/null
+++ b/lib/s390x/asm/uv.h
@@ -0,0 +1,74 @@
+/*
+ * 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;
+
+	/*
+	 * The brc instruction will take care of the cc 2/3 case where
+	 * we need to continue the execution because we were
+	 * interrupted. The inline assembly will only return on
+	 * success/error i.e. cc 0/1.
+	*/
+	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..6d50c63 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
diff --git a/s390x/uv-guest.c b/s390x/uv-guest.c
new file mode 100644
index 0000000..1aaf7ca
--- /dev/null
+++ b/s390x/uv-guest.c
@@ -0,0 +1,162 @@
+/*
+ * 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,
+	};
+	int cc;
+
+	report_prefix_push("query");
+	cc = uv_call(0, (u64)&uvcb);
+	report(cc == 1 && uvcb.header.rc == UVC_RC_INV_LEN, "length");
+
+	uvcb.header.len = sizeof(uvcb);
+	cc = uv_call(0, (u64)&uvcb);
+	report(cc == 0 && 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,
+	};
+	int cc;
+
+	report_prefix_push("share");
+	cc = uv_call(0, (u64)&uvcb);
+	report(cc == 1 && 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;
+	cc = uv_call(0, (u64)&uvcb);
+	report(cc == 1 && 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,
+	};
+	int cc;
+
+	cc = uv_call(0, (u64)&uvcb);
+	report(cc == 1 && 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:
+	report_prefix_pop();
+	return report_summary();
+}
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [kvm-unit-tests PATCH v2 2/3] s390x: skrf: Add exception new skey test and add test to unittests.cfg
  2020-08-07 11:15 ` [kvm-unit-tests PATCH v2 2/3] s390x: skrf: Add exception new skey test and add test to unittests.cfg Janosch Frank
@ 2020-08-07 12:12   ` Thomas Huth
  2020-08-10 14:38   ` Cornelia Huck
  1 sibling, 0 replies; 22+ messages in thread
From: Thomas Huth @ 2020-08-07 12:12 UTC (permalink / raw)
  To: Janosch Frank, kvm; +Cc: linux-s390, david, borntraeger, cohuck, imbrenda

On 07/08/2020 13.15, Janosch Frank wrote:
> When an exception new psw with a storage key in its mask is loaded
> from lowcore, a specification exception is raised. This differs from
> the behavior when trying to execute skey related instructions, which
> will result in special operation exceptions.
> 
> Also let's add the test to unittests.cfg so it is run more often.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>  s390x/skrf.c        | 79 +++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg |  4 +++
>  2 files changed, 83 insertions(+)

It's hot here today, so my review skills might have melted a little bit,
but as far as I can see, it looks fine now:

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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [kvm-unit-tests PATCH v2 3/3] s390x: Ultravisor guest API test
  2020-08-07 11:15 ` [kvm-unit-tests PATCH v2 3/3] s390x: Ultravisor guest API test Janosch Frank
@ 2020-08-07 12:14   ` Thomas Huth
  2020-08-10 14:50   ` Cornelia Huck
  1 sibling, 0 replies; 22+ messages in thread
From: Thomas Huth @ 2020-08-07 12:14 UTC (permalink / raw)
  To: Janosch Frank, kvm; +Cc: linux-s390, david, borntraeger, cohuck, imbrenda

On 07/08/2020 13.15, 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  |  74 ++++++++++++++++++++
>  s390x/Makefile      |   1 +
>  s390x/unittests.cfg |   3 +
>  s390x/uv-guest.c    | 162 ++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 240 insertions(+)
>  create mode 100644 lib/s390x/asm/uv.h
>  create mode 100644 s390x/uv-guest.
Acked-by: Thomas Huth <thuth@redhat.com>


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [kvm-unit-tests PATCH v2 1/3] s390x: Add custom pgm cleanup function
  2020-08-07 11:15 ` [kvm-unit-tests PATCH v2 1/3] s390x: Add custom pgm cleanup function Janosch Frank
@ 2020-08-10 12:21   ` Cornelia Huck
  0 siblings, 0 replies; 22+ messages in thread
From: Cornelia Huck @ 2020-08-10 12:21 UTC (permalink / raw)
  To: Janosch Frank; +Cc: kvm, thuth, linux-s390, david, borntraeger, imbrenda

On Fri,  7 Aug 2020 07:15:53 -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 let's add a way to register a cleanup function.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>  lib/s390x/asm/interrupt.h |  1 +
>  lib/s390x/interrupt.c     | 12 +++++++++++-
>  2 files changed, 12 insertions(+), 1 deletion(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [kvm-unit-tests PATCH v2 2/3] s390x: skrf: Add exception new skey test and add test to unittests.cfg
  2020-08-07 11:15 ` [kvm-unit-tests PATCH v2 2/3] s390x: skrf: Add exception new skey test and add test to unittests.cfg Janosch Frank
  2020-08-07 12:12   ` Thomas Huth
@ 2020-08-10 14:38   ` Cornelia Huck
  1 sibling, 0 replies; 22+ messages in thread
From: Cornelia Huck @ 2020-08-10 14:38 UTC (permalink / raw)
  To: Janosch Frank; +Cc: kvm, thuth, linux-s390, david, borntraeger, imbrenda

On Fri,  7 Aug 2020 07:15:54 -0400
Janosch Frank <frankja@linux.ibm.com> wrote:

> When an exception new psw with a storage key in its mask is loaded
> from lowcore, a specification exception is raised. This differs from
> the behavior when trying to execute skey related instructions, which
> will result in special operation exceptions.
> 
> Also let's add the test to unittests.cfg so it is run more often.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>  s390x/skrf.c        | 79 +++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg |  4 +++
>  2 files changed, 83 insertions(+)

Didn't review deeply.

Acked-by: Cornelia Huck <cohuck@redhat.com>


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [kvm-unit-tests PATCH v2 3/3] s390x: Ultravisor guest API test
  2020-08-07 11:15 ` [kvm-unit-tests PATCH v2 3/3] s390x: Ultravisor guest API test Janosch Frank
  2020-08-07 12:14   ` Thomas Huth
@ 2020-08-10 14:50   ` Cornelia Huck
  2020-08-10 15:27     ` Janosch Frank
  1 sibling, 1 reply; 22+ messages in thread
From: Cornelia Huck @ 2020-08-10 14:50 UTC (permalink / raw)
  To: Janosch Frank; +Cc: kvm, thuth, linux-s390, david, borntraeger, imbrenda

On Fri,  7 Aug 2020 07:15:55 -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>
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>  lib/s390x/asm/uv.h  |  74 ++++++++++++++++++++
>  s390x/Makefile      |   1 +
>  s390x/unittests.cfg |   3 +
>  s390x/uv-guest.c    | 162 ++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 240 insertions(+)
>  create mode 100644 lib/s390x/asm/uv.h
>  create mode 100644 s390x/uv-guest.c

(...)

> +static inline int uv_call(unsigned long r1, unsigned long r2)
> +{
> +	int cc;
> +
> +	/*
> +	 * The brc instruction will take care of the cc 2/3 case where
> +	 * we need to continue the execution because we were
> +	 * interrupted. The inline assembly will only return on
> +	 * success/error i.e. cc 0/1.
> +	*/

Thanks, that is helpful.

> +	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/uv-guest.c b/s390x/uv-guest.c
> new file mode 100644
> index 0000000..1aaf7ca
> --- /dev/null
> +++ b/s390x/uv-guest.c
> @@ -0,0 +1,162 @@
> +/*
> + * 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;

Any reason why you're not checking rc and cc here...

> +}
> +
> +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_sharing(void)
> +{
> +	struct uv_cb_share uvcb = {
> +		.header.cmd = UVC_CMD_SET_SHARED_ACCESS,
> +		.header.len = sizeof(uvcb) - 8,
> +	};
> +	int cc;
> +
> +	report_prefix_push("share");
> +	cc = uv_call(0, (u64)&uvcb);
> +	report(cc == 1 && uvcb.header.rc == UVC_RC_INV_LEN, "length");

...while you do it for this command (as for all the others)?

> +	report(uv_set_shared(page) == UVC_RC_EXECUTED, "share");

So, is that one of the cases where something is actually indicated in
rc on success? Or does cc=0/1 have a different meaning for these calls?

> +	report_prefix_pop();
> +
> +	report_prefix_push("unshare");
> +	uvcb.header.cmd = UVC_CMD_REMOVE_SHARED_ACCESS;
> +	cc = uv_call(0, (u64)&uvcb);
> +	report(cc == 1 && uvcb.header.rc == UVC_RC_INV_LEN, "length");
> +	report(uv_remove_shared(page) == UVC_RC_EXECUTED, "unshare");
> +	report_prefix_pop();
> +
> +	report_prefix_pop();
> +}

(...)


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [kvm-unit-tests PATCH v2 3/3] s390x: Ultravisor guest API test
  2020-08-10 14:50   ` Cornelia Huck
@ 2020-08-10 15:27     ` Janosch Frank
  2020-08-10 15:32       ` Cornelia Huck
  0 siblings, 1 reply; 22+ messages in thread
From: Janosch Frank @ 2020-08-10 15:27 UTC (permalink / raw)
  To: Cornelia Huck; +Cc: kvm, thuth, linux-s390, david, borntraeger, imbrenda


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

On 8/10/20 4:50 PM, Cornelia Huck wrote:
> On Fri,  7 Aug 2020 07:15:55 -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>
>> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
>> ---
>>  lib/s390x/asm/uv.h  |  74 ++++++++++++++++++++
>>  s390x/Makefile      |   1 +
>>  s390x/unittests.cfg |   3 +
>>  s390x/uv-guest.c    | 162 ++++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 240 insertions(+)
>>  create mode 100644 lib/s390x/asm/uv.h
>>  create mode 100644 s390x/uv-guest.c
> 
> (...)
> 
>> +static inline int uv_call(unsigned long r1, unsigned long r2)
>> +{
>> +	int cc;
>> +
>> +	/*
>> +	 * The brc instruction will take care of the cc 2/3 case where
>> +	 * we need to continue the execution because we were
>> +	 * interrupted. The inline assembly will only return on
>> +	 * success/error i.e. cc 0/1.
>> +	*/
> 
> Thanks, that is helpful.
> 
>> +	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/uv-guest.c b/s390x/uv-guest.c
>> new file mode 100644
>> index 0000000..1aaf7ca
>> --- /dev/null
>> +++ b/s390x/uv-guest.c
>> @@ -0,0 +1,162 @@
>> +/*
>> + * 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;
> 
> Any reason why you're not checking rc and cc here...

Well, this is a helper function not a test function.
Since I can only return one value and since I'm lazy, I chose to ignore
the CC and went for the uvcb rc. That's basically also the answer for
your following questions.


Alright, I'll remove the helpers and execute those tests the hard way.

> 
>> +}
>> +
>> +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_sharing(void)
>> +{
>> +	struct uv_cb_share uvcb = {
>> +		.header.cmd = UVC_CMD_SET_SHARED_ACCESS,
>> +		.header.len = sizeof(uvcb) - 8,
>> +	};
>> +	int cc;
>> +
>> +	report_prefix_push("share");
>> +	cc = uv_call(0, (u64)&uvcb);
>> +	report(cc == 1 && uvcb.header.rc == UVC_RC_INV_LEN, "length");
> 
> ...while you do it for this command (as for all the others)?
> 
>> +	report(uv_set_shared(page) == UVC_RC_EXECUTED, "share");
> 
> So, is that one of the cases where something is actually indicated in
> rc on success? Or does cc=0/1 have a different meaning for these calls?
> 
>> +	report_prefix_pop();
>> +
>> +	report_prefix_push("unshare");
>> +	uvcb.header.cmd = UVC_CMD_REMOVE_SHARED_ACCESS;
>> +	cc = uv_call(0, (u64)&uvcb);
>> +	report(cc == 1 && uvcb.header.rc == UVC_RC_INV_LEN, "length");
>> +	report(uv_remove_shared(page) == UVC_RC_EXECUTED, "unshare");
>> +	report_prefix_pop();
>> +
>> +	report_prefix_pop();
>> +}
> 
> (...)
> 



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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [kvm-unit-tests PATCH v2 3/3] s390x: Ultravisor guest API test
  2020-08-10 15:27     ` Janosch Frank
@ 2020-08-10 15:32       ` Cornelia Huck
  2020-08-10 15:45         ` [kvm-unit-tests PATCH v3] " Janosch Frank
  0 siblings, 1 reply; 22+ messages in thread
From: Cornelia Huck @ 2020-08-10 15:32 UTC (permalink / raw)
  To: Janosch Frank; +Cc: kvm, thuth, linux-s390, david, borntraeger, imbrenda

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

On Mon, 10 Aug 2020 17:27:36 +0200
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 8/10/20 4:50 PM, Cornelia Huck wrote:
> > On Fri,  7 Aug 2020 07:15:55 -0400
> > Janosch Frank <frankja@linux.ibm.com> wrote:

> >> +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;  
> > 
> > Any reason why you're not checking rc and cc here...  
> 
> Well, this is a helper function not a test function.
> Since I can only return one value and since I'm lazy, I chose to ignore
> the CC and went for the uvcb rc. That's basically also the answer for
> your following questions.

Maybe I'm just confused regarding the command execution here.

> 
> 
> Alright, I'll remove the helpers and execute those tests the hard way.

As a plus point, you see exactly what is being done.

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [kvm-unit-tests PATCH v3] s390x: Ultravisor guest API test
  2020-08-10 15:32       ` Cornelia Huck
@ 2020-08-10 15:45         ` Janosch Frank
  2020-08-10 15:50           ` Cornelia Huck
  0 siblings, 1 reply; 22+ messages in thread
From: Janosch Frank @ 2020-08-10 15:45 UTC (permalink / raw)
  To: kvm; +Cc: thuth, linux-s390, david, borntraeger, cohuck, imbrenda

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>
Acked-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/asm/uv.h  |  74 ++++++++++++++++++++++
 s390x/Makefile      |   1 +
 s390x/unittests.cfg |   3 +
 s390x/uv-guest.c    | 150 ++++++++++++++++++++++++++++++++++++++++++++
 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..4c2fc48
--- /dev/null
+++ b/lib/s390x/asm/uv.h
@@ -0,0 +1,74 @@
+/*
+ * 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;
+
+	/*
+	 * The brc instruction will take care of the cc 2/3 case where
+	 * we need to continue the execution because we were
+	 * interrupted. The inline assembly will only return on
+	 * success/error i.e. cc 0/1.
+	*/
+	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..6d50c63 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
diff --git a/s390x/uv-guest.c b/s390x/uv-guest.c
new file mode 100644
index 0000000..d47333e
--- /dev/null
+++ b/s390x/uv-guest.c
@@ -0,0 +1,150 @@
+/*
+ * 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 void test_priv(void)
+{
+	struct uv_cb_header uvcb = {};
+
+	report_prefix_push("privileged");
+
+	report_prefix_push("query");
+	uvcb.cmd = UVC_CMD_QUI;
+	uvcb.len = sizeof(struct uv_cb_qui);
+	expect_pgm_int();
+	enter_pstate();
+	uv_call(0, (u64)&uvcb);
+	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
+	report_prefix_pop();
+
+	report_prefix_push("share");
+	uvcb.cmd = UVC_CMD_SET_SHARED_ACCESS;
+	uvcb.len = sizeof(struct uv_cb_share);
+	expect_pgm_int();
+	enter_pstate();
+	uv_call(0, (u64)&uvcb);
+	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
+	report_prefix_pop();
+
+	report_prefix_push("unshare");
+	uvcb.cmd = UVC_CMD_REMOVE_SHARED_ACCESS;
+	uvcb.len = sizeof(struct uv_cb_share);
+	expect_pgm_int();
+	enter_pstate();
+	uv_call(0, (u64)&uvcb);
+	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,
+	};
+	int cc;
+
+	report_prefix_push("query");
+	cc = uv_call(0, (u64)&uvcb);
+	report(cc == 1 && uvcb.header.rc == UVC_RC_INV_LEN, "length");
+
+	uvcb.header.len = sizeof(uvcb);
+	cc = uv_call(0, (u64)&uvcb);
+	report(cc == 0 && 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,
+		.paddr = page,
+	};
+	int cc;
+
+	report_prefix_push("share");
+	cc = uv_call(0, (u64)&uvcb);
+	report(cc == 1 && uvcb.header.rc == UVC_RC_INV_LEN, "length");
+	uvcb.header.len = sizeof(uvcb);
+	cc = uv_call(0, (u64)&uvcb);
+	report(cc == 0 && uvcb.header.rc == UVC_RC_EXECUTED, "share");
+	report_prefix_pop();
+
+	report_prefix_push("unshare");
+	uvcb.header.cmd = UVC_CMD_REMOVE_SHARED_ACCESS;
+	uvcb.header.len -= 8;
+	cc = uv_call(0, (u64)&uvcb);
+	report(cc == 1 && uvcb.header.rc == UVC_RC_INV_LEN, "length");
+	uvcb.header.len = sizeof(uvcb);
+	cc = uv_call(0, (u64)&uvcb);
+	report(cc == 0 && uvcb.header.rc == UVC_RC_EXECUTED, "unshare");
+	report_prefix_pop();
+
+	report_prefix_pop();
+}
+
+static void test_invalid(void)
+{
+	struct uv_cb_header uvcb = {
+		.len = 16,
+		.cmd = 0x4242,
+	};
+	int cc;
+
+	cc = uv_call(0, (u64)&uvcb);
+	report(cc == 1 && 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:
+	report_prefix_pop();
+	return report_summary();
+}
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [kvm-unit-tests PATCH v3] s390x: Ultravisor guest API test
  2020-08-10 15:45         ` [kvm-unit-tests PATCH v3] " Janosch Frank
@ 2020-08-10 15:50           ` Cornelia Huck
  2020-08-11  7:18             ` Janosch Frank
  0 siblings, 1 reply; 22+ messages in thread
From: Cornelia Huck @ 2020-08-10 15:50 UTC (permalink / raw)
  To: Janosch Frank; +Cc: kvm, thuth, linux-s390, david, borntraeger, imbrenda

On Mon, 10 Aug 2020 11:45:41 -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>
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Acked-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/asm/uv.h  |  74 ++++++++++++++++++++++
>  s390x/Makefile      |   1 +
>  s390x/unittests.cfg |   3 +
>  s390x/uv-guest.c    | 150 ++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 228 insertions(+)
>  create mode 100644 lib/s390x/asm/uv.h
>  create mode 100644 s390x/uv-guest.c

Acked-by: Cornelia Huck <cohuck@redhat.com>


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [kvm-unit-tests PATCH v3] s390x: Ultravisor guest API test
  2020-08-10 15:50           ` Cornelia Huck
@ 2020-08-11  7:18             ` Janosch Frank
  0 siblings, 0 replies; 22+ messages in thread
From: Janosch Frank @ 2020-08-11  7:18 UTC (permalink / raw)
  To: Cornelia Huck; +Cc: kvm, thuth, linux-s390, david, borntraeger, imbrenda


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

On 8/10/20 5:50 PM, Cornelia Huck wrote:
> On Mon, 10 Aug 2020 11:45:41 -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>
>> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
>> Acked-by: Thomas Huth <thuth@redhat.com>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  lib/s390x/asm/uv.h  |  74 ++++++++++++++++++++++
>>  s390x/Makefile      |   1 +
>>  s390x/unittests.cfg |   3 +
>>  s390x/uv-guest.c    | 150 ++++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 228 insertions(+)
>>  create mode 100644 lib/s390x/asm/uv.h
>>  create mode 100644 s390x/uv-guest.c
> 
> Acked-by: Cornelia Huck <cohuck@redhat.com>
> 
Thanks!


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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [kvm-unit-tests PATCH v2 0/3] PV tests part 1
  2020-08-07 11:15 [kvm-unit-tests PATCH v2 0/3] PV tests part 1 Janosch Frank
                   ` (2 preceding siblings ...)
  2020-08-07 11:15 ` [kvm-unit-tests PATCH v2 3/3] s390x: Ultravisor guest API test Janosch Frank
@ 2020-08-11  7:29 ` Janosch Frank
  3 siblings, 0 replies; 22+ messages in thread
From: Janosch Frank @ 2020-08-11  7:29 UTC (permalink / raw)
  To: kvm; +Cc: thuth, linux-s390, david, borntraeger, cohuck, imbrenda


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

On 8/7/20 1:15 PM, Janosch Frank wrote:
> 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 is that if such a PSW is loaded on an
> exception it will result in a specification exception and not a
> special operation exception.
> 
> 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://gitlab.com/frankja/kvm-unit-tests/-/tree/queue

Picked the series onto:
https://gitlab.com/frankja/kvm-unit-tests/-/commits/next

> 
> 
> v2:
> 	* Renamed pgm_int_func to pgm_cleanup_func() and moved the call to handle_pgm_int()
> 	* Added page allocation to UV test
> 	* Cleanups
> 
> Janosch Frank (3):
>   s390x: Add custom pgm cleanup function
>   s390x: skrf: Add exception new skey test and add test to unittests.cfg
>   s390x: Ultravisor guest API test
> 
>  lib/s390x/asm/interrupt.h |   1 +
>  lib/s390x/asm/uv.h        |  74 +++++++++++++++++
>  lib/s390x/interrupt.c     |  12 ++-
>  s390x/Makefile            |   1 +
>  s390x/skrf.c              |  79 +++++++++++++++++++
>  s390x/unittests.cfg       |   7 ++
>  s390x/uv-guest.c          | 162 ++++++++++++++++++++++++++++++++++++++
>  7 files changed, 335 insertions(+), 1 deletion(-)
>  create mode 100644 lib/s390x/asm/uv.h
>  create mode 100644 s390x/uv-guest.c
> 



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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [kvm-unit-tests PATCH v2 3/3] s390x: Ultravisor guest API test
  2020-07-31  9:06           ` Janosch Frank
@ 2020-07-31  9:21             ` Cornelia Huck
  0 siblings, 0 replies; 22+ messages in thread
From: Cornelia Huck @ 2020-07-31  9:21 UTC (permalink / raw)
  To: Janosch Frank; +Cc: Thomas Huth, kvm, linux-s390, david, borntraeger, imbrenda

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

On Fri, 31 Jul 2020 11:06:25 +0200
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 7/31/20 10:42 AM, Cornelia Huck wrote:
> > On Fri, 31 Jul 2020 09:34:41 +0200
> > Janosch Frank <frankja@linux.ibm.com> wrote:
> >   
> >> On 7/30/20 5:58 PM, Thomas Huth wrote:  
> >>> On 30/07/2020 13.16, Cornelia Huck wrote:    
> >>>> On Mon, 27 Jul 2020 05:54:15 -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>
> >>>>> 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    | 159 ++++++++++++++++++++++++++++++++++++++++++++
> >>>>>  4 files changed, 231 insertions(+)
> >>>>>  create mode 100644 lib/s390x/asm/uv.h
> >>>>>  create mode 100644 s390x/uv-guest.c
> >>>>>    
> >>>>
> >>>> (...)
> >>>>    
> >>>>> +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;
> >>>>> +}    
> >>>>
> >>>> This returns the condition code, but no caller seems to check it
> >>>> (instead, they look at header.rc, which is presumably only set if the
> >>>> instruction executed successfully in some way?)
> >>>>
> >>>> Looking at the kernel, it retries for cc > 1 (presumably busy
> >>>> conditions), and cc != 0 seems to be considered a failure. Do we want
> >>>> to look at the cc here as well?    
> >>>
> >>> It's there - but here it's in the assembly code, the "brc 3,0b".    
> > 
> > Ah yes, I missed that.
> >   
> >>
> >> Yes, we needed to factor that out in KVM because we sometimes need to
> >> schedule and then it looks nicer handling that in C code. The branch on
> >> condition will jump back for cc 2 and 3. cc 0 and 1 are success and
> >> error respectively and only then the rc and rrc in the UV header are set.  
> > 
> > Yeah, it's a bit surprising that rc/rrc are also set with cc 1.  
> 
> Is it?
> The (r)rc *only* contain meaningful information on CC 1.
> On CC 0 they will simply say everything is fine which CC 0 states
> already anyway.

I would consider "things worked" to actually be meaningful :)

(I've seen other instructions indicating different kinds of success.)

> 
> > 
> > (Can you add a comment? Just so that it is clear that callers never
> > need to check the cc, as rc/rrc already contain more information than
> > that.)  
> 
> I'd rather fix my test code and also check the CC.
> I did check it for my other UV tests so I've no idea why I didn't do it
> here...
> 
> 
> How about adding a comment for the cc 2/3 case?
> "The brc instruction will take care of the cc 2/3 case where we need to
> continue the execution because we were interrupted.
> The inline assembly will only return on success/error i.e. cc 0/1."

Sounds good.

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [kvm-unit-tests PATCH v2 3/3] s390x: Ultravisor guest API test
  2020-07-31  8:42         ` Cornelia Huck
@ 2020-07-31  9:06           ` Janosch Frank
  2020-07-31  9:21             ` Cornelia Huck
  0 siblings, 1 reply; 22+ messages in thread
From: Janosch Frank @ 2020-07-31  9:06 UTC (permalink / raw)
  To: Cornelia Huck; +Cc: Thomas Huth, kvm, linux-s390, david, borntraeger, imbrenda


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

On 7/31/20 10:42 AM, Cornelia Huck wrote:
> On Fri, 31 Jul 2020 09:34:41 +0200
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> On 7/30/20 5:58 PM, Thomas Huth wrote:
>>> On 30/07/2020 13.16, Cornelia Huck wrote:  
>>>> On Mon, 27 Jul 2020 05:54:15 -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>
>>>>> 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    | 159 ++++++++++++++++++++++++++++++++++++++++++++
>>>>>  4 files changed, 231 insertions(+)
>>>>>  create mode 100644 lib/s390x/asm/uv.h
>>>>>  create mode 100644 s390x/uv-guest.c
>>>>>  
>>>>
>>>> (...)
>>>>  
>>>>> +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;
>>>>> +}  
>>>>
>>>> This returns the condition code, but no caller seems to check it
>>>> (instead, they look at header.rc, which is presumably only set if the
>>>> instruction executed successfully in some way?)
>>>>
>>>> Looking at the kernel, it retries for cc > 1 (presumably busy
>>>> conditions), and cc != 0 seems to be considered a failure. Do we want
>>>> to look at the cc here as well?  
>>>
>>> It's there - but here it's in the assembly code, the "brc 3,0b".  
> 
> Ah yes, I missed that.
> 
>>
>> Yes, we needed to factor that out in KVM because we sometimes need to
>> schedule and then it looks nicer handling that in C code. The branch on
>> condition will jump back for cc 2 and 3. cc 0 and 1 are success and
>> error respectively and only then the rc and rrc in the UV header are set.
> 
> Yeah, it's a bit surprising that rc/rrc are also set with cc 1.

Is it?
The (r)rc *only* contain meaningful information on CC 1.
On CC 0 they will simply say everything is fine which CC 0 states
already anyway.

> 
> (Can you add a comment? Just so that it is clear that callers never
> need to check the cc, as rc/rrc already contain more information than
> that.)

I'd rather fix my test code and also check the CC.
I did check it for my other UV tests so I've no idea why I didn't do it
here...


How about adding a comment for the cc 2/3 case?
"The brc instruction will take care of the cc 2/3 case where we need to
continue the execution because we were interrupted.
The inline assembly will only return on success/error i.e. cc 0/1."

> 
>>
>>>
>>> Patch looks ok to me (but I didn't do a full review):
>>>
>>> Acked-by: Thomas Huth <thuth@redhat.com>
>>>   
>>
>>
> 



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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [kvm-unit-tests PATCH v2 3/3] s390x: Ultravisor guest API test
  2020-07-31  7:34       ` Janosch Frank
@ 2020-07-31  8:42         ` Cornelia Huck
  2020-07-31  9:06           ` Janosch Frank
  0 siblings, 1 reply; 22+ messages in thread
From: Cornelia Huck @ 2020-07-31  8:42 UTC (permalink / raw)
  To: Janosch Frank; +Cc: Thomas Huth, kvm, linux-s390, david, borntraeger, imbrenda

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

On Fri, 31 Jul 2020 09:34:41 +0200
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 7/30/20 5:58 PM, Thomas Huth wrote:
> > On 30/07/2020 13.16, Cornelia Huck wrote:  
> >> On Mon, 27 Jul 2020 05:54:15 -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>
> >>> 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    | 159 ++++++++++++++++++++++++++++++++++++++++++++
> >>>  4 files changed, 231 insertions(+)
> >>>  create mode 100644 lib/s390x/asm/uv.h
> >>>  create mode 100644 s390x/uv-guest.c
> >>>  
> >>
> >> (...)
> >>  
> >>> +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;
> >>> +}  
> >>
> >> This returns the condition code, but no caller seems to check it
> >> (instead, they look at header.rc, which is presumably only set if the
> >> instruction executed successfully in some way?)
> >>
> >> Looking at the kernel, it retries for cc > 1 (presumably busy
> >> conditions), and cc != 0 seems to be considered a failure. Do we want
> >> to look at the cc here as well?  
> > 
> > It's there - but here it's in the assembly code, the "brc 3,0b".  

Ah yes, I missed that.

> 
> Yes, we needed to factor that out in KVM because we sometimes need to
> schedule and then it looks nicer handling that in C code. The branch on
> condition will jump back for cc 2 and 3. cc 0 and 1 are success and
> error respectively and only then the rc and rrc in the UV header are set.

Yeah, it's a bit surprising that rc/rrc are also set with cc 1.

(Can you add a comment? Just so that it is clear that callers never
need to check the cc, as rc/rrc already contain more information than
that.)

> 
> > 
> > Patch looks ok to me (but I didn't do a full review):
> > 
> > Acked-by: Thomas Huth <thuth@redhat.com>
> >   
> 
> 


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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [kvm-unit-tests PATCH v2 3/3] s390x: Ultravisor guest API test
  2020-07-30 15:58     ` Thomas Huth
@ 2020-07-31  7:34       ` Janosch Frank
  2020-07-31  8:42         ` Cornelia Huck
  0 siblings, 1 reply; 22+ messages in thread
From: Janosch Frank @ 2020-07-31  7:34 UTC (permalink / raw)
  To: Thomas Huth, Cornelia Huck; +Cc: kvm, linux-s390, david, borntraeger, imbrenda


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

On 7/30/20 5:58 PM, Thomas Huth wrote:
> On 30/07/2020 13.16, Cornelia Huck wrote:
>> On Mon, 27 Jul 2020 05:54:15 -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>
>>> 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    | 159 ++++++++++++++++++++++++++++++++++++++++++++
>>>  4 files changed, 231 insertions(+)
>>>  create mode 100644 lib/s390x/asm/uv.h
>>>  create mode 100644 s390x/uv-guest.c
>>>
>>
>> (...)
>>
>>> +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;
>>> +}
>>
>> This returns the condition code, but no caller seems to check it
>> (instead, they look at header.rc, which is presumably only set if the
>> instruction executed successfully in some way?)
>>
>> Looking at the kernel, it retries for cc > 1 (presumably busy
>> conditions), and cc != 0 seems to be considered a failure. Do we want
>> to look at the cc here as well?
> 
> It's there - but here it's in the assembly code, the "brc 3,0b".

Yes, we needed to factor that out in KVM because we sometimes need to
schedule and then it looks nicer handling that in C code. The branch on
condition will jump back for cc 2 and 3. cc 0 and 1 are success and
error respectively and only then the rc and rrc in the UV header are set.

> 
> Patch looks ok to me (but I didn't do a full review):
> 
> Acked-by: Thomas Huth <thuth@redhat.com>
> 



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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [kvm-unit-tests PATCH v2 3/3] s390x: Ultravisor guest API test
  2020-07-30 11:16   ` Cornelia Huck
@ 2020-07-30 15:58     ` Thomas Huth
  2020-07-31  7:34       ` Janosch Frank
  0 siblings, 1 reply; 22+ messages in thread
From: Thomas Huth @ 2020-07-30 15:58 UTC (permalink / raw)
  To: Cornelia Huck, Janosch Frank
  Cc: kvm, linux-s390, david, borntraeger, imbrenda

On 30/07/2020 13.16, Cornelia Huck wrote:
> On Mon, 27 Jul 2020 05:54:15 -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>
>> 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    | 159 ++++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 231 insertions(+)
>>  create mode 100644 lib/s390x/asm/uv.h
>>  create mode 100644 s390x/uv-guest.c
>>
> 
> (...)
> 
>> +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;
>> +}
> 
> This returns the condition code, but no caller seems to check it
> (instead, they look at header.rc, which is presumably only set if the
> instruction executed successfully in some way?)
> 
> Looking at the kernel, it retries for cc > 1 (presumably busy
> conditions), and cc != 0 seems to be considered a failure. Do we want
> to look at the cc here as well?

It's there - but here it's in the assembly code, the "brc 3,0b".

Patch looks ok to me (but I didn't do a full review):

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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [kvm-unit-tests PATCH v2 3/3] s390x: Ultravisor guest API test
  2020-07-27  9:54 ` [kvm-unit-tests PATCH v2 3/3] s390x: Ultravisor guest API test Janosch Frank
@ 2020-07-30 11:16   ` Cornelia Huck
  2020-07-30 15:58     ` Thomas Huth
  0 siblings, 1 reply; 22+ messages in thread
From: Cornelia Huck @ 2020-07-30 11:16 UTC (permalink / raw)
  To: Janosch Frank; +Cc: kvm, thuth, linux-s390, david, borntraeger, imbrenda

On Mon, 27 Jul 2020 05:54:15 -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>
> 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    | 159 ++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 231 insertions(+)
>  create mode 100644 lib/s390x/asm/uv.h
>  create mode 100644 s390x/uv-guest.c
> 

(...)

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

This returns the condition code, but no caller seems to check it
(instead, they look at header.rc, which is presumably only set if the
instruction executed successfully in some way?)

Looking at the kernel, it retries for cc > 1 (presumably busy
conditions), and cc != 0 seems to be considered a failure. Do we want
to look at the cc here as well?

(...)


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [kvm-unit-tests PATCH v2 3/3] s390x: Ultravisor guest API test
  2020-07-27  9:54 Janosch Frank
@ 2020-07-27  9:54 ` Janosch Frank
  2020-07-30 11:16   ` Cornelia Huck
  0 siblings, 1 reply; 22+ messages in thread
From: Janosch Frank @ 2020-07-27  9:54 UTC (permalink / raw)
  To: kvm; +Cc: thuth, linux-s390, david, borntraeger, cohuck, imbrenda

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    | 159 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 231 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..6d50c63 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
diff --git a/s390x/uv-guest.c b/s390x/uv-guest.c
new file mode 100644
index 0000000..15720a9
--- /dev/null
+++ b/s390x/uv-guest.c
@@ -0,0 +1,159 @@
+/*
+ * 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:
+	report_prefix_pop();
+	return report_summary();
+}
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2020-08-11  7:31 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-07 11:15 [kvm-unit-tests PATCH v2 0/3] PV tests part 1 Janosch Frank
2020-08-07 11:15 ` [kvm-unit-tests PATCH v2 1/3] s390x: Add custom pgm cleanup function Janosch Frank
2020-08-10 12:21   ` Cornelia Huck
2020-08-07 11:15 ` [kvm-unit-tests PATCH v2 2/3] s390x: skrf: Add exception new skey test and add test to unittests.cfg Janosch Frank
2020-08-07 12:12   ` Thomas Huth
2020-08-10 14:38   ` Cornelia Huck
2020-08-07 11:15 ` [kvm-unit-tests PATCH v2 3/3] s390x: Ultravisor guest API test Janosch Frank
2020-08-07 12:14   ` Thomas Huth
2020-08-10 14:50   ` Cornelia Huck
2020-08-10 15:27     ` Janosch Frank
2020-08-10 15:32       ` Cornelia Huck
2020-08-10 15:45         ` [kvm-unit-tests PATCH v3] " Janosch Frank
2020-08-10 15:50           ` Cornelia Huck
2020-08-11  7:18             ` Janosch Frank
2020-08-11  7:29 ` [kvm-unit-tests PATCH v2 0/3] PV tests part 1 Janosch Frank
  -- strict thread matches above, loose matches on Subject: below --
2020-07-27  9:54 Janosch Frank
2020-07-27  9:54 ` [kvm-unit-tests PATCH v2 3/3] s390x: Ultravisor guest API test Janosch Frank
2020-07-30 11:16   ` Cornelia Huck
2020-07-30 15:58     ` Thomas Huth
2020-07-31  7:34       ` Janosch Frank
2020-07-31  8:42         ` Cornelia Huck
2020-07-31  9:06           ` Janosch Frank
2020-07-31  9:21             ` Cornelia Huck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).