[kvm-unit-tests,3/4] s390x: smp: Test all CRs on initial reset
diff mbox series

Message ID 20200114153054.77082-4-frankja@linux.ibm.com
State New
Headers show
Series
  • s390x: smp: Improve smp code and reset checks
Related show

Commit Message

Janosch Frank Jan. 14, 2020, 3:30 p.m. UTC
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(-)

Comments

Thomas Huth Jan. 14, 2020, 5:01 p.m. UTC | #1
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
Cornelia Huck Jan. 14, 2020, 5:51 p.m. UTC | #2
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
Christian Borntraeger Jan. 14, 2020, 6:42 p.m. UTC | #3
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.
Thomas Huth Jan. 15, 2020, 6:17 a.m. UTC | #4
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
Janosch Frank Jan. 15, 2020, 7:57 a.m. UTC | #5
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.
Thomas Huth Jan. 15, 2020, 8:11 a.m. UTC | #6
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

Patch
diff mbox series

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();
 }