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

[kvm-unit-tests PATCH v4 0/7] 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 sheer complexity.

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

v4:
	- Rebase on the cc dirty series
	- Bumped year to 2024
v3:
	- Renamed ap_check() to ap_setup() and added comment
v2:
	- Re-worked the ap_check() function to test for stfle 12 since
          we rely on PQAP QCI in the library functions
	- Re-worked APQN management
	- Fixed faulty loop variable initializers in ap.c
	- Fixed report messages
	- Extended clobber lists
	- Extended length bit checks for nqap
	- Now using ARRAY_SIZE where applicabale
	- NIB is now allocated as IO memory


Janosch Frank (7):
  lib: s390x: Add ap library
  s390x: Add guest 2 AP test
  lib: s390x: ap: Add proper ap setup code
  s390x: ap: Add pqap aqic tests
  s390x: ap: Add reset tests
  lib: s390x: ap: Add tapq test facility bit
  s390x: ap: Add nq/dq len test

 lib/s390x/ap.c      | 286 ++++++++++++++++++++++
 lib/s390x/ap.h      | 119 ++++++++++
 s390x/Makefile      |   2 +
 s390x/ap.c          | 564 ++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |   3 +
 5 files changed, 974 insertions(+)
 create mode 100644 lib/s390x/ap.c
 create mode 100644 lib/s390x/ap.h
 create mode 100644 s390x/ap.c

-- 
2.40.1

[kvm-unit-tests PATCH v4 1/7] 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 | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/s390x/ap.h | 88 +++++++++++++++++++++++++++++++++++++++++++++
 s390x/Makefile |  1 +
 3 files changed, 186 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..9560ff64
