[kvm-unit-tests PATCH v2 0/4] More skey instr. emulation test

[kvm-unit-tests PATCH v2 0/4] More skey instr. emulation test

[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.
Test that DIAG 308 is not subject to key protection.
Additionally, check the transaction exception identification on
protection exceptions.

v1 -> v2
 * don't mixup sclp fix with new bits for the TEID patch
 * address feedback
       * cosmetic changes, i.e. shortening identifiers
       * remove unconditional report_info
 * add DIAG 308 test

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

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

Range-diff against v1:
1:  4c833d84 ! 1:  d6e6b532 s390x: Fix sclp facility bit numbers
    @@ Commit message
     
         Fixes: 4dd649c8 ("lib: s390x: sclp: Extend feature probing")
         Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
    +    Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
    +    Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
     
      ## lib/s390x/sclp.h ##
     @@ lib/s390x/sclp.h: struct sclp_facilities {
    @@ lib/s390x/sclp.h: struct sclp_facilities {
     -#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
2:  1defccd8 ! 2:  3c03bfba s390x: Test TEID values in storage key test
    @@ lib/s390x/sclp.h: struct sclp_facilities {
      	uint64_t has_cmma : 1;
      	uint64_t has_64bscao : 1;
      	uint64_t has_esca : 1;
    +@@ lib/s390x/sclp.h: struct sclp_facilities {
    + };
    + 
    + /* bit number within a certain byte */
    ++#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
     
      ## lib/s390x/sclp.c ##
     @@ lib/s390x/sclp.c: void sclp_facilities_setup(void)
    @@ s390x/skey.c: static void test_test_protection(void)
      }
      
     +enum access {
    -+	ACC_FETCH = 2,
     +	ACC_STORE = 1,
    ++	ACC_FETCH = 2,
     +	ACC_UPDATE = 3,
     +};
     +
    @@ s390x/skey.c: static void test_test_protection(void)
     +			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");
    ++				/* no relevant information if code is 0 or 3 */
     +			}
     +		}
     +		break;
3:  58893c9c ! 3:  0b7b0e57 s390x: Test effect of storage keys on some more instructions
    @@ s390x/skey.c: static void test_store_cpu_address(void)
     + * Perform CHANNEL SUBSYSTEM CALL (CHSC)  instruction while temporarily executing
     + * with access key 1.
     + */
    -+static unsigned int channel_subsystem_call_key_1(void *communication_block)
    ++static unsigned int chsc_key_1(void *comm_block)
     +{
     +	uint32_t program_mask;
     +
     +	asm volatile (
     +		"spka	0x10\n\t"
    -+		".insn	rre,0xb25f0000,%[communication_block],0\n\t"
    ++		".insn	rre,0xb25f0000,%[comm_block],0\n\t"
     +		"spka	0\n\t"
     +		"ipm	%[program_mask]\n"
     +		: [program_mask] "=d" (program_mask)
    -+		: [communication_block] "d" (communication_block)
    ++		: [comm_block] "d" (comm_block)
     +		: "memory"
     +	);
     +	return program_mask >> 28;
     +}
     +
    -+static void init_store_channel_subsystem_characteristics(uint16_t *communication_block)
    ++static const char chsc_msg[] = "Performed store-channel-subsystem-characteristics";
    ++static void init_comm_block(uint16_t *comm_block)
     +{
    -+	memset(communication_block, 0, PAGE_SIZE);
    -+	communication_block[0] = 0x10;
    -+	communication_block[1] = 0x10;
    -+	communication_block[9] = 0;
    ++	memset(comm_block, 0, PAGE_SIZE);
    ++	/* store-channel-subsystem-characteristics command */
    ++	comm_block[0] = 0x10;
    ++	comm_block[1] = 0x10;
    ++	comm_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;
    ++	uint16_t *comm_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);
    ++	init_comm_block(comm_block);
    ++	set_storage_key(comm_block, 0x10, 0);
     +	asm volatile (
    -+		".insn	rre,0xb25f0000,%[communication_block],0\n\t"
    ++		".insn	rre,0xb25f0000,%[comm_block],0\n\t"
     +		"ipm	%[cc]\n"
     +		: [cc] "=d" (cc)
    -+		: [communication_block] "d" (communication_block)
    ++		: [comm_block] "d" (comm_block)
     +		: "memory"
     +	);
     +	cc = cc >> 28;
    -+	report(cc == 0 && communication_block[9], request_name);
    ++	report(cc == 0 && comm_block[9], chsc_msg);
     +	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);
    ++	init_comm_block(comm_block);
    ++	set_storage_key(comm_block, 0x10, 0);
    ++	cc = chsc_key_1(comm_block);
    ++	report(cc == 0 && comm_block[9], chsc_msg);
     +	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);
    ++	init_comm_block(comm_block);
    ++	set_storage_key(comm_block, 0x20, 0);
     +	expect_pgm_int();
    -+	channel_subsystem_call_key_1(communication_block);
    ++	chsc_key_1(comm_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);
    ++	init_comm_block(comm_block);
    ++	set_storage_key(comm_block, 0x28, 0);
     +	expect_pgm_int();
    -+	channel_subsystem_call_key_1(communication_block);
    ++	chsc_key_1(comm_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);
    ++	set_storage_key(comm_block, 0x20, 0);
    ++	init_comm_block(comm_block);
     +	expect_pgm_int();
    -+	channel_subsystem_call_key_1(communication_block);
    ++	chsc_key_1(comm_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);
    ++	init_comm_block(comm_block);
    ++	set_storage_key(comm_block, 0x90, 0);
    ++	cc = chsc_key_1(comm_block);
    ++	report(cc == 0 && comm_block[9], chsc_msg);
     +	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);
    ++	init_comm_block(comm_block);
    ++	set_storage_key(comm_block, 0x90, 0);
     +	expect_pgm_int();
    -+	channel_subsystem_call_key_1(communication_block);
    ++	chsc_key_1(comm_block);
     +	check_key_prot_exc(ACC_UPDATE, PROT_STORE);
     +	report_prefix_pop();
     +
     +	report_prefix_pop();
     +
    -+	set_storage_key(communication_block, 0x00, 0);
    ++	set_storage_key(comm_block, 0x00, 0);
     +	report_prefix_pop();
     +}
     +
    @@ s390x/skey.c: static void test_set_prefix(void)
     +{
     +	uint32_t program_mask;
     +
    ++/*
    ++ * gcc 12.0.1 warns if schib is < 4k.
    ++ * We need such addresses to test fetch protection override.
    ++ */
    ++#pragma GCC diagnostic push
    ++#pragma GCC diagnostic ignored "-Warray-bounds"
     +	asm volatile (
     +		"lr %%r1,%[sid]\n\t"
     +		"spka	0x10\n\t"
    @@ s390x/skey.c: static void test_set_prefix(void)
     +		  [schib] "Q" (*schib)
     +		: "%r1"
     +	);
    ++#pragma GCC diagnostic pop
     +	return program_mask >> 28;
     +}
     +
