[kvm-unit-tests PATCH 0/3] s390x: Add skey removal facility test

[kvm-unit-tests PATCH 0/3] s390x: Add skey removal facility test

[Janosch Frank]

The storage key removal facility (skrf) is an anti-facility, which
makes skey related instructions result in a special operation
exception when they handle storage keys. E.g. pfmf in clearing mode
does not result in an exception, but pfmf key setting does.

The skrf is always active in protected virtualization guests and can
be emulated by KVM (expected to be upstreamed with the remaining hpage
patches).

Janosch Frank (3):
  s390x: Move pfmf to lib and make address void
  s390x: Storage key library functions now take void ptr addresses
  s390x: Add storage key removal facility

 lib/s390x/asm/mem.h |  40 ++++++++++++--
 s390x/Makefile      |   1 +
 s390x/pfmf.c        |  77 +++++++++++---------------
 s390x/skey.c        |  29 +++++-----
 s390x/skrf.c        | 130 ++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 212 insertions(+), 65 deletions(-)
 create mode 100644 s390x/skrf.c

-- 
2.17.0

[kvm-unit-tests PATCH 1/3] s390x: Move pfmf to lib and make address void

[Janosch Frank]

It's needed by other tests soon.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/asm/mem.h | 31 ++++++++++++++++++++++
 s390x/pfmf.c        | 63 ++++++++++++++-------------------------------
 2 files changed, 50 insertions(+), 44 deletions(-)

diff --git a/lib/s390x/asm/mem.h b/lib/s390x/asm/mem.h
index 75bd778..9b8fd70 100644
--- a/lib/s390x/asm/mem.h
+++ b/lib/s390x/asm/mem.h
@@ -54,4 +54,35 @@ static inline unsigned char get_storage_key(unsigned long addr)
 	asm volatile("iske %0,%1" : "=d" (skey) : "a" (addr));
 	return skey;
 }
+
+#define PFMF_FSC_4K 0
+#define PFMF_FSC_1M 1
+#define PFMF_FSC_2G 2
+
+union pfmf_r1 {
+	struct {
+		unsigned long pad0 : 32;
+		unsigned long pad1 : 12;
+		unsigned long pad_fmfi : 2;
+		unsigned long sk : 1; /* set key*/
+		unsigned long cf : 1; /* clear frame */
+		unsigned long ui : 1; /* usage indication */
+		unsigned long fsc : 3;
+		unsigned long pad2 : 1;
+		unsigned long mr : 1;
+		unsigned long mc : 1;
+		unsigned long pad3 : 1;
+		unsigned long key : 8; /* storage keys */
+	} reg;
+	unsigned long val;
+};
+
+static inline void *pfmf(unsigned long r1, void *paddr)
+{
+	register void * addr asm("1") = paddr;
+
+	asm volatile(".insn rre,0xb9af0000,%[r1],%[addr]"
+		     : [addr] "+a" (addr) : [r1] "d" (r1) : "memory");
+	return addr;
+}
 #endif
diff --git a/s390x/pfmf.c b/s390x/pfmf.c
index 9bf434a..0aa5822 100644
--- a/s390x/pfmf.c
+++ b/s390x/pfmf.c
@@ -16,60 +16,29 @@
 #include <asm/facility.h>
 #include <asm/mem.h>
 
-#define FSC_4K 0
-#define FSC_1M 1
-#define FSC_2G 2
-
-union r1 {
-	struct {
-		unsigned long pad0 : 32;
-		unsigned long pad1 : 12;
-		unsigned long pad_fmfi : 2;
-		unsigned long sk : 1; /* set key*/
-		unsigned long cf : 1; /* clear frame */
-		unsigned long ui : 1; /* usage indication */
-		unsigned long fsc : 3;
-		unsigned long pad2 : 1;
-		unsigned long mr : 1;
-		unsigned long mc : 1;
-		unsigned long pad3 : 1;
-		unsigned long key : 8; /* storage keys */
-	} reg;
-	unsigned long val;
-};
-
 static uint8_t pagebuf[PAGE_SIZE * 256] __attribute__((aligned(PAGE_SIZE * 256)));
 