--- /dev/null
+++ b/lib/s390x/ap.c
@@ -0,0 +1,97 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * AP crypto functions
+ *
+ * Some parts taken from the Linux AP driver.
+ *
+ * Copyright IBM Corp. 2024
+ * 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>
+#include <asm/facility.h>
+
+int ap_pqap_tapq(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw,
+		 struct pqap_r2 *r2)
+{
+	struct pqap_r0 r0 = {
+		.ap = ap,
+		.qn = qn,
+		.fc = PQAP_TEST_APQ
+	};
+	uint64_t bogus_cc = 2;
+	int cc;
+
+	/*
+	 * Test AP Queue
+	 *
+	 * Writes AP configuration information to the memory pointed
+	 * at by GR2.
+	 *
+	 * Inputs: GR0
+	 * Outputs: GR1 (APQSW), GR2 (tapq data)
+	 * Synchronous
+	 */
+	asm volatile(
+		"       tmll	%[bogus_cc],3\n"
+		"	lgr	0,%[r0]\n"
+		"	.insn	rre,0xb2af0000,0,0\n" /* PQAP */
+		"	stg	1,%[apqsw]\n"
+		"	stg	2,%[r2]\n"
+		"	ipm	%[cc]\n"
+		"	srl	%[cc],28\n"
+		: [apqsw] "=&T" (*apqsw), [r2] "=&T" (*r2), [cc] "=&d" (cc)
+		: [r0] "d" (r0), [bogus_cc] "d" (bogus_cc));
+
+	return cc;
+}
+
+int ap_pqap_qci(struct ap_config_info *info)
+{
+	struct pqap_r0 r0 = { .fc = PQAP_QUERY_AP_CONF_INFO };
+	uint64_t bogus_cc = 2;
+	int cc;
+
+	/*
+	 * Query AP Configuration Information
+	 *
+	 * Writes AP configuration information to the memory pointed
+	 * at by GR2.
+	 *
+	 * Inputs: GR0, GR2 (QCI block address)
+	 * Outputs: memory at GR2 address
+	 * Synchronous
+	 */
+	asm volatile(
+		"       tmll	%[bogus_cc],3\n"
+		"	lgr	0,%[r0]\n"
+		"	lgr	2,%[info]\n"
+		"	.insn	rre,0xb2af0000,0,0\n" /* PQAP */
+		"	ipm	%[cc]\n"
+		"	srl	%[cc],28\n"
+		: [cc] "=&d" (cc)
+		: [r0] "d" (r0), [info] "d" (info), [bogus_cc] "d" (bogus_cc)
+		: "cc", "memory", "0", "2");
+
+	return cc;
+}
+
+/* Will later be extended to a proper setup function */
+bool ap_setup(void)
+{
+	/*
+	 * Base AP support has no STFLE or SCLP feature bit but the
+	 * PQAP QCI support is indicated via stfle bit 12. As this
+	 * library relies on QCI we bail out if it's not available.
+	 */
+	if (!test_facility(12))
+		return false;
+
+	return true;
+}
diff --git a/lib/s390x/ap.h b/lib/s390x/ap.h
new file mode 100644
index 00000000..b806513f
--- /dev/null
+++ b/lib/s390x/ap.h
@@ -0,0 +1,88 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * AP definitions
+ *
+ * Some parts taken from the Linux AP driver.
+ *
+ * Copyright IBM Corp. 2024
+ * 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_setup(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
diff --git a/s390x/Makefile b/s390x/Makefile
index 7fce9f9d..4f6c627d 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -110,6 +110,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
 
-- 
2.40.1

[kvm-unit-tests PATCH v4 2/7] 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      |   1 +
 s390x/ap.c          | 309 ++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |   3 +
 3 files changed, 313 insertions(+)
 create mode 100644 s390x/ap.c

diff --git a/s390x/Makefile b/s390x/Makefile
index 4f6c627d..6d28a5bf 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -42,6 +42,7 @@ tests += $(TEST_DIR)/exittime.elf
 tests += $(TEST_DIR)/ex.elf
 tests += $(TEST_DIR)/topology.elf
 tests += $(TEST_DIR)/sie-dat.elf
+tests += $(TEST_DIR)/ap.elf
 
 pv-tests += $(TEST_DIR)/pv-diags.elf
 pv-tests += $(TEST_DIR)/pv-icptcode.elf
diff --git a/s390x/ap.c b/s390x/ap.c
new file mode 100644
index 00000000..b3cee37a
--- /dev/null
+++ b/s390x/ap.c
@@ -0,0 +1,309 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * AP instruction G2 tests
+ *
+ * Copyright (c) 2024 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");
+	/*
+	 * GR0 bits 41 - 47 are defined 0 and result in a
+	 * specification exception if set to 1.
+	 */
+	for (i = 0; i < 48 - 41; i++) {
+		grs[0] = BIT(63 - 47 + i);
+
+		expect_pgm_int();
+		pqap(grs);
+		pgm = clear_pgm_int();
+
+		if (pgm != PGM_INT_CODE_SPECIFICATION) {
+			report_fail("fail on bit %d", 42 + 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 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 alignments 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", "4", "6", "7");
+	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", "2", "3", "4", "6", "7");
+	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+	report_prefix_pop();
+
+	report_prefix_push("both");
+	expect_pgm_int();
+	asm volatile (
+		".insn	rre,0xb2ad0000,3,7\n"
+		: : : "cc", "memory", "0", "1", "2", "3", "4", "6", "7");
+	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", "6", "7");
+	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+	report_prefix_pop();
+
+	report_prefix_push("gr0_zero_bits");
+	fail = false;
+	for (i = 0; i < ARRAY_SIZE(gr0_zeroes_bits); 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", "6", "7");
+		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 gr0 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", "4", "6", "7");
+	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", "2", "3", "4", "6", "7");
+	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+	report_prefix_pop();
+
+	report_prefix_push("both");
+	expect_pgm_int();
+	asm volatile (
+		".insn	rre,0xb2ae0000,3,7\n"
+		: : : "cc", "memory", "0", "1", "2", "3", "4", "6", "7");
+	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", "6", "7");
+	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+	report_prefix_pop();
+
+	report_prefix_push("gr0_zero_bits");
+	fail = false;
+	for (i = 0; i < ARRAY_SIZE(gr0_zeroes_bits); 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", "6", "7");
+		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 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 018e4129..578375e4 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -386,3 +386,6 @@ file = sie-dat.elf
 
 [pv-attest]
 file = pv-attest.elf
+
+[ap]
+file = ap.elf
-- 
2.40.1

[kvm-unit-tests PATCH v4 3/7] lib: s390x: ap: Add proper ap setup code

[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 stfle 12 check to proper setup code that also
returns arrays for the aps and qns.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/ap.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++---
 lib/s390x/ap.h | 17 +++++++++-
 s390x/ap.c     |  4 ++-
 3 files changed, 105 insertions(+), 6 deletions(-)

diff --git a/lib/s390x/ap.c b/lib/s390x/ap.c
index 9560ff64..c85172a8 100644
--- a/lib/s390x/ap.c
+++ b/lib/s390x/ap.c
@@ -13,10 +13,18 @@
 
 #include <libcflat.h>
 #include <interrupt.h>
+#include <alloc.h>
+#include <bitops.h>
 #include <ap.h>
 #include <asm/time.h>
 #include <asm/facility.h>
 
+static uint8_t num_ap;
+static uint8_t num_queue;
+static uint8_t *array_ap;
+static uint8_t *array_queue;
+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)
 {
@@ -82,16 +90,90 @@ int ap_pqap_qci(struct ap_config_info *info)
 	return cc;
 }
 
-/* Will later be extended to a proper setup function */
-bool ap_setup(void)
+static int get_entry(uint8_t *ptr, int i, size_t len)
 {
+	/* Skip over the last entry */
+	if (i)
+		i++;
+
+	for (; i < 8 * len; i++) {
+		/* Do we even need to check the byte? */
+		if (!ptr[i / 8]) {
+			i += 7;
+			continue;
+		}
+
+		/* Check the bit in big-endian order */
+		if (ptr[i / 8] & BIT(7 - (i % 8)))
+			return i;
+	}
+	return -1;
+}
+
+static void setup_info(void)
+{
+	int i, entry = 0;
+
+	ap_pqap_qci(&qci);
+
+	for (i = 0; i < AP_ARRAY_MAX_NUM; i++) {
+		entry = get_entry((uint8_t *)qci.apm, entry, sizeof(qci.apm));
+		if (entry == -1)
+			break;
+
+		if (!num_ap) {
+			/*
+			 * We have at least one AP, time to
+			 * allocate the queue arrays
+			 */
+			array_ap = malloc(AP_ARRAY_MAX_NUM);
+			array_queue = malloc(AP_ARRAY_MAX_NUM);
+		}
+		array_ap[num_ap] = entry;
+		num_ap += 1;
+	}
+
+	/* Without an AP we don't even need to look at the queues */
+	if (!num_ap)
+		return;
+
+	entry = 0;
+	for (i = 0; i < AP_ARRAY_MAX_NUM; i++) {
+		entry = get_entry((uint8_t *)qci.aqm, entry, sizeof(qci.aqm));
+		if (entry == -1)
+			return;
+
+		array_queue[num_queue] = entry;
+		num_queue += 1;
+	}
+
+}
+
+int ap_setup(uint8_t **ap_array, uint8_t **qn_array, uint8_t *naps, uint8_t *nqns)
+{
+	assert(!num_ap);
+
 	/*
 	 * Base AP support has no STFLE or SCLP feature bit but the
 	 * PQAP QCI support is indicated via stfle bit 12. As this
 	 * library relies on QCI we bail out if it's not available.
 	 */
 	if (!test_facility(12))
-		return false;
+		return AP_SETUP_NOINSTR;
 
-	return true;
+	/* Setup ap and queue arrays */
+	setup_info();
+
+	if (!num_ap)
+		return AP_SETUP_NOAPQN;
+
+	/* Only provide the data if the caller actually needs it. */
+	if (ap_array && qn_array && naps && nqns) {
+		*ap_array = array_ap;
+		*qn_array = array_queue;
+		*naps = num_ap;
+		*nqns = num_queue;
+	}
+
+	return AP_SETUP_READY;
 }
diff --git a/lib/s390x/ap.h b/lib/s390x/ap.h
index b806513f..8feca43a 100644
--- a/lib/s390x/ap.h
+++ b/lib/s390x/ap.h
@@ -81,7 +81,22 @@ struct pqap_r2 {
 } __attribute__((packed))  __attribute__((aligned(8)));
 _Static_assert(sizeof(struct pqap_r2) == sizeof(uint64_t), "pqap_r2 size");
 
-bool ap_setup(void);
+/*
+ * Maximum number of APQNs that we keep track of.
+ *
+ * This value is already way larger than the number of APQNs a AP test
+ * is probably going to use.
+ */
+#define AP_ARRAY_MAX_NUM	16
+
+/* Return values of ap_setup() */
+enum {
+	AP_SETUP_NOINSTR,	/* AP instructions not available */
+	AP_SETUP_NOAPQN,	/* Instructions available but no APQNs */
+	AP_SETUP_READY		/* Full setup complete, at least one APQN */
+};
+
+int ap_setup(uint8_t **ap_array, uint8_t **qn_array, uint8_t *naps, uint8_t *nqns);
 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 b3cee37a..5c458e7e 100644
--- a/s390x/ap.c
+++ b/s390x/ap.c
@@ -292,8 +292,10 @@ static void test_priv(void)
 
 int main(void)
 {
+	int setup_rc = ap_setup(NULL, NULL, NULL, NULL);
+
 	report_prefix_push("ap");
-	if (!ap_check()) {
+	if (setup_rc == AP_SETUP_NOINSTR) {
 		report_skip("AP instructions not available");
 		goto done;
 	}
-- 
2.40.1

[kvm-unit-tests PATCH v4 4/7] 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 | 35 +++++++++++++++++++++++
 lib/s390x/ap.h | 11 ++++++++
 s390x/ap.c     | 75 +++++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 120 insertions(+), 1 deletion(-)

diff --git a/lib/s390x/ap.c b/lib/s390x/ap.c
index c85172a8..4e7d3d34 100644
--- a/lib/s390x/ap.c
+++ b/lib/s390x/ap.c
@@ -60,6 +60,41 @@ 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)
+{
+	uint64_t bogus_cc = 2;
+	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(
+		"       tmll	%[bogus_cc],3\n"
+		"	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), [bogus_cc] "d" (bogus_cc)
+		: "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 8feca43a..15394d56 100644
--- a/lib/s390x/ap.h
+++ b/lib/s390x/ap.h
@@ -81,6 +81,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");
+
 /*
  * Maximum number of APQNs that we keep track of.
  *
@@ -100,4 +109,6 @@ int ap_setup(uint8_t **ap_array, uint8_t **qn_array, uint8_t *naps, uint8_t *nqn
 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 5c458e7e..6f95a716 100644
--- a/s390x/ap.c
+++ b/s390x/ap.c
@@ -12,10 +12,15 @@
 #include <interrupt.h>
 #include <bitops.h>
 #include <alloc_page.h>
+#include <malloc_io.h>
+#include <asm/page.h>
 #include <asm/facility.h>
 #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])
 {
@@ -290,9 +295,63 @@ static void test_priv(void)
 	report_prefix_pop();
 }
 
+static void test_pqap_aqic(void)
+{
+	uint8_t *not_ind_byte = alloc_io_mem(PAGE_SIZE, 0);
+	struct ap_queue_status apqsw = {};
+	struct ap_qirq_ctrl aqic = {};
+	struct pqap_r2 r2 = {};
+	int cc;
+
+	report_prefix_push("aqic");
+	*not_ind_byte = 0;
+
+	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 IRQs while already disabled");
+
+	aqic.ir = 1;
+	cc = ap_pqap_aqic(apn, qn, &apqsw, aqic, (uintptr_t)not_ind_byte);
+	report(cc == 0 && apqsw.rc == 0, "enable IRQs");
+
+	do {
+		mdelay(20);
+		cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
+	} while (cc == 0 && apqsw.irq_enabled == 0);
+	report(cc == 0 && apqsw.irq_enabled == 1, "enable IRQs tapq data");
+
+	cc = ap_pqap_aqic(apn, qn, &apqsw, aqic, (uintptr_t)not_ind_byte);
+	report(cc == 3 && apqsw.rc == 7, "enable IRQs while already enabled");
+
+	aqic.ir = 0;
+	cc = ap_pqap_aqic(apn, qn, &apqsw, aqic, (uintptr_t)not_ind_byte);
+	report(cc == 0 && apqsw.rc == 0, "disable IRQs");
+
+	do {
+		mdelay(20);
+		cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
+	} while (cc == 0 && apqsw.irq_enabled == 1);
+	report(cc == 0 && apqsw.irq_enabled == 0, "disable IRQs tapq data");
+
+	report_prefix_pop();
+
+	free_io_mem(not_ind_byte, PAGE_SIZE);
+}
+
 int main(void)
 {
-	int setup_rc = ap_setup(NULL, NULL, NULL, NULL);
+	uint8_t num_ap, num_qn;
+	uint8_t *apns;
+	uint8_t *qns;
+	int setup_rc = ap_setup(&apns, &qns, &num_ap, &num_qn);
 
 	report_prefix_push("ap");
 	if (setup_rc == AP_SETUP_NOINSTR) {
@@ -305,6 +364,20 @@ 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;
+	}
+	apn = apns[0];
+	qn = qns[0];
+	report_prefix_push("pqap");
+	if (test_facility(65))
+		test_pqap_aqic();
+	else
+		report_skip("no AQIC and reset tests without IRQ support");
+	report_prefix_pop();
+
 done:
 	report_prefix_pop();
 	return report_summary();
-- 
2.40.1

[kvm-unit-tests PATCH v4 5/7] 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 | 71 +++++++++++++++++++++++++++++++++++++++++++
 lib/s390x/ap.h |  4 +++
 s390x/ap.c     | 81 ++++++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 154 insertions(+), 2 deletions(-)

diff --git a/lib/s390x/ap.c b/lib/s390x/ap.c
index 4e7d3d34..0f92739d 100644
--- a/lib/s390x/ap.c
+++ b/lib/s390x/ap.c
@@ -125,6 +125,77 @@ 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 *apqsw,
+		      bool zeroize)
+{
+	uint64_t bogus_cc = 2;
+	struct pqap_r0 r0 = {};
+	int cc;
+
+	/*
+	 * Reset/zeroize AP Queue
+	 *
+	 * Resets/zeroizes a queue and disables IRQs
+	 *
+	 * Inputs: GR0
+	 * Outputs: GR1 (APQSW)
+	 * Asynchronous
+	 */
+	r0.ap = ap;
+	r0.qn = qn;
+	r0.fc = zeroize ? PQAP_ZEROIZE_APQ : PQAP_RESET_APQ;
+	asm volatile(
+		"       tmll	%[bogus_cc],3\n"
+		"	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), [cc] "=&d" (cc)
+		: [r0] "d" (r0), [bogus_cc] "d" (bogus_cc)
+		: "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 {
+		/* Give it some time to process before the retry */
+		mdelay(20);
+		cc = ap_pqap_tapq(ap, qn, apqsw, &r2);
+	} 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 get_entry(uint8_t *ptr, int i, size_t len)
 {
 	/* Skip over the last entry */
diff --git a/lib/s390x/ap.h b/lib/s390x/ap.h
index 15394d56..dbdb55c4 100644
--- a/lib/s390x/ap.h
+++ b/lib/s390x/ap.h
@@ -14,6 +14,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,
@@ -108,6 +110,8 @@ enum {
 int ap_setup(uint8_t **ap_array, uint8_t **qn_array, uint8_t *naps, uint8_t *nqns);
 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 6f95a716..38be03eb 100644
--- a/s390x/ap.c
+++ b/s390x/ap.c
@@ -13,6 +13,7 @@
 #include <bitops.h>
 #include <alloc_page.h>
 #include <malloc_io.h>
+#include <uv.h>
 #include <asm/page.h>
 #include <asm/facility.h>
 #include <asm/time.h>
@@ -346,6 +347,80 @@ static void test_pqap_aqic(void)
 	free_io_mem(not_ind_byte, PAGE_SIZE);
 }
 
+static void test_pqap_resets(void)
+{
+	uint8_t *not_ind_byte = alloc_io_mem(sizeof(*not_ind_byte), 0);
+	struct ap_queue_status apqsw = {};
+	struct ap_qirq_ctrl aqic = {};
+	struct pqap_r2 r2 = {};
+
+	int cc;
+
+	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 IRQs for reset tests");
+
+	do {
+		mdelay(20);
+		cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
+	} while (cc == 0 && apqsw.irq_enabled == 0);
+	report(apqsw.irq_enabled == 1, "IRQs enabled tapq data");
+
+	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 via reset");
+
+	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 IRQs for reset tests");
+
+	do {
+		mdelay(20);
+		cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
+	} while (cc == 0 && apqsw.irq_enabled == 0);
+	report(apqsw.irq_enabled == 1, "IRQs enabled tapq data");
+
+	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 via reset");
+
+	report_prefix_pop();
+	/*
+	 * This is a wrinkle in the architecture for PV guests.
+	 *
+	 * The notification byte is pinned shared for PV guests.
+	 * RAPQ, ZAPQ and AQIC can all disable IRQs but there's no
+	 * intercept for resets triggered by a PV guests. Hence the
+	 * host keeps the notification byte page pinned UNTIL IRQs are
+	 * disabled via AQIC.
+	 *
+	 * Firmware will not generate an intercept if the IRQs have
+	 * already been disabled via a reset. Therefore we need to
+	 * enable AND disable to achieve a disable.
+	 */
+	if (uv_os_is_guest()) {
+		aqic.ir = 1;
+		cc = ap_pqap_aqic(apn, qn, &apqsw, aqic,
+				  (uintptr_t)not_ind_byte);
+		assert(cc == 0 && apqsw.rc == 0);
+		aqic.ir = 0;
+		cc = ap_pqap_aqic(apn, qn, &apqsw, aqic,
+				  (uintptr_t)not_ind_byte);
+		assert(cc == 0 && apqsw.rc == 0);
+	}
+	free_io_mem(not_ind_byte, sizeof(*not_ind_byte));
+}
+
 int main(void)
 {
 	uint8_t num_ap, num_qn;
@@ -372,10 +447,12 @@ int main(void)
 	apn = apns[0];
 	qn = qns[0];
 	report_prefix_push("pqap");
-	if (test_facility(65))
+	if (test_facility(65)) {
 		test_pqap_aqic();
-	else
+		test_pqap_resets();
+	} else {
 		report_skip("no AQIC and reset tests without IRQ support");
+	}
 	report_prefix_pop();
 
 done:
-- 
2.40.1

[kvm-unit-tests PATCH v4 6/7] lib: s390x: ap: Add tapq test facility bit

[Janosch Frank]

When the t bit is one the first 32 bits of register 2 are populated on
a tapq. Those bits tell us in which mode the queu is and which
facilities it supports.

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

diff --git a/lib/s390x/ap.c b/lib/s390x/ap.c
index 0f92739d..64705a56 100644
--- a/lib/s390x/ap.c
+++ b/lib/s390x/ap.c
@@ -26,11 +26,12 @@ static uint8_t *array_queue;
 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)
+		 struct pqap_r2 *r2, bool t)
 {
 	struct pqap_r0 r0 = {
 		.ap = ap,
 		.qn = qn,
+		.t = t,
 		.fc = PQAP_TEST_APQ
 	};
 	uint64_t bogus_cc = 2;
@@ -177,7 +178,7 @@ static int pqap_reset_wait(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw
 	do {
 		/* Give it some time to process before the retry */
 		mdelay(20);
-		cc = ap_pqap_tapq(ap, qn, apqsw, &r2);
+		cc = ap_pqap_tapq(ap, qn, apqsw, &r2, false);
 	} while (apqsw->rc == AP_RC_RESET_IN_PROGRESS);
 
 	if (apqsw->rc)
diff --git a/lib/s390x/ap.h b/lib/s390x/ap.h
index dbdb55c4..eecb39be 100644
--- a/lib/s390x/ap.h
+++ b/lib/s390x/ap.h
@@ -109,7 +109,7 @@ enum {
 
 int ap_setup(uint8_t **ap_array, uint8_t **qn_array, uint8_t *naps, uint8_t *nqns);
 int ap_pqap_tapq(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw,
-		 struct pqap_r2 *r2);
+		 struct pqap_r2 *r2, bool t);
 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);
diff --git a/s390x/ap.c b/s390x/ap.c
index 38be03eb..0f2d03c2 100644
--- a/s390x/ap.c
+++ b/s390x/ap.c
@@ -325,7 +325,7 @@ static void test_pqap_aqic(void)
 
 	do {
 		mdelay(20);
-		cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
+		cc = ap_pqap_tapq(apn, qn, &apqsw, &r2, false);
 	} while (cc == 0 && apqsw.irq_enabled == 0);
 	report(cc == 0 && apqsw.irq_enabled == 1, "enable IRQs tapq data");
 
@@ -338,7 +338,7 @@ static void test_pqap_aqic(void)
 
 	do {
 		mdelay(20);
-		cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
+		cc = ap_pqap_tapq(apn, qn, &apqsw, &r2, false);
 	} while (cc == 0 && apqsw.irq_enabled == 1);
 	report(cc == 0 && apqsw.irq_enabled == 0, "disable IRQs tapq data");
 
@@ -365,12 +365,12 @@ static void test_pqap_resets(void)
 
 	do {
 		mdelay(20);
-		cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
+		cc = ap_pqap_tapq(apn, qn, &apqsw, &r2, false);
 	} while (cc == 0 && apqsw.irq_enabled == 0);
 	report(apqsw.irq_enabled == 1, "IRQs enabled tapq data");
 
 	ap_pqap_reset(apn, qn, &apqsw);
-	cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
+	cc = ap_pqap_tapq(apn, qn, &apqsw, &r2, false);
 	assert(!cc);
 	report(apqsw.irq_enabled == 0, "IRQs have been disabled via reset");
 
@@ -385,12 +385,12 @@ static void test_pqap_resets(void)
 
 	do {
 		mdelay(20);
-		cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
+		cc = ap_pqap_tapq(apn, qn, &apqsw, &r2, false);
 	} while (cc == 0 && apqsw.irq_enabled == 0);
 	report(apqsw.irq_enabled == 1, "IRQs enabled tapq data");
 
 	ap_pqap_reset_zeroize(apn, qn, &apqsw);
-	cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
+	cc = ap_pqap_tapq(apn, qn, &apqsw, &r2, false);
 	assert(!cc);
 	report(apqsw.irq_enabled == 0, "IRQs have been disabled via reset");
 
-- 
2.40.1

[kvm-unit-tests PATCH v4 7/7] s390x: ap: Add nq/dq len test

[Janosch Frank]

For years the nqap/dqap max length was 12KB but with a recent machine
extended message length support was introduced. The support is AP type
and generation specific, so it can vary from card to card which
complicates testing by a lot.

This test will use the APQN that all other tests use no matter if
there's extended length support or not. But if longer messages are
supported by the APQN we need to adapt our tests.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/ap.h |   3 +-
 s390x/ap.c     | 103 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 105 insertions(+), 1 deletion(-)

diff --git a/lib/s390x/ap.h b/lib/s390x/ap.h
index eecb39be..792149ea 100644
--- a/lib/s390x/ap.h
+++ b/lib/s390x/ap.h
@@ -77,7 +77,8 @@ struct pqap_r2 {
 	uint8_t pad_1[3];
 	uint8_t at;
 	uint8_t nd;
-	uint8_t pad_6;
+	uint8_t pad_6 : 4;
+	uint8_t ml : 4;
 	uint8_t pad_7 : 4;
 	uint8_t qd : 4;
 } __attribute__((packed))  __attribute__((aligned(8)));
diff --git a/s390x/ap.c b/s390x/ap.c
index 0f2d03c2..edb4943b 100644
--- a/s390x/ap.c
+++ b/s390x/ap.c
@@ -257,6 +257,106 @@ static void test_pgms_dqap(void)
 	report_prefix_pop();
 }
 
+/*
+ * For years the nqap/dqap max length was 12KB but with a recent
+ * machine extended message length support was introduced. The support
+ * is AP type and generation specific, so it can vary from card to
+ * card.
+ *
+ * This test will use the APQN that all other tests use no matter if
+ * there's extended length support or not. But if longer messages are
+ * supported by the APQN we need to adapt our tests.
+ */
+static void test_pgms_nqdq_len(void)
+{
+	struct ap_queue_status apqsw = {};
+	struct pqap_r2 r2 = {};
+	uint64_t len, mlen;
+	bool fail;
+	int i;
+
+	/* Extended message support is reported via tapq with T=1 */
+	ap_pqap_tapq(apn, qn, &apqsw, &r2, true);
+	/* < 3 means 3 because of backwards compatibility */
+	mlen = r2.ml ? r2.ml : 3;
+	/* Len is reported in pages */
+	mlen *= PAGE_SIZE;
+
+	report_prefix_push("nqap");
+	report_prefix_push("spec");
+
+	report_prefix_push("len + 1");
+	expect_pgm_int();
+	len = mlen + 1;
+	asm volatile (
+		"lg	5,  0(%[len])\n"
+		".insn	rre,0xb2ae0000,2,4\n"
+		: : [len] "a" (&len)
+		: "cc", "memory", "0", "1", "2", "3", "4", "5", "6", "7");
+	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+	report_prefix_pop();
+
+	report_prefix_push("len bits");
+	fail = false;
+	for (i = 12; i < 63; i++) {
+		len = BIT(i);
+		if (len < mlen)
+			continue;
+		expect_pgm_int();
+		asm volatile (
+			"lg	5,  0(%[len])\n"
+			".insn	rre,0xb2ae0000,2,4\n"
+			: : [len] "a" (&len)
+			: "cc", "memory", "0", "1", "2", "3", "4", "5", "6", "7");
+		if (clear_pgm_int() != PGM_INT_CODE_SPECIFICATION) {
+			report_fail("setting len to %lx did not result in a spec exception",
+				    len);
+			fail = true;
+		}
+	}
+	report(!fail, "length pgms");
+	report_prefix_pop();
+	report_prefix_pop();
+	report_prefix_pop();
+
+	report_prefix_push("dqap");
+	report_prefix_push("spec");
+
+	report_prefix_push("len + 1");
+	expect_pgm_int();
+	len = mlen + 1;
+	asm volatile (
+		"lg	5,  0(%[len])\n"
+		".insn	rre,0xb2ae0000,2,4\n"
+		: : [len] "a" (&len)
+		: "cc", "memory", "0", "1", "2", "3", "4", "5", "6", "7");
+	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+	report_prefix_pop();
+
+	report_prefix_push("len bits");
+	fail = false;
+	for (i = 12; i < 63; i++) {
+		len = BIT(i);
+		if (len < mlen)
+			continue;
+		expect_pgm_int();
+		asm volatile (
+			"lg	5,  0(%[len])\n"
+			".insn	rre,0xb2ae0000,2,4\n"
+			: : [len] "a" (&len)
+			: "cc", "memory", "0", "1", "2", "3", "4", "5", "6", "7");
+		if (clear_pgm_int() != PGM_INT_CODE_SPECIFICATION) {
+			report_fail("setting len to %lx did not result in a spec exception",
+				    len);
+			fail = true;
+		}
+	}
+	report(!fail, "length pgms");
+	report_prefix_pop();
+	report_prefix_pop();
+	report_prefix_pop();
+}
+
 static void test_priv(void)
 {
 	struct ap_config_info info = {};
@@ -446,6 +546,9 @@ int main(void)
 	}
 	apn = apns[0];
 	qn = qns[0];
+
+	test_pgms_nqdq_len();
+
 	report_prefix_push("pqap");
 	if (test_facility(65)) {
 		test_pqap_aqic();
-- 
2.40.1

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

[Anthony Krowiak]

I made a few comments and suggestions. I am not very well-versed in the 
inline assembly code, so I'll leave that up to someone who is more 
knowledgeable. I copied @Harald since I believe it was him who wrote it.

On 2/2/24 9:59 AM, Janosch Frank wrote:
> 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 | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   lib/s390x/ap.h | 88 +++++++++++++++++++++++++++++++++++++++++++++
>   s390x/Makefile |  1 +
>   3 files changed, 186 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..9560ff64
> --- /dev/null
> +++ b/lib/s390x/ap.c
> @@ -0,0 +1,97 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * AP crypto functions
> + *
> + * Some parts taken from the Linux AP driver.
> + *
> + * Copyright IBM Corp. 2024
> + * 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>
> +#include <asm/facility.h>
> +
> +int ap_pqap_tapq(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw,
> +		 struct pqap_r2 *r2)
> +{
> +	struct pqap_r0 r0 = {
> +		.ap = ap,
> +		.qn = qn,
> +		.fc = PQAP_TEST_APQ
> +	};
> +	uint64_t bogus_cc = 2;
> +	int cc;
> +
> +	/*
> +	 * Test AP Queue
> +	 *
> +	 * Writes AP configuration information to the memory pointed
> +	 * at by GR2.
> +	 *
> +	 * Inputs: GR0
> +	 * Outputs: GR1 (APQSW), GR2 (tapq data)
> +	 * Synchronous
> +	 */
> +	asm volatile(
> +		"       tmll	%[bogus_cc],3\n"
> +		"	lgr	0,%[r0]\n"
> +		"	.insn	rre,0xb2af0000,0,0\n" /* PQAP */
> +		"	stg	1,%[apqsw]\n"
> +		"	stg	2,%[r2]\n"
> +		"	ipm	%[cc]\n"
> +		"	srl	%[cc],28\n"
> +		: [apqsw] "=&T" (*apqsw), [r2] "=&T" (*r2), [cc] "=&d" (cc)
> +		: [r0] "d" (r0), [bogus_cc] "d" (bogus_cc));
> +
> +	return cc;
> +}
> +
> +int ap_pqap_qci(struct ap_config_info *info)
> +{
> +	struct pqap_r0 r0 = { .fc = PQAP_QUERY_AP_CONF_INFO };
> +	uint64_t bogus_cc = 2;
> +	int cc;
> +
> +	/*
> +	 * Query AP Configuration Information
> +	 *
> +	 * Writes AP configuration information to the memory pointed
> +	 * at by GR2.
> +	 *
> +	 * Inputs: GR0, GR2 (QCI block address)
> +	 * Outputs: memory at GR2 address
> +	 * Synchronous
> +	 */
> +	asm volatile(
> +		"       tmll	%[bogus_cc],3\n"
> +		"	lgr	0,%[r0]\n"
> +		"	lgr	2,%[info]\n"
> +		"	.insn	rre,0xb2af0000,0,0\n" /* PQAP */
> +		"	ipm	%[cc]\n"
> +		"	srl	%[cc],28\n"
> +		: [cc] "=&d" (cc)
> +		: [r0] "d" (r0), [info] "d" (info), [bogus_cc] "d" (bogus_cc)
> +		: "cc", "memory", "0", "2");
> +
> +	return cc;
> +}
> +
> +/* Will later be extended to a proper setup function */
> +bool ap_setup(void)
> +{
> +	/*
> +	 * Base AP support has no STFLE or SCLP feature bit but the
> +	 * PQAP QCI support is indicated via stfle bit 12. As this
> +	 * library relies on QCI we bail out if it's not available.
> +	 */
> +	if (!test_facility(12))
> +		return false;


The STFLE.12 can be turned off when starting the guest, so this may not 
be a valid test.

We use the ap_instructions_available function (in ap.h) which executes 
the TAPQ command to verify whether the AP instructions are installed or 
not. Maybe you can do something similar here:


static inline bool ap_instructions_available(void)

{
     unsigned long reg0 = 0x0000;
     unsigned long reg1 = 0;

     asm volatile(
         "    lgr    0,%[reg0]\n"        /* qid into gr0 */
         "    lghi    1,0\n"            /* 0 into gr1 */
         "    lghi    2,0\n"            /* 0 into gr2 */
         "    .insn    rre,0xb2af0000,0,0\n"    /* PQAP(TAPQ) */
         "0:    la    %[reg1],1\n"        /* 1 into reg1 */
         "1:\n"
         EX_TABLE(0b, 1b)
         : [reg1] "+&d" (reg1)
         : [reg0] "d" (reg0)
         : "cc", "0", "1", "2");
     return reg1 != 0;
}
> +
> +	return true;
> +}
> diff --git a/lib/s390x/ap.h b/lib/s390x/ap.h
> new file mode 100644
> index 00000000..b806513f
> --- /dev/null
> +++ b/lib/s390x/ap.h
> @@ -0,0 +1,88 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * AP definitions
> + *
> + * Some parts taken from the Linux AP driver.
> + *
> + * Copyright IBM Corp. 2024
> + * 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,


Maybe use abbreviations like your function names above?

	PQAP_TAPQ,
	PQAP_RAPQ,
	PQAP_ZAPQ,
	PQAP_AQIC,
	PQAP_QCI,
	PQAP_QACT,
	PQAP_QBAP


> +};
> +
> +struct ap_queue_status {
> +	uint32_t pad0;			/* Ignored padding for zArch  */


Bit 0 is the E (queue empty) bit.


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


This is the APID part of an APQN, so how about renaming to 'apid'


> +	uint8_t qn;


This is the APQI  part of an APQN, so how about renaming to 'apqi'


> +} __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];


Is there a reason why the 'Classification'  field is left out?


> +	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_setup(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
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 7fce9f9d..4f6c627d 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -110,6 +110,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
>   

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

[Harald Freudenberger]

On 2024-02-05 19:15, Anthony Krowiak wrote:
> I made a few comments and suggestions. I am not very well-versed in
> the inline assembly code, so I'll leave that up to someone who is more
> knowledgeable. I copied @Harald since I believe it was him who wrote
> it.
> 
> On 2/2/24 9:59 AM, Janosch Frank wrote:
>> 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 | 97 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>   lib/s390x/ap.h | 88 +++++++++++++++++++++++++++++++++++++++++++++
>>   s390x/Makefile |  1 +
>>   3 files changed, 186 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..9560ff64
>> --- /dev/null
>> +++ b/lib/s390x/ap.c
>> @@ -0,0 +1,97 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * AP crypto functions
>> + *
>> + * Some parts taken from the Linux AP driver.
>> + *
>> + * Copyright IBM Corp. 2024
>> + * 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>
>> +#include <asm/facility.h>
>> +
>> +int ap_pqap_tapq(uint8_t ap, uint8_t qn, struct ap_queue_status 
>> *apqsw,
>> +		 struct pqap_r2 *r2)
>> +{
>> +	struct pqap_r0 r0 = {
>> +		.ap = ap,
>> +		.qn = qn,
>> +		.fc = PQAP_TEST_APQ
>> +	};
>> +	uint64_t bogus_cc = 2;
>> +	int cc;
>> +
>> +	/*
>> +	 * Test AP Queue
>> +	 *
>> +	 * Writes AP configuration information to the memory pointed
>> +	 * at by GR2.
>> +	 *
>> +	 * Inputs: GR0
>> +	 * Outputs: GR1 (APQSW), GR2 (tapq data)
>> +	 * Synchronous
>> +	 */
>> +	asm volatile(
>> +		"       tmll	%[bogus_cc],3\n"
>> +		"	lgr	0,%[r0]\n"
>> +		"	.insn	rre,0xb2af0000,0,0\n" /* PQAP */
>> +		"	stg	1,%[apqsw]\n"
>> +		"	stg	2,%[r2]\n"
>> +		"	ipm	%[cc]\n"
>> +		"	srl	%[cc],28\n"
>> +		: [apqsw] "=&T" (*apqsw), [r2] "=&T" (*r2), [cc] "=&d" (cc)
>> +		: [r0] "d" (r0), [bogus_cc] "d" (bogus_cc));
>> +
>> +	return cc;
>> +}
>> +
>> +int ap_pqap_qci(struct ap_config_info *info)
>> +{
>> +	struct pqap_r0 r0 = { .fc = PQAP_QUERY_AP_CONF_INFO };
>> +	uint64_t bogus_cc = 2;
>> +	int cc;
>> +
>> +	/*
>> +	 * Query AP Configuration Information
>> +	 *
>> +	 * Writes AP configuration information to the memory pointed
>> +	 * at by GR2.
>> +	 *
>> +	 * Inputs: GR0, GR2 (QCI block address)
>> +	 * Outputs: memory at GR2 address
>> +	 * Synchronous
>> +	 */
>> +	asm volatile(
>> +		"       tmll	%[bogus_cc],3\n"
>> +		"	lgr	0,%[r0]\n"
>> +		"	lgr	2,%[info]\n"
>> +		"	.insn	rre,0xb2af0000,0,0\n" /* PQAP */
>> +		"	ipm	%[cc]\n"
>> +		"	srl	%[cc],28\n"
>> +		: [cc] "=&d" (cc)
>> +		: [r0] "d" (r0), [info] "d" (info), [bogus_cc] "d" (bogus_cc)
>> +		: "cc", "memory", "0", "2");
>> +
>> +	return cc;
>> +}
>> +
>> +/* Will later be extended to a proper setup function */
>> +bool ap_setup(void)
>> +{
>> +	/*
>> +	 * Base AP support has no STFLE or SCLP feature bit but the
>> +	 * PQAP QCI support is indicated via stfle bit 12. As this
>> +	 * library relies on QCI we bail out if it's not available.
>> +	 */
>> +	if (!test_facility(12))
>> +		return false;
> 
> 
> The STFLE.12 can be turned off when starting the guest, so this may
> not be a valid test.
> 
> We use the ap_instructions_available function (in ap.h) which executes
> the TAPQ command to verify whether the AP instructions are installed
> or not. Maybe you can do something similar here:
> 
> 
> static inline bool ap_instructions_available(void)
> 
> {
>     unsigned long reg0 = 0x0000;
>     unsigned long reg1 = 0;
> 
>     asm volatile(
>         "    lgr    0,%[reg0]\n"        /* qid into gr0 */
>         "    lghi    1,0\n"            /* 0 into gr1 */
>         "    lghi    2,0\n"            /* 0 into gr2 */
>         "    .insn    rre,0xb2af0000,0,0\n"    /* PQAP(TAPQ) */
>         "0:    la    %[reg1],1\n"        /* 1 into reg1 */
>         "1:\n"
>         EX_TABLE(0b, 1b)
>         : [reg1] "+&d" (reg1)
>         : [reg0] "d" (reg0)
>         : "cc", "0", "1", "2");
>     return reg1 != 0;
> }

Well, as Janos wrote - this lib functions rely on having QCI available.
However, be carefully using the above function as it is setting up
an exception table entry which is to catch the illegal instruction
which will arise when no AP bus support is available.

>> +
>> +	return true;
>> +}
>> diff --git a/lib/s390x/ap.h b/lib/s390x/ap.h
>> new file mode 100644
>> index 00000000..b806513f
>> --- /dev/null
>> +++ b/lib/s390x/ap.h
>> @@ -0,0 +1,88 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * AP definitions
>> + *
>> + * Some parts taken from the Linux AP driver.
>> + *
>> + * Copyright IBM Corp. 2024
>> + * 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,

What is that       ^^^^^^^^^^^^^ ?
The QACT ? But that's only one PQAP subfuntion.

>> +	PQAP_BEST_AP,

And this ^^^^^^^^^^^^ ? Never heard about.

> 
> 
> Maybe use abbreviations like your function names above?
> 
> 	PQAP_TAPQ,
> 	PQAP_RAPQ,
> 	PQAP_ZAPQ,
> 	PQAP_AQIC,
> 	PQAP_QCI,
> 	PQAP_QACT,
> 	PQAP_QBAP
> 
> 
>> +};
>> +
>> +struct ap_queue_status {
>> +	uint32_t pad0;			/* Ignored padding for zArch  */
> 
> 
> Bit 0 is the E (queue empty) bit.
> 
> 
>> +	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;
> 
> 
> This is the APID part of an APQN, so how about renaming to 'apid'
> 
> 
>> +	uint8_t qn;
> 
> 
> This is the APQI  part of an APQN, so how about renaming to 'apqi'
> 
> 
>> +} __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];
> 
> 
> Is there a reason why the 'Classification'  field is left out?
> 
> 
>> +	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");
>> +

Isn't this all going into the kernel? So consider using the
arch/s390/include/asm/ap.h header file instead of re-defining the 
structs.
Feel free to improve this file with reworked structs and field names if
that's required.

>> +bool ap_setup(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
>> diff --git a/s390x/Makefile b/s390x/Makefile
>> index 7fce9f9d..4f6c627d 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -110,6 +110,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
>> 

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

[Janosch Frank]

On 2/5/24 19:15, Anthony Krowiak wrote:
> I made a few comments and suggestions. I am not very well-versed in the
> inline assembly code, so I'll leave that up to someone who is more
> knowledgeable. I copied @Harald since I believe it was him who wrote it.
> 
> On 2/2/24 9:59 AM, Janosch Frank wrote:
>> Add functions and definitions needed to test the Adjunct
>> Processor (AP) crypto interface.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

[...]

>> +/* Will later be extended to a proper setup function */
>> +bool ap_setup(void)
>> +{
>> +	/*
>> +	 * Base AP support has no STFLE or SCLP feature bit but the
>> +	 * PQAP QCI support is indicated via stfle bit 12. As this
>> +	 * library relies on QCI we bail out if it's not available.
>> +	 */
>> +	if (!test_facility(12))
>> +		return false;
> 
> 
> The STFLE.12 can be turned off when starting the guest, so this may not
> be a valid test.
> 
> We use the ap_instructions_available function (in ap.h) which executes
> the TAPQ command to verify whether the AP instructions are installed or
> not. Maybe you can do something similar here:

This library relies on QCI, hence we only check for stfle.
I see no sense in manually probing the whole APQN space.


If stfle 12 is indicated I'd expect AP instructions to not generate 
exceptions or do they in a sane CPU model?


>> +
>> +	return true;
>> +}
>> diff --git a/lib/s390x/ap.h b/lib/s390x/ap.h
>> new file mode 100644
>> index 00000000..b806513f
>> --- /dev/null
>> +++ b/lib/s390x/ap.h
>> @@ -0,0 +1,88 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * AP definitions
>> + *
>> + * Some parts taken from the Linux AP driver.
>> + *
>> + * Copyright IBM Corp. 2024
>> + * 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,
> 
> 
> Maybe use abbreviations like your function names above?
> 
> 	PQAP_TAPQ,
> 	PQAP_RAPQ,
> 	PQAP_ZAPQ,
> 	PQAP_AQIC,
> 	PQAP_QCI,
> 	PQAP_QACT,
> 	PQAP_QBAP
> 

Hmmmmmmm(TM)
My guess is that I tried making these constants readable without 
consulting architecture documents. But another option is using the 
constants that you suggested and adding comments with a long version.

Will do

[...]

>> +struct pqap_r0 {
>> +	uint32_t pad0;
>> +	uint8_t fc;
>> +	uint8_t t : 1;		/* Test facilities (TAPQ)*/
>> +	uint8_t pad1 : 7;
>> +	uint8_t ap;
> 
> 
> This is the APID part of an APQN, so how about renaming to 'apid'
> 
> 
>> +	uint8_t qn;
> 
> 
> This is the APQI  part of an APQN, so how about renaming to 'apqi'

Hmm Linux uses qid
I'll change it to the Linux naming convention, might take me a while though

> 
> 
>> +} __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];
> 
> 
> Is there a reason why the 'Classification'  field is left out?
> 

It's not used in this library and therefore I chose to not name it to 
make structs a bit more readable.

> 
>> +	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_setup(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
>> diff --git a/s390x/Makefile b/s390x/Makefile
>> index 7fce9f9d..4f6c627d 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -110,6 +110,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
>>    

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

[Anthony Krowiak]

On 2/6/24 3:48 AM, Harald Freudenberger wrote:
> On 2024-02-05 19:15, Anthony Krowiak wrote:
>> I made a few comments and suggestions. I am not very well-versed in
>> the inline assembly code, so I'll leave that up to someone who is more
>> knowledgeable. I copied @Harald since I believe it was him who wrote
>> it.
>>
>> On 2/2/24 9:59 AM, Janosch Frank wrote:
>>> 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 | 97 
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   lib/s390x/ap.h | 88 +++++++++++++++++++++++++++++++++++++++++++++
>>>   s390x/Makefile |  1 +
>>>   3 files changed, 186 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..9560ff64
>>> --- /dev/null
>>> +++ b/lib/s390x/ap.c
>>> @@ -0,0 +1,97 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * AP crypto functions
>>> + *
>>> + * Some parts taken from the Linux AP driver.
>>> + *
>>> + * Copyright IBM Corp. 2024
>>> + * 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>
>>> +#include <asm/facility.h>
>>> +
>>> +int ap_pqap_tapq(uint8_t ap, uint8_t qn, struct ap_queue_status 
>>> *apqsw,
>>> +         struct pqap_r2 *r2)
>>> +{
>>> +    struct pqap_r0 r0 = {
>>> +        .ap = ap,
>>> +        .qn = qn,
>>> +        .fc = PQAP_TEST_APQ
>>> +    };
>>> +    uint64_t bogus_cc = 2;
>>> +    int cc;
>>> +
>>> +    /*
>>> +     * Test AP Queue
>>> +     *
>>> +     * Writes AP configuration information to the memory pointed
>>> +     * at by GR2.
>>> +     *
>>> +     * Inputs: GR0
>>> +     * Outputs: GR1 (APQSW), GR2 (tapq data)
>>> +     * Synchronous
>>> +     */
>>> +    asm volatile(
>>> +        "       tmll    %[bogus_cc],3\n"
>>> +        "    lgr    0,%[r0]\n"
>>> +        "    .insn    rre,0xb2af0000,0,0\n" /* PQAP */
>>> +        "    stg    1,%[apqsw]\n"
>>> +        "    stg    2,%[r2]\n"
>>> +        "    ipm    %[cc]\n"
>>> +        "    srl    %[cc],28\n"
>>> +        : [apqsw] "=&T" (*apqsw), [r2] "=&T" (*r2), [cc] "=&d" (cc)
>>> +        : [r0] "d" (r0), [bogus_cc] "d" (bogus_cc));
>>> +
>>> +    return cc;
>>> +}
>>> +
>>> +int ap_pqap_qci(struct ap_config_info *info)
>>> +{
>>> +    struct pqap_r0 r0 = { .fc = PQAP_QUERY_AP_CONF_INFO };
>>> +    uint64_t bogus_cc = 2;
>>> +    int cc;
>>> +
>>> +    /*
>>> +     * Query AP Configuration Information
>>> +     *
>>> +     * Writes AP configuration information to the memory pointed
>>> +     * at by GR2.
>>> +     *
>>> +     * Inputs: GR0, GR2 (QCI block address)
>>> +     * Outputs: memory at GR2 address
>>> +     * Synchronous
>>> +     */
>>> +    asm volatile(
>>> +        "       tmll    %[bogus_cc],3\n"
>>> +        "    lgr    0,%[r0]\n"
>>> +        "    lgr    2,%[info]\n"
>>> +        "    .insn    rre,0xb2af0000,0,0\n" /* PQAP */
>>> +        "    ipm    %[cc]\n"
>>> +        "    srl    %[cc],28\n"
>>> +        : [cc] "=&d" (cc)
>>> +        : [r0] "d" (r0), [info] "d" (info), [bogus_cc] "d" (bogus_cc)
>>> +        : "cc", "memory", "0", "2");
>>> +
>>> +    return cc;
>>> +}
>>> +
>>> +/* Will later be extended to a proper setup function */
>>> +bool ap_setup(void)
>>> +{
>>> +    /*
>>> +     * Base AP support has no STFLE or SCLP feature bit but the
>>> +     * PQAP QCI support is indicated via stfle bit 12. As this
>>> +     * library relies on QCI we bail out if it's not available.
>>> +     */
>>> +    if (!test_facility(12))
>>> +        return false;
>>
>>
>> The STFLE.12 can be turned off when starting the guest, so this may
>> not be a valid test.
>>
>> We use the ap_instructions_available function (in ap.h) which executes
>> the TAPQ command to verify whether the AP instructions are installed
>> or not. Maybe you can do something similar here:
>>
>>
>> static inline bool ap_instructions_available(void)
>>
>> {
>>     unsigned long reg0 = 0x0000;
>>     unsigned long reg1 = 0;
>>
>>     asm volatile(
>>         "    lgr    0,%[reg0]\n"        /* qid into gr0 */
>>         "    lghi    1,0\n"            /* 0 into gr1 */
>>         "    lghi    2,0\n"            /* 0 into gr2 */
>>         "    .insn    rre,0xb2af0000,0,0\n"    /* PQAP(TAPQ) */
>>         "0:    la    %[reg1],1\n"        /* 1 into reg1 */
>>         "1:\n"
>>         EX_TABLE(0b, 1b)
>>         : [reg1] "+&d" (reg1)
>>         : [reg0] "d" (reg0)
>>         : "cc", "0", "1", "2");
>>     return reg1 != 0;
>> }
>
> Well, as Janos wrote - this lib functions rely on having QCI available.
> However, be carefully using the above function as it is setting up
> an exception table entry which is to catch the illegal instruction
> which will arise when no AP bus support is available.
>
>>> +
>>> +    return true;
>>> +}
>>> diff --git a/lib/s390x/ap.h b/lib/s390x/ap.h
>>> new file mode 100644
>>> index 00000000..b806513f
>>> --- /dev/null
>>> +++ b/lib/s390x/ap.h
>>> @@ -0,0 +1,88 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * AP definitions
>>> + *
>>> + * Some parts taken from the Linux AP driver.
>>> + *
>>> + * Copyright IBM Corp. 2024
>>> + * 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,
>
> What is that       ^^^^^^^^^^^^^ ?
> The QACT ? But that's only one PQAP subfuntion.


PQAP(QBAP)?


>
>>> +    PQAP_BEST_AP,
>
> And this ^^^^^^^^^^^^ ? Never heard about.
>
>>
>>
>> Maybe use abbreviations like your function names above?
>>
>>     PQAP_TAPQ,
>>     PQAP_RAPQ,
>>     PQAP_ZAPQ,
>>     PQAP_AQIC,
>>     PQAP_QCI,
>>     PQAP_QACT,
>>     PQAP_QBAP
>>
>>
>>> +};
>>> +
>>> +struct ap_queue_status {
>>> +    uint32_t pad0;            /* Ignored padding for zArch */
>>
>>
>> Bit 0 is the E (queue empty) bit.
>>
>>
>>> +    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;
>>
>>
>> This is the APID part of an APQN, so how about renaming to 'apid'
>>
>>
>>> +    uint8_t qn;
>>
>>
>> This is the APQI  part of an APQN, so how about renaming to 'apqi'
>>
>>
>>> +} __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];
>>
>>
>> Is there a reason why the 'Classification'  field is left out?
>>
>>
>>> +    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");
>>> +
>
> Isn't this all going into the kernel? So consider using the
> arch/s390/include/asm/ap.h header file instead of re-defining the 
> structs.
> Feel free to improve this file with reworked structs and field names if
> that's required.
>
>>> +bool ap_setup(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
>>> diff --git a/s390x/Makefile b/s390x/Makefile
>>> index 7fce9f9d..4f6c627d 100644
>>> --- a/s390x/Makefile
>>> +++ b/s390x/Makefile
>>> @@ -110,6 +110,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
>>>
>

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

[Anthony Krowiak]

On 2/6/24 8:42 AM, Janosch Frank wrote:
> On 2/5/24 19:15, Anthony Krowiak wrote:
>> I made a few comments and suggestions. I am not very well-versed in the
>> inline assembly code, so I'll leave that up to someone who is more
>> knowledgeable. I copied @Harald since I believe it was him who wrote it.
>>
>> On 2/2/24 9:59 AM, Janosch Frank wrote:
>>> Add functions and definitions needed to test the Adjunct
>>> Processor (AP) crypto interface.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>
> [...]
>
>>> +/* Will later be extended to a proper setup function */
>>> +bool ap_setup(void)
>>> +{
>>> +    /*
>>> +     * Base AP support has no STFLE or SCLP feature bit but the
>>> +     * PQAP QCI support is indicated via stfle bit 12. As this
>>> +     * library relies on QCI we bail out if it's not available.
>>> +     */
>>> +    if (!test_facility(12))
>>> +        return false;
>>
>>
>> The STFLE.12 can be turned off when starting the guest, so this may not
>> be a valid test.
>>
>> We use the ap_instructions_available function (in ap.h) which executes
>> the TAPQ command to verify whether the AP instructions are installed or
>> not. Maybe you can do something similar here:
>
> This library relies on QCI, hence we only check for stfle.
> I see no sense in manually probing the whole APQN space.


Makes sense. I was thrown off by the PQAP_FC enumeration which includes 
all of the AP function codes.


>
>
> If stfle 12 is indicated I'd expect AP instructions to not generate 
> exceptions or do they in a sane CPU model?


No, I would not expect PQAP(QCI) to generate an exception if STFLE 12 is 
indicated.


>
>
>>> +
>>> +    return true;
>>> +}
>>> diff --git a/lib/s390x/ap.h b/lib/s390x/ap.h
>>> new file mode 100644
>>> index 00000000..b806513f
>>> --- /dev/null
>>> +++ b/lib/s390x/ap.h
>>> @@ -0,0 +1,88 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * AP definitions
>>> + *
>>> + * Some parts taken from the Linux AP driver.
>>> + *
>>> + * Copyright IBM Corp. 2024
>>> + * 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,
>>
>>
>> Maybe use abbreviations like your function names above?
>>
>>     PQAP_TAPQ,
>>     PQAP_RAPQ,
>>     PQAP_ZAPQ,
>>     PQAP_AQIC,
>>     PQAP_QCI,
>>     PQAP_QACT,
>>     PQAP_QBAP
>>
>
> Hmmmmmmm(TM)
> My guess is that I tried making these constants readable without 
> consulting architecture documents. But another option is using the 
> constants that you suggested and adding comments with a long version.


I think that works out better; you won't have to abbreviate the longer 
version which will make it easier to understand.


>
> Will do
>
> [...]
>
>>> +struct pqap_r0 {
>>> +    uint32_t pad0;
>>> +    uint8_t fc;
>>> +    uint8_t t : 1;        /* Test facilities (TAPQ)*/
>>> +    uint8_t pad1 : 7;
>>> +    uint8_t ap;
>>
>>
>> This is the APID part of an APQN, so how about renaming to 'apid'
>>
>>
>>> +    uint8_t qn;
>>
>>
>> This is the APQI  part of an APQN, so how about renaming to 'apqi'
>
> Hmm Linux uses qid
> I'll change it to the Linux naming convention, might take me a while 
> though


Well, the AP bus uses qid, but the vfio_ap module and the architecture 
doc uses APQN. In any case, it's a nit and I'm not terribly concerned 
about it.


>
>>
>>
>>> +} __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];
>>
>>
>> Is there a reason why the 'Classification'  field is left out?
>>
>
> It's not used in this library and therefore I chose to not name it to 
> make structs a bit more readable.


Okay, not a problem.


>
>>
>>> +    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_setup(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
>>> diff --git a/s390x/Makefile b/s390x/Makefile
>>> index 7fce9f9d..4f6c627d 100644
>>> --- a/s390x/Makefile
>>> +++ b/s390x/Makefile
>>> @@ -110,6 +110,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
>

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

[Harald Freudenberger]

On 2024-02-06 16:55, Anthony Krowiak wrote:
> On 2/6/24 8:42 AM, Janosch Frank wrote:
>> On 2/5/24 19:15, Anthony Krowiak wrote:
>>> I made a few comments and suggestions. I am not very well-versed in 
>>> the
>>> inline assembly code, so I'll leave that up to someone who is more
>>> knowledgeable. I copied @Harald since I believe it was him who wrote 
>>> it.
>>> 
>>> On 2/2/24 9:59 AM, Janosch Frank wrote:
>>>> Add functions and definitions needed to test the Adjunct
>>>> Processor (AP) crypto interface.
>>>> 
>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> 
>> [...]
>> 
>>>> +/* Will later be extended to a proper setup function */
>>>> +bool ap_setup(void)
>>>> +{
>>>> +    /*
>>>> +     * Base AP support has no STFLE or SCLP feature bit but the
>>>> +     * PQAP QCI support is indicated via stfle bit 12. As this
>>>> +     * library relies on QCI we bail out if it's not available.
>>>> +     */
>>>> +    if (!test_facility(12))
>>>> +        return false;
>>> 
>>> 
>>> The STFLE.12 can be turned off when starting the guest, so this may 
>>> not
>>> be a valid test.
>>> 
>>> We use the ap_instructions_available function (in ap.h) which 
>>> executes
>>> the TAPQ command to verify whether the AP instructions are installed 
>>> or
>>> not. Maybe you can do something similar here:
>> 
>> This library relies on QCI, hence we only check for stfle.
>> I see no sense in manually probing the whole APQN space.
> 
> 
> Makes sense. I was thrown off by the PQAP_FC enumeration which
> includes all of the AP function codes.
> 
> 
>> 
>> 
>> If stfle 12 is indicated I'd expect AP instructions to not generate 
>> exceptions or do they in a sane CPU model?
> 
> 
> No, I would not expect PQAP(QCI) to generate an exception if STFLE 12
> is indicated.
> 

Hm, I am not sure if you can rely just on checking stfle bit 12 and if 
that's available assume
you have AP instructions. I never tried this. But as far as I know the 
KVM guys there is a chance
that you see a stfle bit 12 but get an illegal instruction exception the 
moment you call
an AP instruction... Maybe check this before relying on such a thing.

>> 
>> 
>>>> +
>>>> +    return true;
>>>> +}
>>>> diff --git a/lib/s390x/ap.h b/lib/s390x/ap.h
>>>> new file mode 100644
>>>> index 00000000..b806513f
>>>> --- /dev/null
>>>> +++ b/lib/s390x/ap.h
>>>> @@ -0,0 +1,88 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>>> +/*
>>>> + * AP definitions
>>>> + *
>>>> + * Some parts taken from the Linux AP driver.
>>>> + *
>>>> + * Copyright IBM Corp. 2024
>>>> + * 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,
>>> 
>>> 
>>> Maybe use abbreviations like your function names above?
>>> 
>>>     PQAP_TAPQ,
>>>     PQAP_RAPQ,
>>>     PQAP_ZAPQ,
>>>     PQAP_AQIC,
>>>     PQAP_QCI,
>>>     PQAP_QACT,
>>>     PQAP_QBAP
>>> 
>> 
>> Hmmmmmmm(TM)
>> My guess is that I tried making these constants readable without 
>> consulting architecture documents. But another option is using the 
>> constants that you suggested and adding comments with a long version.
> 
> 
> I think that works out better; you won't have to abbreviate the longer
> version which will make it easier to understand.
> 
> 
>> 
>> Will do
>> 
>> [...]
>> 
>>>> +struct pqap_r0 {
>>>> +    uint32_t pad0;
>>>> +    uint8_t fc;
>>>> +    uint8_t t : 1;        /* Test facilities (TAPQ)*/
>>>> +    uint8_t pad1 : 7;
>>>> +    uint8_t ap;
>>> 
>>> 
>>> This is the APID part of an APQN, so how about renaming to 'apid'
>>> 
>>> 
>>>> +    uint8_t qn;
>>> 
>>> 
>>> This is the APQI  part of an APQN, so how about renaming to 'apqi'
>> 
>> Hmm Linux uses qid
>> I'll change it to the Linux naming convention, might take me a while 
>> though
> 
> 
> Well, the AP bus uses qid, but the vfio_ap module and the architecture
> doc uses APQN. In any case, it's a nit and I'm not terribly concerned
> about it.
> 
> 
>> 
>>> 
>>> 
>>>> +} __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];
>>> 
>>> 
>>> Is there a reason why the 'Classification'  field is left out?
>>> 
>> 
>> It's not used in this library and therefore I chose to not name it to 
>> make structs a bit more readable.
> 
> 
> Okay, not a problem.
> 
> 
>> 
>>> 
>>>> +    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_setup(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
>>>> diff --git a/s390x/Makefile b/s390x/Makefile
>>>> index 7fce9f9d..4f6c627d 100644
>>>> --- a/s390x/Makefile
>>>> +++ b/s390x/Makefile
>>>> @@ -110,6 +110,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
>> 

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

[Anthony Krowiak]

On 2/7/24 3:06 AM, Harald Freudenberger wrote:
> On 2024-02-06 16:55, Anthony Krowiak wrote:
>> On 2/6/24 8:42 AM, Janosch Frank wrote:
>>> On 2/5/24 19:15, Anthony Krowiak wrote:
>>>> I made a few comments and suggestions. I am not very well-versed in 
>>>> the
>>>> inline assembly code, so I'll leave that up to someone who is more
>>>> knowledgeable. I copied @Harald since I believe it was him who 
>>>> wrote it.
>>>>
>>>> On 2/2/24 9:59 AM, Janosch Frank wrote:
>>>>> Add functions and definitions needed to test the Adjunct
>>>>> Processor (AP) crypto interface.
>>>>>
>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>
>>> [...]
>>>
>>>>> +/* Will later be extended to a proper setup function */
>>>>> +bool ap_setup(void)
>>>>> +{
>>>>> +    /*
>>>>> +     * Base AP support has no STFLE or SCLP feature bit but the
>>>>> +     * PQAP QCI support is indicated via stfle bit 12. As this
>>>>> +     * library relies on QCI we bail out if it's not available.
>>>>> +     */
>>>>> +    if (!test_facility(12))
>>>>> +        return false;
>>>>
>>>>
>>>> The STFLE.12 can be turned off when starting the guest, so this may 
>>>> not
>>>> be a valid test.
>>>>
>>>> We use the ap_instructions_available function (in ap.h) which executes
>>>> the TAPQ command to verify whether the AP instructions are 
>>>> installed or
>>>> not. Maybe you can do something similar here:
>>>
>>> This library relies on QCI, hence we only check for stfle.
>>> I see no sense in manually probing the whole APQN space.
>>
>>
>> Makes sense. I was thrown off by the PQAP_FC enumeration which
>> includes all of the AP function codes.
>>
>>
>>>
>>>
>>> If stfle 12 is indicated I'd expect AP instructions to not generate 
>>> exceptions or do they in a sane CPU model?
>>
>>
>> No, I would not expect PQAP(QCI) to generate an exception if STFLE 12
>> is indicated.
>>
>
> Hm, I am not sure if you can rely just on checking stfle bit 12 and if 
> that's available assume
> you have AP instructions. I never tried this. But as far as I know the 
> KVM guys there is a chance
> that you see a stfle bit 12 but get an illegal instruction exception 
> the moment you call
> an AP instruction... Maybe check this before relying on such a thing.


In order to set stfle bit 12 in the CPU model for a guest (apqci=on), 
you first have to set ap=on in the CPU model indicating that you want AP 
installed on the guest; so I don't think you would ever see stfle 12 
without having AP instructions. You can, however, have AP instructions 
without stfle 12 (apqci=off) in the CPU model.


>
>>>
>>>
>>>>> +
>>>>> +    return true;
>>>>> +}
>>>>> diff --git a/lib/s390x/ap.h b/lib/s390x/ap.h
>>>>> new file mode 100644
>>>>> index 00000000..b806513f
>>>>> --- /dev/null
>>>>> +++ b/lib/s390x/ap.h
>>>>> @@ -0,0 +1,88 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>>>> +/*
>>>>> + * AP definitions
>>>>> + *
>>>>> + * Some parts taken from the Linux AP driver.
>>>>> + *
>>>>> + * Copyright IBM Corp. 2024
>>>>> + * 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,
>>>>
>>>>
>>>> Maybe use abbreviations like your function names above?
>>>>
>>>>     PQAP_TAPQ,
>>>>     PQAP_RAPQ,
>>>>     PQAP_ZAPQ,
>>>>     PQAP_AQIC,
>>>>     PQAP_QCI,
>>>>     PQAP_QACT,
>>>>     PQAP_QBAP
>>>>
>>>
>>> Hmmmmmmm(TM)
>>> My guess is that I tried making these constants readable without 
>>> consulting architecture documents. But another option is using the 
>>> constants that you suggested and adding comments with a long version.
>>
>>
>> I think that works out better; you won't have to abbreviate the longer
>> version which will make it easier to understand.
>>
>>
>>>
>>> Will do
>>>
>>> [...]
>>>
>>>>> +struct pqap_r0 {
>>>>> +    uint32_t pad0;
>>>>> +    uint8_t fc;
>>>>> +    uint8_t t : 1;        /* Test facilities (TAPQ)*/
>>>>> +    uint8_t pad1 : 7;
>>>>> +    uint8_t ap;
>>>>
>>>>
>>>> This is the APID part of an APQN, so how about renaming to 'apid'
>>>>
>>>>
>>>>> +    uint8_t qn;
>>>>
>>>>
>>>> This is the APQI  part of an APQN, so how about renaming to 'apqi'
>>>
>>> Hmm Linux uses qid
>>> I'll change it to the Linux naming convention, might take me a while 
>>> though
>>
>>
>> Well, the AP bus uses qid, but the vfio_ap module and the architecture
>> doc uses APQN. In any case, it's a nit and I'm not terribly concerned
>> about it.
>>
>>
>>>
>>>>
>>>>
>>>>> +} __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];
>>>>
>>>>
>>>> Is there a reason why the 'Classification'  field is left out?
>>>>
>>>
>>> It's not used in this library and therefore I chose to not name it 
>>> to make structs a bit more readable.
>>
>>
>> Okay, not a problem.
>>
>>
>>>
>>>>
>>>>> +    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_setup(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
>>>>> diff --git a/s390x/Makefile b/s390x/Makefile
>>>>> index 7fce9f9d..4f6c627d 100644
>>>>> --- a/s390x/Makefile
>>>>> +++ b/s390x/Makefile
>>>>> @@ -110,6 +110,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
>>>
>

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

[Anthony Krowiak]

I made a couple of function name change suggestions, but those are not 
critical:

Acked-by: Anthony Krowiak <akrowiak@linux.ibm.com>

On 2/2/24 9:59 AM, Janosch Frank 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>
> ---
>   s390x/Makefile      |   1 +
>   s390x/ap.c          | 309 ++++++++++++++++++++++++++++++++++++++++++++
>   s390x/unittests.cfg |   3 +
>   3 files changed, 313 insertions(+)
>   create mode 100644 s390x/ap.c
>
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 4f6c627d..6d28a5bf 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -42,6 +42,7 @@ tests += $(TEST_DIR)/exittime.elf
>   tests += $(TEST_DIR)/ex.elf
>   tests += $(TEST_DIR)/topology.elf
>   tests += $(TEST_DIR)/sie-dat.elf
> +tests += $(TEST_DIR)/ap.elf
>   
>   pv-tests += $(TEST_DIR)/pv-diags.elf
>   pv-tests += $(TEST_DIR)/pv-icptcode.elf
> diff --git a/s390x/ap.c b/s390x/ap.c
> new file mode 100644
> index 00000000..b3cee37a
> --- /dev/null
> +++ b/s390x/ap.c
> @@ -0,0 +1,309 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * AP instruction G2 tests
> + *
> + * Copyright (c) 2024 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)


If I saw this function name without having read the patch description, I 
wouldn't have any idea what is being tested.

Maybe test_pqap_pgm_chk?


> +{
> +	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;


Just out of curiosity, why 42? Why not some ridiculous number that will 
never be used for a function code, like 4294967295?


> +	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");
> +	/*
> +	 * GR0 bits 41 - 47 are defined 0 and result in a
> +	 * specification exception if set to 1.
> +	 */
> +	for (i = 0; i < 48 - 41; i++) {
> +		grs[0] = BIT(63 - 47 + i);
> +
> +		expect_pgm_int();
> +		pqap(grs);
> +		pgm = clear_pgm_int();
> +
> +		if (pgm != PGM_INT_CODE_SPECIFICATION) {
> +			report_fail("fail on bit %d", 42 + 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 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 alignments tested");
> +	report_prefix_pop();
> +	report_prefix_pop();
> +
> +	free_page(data);
> +	report_prefix_pop();
> +}
> +
> +static void test_pgms_nqap(void)


Same as above:
test_nqap_pgm_chk


> +{
> +	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", "4", "6", "7");
> +	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", "2", "3", "4", "6", "7");
> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("both");
> +	expect_pgm_int();
> +	asm volatile (
> +		".insn	rre,0xb2ad0000,3,7\n"
> +		: : : "cc", "memory", "0", "1", "2", "3", "4", "6", "7");
> +	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", "6", "7");
> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("gr0_zero_bits");
> +	fail = false;
> +	for (i = 0; i < ARRAY_SIZE(gr0_zeroes_bits); 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", "6", "7");
> +		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 gr0 pgms");
> +	report_prefix_pop();
> +
> +	report_prefix_pop();
> +	report_prefix_pop();
> +}
> +
> +static void test_pgms_dqap(void)


Same as above:
test_dqap_pgm_chk


> +{
> +	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", "4", "6", "7");
> +	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", "2", "3", "4", "6", "7");
> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("both");
> +	expect_pgm_int();
> +	asm volatile (
> +		".insn	rre,0xb2ae0000,3,7\n"
> +		: : : "cc", "memory", "0", "1", "2", "3", "4", "6", "7");
> +	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", "6", "7");
> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("gr0_zero_bits");
> +	fail = false;
> +	for (i = 0; i < ARRAY_SIZE(gr0_zeroes_bits); 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", "6", "7");
> +		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 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 018e4129..578375e4 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -386,3 +386,6 @@ file = sie-dat.elf
>   
>   [pv-attest]
>   file = pv-attest.elf
> +
> +[ap]
> +file = ap.elf

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

[Janosch Frank]

On 2/20/24 17:38, Anthony Krowiak wrote:
> I made a couple of function name change suggestions, but those are not
> critical:
> 
> Acked-by: Anthony Krowiak <akrowiak@linux.ibm.com>
> 
> On 2/2/24 9:59 AM, Janosch Frank 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>
>> ---
>>    s390x/Makefile      |   1 +
>>    s390x/ap.c          | 309 ++++++++++++++++++++++++++++++++++++++++++++
>>    s390x/unittests.cfg |   3 +
>>    3 files changed, 313 insertions(+)
>>    create mode 100644 s390x/ap.c
>>
>> diff --git a/s390x/Makefile b/s390x/Makefile
>> index 4f6c627d..6d28a5bf 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -42,6 +42,7 @@ tests += $(TEST_DIR)/exittime.elf
>>    tests += $(TEST_DIR)/ex.elf
>>    tests += $(TEST_DIR)/topology.elf
>>    tests += $(TEST_DIR)/sie-dat.elf
>> +tests += $(TEST_DIR)/ap.elf
>>    
>>    pv-tests += $(TEST_DIR)/pv-diags.elf
>>    pv-tests += $(TEST_DIR)/pv-icptcode.elf
>> diff --git a/s390x/ap.c b/s390x/ap.c
>> new file mode 100644
>> index 00000000..b3cee37a
>> --- /dev/null
>> +++ b/s390x/ap.c
>> @@ -0,0 +1,309 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * AP instruction G2 tests
>> + *
>> + * Copyright (c) 2024 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)
> 
> 
> If I saw this function name without having read the patch description, I
> wouldn't have any idea what is being tested.
> 
> Maybe test_pqap_pgm_chk?

If I'm not mistaken, then it should be consistent with the pgm checks 
for other instructions but I don't mind renaming it.

> 
> 
>> +{
>> +	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;
> 
> 
> Just out of curiosity, why 42? Why not some ridiculous number that will
> never be used for a function code, like 4294967295?

No particular reason.

Increasing the number would increase the buffer zone of invalid values 
but there's currently only a hand full of values defined anyway.
I might add a couple of 0s to this for the next version and call it a day.