[kvm-unit-tests PATCH 0/3] s390x: More storage key instruction

[kvm-unit-tests PATCH 0/3] s390x: More storage key instruction

[Janis Schoetterl-Glausch]

Add test cases similar to those testing the effect of storage keys on
instructions emulated by KVM, but test instructions emulated by user
space/qemu instead.
Additionally, check the transaction exception identification on
protection exceptions

Based on the previous storage key test series.

Janis Schoetterl-Glausch (3):
  s390x: Fix sclp facility bit numbers
  s390x: Test TEID values in storage key test
  s390x: Test effect of storage keys on some more instructions

 lib/s390x/asm/facility.h |  21 +++
 lib/s390x/sclp.h         |  18 ++-
 lib/s390x/sclp.c         |   2 +
 s390x/skey.c             | 337 ++++++++++++++++++++++++++++++++++++++-
 s390x/unittests.cfg      |   1 +
 5 files changed, 366 insertions(+), 13 deletions(-)


base-commit: 6a7a83ed106211fc0ee530a3a05f171f6a4c4e66
prerequisite-patch-id: fbcb3161ffa816cec4edea484bd9d9b22b11518b
prerequisite-patch-id: f7e97c6d2555ac61a603fa9054ba5ad391aa5dbf
prerequisite-patch-id: 14c928967e08fb48de955ccd37d5698c972e54f9
-- 
2.33.1

[kvm-unit-tests PATCH 1/3] s390x: Fix sclp facility bit numbers

[Janis Schoetterl-Glausch]

sclp_feat_check takes care of adjusting the bit numbering such that they
can be defined as they are in the documentation.

Fixes: 4dd649c8 ("lib: s390x: sclp: Extend feature probing")
Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
 lib/s390x/sclp.h | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
index fead007a..4ce2209f 100644
--- a/lib/s390x/sclp.h
+++ b/lib/s390x/sclp.h
@@ -134,13 +134,15 @@ struct sclp_facilities {
 };
 
 /* bit number within a certain byte */
-#define SCLP_FEAT_85_BIT_GSLS		7
-#define SCLP_FEAT_98_BIT_KSS		0
-#define SCLP_FEAT_116_BIT_64BSCAO	7
-#define SCLP_FEAT_116_BIT_CMMA		6
-#define SCLP_FEAT_116_BIT_ESCA		3
-#define SCLP_FEAT_117_BIT_PFMFI		6
-#define SCLP_FEAT_117_BIT_IBS		5
+#define SCLP_FEAT_80_BIT_SOP		2
+#define SCLP_FEAT_85_BIT_GSLS		0
+#define SCLP_FEAT_85_BIT_ESOP		6
+#define SCLP_FEAT_98_BIT_KSS		7
+#define SCLP_FEAT_116_BIT_64BSCAO	0
+#define SCLP_FEAT_116_BIT_CMMA		1
+#define SCLP_FEAT_116_BIT_ESCA		4
+#define SCLP_FEAT_117_BIT_PFMFI		1
+#define SCLP_FEAT_117_BIT_IBS		2
 
 typedef struct ReadInfo {
 	SCCBHeader h;
-- 
2.33.1

[kvm-unit-tests PATCH 2/3] s390x: Test TEID values in storage key test

[Janis Schoetterl-Glausch]

On a protection exception, test that the Translation-Exception
Identification (TEID) values are correct given the circumstances of the
particular test.
The meaning of the TEID values is dependent on the installed
suppression-on-protection facility.

Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
 lib/s390x/asm/facility.h | 21 ++++++++++++++
 lib/s390x/sclp.h         |  2 ++
 lib/s390x/sclp.c         |  2 ++
 s390x/skey.c             | 60 ++++++++++++++++++++++++++++++++++++----
 4 files changed, 79 insertions(+), 6 deletions(-)

diff --git a/lib/s390x/asm/facility.h b/lib/s390x/asm/facility.h
index ef0fd037..f21bb9d7 100644
--- a/lib/s390x/asm/facility.h
+++ b/lib/s390x/asm/facility.h
@@ -12,6 +12,7 @@
 #include <asm/facility.h>
 #include <asm/arch_def.h>
 #include <bitops.h>
+#include <sclp.h>
 
 #define NB_STFL_DOUBLEWORDS 32
 extern uint64_t stfl_doublewords[];
@@ -44,4 +45,24 @@ static inline void setup_facilities(void)
 		stfle(stfl_doublewords, NB_STFL_DOUBLEWORDS);
 }
 
+enum supp_on_prot_facility {
+	SOP_NONE,
+	SOP_BASIC,
+	SOP_ENHANCED_1,
+	SOP_ENHANCED_2,
+};
+
+static inline enum supp_on_prot_facility get_supp_on_prot_facility(void)
+{
+	if (sclp_facilities.has_esop) {
+		if (test_facility(131)) /* side-effect-access facility */
+			return SOP_ENHANCED_2;
+		else
+			return SOP_ENHANCED_1;
+	}
+	if (sclp_facilities.has_sop)
+		return SOP_BASIC;
+	return SOP_NONE;
+}
+
 #endif
diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
index 4ce2209f..f57896b2 100644
--- a/lib/s390x/sclp.h
+++ b/lib/s390x/sclp.h
@@ -123,7 +123,9 @@ struct sclp_facilities {
 	uint64_t has_cei : 1;
 
 	uint64_t has_diag318 : 1;
+	uint64_t has_sop : 1;
 	uint64_t has_gsls : 1;
+	uint64_t has_esop : 1;
 	uint64_t has_cmma : 1;
 	uint64_t has_64bscao : 1;
 	uint64_t has_esca : 1;
diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
index b8204c5f..e6017f64 100644
--- a/lib/s390x/sclp.c
+++ b/lib/s390x/sclp.c
@@ -152,7 +152,9 @@ void sclp_facilities_setup(void)
 	cpu = sclp_get_cpu_entries();
 	if (read_info->offset_cpu > 134)
 		sclp_facilities.has_diag318 = read_info->byte_134_diag318;
+	sclp_facilities.has_sop = sclp_feat_check(80, SCLP_FEAT_80_BIT_SOP);
 	sclp_facilities.has_gsls = sclp_feat_check(85, SCLP_FEAT_85_BIT_GSLS);
+	sclp_facilities.has_esop = sclp_feat_check(85, SCLP_FEAT_85_BIT_ESOP);
 	sclp_facilities.has_kss = sclp_feat_check(98, SCLP_FEAT_98_BIT_KSS);
 	sclp_facilities.has_cmma = sclp_feat_check(116, SCLP_FEAT_116_BIT_CMMA);
 	sclp_facilities.has_64bscao = sclp_feat_check(116, SCLP_FEAT_116_BIT_64BSCAO);
diff --git a/s390x/skey.c b/s390x/skey.c
index 32bf1070..56bf5f45 100644
--- a/s390x/skey.c
+++ b/s390x/skey.c
@@ -8,6 +8,7 @@
  *  Janosch Frank <frankja@linux.vnet.ibm.com>
  */
 #include <libcflat.h>
+#include <bitops.h>
 #include <asm/asm-offsets.h>
 #include <asm/interrupt.h>
 #include <vmalloc.h>
@@ -158,6 +159,53 @@ static void test_test_protection(void)
 	report_prefix_pop();
 }
 