-:  -------- > 4:  7fb70993 s390x: Test effect of storage keys on diag 308

base-commit: c315f52b88b967cfb4cd58f3b4e1987378c47f3b
prerequisite-patch-id: 1c147bf31109769a454ef133daacb0786ac69a2d
-- 
2.33.1

[kvm-unit-tests PATCH v2 1/4] 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>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/sclp.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
index e48a5a3d..3488f4d2 100644
--- a/lib/s390x/sclp.h
+++ b/lib/s390x/sclp.h
@@ -134,13 +134,13 @@ 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_85_BIT_GSLS		0
+#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 v2 2/4] 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         |  4 +++
 lib/s390x/sclp.c         |  2 ++
 s390x/skey.c             | 60 ++++++++++++++++++++++++++++++++++++----
 4 files changed, 81 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 3488f4d2..853529bf 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;
@@ -134,7 +136,9 @@ struct sclp_facilities {
 };
 
 /* bit number within a certain byte */
+#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
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 7aa91d19..19fa5721 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_STORE = 1,
+	ACC_FETCH = 2,
+	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;
+
+				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");
+				/* no relevant information if code is 0 or 3 */
+			}
+		}
+		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();
 
@@ -321,7 +369,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();
 
@@ -334,7 +382,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();
 
@@ -358,7 +406,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

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

[Claudio Imbrenda]

On Tue, 17 May 2022 13:56:05 +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         |  4 +++
>  lib/s390x/sclp.c         |  2 ++
>  s390x/skey.c             | 60 ++++++++++++++++++++++++++++++++++++----
>  4 files changed, 81 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 3488f4d2..853529bf 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;
> @@ -134,7 +136,9 @@ struct sclp_facilities {
>  };
>  
>  /* bit number within a certain byte */
> +#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
> 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 7aa91d19..19fa5721 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_STORE = 1,
> +	ACC_FETCH = 2,
> +	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)

can you at least replace the hardcoded values with a macro or a const
variable?

like:

	const unsigned long esop_bit = BIT(63 - 61);

	...

		if (!(teid.val & esop_bit))

> +			report_pass("key-controlled protection");

actually, now that I think of it, aren't we expecting the bit to be
zero? should that not be like this?

report (!(teid.val & esop_bit), ...);

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

const unsigned long esop2_bits = 0x8C;	/* bits 56, 60, and 61 */
const unsigned long esop2_key_prot = BIT(63 - 60);

