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

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

[Nico Boehr]

v1->v2:
---
* remove the skey library and move both versions of the skey migration
  test to a single file
* rename skey_set_keys/verify_keys to set_test_pattern/verify_test_pattern
* add a few comments

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 (1):
  s390x: add parallel skey migration test

 s390x/migration-skey.c | 214 +++++++++++++++++++++++++++++++++++------
 s390x/unittests.cfg    |  15 ++-
 2 files changed, 198 insertions(+), 31 deletions(-)

-- 
2.36.1

[kvm-unit-tests PATCH v2 1/1] s390x: add parallel skey migration test

[Nico Boehr]

Right now, we have a test which sets storage keys, then migrates the VM
and - after migration finished - verifies the skeys are still there.

Add a new version of the test which changes storage keys while the
migration is in progress. This is achieved by adding a command line
argument to the existing migration-skey test.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 s390x/migration-skey.c | 214 +++++++++++++++++++++++++++++++++++------
 s390x/unittests.cfg    |  15 ++-
 2 files changed, 198 insertions(+), 31 deletions(-)

diff --git a/s390x/migration-skey.c b/s390x/migration-skey.c
index b7bd82581abe..9b9a45f4ad3b 100644
--- a/s390x/migration-skey.c
+++ b/s390x/migration-skey.c
@@ -2,6 +2,12 @@
 /*
  * Storage Key migration tests
  *
+ * There are two variants of this test:
+ * - sequential: sets some storage keys on pages, migrates the VM and then
+ *   verifies the storage keys are still as we expect.
+ * - parallel: start migration of a VM and set and check storage keys on some
+ *   pages while migration is in process.
+ *
  * Copyright IBM Corp. 2022
  *
  * Authors:
@@ -10,20 +16,44 @@
 
 #include <libcflat.h>
 #include <asm/facility.h>
-#include <asm/page.h>
 #include <asm/mem.h>
-#include <asm/interrupt.h>
+#include <asm/page.h>
+#include <asm/barrier.h>
 #include <hardware.h>
+#include <smp.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;
+};
 
 #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 struct skey_verify_result result;
 
-static void test_migration(void)
+static unsigned int thread_iters;
+static bool thread_should_exit;
+static bool thread_exited;
+
+/*
+ * Set storage key test pattern on pagebuf with a seed for the storage keys.
+ *
+ * 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.
+ *
+ * pagebuf must point to page_count consecutive pages.
+ * Only the lower seven bits of the seed are considered.
+ */
+static void skey_set_test_pattern(uint8_t *pagebuf, unsigned long page_count, unsigned char seed)
 {
-	union skey expected_key, actual_key;
-	int i, key_to_set, key_mismatches = 0;
+	unsigned char key_to_set;
+	unsigned long i;
 
-	for (i = 0; i < NUM_PAGES; i++) {
+	for (i = 0; i < page_count; i++) {
 		/*
 		 * Storage keys are 7 bit, lowest bit is always returned as zero
 		 * by iske.
@@ -31,16 +61,34 @@ static void test_migration(void)
 		 * 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);
+		key_to_set = (i ^ seed) * 2;
+		set_storage_key(pagebuf + i * PAGE_SIZE, key_to_set, 1);
 	}
+}
 
-	puts("Please migrate me, then press return\n");
-	(void)getchar();
+/*
+ * Verify storage keys on pagebuf.
+ * Storage keys must have been set by skey_set_test_pattern 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.
+ */
+static struct skey_verify_result skey_verify_test_pattern(uint8_t *pagebuf, unsigned long page_count, unsigned char seed)
+{
+	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 < NUM_PAGES; i++) {
-		actual_key.val = get_storage_key(pagebuf[i]);
-		expected_key.val = i * 2;
+	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 ^ seed) * 2;
 
 		/*
 		 * The PoP neither gives a guarantee that the reference bit is
@@ -51,33 +99,145 @@ static void test_migration(void)
 		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);
+			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;
 		}
 	}
 
-	report(!key_mismatches, "skeys after migration match");
+	result.verify_failed = false;
+	return result;
+}
+
+static 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");
+}
+
+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)
+static void test_skey_migration_sequential(void)
+{
+	report_prefix_push("sequential");
+
+	skey_set_test_pattern(pagebuf, NUM_PAGES, 0);
+
+	migrate_once();
+
+	result = skey_verify_test_pattern(pagebuf, NUM_PAGES, 0);
+	skey_report_verify(&result);
+
+	report_prefix_pop();
+}
+
+static void set_skeys_thread(void)
+{
+	while (!READ_ONCE(thread_should_exit)) {
+		skey_set_test_pattern(pagebuf, NUM_PAGES, thread_iters);
+
+		result = skey_verify_test_pattern(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 test_skey_migration_parallel(void)
+{
+	report_prefix_push("parallel");
+
+	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(set_skeys_thread));
+
+	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_test_pattern(pagebuf, NUM_PAGES, thread_iters - 1);
+	skey_report_verify(&result);
+	report_prefix_pop();
+
+error:
+	report_prefix_pop();
+}
+
+static void print_usage(void)
+{
+	report_info("Usage: migration-skey [parallel|sequential]");
+}
+
+int main(int argc, char **argv)
 {
 	report_prefix_push("migration-skey");
 	if (test_facility(169)) {
 		report_skip("storage key removal facility is active");
+		goto error;
+	}
 
-		/*
-		 * If we just exit and don't ask migrate_cmd to migrate us, it
-		 * will just hang forever. Hence, also ask for migration when we
-		 * skip this test altogether.
-		 */
-		puts("Please migrate me, then press return\n");
-		(void)getchar();
+	if (argc < 2) {
+		print_usage();
+		goto error;
+	}
+
+	if (!strcmp("parallel", argv[1])) {
+		test_skey_migration_parallel();
+	} else if (!strcmp("sequential", argv[1])) {
+		test_skey_migration_sequential();
 	} else {
-		test_migration();
+		print_usage();
 	}
 
+error:
+	migrate_once();
 	report_prefix_pop();
 	return report_summary();
 }
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index 3caf81eda396..7763cb857954 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -185,10 +185,6 @@ smp = 2
 file = migration-cmm.elf
 groups = migration
 
-[migration-skey]
-file = migration-skey.elf
-groups = migration
-
 [panic-loop-extint]
 file = panic-loop-extint.elf
 groups = panic
@@ -208,3 +204,14 @@ groups = migration
 [exittime]
 file = exittime.elf
 smp = 2
+
+[migration-skey-sequential]
+file = migration-skey.elf
+groups = migration
+extra_params = -append 'sequential'
+
+[migration-skey-parallel]
+file = migration-skey.elf
+smp = 2
+groups = migration
+extra_params = -append 'parallel'
-- 
2.36.1

Re: [kvm-unit-tests PATCH v2 1/1] s390x: add parallel skey migration test

[Nina Schoetterl-Glausch]

On Fri, 2022-12-09 at 11:21 +0100, Nico Boehr wrote:
> Right now, we have a test which sets storage keys, then migrates the VM
> and - after migration finished - verifies the skeys are still there.
> 
> Add a new version of the test which changes storage keys while the
> migration is in progress. This is achieved by adding a command line
> argument to the existing migration-skey test.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>  s390x/migration-skey.c | 214 +++++++++++++++++++++++++++++++++++------
>  s390x/unittests.cfg    |  15 ++-
>  2 files changed, 198 insertions(+), 31 deletions(-)
> 
> diff --git a/s390x/migration-skey.c b/s390x/migration-skey.c
> index b7bd82581abe..9b9a45f4ad3b 100644
> --- a/s390x/migration-skey.c
> +++ b/s390x/migration-skey.c
> 
[...]

> +static void test_skey_migration_parallel(void)
> +{
> +	report_prefix_push("parallel");
> +
> +	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(set_skeys_thread));
> +
> +	migrate_once();
> +
> +	WRITE_ONCE(thread_should_exit, 1);
> +
> +	while (!thread_exited)
> +		mb();

Are you doing it this way instead of while(!READ_ONCE(thread_exited)); so the mb() does double duty
and ensures "result" is also read from memory a couple of lines down?
If so, I wonder if the compiler is allowed to arrange the control flow such that if the loop condition
is false on the first iteration it uses a cached value of "result" (I'd be guessing yes, but what do I know).
In any case using a do while loop instead would eliminate the question.
A comment might be nice, too.

> +
> +	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_test_pattern(pagebuf, NUM_PAGES, thread_iters - 1);
> +	skey_report_verify(&result);
> +	report_prefix_pop();
> +
> +error:
> +	report_prefix_pop();
> +}
> +
[...]

Re: [kvm-unit-tests PATCH v2 1/1] s390x: add parallel skey migration test

[Nico Boehr]

Quoting Nina Schoetterl-Glausch (2022-12-12 21:37:28)
> On Fri, 2022-12-09 at 11:21 +0100, Nico Boehr wrote:
> > Right now, we have a test which sets storage keys, then migrates the VM
> > and - after migration finished - verifies the skeys are still there.
> > 
> > Add a new version of the test which changes storage keys while the
> > migration is in progress. This is achieved by adding a command line
> > argument to the existing migration-skey test.
> > 
> > Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> > ---
> >  s390x/migration-skey.c | 214 +++++++++++++++++++++++++++++++++++------
> >  s390x/unittests.cfg    |  15 ++-
> >  2 files changed, 198 insertions(+), 31 deletions(-)
> > 
> > diff --git a/s390x/migration-skey.c b/s390x/migration-skey.c
> > index b7bd82581abe..9b9a45f4ad3b 100644
> > --- a/s390x/migration-skey.c
> > +++ b/s390x/migration-skey.c
> > 
> [...]
> 
> > +static void test_skey_migration_parallel(void)
> > +{
> > +     report_prefix_push("parallel");
> > +
> > +     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(set_skeys_thread));
> > +
> > +     migrate_once();
> > +
> > +     WRITE_ONCE(thread_should_exit, 1);
> > +
> > +     while (!thread_exited)
> > +             mb();
> 
> Are you doing it this way instead of while(!READ_ONCE(thread_exited)); so the mb() does double duty
> and ensures "result" is also read from memory a couple of lines down?

It is a good point, actually I just did what we already do in wait_for_flag in s390x/smp.c. :-)

> If so, I wonder if the compiler is allowed to arrange the control flow such that if the loop condition
> is false on the first iteration it uses a cached value of "result" (I'd be guessing yes, but what do I know).

I agree, but it does not matter, does it? At latest the second iteration will actually read from memory, no?

> In any case using a do while loop instead would eliminate the question.
> A comment might be nice, too.

How about I change to
  while(!READ_ONCE(thread_exited)); 
and add an explicit mb() below to ensure result is read from memory?

Re: [kvm-unit-tests PATCH v2 1/1] s390x: add parallel skey migration test

[Nina Schoetterl-Glausch]

On Tue, 2022-12-13 at 09:50 +0100, Nico Boehr wrote:
> Quoting Nina Schoetterl-Glausch (2022-12-12 21:37:28)
> > On Fri, 2022-12-09 at 11:21 +0100, Nico Boehr wrote:
> > > Right now, we have a test which sets storage keys, then migrates the VM
> > > and - after migration finished - verifies the skeys are still there.
> > > 
> > > Add a new version of the test which changes storage keys while the
> > > migration is in progress. This is achieved by adding a command line
> > > argument to the existing migration-skey test.
> > > 
> > > Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> > > ---
> > >  s390x/migration-skey.c | 214 +++++++++++++++++++++++++++++++++++------
> > >  s390x/unittests.cfg    |  15 ++-
> > >  2 files changed, 198 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/s390x/migration-skey.c b/s390x/migration-skey.c
> > > index b7bd82581abe..9b9a45f4ad3b 100644
> > > --- a/s390x/migration-skey.c
> > > +++ b/s390x/migration-skey.c
> > > 
> > [...]
> > 
> > > +static void test_skey_migration_parallel(void)
> > > +{
> > > +     report_prefix_push("parallel");
> > > +
> > > +     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(set_skeys_thread));
> > > +
> > > +     migrate_once();
> > > +
> > > +     WRITE_ONCE(thread_should_exit, 1);
> > > +
> > > +     while (!thread_exited)
> > > +             mb();
> > 
> > Are you doing it this way instead of while(!READ_ONCE(thread_exited)); so the mb() does double duty
> > and ensures "result" is also read from memory a couple of lines down?
> 
> It is a good point, actually I just did what we already do in wait_for_flag in s390x/smp.c. :-)
> 
> > If so, I wonder if the compiler is allowed to arrange the control flow such that if the loop condition
> > is false on the first iteration it uses a cached value of "result" (I'd be guessing yes, but what do I know).
> 
> I agree, but it does not matter, does it? At latest the second iteration will actually read from memory, no?

Well, if the condition is false on the first iteration, there won't be a second one.
> 
> > In any case using a do while loop instead would eliminate the question.
> > A comment might be nice, too.
> 
> How about I change to
>   while(!READ_ONCE(thread_exited)); 
> and add an explicit mb() below to ensure result is read from memory?

Fine by me. Could also use READ_ONCE for result. You decide.
Btw, doesn't checkpatch complain about mb() without comment?
Although I think I've ignored that before, too.

Re: [kvm-unit-tests PATCH v2 1/1] s390x: add parallel skey migration test

[Claudio Imbrenda]

On Tue, 13 Dec 2022 12:11:29 +0100
Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:

> On Tue, 2022-12-13 at 09:50 +0100, Nico Boehr wrote:
> > Quoting Nina Schoetterl-Glausch (2022-12-12 21:37:28)  
> > > On Fri, 2022-12-09 at 11:21 +0100, Nico Boehr wrote:  
> > > > Right now, we have a test which sets storage keys, then migrates the VM
> > > > and - after migration finished - verifies the skeys are still there.
> > > > 
> > > > Add a new version of the test which changes storage keys while the
> > > > migration is in progress. This is achieved by adding a command line
> > > > argument to the existing migration-skey test.
> > > > 
> > > > Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> > > > ---
> > > >  s390x/migration-skey.c | 214 +++++++++++++++++++++++++++++++++++------
> > > >  s390x/unittests.cfg    |  15 ++-
> > > >  2 files changed, 198 insertions(+), 31 deletions(-)
> > > > 
> > > > diff --git a/s390x/migration-skey.c b/s390x/migration-skey.c
> > > > index b7bd82581abe..9b9a45f4ad3b 100644
> > > > --- a/s390x/migration-skey.c
> > > > +++ b/s390x/migration-skey.c
> > > >   
> > > [...]
> > >   
> > > > +static void test_skey_migration_parallel(void)
> > > > +{
> > > > +     report_prefix_push("parallel");
> > > > +
> > > > +     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(set_skeys_thread));
> > > > +
> > > > +     migrate_once();
> > > > +
> > > > +     WRITE_ONCE(thread_should_exit, 1);
> > > > +
> > > > +     while (!thread_exited)
> > > > +             mb();  
> > > 
> > > Are you doing it this way instead of while(!READ_ONCE(thread_exited)); so the mb() does double duty
> > > and ensures "result" is also read from memory a couple of lines down?  
> > 
> > It is a good point, actually I just did what we already do in wait_for_flag in s390x/smp.c. :-)
> >   
> > > If so, I wonder if the compiler is allowed to arrange the control flow such that if the loop condition
> > > is false on the first iteration it uses a cached value of "result" (I'd be guessing yes, but what do I know).  
> > 
> > I agree, but it does not matter, does it? At latest the second iteration will actually read from memory, no?  
> 
> Well, if the condition is false on the first iteration, there won't be a second one.
> >   
> > > In any case using a do while loop instead would eliminate the question.
> > > A comment might be nice, too.  
> > 
> > How about I change to
> >   while(!READ_ONCE(thread_exited)); 
> > and add an explicit mb() below to ensure result is read from memory?  
> 
> Fine by me. Could also use READ_ONCE for result. You decide.
> Btw, doesn't checkpatch complain about mb() without comment?

there is no checkpatch for kvm unit tests :)

> Although I think I've ignored that before, too.
> 

Re: [kvm-unit-tests PATCH v2 1/1] s390x: add parallel skey migration test

[Nina Schoetterl-Glausch]

On Tue, 2022-12-13 at 13:14 +0100, Claudio Imbrenda wrote:
> On Tue, 13 Dec 2022 12:11:29 +0100
> Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:
> 
> > On Tue, 2022-12-13 at 09:50 +0100, Nico Boehr wrote:
> > > Quoting Nina Schoetterl-Glausch (2022-12-12 21:37:28)  
> > > > On Fri, 2022-12-09 at 11:21 +0100, Nico Boehr wrote:  
> > > > > Right now, we have a test which sets storage keys, then migrates the VM
> > > > > and - after migration finished - verifies the skeys are still there.
> > > > > 
> > > > > Add a new version of the test which changes storage keys while the
> > > > > migration is in progress. This is achieved by adding a command line
> > > > > argument to the existing migration-skey test.
> > > > > 
> > > > > Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> > > > > ---
> > > > >  s390x/migration-skey.c | 214 +++++++++++++++++++++++++++++++++++------
> > > > >  s390x/unittests.cfg    |  15 ++-
> > > > >  2 files changed, 198 insertions(+), 31 deletions(-)
> > > > > 
> > > > > diff --git a/s390x/migration-skey.c b/s390x/migration-skey.c
> > > > > index b7bd82581abe..9b9a45f4ad3b 100644
> > > > > --- a/s390x/migration-skey.c
> > > > > +++ b/s390x/migration-skey.c
> > > > >   
> > > > [...]
> > > >   
> > > > > +static void test_skey_migration_parallel(void)
> > > > > +{
> > > > > +     report_prefix_push("parallel");
> > > > > +
> > > > > +     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(set_skeys_thread));
> > > > > +
> > > > > +     migrate_once();
> > > > > +
> > > > > +     WRITE_ONCE(thread_should_exit, 1);
> > > > > +
> > > > > +     while (!thread_exited)
> > > > > +             mb();  
> > > > 
> > > > Are you doing it this way instead of while(!READ_ONCE(thread_exited)); so the mb() does double duty
> > > > and ensures "result" is also read from memory a couple of lines down?  
> > > 
> > > It is a good point, actually I just did what we already do in wait_for_flag in s390x/smp.c. :-)
> > >   
> > > > If so, I wonder if the compiler is allowed to arrange the control flow such that if the loop condition
> > > > is false on the first iteration it uses a cached value of "result" (I'd be guessing yes, but what do I know).  
> > > 
> > > I agree, but it does not matter, does it? At latest the second iteration will actually read from memory, no?  
> > 
> > Well, if the condition is false on the first iteration, there won't be a second one.
> > >   
> > > > In any case using a do while loop instead would eliminate the question.
> > > > A comment might be nice, too.  
> > > 
> > > How about I change to
> > >   while(!READ_ONCE(thread_exited)); 
> > > and add an explicit mb() below to ensure result is read from memory?  
> > 
> > Fine by me. Could also use READ_ONCE for result. You decide.
> > Btw, doesn't checkpatch complain about mb() without comment?
> 
> there is no checkpatch for kvm unit tests :)

