[PATCH 0/2] s390x: Add tests for SIGP store adtl status

[PATCH 0/2] s390x: Add tests for SIGP store adtl status

[Nico Boehr]

As suggested by Claudio, move the store adtl status I sent previously
("[kvm-unit-tests PATCH v2 0/9] s390x: Further extend instruction interception
 tests") into its own file.

Nico Boehr (2):
  s390x: gs: move to new header file
  s390x: add test for SIGP STORE_ADTL_STATUS order

 lib/s390x/gs.h      |  69 ++++++++
 s390x/Makefile      |   1 +
 s390x/adtl_status.c | 407 ++++++++++++++++++++++++++++++++++++++++++++
 s390x/gs.c          |  54 +-----
 s390x/unittests.cfg |  25 +++
 5 files changed, 503 insertions(+), 53 deletions(-)
 create mode 100644 lib/s390x/gs.h
 create mode 100644 s390x/adtl_status.c

-- 
2.31.1

[PATCH 1/2] 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.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 lib/s390x/gs.h | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++
 s390x/gs.c     | 54 +--------------------------------------
 2 files changed, 70 insertions(+), 53 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..9c94e580b4b9
--- /dev/null
+++ b/lib/s390x/gs.h
@@ -0,0 +1,69 @@
+/* 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));
+}
+
+#endif
diff --git a/s390x/gs.c b/s390x/gs.c
index 7567bb78fecb..4993eb8f43a9 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,16 +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;
-- 
2.31.1

[PATCH 2/2] s390x: 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
test with both guarded-storage and vector facility off and on. In TCG, we don't
have guarded-storage support, so we just run with vector facility off and on.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 s390x/Makefile      |   1 +
 s390x/adtl_status.c | 407 ++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |  25 +++
 3 files changed, 433 insertions(+)
 create mode 100644 s390x/adtl_status.c

diff --git a/s390x/Makefile b/s390x/Makefile
index 53b0fe044fe7..47e915fbdc51 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -26,6 +26,7 @@ tests += $(TEST_DIR)/edat.elf
 tests += $(TEST_DIR)/mvpg-sie.elf
 tests += $(TEST_DIR)/spec_ex-sie.elf
 tests += $(TEST_DIR)/firq.elf
+tests += $(TEST_DIR)/adtl_status.elf
 
 pv-tests += $(TEST_DIR)/pv-diags.elf
 
diff --git a/s390x/adtl_status.c b/s390x/adtl_status.c
new file mode 100644
index 000000000000..7a2bd2b07804
--- /dev/null
+++ b/s390x/adtl_status.c
@@ -0,0 +1,407 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Tests sigp store additional status order
+ *
+ * Copyright IBM Corp. 2022
+ *
+ * Authors:
+ *    Nico Boehr <nrb@linux.ibm.com>
+ */
+#include <libcflat.h>
+#include <asm/asm-offsets.h>
+#include <asm/interrupt.h>
+#include <asm/page.h>
+#include <asm/facility.h>
+#include <asm-generic/barrier.h>
+#include <asm/sigp.h>
+
+#include <smp.h>
+#include <gs.h>
+#include <alloc_page.h>
+
+static int testflag = 0;
+
+#define INVALID_CPU_ADDRESS -4711
+
+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 bool 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 wait_for_flag(void)
+{
+	while (!testflag)
+		mb();
+}
+
+static void set_flag(int val)
+{
+	mb();
+	testflag = val;
+	mb();
+}
+
+static void test_func(void)
+{
+	set_flag(1);
+}
+
+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(memisset(&adtl_status, 0xff, sizeof(adtl_status)),
+	       "additional status not touched");
+
+	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);
+
+	memset(&adtl_status, 0xff, sizeof(adtl_status));
+
+	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(memisset(&adtl_status, 0xff, sizeof(adtl_status)),
+	       "additional status not touched");
+
+	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();
+}
+
+int main(void)
+{
+	struct psw psw;
+	report_prefix_push("adtl_status");
+
+	if (smp_query_num_cpus() == 1) {
+		report_skip("need at least 2 cpus for this test");
+		goto done;
+	}
+
+	/* Setting up the cpu to give it a stack and lowcore */
+	psw.mask = extract_psw_mask();
+	psw.addr = (unsigned long)test_func;
+	smp_cpu_setup(1, psw);
+	smp_cpu_stop(1);
+
+	test_store_adtl_status_unavail();
+	test_store_adtl_status_vector();
+	test_store_adtl_status_gs();
+	test_store_adtl_status();
+	smp_cpu_destroy(1);
+
+done:
+	report_prefix_pop();
+	return report_summary();
+}
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index 1600e714c8b9..2e65106fa140 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -78,6 +78,31 @@ extra_params=-name kvm-unit-test --uuid 0fb84a86-727c-11ea-bc55-0242ac130003 -sm
 file = smp.elf
 smp = 2
 