if ((teid.val & esop2_bits) == 0) {
	report_pass(...);

> +			report_pass("key-controlled protection");
> +			if (teid.val & BIT(63 - 60)) {

} else if ((teid.val & esop2_bits) == esop_key_prot) {

> +				int access_code = teid.fetch << 1 | teid.store;
> +
> +				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");
> +				/* no relevant information if code is 0 or 3 */

here you should check for the access-exception-fetch/store-indi-
cation facility, then you can check the access code

and at this point you should check for 0 explicitly (always pass) and 3
(always fail)

> +			}
> +		}

} else {
	/* not key protection */
	report_fail(...);
}
> +		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();
>  
> @@ -321,7 +369,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();
>  
> @@ -334,7 +382,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();
>  
> @@ -358,7 +406,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 v2 2/4] s390x: Test TEID values in storage key test

[Janis Schoetterl-Glausch]

On Tue, 2022-05-17 at 15:46 +0200, Claudio Imbrenda wrote:
> On Tue, 17 May 2022 13:56:05 +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         |  4 +++
> >  lib/s390x/sclp.c         |  2 ++
> >  s390x/skey.c             | 60 ++++++++++++++++++++++++++++++++++++----
> >  4 files changed, 81 insertions(+), 6 deletions(-)
> > 
[...]

> > +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)
> 
> can you at least replace the hardcoded values with a macro or a const
> variable?

I'll see if maybe I can come up with a nice way to extend the teid, but
I'll use a const if not.
> 
> like:
> 
> 	const unsigned long esop_bit = BIT(63 - 61);
> 
> 	...
> 
> 		if (!(teid.val & esop_bit))
> 
> > +			report_pass("key-controlled protection");
> 
> actually, now that I think of it, aren't we expecting the bit to be
> zero? should that not be like this?
> 
> report (!(teid.val & esop_bit), ...);

Indeed.
> 
> > +		break;
> > +	case SOP_ENHANCED_2:
> > +		if ((teid.val & (BIT(63 - 56) | BIT(63 - 61))) == 0) {
> 
> const unsigned long esop2_bits = 0x8C;	/* bits 56, 60, and 61 */
> const unsigned long esop2_key_prot = BIT(63 - 60);
> 
> if ((teid.val & esop2_bits) == 0) {
> 	report_pass(...);
> 
> > +			report_pass("key-controlled protection");
> > +			if (teid.val & BIT(63 - 60)) {
> 
> } else if ((teid.val & esop2_bits) == esop_key_prot) {

010 binary also means key protection, so we should pass that test here,
too. The access code checking is an additional test, IMO.
> 
> > +				int access_code = teid.fetch << 1 | teid.store;
> > +
> > +				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");
> > +				/* no relevant information if code is 0 or 3 */
> 
> here you should check for the access-exception-fetch/store-indi-
> cation facility, then you can check the access code

Oh, yes. By the way, can we get rid of magic numbers for facility
checking? Just defining an enum in lib/asm/facility.h and doing
test_facility(FCLTY_ACCESS_EXC_FETCH_STORE_INDICATION) would be an
improvement.
Well, I guess you'd end up with quite horribly long names, but at least
you have to review the values only once and not for every patch that
tests a facility. 
> 
> and at this point you should check for 0 explicitly (always pass) and 3
> (always fail)

I'm fine with passing 0, but I'm not so sure about 3.
The value is reserved, so the correct thing to do is to not attribute
*any* meaning to it. But kvm currently really should not set it either.
> > +			}
> > +		}
> 
> } else {
> 	/* not key protection */
> 	report_fail(...);
> }
> > +		break;
> > +	}
> > +	report_prefix_pop();
> > +}
> > +
[...]

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

[Claudio Imbrenda]

On Tue, 17 May 2022 17:11:37 +0200
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