+enum access {
+	ACC_FETCH = 2,
+	ACC_STORE = 1,
+	ACC_UPDATE = 3,
+};
+
+enum protection {
+	PROT_STORE = 1,
+	PROT_FETCH_STORE = 3,
+};
+
+static void check_key_prot_exc(enum access access, enum protection prot)
+{
+	struct lowcore *lc = 0;
+	union teid teid;
+
+	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
+	report_prefix_push("TEID");
+	teid.val = lc->trans_exc_id;
+	switch (get_supp_on_prot_facility()) {
+	case SOP_NONE:
+	case SOP_BASIC:
+		break;
+	case SOP_ENHANCED_1:
+		if ((teid.val & (BIT(63 - 61))) == 0)
+			report_pass("key-controlled protection");
+		break;
+	case SOP_ENHANCED_2:
+		if ((teid.val & (BIT(63 - 56) | BIT(63 - 61))) == 0) {
+			report_pass("key-controlled protection");
+			if (teid.val & BIT(63 - 60)) {
+				int access_code = teid.fetch << 1 | teid.store;
+
+				report_info("access code: %d", access_code);
+				if (access_code == 2)
+					report((access & 2) && (prot & 2),
+					       "exception due to fetch");
+				if (access_code == 1)
+					report((access & 1) && (prot & 1),
+					       "exception due to store");
+			}
+		}
+		break;
+	}
+	report_prefix_pop();
+}
+
 /*
  * Perform STORE CPU ADDRESS (STAP) instruction while temporarily executing
  * with access key 1.
@@ -199,7 +247,7 @@ static void test_store_cpu_address(void)
 	expect_pgm_int();
 	*out = 0xbeef;
 	store_cpu_address_key_1(out);
-	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
+	check_key_prot_exc(ACC_STORE, PROT_STORE);
 	report(*out == 0xbeef, "no store occurred");
 	report_prefix_pop();
 
@@ -210,7 +258,7 @@ static void test_store_cpu_address(void)
 	expect_pgm_int();
 	*out = 0xbeef;
 	store_cpu_address_key_1(out);
-	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
+	check_key_prot_exc(ACC_STORE, PROT_STORE);
 	report(*out == 0xbeef, "no store occurred");
 	report_prefix_pop();
 
@@ -228,7 +276,7 @@ static void test_store_cpu_address(void)
 	expect_pgm_int();
 	*out = 0xbeef;
 	store_cpu_address_key_1(out);
-	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
+	check_key_prot_exc(ACC_STORE, PROT_STORE);
 	report(*out == 0xbeef, "no store occurred");
 	report_prefix_pop();
 
@@ -314,7 +362,7 @@ static void test_set_prefix(void)
 	set_storage_key(pagebuf, 0x28, 0);
 	expect_pgm_int();
 	set_prefix_key_1(prefix_ptr);
-	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
+	check_key_prot_exc(ACC_FETCH, PROT_FETCH_STORE);
 	report(get_prefix() == old_prefix, "did not set prefix");
 	report_prefix_pop();
 
@@ -327,7 +375,7 @@ static void test_set_prefix(void)
 	install_page(root, virt_to_pte_phys(root, pagebuf), 0);
 	set_prefix_key_1((uint32_t *)0);
 	install_page(root, 0, 0);
-	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
+	check_key_prot_exc(ACC_FETCH, PROT_FETCH_STORE);
 	report(get_prefix() == old_prefix, "did not set prefix");
 	report_prefix_pop();
 
@@ -351,7 +399,7 @@ static void test_set_prefix(void)
 	install_page(root, virt_to_pte_phys(root, pagebuf), 0);
 	set_prefix_key_1((uint32_t *)2048);
 	install_page(root, 0, 0);
-	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
+	check_key_prot_exc(ACC_FETCH, PROT_FETCH_STORE);
 	report(get_prefix() == old_prefix, "did not set prefix");
 	report_prefix_pop();
 
-- 
2.33.1

[kvm-unit-tests PATCH 3/3] s390x: Test effect of storage keys on some more instructions

[Janis Schoetterl-Glausch]

Test correctness of some instructions handled by user space instead of
KVM with regards to storage keys.
Test success and error conditions, including coverage of storage and
fetch protection override.

Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
 s390x/skey.c        | 277 ++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |   1 +
 2 files changed, 278 insertions(+)

diff --git a/s390x/skey.c b/s390x/skey.c
index 56bf5f45..d50470a8 100644
--- a/s390x/skey.c
+++ b/s390x/skey.c
@@ -12,6 +12,7 @@
 #include <asm/asm-offsets.h>
 #include <asm/interrupt.h>
 #include <vmalloc.h>
+#include <css.h>
 #include <asm/page.h>
 #include <asm/facility.h>
 #include <asm/mem.h>
@@ -284,6 +285,114 @@ static void test_store_cpu_address(void)
 	report_prefix_pop();
 }
 
+/*
+ * Perform CHANNEL SUBSYSTEM CALL (CHSC)  instruction while temporarily executing
+ * with access key 1.
+ */
+static unsigned int channel_subsystem_call_key_1(void *communication_block)
+{
+	uint32_t program_mask;
+
+	asm volatile (
+		"spka	0x10\n\t"
+		".insn	rre,0xb25f0000,%[communication_block],0\n\t"
+		"spka	0\n\t"
+		"ipm	%[program_mask]\n"
+		: [program_mask] "=d" (program_mask)
+		: [communication_block] "d" (communication_block)
+		: "memory"
+	);
+	return program_mask >> 28;
+}
+
+static void init_store_channel_subsystem_characteristics(uint16_t *communication_block)
+{
+	memset(communication_block, 0, PAGE_SIZE);
+	communication_block[0] = 0x10;
+	communication_block[1] = 0x10;
+	communication_block[9] = 0;
+}
+
+static void test_channel_subsystem_call(void)
+{
+	static const char request_name[] = "Store channel-subsystem-characteristics";
+	uint16_t *communication_block = (uint16_t *)&pagebuf;
+	unsigned int cc;
+
+	report_prefix_push("CHANNEL SUBSYSTEM CALL");
+
+	report_prefix_push("zero key");
+	init_store_channel_subsystem_characteristics(communication_block);
+	set_storage_key(communication_block, 0x10, 0);
+	asm volatile (
+		".insn	rre,0xb25f0000,%[communication_block],0\n\t"
+		"ipm	%[cc]\n"
+		: [cc] "=d" (cc)
+		: [communication_block] "d" (communication_block)
+		: "memory"
+	);
+	cc = cc >> 28;
+	report(cc == 0 && communication_block[9], request_name);
+	report_prefix_pop();
+
+	report_prefix_push("matching key");
+	init_store_channel_subsystem_characteristics(communication_block);
+	set_storage_key(communication_block, 0x10, 0);
+	cc = channel_subsystem_call_key_1(communication_block);
+	report(cc == 0 && communication_block[9], request_name);
+	report_prefix_pop();
+
+	report_prefix_push("mismatching key");
+
+	report_prefix_push("no fetch protection");
+	init_store_channel_subsystem_characteristics(communication_block);
+	set_storage_key(communication_block, 0x20, 0);
+	expect_pgm_int();
+	channel_subsystem_call_key_1(communication_block);
+	check_key_prot_exc(ACC_UPDATE, PROT_STORE);
+	report_prefix_pop();
+
+	report_prefix_push("fetch protection");
+	init_store_channel_subsystem_characteristics(communication_block);
+	set_storage_key(communication_block, 0x28, 0);
+	expect_pgm_int();
+	channel_subsystem_call_key_1(communication_block);
+	check_key_prot_exc(ACC_UPDATE, PROT_FETCH_STORE);
+	report_prefix_pop();
+
+	ctl_set_bit(0, CTL0_STORAGE_PROTECTION_OVERRIDE);
+
+	report_prefix_push("storage-protection override, invalid key");
+	set_storage_key(communication_block, 0x20, 0);
+	init_store_channel_subsystem_characteristics(communication_block);
+	expect_pgm_int();
+	channel_subsystem_call_key_1(communication_block);
+	check_key_prot_exc(ACC_UPDATE, PROT_STORE);
+	report_prefix_pop();
+
+	report_prefix_push("storage-protection override, override key");
+	init_store_channel_subsystem_characteristics(communication_block);
+	set_storage_key(communication_block, 0x90, 0);
+	cc = channel_subsystem_call_key_1(communication_block);
+	report(cc == 0 && communication_block[9], request_name);
+	report_prefix_pop();
+
+	ctl_clear_bit(0, CTL0_STORAGE_PROTECTION_OVERRIDE);
+
+	report_prefix_push("storage-protection override disabled, override key");
+	init_store_channel_subsystem_characteristics(communication_block);
+	set_storage_key(communication_block, 0x90, 0);
+	expect_pgm_int();
+	channel_subsystem_call_key_1(communication_block);
+	check_key_prot_exc(ACC_UPDATE, PROT_STORE);
+	report_prefix_pop();
+
+	report_prefix_pop();
+
+	set_storage_key(communication_block, 0x00, 0);
+	report_prefix_pop();
+}
+
 /*
  * Perform SET PREFIX (SPX) instruction while temporarily executing
  * with access key 1.
@@ -410,6 +519,172 @@ static void test_set_prefix(void)
 	report_prefix_pop();
 }
 
+/*
+ * Perform MODIFY SUBCHANNEL (MSCH) instruction while temporarily executing
+ * with access key 1.
+ */
+static uint32_t modify_subchannel_key_1(uint32_t sid, struct schib *schib)
+{
+	uint32_t program_mask;
+
+	asm volatile (
+		"lr %%r1,%[sid]\n\t"
+		"spka	0x10\n\t"
+		"msch	%[schib]\n\t"
+		"spka	0\n\t"
+		"ipm	%[program_mask]\n"
+		: [program_mask] "=d" (program_mask)
+		: [sid] "d" (sid),
+		  [schib] "Q" (*schib)
+		: "%r1"
+	);
+	return program_mask >> 28;
+}
+
+static void test_msch(void)
+{
+	struct schib *schib = (struct schib *)pagebuf;
+	struct schib *no_override_schib;
+	int test_device_sid;
+	pgd_t *root;
+	int cc;
+
+	report_prefix_push("MSCH");
+	root = (pgd_t *)(stctg(1) & PAGE_MASK);
+	test_device_sid = css_enumerate();
+
+	if (!(test_device_sid & SCHID_ONE)) {
+		report_fail("no I/O device found");
+		return;
+	}
+
+	cc = stsch(test_device_sid, schib);
+	if (cc) {
+		report_fail("could not store SCHIB");
+		return;
+	}
+
+	report_prefix_push("zero key");
+	schib->pmcw.intparm = 100;
+	set_storage_key(schib, 0x28, 0);
+	cc = msch(test_device_sid, schib);
+	if (!cc) {
+		WRITE_ONCE(schib->pmcw.intparm, 0);
+		cc = stsch(test_device_sid, schib);
+		report(!cc && schib->pmcw.intparm == 100, "fetched from SCHIB");
+	} else {
+		report_fail("MSCH cc != 0");
+	}
+	report_prefix_pop();
+
+	report_prefix_push("matching key");
+	schib->pmcw.intparm = 200;
+	set_storage_key(schib, 0x18, 0);
+	cc = modify_subchannel_key_1(test_device_sid, schib);
+	if (!cc) {
+		WRITE_ONCE(schib->pmcw.intparm, 0);
+		cc = stsch(test_device_sid, schib);
+		report(!cc && schib->pmcw.intparm == 200, "fetched from SCHIB");
+	} else {
+		report_fail("MSCH cc != 0");
+	}
+	report_prefix_pop();
+
+	report_prefix_push("mismatching key");
+
+	report_prefix_push("no fetch protection");
+	schib->pmcw.intparm = 300;
+	set_storage_key(schib, 0x20, 0);
+	cc = modify_subchannel_key_1(test_device_sid, schib);
+	if (!cc) {
+		WRITE_ONCE(schib->pmcw.intparm, 0);
+		cc = stsch(test_device_sid, schib);
+		report(!cc && schib->pmcw.intparm == 300, "fetched from SCHIB");
+	} else {
+		report_fail("MSCH cc != 0");
+	}
+	report_prefix_pop();
+
+	schib->pmcw.intparm = 0;
+	if (!msch(test_device_sid, schib)) {
+		report_prefix_push("fetch protection");
+		schib->pmcw.intparm = 400;
+		set_storage_key(schib, 0x28, 0);
+		expect_pgm_int();
+		modify_subchannel_key_1(test_device_sid, schib);
+		check_key_prot_exc(ACC_FETCH, PROT_FETCH_STORE);
+		WRITE_ONCE(schib->pmcw.intparm, 0);
+		cc = stsch(test_device_sid, schib);
+		report(!cc && schib->pmcw.intparm == 0, "did not modify subchannel");
+		report_prefix_pop();
+	} else {
+		report_fail("could not reset SCHIB");
+	}
+
+	register_pgm_cleanup_func(dat_fixup_pgm_int);
+
+	schib->pmcw.intparm = 0;
+	if (!msch(test_device_sid, schib)) {
+		report_prefix_push("remapped page, fetch protection");
+		schib->pmcw.intparm = 500;
+		set_storage_key(pagebuf, 0x28, 0);
+		expect_pgm_int();
+		install_page(root, virt_to_pte_phys(root, pagebuf), 0);
+		modify_subchannel_key_1(test_device_sid, (struct schib *)0);
+		install_page(root, 0, 0);
+		check_key_prot_exc(ACC_FETCH, PROT_FETCH_STORE);
+		WRITE_ONCE(schib->pmcw.intparm, 0);
+		cc = stsch(test_device_sid, schib);
+		report(!cc && schib->pmcw.intparm == 0, "did not modify subchannel");
+		report_prefix_pop();
+	} else {
+		report_fail("could not reset SCHIB");
+	}
+
+	ctl_set_bit(0, CTL0_FETCH_PROTECTION_OVERRIDE);
+
+	report_prefix_push("fetch-protection override applies");
+	schib->pmcw.intparm = 600;
+	set_storage_key(pagebuf, 0x28, 0);
+	install_page(root, virt_to_pte_phys(root, pagebuf), 0);
+	cc = modify_subchannel_key_1(test_device_sid, (struct schib *)0);
+	install_page(root, 0, 0);
+	if (!cc) {
+		WRITE_ONCE(schib->pmcw.intparm, 0);
+		cc = stsch(test_device_sid, schib);
+		report(!cc && schib->pmcw.intparm == 600, "fetched from SCHIB");
+	} else {
+		report_fail("MSCH cc != 0");
+	}
+	report_prefix_pop();
+
+	schib->pmcw.intparm = 0;
+	if (!msch(test_device_sid, schib)) {
+		report_prefix_push("fetch-protection override does not apply");
+		schib->pmcw.intparm = 700;
+		no_override_schib = (struct schib *)(pagebuf + 2048);
+		memcpy(no_override_schib, schib, sizeof(struct schib));
+		set_storage_key(pagebuf, 0x28, 0);
+		expect_pgm_int();
+		install_page(root, virt_to_pte_phys(root, pagebuf), 0);
+		modify_subchannel_key_1(test_device_sid, (struct schib *)2048);
+		install_page(root, 0, 0);
+		check_key_prot_exc(ACC_FETCH, PROT_FETCH_STORE);
+		WRITE_ONCE(schib->pmcw.intparm, 0);
+		cc = stsch(test_device_sid, schib);
+		report(!cc && schib->pmcw.intparm == 0, "did not modify subchannel");
+		report_prefix_pop();
+	} else {
+		report_fail("could not reset SCHIB");
+	}
+
+	ctl_clear_bit(0, CTL0_FETCH_PROTECTION_OVERRIDE);
+	register_pgm_cleanup_func(NULL);
+	report_prefix_pop();
+	set_storage_key(schib, 0x00, 0);
+	report_prefix_pop();
+}
+
 int main(void)
 {
 	report_prefix_push("skey");
@@ -424,9 +699,11 @@ int main(void)
 	test_chg();
 	test_test_protection();
 	test_store_cpu_address();
+	test_channel_subsystem_call();
 
 	setup_vm();
 	test_set_prefix();
+	test_msch();
 done:
 	report_prefix_pop();
 	return report_summary();
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index 743013b2..069c41a7 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -41,6 +41,7 @@ file = sthyi.elf
 
 [skey]
 file = skey.elf
+extra_params = -device virtio-net-ccw
 
 [diag10]
 file = diag10.elf
-- 
2.33.1

Re: [kvm-unit-tests PATCH 1/3] s390x: Fix sclp facility bit numbers

[Claudio Imbrenda]

On Thu,  5 May 2022 14:46:54 +0200
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

> sclp_feat_check takes care of adjusting the bit numbering such that they
> can be defined as they are in the documentation.

this means we had it wrong all along and we somehow never noticed

ooops!

anyway:

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> 
> Fixes: 4dd649c8 ("lib: s390x: sclp: Extend feature probing")
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
>  lib/s390x/sclp.h | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
> index fead007a..4ce2209f 100644
> --- a/lib/s390x/sclp.h
> +++ b/lib/s390x/sclp.h
> @@ -134,13 +134,15 @@ struct sclp_facilities {
>  };
>  
>  /* bit number within a certain byte */
> -#define SCLP_FEAT_85_BIT_GSLS		7
> -#define SCLP_FEAT_98_BIT_KSS		0
> -#define SCLP_FEAT_116_BIT_64BSCAO	7
> -#define SCLP_FEAT_116_BIT_CMMA		6
> -#define SCLP_FEAT_116_BIT_ESCA		3
> -#define SCLP_FEAT_117_BIT_PFMFI		6
> -#define SCLP_FEAT_117_BIT_IBS		5
> +#define SCLP_FEAT_80_BIT_SOP		2
> +#define SCLP_FEAT_85_BIT_GSLS		0
> +#define SCLP_FEAT_85_BIT_ESOP		6
> +#define SCLP_FEAT_98_BIT_KSS		7
> +#define SCLP_FEAT_116_BIT_64BSCAO	0
> +#define SCLP_FEAT_116_BIT_CMMA		1
> +#define SCLP_FEAT_116_BIT_ESCA		4
> +#define SCLP_FEAT_117_BIT_PFMFI		1
> +#define SCLP_FEAT_117_BIT_IBS		2
>  
>  typedef struct ReadInfo {
>  	SCCBHeader h;

Re: [kvm-unit-tests PATCH 2/3] s390x: Test TEID values in storage key test

[Claudio Imbrenda]

On Thu,  5 May 2022 14:46:55 +0200
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

> On a protection exception, test that the Translation-Exception
> Identification (TEID) values are correct given the circumstances of the
> particular test.
> The meaning of the TEID values is dependent on the installed
> suppression-on-protection facility.
> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
>  lib/s390x/asm/facility.h | 21 ++++++++++++++
>  lib/s390x/sclp.h         |  2 ++
>  lib/s390x/sclp.c         |  2 ++
>  s390x/skey.c             | 60 ++++++++++++++++++++++++++++++++++++----
>  4 files changed, 79 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/s390x/asm/facility.h b/lib/s390x/asm/facility.h
> index ef0fd037..f21bb9d7 100644
> --- a/lib/s390x/asm/facility.h
> +++ b/lib/s390x/asm/facility.h
> @@ -12,6 +12,7 @@
>  #include <asm/facility.h>
>  #include <asm/arch_def.h>
>  #include <bitops.h>
> +#include <sclp.h>
>  
>  #define NB_STFL_DOUBLEWORDS 32
>  extern uint64_t stfl_doublewords[];
> @@ -44,4 +45,24 @@ static inline void setup_facilities(void)
>  		stfle(stfl_doublewords, NB_STFL_DOUBLEWORDS);
>  }
>  
> +enum supp_on_prot_facility {
> +	SOP_NONE,
> +	SOP_BASIC,
> +	SOP_ENHANCED_1,
> +	SOP_ENHANCED_2,
> +};
> +
> +static inline enum supp_on_prot_facility get_supp_on_prot_facility(void)
> +{
> +	if (sclp_facilities.has_esop) {
> +		if (test_facility(131)) /* side-effect-access facility */
> +			return SOP_ENHANCED_2;
> +		else
> +			return SOP_ENHANCED_1;
> +	}
> +	if (sclp_facilities.has_sop)
> +		return SOP_BASIC;
> +	return SOP_NONE;
> +}
> +
>  #endif
> diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
> index 4ce2209f..f57896b2 100644
> --- a/lib/s390x/sclp.h
> +++ b/lib/s390x/sclp.h
> @@ -123,7 +123,9 @@ struct sclp_facilities {
>  	uint64_t has_cei : 1;
>  
>  	uint64_t has_diag318 : 1;
> +	uint64_t has_sop : 1;
>  	uint64_t has_gsls : 1;
> +	uint64_t has_esop : 1;
>  	uint64_t has_cmma : 1;
>  	uint64_t has_64bscao : 1;
>  	uint64_t has_esca : 1;
> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
> index b8204c5f..e6017f64 100644
> --- a/lib/s390x/sclp.c
> +++ b/lib/s390x/sclp.c
> @@ -152,7 +152,9 @@ void sclp_facilities_setup(void)
>  	cpu = sclp_get_cpu_entries();
>  	if (read_info->offset_cpu > 134)
>  		sclp_facilities.has_diag318 = read_info->byte_134_diag318;
> +	sclp_facilities.has_sop = sclp_feat_check(80, SCLP_FEAT_80_BIT_SOP);
>  	sclp_facilities.has_gsls = sclp_feat_check(85, SCLP_FEAT_85_BIT_GSLS);
> +	sclp_facilities.has_esop = sclp_feat_check(85, SCLP_FEAT_85_BIT_ESOP);
>  	sclp_facilities.has_kss = sclp_feat_check(98, SCLP_FEAT_98_BIT_KSS);
>  	sclp_facilities.has_cmma = sclp_feat_check(116, SCLP_FEAT_116_BIT_CMMA);
>  	sclp_facilities.has_64bscao = sclp_feat_check(116, SCLP_FEAT_116_BIT_64BSCAO);
> diff --git a/s390x/skey.c b/s390x/skey.c
> index 32bf1070..56bf5f45 100644
> --- a/s390x/skey.c
> +++ b/s390x/skey.c
> @@ -8,6 +8,7 @@
>   *  Janosch Frank <frankja@linux.vnet.ibm.com>
>   */
>  #include <libcflat.h>
> +#include <bitops.h>
>  #include <asm/asm-offsets.h>
>  #include <asm/interrupt.h>
>  #include <vmalloc.h>
> @@ -158,6 +159,53 @@ static void test_test_protection(void)
>  	report_prefix_pop();
>  }
>  
> +enum access {
> +	ACC_FETCH = 2,
> +	ACC_STORE = 1,
> +	ACC_UPDATE = 3,
> +};

why not in numerical order?

> +
> +enum protection {
> +	PROT_STORE = 1,
> +	PROT_FETCH_STORE = 3,
> +};

what happened to 2?

> +
> +static void check_key_prot_exc(enum access access, enum protection prot)
> +{
> +	struct lowcore *lc = 0;
> +	union teid teid;
> +
> +	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
> +	report_prefix_push("TEID");
> +	teid.val = lc->trans_exc_id;
> +	switch (get_supp_on_prot_facility()) {
> +	case SOP_NONE:
> +	case SOP_BASIC:
> +		break;
> +	case SOP_ENHANCED_1:
> +		if ((teid.val & (BIT(63 - 61))) == 0)

why not teid.m?

> +			report_pass("key-controlled protection");
> +		break;
> +	case SOP_ENHANCED_2:
> +		if ((teid.val & (BIT(63 - 56) | BIT(63 - 61))) == 0) {

maybe here you need to expand struct teid a little to accomodate for
bit 56.

> +			report_pass("key-controlled protection");
> +			if (teid.val & BIT(63 - 60)) {
> +				int access_code = teid.fetch << 1 | teid.store;
> +
> +				report_info("access code: %d", access_code);

I don't like an unconditional report_info (it's ok to aid debugging if
something fails)

> +				if (access_code == 2)
> +					report((access & 2) && (prot & 2),
> +					       "exception due to fetch");
> +				if (access_code == 1)
> +					report((access & 1) && (prot & 1),
> +					       "exception due to store");

what about cases 0 and 3?
if they should never happen, handle it properly
and if they can happen... handle it properly

> +			}
> +		}
> +		break;
> +	}
> +	report_prefix_pop();
> +}
> +
>  /*
>   * Perform STORE CPU ADDRESS (STAP) instruction while temporarily executing
>   * with access key 1.
> @@ -199,7 +247,7 @@ static void test_store_cpu_address(void)
>  	expect_pgm_int();
>  	*out = 0xbeef;
>  	store_cpu_address_key_1(out);
> -	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
> +	check_key_prot_exc(ACC_STORE, PROT_STORE);
>  	report(*out == 0xbeef, "no store occurred");
>  	report_prefix_pop();
>  
> @@ -210,7 +258,7 @@ static void test_store_cpu_address(void)
>  	expect_pgm_int();
>  	*out = 0xbeef;
>  	store_cpu_address_key_1(out);
> -	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
> +	check_key_prot_exc(ACC_STORE, PROT_STORE);
>  	report(*out == 0xbeef, "no store occurred");
>  	report_prefix_pop();
>  
> @@ -228,7 +276,7 @@ static void test_store_cpu_address(void)
>  	expect_pgm_int();
>  	*out = 0xbeef;
>  	store_cpu_address_key_1(out);
> -	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
> +	check_key_prot_exc(ACC_STORE, PROT_STORE);
>  	report(*out == 0xbeef, "no store occurred");
>  	report_prefix_pop();
>  
> @@ -314,7 +362,7 @@ static void test_set_prefix(void)
>  	set_storage_key(pagebuf, 0x28, 0);
>  	expect_pgm_int();
>  	set_prefix_key_1(prefix_ptr);
> -	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
> +	check_key_prot_exc(ACC_FETCH, PROT_FETCH_STORE);
>  	report(get_prefix() == old_prefix, "did not set prefix");
>  	report_prefix_pop();
>  
> @@ -327,7 +375,7 @@ static void test_set_prefix(void)
>  	install_page(root, virt_to_pte_phys(root, pagebuf), 0);
>  	set_prefix_key_1((uint32_t *)0);
>  	install_page(root, 0, 0);
> -	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
> +	check_key_prot_exc(ACC_FETCH, PROT_FETCH_STORE);
>  	report(get_prefix() == old_prefix, "did not set prefix");
>  	report_prefix_pop();
>  
> @@ -351,7 +399,7 @@ static void test_set_prefix(void)
>  	install_page(root, virt_to_pte_phys(root, pagebuf), 0);
>  	set_prefix_key_1((uint32_t *)2048);
>  	install_page(root, 0, 0);
> -	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
> +	check_key_prot_exc(ACC_FETCH, PROT_FETCH_STORE);
>  	report(get_prefix() == old_prefix, "did not set prefix");
>  	report_prefix_pop();
>  

Re: [kvm-unit-tests PATCH 3/3] s390x: Test effect of storage keys on some more instructions

[Claudio Imbrenda]

On Thu,  5 May 2022 14:46:56 +0200
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

> Test correctness of some instructions handled by user space instead of
> KVM with regards to storage keys.
> Test success and error conditions, including coverage of storage and
> fetch protection override.
> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
>  s390x/skey.c        | 277 ++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg |   1 +
>  2 files changed, 278 insertions(+)
> 
> diff --git a/s390x/skey.c b/s390x/skey.c
> index 56bf5f45..d50470a8 100644
> --- a/s390x/skey.c
> +++ b/s390x/skey.c
> @@ -12,6 +12,7 @@
>  #include <asm/asm-offsets.h>
>  #include <asm/interrupt.h>
>  #include <vmalloc.h>
> +#include <css.h>
>  #include <asm/page.h>
>  #include <asm/facility.h>
>  #include <asm/mem.h>
> @@ -284,6 +285,114 @@ static void test_store_cpu_address(void)
>  	report_prefix_pop();
>  }
>  
> +/*
> + * Perform CHANNEL SUBSYSTEM CALL (CHSC)  instruction while temporarily executing
> + * with access key 1.
> + */
> +static unsigned int channel_subsystem_call_key_1(void *communication_block)