+[adtl_status-kvm]
+file = adtl_status.elf
+smp = 2
+accel = kvm
+extra_params = -cpu host,gs=on,vx=on
+
+[adtl_status-no-vec-no-gs-kvm]
+file = adtl_status.elf
+smp = 2
+accel = kvm
+extra_params = -cpu host,gs=off,vx=off
+
+[adtl_status-tcg]
+file = adtl_status.elf
+smp = 2
+accel = tcg
+# no guarded-storage support in tcg
+extra_params = -cpu qemu,vx=on
+
+[adtl_status-no-vec-no-gs-tcg]
+file = adtl_status.elf
+smp = 2
+accel = tcg
+extra_params = -cpu qemu,gs=off,vx=off
+
 [sclp-1g]
 file = sclp.elf
 extra_params = -m 1G
-- 
2.31.1

Re: [PATCH 2/2] s390x: add test for SIGP STORE_ADTL_STATUS order

[Janosch Frank]

On 3/28/22 11:30, Nico Boehr 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
> test with both guarded-storage and vector facility off and on. In TCG, we don't
> have guarded-storage support, so we just run with vector facility off and on.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>   s390x/Makefile      |   1 +
>   s390x/adtl_status.c | 407 ++++++++++++++++++++++++++++++++++++++++++++
>   s390x/unittests.cfg |  25 +++
>   3 files changed, 433 insertions(+)
>   create mode 100644 s390x/adtl_status.c
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 53b0fe044fe7..47e915fbdc51 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -26,6 +26,7 @@ tests += $(TEST_DIR)/edat.elf
>   tests += $(TEST_DIR)/mvpg-sie.elf
>   tests += $(TEST_DIR)/spec_ex-sie.elf
>   tests += $(TEST_DIR)/firq.elf
> +tests += $(TEST_DIR)/adtl_status.elf
>   
>   pv-tests += $(TEST_DIR)/pv-diags.elf
>   
> diff --git a/s390x/adtl_status.c b/s390x/adtl_status.c
> new file mode 100644
> index 000000000000..7a2bd2b07804
> --- /dev/null
> +++ b/s390x/adtl_status.c
> @@ -0,0 +1,407 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Tests sigp store additional status order
> + *
> + * Copyright IBM Corp. 2022
> + *
> + * Authors:
> + *    Nico Boehr <nrb@linux.ibm.com>
> + */
> +#include <libcflat.h>
> +#include <asm/asm-offsets.h>
> +#include <asm/interrupt.h>
> +#include <asm/page.h>
> +#include <asm/facility.h>
> +#include <asm-generic/barrier.h>
> +#include <asm/sigp.h>
> +
> +#include <smp.h>
> +#include <gs.h>
> +#include <alloc_page.h>
> +
> +static int testflag = 0;
> +
> +#define INVALID_CPU_ADDRESS -4711
> +
> +struct mcesa_lc12 {
> +	uint8_t vector_reg[0x200];            /* 0x000 */

Hrm we could do:
__uint128_t vregs[32];

or:
uint64_t vregs[16][2];

or leave it as it is.

> +	uint8_t reserved200[0x400 - 0x200];   /* 0x200 */
> +	struct gs_cb gs_cb;                   /* 0x400 */
> +	uint8_t reserved420[0x800 - 0x420];   /* 0x420 */
> +	uint8_t reserved800[0x1000 - 0x800];  /* 0x800 */
> +};

Do we have plans to use this struct in the future for other tests?

> +
> +static struct mcesa_lc12 adtl_status __attribute__((aligned(4096)));
> +
> +#define NUM_VEC_REGISTERS 32
> +#define VEC_REGISTER_SIZE 16

I'd shove that into lib/s390x/asm/float.h or create a vector.h as
#define VEC_REGISTERS_NUM 32
#define VEC_REGISTERS_SIZE 16

Most likely vector.h since we can do both int and float with vector regs.

> +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 bool 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 wait_for_flag(void)
> +{
> +	while (!testflag)
> +		mb();
> +}
> +
> +static void set_flag(int val)
> +{
> +	mb();
> +	testflag = val;
> +	mb();
> +}
> +
> +static void test_func(void)
> +{
> +	set_flag(1);
> +}
> +
> +static int have_adtl_status(void)

bool