> On Tue, 2022-05-17 at 15:46 +0200, Claudio Imbrenda wrote:
> > On Tue, 17 May 2022 13:56:05 +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         |  4 +++
> > >  lib/s390x/sclp.c         |  2 ++
> > >  s390x/skey.c             | 60 ++++++++++++++++++++++++++++++++++++----
> > >  4 files changed, 81 insertions(+), 6 deletions(-)
> > >   
> [...]
> 
> > > +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)  
> > 
> > can you at least replace the hardcoded values with a macro or a const
> > variable?  
> 
> I'll see if maybe I can come up with a nice way to extend the teid, but
> I'll use a const if not.
> > 
> > like:
> > 
> > 	const unsigned long esop_bit = BIT(63 - 61);
> > 
> > 	...
> > 
> > 		if (!(teid.val & esop_bit))
> >   
> > > +			report_pass("key-controlled protection");  
> > 
> > actually, now that I think of it, aren't we expecting the bit to be
> > zero? should that not be like this?
> > 
> > report (!(teid.val & esop_bit), ...);  
> 
> Indeed.
> >   
> > > +		break;
> > > +	case SOP_ENHANCED_2:
> > > +		if ((teid.val & (BIT(63 - 56) | BIT(63 - 61))) == 0) {  
> > 
> > const unsigned long esop2_bits = 0x8C;	/* bits 56, 60, and 61 */
> > const unsigned long esop2_key_prot = BIT(63 - 60);
> > 
> > if ((teid.val & esop2_bits) == 0) {
> > 	report_pass(...);
> >   
> > > +			report_pass("key-controlled protection");
> > > +			if (teid.val & BIT(63 - 60)) {  
> > 
> > } else if ((teid.val & esop2_bits) == esop_key_prot) {  
> 
> 010 binary also means key protection, so we should pass that test here,
> too. The access code checking is an additional test, IMO.

yes, my idea was to pass it only once when checking the fetch/store
indication (if any)

> >   
> > > +				int access_code = teid.fetch << 1 | teid.store;
> > > +
> > > +				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");
> > > +				/* no relevant information if code is 0 or 3 */  
> > 
> > here you should check for the access-exception-fetch/store-indi-
> > cation facility, then you can check the access code  
> 
> Oh, yes. By the way, can we get rid of magic numbers for facility
> checking? Just defining an enum in lib/asm/facility.h and doing
> test_facility(FCLTY_ACCESS_EXC_FETCH_STORE_INDICATION) would be an
> improvement.

not sure, that name looks very ugly

although that would definitely make future review easier, as you say

but that would be something for another patchseries :)

> Well, I guess you'd end up with quite horribly long names, but at least
> you have to review the values only once and not for every patch that
> tests a facility. 
> > 
> > and at this point you should check for 0 explicitly (always pass) and 3
> > (always fail)  
> 
> I'm fine with passing 0, but I'm not so sure about 3.
> The value is reserved, so the correct thing to do is to not attribute
> *any* meaning to it. But kvm currently really should not set it either.

fine

> > > +			}
> > > +		}  
> > 
> > } else {
> > 	/* not key protection */
> > 	report_fail(...);
> > }  
> > > +		break;
> > > +	}
> > > +	report_prefix_pop();
> > > +}
> > > +  
> [...]
> 

[kvm-unit-tests PATCH v2 3/4] 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        | 285 ++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |   1 +
 2 files changed, 286 insertions(+)