this function name is very long (maybe chsc_with_key_1 instead?)

> +{
> +	uint32_t program_mask;
> +
> +	asm volatile (
> +		"spka	0x10\n\t"
> +		".insn	rre,0xb25f0000,%[communication_block],0\n\t"
> +		"spka	0\n\t"
> +		"ipm	%[program_mask]\n"
> +		: [program_mask] "=d" (program_mask)
> +		: [communication_block] "d" (communication_block)
> +		: "memory"
> +	);
> +	return program_mask >> 28;
> +}
> +
> +static void init_store_channel_subsystem_characteristics(uint16_t *communication_block)

same here (init_comm_block?)

> +{
> +	memset(communication_block, 0, PAGE_SIZE);
> +	communication_block[0] = 0x10;
> +	communication_block[1] = 0x10;
> +	communication_block[9] = 0;
> +}
> +
> +static void test_channel_subsystem_call(void)
> +{
> +	static const char request_name[] = "Store channel-subsystem-characteristics";

so this "request_name" is for when CHSC succeeds? why not just
"Success" then?

> +	uint16_t *communication_block = (uint16_t *)&pagebuf;

long name (consider comm_block, or even cb)

> +	unsigned int cc;
> +
> +	report_prefix_push("CHANNEL SUBSYSTEM CALL");
> +
> +	report_prefix_push("zero key");
> +	init_store_channel_subsystem_characteristics(communication_block);

see what I mean when I say that the names are too long? ^

> +	set_storage_key(communication_block, 0x10, 0);
> +	asm volatile (
> +		".insn	rre,0xb25f0000,%[communication_block],0\n\t"
> +		"ipm	%[cc]\n"
> +		: [cc] "=d" (cc)
> +		: [communication_block] "d" (communication_block)
> +		: "memory"
> +	);
> +	cc = cc >> 28;
> +	report(cc == 0 && communication_block[9], request_name);
> +	report_prefix_pop();
> +
> +	report_prefix_push("matching key");
> +	init_store_channel_subsystem_characteristics(communication_block);
> +	set_storage_key(communication_block, 0x10, 0);

you just set the storage key in the previous test, and you did not set
it back to 0, why do you need to set it again?

> +	cc = channel_subsystem_call_key_1(communication_block);
> +	report(cc == 0 && communication_block[9], request_name);
> +	report_prefix_pop();
> +
> +	report_prefix_push("mismatching key");
> +
> +	report_prefix_push("no fetch protection");
> +	init_store_channel_subsystem_characteristics(communication_block);
> +	set_storage_key(communication_block, 0x20, 0);
> +	expect_pgm_int();
> +	channel_subsystem_call_key_1(communication_block);
> +	check_key_prot_exc(ACC_UPDATE, PROT_STORE);
> +	report_prefix_pop();
> +
> +	report_prefix_push("fetch protection");
> +	init_store_channel_subsystem_characteristics(communication_block);
> +	set_storage_key(communication_block, 0x28, 0);
> +	expect_pgm_int();
> +	channel_subsystem_call_key_1(communication_block);
> +	check_key_prot_exc(ACC_UPDATE, PROT_FETCH_STORE);
> +	report_prefix_pop();
> +
> +	ctl_set_bit(0, CTL0_STORAGE_PROTECTION_OVERRIDE);
> +
> +	report_prefix_push("storage-protection override, invalid key");
> +	set_storage_key(communication_block, 0x20, 0);
> +	init_store_channel_subsystem_characteristics(communication_block);
> +	expect_pgm_int();
> +	channel_subsystem_call_key_1(communication_block);
> +	check_key_prot_exc(ACC_UPDATE, PROT_STORE);
> +	report_prefix_pop();
> +
> +	report_prefix_push("storage-protection override, override key");
> +	init_store_channel_subsystem_characteristics(communication_block);
> +	set_storage_key(communication_block, 0x90, 0);
> +	cc = channel_subsystem_call_key_1(communication_block);
> +	report(cc == 0 && communication_block[9], request_name);
> +	report_prefix_pop();
> +
> +	ctl_clear_bit(0, CTL0_STORAGE_PROTECTION_OVERRIDE);
> +
> +	report_prefix_push("storage-protection override disabled, override key");
> +	init_store_channel_subsystem_characteristics(communication_block);
> +	set_storage_key(communication_block, 0x90, 0);
> +	expect_pgm_int();
> +	channel_subsystem_call_key_1(communication_block);
> +	check_key_prot_exc(ACC_UPDATE, PROT_STORE);
> +	report_prefix_pop();
> +
> +	report_prefix_pop();
> +
> +	set_storage_key(communication_block, 0x00, 0);
> +	report_prefix_pop();
> +}
> +
>  /*
>   * Perform SET PREFIX (SPX) instruction while temporarily executing
>   * with access key 1.
> @@ -410,6 +519,172 @@ static void test_set_prefix(void)
>  	report_prefix_pop();
>  }
>  
> +/*
> + * Perform MODIFY SUBCHANNEL (MSCH) instruction while temporarily executing
> + * with access key 1.
> + */
> +static uint32_t modify_subchannel_key_1(uint32_t sid, struct schib *schib)
> +{
> +	uint32_t program_mask;
> +
> +	asm volatile (
> +		"lr %%r1,%[sid]\n\t"
> +		"spka	0x10\n\t"
> +		"msch	%[schib]\n\t"
> +		"spka	0\n\t"
> +		"ipm	%[program_mask]\n"
> +		: [program_mask] "=d" (program_mask)
> +		: [sid] "d" (sid),
> +		  [schib] "Q" (*schib)
> +		: "%r1"
> +	);
> +	return program_mask >> 28;
> +}
> +
> +static void test_msch(void)
> +{
> +	struct schib *schib = (struct schib *)pagebuf;
> +	struct schib *no_override_schib;
> +	int test_device_sid;
> +	pgd_t *root;
> +	int cc;
> +
> +	report_prefix_push("MSCH");
> +	root = (pgd_t *)(stctg(1) & PAGE_MASK);
> +	test_device_sid = css_enumerate();
> +
> +	if (!(test_device_sid & SCHID_ONE)) {
> +		report_fail("no I/O device found");
> +		return;
> +	}
> +
> +	cc = stsch(test_device_sid, schib);
> +	if (cc) {
> +		report_fail("could not store SCHIB");
> +		return;
> +	}
> +
> +	report_prefix_push("zero key");
> +	schib->pmcw.intparm = 100;
> +	set_storage_key(schib, 0x28, 0);
> +	cc = msch(test_device_sid, schib);
> +	if (!cc) {
> +		WRITE_ONCE(schib->pmcw.intparm, 0);

why are you using WRITE_ONCE here?

> +		cc = stsch(test_device_sid, schib);
> +		report(!cc && schib->pmcw.intparm == 100, "fetched from SCHIB");
> +	} else {
> +		report_fail("MSCH cc != 0");
> +	}
> +	report_prefix_pop();
> +
> +	report_prefix_push("matching key");
> +	schib->pmcw.intparm = 200;
> +	set_storage_key(schib, 0x18, 0);
> +	cc = modify_subchannel_key_1(test_device_sid, schib);
> +	if (!cc) {
> +		WRITE_ONCE(schib->pmcw.intparm, 0);
> +		cc = stsch(test_device_sid, schib);
> +		report(!cc && schib->pmcw.intparm == 200, "fetched from SCHIB");
> +	} else {
> +		report_fail("MSCH cc != 0");
> +	}
> +	report_prefix_pop();
> +
> +	report_prefix_push("mismatching key");
> +
> +	report_prefix_push("no fetch protection");
> +	schib->pmcw.intparm = 300;
> +	set_storage_key(schib, 0x20, 0);
> +	cc = modify_subchannel_key_1(test_device_sid, schib);
> +	if (!cc) {
> +		WRITE_ONCE(schib->pmcw.intparm, 0);
> +		cc = stsch(test_device_sid, schib);
> +		report(!cc && schib->pmcw.intparm == 300, "fetched from SCHIB");
> +	} else {
> +		report_fail("MSCH cc != 0");
> +	}
> +	report_prefix_pop();
> +
> +	schib->pmcw.intparm = 0;
> +	if (!msch(test_device_sid, schib)) {
> +		report_prefix_push("fetch protection");
> +		schib->pmcw.intparm = 400;
> +		set_storage_key(schib, 0x28, 0);
> +		expect_pgm_int();
> +		modify_subchannel_key_1(test_device_sid, schib);
> +		check_key_prot_exc(ACC_FETCH, PROT_FETCH_STORE);
> +		WRITE_ONCE(schib->pmcw.intparm, 0);
> +		cc = stsch(test_device_sid, schib);
> +		report(!cc && schib->pmcw.intparm == 0, "did not modify subchannel");
> +		report_prefix_pop();
> +	} else {
> +		report_fail("could not reset SCHIB");
> +	}
> +
> +	register_pgm_cleanup_func(dat_fixup_pgm_int);
> +
> +	schib->pmcw.intparm = 0;
> +	if (!msch(test_device_sid, schib)) {
> +		report_prefix_push("remapped page, fetch protection");
> +		schib->pmcw.intparm = 500;
> +		set_storage_key(pagebuf, 0x28, 0);
> +		expect_pgm_int();
> +		install_page(root, virt_to_pte_phys(root, pagebuf), 0);
> +		modify_subchannel_key_1(test_device_sid, (struct schib *)0);
> +		install_page(root, 0, 0);
> +		check_key_prot_exc(ACC_FETCH, PROT_FETCH_STORE);
> +		WRITE_ONCE(schib->pmcw.intparm, 0);
> +		cc = stsch(test_device_sid, schib);
> +		report(!cc && schib->pmcw.intparm == 0, "did not modify subchannel");
> +		report_prefix_pop();
> +	} else {
> +		report_fail("could not reset SCHIB");
> +	}
> +
> +	ctl_set_bit(0, CTL0_FETCH_PROTECTION_OVERRIDE);
> +
> +	report_prefix_push("fetch-protection override applies");
> +	schib->pmcw.intparm = 600;
> +	set_storage_key(pagebuf, 0x28, 0);
> +	install_page(root, virt_to_pte_phys(root, pagebuf), 0);
> +	cc = modify_subchannel_key_1(test_device_sid, (struct schib *)0);
> +	install_page(root, 0, 0);
> +	if (!cc) {
> +		WRITE_ONCE(schib->pmcw.intparm, 0);
> +		cc = stsch(test_device_sid, schib);
> +		report(!cc && schib->pmcw.intparm == 600, "fetched from SCHIB");
> +	} else {
> +		report_fail("MSCH cc != 0");
> +	}
> +	report_prefix_pop();
> +
> +	schib->pmcw.intparm = 0;
> +	if (!msch(test_device_sid, schib)) {
> +		report_prefix_push("fetch-protection override does not apply");
> +		schib->pmcw.intparm = 700;
> +		no_override_schib = (struct schib *)(pagebuf + 2048);
> +		memcpy(no_override_schib, schib, sizeof(struct schib));
> +		set_storage_key(pagebuf, 0x28, 0);
> +		expect_pgm_int();
> +		install_page(root, virt_to_pte_phys(root, pagebuf), 0);
> +		modify_subchannel_key_1(test_device_sid, (struct schib *)2048);
> +		install_page(root, 0, 0);
> +		check_key_prot_exc(ACC_FETCH, PROT_FETCH_STORE);
> +		WRITE_ONCE(schib->pmcw.intparm, 0);
> +		cc = stsch(test_device_sid, schib);
> +		report(!cc && schib->pmcw.intparm == 0, "did not modify subchannel");
> +		report_prefix_pop();
> +	} else {
> +		report_fail("could not reset SCHIB");
> +	}
> +
> +	ctl_clear_bit(0, CTL0_FETCH_PROTECTION_OVERRIDE);
> +	register_pgm_cleanup_func(NULL);
> +	report_prefix_pop();
> +	set_storage_key(schib, 0x00, 0);
> +	report_prefix_pop();
> +}
> +
>  int main(void)
>  {
>  	report_prefix_push("skey");
> @@ -424,9 +699,11 @@ int main(void)
>  	test_chg();
>  	test_test_protection();
>  	test_store_cpu_address();
> +	test_channel_subsystem_call();
>  
>  	setup_vm();
>  	test_set_prefix();
> +	test_msch();
>  done:
>  	report_prefix_pop();
>  	return report_summary();
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index 743013b2..069c41a7 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -41,6 +41,7 @@ file = sthyi.elf
>  
>  [skey]
>  file = skey.elf
> +extra_params = -device virtio-net-ccw
>  
>  [diag10]
>  file = diag10.elf

Re: [kvm-unit-tests PATCH 2/3] s390x: Test TEID values in storage key test

[Janis Schoetterl-Glausch]

On 5/6/22 17:37, Claudio Imbrenda wrote:
> On Thu,  5 May 2022 14:46:55 +0200
> Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
> 
>> On a protection exception, test that the Translation-Exception
>> Identification (TEID) values are correct given the circumstances of the
>> particular test.
>> The meaning of the TEID values is dependent on the installed
>> suppression-on-protection facility.
>>
>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>> ---
>>  lib/s390x/asm/facility.h | 21 ++++++++++++++
>>  lib/s390x/sclp.h         |  2 ++
>>  lib/s390x/sclp.c         |  2 ++
>>  s390x/skey.c             | 60 ++++++++++++++++++++++++++++++++++++----
>>  4 files changed, 79 insertions(+), 6 deletions(-)
>>

[...]

>>  
>> +enum access {
>> +	ACC_FETCH = 2,
>> +	ACC_STORE = 1,
>> +	ACC_UPDATE = 3,
>> +};
> 
> why not in numerical order?

The numbers are chosen such that the bit masking in the function below
is nicer, but the ordering is basically arbitrary.
Somehow fetch, store, update seems natural to me, but I can sort it.
I had ACC_NONE for a bit, but as I don't need it, I removed it.
> 
>> +
>> +enum protection {
>> +	PROT_STORE = 1,
>> +	PROT_FETCH_STORE = 3,
>> +};
> 
> what happened to 2?

There is no such thing as fetch only protection, so that's a result
of the choice of values for masking.
> 
>> +
>> +static void check_key_prot_exc(enum access access, enum protection prot)
>> +{
>> +	struct lowcore *lc = 0;
>> +	union teid teid;
>> +
>> +	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
>> +	report_prefix_push("TEID");
>> +	teid.val = lc->trans_exc_id;
>> +	switch (get_supp_on_prot_facility()) {
>> +	case SOP_NONE:
>> +	case SOP_BASIC:
>> +		break;
>> +	case SOP_ENHANCED_1:
>> +		if ((teid.val & (BIT(63 - 61))) == 0)
> 
> why not teid.m?

I considered using .m, but the name does not contain any more information
on the meaning than just the number.
The PoP talks of the bits by their numbers in the suppression on detection
chapter, so this is the most straight forward translation into code.
> 
>> +			report_pass("key-controlled protection");
>> +		break;
>> +	case SOP_ENHANCED_2:
>> +		if ((teid.val & (BIT(63 - 56) | BIT(63 - 61))) == 0) {
> 
> maybe here you need to expand struct teid a little to accomodate for
> bit 56.

I could not think of a good name.
> 
>> +			report_pass("key-controlled protection");
>> +			if (teid.val & BIT(63 - 60)) {
>> +				int access_code = teid.fetch << 1 | teid.store;
>> +
>> +				report_info("access code: %d", access_code);
> 
> I don't like an unconditional report_info (it's ok to aid debugging if
> something fails)

In the case of update references the value you get is unspecified, so I found
it interesting to see what happens in LPAR.
I could only print it for update references, but I'm also fine with just
dropping it. What do you think?
> 
>> +				if (access_code == 2)
>> +					report((access & 2) && (prot & 2),
>> +					       "exception due to fetch");
>> +				if (access_code == 1)
>> +					report((access & 1) && (prot & 1),
>> +					       "exception due to store");
> 
> what about cases 0 and 3?

Case 0 is specified to not contain any information and 3 is reserved,
so also no information.

> if they should never happen, handle it properly
> and if they can happen... handle it properly
> 
>> +			}
>> +		}
>> +		break;
>> +	}
>> +	report_prefix_pop();
>> +}
>> +
>>  /*
>>   * Perform STORE CPU ADDRESS (STAP) instruction while temporarily executing
>>   * with access key 1.
>> @@ -199,7 +247,7 @@ static void test_store_cpu_address(void)
>>  	expect_pgm_int();
>>  	*out = 0xbeef;
>>  	store_cpu_address_key_1(out);
>> -	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
>> +	check_key_prot_exc(ACC_STORE, PROT_STORE);
>>  	report(*out == 0xbeef, "no store occurred");
>>  	report_prefix_pop();
>>  
>> @@ -210,7 +258,7 @@ static void test_store_cpu_address(void)
>>  	expect_pgm_int();
>>  	*out = 0xbeef;
>>  	store_cpu_address_key_1(out);
>> -	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
>> +	check_key_prot_exc(ACC_STORE, PROT_STORE);
>>  	report(*out == 0xbeef, "no store occurred");
>>  	report_prefix_pop();
>>  
>> @@ -228,7 +276,7 @@ static void test_store_cpu_address(void)
>>  	expect_pgm_int();
>>  	*out = 0xbeef;
>>  	store_cpu_address_key_1(out);
>> -	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
>> +	check_key_prot_exc(ACC_STORE, PROT_STORE);
>>  	report(*out == 0xbeef, "no store occurred");
>>  	report_prefix_pop();
>>  
>> @@ -314,7 +362,7 @@ static void test_set_prefix(void)
>>  	set_storage_key(pagebuf, 0x28, 0);
>>  	expect_pgm_int();
>>  	set_prefix_key_1(prefix_ptr);
>> -	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
>> +	check_key_prot_exc(ACC_FETCH, PROT_FETCH_STORE);
>>  	report(get_prefix() == old_prefix, "did not set prefix");
>>  	report_prefix_pop();
>>  
>> @@ -327,7 +375,7 @@ static void test_set_prefix(void)
>>  	install_page(root, virt_to_pte_phys(root, pagebuf), 0);
>>  	set_prefix_key_1((uint32_t *)0);
>>  	install_page(root, 0, 0);
>> -	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
>> +	check_key_prot_exc(ACC_FETCH, PROT_FETCH_STORE);
>>  	report(get_prefix() == old_prefix, "did not set prefix");
>>  	report_prefix_pop();
>>  
>> @@ -351,7 +399,7 @@ static void test_set_prefix(void)
>>  	install_page(root, virt_to_pte_phys(root, pagebuf), 0);
>>  	set_prefix_key_1((uint32_t *)2048);
>>  	install_page(root, 0, 0);
>> -	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
>> +	check_key_prot_exc(ACC_FETCH, PROT_FETCH_STORE);
>>  	report(get_prefix() == old_prefix, "did not set prefix");
>>  	report_prefix_pop();
>>  
> 

Re: [kvm-unit-tests PATCH 3/3] s390x: Test effect of storage keys on some more instructions

[Janis Schoetterl-Glausch]

On 5/6/22 18:52, Claudio Imbrenda wrote:
> On Thu,  5 May 2022 14:46:56 +0200
> Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
> 
>> Test correctness of some instructions handled by user space instead of
>> KVM with regards to storage keys.
>> Test success and error conditions, including coverage of storage and
>> fetch protection override.
>>
>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>> ---
>>  s390x/skey.c        | 277 ++++++++++++++++++++++++++++++++++++++++++++
>>  s390x/unittests.cfg |   1 +
>>  2 files changed, 278 insertions(+)
>>
>> diff --git a/s390x/skey.c b/s390x/skey.c
>> index 56bf5f45..d50470a8 100644
>> --- a/s390x/skey.c
>> +++ b/s390x/skey.c
>> @@ -12,6 +12,7 @@
>>  #include <asm/asm-offsets.h>
>>  #include <asm/interrupt.h>
>>  #include <vmalloc.h>
>> +#include <css.h>
>>  #include <asm/page.h>
>>  #include <asm/facility.h>
>>  #include <asm/mem.h>
>> @@ -284,6 +285,114 @@ static void test_store_cpu_address(void)
>>  	report_prefix_pop();
>>  }
>>  
>> +/*
>> + * Perform CHANNEL SUBSYSTEM CALL (CHSC)  instruction while temporarily executing
>> + * with access key 1.
>> + */
>> +static unsigned int channel_subsystem_call_key_1(void *communication_block)
> 
> this function name is very long (maybe chsc_with_key_1 instead?)