> +{
> +	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(memisset(&adtl_status, 0xff, sizeof(adtl_status)),
> +	       "additional status not touched");
> +
> +	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");

unavailable

> +
> +	if (have_adtl_status()) {
> +		report_skip("guarded-storage or vector facility installed");
> +		goto out;
> +	}
> +
> +	report_prefix_push("not accepted");
> +	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 == 1, "CC = 1");
> +	report(status == SIGP_STATUS_INVALID_ORDER,
> +	       "status = INVALID_ORDER");
> +	report(memisset(&adtl_status, 0xff, sizeof(adtl_status)),
> +	       "additional status not touched");
> +
> +	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"

We change memory on a load?

> +	);

We could also move vlm as a function to vector.h and do two calls.

[...]
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index 1600e714c8b9..2e65106fa140 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -78,6 +78,31 @@ extra_params=-name kvm-unit-test --uuid 0fb84a86-727c-11ea-bc55-0242ac130003 -sm
>   file = smp.elf
>   smp = 2
>   
> +[adtl_status-kvm]

Hmmmmm (TM) I don't really want to mix - and _.
Having spec_ex-sie.c is already bad enough.

> +file = adtl_status.elf
> +smp = 2
> +accel = kvm
> +extra_params = -cpu host,gs=on,vx=on
> +
> +[adtl_status-no-vec-no-gs-kvm]
> +file = adtl_status.elf
> +smp = 2
> +accel = kvm
> +extra_params = -cpu host,gs=off,vx=off
> +
> +[adtl_status-tcg]
> +file = adtl_status.elf
> +smp = 2
> +accel = tcg
> +# no guarded-storage support in tcg
> +extra_params = -cpu qemu,vx=on
> +
> +[adtl_status-no-vec-no-gs-tcg]
> +file = adtl_status.elf
> +smp = 2
> +accel = tcg
> +extra_params = -cpu qemu,gs=off,vx=off
> +

Are you trying to sort this in any way?
Normally we put new entries at the EOF.

>   [sclp-1g]
>   file = sclp.elf
>   extra_params = -m 1G

Re: [PATCH 2/2] s390x: add test for SIGP STORE_ADTL_STATUS order

[Nico Boehr]

On Mon, 2022-03-28 at 13:54 +0200, Janosch Frank wrote:
> > diff --git a/s390x/adtl_status.c b/s390x/adtl_status.c
> > new file mode 100644
> > index 000000000000..7a2bd2b07804
> > --- /dev/null
> > +++ b/s390x/adtl_status.c
[...]
> > +struct mcesa_lc12 {
> > +       uint8_t vector_reg[0x200];            /* 0x000 */
> 
> Hrm we could do:
> __uint128_t vregs[32];
> 
> or:
> uint64_t vregs[16][2];
> 
> or leave it as it is.

No strong preference about the type. uint8_t makes it easy to check the
offsets.

> 
> > +       uint8_t reserved200[0x400 - 0x200];   /* 0x200 */
> > +       struct gs_cb gs_cb;                   /* 0x400 */
> > +       uint8_t reserved420[0x800 - 0x420];   /* 0x420 */
> > +       uint8_t reserved800[0x1000 - 0x800];  /* 0x800 */
> > +};
> 
> Do we have plans to use this struct in the future for other tests?

Maybe at some point if we add checks for machine check handling, but
right now we don't have the infrastructure in kvm-unit-tests to do that
I think.

> 
> > +
> > +static struct mcesa_lc12 adtl_status
> > __attribute__((aligned(4096)));
> > +
> > +#define NUM_VEC_REGISTERS 32
> > +#define VEC_REGISTER_SIZE 16
> 
> I'd shove that into lib/s390x/asm/float.h or create a vector.h as
> #define VEC_REGISTERS_NUM 32
> #define VEC_REGISTERS_SIZE 16
> 
> Most likely vector.h since we can do both int and float with vector
> regs.

OK, will do.

[...]
> > +
> > +static int have_adtl_status(void)
> 
> bool

Changed.

[...]
> > +static void test_store_adtl_status_unavail(void)
> > +{
> > +       uint32_t status = 0;
> > +       int cc;
> > +
> > +       report_prefix_push("store additional status unvailable");
> 
> unavailable

Thanks.

[...]
> > +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"
> 
> We change memory on a load?

To my understanding, this might be neccesary if expected_vec_contents
ends up in a register, but that won't happen, so I can remove it.

> 
> > +       );
> 
> We could also move vlm as a function to vector.h and do two calls.

I think that won't work because that function might clean its float
registers in the epilogue and hence destroy the contents. Except if you
have an idea on how to avoid that?