diff --git a/s390x/skey.c b/s390x/skey.c
index 19fa5721..60ae8158 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,115 @@ 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 chsc_key_1(void *comm_block)
+{
+	uint32_t program_mask;
+
+	asm volatile (
+		"spka	0x10\n\t"
+		".insn	rre,0xb25f0000,%[comm_block],0\n\t"
+		"spka	0\n\t"
+		"ipm	%[program_mask]\n"
+		: [program_mask] "=d" (program_mask)
+		: [comm_block] "d" (comm_block)
+		: "memory"
+	);
+	return program_mask >> 28;
+}
+
+static const char chsc_msg[] = "Performed store-channel-subsystem-characteristics";
+static void init_comm_block(uint16_t *comm_block)
+{
+	memset(comm_block, 0, PAGE_SIZE);
+	/* store-channel-subsystem-characteristics command */
+	comm_block[0] = 0x10;
+	comm_block[1] = 0x10;
+	comm_block[9] = 0;
+}
+
+static void test_channel_subsystem_call(void)
+{
+	uint16_t *comm_block = (uint16_t *)&pagebuf;
+	unsigned int cc;
+
+	report_prefix_push("CHANNEL SUBSYSTEM CALL");
+
+	report_prefix_push("zero key");
+	init_comm_block(comm_block);
+	set_storage_key(comm_block, 0x10, 0);
+	asm volatile (
+		".insn	rre,0xb25f0000,%[comm_block],0\n\t"
+		"ipm	%[cc]\n"
+		: [cc] "=d" (cc)
+		: [comm_block] "d" (comm_block)
+		: "memory"
+	);
+	cc = cc >> 28;
+	report(cc == 0 && comm_block[9], chsc_msg);
+	report_prefix_pop();
+
+	report_prefix_push("matching key");
+	init_comm_block(comm_block);
+	set_storage_key(comm_block, 0x10, 0);
+	cc = chsc_key_1(comm_block);
+	report(cc == 0 && comm_block[9], chsc_msg);
+	report_prefix_pop();
+
+	report_prefix_push("mismatching key");
+
+	report_prefix_push("no fetch protection");
+	init_comm_block(comm_block);
+	set_storage_key(comm_block, 0x20, 0);
+	expect_pgm_int();
+	chsc_key_1(comm_block);
+	check_key_prot_exc(ACC_UPDATE, PROT_STORE);
+	report_prefix_pop();
+
+	report_prefix_push("fetch protection");
+	init_comm_block(comm_block);
+	set_storage_key(comm_block, 0x28, 0);
+	expect_pgm_int();
+	chsc_key_1(comm_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(comm_block, 0x20, 0);
+	init_comm_block(comm_block);
+	expect_pgm_int();
+	chsc_key_1(comm_block);
+	check_key_prot_exc(ACC_UPDATE, PROT_STORE);
+	report_prefix_pop();
+
+	report_prefix_push("storage-protection override, override key");
+	init_comm_block(comm_block);
+	set_storage_key(comm_block, 0x90, 0);
+	cc = chsc_key_1(comm_block);
+	report(cc == 0 && comm_block[9], chsc_msg);
+	report_prefix_pop();
+
+	ctl_clear_bit(0, CTL0_STORAGE_PROTECTION_OVERRIDE);
+
+	report_prefix_push("storage-protection override disabled, override key");
+	init_comm_block(comm_block);
+	set_storage_key(comm_block, 0x90, 0);
+	expect_pgm_int();
+	chsc_key_1(comm_block);
+	check_key_prot_exc(ACC_UPDATE, PROT_STORE);
+	report_prefix_pop();
+
+	report_prefix_pop();
+
+	set_storage_key(comm_block, 0x00, 0);
+	report_prefix_pop();
+}
+
 /*
  * Perform SET PREFIX (SPX) instruction while temporarily executing
  * with access key 1.
@@ -417,6 +527,179 @@ 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;
+
+/*
+ * gcc 12.0.1 warns if schib is < 4k.
+ * We need such addresses to test fetch protection override.
+ */
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Warray-bounds"
+	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"
+	);
+#pragma GCC diagnostic pop
+	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");
@@ -431,9 +714,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 b456b288..1280ff0f 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 v2 3/4] s390x: Test effect of storage keys on some more instructions

[Claudio Imbrenda]

On Tue, 17 May 2022 13:56:06 +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        | 285 ++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg |   1 +
>  2 files changed, 286 insertions(+)
> 
> diff --git a/s390x/skey.c b/s390x/skey.c
> index 19fa5721..60ae8158 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,115 @@ 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 chsc_key_1(void *comm_block)
> +{
> +	uint32_t program_mask;
> +
> +	asm volatile (
> +		"spka	0x10\n\t"
> +		".insn	rre,0xb25f0000,%[comm_block],0\n\t"
> +		"spka	0\n\t"
> +		"ipm	%[program_mask]\n"
> +		: [program_mask] "=d" (program_mask)
> +		: [comm_block] "d" (comm_block)
> +		: "memory"
> +	);
> +	return program_mask >> 28;
> +}
> +
> +static const char chsc_msg[] = "Performed store-channel-subsystem-characteristics";
> +static void init_comm_block(uint16_t *comm_block)
> +{
> +	memset(comm_block, 0, PAGE_SIZE);
> +	/* store-channel-subsystem-characteristics command */
> +	comm_block[0] = 0x10;
> +	comm_block[1] = 0x10;
> +	comm_block[9] = 0;
> +}
> +
> +static void test_channel_subsystem_call(void)
> +{
> +	uint16_t *comm_block = (uint16_t *)&pagebuf;
> +	unsigned int cc;
> +
> +	report_prefix_push("CHANNEL SUBSYSTEM CALL");
> +
> +	report_prefix_push("zero key");
> +	init_comm_block(comm_block);
> +	set_storage_key(comm_block, 0x10, 0);
> +	asm volatile (
> +		".insn	rre,0xb25f0000,%[comm_block],0\n\t"
> +		"ipm	%[cc]\n"
> +		: [cc] "=d" (cc)
> +		: [comm_block] "d" (comm_block)
> +		: "memory"
> +	);
> +	cc = cc >> 28;
> +	report(cc == 0 && comm_block[9], chsc_msg);
> +	report_prefix_pop();
> +
> +	report_prefix_push("matching key");
> +	init_comm_block(comm_block);
> +	set_storage_key(comm_block, 0x10, 0);
> +	cc = chsc_key_1(comm_block);
> +	report(cc == 0 && comm_block[9], chsc_msg);
> +	report_prefix_pop();
> +
> +	report_prefix_push("mismatching key");
> +
> +	report_prefix_push("no fetch protection");
> +	init_comm_block(comm_block);
> +	set_storage_key(comm_block, 0x20, 0);
> +	expect_pgm_int();
> +	chsc_key_1(comm_block);
> +	check_key_prot_exc(ACC_UPDATE, PROT_STORE);

I wonder if ACC_UPDATE is really needed here? you should clearly never
get a read error, right?

> +	report_prefix_pop();
> +
> +	report_prefix_push("fetch protection");
> +	init_comm_block(comm_block);
> +	set_storage_key(comm_block, 0x28, 0);
> +	expect_pgm_int();
> +	chsc_key_1(comm_block);
> +	check_key_prot_exc(ACC_UPDATE, PROT_FETCH_STORE);

and here, I guess you would wait for a read error? or is it actually
defined as unpredictable?

(same for all ACC_UPDATE below)

> +	report_prefix_pop();
> +
> +	ctl_set_bit(0, CTL0_STORAGE_PROTECTION_OVERRIDE);
> +
> +	report_prefix_push("storage-protection override, invalid key");
> +	set_storage_key(comm_block, 0x20, 0);
> +	init_comm_block(comm_block);
> +	expect_pgm_int();
> +	chsc_key_1(comm_block);
> +	check_key_prot_exc(ACC_UPDATE, PROT_STORE);
> +	report_prefix_pop();
> +
> +	report_prefix_push("storage-protection override, override key");
> +	init_comm_block(comm_block);
> +	set_storage_key(comm_block, 0x90, 0);
> +	cc = chsc_key_1(comm_block);
> +	report(cc == 0 && comm_block[9], chsc_msg);
> +	report_prefix_pop();
> +
> +	ctl_clear_bit(0, CTL0_STORAGE_PROTECTION_OVERRIDE);
> +
> +	report_prefix_push("storage-protection override disabled, override key");
> +	init_comm_block(comm_block);
> +	set_storage_key(comm_block, 0x90, 0);
> +	expect_pgm_int();
> +	chsc_key_1(comm_block);
> +	check_key_prot_exc(ACC_UPDATE, PROT_STORE);
> +	report_prefix_pop();
> +
> +	report_prefix_pop();
> +
> +	set_storage_key(comm_block, 0x00, 0);
> +	report_prefix_pop();
> +}
> +
>  /*
>   * Perform SET PREFIX (SPX) instruction while temporarily executing
>   * with access key 1.
> @@ -417,6 +527,179 @@ 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;
> +
> +/*
> + * gcc 12.0.1 warns if schib is < 4k.
> + * We need such addresses to test fetch protection override.
> + */
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Warray-bounds"

