[kvm-unit-tests PATCH v1 0/3] s390x: test storage keys during migration

[kvm-unit-tests PATCH v1 0/3] s390x: test storage keys during migration

[Nico Boehr]

Add a test which changes storage keys while VM is being migrated.

This series bases on Claudio's new PSW macros ("[PATCH v3 0/2] lib:
s390x: add PSW and PSW_WITH_CUR_MASK macros").

Nico Boehr (3):
  s390x: add library for skey-related functions
  lib: s390x: skey: add seed value for storage keys
  s390x: add storage key test during migration

 lib/s390x/skey.c              |  94 +++++++++++++++++++++++++++++++
 lib/s390x/skey.h              |  42 ++++++++++++++
 s390x/Makefile                |   2 +
 s390x/migration-during-skey.c | 103 ++++++++++++++++++++++++++++++++++
 s390x/migration-skey.c        |  44 ++-------------
 s390x/unittests.cfg           |   5 ++
 6 files changed, 252 insertions(+), 38 deletions(-)
 create mode 100644 lib/s390x/skey.c
 create mode 100644 lib/s390x/skey.h
 create mode 100644 s390x/migration-during-skey.c

-- 
2.36.1

[kvm-unit-tests PATCH v1 1/3] s390x: add library for skey-related functions

[Nico Boehr]

Upcoming changes will add a test which is very similar to the existing
skey migration test. To reduce code duplication, move the common
functions to a library which can be re-used by both tests.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 lib/s390x/skey.c       | 92 ++++++++++++++++++++++++++++++++++++++++++
 lib/s390x/skey.h       | 32 +++++++++++++++
 s390x/Makefile         |  1 +
 s390x/migration-skey.c | 44 +++-----------------
 4 files changed, 131 insertions(+), 38 deletions(-)
 create mode 100644 lib/s390x/skey.c
 create mode 100644 lib/s390x/skey.h

diff --git a/lib/s390x/skey.c b/lib/s390x/skey.c
new file mode 100644
index 000000000000..100f0949a244
--- /dev/null
+++ b/lib/s390x/skey.c
@@ -0,0 +1,92 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Storage key migration test library
+ *
+ * Copyright IBM Corp. 2022
+ *
+ * Authors:
+ *  Nico Boehr <nrb@linux.ibm.com>
+ */
+
+#include <libcflat.h>
+#include <asm/facility.h>
+#include <asm/mem.h>
+#include <skey.h>
+
+/*
+ * Set storage keys on pagebuf.
+ * pagebuf must point to page_count consecutive pages.
+ */
+void skey_set_keys(uint8_t *pagebuf, unsigned long page_count)
+{
+	unsigned char key_to_set;
+	unsigned long i;
+
+	for (i = 0; i < page_count; i++) {
+		/*
+		 * Storage keys are 7 bit, lowest bit is always returned as zero
+		 * by iske.
+		 * This loop will set all 7 bits which means we set fetch
+		 * protection as well as reference and change indication for
+		 * some keys.
+		 */
+		key_to_set = i * 2;
+		set_storage_key(pagebuf + i * PAGE_SIZE, key_to_set, 1);
+	}
+}
+
+/*
+ * Verify storage keys on pagebuf.
+ * Storage keys must have been set by skey_set_keys on pagebuf before.
+ *
+ * If storage keys match the expected result, will return a skey_verify_result
+ * with verify_failed false. All other fields are then invalid.
+ * If there is a mismatch, returned struct will have verify_failed true and will
+ * be filled with the details on the first mismatch encountered.
+ */
+struct skey_verify_result skey_verify_keys(uint8_t *pagebuf, unsigned long page_count)
+{
+	union skey expected_key, actual_key;
+	struct skey_verify_result result = {
+		.verify_failed = true
+	};
+	uint8_t *cur_page;
+	unsigned long i;
+
+	for (i = 0; i < page_count; i++) {
+		cur_page = pagebuf + i * PAGE_SIZE;
+		actual_key.val = get_storage_key(cur_page);
+		expected_key.val = i * 2;
+
+		/*
+		 * The PoP neither gives a guarantee that the reference bit is
+		 * accurate nor that it won't be cleared by hardware. Hence we
+		 * don't rely on it and just clear the bits to avoid compare
+		 * errors.
+		 */
+		actual_key.str.rf = 0;
+		expected_key.str.rf = 0;
+
+		if (actual_key.val != expected_key.val) {
+			result.expected_key.val = expected_key.val;
+			result.actual_key.val = actual_key.val;
+			result.page_mismatch_idx = i;
+			result.page_mismatch_addr = (unsigned long)cur_page;
+			return result;
+		}
+	}
+
+	result.verify_failed = false;
+	return result;
+}
+
+void skey_report_verify(struct skey_verify_result * const result)
+{
+	if (result->verify_failed)
+		report_fail("page skey mismatch: first page idx = %lu, addr = 0x%lx, "
+			"expected_key = 0x%x, actual_key = 0x%x",
+			result->page_mismatch_idx, result->page_mismatch_addr,
+			result->expected_key.val, result->actual_key.val);
+	else
+		report_pass("skeys match");
+}
diff --git a/lib/s390x/skey.h b/lib/s390x/skey.h
new file mode 100644
index 000000000000..a0f8caa1270b
--- /dev/null
+++ b/lib/s390x/skey.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Storage key migration test library
+ *
+ * Copyright IBM Corp. 2022
+ *
+ * Authors:
+ *  Nico Boehr <nrb@linux.ibm.com>
+ */
+#ifndef S390X_SKEY_H
+#define S390X_SKEY_H
+
+#include <libcflat.h>
+#include <asm/facility.h>
+#include <asm/page.h>
+#include <asm/mem.h>
+
+struct skey_verify_result {
+	bool verify_failed;
+	union skey expected_key;
+	union skey actual_key;
+	unsigned long page_mismatch_idx;
+	unsigned long page_mismatch_addr;
+};
+
+void skey_set_keys(uint8_t *pagebuf, unsigned long page_count);
+
+struct skey_verify_result skey_verify_keys(uint8_t *pagebuf, unsigned long page_count);
+
+void skey_report_verify(struct skey_verify_result * const result);
+
+#endif /* S390X_SKEY_H */
diff --git a/s390x/Makefile b/s390x/Makefile
index bf1504f9d58c..d097b7071dfb 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -99,6 +99,7 @@ cflatobjs += lib/s390x/malloc_io.o
 cflatobjs += lib/s390x/uv.o
 cflatobjs += lib/s390x/sie.o
 cflatobjs += lib/s390x/fault.o
+cflatobjs += lib/s390x/skey.o
 
 OBJDIRS += lib/s390x
 
diff --git a/s390x/migration-skey.c b/s390x/migration-skey.c
index b7bd82581abe..fed6fc1ed0f8 100644
--- a/s390x/migration-skey.c
+++ b/s390x/migration-skey.c
@@ -10,55 +10,23 @@
 
 #include <libcflat.h>
 #include <asm/facility.h>
-#include <asm/page.h>
-#include <asm/mem.h>
-#include <asm/interrupt.h>
 #include <hardware.h>
+#include <skey.h>
 
 #define NUM_PAGES 128
-static uint8_t pagebuf[NUM_PAGES][PAGE_SIZE] __attribute__((aligned(PAGE_SIZE)));
+static uint8_t pagebuf[NUM_PAGES * PAGE_SIZE] __attribute__((aligned(PAGE_SIZE)));
 
 static void test_migration(void)
 {
-	union skey expected_key, actual_key;
-	int i, key_to_set, key_mismatches = 0;
+	struct skey_verify_result result;
 
-	for (i = 0; i < NUM_PAGES; i++) {
-		/*
-		 * Storage keys are 7 bit, lowest bit is always returned as zero
-		 * by iske.
-		 * This loop will set all 7 bits which means we set fetch
-		 * protection as well as reference and change indication for
-		 * some keys.
-		 */
-		key_to_set = i * 2;
-		set_storage_key(pagebuf[i], key_to_set, 1);
-	}
+	skey_set_keys(pagebuf, NUM_PAGES);
 
 	puts("Please migrate me, then press return\n");
 	(void)getchar();
 
-	for (i = 0; i < NUM_PAGES; i++) {
-		actual_key.val = get_storage_key(pagebuf[i]);
-		expected_key.val = i * 2;
-
-		/*
-		 * The PoP neither gives a guarantee that the reference bit is
-		 * accurate nor that it won't be cleared by hardware. Hence we
-		 * don't rely on it and just clear the bits to avoid compare
-		 * errors.
-		 */
-		actual_key.str.rf = 0;
-		expected_key.str.rf = 0;
-
-		/* don't log anything when key matches to avoid spamming the log */
-		if (actual_key.val != expected_key.val) {
-			key_mismatches++;
-			report_fail("page %d expected_key=0x%x actual_key=0x%x", i, expected_key.val, actual_key.val);
-		}
-	}
-
-	report(!key_mismatches, "skeys after migration match");
+	result = skey_verify_keys(pagebuf, NUM_PAGES);
+	skey_report_verify(&result);
 }
 
 int main(void)
