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

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

[Nico Boehr]

Changelog from v1:
* check reserved fields in store adtl status (thanks Claudio)
* check all possible length codes for vector and gs store adtl status (thanks
  Claudio)
* store adtl status: fix run under TCG
* store adtl status: avoid 0 in vector reg 0, use a struct instead of offsets
  for the mcesa, add some comments (thanks Claudio)

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         | 519 ++++++++++++++++++++++++++++++++++++++++++++
 s390x/stsi.c        |  42 +++-
 s390x/tprot.c       | 108 +++++++++
 s390x/unittests.cfg |  25 ++-
 7 files changed, 765 insertions(+), 75 deletions(-)
 create mode 100644 lib/s390x/gs.h
 create mode 100644 s390x/tprot.c

-- 
2.31.1

[kvm-unit-tests PATCH v2 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 v2 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 v2 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 v2 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         | 341 ++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |  22 ++-
 2 files changed, 362 insertions(+), 1 deletion(-)

diff --git a/s390x/smp.c b/s390x/smp.c
index e5a16eb5a46a..344f508a245d 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,37 @@ static const struct sigp_invalid_cases cases_valid_cpu_addr[] = {
 	{ INVALID_ORDER_CODE,         "invalid order code" },
 };
 
+struct mcesa_lc12 {
+	uint8_t vector_reg[0x200];            /* 0x000 */
+	uint8_t reserved200[0x400 - 0x200];   /* 0x200 */
+	struct gs_cb gs_cb;                   /* 0x400 */
+	uint8_t reserved420[0x800 - 0x420];   /* 0x420 */
+	uint8_t reserved800[0x1000 - 0x800];  /* 0x800 */
+};
+
+static struct mcesa_lc12 adtl_status __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 int memisset(void *s, int c, size_t n)
+{
+	uint8_t *p = s;
+	size_t i;
+
+	for (i = 0; i < n; i++) {
+		if (p[i] != c) {
+			return false;
+		}
+	}
+
+	return true;
+}
+
 static void test_invalid(void)
 {
 	const struct sigp_invalid_cases *c;
@@ -200,6 +232,311 @@ 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(memisset(&adtl_status, 0xff, sizeof(adtl_status)),
+	       "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(memisset(&adtl_status, 0xff, sizeof(adtl_status)),
+	       "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;
+	/*
+	 * vlm handles at most 16 registers at a time
+	 */
+	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];
+		/*
+		 * i+1 to avoid zero content
+		 */
+		memset(vec_reg, i + 1, 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 int adtl_status_check_unmodified_fields_for_lc(unsigned long lc)
+{
+	assert (!lc || (lc >= 10 && lc <= 12));
+
+	if (lc <= 10 && !memisset(&adtl_status.gs_cb, 0xff, sizeof(adtl_status.gs_cb)))
+		return false;
+
+	if (!memisset(adtl_status.reserved200, 0xff, sizeof(adtl_status.reserved200)))
+		return false;
+
+	if (!memisset(adtl_status.reserved420, 0xff, sizeof(adtl_status.reserved420)))
+		return false;
+	
+	if (!memisset(adtl_status.reserved800, 0xff, sizeof(adtl_status.reserved800)))
+		return false;
+
+	return true;
+}
+
+static void __store_adtl_status_vector_lc(unsigned long lc)
+{
+	uint32_t status = -1;
+	struct psw psw;
+	int cc;
+
+	report_prefix_pushf("LC %lu", lc);
+
+	if (!test_facility(133) && lc) {
+		report_skip("not supported, no guarded-storage facility");
+		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 | lc, &status);
+	report(!cc, "CC = 0");
+
+	report(!memcmp(adtl_status.vector_reg,
+		       expected_vec_contents, sizeof(expected_vec_contents)),
+	       "additional status contents match");
+
+	report(adtl_status_check_unmodified_fields_for_lc(lc),
+	       "no write outside expected fields");
+
+	/*
+	 * 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 test_store_adtl_status_vector(void)
+{
+	report_prefix_push("store additional status vector");
+
+	if (!test_facility(129)) {
+		report_skip("vector facility not installed");
+		goto out;
+	}
+
+	__store_adtl_status_vector_lc(0);
+	__store_adtl_status_vector_lc(10);
+	__store_adtl_status_vector_lc(11);
+	__store_adtl_status_vector_lc(12);
+
+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);
+
+	/*
+	 * Safe to return here. r14 will point to the endless loop in
+	 * smp_cpu_setup_state.
+	 */
+}
+
+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 __store_adtl_status_gs(unsigned long lc)
+{
+	uint32_t status = 0;
+	int cc;
+
+	report_prefix_pushf("LC %lu", lc);
+
+	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 | lc, &status);
+	report(!cc, "CC = 0");
+
+	report(!memcmp(&adtl_status.gs_cb, &gs_cb, sizeof(gs_cb)),
+	       "additional status contents match");
+
+	report(adtl_status_check_unmodified_fields_for_lc(lc),
+	       "no write outside expected fields");
+
+	report_prefix_pop();
+}
+
+static void test_store_adtl_status_gs(void)
+{
+	report_prefix_push("store additional status guarded-storage");
+
+	if (!test_facility(133)) {
+		report_skip("guarded-storage facility not installed");
+		goto out;
+	}
+
+	__store_adtl_status_gs(11);
+	__store_adtl_status_gs(12);
+
+out:
+	report_prefix_pop();
+}
+
 static void ecall(void)
 {
 	unsigned long mask;
@@ -388,6 +725,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..843fd323bce9 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -74,9 +74,29 @@ extra_params=-device diag288,id=watchdog0 --watchdog-action inject-nmi
 file = stsi.elf
 extra_params=-name kvm-unit-test --uuid 0fb84a86-727c-11ea-bc55-0242ac130003 -smp 1,maxcpus=8
 
-[smp]
+[smp-kvm]
 file = smp.elf
 smp = 2
+accel = kvm
+extra_params = -cpu host,gs=on,vx=on
+
+[smp-no-vec-no-gs-kvm]
+file = smp.elf
+smp = 2
+accel = kvm
+extra_params = -cpu host,gs=off,vx=off
+
+[smp-tcg]
+file = smp.elf
+smp = 2
+accel = tcg
+extra_params = -cpu qemu,vx=on
+
+[smp-no-vec-no-gs-tcg]
+file = smp.elf
+smp = 2
+accel = tcg
+extra_params = -cpu qemu,gs=off,vx=off
 
 [sclp-1g]
 file = sclp.elf
-- 
2.31.1

[kvm-unit-tests PATCH v2 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 344f508a245d..36014e809f2e 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -55,6 +55,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 int memisset(void *s, int c, size_t n)
 {
 	uint8_t *p = s;
@@ -537,6 +539,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;
@@ -729,6 +801,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 v2 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 36014e809f2e..6e67bea72b75 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -662,6 +662,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;
 
@@ -674,6 +675,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 v2 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 6e67bea72b75..96935dd6c1ac 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>
 
@@ -686,6 +687,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");
@@ -813,6 +855,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 v2 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 843fd323bce9..bcf223cd7365 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -156,3 +156,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 v2 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 v2 4/9] s390x: smp: add test for SIGP_STORE_ADTL_STATUS order

[Claudio Imbrenda]

On Wed, 23 Mar 2022 18:03:20 +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         | 341 ++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg |  22 ++-
>  2 files changed, 362 insertions(+), 1 deletion(-)
> 
> diff --git a/s390x/smp.c b/s390x/smp.c
> index e5a16eb5a46a..344f508a245d 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,37 @@ static const struct sigp_invalid_cases cases_valid_cpu_addr[] = {
>  	{ INVALID_ORDER_CODE,         "invalid order code" },
>  };
>  
> +struct mcesa_lc12 {
> +	uint8_t vector_reg[0x200];            /* 0x000 */
> +	uint8_t reserved200[0x400 - 0x200];   /* 0x200 */
> +	struct gs_cb gs_cb;                   /* 0x400 */
> +	uint8_t reserved420[0x800 - 0x420];   /* 0x420 */
> +	uint8_t reserved800[0x1000 - 0x800];  /* 0x800 */
> +};
> +
> +static struct mcesa_lc12 adtl_status __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 int memisset(void *s, int c, size_t n)

function should return bool..

> +{
> +	uint8_t *p = s;
> +	size_t i;
> +
> +	for (i = 0; i < n; i++) {
> +		if (p[i] != c) {
> +			return false;
> +		}
> +	}
> +
> +	return true;

..especially since you return true and false

> +}
> +
>  static void test_invalid(void)
>  {
>  	const struct sigp_invalid_cases *c;
> @@ -200,6 +232,311 @@ 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(memisset(&adtl_status, 0xff, sizeof(adtl_status)),
> +	       "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(memisset(&adtl_status, 0xff, sizeof(adtl_status)),
> +	       "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");

and check again that nothing has been written to

> +
> +	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");
> +

I would still check that nothing is written even when the order is
rejected

> +	report_prefix_pop();
> +
> +out:
> +	report_prefix_pop();
> +}
> +
> +static void restart_write_vector(void)
> +{
> +	uint8_t *vec_reg;
> +	/*
> +	 * vlm handles at most 16 registers at a time
> +	 */

this comment can /* go on a single line */

> +	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];
> +		/*
> +		 * i+1 to avoid zero content
> +		 */

same /* here */

> +		memset(vec_reg, i + 1, 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 int adtl_status_check_unmodified_fields_for_lc(unsigned long lc)
> +{
> +	assert (!lc || (lc >= 10 && lc <= 12));
> +
> +	if (lc <= 10 && !memisset(&adtl_status.gs_cb, 0xff, sizeof(adtl_status.gs_cb)))
> +		return false;
> +
> +	if (!memisset(adtl_status.reserved200, 0xff, sizeof(adtl_status.reserved200)))
> +		return false;
> +
> +	if (!memisset(adtl_status.reserved420, 0xff, sizeof(adtl_status.reserved420)))
> +		return false;
> +	
> +	if (!memisset(adtl_status.reserved800, 0xff, sizeof(adtl_status.reserved800)))
> +		return false;
> +
> +	return true;
> +}
> +
> +static void __store_adtl_status_vector_lc(unsigned long lc)
> +{
> +	uint32_t status = -1;
> +	struct psw psw;
> +	int cc;
> +
> +	report_prefix_pushf("LC %lu", lc);
> +
> +	if (!test_facility(133) && lc) {
> +		report_skip("not supported, no guarded-storage facility");
> +		goto out;
> +	}

I think this ^ should not be there at all

> +
> +	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 | lc, &status);
> +	report(!cc, "CC = 0");
> +
> +	report(!memcmp(adtl_status.vector_reg,
> +		       expected_vec_contents, sizeof(expected_vec_contents)),
> +	       "additional status contents match");
> +
> +	report(adtl_status_check_unmodified_fields_for_lc(lc),
> +	       "no write outside expected fields");
> +
> +	/*
> +	 * 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 test_store_adtl_status_vector(void)
> +{
> +	report_prefix_push("store additional status vector");
> +
> +	if (!test_facility(129)) {
> +		report_skip("vector facility not installed");
> +		goto out;
> +	}
> +
> +	__store_adtl_status_vector_lc(0);
> +	__store_adtl_status_vector_lc(10);
> +	__store_adtl_status_vector_lc(11);
> +	__store_adtl_status_vector_lc(12);
> +
> +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);
> +
> +	/*
> +	 * Safe to return here. r14 will point to the endless loop in
> +	 * smp_cpu_setup_state.
> +	 */
> +}
> +
> +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 __store_adtl_status_gs(unsigned long lc)
> +{
> +	uint32_t status = 0;
> +	int cc;
> +
> +	report_prefix_pushf("LC %lu", lc);
> +
> +	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 | lc, &status);
> +	report(!cc, "CC = 0");
> +
> +	report(!memcmp(&adtl_status.gs_cb, &gs_cb, sizeof(gs_cb)),
> +	       "additional status contents match");
> +
> +	report(adtl_status_check_unmodified_fields_for_lc(lc),
> +	       "no write outside expected fields");
> +
> +	report_prefix_pop();
> +}
> +
> +static void test_store_adtl_status_gs(void)
> +{
> +	report_prefix_push("store additional status guarded-storage");
> +
> +	if (!test_facility(133)) {
> +		report_skip("guarded-storage facility not installed");
> +		goto out;
> +	}
> +
> +	__store_adtl_status_gs(11);
> +	__store_adtl_status_gs(12);
> +
> +out:
> +	report_prefix_pop();
> +}
> +
>  static void ecall(void)
>  {
>  	unsigned long mask;
> @@ -388,6 +725,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..843fd323bce9 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -74,9 +74,29 @@ extra_params=-device diag288,id=watchdog0 --watchdog-action inject-nmi
>  file = stsi.elf
>  extra_params=-name kvm-unit-test --uuid 0fb84a86-727c-11ea-bc55-0242ac130003 -smp 1,maxcpus=8
>  
> -[smp]
> +[smp-kvm]
>  file = smp.elf
>  smp = 2
> +accel = kvm
> +extra_params = -cpu host,gs=on,vx=on
> +
> +[smp-no-vec-no-gs-kvm]
> +file = smp.elf
> +smp = 2
> +accel = kvm
> +extra_params = -cpu host,gs=off,vx=off
> +
> +[smp-tcg]
> +file = smp.elf
> +smp = 2
> +accel = tcg
> +extra_params = -cpu qemu,vx=on

why not gs=on as well?

> +
> +[smp-no-vec-no-gs-tcg]
> +file = smp.elf
> +smp = 2
> +accel = tcg
> +extra_params = -cpu qemu,gs=off,vx=off
>  
>  [sclp-1g]
>  file = sclp.elf

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

[Nico Boehr]

On Wed, 2022-03-23 at 18:45 +0100, Claudio Imbrenda wrote:
> On Wed, 23 Mar 2022 18:03:20 +0100
> Nico Boehr <nrb@linux.ibm.com> wrote:
> 
[...]
> > +
> > +static int memisset(void *s, int c, size_t n)
> 
> function should return bool..

Sure, changed.

[...]
> > +static void test_store_adtl_status(void)
> > +{
> > 
[...]
> > +
> > +       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");
> 
> and check again that nothing has been written to

Oh, thanks. Fixed.

[...]
> > +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");
> > +
> 
> I would still check that nothing is written even when the order is
> rejected

Won't hurt, added.

[...]
> > +static void restart_write_vector(void)
> > +{
> > +       uint8_t *vec_reg;
> > +       /*
> > +        * vlm handles at most 16 registers at a time
> > +        */
> 
> this comment can /* go on a single line */

OK

[...]
> > +               /*
> > +                * i+1 to avoid zero content
> > +                */
> 
> same /* here */

OK, changed.

[...]
> > +static void __store_adtl_status_vector_lc(unsigned long lc)
> > +{
> > +       uint32_t status = -1;
> > +       struct psw psw;
> > +       int cc;
> > +
> > +       report_prefix_pushf("LC %lu", lc);
> > +
> > +       if (!test_facility(133) && lc) {
> > +               report_skip("not supported, no guarded-storage
> > facility");
> > +               goto out;
> > +       }
> 
> I think this ^ should not be there at all

It must be. If we don't have guarded-storage only LC 0 is allowed:

"When the guarded-storage facility is not installed, the
length and alignment of the MCESA is 1024 bytes.
When the guarded-storage facility is installed, the
length characteristic (LC) in bits 60-63 of the
MCESAD specifies the length and alignment of the
MCESA as a power of two"

See below for the reason why we don't have gs here.

[...]
> > diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> > index 1600e714c8b9..843fd323bce9 100644
> > --- a/s390x/unittests.cfg
> > +++ b/s390x/unittests.cfg
> > @@ -74,9 +74,29 @@ extra_params=-device diag288,id=watchdog0 --
> > watchdog-action inject-nmi
> >  file = stsi.elf
> >  extra_params=-name kvm-unit-test --uuid 0fb84a86-727c-11ea-bc55-
> > 0242ac130003 -smp 1,maxcpus=8
> >  
> > -[smp]
> > +[smp-kvm]
> >  file = smp.elf
> >  smp = 2
> > +accel = kvm
> > +extra_params = -cpu host,gs=on,vx=on
> > +
> > +[smp-no-vec-no-gs-kvm]
> > +file = smp.elf
> > +smp = 2
> > +accel = kvm
> > +extra_params = -cpu host,gs=off,vx=off
> > +
> > +[smp-tcg]
> > +file = smp.elf
> > +smp = 2
> > +accel = tcg
> > +extra_params = -cpu qemu,vx=on
> 
> why not gs=on as well?

I am not an expert in QEMU CPU model, but it seems to me TCG doesn't
support it.

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

[Claudio Imbrenda]

On Thu, 24 Mar 2022 08:39:29 +0100
Nico Boehr <nrb@linux.ibm.com> wrote:

> On Wed, 2022-03-23 at 18:45 +0100, Claudio Imbrenda wrote:
> > On Wed, 23 Mar 2022 18:03:20 +0100
> > Nico Boehr <nrb@linux.ibm.com> wrote:
> >   
> [...]
> > > +
> > > +static int memisset(void *s, int c, size_t n)  
> > 
> > function should return bool..  
> 
> Sure, changed.
> 
> [...]
> > > +static void test_store_adtl_status(void)
> > > +{
> > >   
> [...]
> > > +
> > > +       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");  
> > 
> > and check again that nothing has been written to  
> 
> Oh, thanks. Fixed.
> 
> [...]
> > > +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");
> > > +  
> > 
> > I would still check that nothing is written even when the order is
> > rejected  
> 
> Won't hurt, added.
> 
> [...]
> > > +static void restart_write_vector(void)
> > > +{
> > > +       uint8_t *vec_reg;
> > > +       /*
> > > +        * vlm handles at most 16 registers at a time
> > > +        */  
> > 
> > this comment can /* go on a single line */  
> 
> OK
> 
> [...]
> > > +               /*
> > > +                * i+1 to avoid zero content
> > > +                */  
> > 
> > same /* here */  
> 
> OK, changed.
> 
> [...]
> > > +static void __store_adtl_status_vector_lc(unsigned long lc)
> > > +{
> > > +       uint32_t status = -1;
> > > +       struct psw psw;
> > > +       int cc;
> > > +
> > > +       report_prefix_pushf("LC %lu", lc);
> > > +
> > > +       if (!test_facility(133) && lc) {
> > > +               report_skip("not supported, no guarded-storage
> > > facility");
> > > +               goto out;
> > > +       }  
> > 
> > I think this ^ should not be there at all  
> 
> It must be. If we don't have guarded-storage only LC 0 is allowed:
> 
> "When the guarded-storage facility is not installed, the
> length and alignment of the MCESA is 1024 bytes.
> When the guarded-storage facility is installed, the
> length characteristic (LC) in bits 60-63 of the
> MCESAD specifies the length and alignment of the
> MCESA as a power of two"

hmm, it seems like that without guarded storage LC is ignored, and the
size is hardcoded to 1024.

this is getting a little out of hand now

I think you should make this into a separate test

> 
> See below for the reason why we don't have gs here.
> 
> [...]
> > > diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> > > index 1600e714c8b9..843fd323bce9 100644
> > > --- a/s390x/unittests.cfg
> > > +++ b/s390x/unittests.cfg
> > > @@ -74,9 +74,29 @@ extra_params=-device diag288,id=watchdog0 --
> > > watchdog-action inject-nmi
> > >  file = stsi.elf
> > >  extra_params=-name kvm-unit-test --uuid 0fb84a86-727c-11ea-bc55-
> > > 0242ac130003 -smp 1,maxcpus=8
> > >  
> > > -[smp]
> > > +[smp-kvm]
> > >  file = smp.elf
> > >  smp = 2
> > > +accel = kvm
> > > +extra_params = -cpu host,gs=on,vx=on
> > > +
> > > +[smp-no-vec-no-gs-kvm]
> > > +file = smp.elf
> > > +smp = 2
> > > +accel = kvm
> > > +extra_params = -cpu host,gs=off,vx=off
> > > +
> > > +[smp-tcg]
> > > +file = smp.elf
> > > +smp = 2
> > > +accel = tcg
> > > +extra_params = -cpu qemu,vx=on  
> > 
> > why not gs=on as well?  
> 
> I am not an expert in QEMU CPU model, but it seems to me TCG doesn't
> support it.

it seems indeed so. maybe add a comment to explain

> 

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

[Heiko Carstens]

On Wed, Mar 23, 2022 at 06:03:19PM +0100, Nico Boehr wrote:
...
> +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;
> +}

It was like that before, but why is r14 within the clobber list?
That doesn't make sense.

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

[Janosch Frank]

On 3/24/22 18:40, Heiko Carstens wrote:
> On Wed, Mar 23, 2022 at 06:03:19PM +0100, Nico Boehr wrote:
> ...
>> +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;
>> +}
> 
> It was like that before, but why is r14 within the clobber list?
> That doesn't make sense.

r14 is changed in the gs handler of the gs test which is executed if the 
"guarded" part of the load takes place.

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

[Nico Boehr]

On Thu, 2022-03-24 at 14:03 +0100, Claudio Imbrenda wrote:
> hmm, it seems like that without guarded storage LC is ignored, and
> the
> size is hardcoded to 1024.
> 
> this is getting a little out of hand now
> 
> I think you should make this into a separate test

Yes, I think that makes a lot of sense. I will factor out the store
adtl status test into its own file and send in a seperate series.

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

[Claudio Imbrenda]

On Fri, 25 Mar 2022 08:29:11 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 3/24/22 18:40, Heiko Carstens wrote:
> > On Wed, Mar 23, 2022 at 06:03:19PM +0100, Nico Boehr wrote:
> > ...  
> >> +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;
> >> +}  
> > 
> > It was like that before, but why is r14 within the clobber list?
> > That doesn't make sense.  
> 
> r14 is changed in the gs handler of the gs test which is executed if the 
> "guarded" part of the load takes place.

I will add a comment explaining that when picking the patch

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

[Janosch Frank]

On 3/25/22 15:30, Claudio Imbrenda wrote:
> On Fri, 25 Mar 2022 08:29:11 +0100
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> On 3/24/22 18:40, Heiko Carstens wrote:
>>> On Wed, Mar 23, 2022 at 06:03:19PM +0100, Nico Boehr wrote:
>>> ...
>>>> +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;
>>>> +}
>>>
>>> It was like that before, but why is r14 within the clobber list?
>>> That doesn't make sense.
>>
>> r14 is changed in the gs handler of the gs test which is executed if the
>> "guarded" part of the load takes place.
> 
> I will add a comment explaining that when picking the patch

Do we need load_guarded() in this new header?
The load/store_gscb() functions have potential to be shared across tests 
but the lg doesn't need to be executed, no?

We could opt to leave it in gs.c instead

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

[Claudio Imbrenda]

On Fri, 25 Mar 2022 16:07:47 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 3/25/22 15:30, Claudio Imbrenda wrote:
> > On Fri, 25 Mar 2022 08:29:11 +0100
> > Janosch Frank <frankja@linux.ibm.com> wrote:
> >   
> >> On 3/24/22 18:40, Heiko Carstens wrote:  
> >>> On Wed, Mar 23, 2022 at 06:03:19PM +0100, Nico Boehr wrote:
> >>> ...  
> >>>> +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;
> >>>> +}  
> >>>
> >>> It was like that before, but why is r14 within the clobber list?
> >>> That doesn't make sense.  
> >>
> >> r14 is changed in the gs handler of the gs test which is executed if the
> >> "guarded" part of the load takes place.  
> > 
> > I will add a comment explaining that when picking the patch  
> 
> Do we need load_guarded() in this new header?
> The load/store_gscb() functions have potential to be shared across tests 
> but the lg doesn't need to be executed, no?
> 
> We could opt to leave it in gs.c instead

yes, probably a better idea. I'd still add the comment, though :)

shall I just fix this up when picking?

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

[Nico Boehr]

On Fri, 2022-03-25 at 17:38 +0100, Claudio Imbrenda wrote:
> > Do we need load_guarded() in this new header?
> > The load/store_gscb() functions have potential to be shared across
> > tests 
> > but the lg doesn't need to be executed, no?
> > 
> > We could opt to leave it in gs.c instead
> 
> yes, probably a better idea. I'd still add the comment, though :)
> 
> shall I just fix this up when picking?

I like the suggestion by Janosch.

Since I will sent the ADTL STATUS test (patch 4) in a seperate series
anyways, I think it makes more sense if I include the patch there and
you don't pick patches 3 and 4 from this series.