I really dislike these pragmas

can we find a nicer way?

> +	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"
> +	);
> +#pragma GCC diagnostic pop
> +	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");
> @@ -431,9 +714,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 b456b288..1280ff0f 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 v2 3/4] s390x: Test effect of storage keys on some more instructions

[Janis Schoetterl-Glausch]

On Tue, 2022-05-17 at 15:54 +0200, Claudio Imbrenda wrote:
> On Tue, 17 May 2022 13:56:06 +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        | 285 ++++++++++++++++++++++++++++++++++++++++++++
> >  s390x/unittests.cfg |   1 +
> >  2 files changed, 286 insertions(+)
> > 
> > diff --git a/s390x/skey.c b/s390x/skey.c
> > index 19fa5721..60ae8158 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,115 @@ 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 chsc_key_1(void *comm_block)
> > +{
> > +	uint32_t program_mask;
> > +
> > +	asm volatile (
> > +		"spka	0x10\n\t"
> > +		".insn	rre,0xb25f0000,%[comm_block],0\n\t"
> > +		"spka	0\n\t"
> > +		"ipm	%[program_mask]\n"
> > +		: [program_mask] "=d" (program_mask)
> > +		: [comm_block] "d" (comm_block)
> > +		: "memory"
> > +	);
> > +	return program_mask >> 28;
> > +}
> > +
> > +static const char chsc_msg[] = "Performed store-channel-subsystem-characteristics";
> > +static void init_comm_block(uint16_t *comm_block)
> > +{
> > +	memset(comm_block, 0, PAGE_SIZE);
> > +	/* store-channel-subsystem-characteristics command */
> > +	comm_block[0] = 0x10;
> > +	comm_block[1] = 0x10;
> > +	comm_block[9] = 0;
> > +}
> > +
> > +static void test_channel_subsystem_call(void)
> > +{
> > +	uint16_t *comm_block = (uint16_t *)&pagebuf;
> > +	unsigned int cc;
> > +
> > +	report_prefix_push("CHANNEL SUBSYSTEM CALL");
> > +
> > +	report_prefix_push("zero key");
> > +	init_comm_block(comm_block);
> > +	set_storage_key(comm_block, 0x10, 0);
> > +	asm volatile (
> > +		".insn	rre,0xb25f0000,%[comm_block],0\n\t"
> > +		"ipm	%[cc]\n"
> > +		: [cc] "=d" (cc)
> > +		: [comm_block] "d" (comm_block)
> > +		: "memory"
> > +	);
> > +	cc = cc >> 28;
> > +	report(cc == 0 && comm_block[9], chsc_msg);
> > +	report_prefix_pop();
> > +
> > +	report_prefix_push("matching key");
> > +	init_comm_block(comm_block);
> > +	set_storage_key(comm_block, 0x10, 0);
> > +	cc = chsc_key_1(comm_block);
> > +	report(cc == 0 && comm_block[9], chsc_msg);
> > +	report_prefix_pop();
> > +
> > +	report_prefix_push("mismatching key");
> > +
> > +	report_prefix_push("no fetch protection");
> > +	init_comm_block(comm_block);
> > +	set_storage_key(comm_block, 0x20, 0);
> > +	expect_pgm_int();
> > +	chsc_key_1(comm_block);
> > +	check_key_prot_exc(ACC_UPDATE, PROT_STORE);
> 
> I wonder if ACC_UPDATE is really needed here? you should clearly never
> get a read error, right?

Maybe the naming isn't great, the first argument specifies the access
if it weren't for protection, not the access actually taking place.
If a read is indicated, that will cause a test failure.
You could use ACC_STORE, but that would be misleading, because it does
do a fetch.
> 
> > +	report_prefix_pop();
> > +
> > +	report_prefix_push("fetch protection");
> > +	init_comm_block(comm_block);
> > +	set_storage_key(comm_block, 0x28, 0);
> > +	expect_pgm_int();
> > +	chsc_key_1(comm_block);
> > +	check_key_prot_exc(ACC_UPDATE, PROT_FETCH_STORE);
> 
> and here, I guess you would wait for a read error? or is it actually
> defined as unpredictable?

Unpredictable, kvm and LPAR do different things IIRC, that's why I had
the report_info.
> 
> (same for all ACC_UPDATE below)

[...]
> >  
> > +/*
> > + * 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;
> > +
> > +/*
> > + * gcc 12.0.1 warns if schib is < 4k.
> > + * We need such addresses to test fetch protection override.
> > + */
> > +#pragma GCC diagnostic push
> > +#pragma GCC diagnostic ignored "-Warray-bounds"
> 
> I really dislike these pragmas
> 
> can we find a nicer way?

I'll do what ever we decide on in the other patch series.
> 
> > +	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"
> > +	);
> > +#pragma GCC diagnostic pop
> > +	return program_mask >> 28;
> > +}
> > +
[...]