It's because of consistency with set_prefix_key_1 where I spelled out the instruction
name too. Granted the name of chsc is longer.
When in doubt I go for what seems more readable/contains more information.
> 
>> +{
>> +	uint32_t program_mask;
>> +
>> +	asm volatile (
>> +		"spka	0x10\n\t"
>> +		".insn	rre,0xb25f0000,%[communication_block],0\n\t"
>> +		"spka	0\n\t"
>> +		"ipm	%[program_mask]\n"
>> +		: [program_mask] "=d" (program_mask)
>> +		: [communication_block] "d" (communication_block)
>> +		: "memory"
>> +	);
>> +	return program_mask >> 28;
>> +}
>> +
>> +static void init_store_channel_subsystem_characteristics(uint16_t *communication_block)
> 
> same here (init_comm_block?)

Since we're only performing one kind of operation in this test, that is fine,
but I'll add a comment saying how we initialize the communication block then.
> 
>> +{
>> +	memset(communication_block, 0, PAGE_SIZE);
>> +	communication_block[0] = 0x10;
>> +	communication_block[1] = 0x10;
>> +	communication_block[9] = 0;
>> +}
>> +
>> +static void test_channel_subsystem_call(void)
>> +{
>> +	static const char request_name[] = "Store channel-subsystem-characteristics";
> 
> so this "request_name" is for when CHSC succeeds? why not just
> "Success" then?

That's the operation being performed. So maybe I should change it to
msg[] = "Performed store channel-subsystem-characteristics" ?
> 
>> +	uint16_t *communication_block = (uint16_t *)&pagebuf;
> 
> long name (consider comm_block, or even cb)
> 
>> +	unsigned int cc;
>> +
>> +	report_prefix_push("CHANNEL SUBSYSTEM CALL");
>> +
>> +	report_prefix_push("zero key");
>> +	init_store_channel_subsystem_characteristics(communication_block);
> 
> see what I mean when I say that the names are too long? ^

Fits in 80 columns ;-)
> 
>> +	set_storage_key(communication_block, 0x10, 0);
>> +	asm volatile (
>> +		".insn	rre,0xb25f0000,%[communication_block],0\n\t"
>> +		"ipm	%[cc]\n"
>> +		: [cc] "=d" (cc)
>> +		: [communication_block] "d" (communication_block)
>> +		: "memory"
>> +	);
>> +	cc = cc >> 28;
>> +	report(cc == 0 && communication_block[9], request_name);
>> +	report_prefix_pop();
>> +
>> +	report_prefix_push("matching key");
>> +	init_store_channel_subsystem_characteristics(communication_block);
>> +	set_storage_key(communication_block, 0x10, 0);
> 
> you just set the storage key in the previous test, and you did not set
> it back to 0, why do you need to set it again?

