| Message ID | 20221011151433.886294-1-nrb@linux.ibm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | s390x: Add migration test for guest TOD clock | expand |
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,
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
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 >
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 > >
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.