Thanks for the review, also for the other patch.

[kvm-unit-tests PATCH v2 4/4] s390x: Test effect of storage keys on diag 308

[Janis Schoetterl-Glausch]

Test that key-controlled protection does not apply to diag 308.

Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
 s390x/skey.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/s390x/skey.c b/s390x/skey.c
index 60ae8158..c2d28ffd 100644
--- a/s390x/skey.c
+++ b/s390x/skey.c
@@ -285,6 +285,31 @@ static void test_store_cpu_address(void)
 	report_prefix_pop();
 }
 
+static void test_diag_308(void)
+{
+	uint16_t response;
+	uint32_t (*ipib)[1024] = (void *)pagebuf;
+
+	report_prefix_push("DIAG 308");
+	(*ipib)[0] = 0; /* Invalid length */
+	set_storage_key(ipib, 0x28, 0);
+	/* key-controlled protection does not apply */
+	asm volatile (
+		"lr	%%r2,%[ipib]\n\t"
+		"spka	0x10\n\t"
+		"diag	%%r2,%[code],0x308\n\t"
+		"spka	0\n\t"
+		"lr	%[response],%%r3\n"
+		: [response] "=d" (response)
+		: [ipib] "d" (ipib),
+		  [code] "d" (5)
+		: "%r2", "%r3"
+	);
+	report(response == 0x402, "no exception on fetch, response: invalid IPIB");
+	set_storage_key(ipib, 0x00, 0);
+	report_prefix_pop();
+}
+
 /*
  * Perform CHANNEL SUBSYSTEM CALL (CHSC)  instruction while temporarily executing
  * with access key 1.
@@ -714,6 +739,7 @@ int main(void)
 	test_chg();
 	test_test_protection();
 	test_store_cpu_address();
+	test_diag_308();
 	test_channel_subsystem_call();
 
 	setup_vm();
-- 
2.33.1

Re: [kvm-unit-tests PATCH v2 4/4] s390x: Test effect of storage keys on diag 308

[Claudio Imbrenda]

On Tue, 17 May 2022 13:56:07 +0200
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

> Test that key-controlled protection does not apply to diag 308.
> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
>  s390x/skey.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/s390x/skey.c b/s390x/skey.c
> index 60ae8158..c2d28ffd 100644
> --- a/s390x/skey.c
> +++ b/s390x/skey.c
> @@ -285,6 +285,31 @@ static void test_store_cpu_address(void)
>  	report_prefix_pop();
>  }
>  
> +static void test_diag_308(void)
> +{
> +	uint16_t response;
> +	uint32_t (*ipib)[1024] = (void *)pagebuf;

why not just uint32_t *ipib = (void *)pagebuf; ?

> +
> +	report_prefix_push("DIAG 308");
> +	(*ipib)[0] = 0; /* Invalid length */