-- 
2.36.1

[kvm-unit-tests PATCH v1 2/3] lib: s390x: skey: add seed value for storage keys

[Nico Boehr]

Upcoming changes will change storage keys in a loop. To make sure each
iteration of the loops sets different keys, add variants of the storage
key library functions which allow to specify a seed.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 lib/s390x/skey.c | 12 +++++++-----
 lib/s390x/skey.h | 14 ++++++++++++--
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/lib/s390x/skey.c b/lib/s390x/skey.c
index 100f0949a244..4ab0828ee98f 100644
--- a/lib/s390x/skey.c
+++ b/lib/s390x/skey.c
@@ -14,10 +14,11 @@
 #include <skey.h>
 
 /*
- * Set storage keys on pagebuf.
+ * Set storage keys on pagebuf with a seed for the storage keys.
  * pagebuf must point to page_count consecutive pages.
+ * Only the lower seven bits of the seed are considered.
  */
-void skey_set_keys(uint8_t *pagebuf, unsigned long page_count)
+void skey_set_keys_with_seed(uint8_t *pagebuf, unsigned long page_count, unsigned char seed)
 {
 	unsigned char key_to_set;
 	unsigned long i;
@@ -30,7 +31,7 @@ void skey_set_keys(uint8_t *pagebuf, unsigned long page_count)
 		 * protection as well as reference and change indication for
 		 * some keys.
 		 */
-		key_to_set = i * 2;
+		key_to_set = (i ^ seed) * 2;
 		set_storage_key(pagebuf + i * PAGE_SIZE, key_to_set, 1);
 	}
 }
@@ -38,13 +39,14 @@ void skey_set_keys(uint8_t *pagebuf, unsigned long page_count)
 /*
  * Verify storage keys on pagebuf.
  * Storage keys must have been set by skey_set_keys on pagebuf before.
+ * skey_set_keys must have been called with the same seed value.
  *
  * If storage keys match the expected result, will return a skey_verify_result
  * with verify_failed false. All other fields are then invalid.
  * If there is a mismatch, returned struct will have verify_failed true and will
  * be filled with the details on the first mismatch encountered.
  */