Well, ok, depends what you consider part of the coding style then :)
Since k-u-t uses kernel style. I run it and then ignore what I judge reasonable to ignore :)
> 
> > Although I think I've ignored that before, too.
> > 
> 

Re: [kvm-unit-tests PATCH v2 1/1] s390x: add parallel skey migration test

[Nico Boehr]

Quoting Nina Schoetterl-Glausch (2022-12-13 12:11:29)
> On Tue, 2022-12-13 at 09:50 +0100, Nico Boehr wrote:
> > Quoting Nina Schoetterl-Glausch (2022-12-12 21:37:28)
> > > On Fri, 2022-12-09 at 11:21 +0100, Nico Boehr wrote:
> > > > Right now, we have a test which sets storage keys, then migrates the VM
> > > > and - after migration finished - verifies the skeys are still there.
> > > > 
> > > > Add a new version of the test which changes storage keys while the
> > > > migration is in progress. This is achieved by adding a command line
> > > > argument to the existing migration-skey test.
> > > > 
> > > > Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> > > > ---
> > > >  s390x/migration-skey.c | 214 +++++++++++++++++++++++++++++++++++------
> > > >  s390x/unittests.cfg    |  15 ++-
> > > >  2 files changed, 198 insertions(+), 31 deletions(-)
> > > > 
> > > > diff --git a/s390x/migration-skey.c b/s390x/migration-skey.c
> > > > index b7bd82581abe..9b9a45f4ad3b 100644
> > > > --- a/s390x/migration-skey.c
> > > > +++ b/s390x/migration-skey.c
> > > > 
> > > [...]
> > > 
> > > > +static void test_skey_migration_parallel(void)
> > > > +{
> > > > +     report_prefix_push("parallel");
> > > > +
> > > > +     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(set_skeys_thread));
> > > > +
> > > > +     migrate_once();
> > > > +
> > > > +     WRITE_ONCE(thread_should_exit, 1);
> > > > +
> > > > +     while (!thread_exited)
> > > > +             mb();
> > > 
> > > Are you doing it this way instead of while(!READ_ONCE(thread_exited)); so the mb() does double duty
> > > and ensures "result" is also read from memory a couple of lines down?
> > 
> > It is a good point, actually I just did what we already do in wait_for_flag in s390x/smp.c. :-)
> > 
> > > If so, I wonder if the compiler is allowed to arrange the control flow such that if the loop condition
> > > is false on the first iteration it uses a cached value of "result" (I'd be guessing yes, but what do I know).
> > 
> > I agree, but it does not matter, does it? At latest the second iteration will actually read from memory, no?
> 
> Well, if the condition is false on the first iteration, there won't be a second one.

Yes, you are right. Thanks for pointing this out.

Re: [kvm-unit-tests PATCH v2 1/1] s390x: add parallel skey migration test

[Claudio Imbrenda]

On Tue, 13 Dec 2022 09:50:06 +0100
Nico Boehr <nrb@linux.ibm.com> wrote:

> Quoting Nina Schoetterl-Glausch (2022-12-12 21:37:28)
> > On Fri, 2022-12-09 at 11:21 +0100, Nico Boehr wrote:  
> > > Right now, we have a test which sets storage keys, then migrates the VM
> > > and - after migration finished - verifies the skeys are still there.
> > > 
> > > Add a new version of the test which changes storage keys while the
> > > migration is in progress. This is achieved by adding a command line
> > > argument to the existing migration-skey test.
> > > 
> > > Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> > > ---
> > >  s390x/migration-skey.c | 214 +++++++++++++++++++++++++++++++++++------
> > >  s390x/unittests.cfg    |  15 ++-
> > >  2 files changed, 198 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/s390x/migration-skey.c b/s390x/migration-skey.c
> > > index b7bd82581abe..9b9a45f4ad3b 100644
> > > --- a/s390x/migration-skey.c
> > > +++ b/s390x/migration-skey.c
> > >   
> > [...]
> >   
> > > +static void test_skey_migration_parallel(void)
> > > +{
> > > +     report_prefix_push("parallel");
> > > +
> > > +     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(set_skeys_thread));
> > > +
> > > +     migrate_once();
> > > +
> > > +     WRITE_ONCE(thread_should_exit, 1);
> > > +
> > > +     while (!thread_exited)
> > > +             mb();  
> > 
> > Are you doing it this way instead of while(!READ_ONCE(thread_exited)); so the mb() does double duty
> > and ensures "result" is also read from memory a couple of lines down?  
> 
> It is a good point, actually I just did what we already do in wait_for_flag in s390x/smp.c. :-)
> 
> > If so, I wonder if the compiler is allowed to arrange the control flow such that if the loop condition
> > is false on the first iteration it uses a cached value of "result" (I'd be guessing yes, but what do I know).  
> 
> I agree, but it does not matter, does it? At latest the second iteration will actually read from memory, no?
> 
> > In any case using a do while loop instead would eliminate the question.
> > A comment might be nice, too.  
> 
> How about I change to
>   while(!READ_ONCE(thread_exited)); 
> and add an explicit mb() below to ensure result is read from memory?

yes please

Re: [kvm-unit-tests PATCH v2 1/1] s390x: add parallel skey migration test

[Claudio Imbrenda]

On Fri,  9 Dec 2022 11:21:22 +0100
Nico Boehr <nrb@linux.ibm.com> wrote:

> Right now, we have a test which sets storage keys, then migrates the VM
> and - after migration finished - verifies the skeys are still there.
> 
> Add a new version of the test which changes storage keys while the
> migration is in progress. This is achieved by adding a command line
> argument to the existing migration-skey test.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>

looks good, except for a few small details below

> ---
>  s390x/migration-skey.c | 214 +++++++++++++++++++++++++++++++++++------
>  s390x/unittests.cfg    |  15 ++-
>  2 files changed, 198 insertions(+), 31 deletions(-)
> 
> diff --git a/s390x/migration-skey.c b/s390x/migration-skey.c
> index b7bd82581abe..9b9a45f4ad3b 100644
> --- a/s390x/migration-skey.c
> +++ b/s390x/migration-skey.c
> @@ -2,6 +2,12 @@
>  /*
>   * Storage Key migration tests
>   *
> + * There are two variants of this test:
> + * - sequential: sets some storage keys on pages, migrates the VM and then
> + *   verifies the storage keys are still as we expect.
> + * - parallel: start migration of a VM and set and check storage keys on some
> + *   pages while migration is in process.
> + *
>   * Copyright IBM Corp. 2022
>   *
>   * Authors:
> @@ -10,20 +16,44 @@
>  
>  #include <libcflat.h>
>  #include <asm/facility.h>
> -#include <asm/page.h>
>  #include <asm/mem.h>
> -#include <asm/interrupt.h>
> +#include <asm/page.h>
> +#include <asm/barrier.h>
>  #include <hardware.h>
> +#include <smp.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;
> +};
>  
>  #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 struct skey_verify_result result;
>  
> -static void test_migration(void)
> +static unsigned int thread_iters;
> +static bool thread_should_exit;
> +static bool thread_exited;
> +
> +/*
> + * Set storage key test pattern on pagebuf with a seed for the storage keys.
> + *
> + * 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.
> + *
> + * pagebuf must point to page_count consecutive pages.
> + * Only the lower seven bits of the seed are considered.
> + */
> +static void skey_set_test_pattern(uint8_t *pagebuf, unsigned long page_count, unsigned char seed)
>  {
> -	union skey expected_key, actual_key;
> -	int i, key_to_set, key_mismatches = 0;
> +	unsigned char key_to_set;
> +	unsigned long i;
>  
> -	for (i = 0; i < NUM_PAGES; i++) {
> +	for (i = 0; i < page_count; i++) {
>  		/*
>  		 * Storage keys are 7 bit, lowest bit is always returned as zero
>  		 * by iske.
> @@ -31,16 +61,34 @@ static void test_migration(void)
>  		 * 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);
> +		key_to_set = (i ^ seed) * 2;
> +		set_storage_key(pagebuf + i * PAGE_SIZE, key_to_set, 1);
>  	}
> +}
>  
> -	puts("Please migrate me, then press return\n");
> -	(void)getchar();
> +/*
> + * Verify storage keys on pagebuf.
> + * Storage keys must have been set by skey_set_test_pattern 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.
> + */
> +static struct skey_verify_result skey_verify_test_pattern(uint8_t *pagebuf, unsigned long page_count, unsigned char seed)

this line is a little too long, even for the KVM unit tests; find a way
to shorten it by a couple of bytes (it would look better to keep it on
one line)

(rename pagebuf? use size_t or uint64_t instead of unsigned long? use
uint8_t for seed? shorten the name of struct verify_result?)

> +{
> +	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 < NUM_PAGES; i++) {
> -		actual_key.val = get_storage_key(pagebuf[i]);
> -		expected_key.val = i * 2;
> +	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 ^ seed) * 2;
>  
>  		/*
>  		 * The PoP neither gives a guarantee that the reference bit is
> @@ -51,33 +99,145 @@ static void test_migration(void)
>  		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);
> +			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;
>  		}
>  	}
>  
> -	report(!key_mismatches, "skeys after migration match");
> +	result.verify_failed = false;
> +	return result;
> +}
> +
> +static 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");
> +}
> +
> +static void migrate_once(void)
> +{
> +	static bool migrated;
> +
> +	if (migrated)
> +		return;
> +
> +	migrated = true;
> +	puts("Please migrate me, then press return\n");
> +	(void)getchar();
>  }

you don't need this any more, since your migration patches are
upstream now :)

>  
> -int main(void)
> +static void test_skey_migration_sequential(void)
> +{
> +	report_prefix_push("sequential");
> +
> +	skey_set_test_pattern(pagebuf, NUM_PAGES, 0);
> +
> +	migrate_once();
> +
> +	result = skey_verify_test_pattern(pagebuf, NUM_PAGES, 0);
> +	skey_report_verify(&result);
> +
> +	report_prefix_pop();
> +}
> +
> +static void set_skeys_thread(void)
> +{
> +	while (!READ_ONCE(thread_should_exit)) {
> +		skey_set_test_pattern(pagebuf, NUM_PAGES, thread_iters);
> +
> +		result = skey_verify_test_pattern(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 test_skey_migration_parallel(void)
> +{
> +	report_prefix_push("parallel");
> +
> +	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(set_skeys_thread));
> +
> +	migrate_once();
> +
> +	WRITE_ONCE(thread_should_exit, 1);
> +
> +	while (!thread_exited)
> +		mb();

this was discussed in the other subthread :)

> +
> +	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_test_pattern(pagebuf, NUM_PAGES, thread_iters - 1);
> +	skey_report_verify(&result);
> +	report_prefix_pop();
> +
> +error:
> +	report_prefix_pop();
> +}
> +
> +static void print_usage(void)
> +{
> +	report_info("Usage: migration-skey [parallel|sequential]");
> +}
> +
> +int main(int argc, char **argv)
>  {
>  	report_prefix_push("migration-skey");
>  	if (test_facility(169)) {
>  		report_skip("storage key removal facility is active");
> +		goto error;
> +	}
>  
> -		/*
> -		 * If we just exit and don't ask migrate_cmd to migrate us, it
> -		 * will just hang forever. Hence, also ask for migration when we
> -		 * skip this test altogether.
> -		 */
> -		puts("Please migrate me, then press return\n");
> -		(void)getchar();
> +	if (argc < 2) {
> +		print_usage();
> +		goto error;
> +	}
> +
> +	if (!strcmp("parallel", argv[1])) {
> +		test_skey_migration_parallel();
> +	} else if (!strcmp("sequential", argv[1])) {
> +		test_skey_migration_sequential();
>  	} else {
> -		test_migration();
> +		print_usage();

hmm I don't like this whole main.

I think the best approach would be to have some kind of separate
parse_args function that will set some flags (or one flag, in this case)

then the main becomes just a bunch of if/else, something like this:

parse_args(argc, argv);
if (test_facility(169))
	skip
else if (parallel_flag)
	parallel();
else
	sequential();

also, default to sequential if there are no parameters, and use
posix-style parameters (--parallel, --sequential)

only if you get an invalid parameter you can print the usage and fail
optionally if you really want you can print the usage info when called
without parameter and inform that the test will default to sequential


one of these days I have to write an argument parsing library :)

>  	}
>  
> +error:
> +	migrate_once();
>  	report_prefix_pop();
>  	return report_summary();
>  }
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index 3caf81eda396..7763cb857954 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -185,10 +185,6 @@ smp = 2
>  file = migration-cmm.elf
>  groups = migration
>  
> -[migration-skey]
> -file = migration-skey.elf
> -groups = migration
> -
>  [panic-loop-extint]
>  file = panic-loop-extint.elf
>  groups = panic
> @@ -208,3 +204,14 @@ groups = migration
>  [exittime]
>  file = exittime.elf
>  smp = 2
> +
> +[migration-skey-sequential]
> +file = migration-skey.elf
> +groups = migration
> +extra_params = -append 'sequential'
> +
> +[migration-skey-parallel]
> +file = migration-skey.elf
> +smp = 2
> +groups = migration
> +extra_params = -append 'parallel'

Re: [kvm-unit-tests PATCH v2 1/1] s390x: add parallel skey migration test

[Nico Boehr]

Quoting Claudio Imbrenda (2022-12-13 18:27:56)
[...]
> > diff --git a/s390x/migration-skey.c b/s390x/migration-skey.c
> > index b7bd82581abe..9b9a45f4ad3b 100644
> > --- a/s390x/migration-skey.c
> > +++ b/s390x/migration-skey.c
[...]
> > +/*
> > + * Verify storage keys on pagebuf.
> > + * Storage keys must have been set by skey_set_test_pattern 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.
> > + */
> > +static struct skey_verify_result skey_verify_test_pattern(uint8_t *pagebuf, unsigned long page_count, unsigned char seed)
> 
> this line is a little too long, even for the KVM unit tests; find a way
> to shorten it by a couple of bytes (it would look better to keep it on
> one line)
> 
> (rename pagebuf? use size_t or uint64_t instead of unsigned long? use
> uint8_t for seed? shorten the name of struct verify_result?)

Well since all these functions are static again, I will:
- remove the skey_ prefix
- get rid of the pagebuf argument and just use the global variable.
That makes everything nice and short.

[...]
> > +static void migrate_once(void)
> > +{
> > +     static bool migrated;
> > +
> > +     if (migrated)
> > +             return;
> > +
> > +     migrated = true;
> > +     puts("Please migrate me, then press return\n");
> > +     (void)getchar();
> >  }
> 
> you don't need this any more, since your migration patches are
> upstream now :)

Yes, rebased.

[...]
> > +static void print_usage(void)
> > +{
> > +     report_info("Usage: migration-skey [parallel|sequential]");
> > +}
> > +
> > +int main(int argc, char **argv)
> >  {
> >       report_prefix_push("migration-skey");
> >       if (test_facility(169)) {
> >               report_skip("storage key removal facility is active");
> > +             goto error;
> > +     }
> >  
> > -             /*
> > -              * If we just exit and don't ask migrate_cmd to migrate us, it
> > -              * will just hang forever. Hence, also ask for migration when we
> > -              * skip this test altogether.
> > -              */
> > -             puts("Please migrate me, then press return\n");
> > -             (void)getchar();
> > +     if (argc < 2) {
> > +             print_usage();
> > +             goto error;
> > +     }
> > +
> > +     if (!strcmp("parallel", argv[1])) {
> > +             test_skey_migration_parallel();
> > +     } else if (!strcmp("sequential", argv[1])) {
> > +             test_skey_migration_sequential();
> >       } else {
> > -             test_migration();
> > +             print_usage();
> 
> hmm I don't like this whole main.
> 
> I think the best approach would be to have some kind of separate
> parse_args function that will set some flags (or one flag, in this case)
> 
> then the main becomes just a bunch of if/else, something like this:
> 
> parse_args(argc, argv);
> if (test_facility(169))
>         skip
> else if (parallel_flag)
>         parallel();
> else
>         sequential();
> 
> also, default to sequential if there are no parameters, and use
> posix-style parameters (--parallel, --sequential)
> 
> only if you get an invalid parameter you can print the usage and fail
> optionally if you really want you can print the usage info when called
> without parameter and inform that the test will default to sequential
> 
> 
> one of these days I have to write an argument parsing library :)

Alright, done.