[kvm-unit-tests PATCH v1 0/4] lib: add function to request migration

[kvm-unit-tests PATCH v1 0/4] lib: add function to request migration

[Nico Boehr]

With this series, I pick up a suggestion Claudio has brought up in my
CMM-migration series[1].

Migration tests can ask migrate_cmd to migrate them to a new QEMU
process. Requesting migration and waiting for completion is hence a
common pattern which is repeated all over the code base. Add a function
which does all of that to avoid repetition.

Since migrate_cmd currently can only migrate exactly once, this function
is called migrate_once() and is a no-op when it has been called before.
This can simplify the control flow, especially when tests are skipped.

[1] https://lore.kernel.org/kvm/20221125154646.5974cb52@p-imbrenda/

Nico Boehr (4):
  lib: add function to request migration
  powerpc: use migrate_once() in migration tests
  s390x: use migrate_once() in migration tests
  arm: use migrate_once() in migration tests

 arm/Makefile.common     |  1 +
 arm/debug.c             | 14 ++++--------
 arm/gic.c               | 49 ++++++++++++-----------------------------
 lib/migrate.c           | 34 ++++++++++++++++++++++++++++
 lib/migrate.h           |  9 ++++++++
 powerpc/Makefile.common |  1 +
 powerpc/sprs.c          |  4 ++--
 s390x/Makefile          |  1 +
 s390x/migration-cmm.c   | 27 ++++++++---------------
 s390x/migration-sck.c   |  4 ++--
 s390x/migration-skey.c  | 23 ++++++++-----------
 s390x/migration.c       |  7 ++----
 12 files changed, 88 insertions(+), 86 deletions(-)
 create mode 100644 lib/migrate.c
 create mode 100644 lib/migrate.h

-- 
2.36.1

[kvm-unit-tests PATCH v1 1/4] lib: add function to request migration

[Nico Boehr]

Migration tests can ask migrate_cmd to migrate them to a new QEMU
process. Requesting migration and waiting for completion is hence a
common pattern which is repeated all over the code base. Add a function
which does all of that to avoid repeating the same pattern.

Since migrate_cmd currently can only migrate exactly once, this function
is called migrate_once() and is a no-op when it has been called before.
This can simplify the control flow, especially when tests are skipped.

Suggested-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 lib/migrate.c | 34 ++++++++++++++++++++++++++++++++++
 lib/migrate.h |  9 +++++++++
 2 files changed, 43 insertions(+)
 create mode 100644 lib/migrate.c
 create mode 100644 lib/migrate.h

diff --git a/lib/migrate.c b/lib/migrate.c
new file mode 100644
index 000000000000..50e78fb08865
--- /dev/null
+++ b/lib/migrate.c
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Migration-related functions
+ *
+ * Copyright IBM Corp. 2022
+ * Author: Nico Boehr <nrb@linux.ibm.com>
+ */
+#include <libcflat.h>
+#include "migrate.h"
+
+/* static for now since we only support migrating exactly once per test. */
+static void migrate(void)
+{
+	puts("Please migrate me, then press return\n");
+	(void)getchar();
+	report_info("Migration complete");
+}
+
+/*
+ * Initiate migration and wait for it to complete.
+ * If this function is called more than once, it is a no-op.
+ * Since migrate_cmd can only migrate exactly once this function can
+ * simplify the control flow, especially when skipping tests.
+ */
+void migrate_once(void)
+{
+	static bool migrated;
+
+	if (migrated)
+		return;
+
+	migrated = true;
+	migrate();
+}
diff --git a/lib/migrate.h b/lib/migrate.h
new file mode 100644
index 000000000000..3c94e6af761c
--- /dev/null
+++ b/lib/migrate.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Migration-related functions
+ *
+ * Copyright IBM Corp. 2022
+ * Author: Nico Boehr <nrb@linux.ibm.com>
+ */
+
+void migrate_once(void);
-- 
2.36.1

Re: [kvm-unit-tests PATCH v1 1/4] lib: add function to request migration

[Claudio Imbrenda]

On Wed, 30 Nov 2022 15:22:46 +0100
Nico Boehr <nrb@linux.ibm.com> wrote:

> Migration tests can ask migrate_cmd to migrate them to a new QEMU
> process. Requesting migration and waiting for completion is hence a
> common pattern which is repeated all over the code base. Add a function
> which does all of that to avoid repeating the same pattern.
> 
> Since migrate_cmd currently can only migrate exactly once, this function
> is called migrate_once() and is a no-op when it has been called before.
> This can simplify the control flow, especially when tests are skipped.
> 
> Suggested-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>

I like the code, I only have one concern

