[kvm-unit-tests PATCH 0/5] s390x: Add base AP support

[kvm-unit-tests PATCH 0/5] s390x: Add base AP support

[Janosch Frank]

As KVM supports passing Adjunct Processor (AP) crypto devices to
guests, we should make sure that the interface works as expected.

Three instructions provide the interface to the AP devices:
 - nqap: Enqueues a crypto request
 - dqap: Dequeues a crypto request
 - pqap: Provides information and processes support functions

nqap & dqap work on crypto requests for which we currently don't want
to add tests due to their complexity.

Which leaves us with pqap which is partly emulated for a guest 2 and
hence is a prime target for testing.

Janosch Frank (5):
  lib: s390x: Add ap library
  s390x: Add guest 2 AP test
  lib: s390x: ap: Add ap_setup
  s390x: ap: Add pqap aqic tests
  s390x: ap: Add reset tests

 lib/s390x/ap.c      | 246 ++++++++++++++++++++++++++
 lib/s390x/ap.h      | 104 +++++++++++
 s390x/Makefile      |   2 +
 s390x/ap.c          | 421 ++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |   4 +
 5 files changed, 777 insertions(+)
 create mode 100644 lib/s390x/ap.c
 create mode 100644 lib/s390x/ap.h
 create mode 100644 s390x/ap.c

-- 
2.34.1

[kvm-unit-tests PATCH 1/5] lib: s390x: Add ap library

[Janosch Frank]

Add functions and definitions needed to test the Adjunct
Processor (AP) crypto interface.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/ap.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/s390x/ap.h | 86 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 179 insertions(+)
 create mode 100644 lib/s390x/ap.c
 create mode 100644 lib/s390x/ap.h

diff --git a/lib/s390x/ap.c b/lib/s390x/ap.c
new file mode 100644
index 00000000..374fa210
--- /dev/null
+++ b/lib/s390x/ap.c
@@ -0,0 +1,93 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * AP crypto functions
+ *
+ * Some parts taken from the Linux AP driver.
+ *
+ * Copyright IBM Corp. 2023
+ * Author: Janosch Frank <frankja@linux.ibm.com>
+ *	   Tony Krowiak <akrowia@linux.ibm.com>
+ *	   Martin Schwidefsky <schwidefsky@de.ibm.com>
+ *	   Harald Freudenberger <freude@de.ibm.com>
+ */
+
+#include <libcflat.h>
+#include <interrupt.h>
+#include <ap.h>
+#include <asm/time.h>
+
+int ap_pqap_tapq(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw,
+		 struct pqap_r2 *r2)
+{
+	struct pqap_r0 r0 = {};
+	int cc;
+
+	/*
+	 * Test AP Queue
+	 *
+	 * Writes AP configuration information to the memory pointed
+	 * at by GR2.
+	 *
+	 * Inputs: 0
+	 * Outputs: 1 (APQSW), 2 (tapq data)
+	 * Synchronous
+	 */
+	r0.ap = ap;
+	r0.qn = qn;
+	r0.fc = PQAP_TEST_APQ;
+	asm volatile(
+		"	lgr	0,%[r0]\n"
+		"	.insn	rre,0xb2af0000,0,0\n" /* PQAP */
+		"	stg	1, %[apqsw]\n"
+		"	ipm	%[cc]\n"
+		"	srl	%[cc],28\n"
+		: [apqsw] "=&T" (*apqsw), [r2] "+&d" (r2), [cc] "=&d" (cc)
+		: [r0] "d" (r0)
+		: "memory");
+
+	return cc;
+}
+
+int ap_pqap_qci(struct ap_config_info *info)
+{
+	struct pqap_r0 r0 = { .fc = PQAP_QUERY_AP_CONF_INFO };
+	unsigned long r1 = 0;
+	int cc;
+
+	/*
+	 * Query AP Configuration Information
+	 *
+	 * Writes AP configuration information to the memory pointed
+	 * at by GR2.
+	 *
+	 * Inputs: 0,2
+	 * Outputs: memory at r2 address
+	 * Synchronous
+	 */
+	asm volatile(
+		"	lgr	0,%[r0]\n"
+		"	lgr	2,%[info]\n"
+		"	.insn	rre,0xb2af0000,0,0\n" /* PQAP */
+		"	ipm	%[cc]\n"
+		"	srl	%[cc],28\n"
+		: [r1] "+&d" (r1), [cc] "=&d" (cc)
+		: [r0] "d" (r0), [info] "d" (info)
+		: "cc", "memory", "0", "2");
+
+	return cc;
+}
+
+bool ap_check(void)
+{
+	struct ap_queue_status r1 = {};
+	struct pqap_r2 r2 = {};
+
+	/* Base AP support has no STFLE or SCLP feature bit */
+	expect_pgm_int();
+	ap_pqap_tapq(0, 0, &r1, &r2);
+
+	if (clear_pgm_int() == PGM_INT_CODE_OPERATION)
+		return false;
+
+	return true;
+}
diff --git a/lib/s390x/ap.h b/lib/s390x/ap.h
new file mode 100644
index 00000000..79fe6eb0
--- /dev/null
+++ b/lib/s390x/ap.h
@@ -0,0 +1,86 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * AP definitions
+ *
+ * Copyright IBM Corp. 2023
+ * Author: Janosch Frank <frankja@linux.ibm.com>
+ *	   Tony Krowiak <akrowia@linux.ibm.com>
+ *	   Martin Schwidefsky <schwidefsky@de.ibm.com>
+ *	   Harald Freudenberger <freude@de.ibm.com>
+ */
+
+#ifndef _S390X_AP_H_
+#define _S390X_AP_H_
+
+enum PQAP_FC {
+	PQAP_TEST_APQ,
+	PQAP_RESET_APQ,
+	PQAP_ZEROIZE_APQ,
+	PQAP_QUEUE_INT_CONTRL,
+	PQAP_QUERY_AP_CONF_INFO,
+	PQAP_QUERY_AP_COMP_TYPE,
+	PQAP_BEST_AP,
+};
+
+struct ap_queue_status {
+	uint32_t pad0;			/* Ignored padding for zArch  */
+	uint32_t empty		: 1;
+	uint32_t replies_waiting: 1;
+	uint32_t full		: 1;
+	uint32_t pad1		: 4;
+	uint32_t irq_enabled	: 1;
+	uint32_t rc		: 8;
+	uint32_t pad2		: 16;
+} __attribute__((packed))  __attribute__((aligned(8)));
+_Static_assert(sizeof(struct ap_queue_status) == sizeof(uint64_t), "APQSW size");
+
+struct ap_config_info {
+	uint8_t apsc	 : 1;	/* S bit */
+	uint8_t apxa	 : 1;	/* N bit */
+	uint8_t qact	 : 1;	/* C bit */
+	uint8_t rc8a	 : 1;	/* R bit */
+	uint8_t l	 : 1;	/* L bit */
+	uint8_t lext	 : 3;	/* Lext bits */
+	uint8_t reserved2[3];
+	uint8_t Na;		/* max # of APs - 1 */
+	uint8_t Nd;		/* max # of Domains - 1 */
+	uint8_t reserved6[10];
+	uint32_t apm[8];	/* AP ID mask */
+	uint32_t aqm[8];	/* AP (usage) queue mask */
+	uint32_t adm[8];	/* AP (control) domain mask */
+	uint8_t _reserved4[16];
+} __attribute__((aligned(8))) __attribute__ ((__packed__));
+_Static_assert(sizeof(struct ap_config_info) == 128, "PQAP QCI size");
+
+struct pqap_r0 {
+	uint32_t pad0;
+	uint8_t fc;
+	uint8_t t : 1;		/* Test facilities (TAPQ)*/
+	uint8_t pad1 : 7;
+	uint8_t ap;
+	uint8_t qn;
+} __attribute__((packed))  __attribute__((aligned(8)));
+
+struct pqap_r2 {
+	uint8_t s : 1;		/* Special Command facility */
+	uint8_t m : 1;		/* AP4KM */
+	uint8_t c : 1;		/* AP4KC */
+	uint8_t cop : 1;	/* AP is in coprocessor mode */
+	uint8_t acc : 1;	/* AP is in accelerator mode */
+	uint8_t xcp : 1;	/* AP is in XCP-mode */
+	uint8_t n : 1;		/* AP extended addressing facility */
+	uint8_t pad_0 : 1;
+	uint8_t pad_1[3];
+	uint8_t at;
+	uint8_t nd;
+	uint8_t pad_6;
+	uint8_t pad_7 : 4;
+	uint8_t qd : 4;
+} __attribute__((packed))  __attribute__((aligned(8)));
+_Static_assert(sizeof(struct pqap_r2) == sizeof(uint64_t), "pqap_r2 size");
+
+bool ap_check(void);
+int ap_pqap_tapq(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw,
+		 struct pqap_r2 *r2);
+int ap_pqap_qci(struct ap_config_info *info);
+#endif
-- 
2.34.1

[kvm-unit-tests PATCH 2/5] s390x: Add guest 2 AP test

[Janosch Frank]

Add a test that checks the exceptions for the PQAP, NQAP and DQAP
adjunct processor (AP) crypto instructions.

Since triggering the exceptions doesn't require actual AP hardware,
this test can run without complicated setup.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/Makefile      |   2 +
 s390x/ap.c          | 308 ++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |   4 +
 3 files changed, 314 insertions(+)
 create mode 100644 s390x/ap.c

diff --git a/s390x/Makefile b/s390x/Makefile
index e8559a4e..f74241d5 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -39,6 +39,7 @@ tests += $(TEST_DIR)/panic-loop-extint.elf
 tests += $(TEST_DIR)/panic-loop-pgm.elf
 tests += $(TEST_DIR)/migration-sck.elf
 tests += $(TEST_DIR)/exittime.elf
+tests += $(TEST_DIR)/ap.elf
 
 pv-tests += $(TEST_DIR)/pv-diags.elf
 pv-tests += $(TEST_DIR)/pv-icptcode.elf
@@ -102,6 +103,7 @@ cflatobjs += lib/s390x/malloc_io.o
 cflatobjs += lib/s390x/uv.o
 cflatobjs += lib/s390x/sie.o
 cflatobjs += lib/s390x/fault.o
+cflatobjs += lib/s390x/ap.o
 
 OBJDIRS += lib/s390x
 
