[kvm-unit-tests PATCH v2 0/2] s390x: Add migration test for guest TOD clock

[kvm-unit-tests PATCH v2 0/2] s390x: Add migration test for guest TOD clock

[Nico Boehr]

v1->v2:
---
- remove unneeded include
- advance clock by 10 minutes instead of 1 minute (thanks Claudio)
- express get_clock_us() using stck() (thanks Claudio)

The guest TOD clock should be preserved on migration. Add a test to
verify that.

To reduce code duplication, move some of the time-related defined
from the sck test to the library.

Nico Boehr (2):
  lib/s390x: move TOD clock related functions to library
  s390x: add migration TOD clock test

 lib/s390x/asm/time.h  | 50 ++++++++++++++++++++++++++++++++++++++++++-
 s390x/Makefile        |  1 +
 s390x/migration-sck.c | 44 +++++++++++++++++++++++++++++++++++++
 s390x/sck.c           | 32 ---------------------------
 s390x/unittests.cfg   |  4 ++++
 5 files changed, 98 insertions(+), 33 deletions(-)
 create mode 100644 s390x/migration-sck.c

-- 
2.36.1

[kvm-unit-tests PATCH v2 1/2] lib/s390x: move TOD clock related functions to library

[Nico Boehr]

The TOD-clock related functions can be useful for other tests beside the
sck test, hence move them to the library.

While at it, add a wrapper for stckf and express get_clock_us() with
stck() to reduce code duplication.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 lib/s390x/asm/time.h | 50 +++++++++++++++++++++++++++++++++++++++++++-
 s390x/sck.c          | 32 ----------------------------
 2 files changed, 49 insertions(+), 33 deletions(-)

diff --git a/lib/s390x/asm/time.h b/lib/s390x/asm/time.h
index d8d91d68a667..c5e1797fd29e 100644
--- a/lib/s390x/asm/time.h
+++ b/lib/s390x/asm/time.h
@@ -18,11 +18,59 @@
 
 #define CPU_TIMER_SHIFT_US	S390_CLOCK_SHIFT_US
 
+static inline int sck(uint64_t *time)
+{
+	int cc;
+
+	asm volatile(
+		"	sck %[time]\n"
+		"	ipm %[cc]\n"
+		"	srl %[cc],28\n"
+		: [cc] "=d"(cc)
+		: [time] "Q"(*time)
+		: "cc"
+	);
+
+	return cc;
+}
+
+static inline int stck(uint64_t *time)
+{
+	int cc;
+
+	asm volatile(
+		"	stck %[time]\n"
+		"	ipm %[cc]\n"
+		"	srl %[cc],28\n"
+		: [cc] "=d" (cc), [time] "=Q" (*time)
+		:
+		: "cc", "memory"
+	);
+
+	return cc;
+}
+
+static inline int stckf(uint64_t *time)
+{
+	int cc;
+
+	asm volatile(
+		"	stckf %[time]\n"
+		"	ipm %[cc]\n"
+		"	srl %[cc],28\n"
+		: [cc] "=d" (cc), [time] "=Q" (*time)
+		:
+		: "cc", "memory"
+	);
+
+	return cc;
+}
+
 static inline uint64_t get_clock_us(void)
 {
 	uint64_t clk;
 
-	asm volatile(" stck %0 " : : "Q"(clk) : "memory");
+	stck(&clk);
 
 	return clk >> STCK_SHIFT_US;
 }
diff --git a/s390x/sck.c b/s390x/sck.c
index 88d52b74a586..dff496187602 100644
--- a/s390x/sck.c
+++ b/s390x/sck.c
@@ -12,38 +12,6 @@
 #include <asm/interrupt.h>
 #include <asm/time.h>
 