> ---
>  lib/migrate.c | 34 ++++++++++++++++++++++++++++++++++
>  lib/migrate.h |  9 +++++++++
>  2 files changed, 43 insertions(+)
>  create mode 100644 lib/migrate.c
>  create mode 100644 lib/migrate.h
> 
> diff --git a/lib/migrate.c b/lib/migrate.c
> new file mode 100644
> index 000000000000..50e78fb08865
> --- /dev/null
> +++ b/lib/migrate.c
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Migration-related functions
> + *
> + * Copyright IBM Corp. 2022
> + * Author: Nico Boehr <nrb@linux.ibm.com>
> + */
> +#include <libcflat.h>
> +#include "migrate.h"
> +
> +/* static for now since we only support migrating exactly once per test. */
> +static void migrate(void)
> +{
> +	puts("Please migrate me, then press return\n");

the other architectures use a slightly different message, maybe we
should use that also on s390x?

In the end it _should_ not matter, since the migrate_cmd looks for one
specific keyword, but I still think we should try to minimize the
impact of this series. And I know that changing the migration message
will not break anything on s390x.

> +	(void)getchar();
> +	report_info("Migration complete");
> +}
> +
> +/*
> + * Initiate migration and wait for it to complete.
> + * If this function is called more than once, it is a no-op.
> + * Since migrate_cmd can only migrate exactly once this function can
> + * simplify the control flow, especially when skipping tests.
> + */
> +void migrate_once(void)
> +{
> +	static bool migrated;
> +
> +	if (migrated)
> +		return;
> +
> +	migrated = true;
> +	migrate();
> +}
> diff --git a/lib/migrate.h b/lib/migrate.h
> new file mode 100644
> index 000000000000..3c94e6af761c
> --- /dev/null
> +++ b/lib/migrate.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Migration-related functions
> + *
> + * Copyright IBM Corp. 2022
> + * Author: Nico Boehr <nrb@linux.ibm.com>
> + */
> +
> +void migrate_once(void);

Re: [kvm-unit-tests PATCH v1 1/4] lib: add function to request migration

[Nico Boehr]