-static inline unsigned long pfmf(unsigned long r1, unsigned long paddr)
-{
-	register uint64_t addr asm("1") = paddr;
-
-	asm volatile(".insn rre,0xb9af0000,%[r1],%[addr]"
-		     : [addr] "+a" (addr) : [r1] "d" (r1) : "memory");
-	return addr;
-}
-
 static void test_priv(void)
 {
 	report_prefix_push("privileged");
 	expect_pgm_int();
 	enter_pstate();
-	pfmf(0, (unsigned long) pagebuf);
+	pfmf(0, pagebuf);
 	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
 	report_prefix_pop();
 }
 
 static void test_4k_key(void)
 {
-	union r1 r1;
+	union pfmf_r1 r1;
 	union skey skey;
 
 	report_prefix_push("4K");
 	r1.val = 0;
 	r1.reg.sk = 1;
-	r1.reg.fsc = FSC_4K;
+	r1.reg.fsc = PFMF_FSC_4K;
 	r1.reg.key = 0x30;
-	pfmf(r1.val, (unsigned long) pagebuf);
+	pfmf(r1.val, pagebuf);
 	skey.val = get_storage_key((unsigned long) pagebuf);
 	skey.val &= SKEY_ACC | SKEY_FP;
 	report("set storage keys", skey.val == 0x30);
@@ -80,15 +49,18 @@ static void test_1m_key(void)
 {
 	int i;
 	bool rp = true;
-	union r1 r1;
+	union pfmf_r1 r1;
 	union skey skey;
+	void *addr = pagebuf;
 
 	report_prefix_push("1M");
 	r1.val = 0;
 	r1.reg.sk = 1;
-	r1.reg.fsc = FSC_1M;
+	r1.reg.fsc = PFMF_FSC_1M;
 	r1.reg.key = 0x30;
-	pfmf(r1.val, (unsigned long) pagebuf);
+	while (addr != pagebuf + 256 * PAGE_SIZE) {
+	       addr = pfmf(r1.val, addr);
+	}
 	for (i = 0; i < 256; i++) {
 		skey.val = get_storage_key((unsigned long) pagebuf + i * PAGE_SIZE);
 		skey.val &= SKEY_ACC | SKEY_FP;
@@ -103,15 +75,15 @@ static void test_1m_key(void)
 
 static void test_4k_clear(void)
 {
-	union r1 r1;
+	union pfmf_r1 r1;
 
 	r1.val = 0;
 	r1.reg.cf = 1;
-	r1.reg.fsc = FSC_4K;
+	r1.reg.fsc = PFMF_FSC_4K;
 
 	report_prefix_push("4K");
 	memset(pagebuf, 42, PAGE_SIZE);
-	pfmf(r1.val, (unsigned long) pagebuf);
+	pfmf(r1.val, pagebuf);
 	report("clear memory", !memcmp(pagebuf, pagebuf + PAGE_SIZE, PAGE_SIZE));
 	report_prefix_pop();
 }
@@ -119,16 +91,19 @@ static void test_4k_clear(void)
 static void test_1m_clear(void)
 {
 	int i;
-	union r1 r1;
+	union pfmf_r1 r1;
 	unsigned long sum = 0;
+	void *addr = pagebuf;
 
 	r1.val = 0;
 	r1.reg.cf = 1;
-	r1.reg.fsc = FSC_1M;
+	r1.reg.fsc = PFMF_FSC_1M;
 
 	report_prefix_push("1M");
 	memset(pagebuf, 42, PAGE_SIZE * 256);
-	pfmf(r1.val, (unsigned long) pagebuf);
+	while (addr != pagebuf + 256 * PAGE_SIZE) {
+	       addr = pfmf(r1.val, addr);
+	}
 	for (i = 0; i < PAGE_SIZE * 256; i++)
 		sum |= pagebuf[i];
 	report("clear memory", !sum);
-- 
2.17.0

[kvm-unit-tests PATCH 2/3] s390x: Storage key library functions now take void ptr addresses

[Janosch Frank]

Now all mem.h functions are consistent in how they take a memory
address. Also we have less casting in the future.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/asm/mem.h |  9 +++------
 s390x/pfmf.c        |  4 ++--
 s390x/skey.c        | 24 +++++++++++-------------
 3 files changed, 16 insertions(+), 21 deletions(-)

diff --git a/lib/s390x/asm/mem.h b/lib/s390x/asm/mem.h
index 9b8fd70..c78bfa2 100644
--- a/lib/s390x/asm/mem.h
+++ b/lib/s390x/asm/mem.h
@@ -26,9 +26,7 @@ union skey {
 	uint8_t val;
 };
 
-static inline void set_storage_key(unsigned long addr,
-				   unsigned char skey,
-				   int nq)
+static inline void set_storage_key(void *addr, unsigned char skey, int nq)
 {
 	if (nq)
 		asm volatile(".insn rrf,0xb22b0000,%0,%1,8,0"
@@ -37,8 +35,7 @@ static inline void set_storage_key(unsigned long addr,
 		asm volatile("sske %0,%1" : : "d" (skey), "a" (addr));
 }
 
-static inline unsigned long set_storage_key_mb(unsigned long addr,
-					       unsigned char skey)
+static inline void *set_storage_key_mb(void *addr, unsigned char skey)
 {
 	assert(test_facility(8));
 
@@ -47,7 +44,7 @@ static inline unsigned long set_storage_key_mb(unsigned long addr,
 	return addr;
 }
 
-static inline unsigned char get_storage_key(unsigned long addr)
+static inline unsigned char get_storage_key(void *addr)
 {
 	unsigned char skey;
 
diff --git a/s390x/pfmf.c b/s390x/pfmf.c
index 0aa5822..2840cf5 100644
--- a/s390x/pfmf.c
+++ b/s390x/pfmf.c
@@ -39,7 +39,7 @@ static void test_4k_key(void)
 	r1.reg.fsc = PFMF_FSC_4K;
 	r1.reg.key = 0x30;
 	pfmf(r1.val, pagebuf);
-	skey.val = get_storage_key((unsigned long) pagebuf);
+	skey.val = get_storage_key(pagebuf);
 	skey.val &= SKEY_ACC | SKEY_FP;
 	report("set storage keys", skey.val == 0x30);
 	report_prefix_pop();
@@ -62,7 +62,7 @@ static void test_1m_key(void)
 	       addr = pfmf(r1.val, addr);
 	}
 	for (i = 0; i < 256; i++) {
-		skey.val = get_storage_key((unsigned long) pagebuf + i * PAGE_SIZE);
+		skey.val = get_storage_key(pagebuf + i * PAGE_SIZE);
 		skey.val &= SKEY_ACC | SKEY_FP;
 		if (skey.val != 0x30) {
 			rp = false;
diff --git a/s390x/skey.c b/s390x/skey.c
index fd4fcc7..efc4eca 100644
--- a/s390x/skey.c
+++ b/s390x/skey.c
@@ -18,14 +18,12 @@
 
 
 static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
-const unsigned long page0 = (unsigned long)pagebuf;
-const unsigned long page1 = (unsigned long)(pagebuf + PAGE_SIZE);
 
 static void test_set_mb(void)
 {
 	union skey skey, ret1, ret2;
-	unsigned long addr = 0x10000 - 2 * PAGE_SIZE;
-	unsigned long end = 0x10000;
+	void *addr = (void *)0x10000 - 2 * PAGE_SIZE;
+	void *end = (void *)0x10000;
 
 	/* Multi block support came with EDAT 1 */
 	if (!test_facility(8))
@@ -46,10 +44,10 @@ static void test_chg(void)
 	union skey skey1, skey2;
 
 	skey1.val = 0x30;
-	set_storage_key(page0, skey1.val, 0);
-	skey1.val = get_storage_key(page0);
+	set_storage_key(pagebuf, skey1.val, 0);
+	skey1.val = get_storage_key(pagebuf);
 	pagebuf[0] = 3;
-	skey2.val = get_storage_key(page0);
+	skey2.val = get_storage_key(pagebuf);
 	report("chg bit test", !skey1.str.ch && skey2.str.ch);
 }
 
@@ -58,9 +56,9 @@ static void test_set(void)
 	union skey skey, ret;
 
 	skey.val = 0x30;
-	ret.val = get_storage_key(page0);
-	set_storage_key(page0, skey.val, 0);
-	ret.val = get_storage_key(page0);
+	ret.val = get_storage_key(pagebuf);
+	set_storage_key(pagebuf, skey.val, 0);
+	ret.val = get_storage_key(pagebuf);
 	/*
 	 * For all set tests we only test the ACC and FP bits. RF and
 	 * CH are set by the machine for memory references and changes
@@ -103,11 +101,11 @@ static void test_priv(void)
 	report_prefix_push("sske");
 	expect_pgm_int();
 	enter_pstate();
-	set_storage_key(page0, 0x30, 0);
+	set_storage_key(pagebuf, 0x30, 0);
 	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
 	report_prefix_pop();
 
-	skey.val = get_storage_key(page0);
+	skey.val = get_storage_key(pagebuf);
 	report("skey did not change on exception", skey.str.acc != 3);
 
 	report_prefix_push("iske");
@@ -117,7 +115,7 @@ static void test_priv(void)
 	} else {
 		expect_pgm_int();
 		enter_pstate();
-		get_storage_key(page0);
+		get_storage_key(pagebuf);
 		check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
 	}
 	report_prefix_pop();
-- 
2.17.0

[kvm-unit-tests PATCH 3/3] s390x: Add storage key removal facility

[Janosch Frank]

The storage key removal facility (stfle bit 169) makes all key related
instructions result in a special operation exception if they handle a
key.

Let's make sure that the skey and pfmf tests only run non key code
(pfmf) or not at all (skey).

Also let's test this new facility. As lots of instructions are
affected by this, only some of them are tested for now.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/Makefile |   1 +
 s390x/pfmf.c   |  10 ++++
 s390x/skey.c   |   5 ++
 s390x/skrf.c   | 130 +++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 146 insertions(+)
 create mode 100644 s390x/skrf.c

diff --git a/s390x/Makefile b/s390x/Makefile
index 76db0bb..007611e 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -14,6 +14,7 @@ tests += $(TEST_DIR)/iep.elf
 tests += $(TEST_DIR)/cpumodel.elf
 tests += $(TEST_DIR)/diag288.elf
 tests += $(TEST_DIR)/stsi.elf
+tests += $(TEST_DIR)/skrf.elf
 tests_binary = $(patsubst %.elf,%.bin,$(tests))
 
 all: directories test_cases test_cases_binary
diff --git a/s390x/pfmf.c b/s390x/pfmf.c
index 2840cf5..78b4a73 100644
--- a/s390x/pfmf.c
+++ b/s390x/pfmf.c
@@ -34,6 +34,10 @@ static void test_4k_key(void)
 	union skey skey;
 
 	report_prefix_push("4K");
+	if (test_facility(169)) {
+		report_skip("storage key removal facility is active");
+		goto out;
+	}
 	r1.val = 0;
 	r1.reg.sk = 1;
 	r1.reg.fsc = PFMF_FSC_4K;
@@ -42,6 +46,7 @@ static void test_4k_key(void)
 	skey.val = get_storage_key(pagebuf);
 	skey.val &= SKEY_ACC | SKEY_FP;
 	report("set storage keys", skey.val == 0x30);
+out:
 	report_prefix_pop();
 }
 
@@ -54,6 +59,10 @@ static void test_1m_key(void)
 	void *addr = pagebuf;
 
 	report_prefix_push("1M");
+	if (test_facility(169)) {
+		report_skip("storage key removal facility is active");
+		goto out;
+	}
 	r1.val = 0;
 	r1.reg.sk = 1;
 	r1.reg.fsc = PFMF_FSC_1M;
@@ -70,6 +79,7 @@ static void test_1m_key(void)
 		}
 	}
 	report("set storage keys", rp);
+out:
 	report_prefix_pop();
 }
 
diff --git a/s390x/skey.c b/s390x/skey.c
index efc4eca..5020e99 100644
--- a/s390x/skey.c
+++ b/s390x/skey.c
@@ -126,10 +126,15 @@ static void test_priv(void)
 int main(void)
 {
 	report_prefix_push("skey");
+	if (test_facility(169)) {
+		report_skip("storage key removal facility is active");
+		goto done;
+	}
 	test_priv();
 	test_set();
 	test_set_mb();
 	test_chg();
+done:
 	report_prefix_pop();
 	return report_summary();
 }
diff --git a/s390x/skrf.c b/s390x/skrf.c
new file mode 100644
index 0000000..8e5baea
--- /dev/null
+++ b/s390x/skrf.c
@@ -0,0 +1,130 @@
+/*
+ * Storage key removal facility tests
+ *
+ * Copyright (c) 2019 IBM Corp
+ *
+ * Authors:
+ *  Janosch Frank <frankja@linux.ibm.com>
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2.
+ */
+#include <libcflat.h>
+#include <asm/asm-offsets.h>
+#include <asm/interrupt.h>
+#include <asm/page.h>
+#include <asm/facility.h>
+#include <asm/mem.h>
+
+static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
+
+static void test_facilities(void)
+{
+	report_prefix_push("facilities");
+	report("!10", !test_facility(10));
+	report("!14", !test_facility(14));
+	report("!66", !test_facility(66));
+	report("!145", !test_facility(145));
+	report("!149", !test_facility(140));
+	report_prefix_pop();
+}
+
+static void test_skey(void)
+{
+	report_prefix_push("(i|s)ske");
+	expect_pgm_int();
+	set_storage_key(pagebuf, 0x30, 0);
+	check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
+	expect_pgm_int();
+	get_storage_key(pagebuf);
+	check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
+	report_prefix_pop();
+}
+
+static void test_pfmf(void)
+{
+	union pfmf_r1 r1;
+
+	report_prefix_push("pfmf");
+	r1.val = 0;
+	r1.reg.sk = 1;
+	r1.reg.fsc = PFMF_FSC_4K;
+	r1.reg.key = 0x30;
+	expect_pgm_int();
+	pfmf(r1.val, pagebuf);
+	check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
+	report_prefix_pop();
+}
+
+static void test_psw_key(void)
+{
+	uint64_t psw_mask = extract_psw_mask() | 0xF0000000000000UL;
+
+	report_prefix_push("psw key");
+	expect_pgm_int();
+	load_psw_mask(psw_mask);
+	check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
+	report_prefix_pop();
+}
+
+static void test_mvcos(void)
+{
+	uint64_t r3 = 64;
+	uint8_t *src = pagebuf;
+	uint8_t *dst = pagebuf + PAGE_SIZE;
+	/* K bit set, as well as keys */
+	register unsigned long oac asm("0") = 0xf002f002;
+
+	report_prefix_push("mvcos");
+	expect_pgm_int();
+	asm volatile(".machine \"z10\"\n"
+		     ".machine \"push\"\n"
+		     "mvcos	%[dst],%[src],%[len]\n"
+		     ".machine \"pop\"\n"
+		     : [dst] "+Q" (*(dst))
+		     : [src] "Q" (*(src)), [len] "d" (r3), "d" (oac)
+		     : "cc", "memory");
+	check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
+	report_prefix_pop();
+}
+
+static void test_spka(void)
+{
+	report_prefix_push("spka");
+	expect_pgm_int();
+	asm volatile("spka	0xf0(0)\n"
+		     : : : );
+	check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
+	report_prefix_pop();
+}
+
+static void test_tprot(void)
+{
+	report_prefix_push("tprot");
+	expect_pgm_int();
+	asm volatile("tprot	%[addr],0xf0(0)\n"
+		     : : [addr] "a" (pagebuf) : );
+	check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
+	report_prefix_pop();
+}
+
+int main(void)
+{
+	report_prefix_push("skrf");
+	if (!test_facility(169)) {
+		report_skip("storage key removal facility not available\n");
+		goto done;
+	}
+
+	test_facilities();
+	test_skey();
+	test_pfmf();
+	test_psw_key();
+	test_mvcos();
+	test_spka();
+	test_tprot();
+
+done:
+	report_prefix_pop();
+	return report_summary();
+}
-- 
2.17.0

Re: [kvm-unit-tests PATCH 1/3] s390x: Move pfmf to lib and make address void

[Thomas Huth]

On 27/08/2019 15.49, Janosch Frank wrote:
> It's needed by other tests soon.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/asm/mem.h | 31 ++++++++++++++++++++++
>  s390x/pfmf.c        | 63 ++++++++++++++-------------------------------
>  2 files changed, 50 insertions(+), 44 deletions(-)
[...]
> @@ -80,15 +49,18 @@ static void test_1m_key(void)
>  {
>  	int i;
>  	bool rp = true;
> -	union r1 r1;
> +	union pfmf_r1 r1;
>  	union skey skey;
> +	void *addr = pagebuf;
>  
>  	report_prefix_push("1M");
>  	r1.val = 0;
>  	r1.reg.sk = 1;
> -	r1.reg.fsc = FSC_1M;
> +	r1.reg.fsc = PFMF_FSC_1M;
>  	r1.reg.key = 0x30;
> -	pfmf(r1.val, (unsigned long) pagebuf);
> +	while (addr != pagebuf + 256 * PAGE_SIZE) {
> +	       addr = pfmf(r1.val, addr);
> +	}

Why this change? If PFMF gets interrupted, the PSW should still point to
the PFMF instruction, so no need to loop here ... or do I miss something?

(See PoP, chapter "Execution of Interruptible Instructions")

>  	for (i = 0; i < 256; i++) {
>  		skey.val = get_storage_key((unsigned long) pagebuf + i * PAGE_SIZE);
>  		skey.val &= SKEY_ACC | SKEY_FP;
> @@ -103,15 +75,15 @@ static void test_1m_key(void)
>  
>  static void test_4k_clear(void)
>  {
> -	union r1 r1;
> +	union pfmf_r1 r1;
>  
>  	r1.val = 0;
>  	r1.reg.cf = 1;
> -	r1.reg.fsc = FSC_4K;
> +	r1.reg.fsc = PFMF_FSC_4K;
>  
>  	report_prefix_push("4K");
>  	memset(pagebuf, 42, PAGE_SIZE);
> -	pfmf(r1.val, (unsigned long) pagebuf);
> +	pfmf(r1.val, pagebuf);
>  	report("clear memory", !memcmp(pagebuf, pagebuf + PAGE_SIZE, PAGE_SIZE));
>  	report_prefix_pop();
>  }
> @@ -119,16 +91,19 @@ static void test_4k_clear(void)
>  static void test_1m_clear(void)
>  {
>  	int i;
> -	union r1 r1;
> +	union pfmf_r1 r1;
>  	unsigned long sum = 0;
> +	void *addr = pagebuf;
>  
>  	r1.val = 0;
>  	r1.reg.cf = 1;
> -	r1.reg.fsc = FSC_1M;
> +	r1.reg.fsc = PFMF_FSC_1M;
>  
>  	report_prefix_push("1M");
>  	memset(pagebuf, 42, PAGE_SIZE * 256);
> -	pfmf(r1.val, (unsigned long) pagebuf);
> +	while (addr != pagebuf + 256 * PAGE_SIZE) {
> +	       addr = pfmf(r1.val, addr);
> +	}

dito.

 Thomas

Re: [kvm-unit-tests PATCH 2/3] s390x: Storage key library functions now take void ptr addresses

[Thomas Huth]

On 27/08/2019 15.49, Janosch Frank wrote:
> Now all mem.h functions are consistent in how they take a memory
> address. Also we have less casting in the future.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/asm/mem.h |  9 +++------
>  s390x/pfmf.c        |  4 ++--
>  s390x/skey.c        | 24 +++++++++++-------------
>  3 files changed, 16 insertions(+), 21 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [kvm-unit-tests PATCH 3/3] s390x: Add storage key removal facility

[Thomas Huth]

On 27/08/2019 15.49, Janosch Frank wrote:
> The storage key removal facility (stfle bit 169) makes all key related
> instructions result in a special operation exception if they handle a
> key.
> 
> Let's make sure that the skey and pfmf tests only run non key code
> (pfmf) or not at all (skey).
> 
> Also let's test this new facility. As lots of instructions are
> affected by this, only some of them are tested for now.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/Makefile |   1 +
>  s390x/pfmf.c   |  10 ++++
>  s390x/skey.c   |   5 ++
>  s390x/skrf.c   | 130 +++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 146 insertions(+)
>  create mode 100644 s390x/skrf.c
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 76db0bb..007611e 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -14,6 +14,7 @@ tests += $(TEST_DIR)/iep.elf
>  tests += $(TEST_DIR)/cpumodel.elf
>  tests += $(TEST_DIR)/diag288.elf
>  tests += $(TEST_DIR)/stsi.elf
> +tests += $(TEST_DIR)/skrf.elf
>  tests_binary = $(patsubst %.elf,%.bin,$(tests))
>  
>  all: directories test_cases test_cases_binary
> diff --git a/s390x/pfmf.c b/s390x/pfmf.c
> index 2840cf5..78b4a73 100644
> --- a/s390x/pfmf.c
> +++ b/s390x/pfmf.c
> @@ -34,6 +34,10 @@ static void test_4k_key(void)
>  	union skey skey;
>  
>  	report_prefix_push("4K");
> +	if (test_facility(169)) {
> +		report_skip("storage key removal facility is active");
> +		goto out;
> +	}
>  	r1.val = 0;
>  	r1.reg.sk = 1;
>  	r1.reg.fsc = PFMF_FSC_4K;
> @@ -42,6 +46,7 @@ static void test_4k_key(void)
>  	skey.val = get_storage_key(pagebuf);
>  	skey.val &= SKEY_ACC | SKEY_FP;
>  	report("set storage keys", skey.val == 0x30);
> +out:
>  	report_prefix_pop();
>  }
>  
> @@ -54,6 +59,10 @@ static void test_1m_key(void)
>  	void *addr = pagebuf;
>  
>  	report_prefix_push("1M");
> +	if (test_facility(169)) {
> +		report_skip("storage key removal facility is active");
> +		goto out;
> +	}
>  	r1.val = 0;
>  	r1.reg.sk = 1;
>  	r1.reg.fsc = PFMF_FSC_1M;
> @@ -70,6 +79,7 @@ static void test_1m_key(void)
>  		}
>  	}
>  	report("set storage keys", rp);
> +out:
>  	report_prefix_pop();
>  }
>  
> diff --git a/s390x/skey.c b/s390x/skey.c
> index efc4eca..5020e99 100644
> --- a/s390x/skey.c
> +++ b/s390x/skey.c
> @@ -126,10 +126,15 @@ static void test_priv(void)
>  int main(void)
>  {
>  	report_prefix_push("skey");
> +	if (test_facility(169)) {
> +		report_skip("storage key removal facility is active");
> +		goto done;
> +	}
>  	test_priv();
>  	test_set();
>  	test_set_mb();
>  	test_chg();
> +done:
>  	report_prefix_pop();
>  	return report_summary();
>  }
> diff --git a/s390x/skrf.c b/s390x/skrf.c
> new file mode 100644
> index 0000000..8e5baea
> --- /dev/null
> +++ b/s390x/skrf.c
> @@ -0,0 +1,130 @@
> +/*
> + * Storage key removal facility tests
> + *
> + * Copyright (c) 2019 IBM Corp
> + *
> + * Authors:
> + *  Janosch Frank <frankja@linux.ibm.com>
> + *
> + * This code is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2.
> + */
> +#include <libcflat.h>
> +#include <asm/asm-offsets.h>
> +#include <asm/interrupt.h>
> +#include <asm/page.h>
> +#include <asm/facility.h>
> +#include <asm/mem.h>
> +
> +static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
> +
> +static void test_facilities(void)
> +{
> +	report_prefix_push("facilities");
> +	report("!10", !test_facility(10));
> +	report("!14", !test_facility(14));
> +	report("!66", !test_facility(66));
> +	report("!145", !test_facility(145));
> +	report("!149", !test_facility(140));
> +	report_prefix_pop();
> +}
> +
> +static void test_skey(void)
> +{
> +	report_prefix_push("(i|s)ske");
> +	expect_pgm_int();
> +	set_storage_key(pagebuf, 0x30, 0);
> +	check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
> +	expect_pgm_int();
> +	get_storage_key(pagebuf);
> +	check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
> +	report_prefix_pop();

Wouldn't it be better to have distinct prefixes for the two tests?

> +}
> +
> +static void test_pfmf(void)
> +{
> +	union pfmf_r1 r1;
> +
> +	report_prefix_push("pfmf");
> +	r1.val = 0;
> +	r1.reg.sk = 1;
> +	r1.reg.fsc = PFMF_FSC_4K;
> +	r1.reg.key = 0x30;
> +	expect_pgm_int();
> +	pfmf(r1.val, pagebuf);
> +	check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
> +	report_prefix_pop();
> +}
> +
> +static void test_psw_key(void)
> +{
> +	uint64_t psw_mask = extract_psw_mask() | 0xF0000000000000UL;
> +
> +	report_prefix_push("psw key");
> +	expect_pgm_int();
> +	load_psw_mask(psw_mask);
> +	check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
> +	report_prefix_pop();
> +}
> +
> +static void test_mvcos(void)
> +{
> +	uint64_t r3 = 64;
> +	uint8_t *src = pagebuf;
> +	uint8_t *dst = pagebuf + PAGE_SIZE;
> +	/* K bit set, as well as keys */
> +	register unsigned long oac asm("0") = 0xf002f002;
> +
> +	report_prefix_push("mvcos");
> +	expect_pgm_int();
> +	asm volatile(".machine \"z10\"\n"
> +		     ".machine \"push\"\n"

Shouldn't that be the other way round? first push the current one, then
set the new one?

Anyway, I've now also checked this patch in the CI:

diff a/s390x/Makefile b/s390x/Makefile
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -25,7 +25,7 @@ CFLAGS += -std=gnu99
 CFLAGS += -ffreestanding
 CFLAGS += -I $(SRCDIR)/lib -I $(SRCDIR)/lib/s390x -I lib
 CFLAGS += -O2
-CFLAGS += -march=z900
+CFLAGS += -march=z10
 CFLAGS += -fno-delete-null-pointer-checks
 LDFLAGS += -nostdlib -Wl,--build-id=none

... and it also seems to work fine with the TCG there:

https://gitlab.com/huth/kvm-unit-tests/-/jobs/281450598

So I think you can simply change it in the Makefile instead.

 Thomas

> +		     "mvcos	%[dst],%[src],%[len]\n"
> +		     ".machine \"pop\"\n"
> +		     : [dst] "+Q" (*(dst))
> +		     : [src] "Q" (*(src)), [len] "d" (r3), "d" (oac)
> +		     : "cc", "memory");
> +	check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
> +	report_prefix_pop();
> +}

Re: [kvm-unit-tests PATCH 3/3] s390x: Add storage key removal facility

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 3220 bytes --]

On 8/27/19 7:58 PM, Thomas Huth wrote:
> On 27/08/2019 15.49, Janosch Frank wrote:
>> The storage key removal facility (stfle bit 169) makes all key related
>> instructions result in a special operation exception if they handle a
>> key.
>>
>> Let's make sure that the skey and pfmf tests only run non key code
>> (pfmf) or not at all (skey).
>>
>> Also let's test this new facility. As lots of instructions are
>> affected by this, only some of them are tested for now.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---

>> +static void test_skey(void)
>> +{
>> +	report_prefix_push("(i|s)ske");
>> +	expect_pgm_int();
>> +	set_storage_key(pagebuf, 0x30, 0);
>> +	check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
>> +	expect_pgm_int();
>> +	get_storage_key(pagebuf);
>> +	check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
>> +	report_prefix_pop();
> 
> Wouldn't it be better to have distinct prefixes for the two tests?

Will do

> 
>> +}
>> +
>> +static void test_pfmf(void)
>> +{
>> +	union pfmf_r1 r1;
>> +
>> +	report_prefix_push("pfmf");
>> +	r1.val = 0;
>> +	r1.reg.sk = 1;
>> +	r1.reg.fsc = PFMF_FSC_4K;
>> +	r1.reg.key = 0x30;
>> +	expect_pgm_int();
>> +	pfmf(r1.val, pagebuf);
>> +	check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
>> +	report_prefix_pop();
>> +}
>> +
>> +static void test_psw_key(void)
>> +{
>> +	uint64_t psw_mask = extract_psw_mask() | 0xF0000000000000UL;
>> +
>> +	report_prefix_push("psw key");
>> +	expect_pgm_int();
>> +	load_psw_mask(psw_mask);
>> +	check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
>> +	report_prefix_pop();
>> +}
>> +
>> +static void test_mvcos(void)
>> +{
>> +	uint64_t r3 = 64;
>> +	uint8_t *src = pagebuf;
>> +	uint8_t *dst = pagebuf + PAGE_SIZE;
>> +	/* K bit set, as well as keys */
>> +	register unsigned long oac asm("0") = 0xf002f002;
>> +
>> +	report_prefix_push("mvcos");
>> +	expect_pgm_int();
>> +	asm volatile(".machine \"z10\"\n"
>> +		     ".machine \"push\"\n"
> 
> Shouldn't that be the other way round? first push the current one, then
> set the new one?

Yes, I interpreted the documentation in the wrong way and it was a PPC
documentation anyway :)

> 
> Anyway, I've now also checked this patch in the CI:
> 
> diff a/s390x/Makefile b/s390x/Makefile
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -25,7 +25,7 @@ CFLAGS += -std=gnu99
>  CFLAGS += -ffreestanding
>  CFLAGS += -I $(SRCDIR)/lib -I $(SRCDIR)/lib/s390x -I lib
>  CFLAGS += -O2
> -CFLAGS += -march=z900
> +CFLAGS += -march=z10
>  CFLAGS += -fno-delete-null-pointer-checks
>  LDFLAGS += -nostdlib -Wl,--build-id=none
> 
> ... and it also seems to work fine with the TCG there:
> 
> https://gitlab.com/huth/kvm-unit-tests/-/jobs/281450598
> 
> So I think you can simply change it in the Makefile instead.

z10 or directly something higher?

> 
>  Thomas
> 
>> +		     "mvcos	%[dst],%[src],%[len]\n"
>> +		     ".machine \"pop\"\n"
>> +		     : [dst] "+Q" (*(dst))
>> +		     : [src] "Q" (*(src)), [len] "d" (r3), "d" (oac)
>> +		     : "cc", "memory");
>> +	check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
>> +	report_prefix_pop();
>> +}



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [kvm-unit-tests PATCH 3/3] s390x: Add storage key removal facility

[Thomas Huth]

[-- Attachment #1.1: Type: text/plain, Size: 883 bytes --]

On 28/08/2019 08.26, Janosch Frank wrote:
> On 8/27/19 7:58 PM, Thomas Huth wrote:
[...]
>> Anyway, I've now also checked this patch in the CI:
>>
>> diff a/s390x/Makefile b/s390x/Makefile
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -25,7 +25,7 @@ CFLAGS += -std=gnu99
>>  CFLAGS += -ffreestanding
>>  CFLAGS += -I $(SRCDIR)/lib -I $(SRCDIR)/lib/s390x -I lib
>>  CFLAGS += -O2
>> -CFLAGS += -march=z900
>> +CFLAGS += -march=z10
>>  CFLAGS += -fno-delete-null-pointer-checks
>>  LDFLAGS += -nostdlib -Wl,--build-id=none
>>
>> ... and it also seems to work fine with the TCG there:
>>
>> https://gitlab.com/huth/kvm-unit-tests/-/jobs/281450598
>>
>> So I think you can simply change it in the Makefile instead.
> 
> z10 or directly something higher?

zEC12 seems to work, too:

https://gitlab.com/huth/kvm-unit-tests/-/jobs/281833366

 Thomas


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]