then you can simply do:

ipib[0] = 0;

> +	set_storage_key(ipib, 0x28, 0);
> +	/* key-controlled protection does not apply */
> +	asm volatile (
> +		"lr	%%r2,%[ipib]\n\t"
> +		"spka	0x10\n\t"
> +		"diag	%%r2,%[code],0x308\n\t"
> +		"spka	0\n\t"
> +		"lr	%[response],%%r3\n"
> +		: [response] "=d" (response)
> +		: [ipib] "d" (ipib),
> +		  [code] "d" (5)
> +		: "%r2", "%r3"
> +	);
> +	report(response == 0x402, "no exception on fetch, response: invalid IPIB");
> +	set_storage_key(ipib, 0x00, 0);
> +	report_prefix_pop();
> +}
> +
>  /*
>   * Perform CHANNEL SUBSYSTEM CALL (CHSC)  instruction while temporarily executing
>   * with access key 1.
> @@ -714,6 +739,7 @@ int main(void)
>  	test_chg();
>  	test_test_protection();
>  	test_store_cpu_address();
> +	test_diag_308();
>  	test_channel_subsystem_call();
>  
>  	setup_vm();

Re: [kvm-unit-tests PATCH v2 4/4] s390x: Test effect of storage keys on diag 308

[Janis Schoetterl-Glausch]

On Tue, 2022-05-17 at 16:52 +0200, Claudio Imbrenda wrote:
> On Tue, 17 May 2022 13:56:07 +0200
> Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
> 
> > Test that key-controlled protection does not apply to diag 308.
> > 
> > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> > ---
> >  s390x/skey.c | 26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> > 
> > diff --git a/s390x/skey.c b/s390x/skey.c
> > index 60ae8158..c2d28ffd 100644
> > --- a/s390x/skey.c
> > +++ b/s390x/skey.c
> > @@ -285,6 +285,31 @@ static void test_store_cpu_address(void)
> >  	report_prefix_pop();
> >  }
> >  
> > +static void test_diag_308(void)
> > +{
> > +	uint16_t response;
> > +	uint32_t (*ipib)[1024] = (void *)pagebuf;
> 
> why not just uint32_t *ipib = (void *)pagebuf; ?
> 
> > +
> > +	report_prefix_push("DIAG 308");
> > +	(*ipib)[0] = 0; /* Invalid length */

In an intermediate version I had
memset(ipib, 0, sizeof(*ipib));
Also, if I could specify that the asm writes to a memory location that
is specified via an address in a register without displacement, it
would tell the compiler what memory would be read.
Alas that is not possible (correct me if I'm wrong), so I actually
would do
WRITE_ONCE(ipib[0], 0)
(an get rid of the pointer to array)
> 
> then you can simply do:
> 
> ipib[0] = 0;
> 
> > +	set_storage_key(ipib, 0x28, 0);
> > +	/* key-controlled protection does not apply */
> > +	asm volatile (
> > +		"lr	%%r2,%[ipib]\n\t"
> > +		"spka	0x10\n\t"
> > +		"diag	%%r2,%[code],0x308\n\t"
> > +		"spka	0\n\t"
> > +		"lr	%[response],%%r3\n"
> > +		: [response] "=d" (response)
> > +		: [ipib] "d" (ipib),
> > +		  [code] "d" (5)
> > +		: "%r2", "%r3"
> > +	);
> > +	report(response == 0x402, "no exception on fetch, response: invalid IPIB");
> > +	set_storage_key(ipib, 0x00, 0);
> > +	report_prefix_pop();
> > +}
> > +
> >  /*
> >   * Perform CHANNEL SUBSYSTEM CALL (CHSC)  instruction while temporarily executing
> >   * with access key 1.
> > @@ -714,6 +739,7 @@ int main(void)
> >  	test_chg();
> >  	test_test_protection();
> >  	test_store_cpu_address();
> > +	test_diag_308();
> >  	test_channel_subsystem_call();
> >  
> >  	setup_vm();
>