[...]
> > diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> > index 1600e714c8b9..2e65106fa140 100644
> > --- a/s390x/unittests.cfg
> > +++ b/s390x/unittests.cfg
> > @@ -78,6 +78,31 @@ extra_params=-name kvm-unit-test --uuid
> > 0fb84a86-727c-11ea-bc55-0242ac130003 -sm
> >   file = smp.elf
> >   smp = 2
> >   
> > +[adtl_status-kvm]
> 
> Hmmmmm (TM) I don't really want to mix - and _.
> Having spec_ex-sie.c is already bad enough.

Yes, thanks.

> 
> > +file = adtl_status.elf
> > +smp = 2
> > +accel = kvm
> > +extra_params = -cpu host,gs=on,vx=on
> > +
> > +[adtl_status-no-vec-no-gs-kvm]
> > +file = adtl_status.elf
> > +smp = 2
> > +accel = kvm
> > +extra_params = -cpu host,gs=off,vx=off
> > +
> > +[adtl_status-tcg]
> > +file = adtl_status.elf
> > +smp = 2
> > +accel = tcg
> > +# no guarded-storage support in tcg
> > +extra_params = -cpu qemu,vx=on
> > +
> > +[adtl_status-no-vec-no-gs-tcg]
> > +file = adtl_status.elf
> > +smp = 2
> > +accel = tcg
> > +extra_params = -cpu qemu,gs=off,vx=off
> > +
> 
> Are you trying to sort this in any way?
> Normally we put new entries at the EOF.

Oh, this was a leftover from when this was still part of the smp test,
moved to the end now.

Re: [PATCH 2/2] s390x: add test for SIGP STORE_ADTL_STATUS order

[Janosch Frank]

On 3/28/22 17:12, Nico Boehr wrote:
> On Mon, 2022-03-28 at 13:54 +0200, Janosch Frank wrote:
>>> diff --git a/s390x/adtl_status.c b/s390x/adtl_status.c
>>> new file mode 100644
>>> index 000000000000..7a2bd2b07804
>>> --- /dev/null
>>> +++ b/s390x/adtl_status.c
> [...]
>>> +struct mcesa_lc12 {
>>> +       uint8_t vector_reg[0x200];            /* 0x000 */
>>
>> Hrm we could do:
>> __uint128_t vregs[32];
>>
>> or:
>> uint64_t vregs[16][2];
>>
>> or leave it as it is.
> 
> No strong preference about the type. uint8_t makes it easy to check the
> offsets.
> 
>>
>>> +       uint8_t reserved200[0x400 - 0x200];   /* 0x200 */
>>> +       struct gs_cb gs_cb;                   /* 0x400 */
>>> +       uint8_t reserved420[0x800 - 0x420];   /* 0x420 */
>>> +       uint8_t reserved800[0x1000 - 0x800];  /* 0x800 */
>>> +};
>>
>> Do we have plans to use this struct in the future for other tests?
> 
> Maybe at some point if we add checks for machine check handling, but
> right now we don't have the infrastructure in kvm-unit-tests to do that
> I think.

Alright, then let's leave the struct in here for now.

[...]

>>> +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"
>>
>> We change memory on a load?
> 
> To my understanding, this might be neccesary if expected_vec_contents
> ends up in a register, but that won't happen, so I can remove it.
> 
>>
>>> +       );
>>
>> We could also move vlm as a function to vector.h and do two calls.
> 
> I think that won't work because that function might clean its float
> registers in the epilogue and hence destroy the contents. Except if you
> have an idea on how to avoid that?

About that:

Well, who guarantees you that the compiler won't change a fpr (and 
thereby the overlapped vrs) between the vlms here and your infinite loop 
at the end of the function? :-) gcc uses fprs and acrs in the most 
interesting places and I've just been hit by that again a few hours ago.

I.e. to be safe we'll need to implement the next few lines in assembly 
as well, no?

Re: [PATCH 2/2] s390x: add test for SIGP STORE_ADTL_STATUS order

[Nico Boehr]

On Tue, 2022-03-29 at 13:39 +0200, Janosch Frank wrote:
> > I think that won't work because that function might clean its float
> > registers in the epilogue and hence destroy the contents. Except if
> > you
> > have an idea on how to avoid that?

I missed that this is a theoretical problem because the function will
be inlined anyway.

> About that:
> 
> Well, who guarantees you that the compiler won't change a fpr (and 
> thereby the overlapped vrs) between the vlms here and your infinite
> loop 
> at the end of the function? :-) gcc uses fprs and acrs in the most 
> interesting places and I've just been hit by that again a few hours
> ago.
> 
> I.e. to be safe we'll need to implement the next few lines in
> assembly 
> as well, no?

I guess so. I will go ahead and reimplement everything in assembly?