-struct skey_verify_result skey_verify_keys(uint8_t *pagebuf, unsigned long page_count)
+struct skey_verify_result skey_verify_keys_with_seed(uint8_t *pagebuf, unsigned long page_count, unsigned char seed)
 {
 	union skey expected_key, actual_key;
 	struct skey_verify_result result = {
@@ -56,7 +58,7 @@ struct skey_verify_result skey_verify_keys(uint8_t *pagebuf, unsigned long page_
 	for (i = 0; i < page_count; i++) {
 		cur_page = pagebuf + i * PAGE_SIZE;
 		actual_key.val = get_storage_key(cur_page);
-		expected_key.val = i * 2;
+		expected_key.val = (i ^ seed) * 2;
 
 		/*
 		 * The PoP neither gives a guarantee that the reference bit is
diff --git a/lib/s390x/skey.h b/lib/s390x/skey.h
index a0f8caa1270b..bba1c131276d 100644
--- a/lib/s390x/skey.h
+++ b/lib/s390x/skey.h
@@ -23,9 +23,19 @@ struct skey_verify_result {
 	unsigned long page_mismatch_addr;
 };
 
-void skey_set_keys(uint8_t *pagebuf, unsigned long page_count);
+void skey_set_keys_with_seed(uint8_t *pagebuf, unsigned long page_count, unsigned char seed);
 
-struct skey_verify_result skey_verify_keys(uint8_t *pagebuf, unsigned long page_count);
+static inline void skey_set_keys(uint8_t *pagebuf, unsigned long page_count)
+{
+	skey_set_keys_with_seed(pagebuf, page_count, 0);
+}
+
+struct skey_verify_result skey_verify_keys_with_seed(uint8_t *pagebuf, unsigned long page_count, unsigned char seed);
+
+static inline struct skey_verify_result skey_verify_keys(uint8_t *pagebuf, unsigned long page_count)
+{
+	return skey_verify_keys_with_seed(pagebuf, page_count, 0);
+}
 
 void skey_report_verify(struct skey_verify_result * const result);
 
-- 
2.36.1

[kvm-unit-tests PATCH v1 3/3] s390x: add storage key test during migration

[Nico Boehr]

Add a test which modifies storage keys while migration is in progress.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 s390x/Makefile                |   1 +
 s390x/migration-during-skey.c | 103 ++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg           |   5 ++
 3 files changed, 109 insertions(+)
 create mode 100644 s390x/migration-during-skey.c

diff --git a/s390x/Makefile b/s390x/Makefile
index d097b7071dfb..d9ba9b5fc392 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -39,6 +39,7 @@ tests += $(TEST_DIR)/panic-loop-extint.elf
 tests += $(TEST_DIR)/panic-loop-pgm.elf
 tests += $(TEST_DIR)/migration-sck.elf
 tests += $(TEST_DIR)/exittime.elf
+tests += $(TEST_DIR)/migration-during-skey.elf
 
 pv-tests += $(TEST_DIR)/pv-diags.elf
 
diff --git a/s390x/migration-during-skey.c b/s390x/migration-during-skey.c
new file mode 100644
index 000000000000..bd0e6feb02bc
--- /dev/null
+++ b/s390x/migration-during-skey.c
@@ -0,0 +1,103 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Perform storage key operations during migration
+ *
+ * Copyright IBM Corp. 2022
+ *
+ * Authors:
+ *  Nico Boehr <nrb@linux.ibm.com>
+ */
+
+#include <libcflat.h>
+#include <asm/facility.h>
+#include <asm/barrier.h>
+#include <hardware.h>
+#include <smp.h>
+#include <skey.h>
+
+#define NUM_PAGES 128
+static uint8_t pagebuf[NUM_PAGES * PAGE_SIZE] __attribute__((aligned(PAGE_SIZE)));
+
+static unsigned int thread_iters;
+static bool thread_should_exit;
+static bool thread_exited;
+static struct skey_verify_result result;
+
+static void test_skeys_during_migration(void)
+{
+	while (!READ_ONCE(thread_should_exit)) {
+		skey_set_keys_with_seed(pagebuf, NUM_PAGES, thread_iters);
+
+		result = skey_verify_keys_with_seed(pagebuf, NUM_PAGES, thread_iters);
+
+		/*
+		 * Always increment even if the verify fails. This ensures primary CPU knows where
+		 * we left off and can do an additional verify round after migration finished.
+		 */
+		thread_iters++;
+
+		if (result.verify_failed)
+			break;
+	}
+
+	WRITE_ONCE(thread_exited, 1);
+}
+
+static void migrate_once(void)
+{
+	static bool migrated;
+
+	if (migrated)
+		return;
+
+	migrated = true;
+	puts("Please migrate me, then press return\n");
+	(void)getchar();
+}
+
+int main(void)
+{
+	report_prefix_push("migration-skey");
+	if (test_facility(169)) {
+		report_skip("storage key removal facility is active");
+		goto error;
+	}
+
+	if (smp_query_num_cpus() == 1) {
+		report_skip("need at least 2 cpus for this test");
+		goto error;
+	}
+
+	smp_cpu_setup(1, PSW_WITH_CUR_MASK(test_skeys_during_migration));
+
+	migrate_once();
+
+	WRITE_ONCE(thread_should_exit, 1);
+
+	while (!thread_exited)
+		mb();
+
+	report_info("thread completed %u iterations", thread_iters);
+
+	report_prefix_push("during migration");
+	skey_report_verify(&result);
+	report_prefix_pop();
+
+	/*
+	 * Verification of skeys occurs on the thread. We don't know if we
+	 * were still migrating during the verification.
+	 * To be sure, make another verification round after the migration
+	 * finished to catch skeys which might not have been migrated
+	 * correctly.
+	 */
+	report_prefix_push("after migration");
+	assert(thread_iters > 0);
+	result = skey_verify_keys_with_seed(pagebuf, NUM_PAGES, thread_iters - 1);
+	skey_report_verify(&result);
+	report_prefix_pop();
+
+error:
+	migrate_once();
+	report_prefix_pop();
+	return report_summary();
+}
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index 3caf81eda396..855c352929a4 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -208,3 +208,8 @@ groups = migration
 [exittime]
 file = exittime.elf
 smp = 2
+
+[migration-during-skey]
+file = migration-during-skey.elf
+smp = 2
+groups = migration
-- 
2.36.1

Re: [kvm-unit-tests PATCH v1 1/3] s390x: add library for skey-related functions

[Claudio Imbrenda]

On Thu,  1 Dec 2022 09:46:40 +0100
Nico Boehr <nrb@linux.ibm.com> wrote:

> Upcoming changes will add a test which is very similar to the existing
> skey migration test. To reduce code duplication, move the common
> functions to a library which can be re-used by both tests.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>

a few nits, otherwise looks pretty straightforward

> ---
>  lib/s390x/skey.c       | 92 ++++++++++++++++++++++++++++++++++++++++++
>  lib/s390x/skey.h       | 32 +++++++++++++++
>  s390x/Makefile         |  1 +
>  s390x/migration-skey.c | 44 +++-----------------
>  4 files changed, 131 insertions(+), 38 deletions(-)
>  create mode 100644 lib/s390x/skey.c
>  create mode 100644 lib/s390x/skey.h
> 
> diff --git a/lib/s390x/skey.c b/lib/s390x/skey.c
> new file mode 100644
> index 000000000000..100f0949a244
> --- /dev/null
> +++ b/lib/s390x/skey.c
> @@ -0,0 +1,92 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Storage key migration test library
> + *
> + * Copyright IBM Corp. 2022
> + *
> + * Authors:
> + *  Nico Boehr <nrb@linux.ibm.com>
> + */
> +
> +#include <libcflat.h>
> +#include <asm/facility.h>
> +#include <asm/mem.h>
> +#include <skey.h>
> +
> +/*
> + * Set storage keys on pagebuf.

surely you should explain better what the function does (e.g. how are
you setting the keys and why)

> + * pagebuf must point to page_count consecutive pages.
> + */
> +void skey_set_keys(uint8_t *pagebuf, unsigned long page_count)

this name does not make clear what the function is doing. at first one
would think that it sets the same key for all pages.

maybe something like set_storage_keys_test_pattern or
skey_set_test_pattern or something like that

> +{
> +	unsigned char key_to_set;
> +	unsigned long i;
> +
> +	for (i = 0; i < page_count; i++) {
> +		/*
> +		 * Storage keys are 7 bit, lowest bit is always returned as zero
> +		 * by iske.
> +		 * This loop will set all 7 bits which means we set fetch
> +		 * protection as well as reference and change indication for
> +		 * some keys.
> +		 */
> +		key_to_set = i * 2;
> +		set_storage_key(pagebuf + i * PAGE_SIZE, key_to_set, 1);

why not just i * 2 instead of using key_to_set ?

> +	}
> +}
> +
> +/*
> + * Verify storage keys on pagebuf.
> + * Storage keys must have been set by skey_set_keys on pagebuf before.
> + *
> + * If storage keys match the expected result, will return a skey_verify_result
> + * with verify_failed false. All other fields are then invalid.
> + * If there is a mismatch, returned struct will have verify_failed true and will
> + * be filled with the details on the first mismatch encountered.
> + */
> +struct skey_verify_result skey_verify_keys(uint8_t *pagebuf, unsigned long page_count)

and here then adjust the function name accordingly

> +{
> +	union skey expected_key, actual_key;
> +	struct skey_verify_result result = {
> +		.verify_failed = true
> +	};
> +	uint8_t *cur_page;
> +	unsigned long i;
> +
> +	for (i = 0; i < page_count; i++) {
> +		cur_page = pagebuf + i * PAGE_SIZE;
> +		actual_key.val = get_storage_key(cur_page);
> +		expected_key.val = i * 2;
> +
> +		/*
> +		 * The PoP neither gives a guarantee that the reference bit is
> +		 * accurate nor that it won't be cleared by hardware. Hence we
> +		 * don't rely on it and just clear the bits to avoid compare
> +		 * errors.
> +		 */
> +		actual_key.str.rf = 0;
> +		expected_key.str.rf = 0;
> +
> +		if (actual_key.val != expected_key.val) {
> +			result.expected_key.val = expected_key.val;
> +			result.actual_key.val = actual_key.val;
> +			result.page_mismatch_idx = i;
> +			result.page_mismatch_addr = (unsigned long)cur_page;
> +			return result;
> +		}
> +	}
> +
> +	result.verify_failed = false;
> +	return result;
> +}
> +
> +void skey_report_verify(struct skey_verify_result * const result)
> +{
> +	if (result->verify_failed)
> +		report_fail("page skey mismatch: first page idx = %lu, addr = 0x%lx, "
> +			"expected_key = 0x%x, actual_key = 0x%x",
> +			result->page_mismatch_idx, result->page_mismatch_addr,
> +			result->expected_key.val, result->actual_key.val);
> +	else
> +		report_pass("skeys match");
> +}
> diff --git a/lib/s390x/skey.h b/lib/s390x/skey.h
> new file mode 100644
> index 000000000000..a0f8caa1270b
> --- /dev/null
> +++ b/lib/s390x/skey.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Storage key migration test library
> + *
> + * Copyright IBM Corp. 2022
> + *
> + * Authors:
> + *  Nico Boehr <nrb@linux.ibm.com>
> + */
> +#ifndef S390X_SKEY_H
> +#define S390X_SKEY_H
> +
> +#include <libcflat.h>
> +#include <asm/facility.h>
> +#include <asm/page.h>
> +#include <asm/mem.h>
> +
> +struct skey_verify_result {
> +	bool verify_failed;
> +	union skey expected_key;
> +	union skey actual_key;
> +	unsigned long page_mismatch_idx;
> +	unsigned long page_mismatch_addr;
> +};
> +
> +void skey_set_keys(uint8_t *pagebuf, unsigned long page_count);
> +
> +struct skey_verify_result skey_verify_keys(uint8_t *pagebuf, unsigned long page_count);
> +
> +void skey_report_verify(struct skey_verify_result * const result);
> +
> +#endif /* S390X_SKEY_H */
> diff --git a/s390x/Makefile b/s390x/Makefile
> index bf1504f9d58c..d097b7071dfb 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -99,6 +99,7 @@ cflatobjs += lib/s390x/malloc_io.o
>  cflatobjs += lib/s390x/uv.o
>  cflatobjs += lib/s390x/sie.o
>  cflatobjs += lib/s390x/fault.o
> +cflatobjs += lib/s390x/skey.o
>  
>  OBJDIRS += lib/s390x
>  
> diff --git a/s390x/migration-skey.c b/s390x/migration-skey.c
> index b7bd82581abe..fed6fc1ed0f8 100644
> --- a/s390x/migration-skey.c
> +++ b/s390x/migration-skey.c
> @@ -10,55 +10,23 @@
>  
>  #include <libcflat.h>
>  #include <asm/facility.h>
> -#include <asm/page.h>
> -#include <asm/mem.h>
> -#include <asm/interrupt.h>
>  #include <hardware.h>
> +#include <skey.h>
>  
>  #define NUM_PAGES 128
> -static uint8_t pagebuf[NUM_PAGES][PAGE_SIZE] __attribute__((aligned(PAGE_SIZE)));
> +static uint8_t pagebuf[NUM_PAGES * PAGE_SIZE] __attribute__((aligned(PAGE_SIZE)));
>  
>  static void test_migration(void)
>  {
> -	union skey expected_key, actual_key;
> -	int i, key_to_set, key_mismatches = 0;
> +	struct skey_verify_result result;
>  
> -	for (i = 0; i < NUM_PAGES; i++) {
> -		/*
> -		 * Storage keys are 7 bit, lowest bit is always returned as zero
> -		 * by iske.
> -		 * This loop will set all 7 bits which means we set fetch
> -		 * protection as well as reference and change indication for
> -		 * some keys.
> -		 */
> -		key_to_set = i * 2;

ah I see, you have simply moved this code :)

> -		set_storage_key(pagebuf[i], key_to_set, 1);
> -	}
> +	skey_set_keys(pagebuf, NUM_PAGES);
>  
>  	puts("Please migrate me, then press return\n");
>  	(void)getchar();
>  
> -	for (i = 0; i < NUM_PAGES; i++) {
> -		actual_key.val = get_storage_key(pagebuf[i]);
> -		expected_key.val = i * 2;
> -
> -		/*
> -		 * The PoP neither gives a guarantee that the reference bit is
> -		 * accurate nor that it won't be cleared by hardware. Hence we
> -		 * don't rely on it and just clear the bits to avoid compare
> -		 * errors.
> -		 */
> -		actual_key.str.rf = 0;
> -		expected_key.str.rf = 0;
> -
> -		/* don't log anything when key matches to avoid spamming the log */
> -		if (actual_key.val != expected_key.val) {
> -			key_mismatches++;
> -			report_fail("page %d expected_key=0x%x actual_key=0x%x", i, expected_key.val, actual_key.val);
> -		}
> -	}
> -
> -	report(!key_mismatches, "skeys after migration match");
> +	result = skey_verify_keys(pagebuf, NUM_PAGES);
> +	skey_report_verify(&result);
>  }
>  
>  int main(void)

Re: [kvm-unit-tests PATCH v1 2/3] lib: s390x: skey: add seed value for storage keys

[Claudio Imbrenda]

On Thu,  1 Dec 2022 09:46:41 +0100
Nico Boehr <nrb@linux.ibm.com> wrote:

> Upcoming changes will change storage keys in a loop. To make sure each
> iteration of the loops sets different keys, add variants of the storage
> key library functions which allow to specify a seed.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>

I wonder if you can simply merge this patch with the previous one

> ---
>  lib/s390x/skey.c | 12 +++++++-----
>  lib/s390x/skey.h | 14 ++++++++++++--
>  2 files changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/s390x/skey.c b/lib/s390x/skey.c
> index 100f0949a244..4ab0828ee98f 100644
> --- a/lib/s390x/skey.c
> +++ b/lib/s390x/skey.c
> @@ -14,10 +14,11 @@
>  #include <skey.h>
>  
>  /*
> - * Set storage keys on pagebuf.
> + * Set storage keys on pagebuf with a seed for the storage keys.
>   * pagebuf must point to page_count consecutive pages.
> + * Only the lower seven bits of the seed are considered.
>   */
> -void skey_set_keys(uint8_t *pagebuf, unsigned long page_count)
> +void skey_set_keys_with_seed(uint8_t *pagebuf, unsigned long page_count, unsigned char seed)
>  {
>  	unsigned char key_to_set;
>  	unsigned long i;
> @@ -30,7 +31,7 @@ void skey_set_keys(uint8_t *pagebuf, unsigned long page_count)
>  		 * protection as well as reference and change indication for
>  		 * some keys.
>  		 */
> -		key_to_set = i * 2;
> +		key_to_set = (i ^ seed) * 2;
>  		set_storage_key(pagebuf + i * PAGE_SIZE, key_to_set, 1);
>  	}
>  }
> @@ -38,13 +39,14 @@ void skey_set_keys(uint8_t *pagebuf, unsigned long page_count)
>  /*
>   * Verify storage keys on pagebuf.
>   * Storage keys must have been set by skey_set_keys on pagebuf before.
> + * skey_set_keys must have been called with the same seed value.
>   *
>   * If storage keys match the expected result, will return a skey_verify_result
>   * with verify_failed false. All other fields are then invalid.
>   * If there is a mismatch, returned struct will have verify_failed true and will
>   * be filled with the details on the first mismatch encountered.
>   */
> -struct skey_verify_result skey_verify_keys(uint8_t *pagebuf, unsigned long page_count)
> +struct skey_verify_result skey_verify_keys_with_seed(uint8_t *pagebuf, unsigned long page_count, unsigned char seed)
>  {
>  	union skey expected_key, actual_key;
>  	struct skey_verify_result result = {
> @@ -56,7 +58,7 @@ struct skey_verify_result skey_verify_keys(uint8_t *pagebuf, unsigned long page_
>  	for (i = 0; i < page_count; i++) {
>  		cur_page = pagebuf + i * PAGE_SIZE;
>  		actual_key.val = get_storage_key(cur_page);
> -		expected_key.val = i * 2;
> +		expected_key.val = (i ^ seed) * 2;
>  
>  		/*
>  		 * The PoP neither gives a guarantee that the reference bit is
> diff --git a/lib/s390x/skey.h b/lib/s390x/skey.h
> index a0f8caa1270b..bba1c131276d 100644
> --- a/lib/s390x/skey.h
> +++ b/lib/s390x/skey.h
> @@ -23,9 +23,19 @@ struct skey_verify_result {
>  	unsigned long page_mismatch_addr;
>  };
>  
> -void skey_set_keys(uint8_t *pagebuf, unsigned long page_count);
> +void skey_set_keys_with_seed(uint8_t *pagebuf, unsigned long page_count, unsigned char seed);
>  
> -struct skey_verify_result skey_verify_keys(uint8_t *pagebuf, unsigned long page_count);
> +static inline void skey_set_keys(uint8_t *pagebuf, unsigned long page_count)
> +{
> +	skey_set_keys_with_seed(pagebuf, page_count, 0);
> +}
> +
> +struct skey_verify_result skey_verify_keys_with_seed(uint8_t *pagebuf, unsigned long page_count, unsigned char seed);
> +
> +static inline struct skey_verify_result skey_verify_keys(uint8_t *pagebuf, unsigned long page_count)
> +{
> +	return skey_verify_keys_with_seed(pagebuf, page_count, 0);
> +}
>  
>  void skey_report_verify(struct skey_verify_result * const result);
>  

Re: [kvm-unit-tests PATCH v1 2/3] lib: s390x: skey: add seed value for storage keys

[Nico Boehr]

Quoting Claudio Imbrenda (2022-12-01 14:27:58)
> On Thu,  1 Dec 2022 09:46:41 +0100
> Nico Boehr <nrb@linux.ibm.com> wrote:
> 
> > Upcoming changes will change storage keys in a loop. To make sure each
> > iteration of the loops sets different keys, add variants of the storage
> > key library functions which allow to specify a seed.
> > 
> > Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> 
> I wonder if you can simply merge this patch with the previous one

Yes, I thought about that too, but I was not sure whether people will like this
approach, so I kept it as a seperate patch so I can undo it easily and it stands
out a bit :)

I will merge with the previous commit if nobody complains.

Re: [kvm-unit-tests PATCH v1 1/3] s390x: add library for skey-related functions

[Nico Boehr]

Quoting Claudio Imbrenda (2022-12-01 14:16:50)
> On Thu,  1 Dec 2022 09:46:40 +0100
> Nico Boehr <nrb@linux.ibm.com> wrote:
[...]
> > diff --git a/lib/s390x/skey.c b/lib/s390x/skey.c
> > new file mode 100644
> > index 000000000000..100f0949a244
[...]
> > +/*
> > + * Set storage keys on pagebuf.
> 
> surely you should explain better what the function does (e.g. how are
> you setting the keys and why)

Well there is the comment below which explains why the * 2 is needed, so what
about this paragraph (after merging the commits as discussed before):

    * Each page's storage key is generated by taking the page's index in pagebuf,
    * XOR-ing that with the given seed and then multipling the result with two.

(But really that's also easy to see from the code below, so I am not sure if
this really adds value.)

> > + * pagebuf must point to page_count consecutive pages.
> > + */
> > +void skey_set_keys(uint8_t *pagebuf, unsigned long page_count)
> 
> this name does not make clear what the function is doing. at first one
> would think that it sets the same key for all pages.
> 
> maybe something like set_storage_keys_test_pattern or
> skey_set_test_pattern or something like that

Oh that's a nice suggestion, thanks.

> 
> > +{
> > +     unsigned char key_to_set;
> > +     unsigned long i;
> > +
> > +     for (i = 0; i < page_count; i++) {
> > +             /*
> > +              * Storage keys are 7 bit, lowest bit is always returned as zero
> > +              * by iske.
> > +              * This loop will set all 7 bits which means we set fetch
> > +              * protection as well as reference and change indication for
> > +              * some keys.
> > +              */
> > +             key_to_set = i * 2;
> > +             set_storage_key(pagebuf + i * PAGE_SIZE, key_to_set, 1);
> 
> why not just i * 2 instead of using key_to_set ?

Well you answered that yourself :)

In patch 2, the key_to_set expression becomes a bit more complex, so the extra
variable makes sense to me.

Re: [kvm-unit-tests PATCH v1 1/3] s390x: add library for skey-related functions

[Claudio Imbrenda]

On Thu, 01 Dec 2022 16:55:42 +0100
Nico Boehr <nrb@linux.ibm.com> wrote:

> Quoting Claudio Imbrenda (2022-12-01 14:16:50)
> > On Thu,  1 Dec 2022 09:46:40 +0100
> > Nico Boehr <nrb@linux.ibm.com> wrote:  
> [...]
> > > diff --git a/lib/s390x/skey.c b/lib/s390x/skey.c
> > > new file mode 100644
> > > index 000000000000..100f0949a244  
> [...]
> > > +/*
> > > + * Set storage keys on pagebuf.  

... according to a pattern

> > 
> > surely you should explain better what the function does (e.g. how are
> > you setting the keys and why)  
> 
> Well there is the comment below which explains why the * 2 is needed, so what
> about this paragraph (after merging the commits as discussed before):
> 
>     * Each page's storage key is generated by taking the page's index in pagebuf,
>     * XOR-ing that with the given seed and then multipling the result with two.

looks good

> 
> (But really that's also easy to see from the code below, so I am not sure if
> this really adds value.)

if you want to add documentation, do it properly, otherwise there is no
point in having documentation at all :)

> 
> > > + * pagebuf must point to page_count consecutive pages.
> > > + */
> > > +void skey_set_keys(uint8_t *pagebuf, unsigned long page_count)  
> > 
> > this name does not make clear what the function is doing. at first one
> > would think that it sets the same key for all pages.
> > 
> > maybe something like set_storage_keys_test_pattern or
> > skey_set_test_pattern or something like that  
> 
> Oh that's a nice suggestion, thanks.
> 
> >   
> > > +{
> > > +     unsigned char key_to_set;
> > > +     unsigned long i;
> > > +
> > > +     for (i = 0; i < page_count; i++) {
> > > +             /*
> > > +              * Storage keys are 7 bit, lowest bit is always returned as zero
> > > +              * by iske.
> > > +              * This loop will set all 7 bits which means we set fetch
> > > +              * protection as well as reference and change indication for
> > > +              * some keys.
> > > +              */
> > > +             key_to_set = i * 2;
> > > +             set_storage_key(pagebuf + i * PAGE_SIZE, key_to_set, 1);  
> > 
> > why not just i * 2 instead of using key_to_set ?  
> 
> Well you answered that yourself :)
> 
> In patch 2, the key_to_set expression becomes a bit more complex, so the extra
> variable makes sense to me.

fair enough

Re: [kvm-unit-tests PATCH v1 1/3] s390x: add library for skey-related functions

[Janosch Frank]

On 12/1/22 09:46, Nico Boehr wrote:
> Upcoming changes will add a test which is very similar to the existing
> skey migration test. To reduce code duplication, move the common
> functions to a library which can be re-used by both tests.
> 

NACK

We're not putting test specific code into the library.

> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>   lib/s390x/skey.c       | 92 ++++++++++++++++++++++++++++++++++++++++++
>   lib/s390x/skey.h       | 32 +++++++++++++++
>   s390x/Makefile         |  1 +
>   s390x/migration-skey.c | 44 +++-----------------
>   4 files changed, 131 insertions(+), 38 deletions(-)
>   create mode 100644 lib/s390x/skey.c
>   create mode 100644 lib/s390x/skey.h
> 
> diff --git a/lib/s390x/skey.c b/lib/s390x/skey.c
> new file mode 100644
> index 000000000000..100f0949a244
> --- /dev/null
> +++ b/lib/s390x/skey.c
> @@ -0,0 +1,92 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Storage key migration test library
> + *
> + * Copyright IBM Corp. 2022
> + *
> + * Authors:
> + *  Nico Boehr <nrb@linux.ibm.com>
> + */
> +
> +#include <libcflat.h>
> +#include <asm/facility.h>
> +#include <asm/mem.h>
> +#include <skey.h>
> +
> +/*
> + * Set storage keys on pagebuf.
> + * pagebuf must point to page_count consecutive pages.
> + */
> +void skey_set_keys(uint8_t *pagebuf, unsigned long page_count)
> +{
> +	unsigned char key_to_set;
> +	unsigned long i;
> +
> +	for (i = 0; i < page_count; i++) {
> +		/*
> +		 * Storage keys are 7 bit, lowest bit is always returned as zero
> +		 * by iske.
> +		 * This loop will set all 7 bits which means we set fetch
> +		 * protection as well as reference and change indication for
> +		 * some keys.
> +		 */
> +		key_to_set = i * 2;
> +		set_storage_key(pagebuf + i * PAGE_SIZE, key_to_set, 1);
> +	}
> +}
> +
> +/*
> + * Verify storage keys on pagebuf.
> + * Storage keys must have been set by skey_set_keys on pagebuf before.
> + *
> + * If storage keys match the expected result, will return a skey_verify_result
> + * with verify_failed false. All other fields are then invalid.
> + * If there is a mismatch, returned struct will have verify_failed true and will
> + * be filled with the details on the first mismatch encountered.
> + */
> +struct skey_verify_result skey_verify_keys(uint8_t *pagebuf, unsigned long page_count)
> +{
> +	union skey expected_key, actual_key;
> +	struct skey_verify_result result = {
> +		.verify_failed = true
> +	};
> +	uint8_t *cur_page;
> +	unsigned long i;
> +
> +	for (i = 0; i < page_count; i++) {
> +		cur_page = pagebuf + i * PAGE_SIZE;
> +		actual_key.val = get_storage_key(cur_page);
> +		expected_key.val = i * 2;
> +
> +		/*
> +		 * The PoP neither gives a guarantee that the reference bit is
> +		 * accurate nor that it won't be cleared by hardware. Hence we
> +		 * don't rely on it and just clear the bits to avoid compare
> +		 * errors.
> +		 */
> +		actual_key.str.rf = 0;
> +		expected_key.str.rf = 0;
> +
> +		if (actual_key.val != expected_key.val) {
> +			result.expected_key.val = expected_key.val;
> +			result.actual_key.val = actual_key.val;
> +			result.page_mismatch_idx = i;
> +			result.page_mismatch_addr = (unsigned long)cur_page;
> +			return result;
> +		}
> +	}
> +
> +	result.verify_failed = false;
> +	return result;
> +}
> +
> +void skey_report_verify(struct skey_verify_result * const result)
> +{
> +	if (result->verify_failed)
> +		report_fail("page skey mismatch: first page idx = %lu, addr = 0x%lx, "
> +			"expected_key = 0x%x, actual_key = 0x%x",
> +			result->page_mismatch_idx, result->page_mismatch_addr,
> +			result->expected_key.val, result->actual_key.val);
> +	else
> +		report_pass("skeys match");
> +}
> diff --git a/lib/s390x/skey.h b/lib/s390x/skey.h
> new file mode 100644
> index 000000000000..a0f8caa1270b
> --- /dev/null
> +++ b/lib/s390x/skey.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Storage key migration test library
> + *
> + * Copyright IBM Corp. 2022
> + *
> + * Authors:
> + *  Nico Boehr <nrb@linux.ibm.com>
> + */
> +#ifndef S390X_SKEY_H
> +#define S390X_SKEY_H
> +
> +#include <libcflat.h>
> +#include <asm/facility.h>
> +#include <asm/page.h>
> +#include <asm/mem.h>
> +
> +struct skey_verify_result {
> +	bool verify_failed;
> +	union skey expected_key;
> +	union skey actual_key;
> +	unsigned long page_mismatch_idx;
> +	unsigned long page_mismatch_addr;
> +};
> +
> +void skey_set_keys(uint8_t *pagebuf, unsigned long page_count);
> +
> +struct skey_verify_result skey_verify_keys(uint8_t *pagebuf, unsigned long page_count);
> +
> +void skey_report_verify(struct skey_verify_result * const result);
> +
> +#endif /* S390X_SKEY_H */
> diff --git a/s390x/Makefile b/s390x/Makefile
> index bf1504f9d58c..d097b7071dfb 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -99,6 +99,7 @@ cflatobjs += lib/s390x/malloc_io.o
>   cflatobjs += lib/s390x/uv.o
>   cflatobjs += lib/s390x/sie.o
>   cflatobjs += lib/s390x/fault.o
> +cflatobjs += lib/s390x/skey.o
>   
>   OBJDIRS += lib/s390x
>   
> diff --git a/s390x/migration-skey.c b/s390x/migration-skey.c
> index b7bd82581abe..fed6fc1ed0f8 100644
> --- a/s390x/migration-skey.c
> +++ b/s390x/migration-skey.c
> @@ -10,55 +10,23 @@
>   
>   #include <libcflat.h>
>   #include <asm/facility.h>
> -#include <asm/page.h>
> -#include <asm/mem.h>
> -#include <asm/interrupt.h>
>   #include <hardware.h>
> +#include <skey.h>
>   
>   #define NUM_PAGES 128
> -static uint8_t pagebuf[NUM_PAGES][PAGE_SIZE] __attribute__((aligned(PAGE_SIZE)));
> +static uint8_t pagebuf[NUM_PAGES * PAGE_SIZE] __attribute__((aligned(PAGE_SIZE)));
>   
>   static void test_migration(void)
>   {
> -	union skey expected_key, actual_key;
> -	int i, key_to_set, key_mismatches = 0;
> +	struct skey_verify_result result;
>   
> -	for (i = 0; i < NUM_PAGES; i++) {
> -		/*
> -		 * Storage keys are 7 bit, lowest bit is always returned as zero
> -		 * by iske.
> -		 * This loop will set all 7 bits which means we set fetch
> -		 * protection as well as reference and change indication for
> -		 * some keys.
> -		 */
> -		key_to_set = i * 2;
> -		set_storage_key(pagebuf[i], key_to_set, 1);
> -	}
> +	skey_set_keys(pagebuf, NUM_PAGES);
>   
>   	puts("Please migrate me, then press return\n");
>   	(void)getchar();
>   
> -	for (i = 0; i < NUM_PAGES; i++) {
> -		actual_key.val = get_storage_key(pagebuf[i]);
> -		expected_key.val = i * 2;
> -
> -		/*
> -		 * The PoP neither gives a guarantee that the reference bit is
> -		 * accurate nor that it won't be cleared by hardware. Hence we
> -		 * don't rely on it and just clear the bits to avoid compare
> -		 * errors.
> -		 */
> -		actual_key.str.rf = 0;
> -		expected_key.str.rf = 0;
> -
> -		/* don't log anything when key matches to avoid spamming the log */
> -		if (actual_key.val != expected_key.val) {
> -			key_mismatches++;
> -			report_fail("page %d expected_key=0x%x actual_key=0x%x", i, expected_key.val, actual_key.val);
> -		}
> -	}
> -
> -	report(!key_mismatches, "skeys after migration match");
> +	result = skey_verify_keys(pagebuf, NUM_PAGES);
> +	skey_report_verify(&result);
>   }
>   
>   int main(void)

Re: [kvm-unit-tests PATCH v1 1/3] s390x: add library for skey-related functions

[Thomas Huth]

On 02/12/2022 10.03, Janosch Frank wrote:
> On 12/1/22 09:46, Nico Boehr wrote:
>> Upcoming changes will add a test which is very similar to the existing
>> skey migration test. To reduce code duplication, move the common
>> functions to a library which can be re-used by both tests.
>>
> 
> NACK
> 
> We're not putting test specific code into the library.

Do we need a new file (in the third patch) for the new test at all, or could 
the new test simply be added to s390x/migration-skey.c instead?

  Thomas


>> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
>> ---
>>   lib/s390x/skey.c       | 92 ++++++++++++++++++++++++++++++++++++++++++
>>   lib/s390x/skey.h       | 32 +++++++++++++++
>>   s390x/Makefile         |  1 +
>>   s390x/migration-skey.c | 44 +++-----------------
>>   4 files changed, 131 insertions(+), 38 deletions(-)
>>   create mode 100644 lib/s390x/skey.c
>>   create mode 100644 lib/s390x/skey.h
>>
>> diff --git a/lib/s390x/skey.c b/lib/s390x/skey.c
>> new file mode 100644
>> index 000000000000..100f0949a244
>> --- /dev/null
>> +++ b/lib/s390x/skey.c
>> @@ -0,0 +1,92 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Storage key migration test library
>> + *
>> + * Copyright IBM Corp. 2022
>> + *
>> + * Authors:
>> + *  Nico Boehr <nrb@linux.ibm.com>
>> + */
>> +
>> +#include <libcflat.h>
>> +#include <asm/facility.h>
>> +#include <asm/mem.h>
>> +#include <skey.h>
>> +
>> +/*
>> + * Set storage keys on pagebuf.
>> + * pagebuf must point to page_count consecutive pages.
>> + */
>> +void skey_set_keys(uint8_t *pagebuf, unsigned long page_count)
>> +{
>> +    unsigned char key_to_set;
>> +    unsigned long i;
>> +
>> +    for (i = 0; i < page_count; i++) {
>> +        /*
>> +         * Storage keys are 7 bit, lowest bit is always returned as zero
>> +         * by iske.
>> +         * This loop will set all 7 bits which means we set fetch
>> +         * protection as well as reference and change indication for
>> +         * some keys.
>> +         */
>> +        key_to_set = i * 2;
>> +        set_storage_key(pagebuf + i * PAGE_SIZE, key_to_set, 1);
>> +    }
>> +}
>> +
>> +/*
>> + * Verify storage keys on pagebuf.
>> + * Storage keys must have been set by skey_set_keys on pagebuf before.
>> + *
>> + * If storage keys match the expected result, will return a 
>> skey_verify_result
>> + * with verify_failed false. All other fields are then invalid.
>> + * If there is a mismatch, returned struct will have verify_failed true 
>> and will
>> + * be filled with the details on the first mismatch encountered.
>> + */
>> +struct skey_verify_result skey_verify_keys(uint8_t *pagebuf, unsigned 
>> long page_count)
>> +{
>> +    union skey expected_key, actual_key;
>> +    struct skey_verify_result result = {
>> +        .verify_failed = true
>> +    };
>> +    uint8_t *cur_page;
>> +    unsigned long i;
>> +
>> +    for (i = 0; i < page_count; i++) {
>> +        cur_page = pagebuf + i * PAGE_SIZE;
>> +        actual_key.val = get_storage_key(cur_page);
>> +        expected_key.val = i * 2;
>> +
>> +        /*
>> +         * The PoP neither gives a guarantee that the reference bit is
>> +         * accurate nor that it won't be cleared by hardware. Hence we
>> +         * don't rely on it and just clear the bits to avoid compare
>> +         * errors.
>> +         */
>> +        actual_key.str.rf = 0;
>> +        expected_key.str.rf = 0;
>> +
>> +        if (actual_key.val != expected_key.val) {
>> +            result.expected_key.val = expected_key.val;
>> +            result.actual_key.val = actual_key.val;
>> +            result.page_mismatch_idx = i;
>> +            result.page_mismatch_addr = (unsigned long)cur_page;
>> +            return result;
>> +        }
>> +    }
>> +
>> +    result.verify_failed = false;
>> +    return result;
>> +}
>> +
>> +void skey_report_verify(struct skey_verify_result * const result)
>> +{
>> +    if (result->verify_failed)
>> +        report_fail("page skey mismatch: first page idx = %lu, addr = 
>> 0x%lx, "
>> +            "expected_key = 0x%x, actual_key = 0x%x",
>> +            result->page_mismatch_idx, result->page_mismatch_addr,
>> +            result->expected_key.val, result->actual_key.val);
>> +    else
>> +        report_pass("skeys match");
>> +}
>> diff --git a/lib/s390x/skey.h b/lib/s390x/skey.h
>> new file mode 100644
>> index 000000000000..a0f8caa1270b
>> --- /dev/null
>> +++ b/lib/s390x/skey.h
>> @@ -0,0 +1,32 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Storage key migration test library
>> + *
>> + * Copyright IBM Corp. 2022
>> + *
>> + * Authors:
>> + *  Nico Boehr <nrb@linux.ibm.com>
>> + */
>> +#ifndef S390X_SKEY_H
>> +#define S390X_SKEY_H
>> +
>> +#include <libcflat.h>
>> +#include <asm/facility.h>
>> +#include <asm/page.h>
>> +#include <asm/mem.h>
>> +
>> +struct skey_verify_result {
>> +    bool verify_failed;
>> +    union skey expected_key;
>> +    union skey actual_key;
>> +    unsigned long page_mismatch_idx;
>> +    unsigned long page_mismatch_addr;
>> +};
>> +
>> +void skey_set_keys(uint8_t *pagebuf, unsigned long page_count);
>> +
>> +struct skey_verify_result skey_verify_keys(uint8_t *pagebuf, unsigned 
>> long page_count);
>> +
>> +void skey_report_verify(struct skey_verify_result * const result);
>> +
>> +#endif /* S390X_SKEY_H */
>> diff --git a/s390x/Makefile b/s390x/Makefile
>> index bf1504f9d58c..d097b7071dfb 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -99,6 +99,7 @@ cflatobjs += lib/s390x/malloc_io.o
>>   cflatobjs += lib/s390x/uv.o
>>   cflatobjs += lib/s390x/sie.o
>>   cflatobjs += lib/s390x/fault.o
>> +cflatobjs += lib/s390x/skey.o
>>   OBJDIRS += lib/s390x
>> diff --git a/s390x/migration-skey.c b/s390x/migration-skey.c
>> index b7bd82581abe..fed6fc1ed0f8 100644
>> --- a/s390x/migration-skey.c
>> +++ b/s390x/migration-skey.c
>> @@ -10,55 +10,23 @@
>>   #include <libcflat.h>
>>   #include <asm/facility.h>
>> -#include <asm/page.h>
>> -#include <asm/mem.h>
>> -#include <asm/interrupt.h>
>>   #include <hardware.h>
>> +#include <skey.h>
>>   #define NUM_PAGES 128
>> -static uint8_t pagebuf[NUM_PAGES][PAGE_SIZE] 
>> __attribute__((aligned(PAGE_SIZE)));
>> +static uint8_t pagebuf[NUM_PAGES * PAGE_SIZE] 
>> __attribute__((aligned(PAGE_SIZE)));
>>   static void test_migration(void)
>>   {
>> -    union skey expected_key, actual_key;
>> -    int i, key_to_set, key_mismatches = 0;
>> +    struct skey_verify_result result;
>> -    for (i = 0; i < NUM_PAGES; i++) {
>> -        /*
>> -         * Storage keys are 7 bit, lowest bit is always returned as zero
>> -         * by iske.
>> -         * This loop will set all 7 bits which means we set fetch
>> -         * protection as well as reference and change indication for
>> -         * some keys.
>> -         */
>> -        key_to_set = i * 2;
>> -        set_storage_key(pagebuf[i], key_to_set, 1);
>> -    }
>> +    skey_set_keys(pagebuf, NUM_PAGES);
>>       puts("Please migrate me, then press return\n");
>>       (void)getchar();
>> -    for (i = 0; i < NUM_PAGES; i++) {
>> -        actual_key.val = get_storage_key(pagebuf[i]);
>> -        expected_key.val = i * 2;
>> -
>> -        /*
>> -         * The PoP neither gives a guarantee that the reference bit is
>> -         * accurate nor that it won't be cleared by hardware. Hence we
>> -         * don't rely on it and just clear the bits to avoid compare
>> -         * errors.
>> -         */
>> -        actual_key.str.rf = 0;
>> -        expected_key.str.rf = 0;
>> -
>> -        /* don't log anything when key matches to avoid spamming the log */
>> -        if (actual_key.val != expected_key.val) {
>> -            key_mismatches++;
>> -            report_fail("page %d expected_key=0x%x actual_key=0x%x", i, 
>> expected_key.val, actual_key.val);
>> -        }
>> -    }
>> -
>> -    report(!key_mismatches, "skeys after migration match");
>> +    result = skey_verify_keys(pagebuf, NUM_PAGES);
>> +    skey_report_verify(&result);
>>   }
>>   int main(void)
> 

Re: [kvm-unit-tests PATCH v1 1/3] s390x: add library for skey-related functions

[Nico Boehr]

Quoting Janosch Frank (2022-12-02 10:03:22)
> On 12/1/22 09:46, Nico Boehr wrote:
> > Upcoming changes will add a test which is very similar to the existing
> > skey migration test. To reduce code duplication, move the common
> > functions to a library which can be re-used by both tests.
> > 
> 
> NACK
> 
> We're not putting test specific code into the library.

What do you mean by "test specific"? After all, it is used by two tests now, possibly more in the future. 

Any alternative suggestions?

Re: [kvm-unit-tests PATCH v1 1/3] s390x: add library for skey-related functions

[Nico Boehr]

Quoting Thomas Huth (2022-12-02 10:09:03)
> On 02/12/2022 10.03, Janosch Frank wrote:
> > On 12/1/22 09:46, Nico Boehr wrote:
> >> Upcoming changes will add a test which is very similar to the existing
> >> skey migration test. To reduce code duplication, move the common
> >> functions to a library which can be re-used by both tests.
> >>
> > 
> > NACK
> > 
> > We're not putting test specific code into the library.
> 
> Do we need a new file (in the third patch) for the new test at all, or could 
> the new test simply be added to s390x/migration-skey.c instead?

Mh, not quite. One test wants to change storage keys *before* migrating, the other *while* migrating. Since we can only migrate once, it is not obvious to me how we could do that in one run.

Speaking of one run, what we could do is add a command line argument which decides which test to run and then call the same test with different arguments in unittests.cfg.

Other options I can think of (I don't like any of them):
- copy the code (probably the worst solution)
- header file in s390x which is included by both tests (better, but still bad, means double compilation of the test functions)

Re: [kvm-unit-tests PATCH v1 1/3] s390x: add library for skey-related functions

[Janosch Frank]

On 12/2/22 11:39, Nico Boehr wrote:
> Quoting Janosch Frank (2022-12-02 10:03:22)
>> On 12/1/22 09:46, Nico Boehr wrote:
>>> Upcoming changes will add a test which is very similar to the existing
>>> skey migration test. To reduce code duplication, move the common
>>> functions to a library which can be re-used by both tests.
>>>
>>
>> NACK
>>
>> We're not putting test specific code into the library.
> 
> What do you mean by "test specific"? After all, it is used by two tests now, possibly more in the future.
> 
> Any alternative suggestions?

For me this is like putting kselftest macros/functions into the kernel.

The KUT library is more or less the kernel on which the tests in s390x/ 
are based on. It provides primitives which (hopefully and mostly) aren't 
specific to tests.

Yes:
Providing skey set and get functions for one or multiple pages to tests.
I.e. sske and iske wrappers.

No:
Providing multi-page skey set and verify functions that set and verify 
skeys based on a pattern which is __hardcoded__ into the function using 
the skey wrappers. I.e. you're trying to create a new layer (test 
functionality) and stuffing it into the unit test kernel library.

What you want is a separate testlib which would reside in s390x/testlib/ 
where we can store often repeated functions and macros.

Re: [kvm-unit-tests PATCH v1 1/3] s390x: add library for skey-related functions

[Claudio Imbrenda]

On Fri, 2 Dec 2022 12:20:34 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 12/2/22 11:39, Nico Boehr wrote:
> > Quoting Janosch Frank (2022-12-02 10:03:22)  
> >> On 12/1/22 09:46, Nico Boehr wrote:  
> >>> Upcoming changes will add a test which is very similar to the existing
> >>> skey migration test. To reduce code duplication, move the common
> >>> functions to a library which can be re-used by both tests.
> >>>  
> >>
> >> NACK
> >>
> >> We're not putting test specific code into the library.  
> > 
> > What do you mean by "test specific"? After all, it is used by two tests now, possibly more in the future.
> > 
> > Any alternative suggestions?  
> 
> For me this is like putting kselftest macros/functions into the kernel.
> 
> The KUT library is more or less the kernel on which the tests in s390x/ 
> are based on. It provides primitives which (hopefully and mostly) aren't 
> specific to tests.
> 
> Yes:
> Providing skey set and get functions for one or multiple pages to tests.
> I.e. sske and iske wrappers.
> 
> No:
> Providing multi-page skey set and verify functions that set and verify 
> skeys based on a pattern which is __hardcoded__ into the function using 
> the skey wrappers. I.e. you're trying to create a new layer (test 
> functionality) and stuffing it into the unit test kernel library.
> 
> What you want is a separate testlib which would reside in s390x/testlib/ 
> where we can store often repeated functions and macros.

oufff, then we need to also fix the cmma migration tests

Re: [kvm-unit-tests PATCH v1 1/3] s390x: add library for skey-related functions

[Thomas Huth]

On 02/12/2022 11.44, Nico Boehr wrote:
> Quoting Thomas Huth (2022-12-02 10:09:03)
>> On 02/12/2022 10.03, Janosch Frank wrote:
>>> On 12/1/22 09:46, Nico Boehr wrote:
>>>> Upcoming changes will add a test which is very similar to the existing
>>>> skey migration test. To reduce code duplication, move the common
>>>> functions to a library which can be re-used by both tests.
>>>>
>>>
>>> NACK
>>>
>>> We're not putting test specific code into the library.
>>
>> Do we need a new file (in the third patch) for the new test at all, or could
>> the new test simply be added to s390x/migration-skey.c instead?
> 
> Mh, not quite. One test wants to change storage keys *before* migrating, the other *while* migrating. Since we can only migrate once, it is not obvious to me how we could do that in one run.
> 
> Speaking of one run, what we could do is add a command line argument which decides which test to run and then call the same test with different arguments in unittests.cfg.

Yes, that's what I had in mind - use a command line argument to select the 
test ... should be OK as long as both variants are listed in unittests.cfg, 
shouldn't it?

  Thomas

Re: [kvm-unit-tests PATCH v1 1/3] s390x: add library for skey-related functions

[Janosch Frank]

On 12/2/22 12:32, Thomas Huth wrote:
> On 02/12/2022 11.44, Nico Boehr wrote:
>> Quoting Thomas Huth (2022-12-02 10:09:03)
>>> On 02/12/2022 10.03, Janosch Frank wrote:
>>>> On 12/1/22 09:46, Nico Boehr wrote:
>>>>> Upcoming changes will add a test which is very similar to the existing
>>>>> skey migration test. To reduce code duplication, move the common
>>>>> functions to a library which can be re-used by both tests.
>>>>>
>>>>
>>>> NACK
>>>>
>>>> We're not putting test specific code into the library.
>>>
>>> Do we need a new file (in the third patch) for the new test at all, or could
>>> the new test simply be added to s390x/migration-skey.c instead?
>>
>> Mh, not quite. One test wants to change storage keys *before* migrating, the other *while* migrating. Since we can only migrate once, it is not obvious to me how we could do that in one run.
>>
>> Speaking of one run, what we could do is add a command line argument which decides which test to run and then call the same test with different arguments in unittests.cfg.
> 
> Yes, that's what I had in mind - use a command line argument to select the
> test ... should be OK as long as both variants are listed in unittests.cfg,
> shouldn't it?
> 
>    Thomas
> 


@Thomas @Claudio:
I see two possible solutions if we want a "testlib" at some point (which 
for the record I don't have anything against):

Putting the files into lib/s390x/testlib/* which will then be part of 
our normal lib.
That's a minimal effort solution. It still puts those files into lib/* 
but they are at least contained in a directory.

Putting the files into s390x/testlib/* and creating a proper new lib.
Which means we'd need a few more lines of makefile changes.


None of that is a huge amount of work.

Re: [kvm-unit-tests PATCH v1 1/3] s390x: add library for skey-related functions

[Thomas Huth]

On 02/12/2022 12.56, Janosch Frank wrote:
> On 12/2/22 12:32, Thomas Huth wrote:
>> On 02/12/2022 11.44, Nico Boehr wrote:
>>> Quoting Thomas Huth (2022-12-02 10:09:03)
>>>> On 02/12/2022 10.03, Janosch Frank wrote:
>>>>> On 12/1/22 09:46, Nico Boehr wrote:
>>>>>> Upcoming changes will add a test which is very similar to the existing
>>>>>> skey migration test. To reduce code duplication, move the common
>>>>>> functions to a library which can be re-used by both tests.
>>>>>>
>>>>>
>>>>> NACK
>>>>>
>>>>> We're not putting test specific code into the library.
>>>>
>>>> Do we need a new file (in the third patch) for the new test at all, or 
>>>> could
>>>> the new test simply be added to s390x/migration-skey.c instead?
>>>
>>> Mh, not quite. One test wants to change storage keys *before* migrating, 
>>> the other *while* migrating. Since we can only migrate once, it is not 
>>> obvious to me how we could do that in one run.
>>>
>>> Speaking of one run, what we could do is add a command line argument 
>>> which decides which test to run and then call the same test with 
>>> different arguments in unittests.cfg.
>>
>> Yes, that's what I had in mind - use a command line argument to select the
>> test ... should be OK as long as both variants are listed in unittests.cfg,
>> shouldn't it?
>>
>>    Thomas
> 
> @Thomas @Claudio:
> I see two possible solutions if we want a "testlib" at some point (which for 
> the record I don't have anything against):
> 
> Putting the files into lib/s390x/testlib/* which will then be part of our 
> normal lib.
> That's a minimal effort solution. It still puts those files into lib/* but 
> they are at least contained in a directory.
> 
> Putting the files into s390x/testlib/* and creating a proper new lib.
> Which means we'd need a few more lines of makefile changes.

Though this is an excellent topic for a Friday afternoon bikeshedding ... I 
don't mind much either way. I maybe just got a small preference to not touch 
the main lib/ folder here. I guess you could even call it 
s390x/migration-skey-common.c and leave the lib logic out of the game ... 
but I don't really mind. Up to you to decide ;-)

  Thomas

Re: [kvm-unit-tests PATCH v1 1/3] s390x: add library for skey-related functions

[Claudio Imbrenda]

On Fri, 2 Dec 2022 13:48:17 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 02/12/2022 12.56, Janosch Frank wrote:
> > On 12/2/22 12:32, Thomas Huth wrote:  
> >> On 02/12/2022 11.44, Nico Boehr wrote:  
> >>> Quoting Thomas Huth (2022-12-02 10:09:03)  
> >>>> On 02/12/2022 10.03, Janosch Frank wrote:  
> >>>>> On 12/1/22 09:46, Nico Boehr wrote:  
> >>>>>> Upcoming changes will add a test which is very similar to the existing
> >>>>>> skey migration test. To reduce code duplication, move the common
> >>>>>> functions to a library which can be re-used by both tests.
> >>>>>>  
> >>>>>
> >>>>> NACK
> >>>>>
> >>>>> We're not putting test specific code into the library.  
> >>>>
> >>>> Do we need a new file (in the third patch) for the new test at all, or 
> >>>> could
> >>>> the new test simply be added to s390x/migration-skey.c instead?  
> >>>
> >>> Mh, not quite. One test wants to change storage keys *before* migrating, 
> >>> the other *while* migrating. Since we can only migrate once, it is not 
> >>> obvious to me how we could do that in one run.
> >>>
> >>> Speaking of one run, what we could do is add a command line argument 
> >>> which decides which test to run and then call the same test with 
> >>> different arguments in unittests.cfg.  
> >>
> >> Yes, that's what I had in mind - use a command line argument to select the
> >> test ... should be OK as long as both variants are listed in unittests.cfg,
> >> shouldn't it?
> >>
> >>    Thomas  
> > 
> > @Thomas @Claudio:
> > I see two possible solutions if we want a "testlib" at some point (which for 
> > the record I don't have anything against):
> > 
> > Putting the files into lib/s390x/testlib/* which will then be part of our 
> > normal lib.
> > That's a minimal effort solution. It still puts those files into lib/* but 
> > they are at least contained in a directory.
> > 
> > Putting the files into s390x/testlib/* and creating a proper new lib.
> > Which means we'd need a few more lines of makefile changes.  
> 
> Though this is an excellent topic for a Friday afternoon bikeshedding ... I 
> don't mind much either way. I maybe just got a small preference to not touch 
> the main lib/ folder here. I guess you could even call it 
> s390x/migration-skey-common.c and leave the lib logic out of the game ... 
> but I don't really mind. Up to you to decide ;-)
> 

I really like the idea of having only one test and use a commandline
parameter to decide which variant to run

this way no need to put things in external files

>   Thomas
> 

Re: [kvm-unit-tests PATCH v1 1/3] s390x: add library for skey-related functions

[Nico Boehr]

Quoting Claudio Imbrenda (2022-12-02 13:56:47)
> On Fri, 2 Dec 2022 13:48:17 +0100
> Thomas Huth <thuth@redhat.com> wrote:
> 
> > On 02/12/2022 12.56, Janosch Frank wrote:
> > > On 12/2/22 12:32, Thomas Huth wrote:  
> > >> On 02/12/2022 11.44, Nico Boehr wrote:  
> > >>> Quoting Thomas Huth (2022-12-02 10:09:03)  
> > >>>> On 02/12/2022 10.03, Janosch Frank wrote:  
> > >>>>> On 12/1/22 09:46, Nico Boehr wrote:  
> > >>>>>> Upcoming changes will add a test which is very similar to the existing
> > >>>>>> skey migration test. To reduce code duplication, move the common
> > >>>>>> functions to a library which can be re-used by both tests.
> > >>>>>>  
> > >>>>>
> > >>>>> NACK
> > >>>>>
> > >>>>> We're not putting test specific code into the library.  
> > >>>>
> > >>>> Do we need a new file (in the third patch) for the new test at all, or 
> > >>>> could
> > >>>> the new test simply be added to s390x/migration-skey.c instead?  
> > >>>
> > >>> Mh, not quite. One test wants to change storage keys *before* migrating, 
> > >>> the other *while* migrating. Since we can only migrate once, it is not 
> > >>> obvious to me how we could do that in one run.
> > >>>
> > >>> Speaking of one run, what we could do is add a command line argument 
> > >>> which decides which test to run and then call the same test with 
> > >>> different arguments in unittests.cfg.  
> > >>
> > >> Yes, that's what I had in mind - use a command line argument to select the
> > >> test ... should be OK as long as both variants are listed in unittests.cfg,
> > >> shouldn't it?
> > >>
> > >>    Thomas  
> > > 
> > > @Thomas @Claudio:
> > > I see two possible solutions if we want a "testlib" at some point (which for 
> > > the record I don't have anything against):
> > > 
> > > Putting the files into lib/s390x/testlib/* which will then be part of our 
> > > normal lib.
> > > That's a minimal effort solution. It still puts those files into lib/* but 
> > > they are at least contained in a directory.
> > > 
> > > Putting the files into s390x/testlib/* and creating a proper new lib.
> > > Which means we'd need a few more lines of makefile changes.  
> > 
> > Though this is an excellent topic for a Friday afternoon bikeshedding ... I 
> > don't mind much either way. I maybe just got a small preference to not touch 
> > the main lib/ folder here. I guess you could even call it 
> > s390x/migration-skey-common.c and leave the lib logic out of the game ... 
> > but I don't really mind. Up to you to decide ;-)
> > 
> 
> I really like the idea of having only one test and use a commandline
> parameter to decide which variant to run
> 
> this way no need to put things in external files

OK, I also like this suggestion, then I'll refactor the tests.