It's not necessary, but I want the tests to be independent from each other,
so you can remove/reorder/add ones without having to think.
> 
>> +	cc = channel_subsystem_call_key_1(communication_block);
>> +	report(cc == 0 && communication_block[9], request_name);
>> +	report_prefix_pop();
>> +

[...]

>> +
>> +	cc = stsch(test_device_sid, schib);
>> +	if (cc) {
>> +		report_fail("could not store SCHIB");
>> +		return;
>> +	}
>> +
>> +	report_prefix_push("zero key");
>> +	schib->pmcw.intparm = 100;
>> +	set_storage_key(schib, 0x28, 0);
>> +	cc = msch(test_device_sid, schib);
>> +	if (!cc) {
>> +		WRITE_ONCE(schib->pmcw.intparm, 0);
> 
> why are you using WRITE_ONCE here?

It's a dead store because of the stsch below.
That line is just for good measure so we know stsch really overwrote the value.
> 
>> +		cc = stsch(test_device_sid, schib);
>> +		report(!cc && schib->pmcw.intparm == 100, "fetched from SCHIB");

[...]

Re: [kvm-unit-tests PATCH 1/3] s390x: Fix sclp facility bit numbers

[Janosch Frank]

On 5/5/22 14:46, Janis Schoetterl-Glausch wrote:
> sclp_feat_check takes care of adjusting the bit numbering such that they
> can be defined as they are in the documentation.
> 
> Fixes: 4dd649c8 ("lib: s390x: sclp: Extend feature probing")
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>

The fixing part of this is:
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

But please add the (E)SOP bits in the other patches.

> ---
>   lib/s390x/sclp.h | 16 +++++++++-------
>   1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
> index fead007a..4ce2209f 100644
> --- a/lib/s390x/sclp.h
> +++ b/lib/s390x/sclp.h
> @@ -134,13 +134,15 @@ struct sclp_facilities {
>   };
>   
>   /* bit number within a certain byte */
> -#define SCLP_FEAT_85_BIT_GSLS		7
> -#define SCLP_FEAT_98_BIT_KSS		0
> -#define SCLP_FEAT_116_BIT_64BSCAO	7
> -#define SCLP_FEAT_116_BIT_CMMA		6
> -#define SCLP_FEAT_116_BIT_ESCA		3
> -#define SCLP_FEAT_117_BIT_PFMFI		6
> -#define SCLP_FEAT_117_BIT_IBS		5
> +#define SCLP_FEAT_80_BIT_SOP		2
> +#define SCLP_FEAT_85_BIT_GSLS		0
> +#define SCLP_FEAT_85_BIT_ESOP		6
> +#define SCLP_FEAT_98_BIT_KSS		7
> +#define SCLP_FEAT_116_BIT_64BSCAO	0
> +#define SCLP_FEAT_116_BIT_CMMA		1
> +#define SCLP_FEAT_116_BIT_ESCA		4
> +#define SCLP_FEAT_117_BIT_PFMFI		1
> +#define SCLP_FEAT_117_BIT_IBS		2
>   
>   typedef struct ReadInfo {
>   	SCCBHeader h;

Re: [kvm-unit-tests PATCH 1/3] s390x: Fix sclp facility bit numbers

[Janosch Frank]

On 5/6/22 17:31, Claudio Imbrenda wrote:
> On Thu,  5 May 2022 14:46:54 +0200
> Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
> 
>> sclp_feat_check takes care of adjusting the bit numbering such that they
>> can be defined as they are in the documentation.
> 
> this means we had it wrong all along and we somehow never noticed

That tends to happen when you add definitions and never actually use 
them. :)

> 
> ooops!
> 
> anyway:
> 
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> 
>>
>> Fixes: 4dd649c8 ("lib: s390x: sclp: Extend feature probing")
>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>> ---
>>   lib/s390x/sclp.h | 16 +++++++++-------
>>   1 file changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
>> index fead007a..4ce2209f 100644
>> --- a/lib/s390x/sclp.h
>> +++ b/lib/s390x/sclp.h
>> @@ -134,13 +134,15 @@ struct sclp_facilities {
>>   };
>>   
>>   /* bit number within a certain byte */
>> -#define SCLP_FEAT_85_BIT_GSLS		7
>> -#define SCLP_FEAT_98_BIT_KSS		0
>> -#define SCLP_FEAT_116_BIT_64BSCAO	7
>> -#define SCLP_FEAT_116_BIT_CMMA		6
>> -#define SCLP_FEAT_116_BIT_ESCA		3
>> -#define SCLP_FEAT_117_BIT_PFMFI		6
>> -#define SCLP_FEAT_117_BIT_IBS		5
>> +#define SCLP_FEAT_80_BIT_SOP		2
>> +#define SCLP_FEAT_85_BIT_GSLS		0
>> +#define SCLP_FEAT_85_BIT_ESOP		6
>> +#define SCLP_FEAT_98_BIT_KSS		7
>> +#define SCLP_FEAT_116_BIT_64BSCAO	0
>> +#define SCLP_FEAT_116_BIT_CMMA		1
>> +#define SCLP_FEAT_116_BIT_ESCA		4
>> +#define SCLP_FEAT_117_BIT_PFMFI		1
>> +#define SCLP_FEAT_117_BIT_IBS		2
>>   
>>   typedef struct ReadInfo {
>>   	SCCBHeader h;
> 

Re: [kvm-unit-tests PATCH 1/3] s390x: Fix sclp facility bit numbers

[Janis Schoetterl-Glausch]

On 5/10/22 19:13, Janosch Frank wrote:
> On 5/5/22 14:46, Janis Schoetterl-Glausch wrote:
>> sclp_feat_check takes care of adjusting the bit numbering such that they
>> can be defined as they are in the documentation.
>>
>> Fixes: 4dd649c8 ("lib: s390x: sclp: Extend feature probing")
>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> 
> The fixing part of this is:
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
> 
> But please add the (E)SOP bits in the other patches.

Oh, yeah, that slipped by me.
> 
>> ---
>>   lib/s390x/sclp.h | 16 +++++++++-------
>>   1 file changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
>> index fead007a..4ce2209f 100644
>> --- a/lib/s390x/sclp.h
>> +++ b/lib/s390x/sclp.h
>> @@ -134,13 +134,15 @@ struct sclp_facilities {
>>   };
>>     /* bit number within a certain byte */
>> -#define SCLP_FEAT_85_BIT_GSLS        7
>> -#define SCLP_FEAT_98_BIT_KSS        0
>> -#define SCLP_FEAT_116_BIT_64BSCAO    7
>> -#define SCLP_FEAT_116_BIT_CMMA        6
>> -#define SCLP_FEAT_116_BIT_ESCA        3
>> -#define SCLP_FEAT_117_BIT_PFMFI        6
>> -#define SCLP_FEAT_117_BIT_IBS        5
>> +#define SCLP_FEAT_80_BIT_SOP        2
>> +#define SCLP_FEAT_85_BIT_GSLS        0
>> +#define SCLP_FEAT_85_BIT_ESOP        6
>> +#define SCLP_FEAT_98_BIT_KSS        7
>> +#define SCLP_FEAT_116_BIT_64BSCAO    0
>> +#define SCLP_FEAT_116_BIT_CMMA        1
>> +#define SCLP_FEAT_116_BIT_ESCA        4
>> +#define SCLP_FEAT_117_BIT_PFMFI        1
>> +#define SCLP_FEAT_117_BIT_IBS        2
>>     typedef struct ReadInfo {
>>       SCCBHeader h;
>