| Message ID | 20200114153054.77082-4-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: smp: Improve smp code and reset checks | expand |
On 14/01/2020 16.30, Janosch Frank wrote: > All CRs are set to 0 and CRs 0 and 14 are set to pre-defined values, > so we also need to test 1-13 and 15 for 0. > > And while we're at it, let's also set some values to cr 1, 7 and 13, so > we can actually be sure that they will be zeroed. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > s390x/smp.c | 21 ++++++++++++++++++++- > 1 file changed, 20 insertions(+), 1 deletion(-) > > diff --git a/s390x/smp.c b/s390x/smp.c > index 767d167..11ab425 100644 > --- a/s390x/smp.c > +++ b/s390x/smp.c > @@ -175,16 +175,31 @@ static void test_emcall(void) > report_prefix_pop(); > } > > +static void test_func_initial(void) > +{ > + lctlg(1, 0x42000UL); > + lctlg(7, 0x43000UL); > + lctlg(13, 0x44000UL); > + testflag = 1; > + mb(); > + cpu_loop(); > +} > + > static void test_reset_initial(void) > { > struct cpu_status *status = alloc_pages(0); > + uint8_t *nullp = alloc_pages(0); Why not simply: uint64_t nullp[12]; ? > struct psw psw; > > + memset(nullp, 0, PAGE_SIZE); > psw.mask = extract_psw_mask(); > - psw.addr = (unsigned long)test_func; > + psw.addr = (unsigned long)test_func_initial; > > report_prefix_push("reset initial"); > + testflag = 0; > + mb(); > smp_cpu_start(1, psw); > + wait_for_flag(); > > sigp_retry(1, SIGP_INITIAL_CPU_RESET, 0, NULL); > sigp(1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, NULL); > @@ -195,6 +210,8 @@ static void test_reset_initial(void) > report(!status->fpc, "fpc"); > report(!status->cputm, "cpu timer"); > report(!status->todpr, "todpr"); > + report(!memcmp(&status->crs[1], nullp, sizeof(status->crs[1]) * 12), "cr1-13 == 0"); > + report(status->crs[15] == 0, "cr15 == 0"); > report_prefix_pop(); > > report_prefix_push("initialized"); > @@ -204,6 +221,7 @@ static void test_reset_initial(void) > > report(smp_cpu_stopped(1), "cpu stopped"); > free_pages(status, PAGE_SIZE); > + free_pages(nullp, PAGE_SIZE); > report_prefix_pop(); > } > > @@ -219,6 +237,7 @@ static void test_reset(void) > > sigp_retry(1, SIGP_CPU_RESET, 0, NULL); > report(smp_cpu_stopped(1), "cpu stopped"); > + smp_cpu_destroy(1); Shouldn't that rather be part of patch 2/4 ? I'd maybe also move this to the main() function instead since you've setup the cpu there...? Also is it still ok to use smp_cpu_start() in test_reset_initial() after you've destroyed the CPU here in test_reset()? > report_prefix_pop(); > } Thomas
On Tue, 14 Jan 2020 18:01:32 +0100 Thomas Huth <thuth@redhat.com> wrote: > On 14/01/2020 16.30, Janosch Frank wrote: > > All CRs are set to 0 and CRs 0 and 14 are set to pre-defined values, > > so we also need to test 1-13 and 15 for 0. > > > > And while we're at it, let's also set some values to cr 1, 7 and 13, so > > we can actually be sure that they will be zeroed. > > > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > > --- > > s390x/smp.c | 21 ++++++++++++++++++++- > > 1 file changed, 20 insertions(+), 1 deletion(-) > > @@ -219,6 +237,7 @@ static void test_reset(void) > > > > sigp_retry(1, SIGP_CPU_RESET, 0, NULL); > > report(smp_cpu_stopped(1), "cpu stopped"); > > + smp_cpu_destroy(1); > > Shouldn't that rather be part of patch 2/4 ? I'd maybe also move this to > the main() function instead since you've setup the cpu there...? Also is > it still ok to use smp_cpu_start() in test_reset_initial() after you've > destroyed the CPU here in test_reset()? Isn't it simply wrong? I thought the pattern was supposed to be - setup cpu - do some tests, including stopping/restarting/etc. - destroy cpu [currently missing] > > > report_prefix_pop(); > > } > > Thomas
On 14.01.20 16:30, Janosch Frank wrote: > All CRs are set to 0 and CRs 0 and 14 are set to pre-defined values, > so we also need to test 1-13 and 15 for 0. > > And while we're at it, let's also set some values to cr 1, 7 and 13, so > we can actually be sure that they will be zeroed. While it does not hurt to have it here, I think the register check for the reset would be better in a kselftest. This allows to check userspace AND guest at the same time.
On 14/01/2020 19.42, Christian Borntraeger wrote: > > > On 14.01.20 16:30, Janosch Frank wrote: >> All CRs are set to 0 and CRs 0 and 14 are set to pre-defined values, >> so we also need to test 1-13 and 15 for 0. >> >> And while we're at it, let's also set some values to cr 1, 7 and 13, so >> we can actually be sure that they will be zeroed. > > While it does not hurt to have it here, I think the register check for the reset > would be better in a kselftest. This allows to check userspace AND guest at the > same time. Agreed. Especially it also allows to test the kernel ioctl on its own, without QEMU in between (which might also clear some registers), so for getting the new reset ioctls right, the selftests are certainly the better place. Thomas
On 1/15/20 7:17 AM, Thomas Huth wrote: > On 14/01/2020 19.42, Christian Borntraeger wrote: >> >> >> On 14.01.20 16:30, Janosch Frank wrote: >>> All CRs are set to 0 and CRs 0 and 14 are set to pre-defined values, >>> so we also need to test 1-13 and 15 for 0. >>> >>> And while we're at it, let's also set some values to cr 1, 7 and 13, so >>> we can actually be sure that they will be zeroed. >> >> While it does not hurt to have it here, I think the register check for the reset >> would be better in a kselftest. This allows to check userspace AND guest at the >> same time. > > Agreed. Especially it also allows to test the kernel ioctl on its own, > without QEMU in between (which might also clear some registers), so for > getting the new reset ioctls right, the selftests are certainly the > better place. Selftests are in development and will be up for discussion this week if all goes well... While the selftest leaves QEMU out of the picture, we're still using kernel APIs to fetch and reset data, so actually getting guests' register values requires some fiddling in the guest code. So I rather have a test that tells me if KVM + QEMU are correct at the beginning of testing, since that's what most people are using anyways.
On 15/01/2020 08.57, Janosch Frank wrote: > On 1/15/20 7:17 AM, Thomas Huth wrote: >> On 14/01/2020 19.42, Christian Borntraeger wrote: >>> >>> >>> On 14.01.20 16:30, Janosch Frank wrote: >>>> All CRs are set to 0 and CRs 0 and 14 are set to pre-defined values, >>>> so we also need to test 1-13 and 15 for 0. >>>> >>>> And while we're at it, let's also set some values to cr 1, 7 and 13, so >>>> we can actually be sure that they will be zeroed. >>> >>> While it does not hurt to have it here, I think the register check for the reset >>> would be better in a kselftest. This allows to check userspace AND guest at the >>> same time. >> >> Agreed. Especially it also allows to test the kernel ioctl on its own, >> without QEMU in between (which might also clear some registers), so for >> getting the new reset ioctls right, the selftests are certainly the >> better place. > > Selftests are in development and will be up for discussion this week if > all goes well... > > While the selftest leaves QEMU out of the picture, we're still using > kernel APIs to fetch and reset data, so actually getting guests' > register values requires some fiddling in the guest code. So I rather > have a test that tells me if KVM + QEMU are correct at the beginning of > testing, since that's what most people are using anyways. Ok, as Christian already said, it certainly can't hurt to test this in kvm-unit-tests, too - I didn't mean that you should drop this code here, sorry if that sounded wrong. Thomas
diff --git a/s390x/smp.c b/s390x/smp.c index 767d167..11ab425 100644 --- a/s390x/smp.c +++ b/s390x/smp.c @@ -175,16 +175,31 @@ static void test_emcall(void) report_prefix_pop(); } +static void test_func_initial(void) +{ + lctlg(1, 0x42000UL); + lctlg(7, 0x43000UL); + lctlg(13, 0x44000UL); + testflag = 1; + mb(); + cpu_loop(); +} + static void test_reset_initial(void) { struct cpu_status *status = alloc_pages(0); + uint8_t *nullp = alloc_pages(0); struct psw psw; + memset(nullp, 0, PAGE_SIZE); psw.mask = extract_psw_mask(); - psw.addr = (unsigned long)test_func; + psw.addr = (unsigned long)test_func_initial; report_prefix_push("reset initial"); + testflag = 0; + mb(); smp_cpu_start(1, psw); + wait_for_flag(); sigp_retry(1, SIGP_INITIAL_CPU_RESET, 0, NULL); sigp(1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, NULL); @@ -195,6 +210,8 @@ static void test_reset_initial(void) report(!status->fpc, "fpc"); report(!status->cputm, "cpu timer"); report(!status->todpr, "todpr"); + report(!memcmp(&status->crs[1], nullp, sizeof(status->crs[1]) * 12), "cr1-13 == 0"); + report(status->crs[15] == 0, "cr15 == 0"); report_prefix_pop(); report_prefix_push("initialized"); @@ -204,6 +221,7 @@ static void test_reset_initial(void) report(smp_cpu_stopped(1), "cpu stopped"); free_pages(status, PAGE_SIZE); + free_pages(nullp, PAGE_SIZE); report_prefix_pop(); } @@ -219,6 +237,7 @@ static void test_reset(void) sigp_retry(1, SIGP_CPU_RESET, 0, NULL); report(smp_cpu_stopped(1), "cpu stopped"); + smp_cpu_destroy(1); report_prefix_pop(); }
All CRs are set to 0 and CRs 0 and 14 are set to pre-defined values, so we also need to test 1-13 and 15 for 0. And while we're at it, let's also set some values to cr 1, 7 and 13, so we can actually be sure that they will be zeroed. Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- s390x/smp.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)