KVM Archive on lore.kernel.org
 help / color / Atom feed
* [kvm-unit-tests PATCH v2 0/3] PV tests part 1
@ 2020-07-27  9:54 Janosch Frank
  2020-07-27  9:54 ` [kvm-unit-tests PATCH v2 1/3] s390x: Add custom pgm cleanup function Janosch Frank
                   ` (2 more replies)
  0 siblings, 3 replies; 19+ messages in thread
From: Janosch Frank @ 2020-07-27  9:54 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://github.com/frankjaa/kvm-unit-tests/tree/queue

v2:
	* Page alloc instead of static memory reservation
	* Moved pgm cleanup function call to pgm handler
	* Commit message changes


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        |  68 ++++++++++++++++
 lib/s390x/interrupt.c     |  10 +++
 s390x/Makefile            |   1 +
 s390x/skrf.c              |  80 +++++++++++++++++++
 s390x/unittests.cfg       |   7 ++
 s390x/uv-guest.c          | 159 ++++++++++++++++++++++++++++++++++++++
 7 files changed, 326 insertions(+)
 create mode 100644 lib/s390x/asm/uv.h
 create mode 100644 s390x/uv-guest.c

-- 
2.25.1


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

* [kvm-unit-tests PATCH v2 1/3] s390x: Add custom pgm cleanup function
  2020-07-27  9:54 [kvm-unit-tests PATCH v2 0/3] PV tests part 1 Janosch Frank
@ 2020-07-27  9:54 ` Janosch Frank
  2020-07-30 10:53   ` Cornelia Huck
  2020-07-27  9:54 ` [kvm-unit-tests PATCH v2 2/3] s390x: skrf: Add exception new skey test and add test to unittests.cfg Janosch Frank
  2020-07-27  9:54 ` [kvm-unit-tests PATCH v2 3/3] s390x: Ultravisor guest API test Janosch Frank
  2 siblings, 1 reply; 19+ messages in thread
From: Janosch Frank @ 2020-07-27  9:54 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 lets 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     | 10 ++++++++++
 2 files changed, 11 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..51cc733 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,6 +52,11 @@ 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)
 {
 	switch (lc->pgm_int_code) {
@@ -115,6 +121,10 @@ void handle_pgm_int(void)
 	}
 
 	pgm_int_expected = false;
+
+	if (pgm_int_func)
+		return (*pgm_int_func)();
+
 	fixup_pgm_int();
 }
 
-- 
2.25.1


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

* [kvm-unit-tests PATCH v2 2/3] s390x: skrf: Add exception new skey test and add test to unittests.cfg
  2020-07-27  9:54 [kvm-unit-tests PATCH v2 0/3] PV tests part 1 Janosch Frank
  2020-07-27  9:54 ` [kvm-unit-tests PATCH v2 1/3] s390x: Add custom pgm cleanup function Janosch Frank
@ 2020-07-27  9:54 ` Janosch Frank
  2020-07-27 12:20   ` Thomas Huth
  2020-07-30 11:03   ` Cornelia Huck
  2020-07-27  9:54 ` [kvm-unit-tests PATCH v2 3/3] s390x: Ultravisor guest API test Janosch Frank
  2 siblings, 2 replies; 19+ messages in thread
From: Janosch Frank @ 2020-07-27  9:54 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 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        | 80 +++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |  4 +++
 2 files changed, 84 insertions(+)

diff --git a/s390x/skrf.c b/s390x/skrf.c
index 9cae589..fe78711 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,81 @@ 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;
+	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 +200,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	[flat|nested] 19+ messages in thread

* [kvm-unit-tests PATCH v2 3/3] s390x: Ultravisor guest API test
  2020-07-27  9:54 [kvm-unit-tests PATCH v2 0/3] PV tests part 1 Janosch Frank
  2020-07-27  9:54 ` [kvm-unit-tests PATCH v2 1/3] s390x: Add custom pgm cleanup function Janosch Frank
  2020-07-27  9:54 ` [kvm-unit-tests PATCH v2 2/3] s390x: skrf: Add exception new skey test and add test to unittests.cfg Janosch Frank
@ 2020-07-27  9:54 ` Janosch Frank
  2020-07-30 11:16   ` Cornelia Huck
  2 siblings, 1 reply; 19+ 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	[flat|nested] 19+ 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-07-27  9:54 ` [kvm-unit-tests PATCH v2 2/3] s390x: skrf: Add exception new skey test and add test to unittests.cfg Janosch Frank