-static inline int sck(uint64_t *time)
-{
-	int cc;
-
-	asm volatile(
-		"	sck %[time]\n"
-		"	ipm %[cc]\n"
-		"	srl %[cc],28\n"
-		: [cc] "=d"(cc)
-		: [time] "Q"(*time)
-		: "cc"
-	);
-
-	return cc;
-}
-
-static inline int stck(uint64_t *time)
-{
-	int cc;
-
-	asm volatile(
-		"	stck %[time]\n"
-		"	ipm %[cc]\n"
-		"	srl %[cc],28\n"
-		: [cc] "=d" (cc), [time] "=Q" (*time)
-		:
-		: "cc", "memory"
-	);
-
-	return cc;
-}
-
 static void test_priv(void)
 {
 	uint64_t time_to_set_privileged = 0xfacef00dcafe0000,
-- 
2.36.1

Re: [kvm-unit-tests PATCH v2 1/2] lib/s390x: move TOD clock related functions to library

[Claudio Imbrenda]

On Tue, 11 Oct 2022 17:14:32 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> The TOD-clock related functions can be useful for other tests beside the
> sck test, hence move them to the library.
> 
> While at it, add a wrapper for stckf and express get_clock_us() with
> stck() to reduce code duplication.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>  lib/s390x/asm/time.h | 50 +++++++++++++++++++++++++++++++++++++++++++-
>  s390x/sck.c          | 32 ----------------------------
>  2 files changed, 49 insertions(+), 33 deletions(-)
> 
> diff --git a/lib/s390x/asm/time.h b/lib/s390x/asm/time.h
> index d8d91d68a667..c5e1797fd29e 100644
> --- a/lib/s390x/asm/time.h
> +++ b/lib/s390x/asm/time.h
> @@ -18,11 +18,59 @@
>  
>  #define CPU_TIMER_SHIFT_US	S390_CLOCK_SHIFT_US
>  
> +static inline int sck(uint64_t *time)
> +{
> +	int cc;
> +
> +	asm volatile(
> +		"	sck %[time]\n"
> +		"	ipm %[cc]\n"
> +		"	srl %[cc],28\n"
> +		: [cc] "=d"(cc)
> +		: [time] "Q"(*time)
> +		: "cc"
> +	);
> +
> +	return cc;
> +}
> +
> +static inline int stck(uint64_t *time)
> +{
> +	int cc;
> +
> +	asm volatile(
> +		"	stck %[time]\n"
> +		"	ipm %[cc]\n"
> +		"	srl %[cc],28\n"
> +		: [cc] "=d" (cc), [time] "=Q" (*time)
> +		:
> +		: "cc", "memory"

why do you need "memory" ?

(I know it was in the old code, but probably it should not have)

> +	);
> +
> +	return cc;
> +}
> +
> +static inline int stckf(uint64_t *time)
> +{
> +	int cc;
> +
> +	asm volatile(
> +		"	stckf %[time]\n"
> +		"	ipm %[cc]\n"
> +		"	srl %[cc],28\n"
> +		: [cc] "=d" (cc), [time] "=Q" (*time)
> +		:
> +		: "cc", "memory"

same here

> +	);
> +
> +	return cc;
> +}
> +
>  static inline uint64_t get_clock_us(void)
>  {
>  	uint64_t clk;
>  
> -	asm volatile(" stck %0 " : : "Q"(clk) : "memory");
> +	stck(&clk);
>  
>  	return clk >> STCK_SHIFT_US;
>  }
> diff --git a/s390x/sck.c b/s390x/sck.c
> index 88d52b74a586..dff496187602 100644
> --- a/s390x/sck.c
> +++ b/s390x/sck.c
> @@ -12,38 +12,6 @@
>  #include <asm/interrupt.h>
>  #include <asm/time.h>
>  
> -static inline int sck(uint64_t *time)
> -{
> -	int cc;
> -
> -	asm volatile(
> -		"	sck %[time]\n"
> -		"	ipm %[cc]\n"
> -		"	srl %[cc],28\n"
> -		: [cc] "=d"(cc)
> -		: [time] "Q"(*time)
> -		: "cc"
> -	);
> -
> -	return cc;
> -}
> -
> -static inline int stck(uint64_t *time)
> -{
> -	int cc;
> -
> -	asm volatile(
> -		"	stck %[time]\n"
> -		"	ipm %[cc]\n"
> -		"	srl %[cc],28\n"
> -		: [cc] "=d" (cc), [time] "=Q" (*time)
> -		:
> -		: "cc", "memory"
> -	);
> -
> -	return cc;
> -}
> -
>  static void test_priv(void)
>  {
>  	uint64_t time_to_set_privileged = 0xfacef00dcafe0000,

[kvm-unit-tests PATCH v2 2/2] s390x: add migration TOD clock test

[Nico Boehr]

On migration, we expect the guest clock value to be preserved. Add a
test to verify this:
- advance the guest TOD by much more than we need to migrate
- migrate the guest
- get the guest TOD

After migration, assert the guest TOD value is at least the value we set
before migration.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 s390x/Makefile        |  1 +
 s390x/migration-sck.c | 44 +++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg   |  4 ++++
 3 files changed, 49 insertions(+)
 create mode 100644 s390x/migration-sck.c

diff --git a/s390x/Makefile b/s390x/Makefile
index 649486f2d4a0..fba09bc2df3a 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -36,6 +36,7 @@ tests += $(TEST_DIR)/migration-cmm.elf
 tests += $(TEST_DIR)/migration-skey.elf
 tests += $(TEST_DIR)/panic-loop-extint.elf
 tests += $(TEST_DIR)/panic-loop-pgm.elf
+tests += $(TEST_DIR)/migration-sck.elf
 
 pv-tests += $(TEST_DIR)/pv-diags.elf
 
diff --git a/s390x/migration-sck.c b/s390x/migration-sck.c
new file mode 100644
index 000000000000..286a693b4858
--- /dev/null
+++ b/s390x/migration-sck.c
@@ -0,0 +1,44 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * SET CLOCK migration tests
+ *
+ * Copyright IBM Corp. 2022
+ *
+ * Authors:
+ *  Nico Boehr <nrb@linux.ibm.com>
+ */
+
+#include <libcflat.h>
+#include <asm/time.h>
+
+static void test_sck_migration(void)
+{
+	uint64_t now_before_set = 0, now_after_migration, time_to_set, time_to_advance;
+	int cc;
+
+	stckf(&now_before_set);
+
+	/* Advance the clock by a lot more than we might ever need to migrate (600s) */
+	time_to_advance = (600ULL * 1000000) << STCK_SHIFT_US;
+	time_to_set = now_before_set + time_to_advance;
+
+	cc = sck(&time_to_set);
+	report(!cc, "setting clock succeeded");
+
+	puts("Please migrate me, then press return\n");
+	(void)getchar();
+
+	cc = stckf(&now_after_migration);
+	report(!cc, "clock still set");
+
+	report(now_after_migration >= time_to_set, "TOD clock value preserved");
+}
+
+int main(void)
+{
+	report_prefix_push("migration-sck");
+
+	test_sck_migration();
+	report_prefix_pop();
+	return report_summary();
+}
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index f9f102abfa89..2c04ae7c7c15 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -197,3 +197,7 @@ file = panic-loop-pgm.elf
 groups = panic
 accel = kvm
 timeout = 5
+
+[migration-sck]
+file = migration-sck.elf
+groups = migration
-- 
2.36.1

Re: [kvm-unit-tests PATCH v2 2/2] s390x: add migration TOD clock test

[Claudio Imbrenda]

On Tue, 11 Oct 2022 17:14:33 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> On migration, we expect the guest clock value to be preserved. Add a
> test to verify this:
> - advance the guest TOD by much more than we need to migrate
> - migrate the guest
> - get the guest TOD
> 
> After migration, assert the guest TOD value is at least the value we set
> before migration.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>  s390x/Makefile        |  1 +
>  s390x/migration-sck.c | 44 +++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg   |  4 ++++
>  3 files changed, 49 insertions(+)
>  create mode 100644 s390x/migration-sck.c
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 649486f2d4a0..fba09bc2df3a 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -36,6 +36,7 @@ tests += $(TEST_DIR)/migration-cmm.elf
>  tests += $(TEST_DIR)/migration-skey.elf
>  tests += $(TEST_DIR)/panic-loop-extint.elf
>  tests += $(TEST_DIR)/panic-loop-pgm.elf
> +tests += $(TEST_DIR)/migration-sck.elf
>  
>  pv-tests += $(TEST_DIR)/pv-diags.elf
>  
> diff --git a/s390x/migration-sck.c b/s390x/migration-sck.c
> new file mode 100644
> index 000000000000..286a693b4858
> --- /dev/null
> +++ b/s390x/migration-sck.c
> @@ -0,0 +1,44 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * SET CLOCK migration tests
> + *
> + * Copyright IBM Corp. 2022
> + *
> + * Authors:
> + *  Nico Boehr <nrb@linux.ibm.com>
> + */
> +
> +#include <libcflat.h>
> +#include <asm/time.h>
> +
> +static void test_sck_migration(void)
> +{
> +	uint64_t now_before_set = 0, now_after_migration, time_to_set, time_to_advance;
> +	int cc;
> +
> +	stckf(&now_before_set);
> +
> +	/* Advance the clock by a lot more than we might ever need to migrate (600s) */
> +	time_to_advance = (600ULL * 1000000) << STCK_SHIFT_US;
> +	time_to_set = now_before_set + time_to_advance;
> +
> +	cc = sck(&time_to_set);
> +	report(!cc, "setting clock succeeded");

I would also check here that the clock is set. This way we can
differentiate between a migration issue or a more general (and more
severe) clock issue. Even if we have this test somewhere else, this
makes it easier when debugging to narrow down the issue to migration.

> +
> +	puts("Please migrate me, then press return\n");
> +	(void)getchar();
> +
> +	cc = stckf(&now_after_migration);
> +	report(!cc, "clock still set");
> +
> +	report(now_after_migration >= time_to_set, "TOD clock value preserved");
> +}
> +
> +int main(void)
> +{
> +	report_prefix_push("migration-sck");
> +
> +	test_sck_migration();
> +	report_prefix_pop();
> +	return report_summary();
> +}
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index f9f102abfa89..2c04ae7c7c15 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -197,3 +197,7 @@ file = panic-loop-pgm.elf
>  groups = panic
>  accel = kvm
>  timeout = 5
> +
> +[migration-sck]
> +file = migration-sck.elf
> +groups = migration

Re: [kvm-unit-tests PATCH v2 0/2] s390x: Add migration test for guest TOD clock

[Christian Borntraeger]

Am 11.10.22 um 17:14 schrieb Nico Boehr:
> v1->v2:
> ---
> - remove unneeded include
> - advance clock by 10 minutes instead of 1 minute (thanks Claudio)
> - express get_clock_us() using stck() (thanks Claudio)
> 
> The guest TOD clock should be preserved on migration. Add a test to
> verify that.

I do not fully agree with this assumption. Its the way it curently is, but we might want to have a configurable or different behaviour in the future.

For example if the difference is smaller than time x it could be allowed to move the time forward to get the guest synced to the new host (never go backward though).
Or to preserve the time but then slowly step towards the target system clock etc (or for this testcase step the epoch difference towards the original difference).

So we maybe want to have a comment in here somehow that this is the as-is behaviour.

> To reduce code duplication, move some of the time-related defined
> from the sck test to the library.
> 
> Nico Boehr (2):
>    lib/s390x: move TOD clock related functions to library
>    s390x: add migration TOD clock test
> 
>   lib/s390x/asm/time.h  | 50 ++++++++++++++++++++++++++++++++++++++++++-
>   s390x/Makefile        |  1 +
>   s390x/migration-sck.c | 44 +++++++++++++++++++++++++++++++++++++
>   s390x/sck.c           | 32 ---------------------------
>   s390x/unittests.cfg   |  4 ++++
>   5 files changed, 98 insertions(+), 33 deletions(-)
>   create mode 100644 s390x/migration-sck.c
> 

Re: [kvm-unit-tests PATCH v2 0/2] s390x: Add migration test for guest TOD clock

[Claudio Imbrenda]

On Tue, 11 Oct 2022 17:58:29 +0200
Christian Borntraeger <borntraeger@linux.ibm.com> wrote:

> Am 11.10.22 um 17:14 schrieb Nico Boehr:
> > v1->v2:
> > ---
> > - remove unneeded include
> > - advance clock by 10 minutes instead of 1 minute (thanks Claudio)
> > - express get_clock_us() using stck() (thanks Claudio)
> > 
> > The guest TOD clock should be preserved on migration. Add a test to
> > verify that.  
> 
> I do not fully agree with this assumption. Its the way it curently is, but we might want to have a configurable or different behaviour in the future.
> 
> For example if the difference is smaller than time x it could be allowed to move the time forward to get the guest synced to the new host (never go backward though).

the test is actually testing that the clock does not go backwards,
rather than staying the same

> Or to preserve the time but then slowly step towards the target system clock etc (or for this testcase step the epoch difference towards the original difference).
> 
> So we maybe want to have a comment in here somehow that this is the as-is behaviour.

comments never hurt :)

> 
> > To reduce code duplication, move some of the time-related defined
> > from the sck test to the library.
> > 
> > Nico Boehr (2):
> >    lib/s390x: move TOD clock related functions to library
> >    s390x: add migration TOD clock test
> > 
> >   lib/s390x/asm/time.h  | 50 ++++++++++++++++++++++++++++++++++++++++++-
> >   s390x/Makefile        |  1 +
> >   s390x/migration-sck.c | 44 +++++++++++++++++++++++++++++++++++++
> >   s390x/sck.c           | 32 ---------------------------
> >   s390x/unittests.cfg   |  4 ++++
> >   5 files changed, 98 insertions(+), 33 deletions(-)
> >   create mode 100644 s390x/migration-sck.c
> >   

Re: [kvm-unit-tests PATCH v2 0/2] s390x: Add migration test for guest TOD clock

[Christian Borntraeger]

Am 11.10.22 um 18:10 schrieb Claudio Imbrenda:
> On Tue, 11 Oct 2022 17:58:29 +0200
> Christian Borntraeger <borntraeger@linux.ibm.com> wrote:
> 
>> Am 11.10.22 um 17:14 schrieb Nico Boehr:
>>> v1->v2:
>>> ---
>>> - remove unneeded include
>>> - advance clock by 10 minutes instead of 1 minute (thanks Claudio)
>>> - express get_clock_us() using stck() (thanks Claudio)
>>>
>>> The guest TOD clock should be preserved on migration. Add a test to
>>> verify that.
>>
>> I do not fully agree with this assumption. Its the way it curently is, but we might want to have a configurable or different behaviour in the future.
>>
>> For example if the difference is smaller than time x it could be allowed to move the time forward to get the guest synced to the new host (never go backward though).
> 
> the test is actually testing that the clock does not go backwards,
> rather than staying the same

I think this is an must and a perfectly valid test.