diff --git a/s390x/ap.c b/s390x/ap.c
new file mode 100644
index 00000000..82ddb6d2
--- /dev/null
+++ b/s390x/ap.c
@@ -0,0 +1,308 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * AP instruction G2 tests
+ *
+ * Copyright (c) 2023 IBM Corp
+ *
+ * Authors:
+ *  Janosch Frank <frankja@linux.ibm.com>
+ */
+
+#include <libcflat.h>
+#include <interrupt.h>
+#include <bitops.h>
+#include <alloc_page.h>
+#include <asm/facility.h>
+#include <asm/time.h>
+#include <ap.h>
+
+/* For PQAP PGM checks where we need full control over the input */
+static void pqap(unsigned long grs[3])
+{
+	asm volatile(
+		"	lgr	0,%[r0]\n"
+		"	lgr	1,%[r1]\n"
+		"	lgr	2,%[r2]\n"
+		"	.insn	rre,0xb2af0000,0,0\n" /* PQAP */
+		::  [r0] "d" (grs[0]), [r1] "d" (grs[1]), [r2] "d" (grs[2])
+		: "cc", "memory", "0", "1", "2");
+}
+
+static void test_pgms_pqap(void)
+{
+	unsigned long grs[3] = {};
+	struct pqap_r0 *r0 = (struct pqap_r0 *)grs;
+	uint8_t *data = alloc_page();
+	uint16_t pgm;
+	int fails = 0;
+	int i;
+
+	report_prefix_push("pqap");
+
+	/* Wrong FC code */
+	report_prefix_push("invalid fc");
+	r0->fc = 42;
+	expect_pgm_int();
+	pqap(grs);
+	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+	memset(grs, 0, sizeof(grs));
+	report_prefix_pop();
+
+	report_prefix_push("invalid gr0 bits");
+	for (i = 42; i < 6; i++) {
+		expect_pgm_int();
+		grs[0] = BIT(63 - i);
+		pqap(grs);
+		pgm = clear_pgm_int();
+
+		if (pgm != PGM_INT_CODE_SPECIFICATION) {
+			report_fail("fail on bit %d", i);
+			fails++;
+		}
+	}
+	report(!fails, "All bits tested");
+	memset(grs, 0, sizeof(grs));
+	fails = 0;
+	report_prefix_pop();
+
+	report_prefix_push("alignment");
+	report_prefix_push("fc=4");
+	r0->fc = PQAP_QUERY_AP_CONF_INFO;
+	grs[2] = (unsigned long)data;
+	for (i = 1; i < 8; i++) {
+		expect_pgm_int();
+		grs[2]++;
+		pqap(grs);
+		pgm = clear_pgm_int();
+		if (pgm != PGM_INT_CODE_SPECIFICATION) {
+			report_fail("fail on bit %d", i);
+			fails++;
+		}
+	}
+	report(!fails, "All bits tested");
+	report_prefix_pop();
+	report_prefix_push("fc=6");
+	r0->fc = PQAP_BEST_AP;
+	grs[2] = (unsigned long)data;
+	for (i = 1; i < 8; i++) {
+		expect_pgm_int();
+		grs[2]++;
+		pqap(grs);
+		pgm = clear_pgm_int();
+		if (pgm != PGM_INT_CODE_SPECIFICATION) {
+			report_fail("fail on bit %d", i);
+			fails++;
+		}
+	}
+	report(!fails, "All bits tested");
+	report_prefix_pop();
+	report_prefix_pop();
+
+	free_page(data);
+	report_prefix_pop();
+}
+
+static void test_pgms_nqap(void)
+{
+	uint8_t gr0_zeroes_bits[] = {
+		32, 34, 35, 40
+	};
+	uint64_t gr0;
+	bool fail;
+	int i;
+
+	report_prefix_push("nqap");
+
+	/* Registers 0 and 1 are always used, the others are even/odd pairs */
+	report_prefix_push("spec");
+	report_prefix_push("r1");
+	expect_pgm_int();
+	asm volatile (
+		".insn	rre,0xb2ad0000,3,6\n"
+		: : : "cc", "memory", "0", "1", "2", "3");
+	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+	report_prefix_pop();
+
+	report_prefix_push("r2");
+	expect_pgm_int();
+	asm volatile (
+		".insn	rre,0xb2ad0000,2,7\n"
+		: : : "cc", "memory", "0", "1", "3", "4");
+	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+	report_prefix_pop();
+
+	report_prefix_push("len==0");
+	expect_pgm_int();
+	asm volatile (
+		"xgr	0,0\n"
+		"xgr	5,5\n"
+		".insn	rre,0xb2ad0000,2,4\n"
+		: : : "cc", "memory", "0", "1", "2", "3", "4", "5");
+	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+	report_prefix_pop();
+
+	report_prefix_push("len>12288");
+	expect_pgm_int();
+	asm volatile (
+		"xgr	5,5\n"
+		"lghi	5, 12289\n"
+		".insn	rre,0xb2ad0000,2,4\n"
+		: : : "cc", "memory", "0", "1", "2", "3", "4", "5");
+	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+	report_prefix_pop();
+
+	report_prefix_push("gr0_zero_bits");
+	fail = false;
+	for (i = 0; i < 4; i++) {
+		expect_pgm_int();
+		gr0 = BIT_ULL(63 - gr0_zeroes_bits[i]);
+		asm volatile (
+			"xgr	5,5\n"
+			"lghi	5, 128\n"
+			"lg	0, 0(%[val])\n"
+			".insn	rre,0xb2ad0000,2,4\n"
+			: : [val] "a" (&gr0)
+			: "cc", "memory", "0", "1", "2", "3", "4", "5");
+		if (clear_pgm_int() != PGM_INT_CODE_SPECIFICATION) {
+			report_fail("setting gr0 bit %d did not result in a spec exception",
+				    gr0_zeroes_bits[i]);
+			fail = true;
+		}
+	}
+	report(!fail, "set bit specification pgms");
+	report_prefix_pop();
+
+	report_prefix_pop();
+	report_prefix_pop();
+}
+
+static void test_pgms_dqap(void)
+{
+	uint8_t gr0_zeroes_bits[] = {
+		33, 34, 35, 40, 41
+	};
+	uint64_t gr0;
+	bool fail;
+	int i;
+
+	report_prefix_push("dqap");
+
+	/* Registers 0 and 1 are always used, the others are even/odd pairs */
+	report_prefix_push("spec");
+	report_prefix_push("r1");
+	expect_pgm_int();
+	asm volatile (
+		".insn	rre,0xb2ae0000,3,6\n"
+		: : : "cc", "memory", "0", "1", "2", "3");
+	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+	report_prefix_pop();
+
+	report_prefix_push("r2");
+	expect_pgm_int();
+	asm volatile (
+		".insn	rre,0xb2ae0000,2,7\n"
+		: : : "cc", "memory", "0", "1", "3", "4");
+	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+	report_prefix_pop();
+
+	report_prefix_push("len==0");
+	expect_pgm_int();
+	asm volatile (
+		"xgr	0,0\n"
+		"xgr	5,5\n"
+		".insn	rre,0xb2ae0000,2,4\n"
+		: : : "cc", "memory", "0", "1", "2", "3", "4", "5");
+	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+	report_prefix_pop();
+
+	report_prefix_push("len>12288");
+	expect_pgm_int();
+	asm volatile (
+		"xgr	5,5\n"
+		"lghi	5, 12289\n"
+		".insn	rre,0xb2ae0000,2,4\n"
+		: : : "cc", "memory", "0", "1", "2", "3", "4", "5");
+	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+	report_prefix_pop();
+
+	report_prefix_push("gr0_zero_bits");
+	fail = false;
+	for (i = 0; i < 5; i++) {
+		expect_pgm_int();
+		gr0 = BIT_ULL(63 - gr0_zeroes_bits[i]);
+		asm volatile (
+			"xgr	5,5\n"
+			"lghi	5, 128\n"
+			"lg	0, 0(%[val])\n"
+			".insn	rre,0xb2ae0000,2,4\n"
+			: : [val] "a" (&gr0)
+			: "cc", "memory", "0", "1", "2", "3", "4", "5");
+		if (clear_pgm_int() != PGM_INT_CODE_SPECIFICATION) {
+			report_info("setting gr0 bit %d did not result in a spec exception",
+				    gr0_zeroes_bits[i]);
+			fail = true;
+		}
+	}
+	report(!fail, "set bit specification pgms");
+	report_prefix_pop();
+
+	report_prefix_pop();
+	report_prefix_pop();
+}
+
+static void test_priv(void)
+{
+	struct ap_config_info info = {};
+
+	report_prefix_push("privileged");
+
+	report_prefix_push("pqap");
+	expect_pgm_int();
+	enter_pstate();
+	ap_pqap_qci(&info);
+	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
+	report_prefix_pop();
+
+	/*
+	 * Enqueue and dequeue take too many registers so a simple
+	 * inline assembly makes more sense than using the library
+	 * functions.
+	 */
+	report_prefix_push("nqap");
+	expect_pgm_int();
+	enter_pstate();
+	asm volatile (
+		".insn	rre,0xb2ad0000,0,2\n"
+		: : : "cc", "memory", "0", "1", "2", "3");
+	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
+	report_prefix_pop();
+
+	report_prefix_push("dqap");
+	expect_pgm_int();
+	enter_pstate();
+	asm volatile (
+		".insn	rre,0xb2ae0000,0,2\n"
+		: : : "cc", "memory", "0", "1", "2", "3");
+	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
+	report_prefix_pop();
+
+	report_prefix_pop();
+}
+
+int main(void)
+{
+	report_prefix_push("ap");
+	if (!ap_check()) {
+		report_skip("AP instructions not available");
+		goto done;
+	}
+
+	test_priv();
+	test_pgms_pqap();
+	test_pgms_nqap();
+	test_pgms_dqap();
+
+done:
+	report_prefix_pop();
+	return report_summary();
+}
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index d97eb5e9..9b7c65c8 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -215,3 +215,7 @@ file = migration-skey.elf
 smp = 2
 groups = migration
 extra_params = -append '--parallel'
+
+[ap]
+file = ap.elf
+
-- 
2.34.1

[kvm-unit-tests PATCH 3/5] lib: s390x: ap: Add ap_setup

[Janosch Frank]

For the next tests we need a valid queue which means we need to grab
the qci info and search the first set bit in the ap and aq masks.

Let's move from the ap_check function to a proper setup function that
also returns the first usable APQN. Later we can extend the setup to
build a list of all available APQNs but right now one APQN is plenty.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/ap.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++-
 lib/s390x/ap.h |  5 ++++-
 s390x/ap.c     |  7 ++++++-
 3 files changed, 63 insertions(+), 3 deletions(-)

diff --git a/lib/s390x/ap.c b/lib/s390x/ap.c
index 374fa210..8d7f2992 100644
--- a/lib/s390x/ap.c
+++ b/lib/s390x/ap.c
@@ -13,9 +13,12 @@
 
 #include <libcflat.h>
 #include <interrupt.h>
+#include <bitops.h>
 #include <ap.h>
 #include <asm/time.h>
 
+static struct ap_config_info qci;
+
 int ap_pqap_tapq(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw,
 		 struct pqap_r2 *r2)
 {
@@ -77,7 +80,44 @@ int ap_pqap_qci(struct ap_config_info *info)
 	return cc;
 }
 