@ 2020-07-27 12:20   ` Thomas Huth
  2020-07-27 12:38     ` Janosch Frank
  2020-07-30 11:03   ` Cornelia Huck
  1 sibling, 1 reply; 19+ messages in thread
From: Thomas Huth @ 2020-07-27 12:20 UTC (permalink / raw)
  To: Janosch Frank, kvm; +Cc: linux-s390, david, borntraeger, cohuck, imbrenda

On 27/07/2020 11.54, 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 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        | 80 +++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg |  4 +++
>  2 files changed, 84 insertions(+)
> 
> diff --git a/s390x/skrf.c b/s390x/skrf.c
> index 9cae589..fe78711 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,81 @@ 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;
> +	lc->pgm_old_psw.addr = (unsigned long)wait_for_flag;

I don't quite understand why you are using wait_for_flag here? Won't
that function return immediately due to the set_flag(1) below? And if it
returns, where does the cpu continue to exec code in that case? Wouldn't
it be better to leave the .addr unchanged, so that the CPU returns to
the endless loop in smp_cpu_setup_state ?

 Thomas


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


^ permalink raw reply	[flat|nested] 19+ 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-07-27 12:20   ` Thomas Huth
@ 2020-07-27 12:38     ` Janosch Frank
  0 siblings, 0 replies; 19+ messages in thread
From: Janosch Frank @ 2020-07-27 12:38 UTC (permalink / raw)
  To: Thomas Huth, kvm; +Cc: linux-s390, david, borntraeger, cohuck, imbrenda

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

On 7/27/20 2:20 PM, Thomas Huth wrote:
> On 27/07/2020 11.54, 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 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        | 80 +++++++++++++++++++++++++++++++++++++++++++++
>>  s390x/unittests.cfg |  4 +++
>>  2 files changed, 84 insertions(+)
>>
>> diff --git a/s390x/skrf.c b/s390x/skrf.c
>> index 9cae589..fe78711 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,81 @@ 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;
>> +	lc->pgm_old_psw.addr = (unsigned long)wait_for_flag;
> 
> I don't quite understand why you are using wait_for_flag here? Won't
> that function return immediately due to the set_flag(1) below? And if it
> returns, where does the cpu continue to exec code in that case? Wouldn't
> it be better to leave the .addr unchanged, so that the CPU returns to
> the endless loop in smp_cpu_setup_state ?

That's a valid point, will change

> 
>  Thomas
> 
> 
>> +	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 +200,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
>>
> 



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

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

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

On Mon, 27 Jul 2020 05:54:13 -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>
> 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     | 10 ++++++++++
>  2 files changed, 11 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..51cc733 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);

Hm, didn't you want to rename this to pgm_cleanup_func?

>  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_int_func(void (*f)(void))

...then, register_pgm_cleanup_func() would probably be a better name
here.

> +{
> +	pgm_int_func = f;
> +}
> +
>  static void fixup_pgm_int(void)
>  {
>  	switch (lc->pgm_int_code) {
> @@ -115,6 +121,10 @@ void handle_pgm_int(void)
>  	}
>  
>  	pgm_int_expected = false;
> +
> +	if (pgm_int_func)
> +		return (*pgm_int_func)();

I'd probably use an else branch, but this is fine as well.

> +
>  	fixup_pgm_int();
>  }
>  


^ permalink raw reply	[flat|nested] 19+ 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-07-27  9:54 ` [kvm-unit-tests PATCH v2 2/3] s390x: skrf: Add exception new skey test and add test to unittests.cfg Janosch Frank
  2020-07-27 12:20   ` Thomas Huth
@ 2020-07-30 11:03   ` Cornelia Huck
  1 sibling, 0 replies; 19+ messages in thread
From: Cornelia Huck @ 2020-07-30 11:03 UTC (permalink / raw)
  To: Janosch Frank; +Cc: kvm, thuth, linux-s390, david, borntraeger, imbrenda

On Mon, 27 Jul 2020 05:54:14 -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 unittests.cfg so it is run more often.

when you respin: s/add the test/add the test to/

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


^ permalink raw reply	[flat|nested] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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
  0 siblings, 0 replies; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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 ` Janosch Frank
  2020-08-07 12:14   ` Thomas Huth
  2020-08-10 14:50   ` Cornelia Huck
  0 siblings, 2 replies; 19+ 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	[flat|nested] 19+ messages in thread

end of thread, back to index

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-27  9:54 [kvm-unit-tests PATCH v2 0/3] PV tests part 1 Janosch Frank
2020-07-27  9:54 ` [kvm-unit-tests PATCH v2 1/3] s390x: Add custom pgm cleanup function Janosch Frank
2020-07-30 10:53   ` Cornelia Huck
2020-07-27  9:54 ` [kvm-unit-tests PATCH v2 2/3] s390x: skrf: Add exception new skey test and add test to unittests.cfg Janosch Frank
2020-07-27 12:20   ` Thomas Huth
2020-07-27 12:38     ` Janosch Frank
2020-07-30 11:03   ` Cornelia Huck
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
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 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

KVM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvm/0 kvm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kvm kvm/ https://lore.kernel.org/kvm \
		kvm@vger.kernel.org
	public-inbox-index kvm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.kvm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git