[kvm-unit-tests PATCH v1 0/9] s390x: Further extend instruction interception tests

[kvm-unit-tests PATCH v1 0/9] s390x: Further extend instruction interception tests

[Nico Boehr]

Further extend the instruction interception tests for s390x. This series focuses
on SIGP, STSI and TPROT instructions.

Some instructions such as STSI already had some coverage and the existing tests
were extended to increase coverage.

The SIGP instruction has coverage, but not for all orders. Orders
STORE_ADTL_STATUS, SET_PREFIX and CONDITIONAL_EMERGENCY didn't
have coverage before and new tests are added. For EMERGENCY_SIGNAL, the existing
test is extended to cover an invalid CPU address. Additionally, new tests are
added for quite a few invalid arguments to SIGP.

TPROT was used in skrf tests, but only for storage keys, so add tests for the
other uses as well.

Nico Boehr (9):
  s390x: smp: add tests for several invalid SIGP orders
  s390x: smp: stop already stopped CPU
  s390x: gs: move to new header file
  s390x: smp: add test for SIGP_STORE_ADTL_STATUS order
  s390x: smp: add tests for SET_PREFIX
  s390x: smp: add test for EMERGENCY_SIGNAL with invalid CPU address
  s390x: smp: add tests for CONDITIONAL EMERGENCY
  s390x: add TPROT tests
  s390x: stsi: check zero and ignored bits in r0 and r1

 lib/s390x/gs.h      |  80 ++++++++
 s390x/Makefile      |   1 +
 s390x/gs.c          |  65 +------
 s390x/smp.c         | 437 ++++++++++++++++++++++++++++++++++++++++++++
 s390x/stsi.c        |  42 ++++-
 s390x/tprot.c       | 108 +++++++++++
 s390x/unittests.cfg |   9 +
 7 files changed, 668 insertions(+), 74 deletions(-)
 create mode 100644 lib/s390x/gs.h
 create mode 100644 s390x/tprot.c

-- 
2.31.1

[kvm-unit-tests PATCH v1 1/9] s390x: smp: add tests for several invalid SIGP orders

[Nico Boehr]

Add tests with for intercepted SIGP orders with invalid values:

- SIGP_STOP with invalid CPU address
- SIGP_START with invalid CPU address
- SIGP_CPU_RESET with invalid CPU address
- SIGP_STOP_AND_STORE_STATUS with invalid CPU address
- SIGP_STORE_STATUS_AT_ADDRESS with invalid CPU address
- invalid order
- invalid order with invalid CPU address

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 s390x/smp.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/s390x/smp.c b/s390x/smp.c
index 068ac74dd28a..911a26c51b10 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -19,6 +19,47 @@
 #include <alloc_page.h>
 
 static int testflag = 0;