-bool ap_check(void)
+static int ap_get_apqn(uint8_t *ap, uint8_t *qn)
+{
+	unsigned long *ptr;
+	uint8_t bit;
+	int i;
+
+	ap_pqap_qci(&qci);
+	*ap = 0;
+	*qn = 0;
+
+	ptr = (unsigned long *)qci.apm;
+	for (i = 0; i < 4; i++) {
+		bit = fls(*ptr);
+		if (bit) {
+			*ap = i * 64 + 64 - bit;
+			break;
+		}
+	}
+	ptr = (unsigned long *)qci.aqm;
+	for (i = 0; i < 4; i++) {
+		bit = fls(*ptr);
+		if (bit) {
+			*qn = i * 64 + 64 - bit;
+			break;
+		}
+	}
+
+	if (!*ap || !*qn)
+		return -1;
+
+	/* fls returns 1 + bit number, so we need to remove 1 here */
+	*ap -= 1;
+	*qn -= 1;
+
+	return 0;
+}
+
+static bool ap_check(void)
 {
 	struct ap_queue_status r1 = {};
 	struct pqap_r2 r2 = {};
@@ -91,3 +131,15 @@ bool ap_check(void)
 
 	return true;
 }
+
+int ap_setup(uint8_t *ap, uint8_t *qn)
+{
+	if (!ap_check())
+		return AP_SETUP_NOINSTR;
+
+	/* Instructions available but no APQNs */
+	if (ap_get_apqn(ap, qn))
+		return AP_SETUP_NOAPQN;
+
+	return 0;
+}
diff --git a/lib/s390x/ap.h b/lib/s390x/ap.h
index 79fe6eb0..59595eba 100644
--- a/lib/s390x/ap.h
+++ b/lib/s390x/ap.h
@@ -79,7 +79,10 @@ struct pqap_r2 {
 } __attribute__((packed))  __attribute__((aligned(8)));
 _Static_assert(sizeof(struct pqap_r2) == sizeof(uint64_t), "pqap_r2 size");
 
-bool ap_check(void);
+#define AP_SETUP_NOINSTR	-1
+#define AP_SETUP_NOAPQN		1
+
+int ap_setup(uint8_t *ap, uint8_t *qn);
 int ap_pqap_tapq(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw,
 		 struct pqap_r2 *r2);
 int ap_pqap_qci(struct ap_config_info *info);
diff --git a/s390x/ap.c b/s390x/ap.c
index 82ddb6d2..20b4e76e 100644
--- a/s390x/ap.c
+++ b/s390x/ap.c
@@ -16,6 +16,9 @@
 #include <asm/time.h>
 #include <ap.h>
 