Quoting Claudio Imbrenda (2022-11-30 18:30:06)
> On Wed, 30 Nov 2022 15:22:46 +0100
> Nico Boehr <nrb@linux.ibm.com> wrote:
[...]
> > diff --git a/lib/migrate.c b/lib/migrate.c
> > new file mode 100644
> > index 000000000000..50e78fb08865
> > --- /dev/null
> > +++ b/lib/migrate.c
> > @@ -0,0 +1,34 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Migration-related functions
> > + *
> > + * Copyright IBM Corp. 2022
> > + * Author: Nico Boehr <nrb@linux.ibm.com>
> > + */
> > +#include <libcflat.h>
> > +#include "migrate.h"
> > +
> > +/* static for now since we only support migrating exactly once per test. */
> > +static void migrate(void)
> > +{
> > +     puts("Please migrate me, then press return\n");
> 
> the other architectures use a slightly different message, maybe we
> should use that also on s390x?
> 
> In the end it _should_ not matter, since the migrate_cmd looks for one
> specific keyword, but I still think we should try to minimize the
> impact of this series. And I know that changing the migration message
> will not break anything on s390x.

Oh yes, that's a very good point. Changed.

[kvm-unit-tests PATCH v1 2/4] powerpc: use migrate_once() in migration tests

[Nico Boehr]

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 powerpc/Makefile.common | 1 +
 powerpc/sprs.c          | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
index 12c280c15fff..8ce00340b6be 100644
--- a/powerpc/Makefile.common
+++ b/powerpc/Makefile.common
@@ -36,6 +36,7 @@ cflatobjs += lib/getchar.o
 cflatobjs += lib/alloc_phys.o
 cflatobjs += lib/alloc.o
 cflatobjs += lib/devicetree.o
+cflatobjs += lib/migrate.o
 cflatobjs += lib/powerpc/io.o
 cflatobjs += lib/powerpc/hcall.o
 cflatobjs += lib/powerpc/setup.o
diff --git a/powerpc/sprs.c b/powerpc/sprs.c
index d3c8780e8376..5cc1cd16cfda 100644
--- a/powerpc/sprs.c
+++ b/powerpc/sprs.c
@@ -21,6 +21,7 @@
  */
 #include <libcflat.h>
 #include <util.h>
+#include <migrate.h>
 #include <alloc.h>
 #include <asm/handlers.h>
 #include <asm/hcall.h>
@@ -285,8 +286,7 @@ int main(int argc, char **argv)
 	get_sprs(before);
 
 	if (pause) {
-		puts("Now migrate the VM, then press a key to continue...\n");
-		(void) getchar();
+		migrate_once();
 	} else {
 		puts("Sleeping...\n");
 		handle_exception(0x900, &dec_except_handler, &decr);
-- 
2.36.1

Re: [kvm-unit-tests PATCH v1 2/4] powerpc: use migrate_once() in migration tests

[Thomas Huth]

On 30/11/2022 15.22, Nico Boehr wrote:
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>   powerpc/Makefile.common | 1 +
>   powerpc/sprs.c          | 4 ++--
>   2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
> index 12c280c15fff..8ce00340b6be 100644
> --- a/powerpc/Makefile.common
> +++ b/powerpc/Makefile.common
> @@ -36,6 +36,7 @@ cflatobjs += lib/getchar.o
>   cflatobjs += lib/alloc_phys.o
>   cflatobjs += lib/alloc.o
>   cflatobjs += lib/devicetree.o
> +cflatobjs += lib/migrate.o
>   cflatobjs += lib/powerpc/io.o
>   cflatobjs += lib/powerpc/hcall.o
>   cflatobjs += lib/powerpc/setup.o
> diff --git a/powerpc/sprs.c b/powerpc/sprs.c
> index d3c8780e8376..5cc1cd16cfda 100644
> --- a/powerpc/sprs.c
> +++ b/powerpc/sprs.c
> @@ -21,6 +21,7 @@
>    */
>   #include <libcflat.h>
>   #include <util.h>
> +#include <migrate.h>
>   #include <alloc.h>
>   #include <asm/handlers.h>
>   #include <asm/hcall.h>
> @@ -285,8 +286,7 @@ int main(int argc, char **argv)
>   	get_sprs(before);
>   
>   	if (pause) {
> -		puts("Now migrate the VM, then press a key to continue...\n");
> -		(void) getchar();
> +		migrate_once();
>   	} else {
>   		puts("Sleeping...\n");
>   		handle_exception(0x900, &dec_except_handler, &decr);

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

[kvm-unit-tests PATCH v1 3/4] s390x: use migrate_once() in migration tests

[Nico Boehr]

migrate_once() can simplify the control flow in migration-skey and
migration-cmm.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 s390x/Makefile         |  1 +
 s390x/migration-cmm.c  | 27 +++++++++------------------
 s390x/migration-sck.c  |  4 ++--
 s390x/migration-skey.c | 23 +++++++++--------------
 s390x/migration.c      |  7 ++-----
 5 files changed, 23 insertions(+), 39 deletions(-)

diff --git a/s390x/Makefile b/s390x/Makefile
index bf1504f9d58c..52a9d821974e 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -85,6 +85,7 @@ cflatobjs += lib/alloc_page.o
 cflatobjs += lib/vmalloc.o
 cflatobjs += lib/alloc_phys.o
 cflatobjs += lib/getchar.o
+cflatobjs += lib/migrate.o
 cflatobjs += lib/s390x/io.o
 cflatobjs += lib/s390x/stack.o
 cflatobjs += lib/s390x/sclp.o
diff --git a/s390x/migration-cmm.c b/s390x/migration-cmm.c
index aa7910ca76bf..64adc6ed9e14 100644
--- a/s390x/migration-cmm.c
+++ b/s390x/migration-cmm.c
@@ -9,6 +9,7 @@
  */
 
 #include <libcflat.h>
+#include <migrate.h>
 #include <asm/interrupt.h>
 #include <asm/page.h>
 #include <asm/cmm.h>
@@ -31,6 +32,11 @@ static void test_migration(void)
 		BIT(ESSA_USAGE_VOLATILE) | BIT(ESSA_USAGE_POT_VOLATILE) /* ESSA_SET_POT_VOLATILE */
 	};
 
+	if (!check_essa_available()) {
+		report_skip("ESSA is not available");
+		return;
+	}
+
 	assert(NUM_PAGES % 4 == 0);
 	for (i = 0; i < NUM_PAGES; i += 4) {
 		essa(ESSA_SET_STABLE, (unsigned long)pagebuf[i]);
@@ -39,8 +45,7 @@ static void test_migration(void)
 		essa(ESSA_SET_POT_VOLATILE, (unsigned long)pagebuf[i + 3]);
 	}
 
-	puts("Please migrate me, then press return\n");
-	(void)getchar();
+	migrate_once();
 
 	for (i = 0; i < NUM_PAGES; i++) {
 		actual_state = essa(ESSA_GET_STATE, (unsigned long)pagebuf[i]);
@@ -53,25 +58,11 @@ static void test_migration(void)
 
 int main(void)
 {
-	bool has_essa = check_essa_available();
-
 	report_prefix_push("migration-cmm");
-	if (!has_essa) {
-		report_skip("ESSA is not available");
-
-		/*
-		 * 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 alltogether.
-		 */
-		puts("Please migrate me, then press return\n");
-		(void)getchar();
-
-		goto done;
-	}
 
 	test_migration();
-done:
+	migrate_once();
+
 	report_prefix_pop();
 	return report_summary();
 }
diff --git a/s390x/migration-sck.c b/s390x/migration-sck.c
index 2d9a195ab4c4..2a9c87071643 100644
--- a/s390x/migration-sck.c
+++ b/s390x/migration-sck.c
@@ -9,6 +9,7 @@
  */
 
 #include <libcflat.h>
+#include <migrate.h>
 #include <asm/time.h>
 
 static void test_sck_migration(void)
@@ -30,8 +31,7 @@ static void test_sck_migration(void)
 	report(!cc, "clock running after set");
 	report(now_after_set >= time_to_set, "TOD clock value is larger than what has been set");
 
-	puts("Please migrate me, then press return\n");
-	(void)getchar();
+	migrate_once();
 
 	cc = stckf(&now_after_migration);
 	report(!cc, "clock still set");
diff --git a/s390x/migration-skey.c b/s390x/migration-skey.c
index b7bd82581abe..a655d30e8312 100644
--- a/s390x/migration-skey.c
+++ b/s390x/migration-skey.c
@@ -9,6 +9,7 @@
  */
 
 #include <libcflat.h>
+#include <migrate.h>
 #include <asm/facility.h>
 #include <asm/page.h>
 #include <asm/mem.h>
@@ -23,6 +24,11 @@ static void test_migration(void)
 	union skey expected_key, actual_key;
 	int i, key_to_set, key_mismatches = 0;
 
+	if (test_facility(169)) {
+		report_skip("storage key removal facility is active");
+		return;
+	}
+
 	for (i = 0; i < NUM_PAGES; i++) {
 		/*
 		 * Storage keys are 7 bit, lowest bit is always returned as zero
@@ -35,8 +41,7 @@ static void test_migration(void)
 		set_storage_key(pagebuf[i], key_to_set, 1);
 	}
 
-	puts("Please migrate me, then press return\n");
-	(void)getchar();
+	migrate_once();
 
 	for (i = 0; i < NUM_PAGES; i++) {
 		actual_key.val = get_storage_key(pagebuf[i]);
@@ -64,19 +69,9 @@ static void test_migration(void)
 int main(void)
 {
 	report_prefix_push("migration-skey");
-	if (test_facility(169)) {
-		report_skip("storage key removal facility is active");
 
-		/*
-		 * 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();
-	} else {
-		test_migration();
-	}
+	test_migration();
+	migrate_once();
 
 	report_prefix_pop();
 	return report_summary();
diff --git a/s390x/migration.c b/s390x/migration.c
index a45296374cd8..fe6ea8369edb 100644
--- a/s390x/migration.c
+++ b/s390x/migration.c
@@ -8,6 +8,7 @@
  *  Nico Boehr <nrb@linux.ibm.com>
  */
 #include <libcflat.h>
+#include <migrate.h>
 #include <asm/arch_def.h>
 #include <asm/vector.h>
 #include <asm/barrier.h>
@@ -178,11 +179,7 @@ int main(void)
 		mb();
 	flag_thread_complete = 0;
 
-	/* ask migrate_cmd to migrate (it listens for 'migrate') */
-	puts("Please migrate me, then press return\n");
-
-	/* wait for migration to finish, we will read a newline */
-	(void)getchar();
+	migrate_once();
 
 	flag_migration_complete = 1;
 
-- 
2.36.1

Re: [kvm-unit-tests PATCH v1 3/4] s390x: use migrate_once() in migration tests

[Claudio Imbrenda]

On Wed, 30 Nov 2022 15:22:48 +0100
Nico Boehr <nrb@linux.ibm.com> wrote:

> migrate_once() can simplify the control flow in migration-skey and
> migration-cmm.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>

looks good in general, but maybe it can be made a little less intrusive
(see below)

> ---
>  s390x/Makefile         |  1 +
>  s390x/migration-cmm.c  | 27 +++++++++------------------
>  s390x/migration-sck.c  |  4 ++--
>  s390x/migration-skey.c | 23 +++++++++--------------
>  s390x/migration.c      |  7 ++-----
>  5 files changed, 23 insertions(+), 39 deletions(-)
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index bf1504f9d58c..52a9d821974e 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -85,6 +85,7 @@ cflatobjs += lib/alloc_page.o
>  cflatobjs += lib/vmalloc.o
>  cflatobjs += lib/alloc_phys.o
>  cflatobjs += lib/getchar.o
> +cflatobjs += lib/migrate.o
>  cflatobjs += lib/s390x/io.o
>  cflatobjs += lib/s390x/stack.o
>  cflatobjs += lib/s390x/sclp.o
> diff --git a/s390x/migration-cmm.c b/s390x/migration-cmm.c
> index aa7910ca76bf..64adc6ed9e14 100644
> --- a/s390x/migration-cmm.c
> +++ b/s390x/migration-cmm.c
> @@ -9,6 +9,7 @@
>   */
>  
>  #include <libcflat.h>
> +#include <migrate.h>
>  #include <asm/interrupt.h>
>  #include <asm/page.h>
>  #include <asm/cmm.h>
> @@ -31,6 +32,11 @@ static void test_migration(void)
>  		BIT(ESSA_USAGE_VOLATILE) | BIT(ESSA_USAGE_POT_VOLATILE) /* ESSA_SET_POT_VOLATILE */
>  	};
>  
> +	if (!check_essa_available()) {
> +		report_skip("ESSA is not available");
> +		return;
> +	}

not here ^

> +
>  	assert(NUM_PAGES % 4 == 0);
>  	for (i = 0; i < NUM_PAGES; i += 4) {
>  		essa(ESSA_SET_STABLE, (unsigned long)pagebuf[i]);
> @@ -39,8 +45,7 @@ static void test_migration(void)
>  		essa(ESSA_SET_POT_VOLATILE, (unsigned long)pagebuf[i + 3]);
>  	}
>  
> -	puts("Please migrate me, then press return\n");
> -	(void)getchar();
> +	migrate_once();
>  
>  	for (i = 0; i < NUM_PAGES; i++) {
>  		actual_state = essa(ESSA_GET_STATE, (unsigned long)pagebuf[i]);
> @@ -53,25 +58,11 @@ static void test_migration(void)
>  
>  int main(void)
>  {
> -	bool has_essa = check_essa_available();
> -
>  	report_prefix_push("migration-cmm");
> -	if (!has_essa) {

keep this ^

> -		report_skip("ESSA is not available");

keep this ^

else
	test_migration();

> -
> -		/*
> -		 * 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 alltogether.
> -		 */
> -		puts("Please migrate me, then press return\n");
> -		(void)getchar();
> -
> -		goto done;
> -	}
>  
>  	test_migration();
> -done:

remove all the above ^^

> +	migrate_once();
> +
>  	report_prefix_pop();
>  	return report_summary();
>  }
> diff --git a/s390x/migration-sck.c b/s390x/migration-sck.c
> index 2d9a195ab4c4..2a9c87071643 100644
> --- a/s390x/migration-sck.c
> +++ b/s390x/migration-sck.c
> @@ -9,6 +9,7 @@
>   */
>  
>  #include <libcflat.h>
> +#include <migrate.h>
>  #include <asm/time.h>
>  
>  static void test_sck_migration(void)
> @@ -30,8 +31,7 @@ static void test_sck_migration(void)
>  	report(!cc, "clock running after set");
>  	report(now_after_set >= time_to_set, "TOD clock value is larger than what has been set");
>  
> -	puts("Please migrate me, then press return\n");
> -	(void)getchar();
> +	migrate_once();
>  
>  	cc = stckf(&now_after_migration);
>  	report(!cc, "clock still set");
> diff --git a/s390x/migration-skey.c b/s390x/migration-skey.c
> index b7bd82581abe..a655d30e8312 100644
> --- a/s390x/migration-skey.c
> +++ b/s390x/migration-skey.c
> @@ -9,6 +9,7 @@
>   */
>  
>  #include <libcflat.h>
> +#include <migrate.h>
>  #include <asm/facility.h>
>  #include <asm/page.h>
>  #include <asm/mem.h>
> @@ -23,6 +24,11 @@ static void test_migration(void)
>  	union skey expected_key, actual_key;
>  	int i, key_to_set, key_mismatches = 0;
>  
> +	if (test_facility(169)) {
> +		report_skip("storage key removal facility is active");
> +		return;
> +	}

same here

> +
>  	for (i = 0; i < NUM_PAGES; i++) {
>  		/*
>  		 * Storage keys are 7 bit, lowest bit is always returned as zero
> @@ -35,8 +41,7 @@ static void test_migration(void)
>  		set_storage_key(pagebuf[i], key_to_set, 1);
>  	}
>  
> -	puts("Please migrate me, then press return\n");
> -	(void)getchar();
> +	migrate_once();
>  
>  	for (i = 0; i < NUM_PAGES; i++) {
>  		actual_key.val = get_storage_key(pagebuf[i]);
> @@ -64,19 +69,9 @@ static void test_migration(void)
>  int main(void)
>  {
>  	report_prefix_push("migration-skey");
> -	if (test_facility(169)) {
> -		report_skip("storage key removal facility is active");

keep the if ^

>  
> -		/*
> -		 * 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();

remove that ^

> -	} else {
> -		test_migration();

keep the else ^

> -	}
> +	test_migration();

not needed anymore ^

> +	migrate_once();
>  
>  	report_prefix_pop();
>  	return report_summary();
> diff --git a/s390x/migration.c b/s390x/migration.c
> index a45296374cd8..fe6ea8369edb 100644
> --- a/s390x/migration.c
> +++ b/s390x/migration.c
> @@ -8,6 +8,7 @@
>   *  Nico Boehr <nrb@linux.ibm.com>
>   */
>  #include <libcflat.h>
> +#include <migrate.h>
>  #include <asm/arch_def.h>
>  #include <asm/vector.h>
>  #include <asm/barrier.h>
> @@ -178,11 +179,7 @@ int main(void)
>  		mb();
>  	flag_thread_complete = 0;
>  
> -	/* ask migrate_cmd to migrate (it listens for 'migrate') */
> -	puts("Please migrate me, then press return\n");
> -
> -	/* wait for migration to finish, we will read a newline */
> -	(void)getchar();
> +	migrate_once();
>  
>  	flag_migration_complete = 1;
>  

[kvm-unit-tests PATCH v1 4/4] arm: use migrate_once() in migration tests

[Nico Boehr]

Some tests already shipped with their own do_migrate() function, remove
it and instead use the new migrate_once() function. The control flow in
the gib tests can be simplified due to migrate_once().

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 arm/Makefile.common |  1 +
 arm/debug.c         | 14 ++++---------
 arm/gic.c           | 49 +++++++++++++--------------------------------
 3 files changed, 19 insertions(+), 45 deletions(-)

diff --git a/arm/Makefile.common b/arm/Makefile.common
index 38385e0c558e..1bbec64f2342 100644
--- a/arm/Makefile.common
+++ b/arm/Makefile.common
@@ -38,6 +38,7 @@ cflatobjs += lib/alloc_page.o
 cflatobjs += lib/vmalloc.o
 cflatobjs += lib/alloc.o
 cflatobjs += lib/devicetree.o
+cflatobjs += lib/migrate.o
 cflatobjs += lib/pci.o
 cflatobjs += lib/pci-host-generic.o
 cflatobjs += lib/pci-testdev.o
diff --git a/arm/debug.c b/arm/debug.c
index e9f805632db7..21b0f5aeb590 100644
--- a/arm/debug.c
+++ b/arm/debug.c
@@ -1,4 +1,5 @@
 #include <libcflat.h>
+#include <migrate.h>
 #include <errata.h>
 #include <asm/setup.h>
 #include <asm/processor.h>
@@ -257,13 +258,6 @@ static void reset_debug_state(void)
 	isb();
 }
 
-static void do_migrate(void)
-{
-	puts("Now migrate the VM, then press a key to continue...\n");
-	(void)getchar();
-	report_info("Migration complete");
-}
-
 static noinline void test_hw_bp(bool migrate)
 {
 	extern unsigned char hw_bp0;
@@ -291,7 +285,7 @@ static noinline void test_hw_bp(bool migrate)
 	isb();
 
 	if (migrate) {
-		do_migrate();
+		migrate_once();
 		report(num_bp == get_num_hw_bp(), "brps match after migrate");
 	}
 
@@ -335,7 +329,7 @@ static noinline void test_wp(bool migrate)
 	isb();
 
 	if (migrate) {
-		do_migrate();
+		migrate_once();
 		report(num_wp == get_num_wp(), "wrps match after migrate");
 	}
 
@@ -369,7 +363,7 @@ static noinline void test_ss(bool migrate)
 	isb();
 
 	if (migrate) {
-		do_migrate();
+		migrate_once();
 	}
 
 	asm volatile("msr daifclr, #8");
diff --git a/arm/gic.c b/arm/gic.c
index 60457e29e73a..c950b0d1597c 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -12,6 +12,7 @@
  * This work is licensed under the terms of the GNU LGPL, version 2.
  */
 #include <libcflat.h>
+#include <migrate.h>
 #include <errata.h>
 #include <asm/setup.h>
 #include <asm/processor.h>
@@ -779,23 +780,15 @@ static void test_its_trigger(void)
 static void test_its_migration(void)
 {
 	struct its_device *dev2, *dev7;
-	bool test_skipped = false;
 	cpumask_t mask;
 
-	if (its_setup1()) {
-		test_skipped = true;
-		goto do_migrate;
-	}
+	if (its_setup1())
+		return;
 
 	dev2 = its_get_device(2);
 	dev7 = its_get_device(7);
 
-do_migrate:
-	puts("Now migrate the VM, then press a key to continue...\n");
-	(void)getchar();
-	report_info("Migration complete");
-	if (test_skipped)
-		return;
+	migrate_once();
 
 	stats_reset();
 	cpumask_clear(&mask);
@@ -822,21 +815,17 @@ static void test_migrate_unmapped_collection(void)
 {
 	struct its_collection *col = NULL;
 	struct its_device *dev2 = NULL, *dev7 = NULL;
-	bool test_skipped = false;
 	cpumask_t mask;
 	int pe0 = 0;
 	u8 config;
 
-	if (its_setup1()) {
-		test_skipped = true;
-		goto do_migrate;
-	}
+	if (its_setup1())
+		return;
 
 	if (!errata(ERRATA_UNMAPPED_COLLECTIONS)) {
 		report_skip("Skipping test, as this test hangs without the fix. "
 			    "Set %s=y to enable.", ERRATA_UNMAPPED_COLLECTIONS);
-		test_skipped = true;
-		goto do_migrate;
+		return;
 	}
 
 	col = its_create_collection(pe0, pe0);
@@ -847,12 +836,7 @@ static void test_migrate_unmapped_collection(void)
 	its_send_mapti(dev2, 8192, 0, col);
 	gicv3_lpi_set_config(8192, LPI_PROP_DEFAULT);
 
-do_migrate:
-	puts("Now migrate the VM, then press a key to continue...\n");
-	(void)getchar();
-	report_info("Migration complete");
-	if (test_skipped)
-		return;
+	migrate_once();
 
 	/* on the destination, map the collection */
 	its_send_mapc(col, true);
@@ -887,15 +871,12 @@ static void test_its_pending_migration(void)
 	struct its_collection *collection[2];
 	int *expected = calloc(nr_cpus, sizeof(int));
 	int pe0 = nr_cpus - 1, pe1 = nr_cpus - 2;
-	bool test_skipped = false;
 	u64 pendbaser;
 	void *ptr;
 	int i;
 
-	if (its_prerequisites(4)) {
-		test_skipped = true;
-		goto do_migrate;
-	}
+	if (its_prerequisites(4))
+		return;
 
 	dev = its_create_device(2 /* dev id */, 8 /* nb_ites */);
 	its_send_mapd(dev, true);
@@ -942,12 +923,7 @@ static void test_its_pending_migration(void)
 	gicv3_lpi_rdist_enable(pe0);
 	gicv3_lpi_rdist_enable(pe1);
 
-do_migrate:
-	puts("Now migrate the VM, then press a key to continue...\n");
-	(void)getchar();
-	report_info("Migration complete");
-	if (test_skipped)
-		return;
+	migrate_once();
 
 	/* let's wait for the 256 LPIs to be handled */
 	mdelay(1000);
@@ -994,14 +970,17 @@ int main(int argc, char **argv)
 	} else if (!strcmp(argv[1], "its-migration")) {
 		report_prefix_push(argv[1]);
 		test_its_migration();
+		migrate_once();
 		report_prefix_pop();
 	} else if (!strcmp(argv[1], "its-pending-migration")) {
 		report_prefix_push(argv[1]);
 		test_its_pending_migration();
+		migrate_once();
 		report_prefix_pop();
 	} else if (!strcmp(argv[1], "its-migrate-unmapped-collection")) {
 		report_prefix_push(argv[1]);
 		test_migrate_unmapped_collection();
+		migrate_once();
 		report_prefix_pop();
 	} else if (strcmp(argv[1], "its-introspection") == 0) {
 		report_prefix_push(argv[1]);
-- 
2.36.1

Re: [kvm-unit-tests PATCH v1 4/4] arm: use migrate_once() in migration tests

[Andrew Jones]

On Wed, Nov 30, 2022 at 03:22:49PM +0100, Nico Boehr wrote:
> Some tests already shipped with their own do_migrate() function, remove
> it and instead use the new migrate_once() function. The control flow in
> the gib tests can be simplified due to migrate_once().

s/gib/gic/

> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>  arm/Makefile.common |  1 +
>  arm/debug.c         | 14 ++++---------
>  arm/gic.c           | 49 +++++++++++++--------------------------------
>  3 files changed, 19 insertions(+), 45 deletions(-)
> 
> diff --git a/arm/Makefile.common b/arm/Makefile.common
> index 38385e0c558e..1bbec64f2342 100644
> --- a/arm/Makefile.common
> +++ b/arm/Makefile.common
> @@ -38,6 +38,7 @@ cflatobjs += lib/alloc_page.o
>  cflatobjs += lib/vmalloc.o
>  cflatobjs += lib/alloc.o
>  cflatobjs += lib/devicetree.o
> +cflatobjs += lib/migrate.o
>  cflatobjs += lib/pci.o
>  cflatobjs += lib/pci-host-generic.o
>  cflatobjs += lib/pci-testdev.o
> diff --git a/arm/debug.c b/arm/debug.c
> index e9f805632db7..21b0f5aeb590 100644
> --- a/arm/debug.c
> +++ b/arm/debug.c
> @@ -1,4 +1,5 @@
>  #include <libcflat.h>
> +#include <migrate.h>
>  #include <errata.h>
>  #include <asm/setup.h>
>  #include <asm/processor.h>
> @@ -257,13 +258,6 @@ static void reset_debug_state(void)
>  	isb();
>  }
>  
> -static void do_migrate(void)
> -{
> -	puts("Now migrate the VM, then press a key to continue...\n");
> -	(void)getchar();
> -	report_info("Migration complete");
> -}
> -
>  static noinline void test_hw_bp(bool migrate)
>  {
>  	extern unsigned char hw_bp0;
> @@ -291,7 +285,7 @@ static noinline void test_hw_bp(bool migrate)
>  	isb();
>  
>  	if (migrate) {
> -		do_migrate();
> +		migrate_once();
>  		report(num_bp == get_num_hw_bp(), "brps match after migrate");
>  	}
>  
> @@ -335,7 +329,7 @@ static noinline void test_wp(bool migrate)
>  	isb();
>  
>  	if (migrate) {
> -		do_migrate();
> +		migrate_once();
>  		report(num_wp == get_num_wp(), "wrps match after migrate");
>  	}
>  
> @@ -369,7 +363,7 @@ static noinline void test_ss(bool migrate)
>  	isb();
>  
>  	if (migrate) {
> -		do_migrate();
> +		migrate_once();
>  	}

While here, please opportunistically drop the unnecessary {}

>  
>  	asm volatile("msr daifclr, #8");
> diff --git a/arm/gic.c b/arm/gic.c
> index 60457e29e73a..c950b0d1597c 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -12,6 +12,7 @@
>   * This work is licensed under the terms of the GNU LGPL, version 2.
>   */
>  #include <libcflat.h>
> +#include <migrate.h>
>  #include <errata.h>
>  #include <asm/setup.h>
>  #include <asm/processor.h>
> @@ -779,23 +780,15 @@ static void test_its_trigger(void)
>  static void test_its_migration(void)
>  {
>  	struct its_device *dev2, *dev7;
> -	bool test_skipped = false;
>  	cpumask_t mask;
>  
> -	if (its_setup1()) {
> -		test_skipped = true;
> -		goto do_migrate;
> -	}
> +	if (its_setup1())
> +		return;
>  
>  	dev2 = its_get_device(2);
>  	dev7 = its_get_device(7);
>  
> -do_migrate:
> -	puts("Now migrate the VM, then press a key to continue...\n");
> -	(void)getchar();
> -	report_info("Migration complete");
> -	if (test_skipped)
> -		return;
> +	migrate_once();
>  
>  	stats_reset();
>  	cpumask_clear(&mask);
> @@ -822,21 +815,17 @@ static void test_migrate_unmapped_collection(void)
>  {
>  	struct its_collection *col = NULL;
>  	struct its_device *dev2 = NULL, *dev7 = NULL;
> -	bool test_skipped = false;
>  	cpumask_t mask;
>  	int pe0 = 0;
>  	u8 config;
>  
> -	if (its_setup1()) {
> -		test_skipped = true;
> -		goto do_migrate;
> -	}
> +	if (its_setup1())
> +		return;
>  
>  	if (!errata(ERRATA_UNMAPPED_COLLECTIONS)) {
>  		report_skip("Skipping test, as this test hangs without the fix. "
>  			    "Set %s=y to enable.", ERRATA_UNMAPPED_COLLECTIONS);
> -		test_skipped = true;
> -		goto do_migrate;
> +		return;
>  	}
>  
>  	col = its_create_collection(pe0, pe0);
> @@ -847,12 +836,7 @@ static void test_migrate_unmapped_collection(void)
>  	its_send_mapti(dev2, 8192, 0, col);
>  	gicv3_lpi_set_config(8192, LPI_PROP_DEFAULT);
>  
> -do_migrate:
> -	puts("Now migrate the VM, then press a key to continue...\n");
> -	(void)getchar();
> -	report_info("Migration complete");
> -	if (test_skipped)
> -		return;
> +	migrate_once();
>  
>  	/* on the destination, map the collection */
>  	its_send_mapc(col, true);
> @@ -887,15 +871,12 @@ static void test_its_pending_migration(void)
>  	struct its_collection *collection[2];
>  	int *expected = calloc(nr_cpus, sizeof(int));
>  	int pe0 = nr_cpus - 1, pe1 = nr_cpus - 2;
> -	bool test_skipped = false;
>  	u64 pendbaser;
>  	void *ptr;
>  	int i;
>  
> -	if (its_prerequisites(4)) {
> -		test_skipped = true;
> -		goto do_migrate;
> -	}
> +	if (its_prerequisites(4))
> +		return;
>  
>  	dev = its_create_device(2 /* dev id */, 8 /* nb_ites */);
>  	its_send_mapd(dev, true);
> @@ -942,12 +923,7 @@ static void test_its_pending_migration(void)
>  	gicv3_lpi_rdist_enable(pe0);
>  	gicv3_lpi_rdist_enable(pe1);
>  
> -do_migrate:
> -	puts("Now migrate the VM, then press a key to continue...\n");
> -	(void)getchar();
> -	report_info("Migration complete");
> -	if (test_skipped)
> -		return;
> +	migrate_once();
>  
>  	/* let's wait for the 256 LPIs to be handled */
>  	mdelay(1000);
> @@ -994,14 +970,17 @@ int main(int argc, char **argv)
>  	} else if (!strcmp(argv[1], "its-migration")) {
>  		report_prefix_push(argv[1]);
>  		test_its_migration();
> +		migrate_once();
>  		report_prefix_pop();
>  	} else if (!strcmp(argv[1], "its-pending-migration")) {
>  		report_prefix_push(argv[1]);
>  		test_its_pending_migration();
> +		migrate_once();
>  		report_prefix_pop();
>  	} else if (!strcmp(argv[1], "its-migrate-unmapped-collection")) {
>  		report_prefix_push(argv[1]);
>  		test_migrate_unmapped_collection();
> +		migrate_once();
>  		report_prefix_pop();
>  	} else if (strcmp(argv[1], "its-introspection") == 0) {
>  		report_prefix_push(argv[1]);
> -- 
> 2.36.1
>

Reviewed-by: Andrew Jones <andrew.jones@linux.dev>

Thanks,
drew

Re: [kvm-unit-tests PATCH v1 4/4] arm: use migrate_once() in migration tests

[Nico Boehr]

Quoting Andrew Jones (2022-12-02 12:56:58)
> On Wed, Nov 30, 2022 at 03:22:49PM +0100, Nico Boehr wrote:
> > Some tests already shipped with their own do_migrate() function, remove
> > it and instead use the new migrate_once() function. The control flow in
> > the gib tests can be simplified due to migrate_once().
> 
> s/gib/gic/

Opsie, thanks :)

[...]
> > diff --git a/arm/debug.c b/arm/debug.c
> > index e9f805632db7..21b0f5aeb590 100644
> > --- a/arm/debug.c
> > +++ b/arm/debug.c
[...]
> > @@ -369,7 +363,7 @@ static noinline void test_ss(bool migrate)
> >       isb();
> >  
> >       if (migrate) {
> > -             do_migrate();
> > +             migrate_once();
> >       }
> 
> While here, please opportunistically drop the unnecessary {}

Done.

[...]
> Reviewed-by: Andrew Jones <andrew.jones@linux.dev>

Thanks.