+#define INVALID_CPU_ADDRESS -4711
+#define INVALID_ORDER_CODE 0xFF
+struct sigp_invalid_cases {
+	int order;
+	char message[100];
+};
+static const struct sigp_invalid_cases cases_invalid_cpu_addr[] = {
+	{ SIGP_STOP,                  "stop with invalid CPU address" },
+	{ SIGP_START,                 "start with invalid CPU address" },
+	{ SIGP_CPU_RESET,             "reset with invalid CPU address" },
+	{ INVALID_ORDER_CODE,         "invalid order code and CPU address" },
+	{ SIGP_SENSE,                 "sense with invalid CPU address" },
+	{ SIGP_STOP_AND_STORE_STATUS, "stop and store status with invalid CPU address" },
+};
+static const struct sigp_invalid_cases cases_valid_cpu_addr[] = {
+	{ INVALID_ORDER_CODE,         "invalid order code" },
+};
+
+static void test_invalid(void)
+{
+	const struct sigp_invalid_cases *c;
+	uint32_t status;
+	int cc;
+	int i;
+
+	report_prefix_push("invalid parameters");
+
+	for (i = 0; i < ARRAY_SIZE(cases_invalid_cpu_addr); i++) {
+		c = &cases_invalid_cpu_addr[i];
+		cc = sigp(INVALID_CPU_ADDRESS, c->order, 0, &status);
+		report(cc == 3, c->message);
+	}
+
+	for (i = 0; i < ARRAY_SIZE(cases_valid_cpu_addr); i++) {
+		c = &cases_valid_cpu_addr[i];
+		cc = smp_sigp(1, c->order, 0, &status);
+		report(cc == 1, c->message);
+	}
+
+	report_prefix_pop();
+}
 
 static void wait_for_flag(void)
 {
@@ -123,10 +164,16 @@ static void test_store_status(void)
 {
 	struct cpu_status *status = alloc_pages_flags(1, AREA_DMA31);
 	uint32_t r;
+	int cc;
 
 	report_prefix_push("store status at address");
 	memset(status, 0, PAGE_SIZE * 2);
 
+	report_prefix_push("invalid CPU address");
+	cc = sigp(INVALID_CPU_ADDRESS, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, &r);
+	report(cc == 3, "returned with CC = 3");
+	report_prefix_pop();
+
 	report_prefix_push("running");
 	smp_cpu_restart(1);
 	smp_sigp(1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, &r);
@@ -331,6 +378,7 @@ int main(void)
 	smp_cpu_stop(1);
 
 	test_start();
+	test_invalid();
 	test_restart();
 	test_stop();
 	test_stop_store_status();
-- 
2.31.1

[kvm-unit-tests PATCH v1 2/9] s390x: smp: stop already stopped CPU

[Nico Boehr]

As per the PoP, the SIGP STOP order is only effective when the CPU is in
the operating state. Hence, we should have a test which tries to stop an
already stopped CPU.

Even though the SIGP order might be processed asynchronously, we assert
the CPU stays stopped.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/smp.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/s390x/smp.c b/s390x/smp.c
index 911a26c51b10..e5a16eb5a46a 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -129,6 +129,11 @@ static void test_stop(void)
 	 */
 	while (!smp_cpu_stopped(1)) {}
 	report_pass("stop");
+
+	report_prefix_push("stop stopped CPU");
+	report(!smp_cpu_stop(1), "STOP succeeds");
+	report(smp_cpu_stopped(1), "CPU is stopped");
+	report_prefix_pop();
 }
 
 static void test_stop_store_status(void)
-- 
2.31.1

[kvm-unit-tests PATCH v1 3/9] s390x: gs: move to new header file

[Nico Boehr]

Move the guarded-storage related structs and instructions to a new
header file because we will also need them for the SIGP store additional
status tests in smp.c.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 lib/s390x/gs.h | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++
 s390x/gs.c     | 65 +---------------------------------------
 2 files changed, 81 insertions(+), 64 deletions(-)
 create mode 100644 lib/s390x/gs.h

diff --git a/lib/s390x/gs.h b/lib/s390x/gs.h
new file mode 100644
index 000000000000..e28fa4e1b893
--- /dev/null
+++ b/lib/s390x/gs.h
@@ -0,0 +1,80 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Guarded storage related definitions
+ *
+ * Copyright 2018 IBM Corp.
+ *
+ * Authors:
+ *    Martin Schwidefsky <schwidefsky@de.ibm.com>
+ *    Janosch Frank <frankja@linux.ibm.com>
+ */
+#include <stdint.h>
+
+#ifndef _S390X_GS_H_
+#define _S390X_GS_H_
+
+struct gs_cb {
+	uint64_t reserved;
+	uint64_t gsd;
+	uint64_t gssm;
+	uint64_t gs_epl_a;
+};
+
+struct gs_epl {
+	uint8_t pad1;
+	union {
+		uint8_t gs_eam;
+		struct {
+			uint8_t		: 6;
+			uint8_t e	: 1;
+			uint8_t b	: 1;
+		};
+	};
+	union {
+		uint8_t gs_eci;
+		struct {
+			uint8_t tx	: 1;
+			uint8_t cx	: 1;
+			uint8_t		: 5;
+			uint8_t in	: 1;
+		};
+	};
+	union {
+		uint8_t gs_eai;
+		struct {
+			uint8_t		: 1;
+			uint8_t t	: 1;
+			uint8_t as	: 2;
+			uint8_t ar	: 4;
+		};
+	};
+	uint32_t pad2;
+	uint64_t gs_eha;
+	uint64_t gs_eia;
+	uint64_t gs_eoa;
+	uint64_t gs_eir;
+	uint64_t gs_era;
+};
+
+static inline void load_gs_cb(struct gs_cb *gs_cb)
+{
+	asm volatile(".insn rxy,0xe3000000004d,0,%0" : : "Q" (*gs_cb));
+}
+
+static inline void store_gs_cb(struct gs_cb *gs_cb)
+{
+	asm volatile(".insn rxy,0xe30000000049,0,%0" : : "Q" (*gs_cb));
+}
+
+static inline unsigned long load_guarded(unsigned long *p)
+{
+	unsigned long v;
+
+	asm(".insn rxy,0xe3000000004c, %0,%1"
+	    : "=d" (v)
+	    : "m" (*p)
+	    : "r14", "memory");
+	return v;
+}
+
+#endif
diff --git a/s390x/gs.c b/s390x/gs.c
index 7567bb78fecb..248f387abf1b 100644
--- a/s390x/gs.c
+++ b/s390x/gs.c
@@ -13,49 +13,7 @@
 #include <asm/facility.h>
 #include <asm/interrupt.h>
 #include <asm-generic/barrier.h>
-
-struct gs_cb {
-	uint64_t reserved;
-	uint64_t gsd;
-	uint64_t gssm;
-	uint64_t gs_epl_a;
-};
-
-struct gs_epl {
-	uint8_t pad1;
-	union {
-		uint8_t gs_eam;
-		struct {
-			uint8_t		: 6;
-			uint8_t e	: 1;
-			uint8_t b	: 1;
-		};
-	};
-	union {
-		uint8_t gs_eci;
-		struct {
-			uint8_t tx	: 1;
-			uint8_t cx	: 1;
-			uint8_t		: 5;
-			uint8_t in	: 1;
-		};
-	};
-	union {
-		uint8_t gs_eai;
-		struct {
-			uint8_t		: 1;
-			uint8_t t	: 1;
-			uint8_t as	: 2;
-			uint8_t ar	: 4;
-		};
-	};
-	uint32_t pad2;
-	uint64_t gs_eha;
-	uint64_t gs_eia;
-	uint64_t gs_eoa;
-	uint64_t gs_eir;
-	uint64_t gs_era;
-};
+#include <gs.h>
 
 static volatile int guarded = 0;
 static struct gs_cb gs_cb;
@@ -64,27 +22,6 @@ static unsigned long gs_area = 0x2000000;
 
 void gs_handler(struct gs_cb *this_cb);
 
-static inline void load_gs_cb(struct gs_cb *gs_cb)
-{
-	asm volatile(".insn rxy,0xe3000000004d,0,%0" : : "Q" (*gs_cb));
-}
-
-static inline void store_gs_cb(struct gs_cb *gs_cb)
-{
-	asm volatile(".insn rxy,0xe30000000049,0,%0" : : "Q" (*gs_cb));
-}
-
-static inline unsigned long load_guarded(unsigned long *p)
-{
-	unsigned long v;
-
-	asm(".insn rxy,0xe3000000004c, %0,%1"
-	    : "=d" (v)
-	    : "m" (*p)
-	    : "r14", "memory");
-	return v;
-}
-
 /* guarded-storage event handler and finally it calls gs_handler */
 extern void gs_handler_asm(void);
 	asm(".globl gs_handler_asm\n"
-- 
2.31.1

[kvm-unit-tests PATCH v1 4/9] s390x: smp: add test for SIGP_STORE_ADTL_STATUS order

[Nico Boehr]

Add a test for SIGP_STORE_ADDITIONAL_STATUS order.

There are several cases to cover:
- when neither vector nor guarded-storage facility is available, check
  the order is rejected.
- when one of the facilities is there, test the order is rejected and
  adtl_status is not touched when the target CPU is running or when an
  invalid CPU address is specified. Also check the order is rejected
  in case of invalid alignment.
- when the vector facility is there, write some data to the CPU's
  vector registers and check we get the right contents.
- when the guarded-storage facility is there, populate the CPU's
  guarded-storage registers with some data and again check we get the
  right contents.

To make sure we cover all these cases, adjust unittests.cfg to run the
smp tests with both guarded-storage and vector facility off and on.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 s390x/smp.c         | 259 ++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |   6 +
 2 files changed, 265 insertions(+)

diff --git a/s390x/smp.c b/s390x/smp.c
index e5a16eb5a46a..5d3265f6be64 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -16,6 +16,7 @@
 #include <asm/sigp.h>
 
 #include <smp.h>
+#include <gs.h>
 #include <alloc_page.h>
 
 static int testflag = 0;
@@ -37,6 +38,19 @@ static const struct sigp_invalid_cases cases_valid_cpu_addr[] = {
 	{ INVALID_ORDER_CODE,         "invalid order code" },
 };
 
+/*
+ * We keep two structs, one for comparing when we want to assert it's not
+ * touched.
+ */
+static uint8_t adtl_status[2][4096] __attribute__((aligned(4096)));
+
+#define NUM_VEC_REGISTERS 32
+#define VEC_REGISTER_SIZE 16
+static uint8_t expected_vec_contents[NUM_VEC_REGISTERS][VEC_REGISTER_SIZE];
+
+static struct gs_cb gs_cb;
+static struct gs_epl gs_epl;
+
 static void test_invalid(void)
 {
 	const struct sigp_invalid_cases *c;
@@ -200,6 +214,247 @@ static void test_store_status(void)
 	report_prefix_pop();
 }
 
+static int have_adtl_status(void)
+{
+	return test_facility(133) || test_facility(129);
+}
+
+static void test_store_adtl_status(void)
+{
+	uint32_t status = -1;
+	int cc;
+
+	report_prefix_push("store additional status");
+
+	if (!have_adtl_status()) {
+		report_skip("no guarded-storage or vector facility installed");
+		goto out;
+	}
+
+	memset(adtl_status, 0xff, sizeof(adtl_status));
+
+	report_prefix_push("running");
+	smp_cpu_restart(1);
+
+	cc = smp_sigp(1, SIGP_STORE_ADDITIONAL_STATUS,
+		  (unsigned long)adtl_status, &status);
+
+	report(cc == 1, "CC = 1");
+	report(status == SIGP_STATUS_INCORRECT_STATE, "status = INCORRECT_STATE");
+	report(!memcmp(adtl_status[0], adtl_status[1], sizeof(adtl_status[0])),
+	       "additional status not touched");
+
+	report_prefix_pop();
+
+	report_prefix_push("invalid CPU address");
+
+	cc = sigp(INVALID_CPU_ADDRESS, SIGP_STORE_ADDITIONAL_STATUS,
+		  (unsigned long)adtl_status, &status);
+	report(cc == 3, "CC = 3");
+	report(!memcmp(adtl_status[0], adtl_status[1], sizeof(adtl_status[0])),
+	       "additional status not touched");
+
+	report_prefix_pop();
+
+	report_prefix_push("unaligned");
+	smp_cpu_stop(1);
+
+	cc = smp_sigp(1, SIGP_STORE_ADDITIONAL_STATUS,
+		  (unsigned long)adtl_status + 256, &status);
+	report(cc == 1, "CC = 1");
+	report(status == SIGP_STATUS_INVALID_PARAMETER, "status = INVALID_PARAMETER");
+
+	report_prefix_pop();
+
+out:
+	report_prefix_pop();
+}
+
+static void test_store_adtl_status_unavail(void)
+{
+	uint32_t status = 0;
+	int cc;
+
+	report_prefix_push("store additional status unvailable");
+
+	if (have_adtl_status()) {
+		report_skip("guarded-storage or vector facility installed");
+		goto out;
+	}
+
+	report_prefix_push("not accepted");
+	smp_cpu_stop(1);
+
+	cc = smp_sigp(1, SIGP_STORE_ADDITIONAL_STATUS,
+		  (unsigned long)adtl_status, &status);
+
+	report(cc == 1, "CC = 1");
+	report(status == SIGP_STATUS_INVALID_ORDER,
+	       "status = INVALID_ORDER");
+
+	report_prefix_pop();
+
+out:
+	report_prefix_pop();
+}
+
+static void restart_write_vector(void)
+{
+	uint8_t *vec_reg;
+	uint8_t *vec_reg_16_31 = &expected_vec_contents[16][0];
+	int i;
+
+	for (i = 0; i < NUM_VEC_REGISTERS; i++) {
+		vec_reg = &expected_vec_contents[i][0];
+		memset(vec_reg, i, VEC_REGISTER_SIZE);
+	}
+
+	ctl_set_bit(0, CTL0_VECTOR);
+
+	asm volatile (
+		"	.machine z13\n"
+		"	vlm 0,15, %[vec_reg_0_15]\n"
+		"	vlm 16,31, %[vec_reg_16_31]\n"
+		:
+		: [vec_reg_0_15] "Q"(expected_vec_contents),
+		  [vec_reg_16_31] "Q"(*vec_reg_16_31)
+		: "v0", "v1", "v2", "v3", "v4", "v5", "v6", "v7", "v8", "v9",
+		  "v10", "v11", "v12", "v13", "v14", "v15", "v16", "v17", "v18",
+		  "v19", "v20", "v21", "v22", "v23", "v24", "v25", "v26", "v27",
+		  "v28", "v29", "v30", "v31", "memory"
+	);
+
+	ctl_clear_bit(0, CTL0_VECTOR);
+
+	set_flag(1);
+
+	/*
+	 * function epilogue will restore floating point registers and hence
+	 * destroy vector register contents
+	 */
+	while (1)
+		;
+}
+
+static void cpu_write_magic_to_vector_regs(uint16_t cpu_idx)
+{
+	struct psw new_psw;
+
+	smp_cpu_stop(cpu_idx);
+
+	new_psw.mask = extract_psw_mask();
+	new_psw.addr = (unsigned long)restart_write_vector;
+
+	set_flag(0);
+
+	smp_cpu_start(cpu_idx, new_psw);
+
+	wait_for_flag();
+}
+
+static void test_store_adtl_status_vector(void)
+{
+	uint32_t status = -1;
+	struct psw psw;
+	int cc;
+
+	report_prefix_push("store additional status vector");
+
+	if (!test_facility(129)) {
+		report_skip("vector facility not installed");
+		goto out;
+	}
+
+	cpu_write_magic_to_vector_regs(1);
+	smp_cpu_stop(1);
+
+	memset(adtl_status, 0xff, sizeof(adtl_status));
+
+	cc = smp_sigp(1, SIGP_STORE_ADDITIONAL_STATUS,
+		  (unsigned long)adtl_status, &status);
+	report(!cc, "CC = 0");
+
+	report(!memcmp(adtl_status, expected_vec_contents, sizeof(expected_vec_contents)),
+	       "additional status contents match");
+
+	/*
+	 * To avoid the floating point/vector registers being cleaned up, we
+	 * stopped CPU1 right in the middle of a function. Hence the cleanup of
+	 * the function didn't run yet and the stackpointer is messed up.
+	 * Destroy and re-initalize the CPU to fix that.
+	 */
+	smp_cpu_destroy(1);
+	psw.mask = extract_psw_mask();
+	psw.addr = (unsigned long)test_func;
+	smp_cpu_setup(1, psw);
+
+out:
+	report_prefix_pop();
+}
+
+static void restart_write_gs_regs(void)
+{
+	const unsigned long gs_area = 0x2000000;
+	const unsigned long gsc = 25; /* align = 32 M, section size = 512K */
+
+	ctl_set_bit(2, CTL2_GUARDED_STORAGE);
+
+	gs_cb.gsd = gs_area | gsc;
+	gs_cb.gssm = 0xfeedc0ffe;
+	gs_cb.gs_epl_a = (uint64_t) &gs_epl;
+
+	load_gs_cb(&gs_cb);
+
+	set_flag(1);
+
+	ctl_clear_bit(2, CTL2_GUARDED_STORAGE);
+}
+
+static void cpu_write_to_gs_regs(uint16_t cpu_idx)
+{
+	struct psw new_psw;
+
+	smp_cpu_stop(cpu_idx);
+
+	new_psw.mask = extract_psw_mask();
+	new_psw.addr = (unsigned long)restart_write_gs_regs;
+
+	set_flag(0);
+
+	smp_cpu_start(cpu_idx, new_psw);
+
+	wait_for_flag();
+}
+
+static void test_store_adtl_status_gs(void)
+{
+	const unsigned long adtl_status_lc_11 = 11;
+	uint32_t status = 0;
+	int cc;
+
+	report_prefix_push("store additional status guarded-storage");
+
+	if (!test_facility(133)) {
+		report_skip("guarded-storage facility not installed");
+		goto out;
+	}
+
+	cpu_write_to_gs_regs(1);
+	smp_cpu_stop(1);
+
+	memset(adtl_status, 0xff, sizeof(adtl_status));
+
+	cc = smp_sigp(1, SIGP_STORE_ADDITIONAL_STATUS,
+		  (unsigned long)adtl_status | adtl_status_lc_11, &status);
+	report(!cc, "CC = 0");
+
+	report(!memcmp(&adtl_status[0][1024], &gs_cb, sizeof(gs_cb)),
+	       "additional status contents match");
+
+out:
+	report_prefix_pop();
+}
+
 static void ecall(void)
 {
 	unsigned long mask;
@@ -388,6 +643,10 @@ int main(void)
 	test_stop();
 	test_stop_store_status();
 	test_store_status();
+	test_store_adtl_status_unavail();
+	test_store_adtl_status_vector();
+	test_store_adtl_status_gs();
+	test_store_adtl_status();
 	test_ecall();
 	test_emcall();
 	test_sense_running();
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index 1600e714c8b9..2d0adc503917 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -77,6 +77,12 @@ extra_params=-name kvm-unit-test --uuid 0fb84a86-727c-11ea-bc55-0242ac130003 -sm
 [smp]
 file = smp.elf
 smp = 2
+extra_params = -cpu host,gs=on,vx=on
+
+[smp-no-vec-no-gs]
+file = smp.elf
+smp = 2
+extra_params = -cpu host,gs=off,vx=off
 
 [sclp-1g]
 file = sclp.elf
-- 
2.31.1

[kvm-unit-tests PATCH v1 5/9] s390x: smp: add tests for SET_PREFIX

[Nico Boehr]

We cover the following cases:

- running CPU
- illegal CPU id

The order should be rejected in both cases.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 s390x/smp.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/s390x/smp.c b/s390x/smp.c
index 5d3265f6be64..f22520b4f4fc 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -51,6 +51,8 @@ static uint8_t expected_vec_contents[NUM_VEC_REGISTERS][VEC_REGISTER_SIZE];
 static struct gs_cb gs_cb;
 static struct gs_epl gs_epl;
 
+static uint32_t cpu1_prefix;
+
 static void test_invalid(void)
 {
 	const struct sigp_invalid_cases *c;
@@ -455,6 +457,76 @@ out:
 	report_prefix_pop();
 }
 
+static void loop(void)
+{
+	while (1)
+		;
+}
+
+static void stpx_and_set_flag(void)
+{
+	asm volatile (
+		"	stpx %[prefix]\n"
+		: [prefix] "=Q" (cpu1_prefix)
+		:
+		:
+	);
+
+	set_flag(1);
+}
+
+static void test_set_prefix(void)
+{
+	struct lowcore *new_lc = alloc_pages_flags(1, AREA_DMA31);
+	struct cpu *cpu1 = smp_cpu_from_idx(1);
+	uint32_t status = 0;
+	struct psw new_psw;
+	int cc;
+
+	report_prefix_push("set prefix");
+
+	assert(new_lc);
+
+	memcpy(new_lc, cpu1->lowcore, sizeof(struct lowcore));
+	new_lc->restart_new_psw.addr = (unsigned long)loop;
+
+	report_prefix_push("running");
+	set_flag(0);
+	new_psw.addr = (unsigned long)stpx_and_set_flag;
+	new_psw.mask = extract_psw_mask();
+	smp_cpu_start(1, new_psw);
+	wait_for_flag();
+	cpu1_prefix = 0xFFFFFFFF;
+
+	cc = smp_sigp(1, SIGP_SET_PREFIX, (unsigned long)new_lc, &status);
+	report(cc == 1, "CC = 1");
+	report(status == SIGP_STATUS_INCORRECT_STATE, "status = INCORRECT_STATE");
+
+	/*
+	 * If the prefix of the other CPU was changed it will enter an endless
+	 * loop. Otherwise, it should eventually set the flag.
+	 */
+	smp_cpu_stop(1);
+	set_flag(0);
+	smp_cpu_restart(1);
+	wait_for_flag();
+	report(cpu1_prefix == (uint64_t)cpu1->lowcore, "prefix unchanged");
+
+	report_prefix_pop();
+
+	report_prefix_push("invalid CPU address");
+
+	cc = sigp(INVALID_CPU_ADDRESS, SIGP_SET_PREFIX, (unsigned long)new_lc, &status);
+	report(cc == 3, "CC = 3");
+
+	report_prefix_pop();
+
+	free_pages(new_lc);
+
+	report_prefix_pop();
+
+}
+
 static void ecall(void)
 {
 	unsigned long mask;
@@ -647,6 +719,7 @@ int main(void)
 	test_store_adtl_status_vector();
 	test_store_adtl_status_gs();
 	test_store_adtl_status();
+	test_set_prefix();
 	test_ecall();
 	test_emcall();
 	test_sense_running();
-- 
2.31.1

[kvm-unit-tests PATCH v1 6/9] s390x: smp: add test for EMERGENCY_SIGNAL with invalid CPU address

[Nico Boehr]

In this case, we expect the order to be rejected.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 s390x/smp.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/s390x/smp.c b/s390x/smp.c
index f22520b4f4fc..3bd7e7c8f5ed 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -580,6 +580,7 @@ static void emcall(void)
 static void test_emcall(void)
 {
 	struct psw psw;
+	int cc;
 	psw.mask = extract_psw_mask();
 	psw.addr = (unsigned long)emcall;
 
@@ -592,6 +593,14 @@ static void test_emcall(void)
 	smp_sigp(1, SIGP_EMERGENCY_SIGNAL, 0, NULL);
 	wait_for_flag();
 	smp_cpu_stop(1);
+
+	report_prefix_push("invalid CPU address");
+
+	cc = sigp(INVALID_CPU_ADDRESS, SIGP_EMERGENCY_SIGNAL, 0, NULL);
+	report(cc == 3, "CC = 3");
+
+	report_prefix_pop();
+
 	report_prefix_pop();
 }
 
-- 
2.31.1

[kvm-unit-tests PATCH v1 7/9] s390x: smp: add tests for CONDITIONAL EMERGENCY

[Nico Boehr]

Add a test for the conditional emergency signal.

We cover the following cases:
- invalid CPU address. Should lead to the order being rejected.
- the order is accepted and an emergency signal is delivered. We make sure
  this is the case by disabling the CPU's PSW for IO interruptions.

Note we intentionally don't cover the case where the signal isn't
delivered. As per the PoP, implementations are allowed to make the signal
an unconditional one when the condition determination is precluded.

This test is unsupported under PV.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 s390x/smp.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/s390x/smp.c b/s390x/smp.c
index 3bd7e7c8f5ed..9c4e27106aef 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -16,6 +16,7 @@
 #include <asm/sigp.h>
 
 #include <smp.h>
+#include <uv.h>
 #include <gs.h>
 #include <alloc_page.h>
 
@@ -604,6 +605,47 @@ static void test_emcall(void)
 	report_prefix_pop();
 }
 
+static void test_cond_emcall(void)
+{
+	uint32_t status = 0;
+	struct psw psw;
+	int cc;
+	psw.mask = extract_psw_mask() & ~PSW_MASK_IO;
+	psw.addr = (unsigned long)emcall;
+
+	report_prefix_push("conditional emergency call");
+
+	if (uv_os_is_guest()) {
+		report_skip("unsupported under PV");
+		goto out;
+	}
+
+	report_prefix_push("invalid CPU address");
+
+	cc = sigp(INVALID_CPU_ADDRESS, SIGP_COND_EMERGENCY_SIGNAL, 0, NULL);
+	report(cc == 3, "CC = 3");
+
+	report_prefix_pop();
+
+	report_prefix_push("success");
+	set_flag(0);
+
+	smp_cpu_start(1, psw);
+	wait_for_flag();
+	set_flag(0);
+	cc = smp_sigp(1, SIGP_COND_EMERGENCY_SIGNAL, 0, &status);
+	report(!cc, "CC = 0");
+
+	wait_for_flag();
+	smp_cpu_stop(1);
+
+	report_prefix_pop();
+
+out:
+	report_prefix_pop();
+
+}
+
 static void test_sense_running(void)
 {
 	report_prefix_push("sense_running");
@@ -731,6 +773,7 @@ int main(void)
 	test_set_prefix();
 	test_ecall();
 	test_emcall();
+	test_cond_emcall();
 	test_sense_running();
 	test_reset();
 	test_reset_initial();
-- 
2.31.1

[kvm-unit-tests PATCH v1 8/9] s390x: add TPROT tests

[Nico Boehr]

Add tests for TEST PROTECTION. We cover the following cases:
- page is read/write
- page is readonly
- lowcore protection
- page is not present
- translation specification exception

We don't cover storage keys and the case where the page can be neither read nor
written right now.

This test mainly applies to the TCG case.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
 s390x/Makefile      |   1 +
 s390x/tprot.c       | 108 ++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |   3 ++
 3 files changed, 112 insertions(+)
 create mode 100644 s390x/tprot.c

diff --git a/s390x/Makefile b/s390x/Makefile
index 53b0fe044fe7..92c1ce4648dd 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -3,6 +3,7 @@ tests += $(TEST_DIR)/intercept.elf
 tests += $(TEST_DIR)/emulator.elf
 tests += $(TEST_DIR)/sieve.elf
 tests += $(TEST_DIR)/sthyi.elf
+tests += $(TEST_DIR)/tprot.elf
 tests += $(TEST_DIR)/skey.elf
 tests += $(TEST_DIR)/diag10.elf
 tests += $(TEST_DIR)/diag308.elf
diff --git a/s390x/tprot.c b/s390x/tprot.c
new file mode 100644
index 000000000000..460a0db7ffcf
--- /dev/null
+++ b/s390x/tprot.c
@@ -0,0 +1,108 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * TEST PROTECTION tests
+ *
+ * Copyright IBM Corp. 2022
+ *
+ * Authors:
+ *  Nico Boehr <nrb@linux.ibm.com>
+ */
+
+#include <libcflat.h>
+#include <bitops.h>
+#include <asm/pgtable.h>
+#include <asm/interrupt.h>
+#include "mmu.h"
+#include <vmalloc.h>
+#include <sclp.h>
+
+static uint8_t pagebuf[PAGE_SIZE] __attribute__((aligned(PAGE_SIZE)));
+
+static void test_tprot_rw(void)
+{
+	int cc;
+
+	report_prefix_push("Page read/writeable");
+
+	cc = tprot((unsigned long)pagebuf, 0);
+	report(cc == 0, "CC = 0");
+
+	report_prefix_pop();
+}
+
+static void test_tprot_ro(void)
+{
+	int cc;
+
+	report_prefix_push("Page readonly");
+
+	protect_dat_entry(pagebuf, PAGE_ENTRY_P, 5);
+
+	cc = tprot((unsigned long)pagebuf, 0);
+	report(cc == 1, "CC = 1");
+
+	unprotect_dat_entry(pagebuf, PAGE_ENTRY_P, 5);
+
+	report_prefix_pop();
+}
+
+static void test_tprot_low_addr_prot(void)
+{
+	int cc;
+
+	report_prefix_push("low-address protection");
+
+	low_prot_enable();
+	cc = tprot(0, 0);
+	low_prot_disable();
+	report(cc == 1, "CC = 1");
+
+	report_prefix_pop();
+}
+
+static void test_tprot_transl_unavail(void)
+{
+	int cc;
+
+	report_prefix_push("Page translation unavailable");
+
+	protect_dat_entry(pagebuf, PAGE_ENTRY_I, 5);
+
+	cc = tprot((unsigned long)pagebuf, 0);
+	report(cc == 3, "CC = 3");
+
+	unprotect_dat_entry(pagebuf, PAGE_ENTRY_I, 5);
+
+	report_prefix_pop();
+}
+
+static void test_tprot_transl_pte_bit52_set(void)
+{
+	report_prefix_push("PTE Bit 52 set");
+
+	protect_dat_entry(pagebuf, BIT(63 - 52), 5);
+
+	expect_pgm_int();
+	tprot((unsigned long)pagebuf, 0);
+	check_pgm_int_code(PGM_INT_CODE_TRANSLATION_SPEC);
+
+	unprotect_dat_entry(pagebuf, BIT(63 - 52), 5);
+
+	report_prefix_pop();
+}
+
+int main(void)
+{
+	report_prefix_push("tprot");
+
+	setup_vm();
+
+	test_tprot_rw();
+	test_tprot_ro();
+	test_tprot_low_addr_prot();
+	test_tprot_transl_unavail();
+	test_tprot_transl_pte_bit52_set();
+
+	report_prefix_pop();
+	return report_summary();
+}
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index 2d0adc503917..6227cd3ba1d0 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -142,3 +142,6 @@ file = firq.elf
 timeout = 20
 extra_params = -smp 1,maxcpus=3 -cpu qemu -device qemu-s390x-cpu,core-id=2 -device qemu-s390x-cpu,core-id=1
 accel = tcg
+
+[tprot]
+file = tprot.elf
-- 
2.31.1

[kvm-unit-tests PATCH v1 9/9] s390x: stsi: check zero and ignored bits in r0 and r1

[Nico Boehr]

We previously only checked for two zero bits, one in r0 and one in r1.
Let's check all the bits which must be zero and which are ignored
to extend the coverage.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 s390x/stsi.c | 42 ++++++++++++++++++++++++++++++++----------
 1 file changed, 32 insertions(+), 10 deletions(-)

diff --git a/s390x/stsi.c b/s390x/stsi.c
index dccc53e7a816..94a579dc3b58 100644
--- a/s390x/stsi.c
+++ b/s390x/stsi.c
@@ -9,6 +9,7 @@
  */
 
 #include <libcflat.h>
+#include <bitops.h>
 #include <asm/page.h>
 #include <asm/asm-offsets.h>
 #include <asm/interrupt.h>
@@ -19,19 +20,40 @@ static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
 
 static void test_specs(void)
 {
+	int i;
+	int cc;
+
 	report_prefix_push("specification");
 
-	report_prefix_push("inv r0");
-	expect_pgm_int();
-	stsi(pagebuf, 0, 1 << 8, 0);
-	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
-	report_prefix_pop();
+	for (i = 36; i <= 55; i++) {
+		report_prefix_pushf("set invalid r0 bit %d", i);
+		expect_pgm_int();
+		stsi(pagebuf, 0, BIT(63 - i), 0);
+		check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+		report_prefix_pop();
+	}
 
-	report_prefix_push("inv r1");
-	expect_pgm_int();
-	stsi(pagebuf, 1, 0, 1 << 16);
-	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
-	report_prefix_pop();
+	for (i = 32; i <= 47; i++) {
+		report_prefix_pushf("set invalid r1 bit %d", i);
+		expect_pgm_int();
+		stsi(pagebuf, 1, 0, BIT(63 - i));
+		check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+		report_prefix_pop();
+	}
+
+	for (i = 0; i < 32; i++) {
+		report_prefix_pushf("r0 bit %d ignored", i);
+		cc = stsi(pagebuf, 3, 2 | BIT(63 - i), 2);
+		report(!cc, "CC = 0");
+		report_prefix_pop();
+	}
+
+	for (i = 0; i < 32; i++) {
+		report_prefix_pushf("r1 bit %d ignored", i);
+		cc = stsi(pagebuf, 3, 2, 2 | BIT(63 - i));
+		report(!cc, "CC = 0");
+		report_prefix_pop();
+	}
 
 	report_prefix_push("unaligned");
 	expect_pgm_int();
-- 
2.31.1

Re: [kvm-unit-tests PATCH v1 4/9] s390x: smp: add test for SIGP_STORE_ADTL_STATUS order

[Claudio Imbrenda]

On Mon, 21 Mar 2022 11:18:59 +0100
Nico Boehr <nrb@linux.ibm.com> wrote:

> Add a test for SIGP_STORE_ADDITIONAL_STATUS order.
> 
> There are several cases to cover:
> - when neither vector nor guarded-storage facility is available, check
>   the order is rejected.
> - when one of the facilities is there, test the order is rejected and
>   adtl_status is not touched when the target CPU is running or when an
>   invalid CPU address is specified. Also check the order is rejected
>   in case of invalid alignment.
> - when the vector facility is there, write some data to the CPU's
>   vector registers and check we get the right contents.
> - when the guarded-storage facility is there, populate the CPU's
>   guarded-storage registers with some data and again check we get the
>   right contents.
> 
> To make sure we cover all these cases, adjust unittests.cfg to run the
> smp tests with both guarded-storage and vector facility off and on.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>  s390x/smp.c         | 259 ++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg |   6 +
>  2 files changed, 265 insertions(+)
> 
> diff --git a/s390x/smp.c b/s390x/smp.c
> index e5a16eb5a46a..5d3265f6be64 100644
> --- a/s390x/smp.c
> +++ b/s390x/smp.c
> @@ -16,6 +16,7 @@
>  #include <asm/sigp.h>
>  
>  #include <smp.h>
> +#include <gs.h>
>  #include <alloc_page.h>
>  
>  static int testflag = 0;
> @@ -37,6 +38,19 @@ static const struct sigp_invalid_cases cases_valid_cpu_addr[] = {
>  	{ INVALID_ORDER_CODE,         "invalid order code" },
>  };
>  
> +/*
> + * We keep two structs, one for comparing when we want to assert it's not
> + * touched.
> + */
> +static uint8_t adtl_status[2][4096] __attribute__((aligned(4096)));

it's a little bit ugly. maybe define a struct, with small buffers inside
for the vector and gs areas? that way we would not need ugly magic
numbers below (see below)

> +
> +#define NUM_VEC_REGISTERS 32
> +#define VEC_REGISTER_SIZE 16
> +static uint8_t expected_vec_contents[NUM_VEC_REGISTERS][VEC_REGISTER_SIZE];
> +
> +static struct gs_cb gs_cb;
> +static struct gs_epl gs_epl;
> +
>  static void test_invalid(void)
>  {
>  	const struct sigp_invalid_cases *c;
> @@ -200,6 +214,247 @@ static void test_store_status(void)
>  	report_prefix_pop();
>  }
>  
> +static int have_adtl_status(void)
> +{
> +	return test_facility(133) || test_facility(129);
> +}
> +
> +static void test_store_adtl_status(void)
> +{
> +	uint32_t status = -1;
> +	int cc;
> +
> +	report_prefix_push("store additional status");
> +
> +	if (!have_adtl_status()) {
> +		report_skip("no guarded-storage or vector facility installed");
> +		goto out;
> +	}
> +
> +	memset(adtl_status, 0xff, sizeof(adtl_status));
> +
> +	report_prefix_push("running");
> +	smp_cpu_restart(1);
> +
> +	cc = smp_sigp(1, SIGP_STORE_ADDITIONAL_STATUS,
> +		  (unsigned long)adtl_status, &status);
> +
> +	report(cc == 1, "CC = 1");
> +	report(status == SIGP_STATUS_INCORRECT_STATE, "status = INCORRECT_STATE");
> +	report(!memcmp(adtl_status[0], adtl_status[1], sizeof(adtl_status[0])),
> +	       "additional status not touched");
> +
> +	report_prefix_pop();
> +
> +	report_prefix_push("invalid CPU address");
> +
> +	cc = sigp(INVALID_CPU_ADDRESS, SIGP_STORE_ADDITIONAL_STATUS,
> +		  (unsigned long)adtl_status, &status);
> +	report(cc == 3, "CC = 3");
> +	report(!memcmp(adtl_status[0], adtl_status[1], sizeof(adtl_status[0])),
> +	       "additional status not touched");
> +
> +	report_prefix_pop();
> +
> +	report_prefix_push("unaligned");
> +	smp_cpu_stop(1);
> +
> +	cc = smp_sigp(1, SIGP_STORE_ADDITIONAL_STATUS,
> +		  (unsigned long)adtl_status + 256, &status);
> +	report(cc == 1, "CC = 1");
> +	report(status == SIGP_STATUS_INVALID_PARAMETER, "status = INVALID_PARAMETER");
> +
> +	report_prefix_pop();
> +
> +out:
> +	report_prefix_pop();
> +}
> +
> +static void test_store_adtl_status_unavail(void)
> +{
> +	uint32_t status = 0;
> +	int cc;
> +
> +	report_prefix_push("store additional status unvailable");
> +
> +	if (have_adtl_status()) {
> +		report_skip("guarded-storage or vector facility installed");
> +		goto out;
> +	}
> +
> +	report_prefix_push("not accepted");
> +	smp_cpu_stop(1);
> +
> +	cc = smp_sigp(1, SIGP_STORE_ADDITIONAL_STATUS,
> +		  (unsigned long)adtl_status, &status);
> +
> +	report(cc == 1, "CC = 1");
> +	report(status == SIGP_STATUS_INVALID_ORDER,
> +	       "status = INVALID_ORDER");
> +
> +	report_prefix_pop();
> +
> +out:
> +	report_prefix_pop();
> +}
> +
> +static void restart_write_vector(void)
> +{
> +	uint8_t *vec_reg;
> +	uint8_t *vec_reg_16_31 = &expected_vec_contents[16][0];

add a comment to explain that vlm only handles at most 16 registers at
a time

> +	int i;
> +
> +	for (i = 0; i < NUM_VEC_REGISTERS; i++) {
> +		vec_reg = &expected_vec_contents[i][0];
> +		memset(vec_reg, i, VEC_REGISTER_SIZE);
> +	}

this way vector register 0 stays 0.
either special case it (e.g. 16, or whatever), or put a magic value
somewhere in every register

> +
> +	ctl_set_bit(0, CTL0_VECTOR);
> +
> +	asm volatile (
> +		"	.machine z13\n"
> +		"	vlm 0,15, %[vec_reg_0_15]\n"
> +		"	vlm 16,31, %[vec_reg_16_31]\n"
> +		:
> +		: [vec_reg_0_15] "Q"(expected_vec_contents),
> +		  [vec_reg_16_31] "Q"(*vec_reg_16_31)
> +		: "v0", "v1", "v2", "v3", "v4", "v5", "v6", "v7", "v8", "v9",
> +		  "v10", "v11", "v12", "v13", "v14", "v15", "v16", "v17", "v18",
> +		  "v19", "v20", "v21", "v22", "v23", "v24", "v25", "v26", "v27",
> +		  "v28", "v29", "v30", "v31", "memory"
> +	);
> +
> +	ctl_clear_bit(0, CTL0_VECTOR);
> +
> +	set_flag(1);
> +
> +	/*
> +	 * function epilogue will restore floating point registers and hence
> +	 * destroy vector register contents
> +	 */
> +	while (1)
> +		;
> +}
> +
> +static void cpu_write_magic_to_vector_regs(uint16_t cpu_idx)
> +{
> +	struct psw new_psw;
> +
> +	smp_cpu_stop(cpu_idx);
> +
> +	new_psw.mask = extract_psw_mask();
> +	new_psw.addr = (unsigned long)restart_write_vector;
> +
> +	set_flag(0);
> +
> +	smp_cpu_start(cpu_idx, new_psw);
> +
> +	wait_for_flag();
> +}
> +
> +static void test_store_adtl_status_vector(void)
> +{
> +	uint32_t status = -1;
> +	struct psw psw;
> +	int cc;
> +
> +	report_prefix_push("store additional status vector");
> +
> +	if (!test_facility(129)) {
> +		report_skip("vector facility not installed");
> +		goto out;
> +	}
> +
> +	cpu_write_magic_to_vector_regs(1);
> +	smp_cpu_stop(1);
> +
> +	memset(adtl_status, 0xff, sizeof(adtl_status));
> +
> +	cc = smp_sigp(1, SIGP_STORE_ADDITIONAL_STATUS,
> +		  (unsigned long)adtl_status, &status);
> +	report(!cc, "CC = 0");
> +
> +	report(!memcmp(adtl_status, expected_vec_contents, sizeof(expected_vec_contents)),
> +	       "additional status contents match");

it would be interesting to check that nothing is stored past the end of
the buffer.

moreover, I think you should also explicitly test with lc_10, to make
sure that works as well (no need to rerun the guest, just add another
sigp call)

> +
> +	/*
> +	 * To avoid the floating point/vector registers being cleaned up, we
> +	 * stopped CPU1 right in the middle of a function. Hence the cleanup of
> +	 * the function didn't run yet and the stackpointer is messed up.
> +	 * Destroy and re-initalize the CPU to fix that.
> +	 */
> +	smp_cpu_destroy(1);
> +	psw.mask = extract_psw_mask();
> +	psw.addr = (unsigned long)test_func;
> +	smp_cpu_setup(1, psw);
> +
> +out:
> +	report_prefix_pop();
> +}
> +
> +static void restart_write_gs_regs(void)
> +{
> +	const unsigned long gs_area = 0x2000000;
> +	const unsigned long gsc = 25; /* align = 32 M, section size = 512K */
> +
> +	ctl_set_bit(2, CTL2_GUARDED_STORAGE);
> +
> +	gs_cb.gsd = gs_area | gsc;
> +	gs_cb.gssm = 0xfeedc0ffe;
> +	gs_cb.gs_epl_a = (uint64_t) &gs_epl;
> +
> +	load_gs_cb(&gs_cb);
> +
> +	set_flag(1);
> +
> +	ctl_clear_bit(2, CTL2_GUARDED_STORAGE);

what happens when the function returns? is r14 set up properly? (or
maybe we just don't care, since we are going to stop the CPU anyway?)

> +}
> +
> +static void cpu_write_to_gs_regs(uint16_t cpu_idx)
> +{
> +	struct psw new_psw;
> +
> +	smp_cpu_stop(cpu_idx);
> +
> +	new_psw.mask = extract_psw_mask();
> +	new_psw.addr = (unsigned long)restart_write_gs_regs;
> +
> +	set_flag(0);
> +
> +	smp_cpu_start(cpu_idx, new_psw);
> +
> +	wait_for_flag();
> +}
> +
> +static void test_store_adtl_status_gs(void)
> +{
> +	const unsigned long adtl_status_lc_11 = 11;
> +	uint32_t status = 0;
> +	int cc;
> +
> +	report_prefix_push("store additional status guarded-storage");
> +
> +	if (!test_facility(133)) {
> +		report_skip("guarded-storage facility not installed");
> +		goto out;
> +	}
> +
> +	cpu_write_to_gs_regs(1);
> +	smp_cpu_stop(1);
> +
> +	memset(adtl_status, 0xff, sizeof(adtl_status));
> +
> +	cc = smp_sigp(1, SIGP_STORE_ADDITIONAL_STATUS,
> +		  (unsigned long)adtl_status | adtl_status_lc_11, &status);
> +	report(!cc, "CC = 0");
> +
> +	report(!memcmp(&adtl_status[0][1024], &gs_cb, sizeof(gs_cb)),

e.g. the 1024 is one of those "magic number" I mentioned above 

> +	       "additional status contents match");

it would be interesting to test that nothing is stored after the end of
the buffer (i.e. everything is still 0xff in the second half of the
page)

> +
> +out:
> +	report_prefix_pop();
> +}
> +
>  static void ecall(void)
>  {
>  	unsigned long mask;
> @@ -388,6 +643,10 @@ int main(void)
>  	test_stop();
>  	test_stop_store_status();
>  	test_store_status();
> +	test_store_adtl_status_unavail();
> +	test_store_adtl_status_vector();
> +	test_store_adtl_status_gs();
> +	test_store_adtl_status();
>  	test_ecall();
>  	test_emcall();
>  	test_sense_running();
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index 1600e714c8b9..2d0adc503917 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -77,6 +77,12 @@ extra_params=-name kvm-unit-test --uuid 0fb84a86-727c-11ea-bc55-0242ac130003 -sm
>  [smp]
>  file = smp.elf
>  smp = 2
> +extra_params = -cpu host,gs=on,vx=on
> +
> +[smp-no-vec-no-gs]
> +file = smp.elf
> +smp = 2
> +extra_params = -cpu host,gs=off,vx=off

using "host" will break TCG
(and using "qemu" will break secure execution)

there are two possible solutions:

use "max" and deal with the warnings, or split each testcase in two,
one using host cpu and "accel = kvm" and the other with "accel = tcg"
and qemu cpu.

what should happen if only one of the two features is installed? should
the buffer for the unavailable feature be stored with 0 or should it be
left untouched? is it worth testing those scenarios?

>  
>  [sclp-1g]
>  file = sclp.elf

Re: [kvm-unit-tests PATCH v1 0/9] s390x: Further extend instruction interception tests

[Claudio Imbrenda]

On Mon, 21 Mar 2022 11:18:55 +0100
Nico Boehr <nrb@linux.ibm.com> wrote:

> Further extend the instruction interception tests for s390x. This series focuses
> on SIGP, STSI and TPROT instructions.
> 
> Some instructions such as STSI already had some coverage and the existing tests
> were extended to increase coverage.
> 
> The SIGP instruction has coverage, but not for all orders. Orders
> STORE_ADTL_STATUS, SET_PREFIX and CONDITIONAL_EMERGENCY didn't
> have coverage before and new tests are added. For EMERGENCY_SIGNAL, the existing
> test is extended to cover an invalid CPU address. Additionally, new tests are
> added for quite a few invalid arguments to SIGP.
> 
> TPROT was used in skrf tests, but only for storage keys, so add tests for the
> other uses as well.

I like how this series turned out.

once the fourth patch is ironed out, I think this would be ready for
queuing

> 
> Nico Boehr (9):
>   s390x: smp: add tests for several invalid SIGP orders
>   s390x: smp: stop already stopped CPU
>   s390x: gs: move to new header file
>   s390x: smp: add test for SIGP_STORE_ADTL_STATUS order
>   s390x: smp: add tests for SET_PREFIX
>   s390x: smp: add test for EMERGENCY_SIGNAL with invalid CPU address
>   s390x: smp: add tests for CONDITIONAL EMERGENCY
>   s390x: add TPROT tests
>   s390x: stsi: check zero and ignored bits in r0 and r1
> 
>  lib/s390x/gs.h      |  80 ++++++++
>  s390x/Makefile      |   1 +
>  s390x/gs.c          |  65 +------
>  s390x/smp.c         | 437 ++++++++++++++++++++++++++++++++++++++++++++
>  s390x/stsi.c        |  42 ++++-
>  s390x/tprot.c       | 108 +++++++++++
>  s390x/unittests.cfg |   9 +
>  7 files changed, 668 insertions(+), 74 deletions(-)
>  create mode 100644 lib/s390x/gs.h
>  create mode 100644 s390x/tprot.c
> 

Re: [kvm-unit-tests PATCH v1 4/9] s390x: smp: add test for SIGP_STORE_ADTL_STATUS order

[Nico Boehr]

On Mon, 2022-03-21 at 15:59 +0100, Claudio Imbrenda wrote:
> On Mon, 21 Mar 2022 11:18:59 +0100
> Nico Boehr <nrb@linux.ibm.com> wrote:
> > diff --git a/s390x/smp.c b/s390x/smp.c
> > index e5a16eb5a46a..5d3265f6be64 100644
> > --- a/s390x/smp.c
[...]
> > +/*
> > + * We keep two structs, one for comparing when we want to assert
> > it's not
> > + * touched.
> > + */
> > +static uint8_t adtl_status[2][4096]
> > __attribute__((aligned(4096)));
> 
> it's a little bit ugly. maybe define a struct, with small buffers
> inside
> for the vector and gs areas? that way we would not need ugly magic
> numbers below (see below)

OK, will do.

[...]
> > +static void restart_write_vector(void)
> > +{
> > +       uint8_t *vec_reg;
> > +       uint8_t *vec_reg_16_31 = &expected_vec_contents[16][0];
> 
> add a comment to explain that vlm only handles at most 16 registers
> at
> a time

OK will do.

> > +       int i;
> > +
> > +       for (i = 0; i < NUM_VEC_REGISTERS; i++) {
> > +               vec_reg = &expected_vec_contents[i][0];
> > +               memset(vec_reg, i, VEC_REGISTER_SIZE);
> > +       }
> 
> this way vector register 0 stays 0.
> either special case it (e.g. 16, or whatever), or put a magic value
> somewhere in every register

adtl_status is initalized with 0xff. Are you OK with i + 1 so we avoid
zero?

[...]
> > +static void test_store_adtl_status_vector(void)
> > +{
> > +       uint32_t status = -1;
> > +       struct psw psw;
> > +       int cc;
> > +
> > +       report_prefix_push("store additional status vector");
> > +
> > +       if (!test_facility(129)) {
> > +               report_skip("vector facility not installed");
> > +               goto out;
> > +       }
> > +
> > +       cpu_write_magic_to_vector_regs(1);
> > +       smp_cpu_stop(1);
> > +
> > +       memset(adtl_status, 0xff, sizeof(adtl_status));
> > +
> > +       cc = smp_sigp(1, SIGP_STORE_ADDITIONAL_STATUS,
> > +                 (unsigned long)adtl_status, &status);
> > +       report(!cc, "CC = 0");
> > +
> > +       report(!memcmp(adtl_status, expected_vec_contents,
> > sizeof(expected_vec_contents)),
> > +              "additional status contents match");
> 
> it would be interesting to check that nothing is stored past the end
> of
> the buffer.

I will add checks to ensure reserved fields are not modified.

> moreover, I think you should also explicitly test with lc_10, to make
> sure that works as well (no need to rerun the guest, just add another
> sigp call)

I will test vector with LC 0, 10, 11, 12 and guarded storage with LC 11
and 12.

[...]
> > +static void restart_write_gs_regs(void)
> > +{
> > +       const unsigned long gs_area = 0x2000000;
> > +       const unsigned long gsc = 25; /* align = 32 M, section size
> > = 512K */
> > +
> > +       ctl_set_bit(2, CTL2_GUARDED_STORAGE);
> > +
> > +       gs_cb.gsd = gs_area | gsc;
> > +       gs_cb.gssm = 0xfeedc0ffe;
> > +       gs_cb.gs_epl_a = (uint64_t) &gs_epl;
> > +
> > +       load_gs_cb(&gs_cb);
> > +
> > +       set_flag(1);
> > +
> > +       ctl_clear_bit(2, CTL2_GUARDED_STORAGE);
> 
> what happens when the function returns? is r14 set up properly? (or
> maybe we just don't care, since we are going to stop the CPU anyway?)

We have an endless loop in smp_cpu_setup_state. So r14 will point there
(verified with gdb).

In the end, I think we don't care. This is in contrast to the vector
test, where the epilogue will clean up the floating point regs.

[...]
> > +static void test_store_adtl_status_gs(void)
> > +{
> > +       const unsigned long adtl_status_lc_11 = 11;
> > +       uint32_t status = 0;
> > +       int cc;
> > +
> > +       report_prefix_push("store additional status guarded-
> > storage");
> > +
> > +       if (!test_facility(133)) {
> > +               report_skip("guarded-storage facility not
> > installed");
> > +               goto out;
> > +       }
> > +
> > +       cpu_write_to_gs_regs(1);
> > +       smp_cpu_stop(1);
> > +
> > +       memset(adtl_status, 0xff, sizeof(adtl_status));
> > +
> > +       cc = smp_sigp(1, SIGP_STORE_ADDITIONAL_STATUS,
> > +                 (unsigned long)adtl_status | adtl_status_lc_11,
> > &status);
> > +       report(!cc, "CC = 0");
> > +
> > +       report(!memcmp(&adtl_status[0][1024], &gs_cb,
> > sizeof(gs_cb)),
> 
> e.g. the 1024 is one of those "magic number" I mentioned above 

OK, fixed.

> 
> > +              "additional status contents match");
> 
> it would be interesting to test that nothing is stored after the end
> of
> the buffer (i.e. everything is still 0xff in the second half of the
> page)

Yes, done.

[...]
> > 
> > diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> > index 1600e714c8b9..2d0adc503917 100644
> > --- a/s390x/unittests.cfg
> > +++ b/s390x/unittests.cfg
> > @@ -77,6 +77,12 @@ extra_params=-name kvm-unit-test --uuid
> > 0fb84a86-727c-11ea-bc55-0242ac130003 -sm
> >  [smp]
> >  file = smp.elf
> >  smp = 2
> > +extra_params = -cpu host,gs=on,vx=on
> > +
> > +[smp-no-vec-no-gs]
> > +file = smp.elf
> > +smp = 2
> > +extra_params = -cpu host,gs=off,vx=off
> 
> using "host" will break TCG
> (and using "qemu" will break secure execution)
> 
> there are two possible solutions:
> 
> use "max" and deal with the warnings, or split each testcase in two,
> one using host cpu and "accel = kvm" and the other with "accel = tcg"
> and qemu cpu.

Uh, thanks for pointing out. I will split in accel = tcg and accel =
kvm.

> what should happen if only one of the two features is installed?
> should
> the buffer for the unavailable feature be stored with 0 or should it
> be
> left untouched? is it worth testing those scenarios?

The PoP specifies: "A facility’s registers are only
stored in the MCESA when the facility is installed."

Maybe I miss something, but it doesn't seem worth it. It would mean
adding yet another instance to the unittests.cfg. Since once needs to
provide the memory for the registers even when the facility isn't
there, there seems little risk for breaking something when we store if
the facility isn't there.

Re: [kvm-unit-tests PATCH v1 4/9] s390x: smp: add test for SIGP_STORE_ADTL_STATUS order

[Claudio Imbrenda]

On Wed, 23 Mar 2022 15:19:33 +0100
Nico Boehr <nrb@linux.ibm.com> wrote:

> On Mon, 2022-03-21 at 15:59 +0100, Claudio Imbrenda wrote:
> > On Mon, 21 Mar 2022 11:18:59 +0100
> > Nico Boehr <nrb@linux.ibm.com> wrote:  
> > > diff --git a/s390x/smp.c b/s390x/smp.c
> > > index e5a16eb5a46a..5d3265f6be64 100644
> > > --- a/s390x/smp.c  
> [...]
> > > +/*
> > > + * We keep two structs, one for comparing when we want to assert
> > > it's not
> > > + * touched.
> > > + */
> > > +static uint8_t adtl_status[2][4096]
> > > __attribute__((aligned(4096)));  
> > 
> > it's a little bit ugly. maybe define a struct, with small buffers
> > inside
> > for the vector and gs areas? that way we would not need ugly magic
> > numbers below (see below)  
> 
> OK, will do.
> 
> [...]
> > > +static void restart_write_vector(void)
> > > +{
> > > +       uint8_t *vec_reg;
> > > +       uint8_t *vec_reg_16_31 = &expected_vec_contents[16][0];  
> > 
> > add a comment to explain that vlm only handles at most 16 registers
> > at
> > a time  
> 
> OK will do.
> 
> > > +       int i;
> > > +
> > > +       for (i = 0; i < NUM_VEC_REGISTERS; i++) {
> > > +               vec_reg = &expected_vec_contents[i][0];
> > > +               memset(vec_reg, i, VEC_REGISTER_SIZE);
> > > +       }  
> > 
> > this way vector register 0 stays 0.
> > either special case it (e.g. 16, or whatever), or put a magic value
> > somewhere in every register  
> 
> adtl_status is initalized with 0xff. Are you OK with i + 1 so we avoid
> zero?

that is fine

> 
> [...]
> > > +static void test_store_adtl_status_vector(void)
> > > +{
> > > +       uint32_t status = -1;
> > > +       struct psw psw;
> > > +       int cc;
> > > +
> > > +       report_prefix_push("store additional status vector");
> > > +
> > > +       if (!test_facility(129)) {
> > > +               report_skip("vector facility not installed");
> > > +               goto out;
> > > +       }
> > > +
> > > +       cpu_write_magic_to_vector_regs(1);
> > > +       smp_cpu_stop(1);
> > > +
> > > +       memset(adtl_status, 0xff, sizeof(adtl_status));
> > > +
> > > +       cc = smp_sigp(1, SIGP_STORE_ADDITIONAL_STATUS,
> > > +                 (unsigned long)adtl_status, &status);
> > > +       report(!cc, "CC = 0");
> > > +
> > > +       report(!memcmp(adtl_status, expected_vec_contents,
> > > sizeof(expected_vec_contents)),
> > > +              "additional status contents match");  
> > 
> > it would be interesting to check that nothing is stored past the end
> > of
> > the buffer.  
> 
> I will add checks to ensure reserved fields are not modified.
> 
> > moreover, I think you should also explicitly test with lc_10, to make
> > sure that works as well (no need to rerun the guest, just add another
> > sigp call)  
> 
> I will test vector with LC 0, 10, 11, 12 and guarded storage with LC 11
> and 12.
> 
> [...]
> > > +static void restart_write_gs_regs(void)
> > > +{
> > > +       const unsigned long gs_area = 0x2000000;
> > > +       const unsigned long gsc = 25; /* align = 32 M, section size
> > > = 512K */
> > > +
> > > +       ctl_set_bit(2, CTL2_GUARDED_STORAGE);
> > > +
> > > +       gs_cb.gsd = gs_area | gsc;
> > > +       gs_cb.gssm = 0xfeedc0ffe;
> > > +       gs_cb.gs_epl_a = (uint64_t) &gs_epl;
> > > +
> > > +       load_gs_cb(&gs_cb);
> > > +
> > > +       set_flag(1);
> > > +
> > > +       ctl_clear_bit(2, CTL2_GUARDED_STORAGE);  
> > 
> > what happens when the function returns? is r14 set up properly? (or
> > maybe we just don't care, since we are going to stop the CPU anyway?)  
> 
> We have an endless loop in smp_cpu_setup_state. So r14 will point there
> (verified with gdb).
> 
> In the end, I think we don't care. This is in contrast to the vector
> test, where the epilogue will clean up the floating point regs.

then add a comment explaining that :)

> 
> [...]
> > > +static void test_store_adtl_status_gs(void)
> > > +{
> > > +       const unsigned long adtl_status_lc_11 = 11;
> > > +       uint32_t status = 0;
> > > +       int cc;
> > > +
> > > +       report_prefix_push("store additional status guarded-
> > > storage");
> > > +
> > > +       if (!test_facility(133)) {
> > > +               report_skip("guarded-storage facility not
> > > installed");
> > > +               goto out;
> > > +       }
> > > +
> > > +       cpu_write_to_gs_regs(1);
> > > +       smp_cpu_stop(1);
> > > +
> > > +       memset(adtl_status, 0xff, sizeof(adtl_status));
> > > +
> > > +       cc = smp_sigp(1, SIGP_STORE_ADDITIONAL_STATUS,
> > > +                 (unsigned long)adtl_status | adtl_status_lc_11,
> > > &status);
> > > +       report(!cc, "CC = 0");
> > > +
> > > +       report(!memcmp(&adtl_status[0][1024], &gs_cb,
> > > sizeof(gs_cb)),  
> > 
> > e.g. the 1024 is one of those "magic number" I mentioned above   
> 
> OK, fixed.
> 
> >   
> > > +              "additional status contents match");  
> > 
> > it would be interesting to test that nothing is stored after the end
> > of
> > the buffer (i.e. everything is still 0xff in the second half of the
> > page)  
> 
> Yes, done.
> 
> [...]
> > > 
> > > diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> > > index 1600e714c8b9..2d0adc503917 100644
> > > --- a/s390x/unittests.cfg
> > > +++ b/s390x/unittests.cfg
> > > @@ -77,6 +77,12 @@ extra_params=-name kvm-unit-test --uuid
> > > 0fb84a86-727c-11ea-bc55-0242ac130003 -sm
> > >  [smp]
> > >  file = smp.elf
> > >  smp = 2
> > > +extra_params = -cpu host,gs=on,vx=on
> > > +
> > > +[smp-no-vec-no-gs]
> > > +file = smp.elf
> > > +smp = 2
> > > +extra_params = -cpu host,gs=off,vx=off  
> > 
> > using "host" will break TCG
> > (and using "qemu" will break secure execution)
> > 
> > there are two possible solutions:
> > 
> > use "max" and deal with the warnings, or split each testcase in two,
> > one using host cpu and "accel = kvm" and the other with "accel = tcg"
> > and qemu cpu.  
> 
> Uh, thanks for pointing out. I will split in accel = tcg and accel =
> kvm.
> 
> > what should happen if only one of the two features is installed?
> > should
> > the buffer for the unavailable feature be stored with 0 or should it
> > be
> > left untouched? is it worth testing those scenarios?  
> 
> The PoP specifies: "A facility’s registers are only
> stored in the MCESA when the facility is installed."
> 
> Maybe I miss something, but it doesn't seem worth it. It would mean
> adding yet another instance to the unittests.cfg. Since once needs to
> provide the memory for the registers even when the facility isn't
> there, there seems little risk for breaking something when we store if
> the facility isn't there.

I mean, technically we should check that nothing is stored for
facilities that are not present, but I guess it's not worth it 
and that would indeed double the number of entries in unittests.cfg