+static uint8_t apn;
+static uint8_t qn;
+
 /* For PQAP PGM checks where we need full control over the input */
 static void pqap(unsigned long grs[3])
 {
@@ -291,8 +294,10 @@ static void test_priv(void)
 
 int main(void)
 {
+	int setup_rc = ap_setup(&apn, &qn);
+
 	report_prefix_push("ap");
-	if (!ap_check()) {
+	if (setup_rc == AP_SETUP_NOINSTR) {
 		report_skip("AP instructions not available");
 		goto done;
 	}
-- 
2.34.1

[kvm-unit-tests PATCH 4/5] s390x: ap: Add pqap aqic tests

[Janosch Frank]

Let's check if we can enable/disable interrupts and if all errors are
reported if we specify bad addresses for the notification indication
byte.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/ap.c | 33 +++++++++++++++++++++++++++++
 lib/s390x/ap.h | 11 ++++++++++
 s390x/ap.c     | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 100 insertions(+)

diff --git a/lib/s390x/ap.c b/lib/s390x/ap.c
index 8d7f2992..aaf5b4b9 100644
--- a/lib/s390x/ap.c
+++ b/lib/s390x/ap.c
@@ -51,6 +51,39 @@ int ap_pqap_tapq(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw,
 	return cc;
 }
 
+int ap_pqap_aqic(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw,
+		 struct ap_qirq_ctrl aqic, unsigned long addr)
+{
+	struct pqap_r0 r0 = {};
+	int cc;
+
+	/*
+	 * AP-Queue Interruption Control
+	 *
+	 * Enables/disables interrupts for a APQN
+	 *
+	 * Inputs: 0,1,2
+	 * Outputs: 1 (APQSW)
+	 * Synchronous
+	 */
+	r0.ap = ap;
+	r0.qn = qn;
+	r0.fc = PQAP_QUEUE_INT_CONTRL;
+	asm volatile(
+		"	lgr	0,%[r0]\n"
+		"	lgr	1,%[aqic]\n"
+		"	lgr	2,%[addr]\n"
+		"	.insn	rre,0xb2af0000,0,0\n" /* PQAP */
+		"	stg	1, %[apqsw]\n"
+		"	ipm	%[cc]\n"
+		"	srl	%[cc],28\n"
+		: [apqsw] "=T" (*apqsw), [cc] "=&d" (cc)
+		: [r0] "d" (r0), [aqic] "d" (aqic), [addr] "d" (addr)
+		: "cc", "memory", "0", "2");
+
+	return cc;
+}
+
 int ap_pqap_qci(struct ap_config_info *info)
 {
 	struct pqap_r0 r0 = { .fc = PQAP_QUERY_AP_CONF_INFO };
diff --git a/lib/s390x/ap.h b/lib/s390x/ap.h
index 59595eba..3f9e2eb6 100644
--- a/lib/s390x/ap.h
+++ b/lib/s390x/ap.h
@@ -79,6 +79,15 @@ struct pqap_r2 {
 } __attribute__((packed))  __attribute__((aligned(8)));
 _Static_assert(sizeof(struct pqap_r2) == sizeof(uint64_t), "pqap_r2 size");
 
+struct ap_qirq_ctrl {
+	uint64_t res0 : 16;
+	uint64_t ir    : 1;	/* ir flag: enable (1) or disable (0) irq */
+	uint64_t res1 : 44;
+	uint64_t isc   : 3;	/* irq sub class */
+} __attribute__((packed))  __attribute__((aligned(8)));
+_Static_assert(sizeof(struct ap_qirq_ctrl) == sizeof(uint64_t),
+	       "struct ap_qirq_ctrl size");
+
 #define AP_SETUP_NOINSTR	-1
 #define AP_SETUP_NOAPQN		1
 
@@ -86,4 +95,6 @@ int ap_setup(uint8_t *ap, uint8_t *qn);
 int ap_pqap_tapq(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw,
 		 struct pqap_r2 *r2);
 int ap_pqap_qci(struct ap_config_info *info);
+int ap_pqap_aqic(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw,
+		 struct ap_qirq_ctrl aqic, unsigned long addr);
 #endif
diff --git a/s390x/ap.c b/s390x/ap.c
index 20b4e76e..31dcfe29 100644
--- a/s390x/ap.c
+++ b/s390x/ap.c
@@ -292,6 +292,55 @@ static void test_priv(void)
 	report_prefix_pop();
 }
 
+static void test_pqap_aqic(void)
+{
+	struct ap_queue_status apqsw = {};
+	static uint8_t not_ind_byte;
+	struct ap_qirq_ctrl aqic = {};
+	struct pqap_r2 r2 = {};
+
+	int cc;
+
+	report_prefix_push("pqap");
+	report_prefix_push("aqic");
+
+	ap_pqap_tapq(apn, qn, &apqsw, &r2);
+
+	aqic.ir = 1;
+	cc = ap_pqap_aqic(apn, qn, &apqsw, aqic, 0);
+	report(cc == 3 && apqsw.rc == 6, "invalid addr 0");
+
+	aqic.ir = 1;
+	cc = ap_pqap_aqic(apn, qn, &apqsw, aqic, -1);
+	report(cc == 3 && apqsw.rc == 6, "invalid addr -1");
+
+	aqic.ir = 0;
+	cc = ap_pqap_aqic(apn, qn, &apqsw, aqic, (uintptr_t)&not_ind_byte);
+	report(cc == 3 && apqsw.rc == 7, "disable");
+
+	aqic.ir = 1;
+	cc = ap_pqap_aqic(apn, qn, &apqsw, aqic, (uintptr_t)&not_ind_byte);
+	report(cc == 0 && apqsw.rc == 0, "enable");
+
+	do {
+		cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
+	} while (cc == 0 && apqsw.irq_enabled == 0);
+
+	cc = ap_pqap_aqic(apn, qn, &apqsw, aqic, (uintptr_t)&not_ind_byte);
+	report(cc == 3 && apqsw.rc == 7, "enable while enabled");
+
+	aqic.ir = 0;
+	cc = ap_pqap_aqic(apn, qn, &apqsw, aqic, (uintptr_t)&not_ind_byte);
+	assert(cc == 0 && apqsw.rc == 0);
+
+	do {
+		cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
+	} while (cc == 0 && apqsw.irq_enabled == 1);
+
+	report_prefix_pop();
+	report_prefix_pop();
+}
+
 int main(void)
 {
 	int setup_rc = ap_setup(&apn, &qn);
@@ -307,6 +356,13 @@ int main(void)
 	test_pgms_nqap();
 	test_pgms_dqap();
 
+	/* The next tests need queues */
+	if (setup_rc == AP_SETUP_NOAPQN) {
+		report_skip("No APQN available");
+		goto done;
+	}
+	test_pqap_aqic();
+
 done:
 	report_prefix_pop();
 	return report_summary();
-- 
2.34.1

[kvm-unit-tests PATCH 5/5] s390x: ap: Add reset tests

[Janosch Frank]

Test if the IRQ enablement is turned off on a reset or zeroize PQAP.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/ap.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/s390x/ap.h |  4 +++
 s390x/ap.c     | 52 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 124 insertions(+)

diff --git a/lib/s390x/ap.c b/lib/s390x/ap.c
index aaf5b4b9..d969b2a5 100644
--- a/lib/s390x/ap.c
+++ b/lib/s390x/ap.c
@@ -113,6 +113,74 @@ int ap_pqap_qci(struct ap_config_info *info)
 	return cc;
 }
 
+static int pqap_reset(uint8_t ap, uint8_t qn, struct ap_queue_status *r1,
+		      bool zeroize)
+{
+	struct pqap_r0 r0 = {};
+	int cc;
+
+	/*
+	 * Reset/zeroize AP Queue
+	 *
+	 * Resets/zeroizes a queue and disables IRQs
+	 *
+	 * Inputs: 0
+	 * Outputs: 1
+	 * Asynchronous
+	 */
+	r0.ap = ap;
+	r0.qn = qn;
+	r0.fc = zeroize ? PQAP_ZEROIZE_APQ : PQAP_RESET_APQ;
+	asm volatile(
+		"	lgr	0,%[r0]\n"
+		"	lgr	1,%[r1]\n"
+		"	.insn	rre,0xb2af0000,0,0\n" /* PQAP */
+		"	ipm	%[cc]\n"
+		"	srl	%[cc],28\n"
+		: [r1] "+&d" (r1), [cc] "=&d" (cc)
+		: [r0] "d" (r0)
+		: "memory");
+
+	return cc;
+}
+
+static int pqap_reset_wait(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw,
+			   bool zeroize)
+{
+	struct pqap_r2 r2 = {};
+	int cc;
+
+	cc = pqap_reset(ap, qn, apqsw, zeroize);
+	/* On a cc == 3 / error we don't need to wait */
+	if (cc)
+		return cc;
+
+	/*
+	 * TAPQ returns AP_RC_RESET_IN_PROGRESS if a reset is being
+	 * processed
+	 */
+	do {
+		cc = ap_pqap_tapq(ap, qn, apqsw, &r2);
+		/* Give it some time to process before the retry */
+		mdelay(20);
+	} while (apqsw->rc == AP_RC_RESET_IN_PROGRESS);
+
+	if (apqsw->rc)
+		printf("Wait for reset failed on ap %d queue %d with tapq rc %d.",
+			ap, qn, apqsw->rc);
+	return cc;
+}
+
+int ap_pqap_reset(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw)
+{
+	return pqap_reset_wait(ap, qn, apqsw, false);
+}
+
+int ap_pqap_reset_zeroize(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw)
+{
+	return pqap_reset_wait(ap, qn, apqsw, true);
+}
+
 static int ap_get_apqn(uint8_t *ap, uint8_t *qn)
 {
 	unsigned long *ptr;
diff --git a/lib/s390x/ap.h b/lib/s390x/ap.h
index 3f9e2eb6..f9343b5f 100644
--- a/lib/s390x/ap.h
+++ b/lib/s390x/ap.h
@@ -12,6 +12,8 @@
 #ifndef _S390X_AP_H_
 #define _S390X_AP_H_
 
+#define AP_RC_RESET_IN_PROGRESS	0x02
+
 enum PQAP_FC {
 	PQAP_TEST_APQ,
 	PQAP_RESET_APQ,
@@ -94,6 +96,8 @@ _Static_assert(sizeof(struct ap_qirq_ctrl) == sizeof(uint64_t),
 int ap_setup(uint8_t *ap, uint8_t *qn);
 int ap_pqap_tapq(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw,
 		 struct pqap_r2 *r2);
+int ap_pqap_reset(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw);
+int ap_pqap_reset_zeroize(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw);
 int ap_pqap_qci(struct ap_config_info *info);
 int ap_pqap_aqic(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw,
 		 struct ap_qirq_ctrl aqic, unsigned long addr);
diff --git a/s390x/ap.c b/s390x/ap.c
index 31dcfe29..47b4f832 100644
--- a/s390x/ap.c
+++ b/s390x/ap.c
@@ -341,6 +341,57 @@ static void test_pqap_aqic(void)
 	report_prefix_pop();
 }
 
+static void test_pqap_resets(void)
+{
+	struct ap_queue_status apqsw = {};
+	static uint8_t not_ind_byte;
+	struct ap_qirq_ctrl aqic = {};
+	struct pqap_r2 r2 = {};
+
+	int cc;
+
+	report_prefix_push("pqap");
+	report_prefix_push("rapq");
+
+	/* Enable IRQs which the resets will disable */
+	aqic.ir = 1;
+	cc = ap_pqap_aqic(apn, qn, &apqsw, aqic, (uintptr_t)&not_ind_byte);
+	report(cc == 0 && apqsw.rc == 0, "enable");
+
+	do {
+		cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
+	} while (cc == 0 && apqsw.irq_enabled == 0);
+	report(apqsw.irq_enabled == 1, "IRQs enabled");
+
+	ap_pqap_reset(apn, qn, &apqsw);
+	cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
+	assert(!cc);
+	report(apqsw.irq_enabled == 0, "IRQs have been disabled");
+
+	report_prefix_pop();
+
+	report_prefix_push("zapq");
+
+	/* Enable IRQs which the resets will disable */
+	aqic.ir = 1;
+	cc = ap_pqap_aqic(apn, qn, &apqsw, aqic, (uintptr_t)&not_ind_byte);
+	report(cc == 0 && apqsw.rc == 0, "enable");
+
+	do {
+		cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
+	} while (cc == 0 && apqsw.irq_enabled == 0);
+	report(apqsw.irq_enabled == 1, "IRQs enabled");
+
+	ap_pqap_reset_zeroize(apn, qn, &apqsw);
+	cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
+	assert(!cc);
+	report(apqsw.irq_enabled == 0, "IRQs have been disabled");
+
+	report_prefix_pop();
+
+	report_prefix_pop();
+}
+
 int main(void)
 {
 	int setup_rc = ap_setup(&apn, &qn);
@@ -362,6 +413,7 @@ int main(void)
 		goto done;
 	}
 	test_pqap_aqic();
+	test_pqap_resets();
 
 done:
 	report_prefix_pop();
-- 
2.34.1

Re: [kvm-unit-tests PATCH 1/5] lib: s390x: Add ap library

[Claudio Imbrenda]

On Thu, 30 Mar 2023 11:42:40 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> Add functions and definitions needed to test the Adjunct
> Processor (AP) crypto interface.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>

[...]

> +bool ap_check(void)
> +{
> +	struct ap_queue_status r1 = {};
> +	struct pqap_r2 r2 = {};
> +
> +	/* Base AP support has no STFLE or SCLP feature bit */

this is true, but you are also indiscriminately using a feature for
which there is a STFLE feature. since it seems you depend on that, you
might as well just check bit for STFLE.12 and assume the base support
is there if it's set

> +	expect_pgm_int();
> +	ap_pqap_tapq(0, 0, &r1, &r2);
> +
> +	if (clear_pgm_int() == PGM_INT_CODE_OPERATION)
> +		return false;
> +
> +	return true;
> +}

[...]

> +struct ap_config_info {
> +	uint8_t apsc	 : 1;	/* S bit */
> +	uint8_t apxa	 : 1;	/* N bit */
> +	uint8_t qact	 : 1;	/* C bit */
> +	uint8_t rc8a	 : 1;	/* R bit */
> +	uint8_t l	 : 1;	/* L bit */
> +	uint8_t lext	 : 3;	/* Lext bits */
> +	uint8_t reserved2[3];
> +	uint8_t Na;		/* max # of APs - 1 */
> +	uint8_t Nd;		/* max # of Domains - 1 */
> +	uint8_t reserved6[10];
> +	uint32_t apm[8];	/* AP ID mask */

is there a specific reason why these are uint32_t?
uint64_t would maybe make your life easier in subsequent patches (see my
comments there)

> +	uint32_t aqm[8];	/* AP (usage) queue mask */
> +	uint32_t adm[8];	/* AP (control) domain mask */
> +	uint8_t _reserved4[16];
> +} __attribute__((aligned(8))) __attribute__ ((__packed__));
> +_Static_assert(sizeof(struct ap_config_info) == 128, "PQAP QCI size");
> +
> +struct pqap_r0 {
> +	uint32_t pad0;
> +	uint8_t fc;
> +	uint8_t t : 1;		/* Test facilities (TAPQ)*/
> +	uint8_t pad1 : 7;
> +	uint8_t ap;
> +	uint8_t qn;
> +} __attribute__((packed))  __attribute__((aligned(8)));
> +
> +struct pqap_r2 {
> +	uint8_t s : 1;		/* Special Command facility */
> +	uint8_t m : 1;		/* AP4KM */
> +	uint8_t c : 1;		/* AP4KC */
> +	uint8_t cop : 1;	/* AP is in coprocessor mode */
> +	uint8_t acc : 1;	/* AP is in accelerator mode */
> +	uint8_t xcp : 1;	/* AP is in XCP-mode */
> +	uint8_t n : 1;		/* AP extended addressing facility */
> +	uint8_t pad_0 : 1;
> +	uint8_t pad_1[3];
> +	uint8_t at;
> +	uint8_t nd;
> +	uint8_t pad_6;
> +	uint8_t pad_7 : 4;
> +	uint8_t qd : 4;
> +} __attribute__((packed))  __attribute__((aligned(8)));
> +_Static_assert(sizeof(struct pqap_r2) == sizeof(uint64_t), "pqap_r2 size");
> +
> +bool ap_check(void);
> +int ap_pqap_tapq(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw,
> +		 struct pqap_r2 *r2);
> +int ap_pqap_qci(struct ap_config_info *info);
> +#endif

Re: [kvm-unit-tests PATCH 2/5] s390x: Add guest 2 AP test

[Claudio Imbrenda]

On Thu, 30 Mar 2023 11:42:41 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> Add a test that checks the exceptions for the PQAP, NQAP and DQAP
> adjunct processor (AP) crypto instructions.
> 
> Since triggering the exceptions doesn't require actual AP hardware,
> this test can run without complicated setup.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---

[...]

> +
> +static void test_pgms_pqap(void)
> +{
> +	unsigned long grs[3] = {};
> +	struct pqap_r0 *r0 = (struct pqap_r0 *)grs;
> +	uint8_t *data = alloc_page();
> +	uint16_t pgm;
> +	int fails = 0;
> +	int i;
> +
> +	report_prefix_push("pqap");
> +
> +	/* Wrong FC code */
> +	report_prefix_push("invalid fc");
> +	r0->fc = 42;

maybe make a macro out of it, both to avoid magic numbers and to change
it easily if code 42 will ever become defined in the future.

> +	expect_pgm_int();
> +	pqap(grs);
> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	memset(grs, 0, sizeof(grs));
> +	report_prefix_pop();
> +
> +	report_prefix_push("invalid gr0 bits");
> +	for (i = 42; i < 6; i++) {

42 is not < 6, this whole thing will be skipped?

> +		expect_pgm_int();
> +		grs[0] = BIT(63 - i);
> +		pqap(grs);
> +		pgm = clear_pgm_int();
> +
> +		if (pgm != PGM_INT_CODE_SPECIFICATION) {
> +			report_fail("fail on bit %d", i);
> +			fails++;
> +		}
> +	}
> +	report(!fails, "All bits tested");
> +	memset(grs, 0, sizeof(grs));
> +	fails = 0;
> +	report_prefix_pop();
> +
> +	report_prefix_push("alignment");
> +	report_prefix_push("fc=4");
> +	r0->fc = PQAP_QUERY_AP_CONF_INFO;
> +	grs[2] = (unsigned long)data;
> +	for (i = 1; i < 8; i++) {
> +		expect_pgm_int();
> +		grs[2]++;
> +		pqap(grs);
> +		pgm = clear_pgm_int();
> +		if (pgm != PGM_INT_CODE_SPECIFICATION) {
> +			report_fail("fail on bit %d", i);
> +			fails++;
> +		}
> +	}
> +	report(!fails, "All bits tested");

you mean "All alignments tested" ?

> +	report_prefix_pop();
> +	report_prefix_push("fc=6");
> +	r0->fc = PQAP_BEST_AP;
> +	grs[2] = (unsigned long)data;
> +	for (i = 1; i < 8; i++) {
> +		expect_pgm_int();
> +		grs[2]++;
> +		pqap(grs);
> +		pgm = clear_pgm_int();
> +		if (pgm != PGM_INT_CODE_SPECIFICATION) {
> +			report_fail("fail on bit %d", i);
> +			fails++;
> +		}
> +	}
> +	report(!fails, "All bits tested");

same here?

> +	report_prefix_pop();
> +	report_prefix_pop();
> +
> +	free_page(data);
> +	report_prefix_pop();
> +}
> +
> +static void test_pgms_nqap(void)
> +{
> +	uint8_t gr0_zeroes_bits[] = {
> +		32, 34, 35, 40
> +	};
> +	uint64_t gr0;
> +	bool fail;
> +	int i;
> +
> +	report_prefix_push("nqap");
> +
> +	/* Registers 0 and 1 are always used, the others are
> even/odd pairs */
> +	report_prefix_push("spec");
> +	report_prefix_push("r1");
> +	expect_pgm_int();
> +	asm volatile (
> +		".insn	rre,0xb2ad0000,3,6\n"
> +		: : : "cc", "memory", "0", "1", "2", "3");

I would say 
"0", "1", "2", "3", "4", "6", "7"

since there are two ways of doing it wrong when it comes to even-odd
register pairs (r and r+1, r&~1 and r&~1+1)

> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("r2");
> +	expect_pgm_int();
> +	asm volatile (
> +		".insn	rre,0xb2ad0000,2,7\n"
> +		: : : "cc", "memory", "0", "1", "3", "4");

same here (with the right numbers of course)

and I would even add a test with both odd registers

> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("len==0");
> +	expect_pgm_int();
> +	asm volatile (
> +		"xgr	0,0\n"
> +		"xgr	5,5\n"
> +		".insn	rre,0xb2ad0000,2,4\n"
> +		: : : "cc", "memory", "0", "1", "2", "3", "4", "5");
> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("len>12288");
> +	expect_pgm_int();
> +	asm volatile (
> +		"xgr	5,5\n"
> +		"lghi	5, 12289\n"

I would also check setting all the bits above bit 13 (i.e. if an
implementation wrongly checks only the lower 16 or 32 bits of the value

> +		".insn	rre,0xb2ad0000,2,4\n"
> +		: : : "cc", "memory", "0", "1", "2", "3", "4", "5");
> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("gr0_zero_bits");
> +	fail = false;
> +	for (i = 0; i < 4; i++) {

i < ARRAY_SIZE(gr0_zeroes_bits) might be more robust and future-proof

> +		expect_pgm_int();
> +		gr0 = BIT_ULL(63 - gr0_zeroes_bits[i]);
> +		asm volatile (
> +			"xgr	5,5\n"
> +			"lghi	5, 128\n"
> +			"lg	0, 0(%[val])\n"
> +			".insn	rre,0xb2ad0000,2,4\n"
> +			: : [val] "a" (&gr0)
> +			: "cc", "memory", "0", "1", "2", "3", "4", "5");
> +		if (clear_pgm_int() != PGM_INT_CODE_SPECIFICATION) {
> +			report_fail("setting gr0 bit %d did not result in a spec exception",
> +				    gr0_zeroes_bits[i]);
> +			fail = true;
> +		}
> +	}
> +	report(!fail, "set bit specification pgms");
> +	report_prefix_pop();
> +
> +	report_prefix_pop();
> +	report_prefix_pop();
> +}
> +
> +static void test_pgms_dqap(void)
> +{
> +	uint8_t gr0_zeroes_bits[] = {
> +		33, 34, 35, 40, 41
> +	};
> +	uint64_t gr0;
> +	bool fail;
> +	int i;
> +
> +	report_prefix_push("dqap");
> +
> +	/* Registers 0 and 1 are always used, the others are even/odd pairs */
> +	report_prefix_push("spec");
> +	report_prefix_push("r1");
> +	expect_pgm_int();
> +	asm volatile (
> +		".insn	rre,0xb2ae0000,3,6\n"
> +		: : : "cc", "memory", "0", "1", "2", "3");

same concern here with the registers like in the previous test

> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("r2");
> +	expect_pgm_int();
> +	asm volatile (
> +		".insn	rre,0xb2ae0000,2,7\n"
> +		: : : "cc", "memory", "0", "1", "3", "4");

as above, plus add a test for both odd

> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("len==0");
> +	expect_pgm_int();
> +	asm volatile (
> +		"xgr	0,0\n"
> +		"xgr	5,5\n"
> +		".insn	rre,0xb2ae0000,2,4\n"
> +		: : : "cc", "memory", "0", "1", "2", "3", "4", "5");
> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("len>12288");
> +	expect_pgm_int();
> +	asm volatile (
> +		"xgr	5,5\n"
> +		"lghi	5, 12289\n"

like the previous test, also test the high bits

> +		".insn	rre,0xb2ae0000,2,4\n"
> +		: : : "cc", "memory", "0", "1", "2", "3", "4", "5");
> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("gr0_zero_bits");
> +	fail = false;
> +	for (i = 0; i < 5; i++) {

same concern here with ARRAY_SIZE

> +		expect_pgm_int();
> +		gr0 = BIT_ULL(63 - gr0_zeroes_bits[i]);
> +		asm volatile (
> +			"xgr	5,5\n"
> +			"lghi	5, 128\n"
> +			"lg	0, 0(%[val])\n"
> +			".insn	rre,0xb2ae0000,2,4\n"
> +			: : [val] "a" (&gr0)
> +			: "cc", "memory", "0", "1", "2", "3", "4", "5");
> +		if (clear_pgm_int() != PGM_INT_CODE_SPECIFICATION) {
> +			report_info("setting gr0 bit %d did not result in a spec exception",
> +				    gr0_zeroes_bits[i]);
> +			fail = true;
> +		}
> +	}
> +	report(!fail, "set bit specification pgms");
> +	report_prefix_pop();
> +
> +	report_prefix_pop();
> +	report_prefix_pop();
> +}
> +
> +static void test_priv(void)
> +{
> +	struct ap_config_info info = {};
> +
> +	report_prefix_push("privileged");
> +
> +	report_prefix_push("pqap");
> +	expect_pgm_int();
> +	enter_pstate();
> +	ap_pqap_qci(&info);
> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
> +	report_prefix_pop();
> +
> +	/*
> +	 * Enqueue and dequeue take too many registers so a simple
> +	 * inline assembly makes more sense than using the library
> +	 * functions.
> +	 */
> +	report_prefix_push("nqap");
> +	expect_pgm_int();
> +	enter_pstate();
> +	asm volatile (
> +		".insn	rre,0xb2ad0000,0,2\n"
> +		: : : "cc", "memory", "0", "1", "2", "3");
> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("dqap");
> +	expect_pgm_int();
> +	enter_pstate();
> +	asm volatile (
> +		".insn	rre,0xb2ae0000,0,2\n"
> +		: : : "cc", "memory", "0", "1", "2", "3");
> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
> +	report_prefix_pop();
> +
> +	report_prefix_pop();
> +}
> +
> +int main(void)
> +{
> +	report_prefix_push("ap");
> +	if (!ap_check()) {
> +		report_skip("AP instructions not available");
> +		goto done;
> +	}
> +
> +	test_priv();
> +	test_pgms_pqap();
> +	test_pgms_nqap();
> +	test_pgms_dqap();
> +
> +done:
> +	report_prefix_pop();
> +	return report_summary();
> +}
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index d97eb5e9..9b7c65c8 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -215,3 +215,7 @@ file = migration-skey.elf
>  smp = 2
>  groups = migration
>  extra_params = -append '--parallel'
> +
> +[ap]
> +file = ap.elf
> +

Re: [kvm-unit-tests PATCH 3/5] lib: s390x: ap: Add ap_setup

[Claudio Imbrenda]

On Thu, 30 Mar 2023 11:42:42 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> For the next tests we need a valid queue which means we need to grab
> the qci info and search the first set bit in the ap and aq masks.
> 
> Let's move from the ap_check function to a proper setup function that
> also returns the first usable APQN. Later we can extend the setup to
> build a list of all available APQNs but right now one APQN is plenty.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/ap.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  lib/s390x/ap.h |  5 ++++-
>  s390x/ap.c     |  7 ++++++-
>  3 files changed, 63 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/s390x/ap.c b/lib/s390x/ap.c
> index 374fa210..8d7f2992 100644
> --- a/lib/s390x/ap.c
> +++ b/lib/s390x/ap.c
> @@ -13,9 +13,12 @@
>  
>  #include <libcflat.h>
>  #include <interrupt.h>
> +#include <bitops.h>
>  #include <ap.h>
>  #include <asm/time.h>
>  
> +static struct ap_config_info qci;
> +
>  int ap_pqap_tapq(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw,
>  		 struct pqap_r2 *r2)
>  {
> @@ -77,7 +80,44 @@ int ap_pqap_qci(struct ap_config_info *info)
>  	return cc;
>  }
>  
> -bool ap_check(void)
> +static int ap_get_apqn(uint8_t *ap, uint8_t *qn)
> +{
> +	unsigned long *ptr;
> +	uint8_t bit;
> +	int i;
> +
> +	ap_pqap_qci(&qci);
> +	*ap = 0;
> +	*qn = 0;
> +
> +	ptr = (unsigned long *)qci.apm;

here it would be nice if qci.apm were an array of longs (as I wrote in
the first patch)

> +	for (i = 0; i < 4; i++) {
> +		bit = fls(*ptr);

fls is implemented using __builtin_clzl, which has undefined behaviour
if the input is all 0

one idea would be to abstract this and write a function that returns
the first bit set in a bit array of arbitrary size

otherwise something like:

for (i = 0; i < 4 && !*ptr; i++);
if (i >= 4)
	return -1;
*ap = 64 * i + 63 - fls(*ptr);

> +		if (bit) {
> +			*ap = i * 64 + 64 - bit;
> +			break;
> +		}
> +	}
> +	ptr = (unsigned long *)qci.aqm;

same here

> +	for (i = 0; i < 4; i++) {
> +		bit = fls(*ptr);
> +		if (bit) {
> +			*qn = i * 64 + 64 - bit;
> +			break;
> +		}
> +	}
> +
> +	if (!*ap || !*qn)
> +		return -1;
> +
> +	/* fls returns 1 + bit number, so we need to remove 1 here */

not really, you did 64 - fls() instead of 63 - fls() (but see my
comment above)

> +	*ap -= 1;
> +	*qn -= 1;
> +
> +	return 0;
> +}
> +
> +static bool ap_check(void)
>  {
>  	struct ap_queue_status r1 = {};
>  	struct pqap_r2 r2 = {};
> @@ -91,3 +131,15 @@ bool ap_check(void)
>  
>  	return true;
>  }
> +
> +int ap_setup(uint8_t *ap, uint8_t *qn)
> +{
> +	if (!ap_check())
> +		return AP_SETUP_NOINSTR;
> +
> +	/* Instructions available but no APQNs */
> +	if (ap_get_apqn(ap, qn))
> +		return AP_SETUP_NOAPQN;
> +
> +	return 0;
> +}
> diff --git a/lib/s390x/ap.h b/lib/s390x/ap.h
> index 79fe6eb0..59595eba 100644
> --- a/lib/s390x/ap.h
> +++ b/lib/s390x/ap.h
> @@ -79,7 +79,10 @@ struct pqap_r2 {
>  } __attribute__((packed))  __attribute__((aligned(8)));
>  _Static_assert(sizeof(struct pqap_r2) == sizeof(uint64_t), "pqap_r2 size");
>  
> -bool ap_check(void);
> +#define AP_SETUP_NOINSTR	-1
> +#define AP_SETUP_NOAPQN		1

this is rather ugly, maybe make it an enum?

	AP_SETUP_OK = 0,
	AP_SETUP_NOAPQN,
	AP_SETUP_NOINSTR

> +
> +int ap_setup(uint8_t *ap, uint8_t *qn);
>  int ap_pqap_tapq(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw,
>  		 struct pqap_r2 *r2);
>  int ap_pqap_qci(struct ap_config_info *info);
> diff --git a/s390x/ap.c b/s390x/ap.c
> index 82ddb6d2..20b4e76e 100644
> --- a/s390x/ap.c
> +++ b/s390x/ap.c
> @@ -16,6 +16,9 @@
>  #include <asm/time.h>
>  #include <ap.h>
>  
> +static uint8_t apn;
> +static uint8_t qn;
> +
>  /* For PQAP PGM checks where we need full control over the input */
>  static void pqap(unsigned long grs[3])
>  {
> @@ -291,8 +294,10 @@ static void test_priv(void)
>  
>  int main(void)
>  {
> +	int setup_rc = ap_setup(&apn, &qn);
> +
>  	report_prefix_push("ap");
> -	if (!ap_check()) {
> +	if (setup_rc == AP_SETUP_NOINSTR) {
>  		report_skip("AP instructions not available");
>  		goto done;
>  	}

Re: [kvm-unit-tests PATCH 4/5] s390x: ap: Add pqap aqic tests

[Claudio Imbrenda]

On Thu, 30 Mar 2023 11:42:43 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> Let's check if we can enable/disable interrupts and if all errors are
> reported if we specify bad addresses for the notification indication
> byte.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/ap.c | 33 +++++++++++++++++++++++++++++
>  lib/s390x/ap.h | 11 ++++++++++
>  s390x/ap.c     | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 100 insertions(+)
> 
> diff --git a/lib/s390x/ap.c b/lib/s390x/ap.c
> index 8d7f2992..aaf5b4b9 100644
> --- a/lib/s390x/ap.c
> +++ b/lib/s390x/ap.c
> @@ -51,6 +51,39 @@ int ap_pqap_tapq(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw,
>  	return cc;
>  }
>  
> +int ap_pqap_aqic(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw,
> +		 struct ap_qirq_ctrl aqic, unsigned long addr)
> +{
> +	struct pqap_r0 r0 = {};
> +	int cc;
> +
> +	/*
> +	 * AP-Queue Interruption Control
> +	 *
> +	 * Enables/disables interrupts for a APQN
> +	 *
> +	 * Inputs: 0,1,2
> +	 * Outputs: 1 (APQSW)
> +	 * Synchronous
> +	 */
> +	r0.ap = ap;
> +	r0.qn = qn;
> +	r0.fc = PQAP_QUEUE_INT_CONTRL;
> +	asm volatile(
> +		"	lgr	0,%[r0]\n"
> +		"	lgr	1,%[aqic]\n"
> +		"	lgr	2,%[addr]\n"
> +		"	.insn	rre,0xb2af0000,0,0\n" /* PQAP */
> +		"	stg	1, %[apqsw]\n"
> +		"	ipm	%[cc]\n"
> +		"	srl	%[cc],28\n"
> +		: [apqsw] "=T" (*apqsw), [cc] "=&d" (cc)
> +		: [r0] "d" (r0), [aqic] "d" (aqic), [addr] "d" (addr)
> +		: "cc", "memory", "0", "2");
> +
> +	return cc;
> +}
> +
>  int ap_pqap_qci(struct ap_config_info *info)
>  {
>  	struct pqap_r0 r0 = { .fc = PQAP_QUERY_AP_CONF_INFO };
> diff --git a/lib/s390x/ap.h b/lib/s390x/ap.h
> index 59595eba..3f9e2eb6 100644
> --- a/lib/s390x/ap.h
> +++ b/lib/s390x/ap.h
> @@ -79,6 +79,15 @@ struct pqap_r2 {
>  } __attribute__((packed))  __attribute__((aligned(8)));
>  _Static_assert(sizeof(struct pqap_r2) == sizeof(uint64_t), "pqap_r2 size");
>  
> +struct ap_qirq_ctrl {
> +	uint64_t res0 : 16;
> +	uint64_t ir    : 1;	/* ir flag: enable (1) or disable (0) irq */
> +	uint64_t res1 : 44;
> +	uint64_t isc   : 3;	/* irq sub class */
> +} __attribute__((packed))  __attribute__((aligned(8)));
> +_Static_assert(sizeof(struct ap_qirq_ctrl) == sizeof(uint64_t),
> +	       "struct ap_qirq_ctrl size");
> +
>  #define AP_SETUP_NOINSTR	-1
>  #define AP_SETUP_NOAPQN		1
>  
> @@ -86,4 +95,6 @@ int ap_setup(uint8_t *ap, uint8_t *qn);
>  int ap_pqap_tapq(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw,
>  		 struct pqap_r2 *r2);
>  int ap_pqap_qci(struct ap_config_info *info);
> +int ap_pqap_aqic(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw,
> +		 struct ap_qirq_ctrl aqic, unsigned long addr);
>  #endif
> diff --git a/s390x/ap.c b/s390x/ap.c
> index 20b4e76e..31dcfe29 100644
> --- a/s390x/ap.c
> +++ b/s390x/ap.c
> @@ -292,6 +292,55 @@ static void test_priv(void)
>  	report_prefix_pop();
>  }
>  
> +static void test_pqap_aqic(void)
> +{
> +	struct ap_queue_status apqsw = {};
> +	static uint8_t not_ind_byte;
> +	struct ap_qirq_ctrl aqic = {};
> +	struct pqap_r2 r2 = {};
> +
> +	int cc;
> +
> +	report_prefix_push("pqap");
> +	report_prefix_push("aqic");
> +
> +	ap_pqap_tapq(apn, qn, &apqsw, &r2);
> +
> +	aqic.ir = 1;
> +	cc = ap_pqap_aqic(apn, qn, &apqsw, aqic, 0);
> +	report(cc == 3 && apqsw.rc == 6, "invalid addr 0");
> +
> +	aqic.ir = 1;
> +	cc = ap_pqap_aqic(apn, qn, &apqsw, aqic, -1);
> +	report(cc == 3 && apqsw.rc == 6, "invalid addr -1");
> +
> +	aqic.ir = 0;
> +	cc = ap_pqap_aqic(apn, qn, &apqsw, aqic, (uintptr_t)&not_ind_byte);
> +	report(cc == 3 && apqsw.rc == 7, "disable");

maybe call it "disable but never enabled"

> +
> +	aqic.ir = 1;
> +	cc = ap_pqap_aqic(apn, qn, &apqsw, aqic, (uintptr_t)&not_ind_byte);
> +	report(cc == 0 && apqsw.rc == 0, "enable");
> +
> +	do {
> +		cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
> +	} while (cc == 0 && apqsw.irq_enabled == 0);

you do this a lot, would it be worth it abstracting it?

ap_pqap_wait_for_irq(..., false); 

(try to find a better name though)

> +
> +	cc = ap_pqap_aqic(apn, qn, &apqsw, aqic, (uintptr_t)&not_ind_byte);
> +	report(cc == 3 && apqsw.rc == 7, "enable while enabled");
> +
> +	aqic.ir = 0;
> +	cc = ap_pqap_aqic(apn, qn, &apqsw, aqic, (uintptr_t)&not_ind_byte);
> +	assert(cc == 0 && apqsw.rc == 0);
> +
> +	do {
> +		cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
> +	} while (cc == 0 && apqsw.irq_enabled == 1);

ap_pqap_wait_for_irq(..., true); 

and here test disable again "disable after disable"

> +
> +	report_prefix_pop();
> +	report_prefix_pop();
> +}
> +
>  int main(void)
>  {
>  	int setup_rc = ap_setup(&apn, &qn);
> @@ -307,6 +356,13 @@ int main(void)
>  	test_pgms_nqap();
>  	test_pgms_dqap();
>  
> +	/* The next tests need queues */
> +	if (setup_rc == AP_SETUP_NOAPQN) {
> +		report_skip("No APQN available");
> +		goto done;
> +	}
> +	test_pqap_aqic();
> +
>  done:
>  	report_prefix_pop();
>  	return report_summary();

Re: [kvm-unit-tests PATCH 5/5] s390x: ap: Add reset tests

[Claudio Imbrenda]

On Thu, 30 Mar 2023 11:42:44 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> Test if the IRQ enablement is turned off on a reset or zeroize PQAP.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---

[...]

> diff --git a/s390x/ap.c b/s390x/ap.c
> index 31dcfe29..47b4f832 100644
> --- a/s390x/ap.c
> +++ b/s390x/ap.c
> @@ -341,6 +341,57 @@ static void test_pqap_aqic(void)
>  	report_prefix_pop();
>  }
>  
> +static void test_pqap_resets(void)
> +{
> +	struct ap_queue_status apqsw = {};
> +	static uint8_t not_ind_byte;
> +	struct ap_qirq_ctrl aqic = {};
> +	struct pqap_r2 r2 = {};
> +
> +	int cc;
> +
> +	report_prefix_push("pqap");
> +	report_prefix_push("rapq");
> +
> +	/* Enable IRQs which the resets will disable */
> +	aqic.ir = 1;
> +	cc = ap_pqap_aqic(apn, qn, &apqsw, aqic, (uintptr_t)&not_ind_byte);
> +	report(cc == 0 && apqsw.rc == 0, "enable");
> +
> +	do {
> +		cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
> +	} while (cc == 0 && apqsw.irq_enabled == 0);

same story here as in the previous patch, waiting for interrupts 

> +	report(apqsw.irq_enabled == 1, "IRQs enabled");
> +
> +	ap_pqap_reset(apn, qn, &apqsw);
> +	cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
> +	assert(!cc);
> +	report(apqsw.irq_enabled == 0, "IRQs have been disabled");
> +
> +	report_prefix_pop();
> +
> +	report_prefix_push("zapq");
> +
> +	/* Enable IRQs which the resets will disable */
> +	aqic.ir = 1;
> +	cc = ap_pqap_aqic(apn, qn, &apqsw, aqic, (uintptr_t)&not_ind_byte);
> +	report(cc == 0 && apqsw.rc == 0, "enable");
> +
> +	do {
> +		cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
> +	} while (cc == 0 && apqsw.irq_enabled == 0);

and here

> +	report(apqsw.irq_enabled == 1, "IRQs enabled");
> +
> +	ap_pqap_reset_zeroize(apn, qn, &apqsw);
> +	cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
> +	assert(!cc);
> +	report(apqsw.irq_enabled == 0, "IRQs have been disabled");
> +
> +	report_prefix_pop();
> +
> +	report_prefix_pop();
> +}
> +
>  int main(void)
>  {
>  	int setup_rc = ap_setup(&apn, &qn);
> @@ -362,6 +413,7 @@ int main(void)
>  		goto done;
>  	}
>  	test_pqap_aqic();
> +	test_pqap_resets();
>  
>  done:
>  	report_prefix_pop();

Re: [kvm-unit-tests PATCH 1/5] lib: s390x: Add ap library

[Janosch Frank]

On 3/30/23 18:09, Claudio Imbrenda wrote:
> On Thu, 30 Mar 2023 11:42:40 +0000
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> Add functions and definitions needed to test the Adjunct
>> Processor (AP) crypto interface.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>
> 
> [...]
> 
>> +bool ap_check(void)
>> +{
>> +	struct ap_queue_status r1 = {};
>> +	struct pqap_r2 r2 = {};
>> +
>> +	/* Base AP support has no STFLE or SCLP feature bit */
> 
> this is true, but you are also indiscriminately using a feature for
> which there is a STFLE feature. since it seems you depend on that, you
> might as well just check bit for STFLE.12 and assume the base support
> is there if it's set

Fair enough.

> 
>> +	expect_pgm_int();
>> +	ap_pqap_tapq(0, 0, &r1, &r2);
>> +
>> +	if (clear_pgm_int() == PGM_INT_CODE_OPERATION)
>> +		return false;
>> +
>> +	return true;
>> +}
> 
> [...]
> 
>> +struct ap_config_info {
>> +	uint8_t apsc	 : 1;	/* S bit */
>> +	uint8_t apxa	 : 1;	/* N bit */
>> +	uint8_t qact	 : 1;	/* C bit */
>> +	uint8_t rc8a	 : 1;	/* R bit */
>> +	uint8_t l	 : 1;	/* L bit */
>> +	uint8_t lext	 : 3;	/* Lext bits */
>> +	uint8_t reserved2[3];
>> +	uint8_t Na;		/* max # of APs - 1 */
>> +	uint8_t Nd;		/* max # of Domains - 1 */
>> +	uint8_t reserved6[10];
>> +	uint32_t apm[8];	/* AP ID mask */
> 
> is there a specific reason why these are uint32_t?
> uint64_t would maybe make your life easier in subsequent patches (see my
> comments there)

That's how the architecture specifies it.
That part of the IO architecture works with words, it seems to be quite old.

Re: [kvm-unit-tests PATCH 2/5] s390x: Add guest 2 AP test

[Janosch Frank]

On 3/30/23 18:34, Claudio Imbrenda wrote:
> On Thu, 30 Mar 2023 11:42:41 +0000
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> Add a test that checks the exceptions for the PQAP, NQAP and DQAP
>> adjunct processor (AP) crypto instructions.
>>
>> Since triggering the exceptions doesn't require actual AP hardware,
>> this test can run without complicated setup.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
> 
> [...]
> 
>> +
>> +static void test_pgms_pqap(void)
>> +{
>> +	unsigned long grs[3] = {};
>> +	struct pqap_r0 *r0 = (struct pqap_r0 *)grs;
>> +	uint8_t *data = alloc_page();
>> +	uint16_t pgm;
>> +	int fails = 0;
>> +	int i;
>> +
>> +	report_prefix_push("pqap");
>> +
>> +	/* Wrong FC code */
>> +	report_prefix_push("invalid fc");
>> +	r0->fc = 42;
> 
> maybe make a macro out of it, both to avoid magic numbers and to change
> it easily if code 42 will ever become defined in the future.

I don't really see a benefit to that.

> 
>> +	expect_pgm_int();
>> +	pqap(grs);
>> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>> +	memset(grs, 0, sizeof(grs));
>> +	report_prefix_pop();
>> +
>> +	report_prefix_push("invalid gr0 bits");
>> +	for (i = 42; i < 6; i++) {
> 
> 42 is not < 6, this whole thing will be skipped?

Right, I've fixed this.

[...]
>> +
>> +static void test_pgms_nqap(void)
>> +{
>> +	uint8_t gr0_zeroes_bits[] = {
>> +		32, 34, 35, 40
>> +	};
>> +	uint64_t gr0;
>> +	bool fail;
>> +	int i;
>> +
>> +	report_prefix_push("nqap");
>> +
>> +	/* Registers 0 and 1 are always used, the others are
>> even/odd pairs */
>> +	report_prefix_push("spec");
>> +	report_prefix_push("r1");
>> +	expect_pgm_int();
>> +	asm volatile (
>> +		".insn	rre,0xb2ad0000,3,6\n"
>> +		: : : "cc", "memory", "0", "1", "2", "3");
> 
> I would say
> "0", "1", "2", "3", "4", "6", "7"
> 
> since there are two ways of doing it wrong when it comes to even-odd
> register pairs (r and r+1, r&~1 and r&~1+1)

R1 & R1 + 1 should never change, same goes for R2.
GR0, GR1, R2 + 1 could potentially change.

But the more interesting question is: Does it make sense to clobber 
anything other than cc (if at all) for the PGM checks? If the PGM fails 
we're in uncharted territory. Seems like I need to look up what the 
other tests do.

Re: [kvm-unit-tests PATCH 5/5] s390x: ap: Add reset tests

[Pierre Morel]

On 3/30/23 13:42, Janosch Frank wrote:
> Test if the IRQ enablement is turned off on a reset or zeroize PQAP.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   lib/s390x/ap.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   lib/s390x/ap.h |  4 +++
>   s390x/ap.c     | 52 ++++++++++++++++++++++++++++++++++++++
>   3 files changed, 124 insertions(+)
>
> diff --git a/lib/s390x/ap.c b/lib/s390x/ap.c
> index aaf5b4b9..d969b2a5 100644
> --- a/lib/s390x/ap.c
> +++ b/lib/s390x/ap.c
> @@ -113,6 +113,74 @@ int ap_pqap_qci(struct ap_config_info *info)
>   	return cc;
>   }
>   
> +static int pqap_reset(uint8_t ap, uint8_t qn, struct ap_queue_status *r1,
> +		      bool zeroize)


NIT. Personal opinion, I find using this bool a little obfuscating and I 
would have prefer 2 different functions.

I see you added a ap_pqap_reset() and ap_pqap_zeroize() next in the code.

Why this intermediate level?


> +{
> +	struct pqap_r0 r0 = {};
> +	int cc;
> +
> +	/*
> +	 * Reset/zeroize AP Queue
> +	 *
> +	 * Resets/zeroizes a queue and disables IRQs
> +	 *
> +	 * Inputs: 0
> +	 * Outputs: 1
> +	 * Asynchronous
> +	 */
> +	r0.ap = ap;
> +	r0.qn = qn;
> +	r0.fc = zeroize ? PQAP_ZEROIZE_APQ : PQAP_RESET_APQ;
> +	asm volatile(
> +		"	lgr	0,%[r0]\n"
> +		"	lgr	1,%[r1]\n"
> +		"	.insn	rre,0xb2af0000,0,0\n" /* PQAP */
> +		"	ipm	%[cc]\n"
> +		"	srl	%[cc],28\n"
> +		: [r1] "+&d" (r1), [cc] "=&d" (cc)
> +		: [r0] "d" (r0)
> +		: "memory");
> +
> +	return cc;
> +}
> +
> +static int pqap_reset_wait(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw,
> +			   bool zeroize)
> +{
> +	struct pqap_r2 r2 = {};
> +	int cc;
> +
> +	cc = pqap_reset(ap, qn, apqsw, zeroize);
> +	/* On a cc == 3 / error we don't need to wait */
> +	if (cc)
> +		return cc;
> +
> +	/*
> +	 * TAPQ returns AP_RC_RESET_IN_PROGRESS if a reset is being
> +	 * processed
> +	 */
> +	do {
> +		cc = ap_pqap_tapq(ap, qn, apqsw, &r2);
> +		/* Give it some time to process before the retry */
> +		mdelay(20);
> +	} while (apqsw->rc == AP_RC_RESET_IN_PROGRESS);
> +
> +	if (apqsw->rc)
> +		printf("Wait for reset failed on ap %d queue %d with tapq rc %d.",
> +			ap, qn, apqsw->rc);
> +	return cc;
> +}
> +
> +int ap_pqap_reset(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw)
> +{
> +	return pqap_reset_wait(ap, qn, apqsw, false);
> +}
> +
> +int ap_pqap_reset_zeroize(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw)
> +{
> +	return pqap_reset_wait(ap, qn, apqsw, true);
> +}
> +
>   static int ap_get_apqn(uint8_t *ap, uint8_t *qn)
>   {
>   	unsigned long *ptr;
> diff --git a/lib/s390x/ap.h b/lib/s390x/ap.h
> index 3f9e2eb6..f9343b5f 100644
> --- a/lib/s390x/ap.h
> +++ b/lib/s390x/ap.h
> @@ -12,6 +12,8 @@
>   #ifndef _S390X_AP_H_
>   #define _S390X_AP_H_
>   
> +#define AP_RC_RESET_IN_PROGRESS	0x02
> +
>   enum PQAP_FC {
>   	PQAP_TEST_APQ,
>   	PQAP_RESET_APQ,
> @@ -94,6 +96,8 @@ _Static_assert(sizeof(struct ap_qirq_ctrl) == sizeof(uint64_t),
>   int ap_setup(uint8_t *ap, uint8_t *qn);
>   int ap_pqap_tapq(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw,
>   		 struct pqap_r2 *r2);
> +int ap_pqap_reset(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw);
> +int ap_pqap_reset_zeroize(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw);
>   int ap_pqap_qci(struct ap_config_info *info);
>   int ap_pqap_aqic(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw,
>   		 struct ap_qirq_ctrl aqic, unsigned long addr);
> diff --git a/s390x/ap.c b/s390x/ap.c
> index 31dcfe29..47b4f832 100644
> --- a/s390x/ap.c
> +++ b/s390x/ap.c
> @@ -341,6 +341,57 @@ static void test_pqap_aqic(void)
>   	report_prefix_pop();
>   }
>   
> +static void test_pqap_resets(void)
> +{
> +	struct ap_queue_status apqsw = {};
> +	static uint8_t not_ind_byte;
> +	struct ap_qirq_ctrl aqic = {};
> +	struct pqap_r2 r2 = {};
> +
> +	int cc;
> +
> +	report_prefix_push("pqap");
> +	report_prefix_push("rapq");
> +
> +	/* Enable IRQs which the resets will disable */
> +	aqic.ir = 1;
> +	cc = ap_pqap_aqic(apn, qn, &apqsw, aqic, (uintptr_t)&not_ind_byte);
> +	report(cc == 0 && apqsw.rc == 0, "enable");

Depending on history I think we could have apqsw == 07 here.

(interrupt already set as requested)


> +
> +	do {
> +		cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);


may be a little delay before retry as you do above for ap_reset_wait()?


> +	} while (cc == 0 && apqsw.irq_enabled == 0);
> +	report(apqsw.irq_enabled == 1, "IRQs enabled");
> +
> +	ap_pqap_reset(apn, qn, &apqsw);
> +	cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
> +	assert(!cc);
> +	report(apqsw.irq_enabled == 0, "IRQs have been disabled");

shouldn't we check that the APQ is fine apqsw.rc == 0 ?


> +
> +	report_prefix_pop();
> +
> +	report_prefix_push("zapq");
> +
> +	/* Enable IRQs which the resets will disable */
> +	aqic.ir = 1;
> +	cc = ap_pqap_aqic(apn, qn, &apqsw, aqic, (uintptr_t)&not_ind_byte);
> +	report(cc == 0 && apqsw.rc == 0, "enable");
> +
> +	do {
> +		cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
> +	} while (cc == 0 && apqsw.irq_enabled == 0);
> +	report(apqsw.irq_enabled == 1, "IRQs enabled");
> +
> +	ap_pqap_reset_zeroize(apn, qn, &apqsw);
> +	cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
> +	assert(!cc);
> +	report(apqsw.irq_enabled == 0, "IRQs have been disabled");
> +
> +	report_prefix_pop();
> +
> +	report_prefix_pop();
> +}
> +
>   int main(void)
>   {
>   	int setup_rc = ap_setup(&apn, &qn);
> @@ -362,6 +413,7 @@ int main(void)
>   		goto done;
>   	}
>   	test_pqap_aqic();
> +	test_pqap_resets();
>   
>   done:
>   	report_prefix_pop();

Re: [kvm-unit-tests PATCH 5/5] s390x: ap: Add reset tests

[Janosch Frank]

On 4/3/23 16:57, Pierre Morel wrote:
> 
> On 3/30/23 13:42, Janosch Frank wrote:
>> Test if the IRQ enablement is turned off on a reset or zeroize PQAP.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>    lib/s390x/ap.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>    lib/s390x/ap.h |  4 +++
>>    s390x/ap.c     | 52 ++++++++++++++++++++++++++++++++++++++
>>    3 files changed, 124 insertions(+)
>>
>> diff --git a/lib/s390x/ap.c b/lib/s390x/ap.c
>> index aaf5b4b9..d969b2a5 100644
>> --- a/lib/s390x/ap.c
>> +++ b/lib/s390x/ap.c
>> @@ -113,6 +113,74 @@ int ap_pqap_qci(struct ap_config_info *info)
>>    	return cc;
>>    }
>>    
>> +static int pqap_reset(uint8_t ap, uint8_t qn, struct ap_queue_status *r1,
>> +		      bool zeroize)
> 
> 
> NIT. Personal opinion, I find using this bool a little obfuscating and I
> would have prefer 2 different functions.
> 
> I see you added a ap_pqap_reset() and ap_pqap_zeroize() next in the code.

Yes, because the names of the functions include the zeroize parts which 
makes it easier for developers to understand how they work instead of 
having a bool argument where they need to look up at which argument 
position it is.

> 
> Why this intermediate level?

So I don't need to repeat the function below for a different r0.fc, no?

[...]

>>    enum PQAP_FC {
>>    	PQAP_TEST_APQ,
>>    	PQAP_RESET_APQ,
>> @@ -94,6 +96,8 @@ _Static_assert(sizeof(struct ap_qirq_ctrl) == sizeof(uint64_t),
>>    int ap_setup(uint8_t *ap, uint8_t *qn);
>>    int ap_pqap_tapq(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw,
>>    		 struct pqap_r2 *r2);
>> +int ap_pqap_reset(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw);
>> +int ap_pqap_reset_zeroize(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw);
>>    int ap_pqap_qci(struct ap_config_info *info);
>>    int ap_pqap_aqic(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw,
>>    		 struct ap_qirq_ctrl aqic, unsigned long addr);
>> diff --git a/s390x/ap.c b/s390x/ap.c
>> index 31dcfe29..47b4f832 100644
>> --- a/s390x/ap.c
>> +++ b/s390x/ap.c
>> @@ -341,6 +341,57 @@ static void test_pqap_aqic(void)
>>    	report_prefix_pop();
>>    }
>>    
>> +static void test_pqap_resets(void)
>> +{
>> +	struct ap_queue_status apqsw = {};
>> +	static uint8_t not_ind_byte;
>> +	struct ap_qirq_ctrl aqic = {};
>> +	struct pqap_r2 r2 = {};
>> +
>> +	int cc;
>> +
>> +	report_prefix_push("pqap");
>> +	report_prefix_push("rapq");
>> +
>> +	/* Enable IRQs which the resets will disable */
>> +	aqic.ir = 1;
>> +	cc = ap_pqap_aqic(apn, qn, &apqsw, aqic, (uintptr_t)&not_ind_byte);
>> +	report(cc == 0 && apqsw.rc == 0, "enable");
> 
> Depending on history I think we could have apqsw == 07 here.
> 
> (interrupt already set as requested)

I'd much rather grab a tapq and assert that ir == 0 so if someone alters 
the code they are responsible for giving this function a reset queue.

I'll add a comment that we expect ir == 0 for this function.

> 
> 
>> +
>> +	do {
>> +		cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
> 
> 
> may be a little delay before retry as you do above for ap_reset_wait()?

Yes

> 
> 
>> +	} while (cc == 0 && apqsw.irq_enabled == 0);
>> +	report(apqsw.irq_enabled == 1, "IRQs enabled");
>> +
>> +	ap_pqap_reset(apn, qn, &apqsw);
>> +	cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
>> +	assert(!cc);
>> +	report(apqsw.irq_enabled == 0, "IRQs have been disabled");
> 
> shouldn't we check that the APQ is fine apqsw.rc == 0 ?

Isn't that covered by the assert above?

> 
> 
>> +
>> +	report_prefix_pop();
>> +
>> +	report_prefix_push("zapq");
>> +
>> +	/* Enable IRQs which the resets will disable */
>> +	aqic.ir = 1;
>> +	cc = ap_pqap_aqic(apn, qn, &apqsw, aqic, (uintptr_t)&not_ind_byte);
>> +	report(cc == 0 && apqsw.rc == 0, "enable");
>> +
>> +	do {
>> +		cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
>> +	} while (cc == 0 && apqsw.irq_enabled == 0);
>> +	report(apqsw.irq_enabled == 1, "IRQs enabled");
>> +
>> +	ap_pqap_reset_zeroize(apn, qn, &apqsw);
>> +	cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
>> +	assert(!cc);
>> +	report(apqsw.irq_enabled == 0, "IRQs have been disabled");
>> +
>> +	report_prefix_pop();
>> +
>> +	report_prefix_pop();
>> +}
>> +
>>    int main(void)
>>    {
>>    	int setup_rc = ap_setup(&apn, &qn);
>> @@ -362,6 +413,7 @@ int main(void)
>>    		goto done;
>>    	}
>>    	test_pqap_aqic();
>> +	test_pqap_resets();
>>    
>>    done:
>>    	report_prefix_pop();

Re: [kvm-unit-tests PATCH 5/5] s390x: ap: Add reset tests

[Pierre Morel]

On 4/4/23 13:40, Janosch Frank wrote:
> On 4/3/23 16:57, Pierre Morel wrote:
>>
>> On 3/30/23 13:42, Janosch Frank wrote:
>>> Test if the IRQ enablement is turned off on a reset or zeroize PQAP.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>>    lib/s390x/ap.c | 68 
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>    lib/s390x/ap.h |  4 +++
>>>    s390x/ap.c     | 52 ++++++++++++++++++++++++++++++++++++++
>>>    3 files changed, 124 insertions(+)
>>>
>>> diff --git a/lib/s390x/ap.c b/lib/s390x/ap.c
>>> index aaf5b4b9..d969b2a5 100644
>>> --- a/lib/s390x/ap.c
>>> +++ b/lib/s390x/ap.c
>>> @@ -113,6 +113,74 @@ int ap_pqap_qci(struct ap_config_info *info)
>>>        return cc;
>>>    }
>>>    +static int pqap_reset(uint8_t ap, uint8_t qn, struct 
>>> ap_queue_status *r1,
>>> +              bool zeroize)
>>
>>
>> NIT. Personal opinion, I find using this bool a little obfuscating and I
>> would have prefer 2 different functions.
>>
>> I see you added a ap_pqap_reset() and ap_pqap_zeroize() next in the 
>> code.
>
> Yes, because the names of the functions include the zeroize parts 
> which makes it easier for developers to understand how they work 
> instead of having a bool argument where they need to look up at which 
> argument position it is.
>
>>
>> Why this intermediate level?
>
> So I don't need to repeat the function below for a different r0.fc, no?


question of taste anyway.


[...]


>>
>>
>>> +    } while (cc == 0 && apqsw.irq_enabled == 0);
>>> +    report(apqsw.irq_enabled == 1, "IRQs enabled");
>>> +
>>> +    ap_pqap_reset(apn, qn, &apqsw);
>>> +    cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
>>> +    assert(!cc);
>>> +    report(apqsw.irq_enabled == 0, "IRQs have been disabled");
>>
>> shouldn't we check that the APQ is fine apqsw.rc == 0 ?
>
> Isn't that covered by the assert above?

May be.

This is the kind of thing where I find the implementation and 
documentation not very logical.

- CC = 0 means that the instruction was processed correctly.

- APQSW reports the status of the AP queue

For any operation but TAPQ I understand that CC=3 if APQSW is != 0

but for TAPQ, if it is processed correctly it should give back the 
APQSW. Isn't it exactly what we ask the TAPQ to do?

I am probably not the only one to think that CC for TAPQ is at least not 
useful, the Linux implementation ignores it.

You are probably right but in doubt I would do as in Linux 
implementation and ignore CC,