[kvm-unit-tests PATCH 0/4] s390x: smp: Improve smp code and reset checks

[kvm-unit-tests PATCH 0/4] s390x: smp: Improve smp code and reset checks

[Janosch Frank]

The first two patches are badly needed cleanup for smp.c.
The last two improve initial reset testing.

Janosch Frank (4):
  s390x: smp: Cleanup smp.c
  s390x: smp: Only use smp_cpu_setup once
  s390x: smp: Test all CRs on initial reset
  s390x: smp: Dirty fpc before initial reset test

 s390x/smp.c | 84 ++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 58 insertions(+), 26 deletions(-)

-- 
2.20.1

[kvm-unit-tests PATCH 1/4] s390x: smp: Cleanup smp.c

[Janosch Frank]

Let's remove a lot of badly formatted code by introducing the
wait_for_flag() function.

Also let's remove some stray spaces.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/smp.c | 41 +++++++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 18 deletions(-)

diff --git a/s390x/smp.c b/s390x/smp.c
index ab7e46c..4dee43e 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -22,6 +22,13 @@
 
 static int testflag = 0;
 
+static void wait_for_flag(void)
+{
+	while (!testflag) {
+		mb();
+	}
+}
+
 static void cpu_loop(void)
 {
 	for (;;) {}
@@ -37,13 +44,11 @@ static void test_func(void)
 static void test_start(void)
 {
 	struct psw psw;
-	psw.mask =  extract_psw_mask();
+	psw.mask = extract_psw_mask();
 	psw.addr = (unsigned long)test_func;
 
 	smp_cpu_setup(1, psw);
-	while (!testflag) {
-		mb();
-	}
+	wait_for_flag();
 	report(1, "start");
 }
 
@@ -115,24 +120,24 @@ static void ecall(void)
 	testflag = 1;
 	while (lc->ext_int_code != 0x1202) { mb(); }
 	report(1, "ecall");
-	testflag= 1;
+	testflag = 1;
 }
 
 static void test_ecall(void)
 {
 	struct psw psw;
-	psw.mask =  extract_psw_mask();
+	psw.mask = extract_psw_mask();
 	psw.addr = (unsigned long)ecall;
 
 	report_prefix_push("ecall");
-	testflag= 0;
+	testflag = 0;
 	smp_cpu_destroy(1);
 
 	smp_cpu_setup(1, psw);
-	while (!testflag) { mb(); }
-	testflag= 0;
+	wait_for_flag();
+	testflag = 0;
 	sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);
-	while(!testflag) {mb();}
+	wait_for_flag();
 	smp_cpu_stop(1);
 	report_prefix_pop();
 }
@@ -147,7 +152,7 @@ static void emcall(void)
 	mask = extract_psw_mask();
 	mask |= PSW_MASK_EXT;
 	load_psw_mask(mask);
-	testflag= 1;
+	testflag = 1;
 	while (lc->ext_int_code != 0x1201) { mb(); }
 	report(1, "ecall");
 	testflag = 1;
@@ -156,18 +161,18 @@ static void emcall(void)
 static void test_emcall(void)
 {
 	struct psw psw;
-	psw.mask =  extract_psw_mask();
+	psw.mask = extract_psw_mask();
 	psw.addr = (unsigned long)emcall;
 
 	report_prefix_push("emcall");
-	testflag= 0;
+	testflag = 0;
 	smp_cpu_destroy(1);
 
 	smp_cpu_setup(1, psw);
-	while (!testflag) { mb(); }
-	testflag= 0;
+	wait_for_flag();
+	testflag = 0;
 	sigp(1, SIGP_EMERGENCY_SIGNAL, 0, NULL);
-	while(!testflag) { mb(); }
+	wait_for_flag();
 	smp_cpu_stop(1);
 	report_prefix_pop();
 }
@@ -177,7 +182,7 @@ static void test_reset_initial(void)
 	struct cpu_status *status = alloc_pages(0);
 	struct psw psw;
 
-	psw.mask =  extract_psw_mask();
+	psw.mask = extract_psw_mask();
 	psw.addr = (unsigned long)test_func;
 
 	report_prefix_push("reset initial");
@@ -208,7 +213,7 @@ static void test_reset(void)
 {
 	struct psw psw;
 
-	psw.mask =  extract_psw_mask();
+	psw.mask = extract_psw_mask();
 	psw.addr = (unsigned long)test_func;
 
 	report_prefix_push("cpu reset");
-- 
2.20.1

Re: [kvm-unit-tests PATCH 1/4] s390x: smp: Cleanup smp.c

[Thomas Huth]

On 14/01/2020 16.30, Janosch Frank wrote:
> Let's remove a lot of badly formatted code by introducing the
> wait_for_flag() function.
> 
> Also let's remove some stray spaces.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/smp.c | 41 +++++++++++++++++++++++------------------
>  1 file changed, 23 insertions(+), 18 deletions(-)

OMG, did I really merge that original code in that shape?

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

Re: [kvm-unit-tests PATCH 1/4] s390x: smp: Cleanup smp.c

[Cornelia Huck]

On Tue, 14 Jan 2020 10:30:50 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> Let's remove a lot of badly formatted code by introducing the
> wait_for_flag() function.
> 
> Also let's remove some stray spaces.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/smp.c | 41 +++++++++++++++++++++++------------------
>  1 file changed, 23 insertions(+), 18 deletions(-)

This looks like some nice copy/paste had been going on :)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

[kvm-unit-tests PATCH 2/4] s390x: smp: Only use smp_cpu_setup once

[Janosch Frank]

Let's stop and start instead of using setup to run a function on a
cpu.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/smp.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/s390x/smp.c b/s390x/smp.c
index 4dee43e..767d167 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -47,7 +47,7 @@ static void test_start(void)
 	psw.mask = extract_psw_mask();
 	psw.addr = (unsigned long)test_func;
 
-	smp_cpu_setup(1, psw);
+	smp_cpu_start(1, psw);
 	wait_for_flag();
 	report(1, "start");
 }
@@ -131,9 +131,8 @@ static void test_ecall(void)
 
 	report_prefix_push("ecall");
 	testflag = 0;
-	smp_cpu_destroy(1);
 
-	smp_cpu_setup(1, psw);
+	smp_cpu_start(1, psw);
 	wait_for_flag();
 	testflag = 0;
 	sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);
@@ -166,9 +165,8 @@ static void test_emcall(void)
 
 	report_prefix_push("emcall");
 	testflag = 0;
-	smp_cpu_destroy(1);
 
-	smp_cpu_setup(1, psw);
+	smp_cpu_start(1, psw);
 	wait_for_flag();
 	testflag = 0;
 	sigp(1, SIGP_EMERGENCY_SIGNAL, 0, NULL);
@@ -186,7 +184,7 @@ static void test_reset_initial(void)
 	psw.addr = (unsigned long)test_func;
 
 	report_prefix_push("reset initial");
-	smp_cpu_setup(1, psw);
+	smp_cpu_start(1, psw);
 
 	sigp_retry(1, SIGP_INITIAL_CPU_RESET, 0, NULL);
 	sigp(1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, NULL);
@@ -217,7 +215,7 @@ static void test_reset(void)
 	psw.addr = (unsigned long)test_func;
 
 	report_prefix_push("cpu reset");
-	smp_cpu_setup(1, psw);
+	smp_cpu_start(1, psw);
 
 	sigp_retry(1, SIGP_CPU_RESET, 0, NULL);
 	report(smp_cpu_stopped(1), "cpu stopped");
@@ -226,6 +224,7 @@ static void test_reset(void)
 
 int main(void)
 {
+	struct psw psw;
 	report_prefix_push("smp");
 
 	if (smp_query_num_cpus() == 1) {
@@ -233,6 +232,12 @@ int main(void)
 		goto done;
 	}
 
+	/* Setting up the cpu to give it a stack and lowcore */
+	psw.mask = extract_psw_mask();
+	psw.addr = (unsigned long)cpu_loop;
+	smp_cpu_setup(1, psw);
+	smp_cpu_stop(1);
+
 	test_start();
 	test_stop();
 	test_stop_store_status();
-- 
2.20.1

Re: [kvm-unit-tests PATCH 2/4] s390x: smp: Only use smp_cpu_setup once

[Thomas Huth]

On 14/01/2020 16.30, Janosch Frank wrote:
> Let's stop and start instead of using setup to run a function on a
> cpu.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/smp.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/s390x/smp.c b/s390x/smp.c
> index 4dee43e..767d167 100644
> --- a/s390x/smp.c
> +++ b/s390x/smp.c
> @@ -47,7 +47,7 @@ static void test_start(void)
>  	psw.mask = extract_psw_mask();
>  	psw.addr = (unsigned long)test_func;
>  
> -	smp_cpu_setup(1, psw);
> +	smp_cpu_start(1, psw);
>  	wait_for_flag();
>  	report(1, "start");
>  }
> @@ -131,9 +131,8 @@ static void test_ecall(void)
>  
>  	report_prefix_push("ecall");
>  	testflag = 0;
> -	smp_cpu_destroy(1);
>  
> -	smp_cpu_setup(1, psw);
> +	smp_cpu_start(1, psw);
>  	wait_for_flag();
>  	testflag = 0;
>  	sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);
> @@ -166,9 +165,8 @@ static void test_emcall(void)
>  
>  	report_prefix_push("emcall");
>  	testflag = 0;
> -	smp_cpu_destroy(1);
>  
> -	smp_cpu_setup(1, psw);
> +	smp_cpu_start(1, psw);
>  	wait_for_flag();
>  	testflag = 0;
>  	sigp(1, SIGP_EMERGENCY_SIGNAL, 0, NULL);
> @@ -186,7 +184,7 @@ static void test_reset_initial(void)
>  	psw.addr = (unsigned long)test_func;
>  
>  	report_prefix_push("reset initial");
> -	smp_cpu_setup(1, psw);
> +	smp_cpu_start(1, psw);
>  
>  	sigp_retry(1, SIGP_INITIAL_CPU_RESET, 0, NULL);
>  	sigp(1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, NULL);
> @@ -217,7 +215,7 @@ static void test_reset(void)
>  	psw.addr = (unsigned long)test_func;
>  
>  	report_prefix_push("cpu reset");
> -	smp_cpu_setup(1, psw);
> +	smp_cpu_start(1, psw);

I think this also fixes a memory leak in case the code did not call
smp_cpu_destroy() inbetween (since smp_cpu_setup() allocates new memory
for the low-core). So as far as I can see, this is a good idea:

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

Re: [kvm-unit-tests PATCH 2/4] s390x: smp: Only use smp_cpu_setup once

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 2262 bytes --]

On 1/14/20 5:45 PM, Thomas Huth wrote:
> On 14/01/2020 16.30, Janosch Frank wrote:
>> Let's stop and start instead of using setup to run a function on a
>> cpu.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  s390x/smp.c | 19 ++++++++++++-------
>>  1 file changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/s390x/smp.c b/s390x/smp.c
>> index 4dee43e..767d167 100644
>> --- a/s390x/smp.c
>> +++ b/s390x/smp.c
>> @@ -47,7 +47,7 @@ static void test_start(void)
>>  	psw.mask = extract_psw_mask();
>>  	psw.addr = (unsigned long)test_func;
>>  
>> -	smp_cpu_setup(1, psw);
>> +	smp_cpu_start(1, psw);
>>  	wait_for_flag();
>>  	report(1, "start");
>>  }
>> @@ -131,9 +131,8 @@ static void test_ecall(void)
>>  
>>  	report_prefix_push("ecall");
>>  	testflag = 0;
>> -	smp_cpu_destroy(1);
>>  
>> -	smp_cpu_setup(1, psw);
>> +	smp_cpu_start(1, psw);
>>  	wait_for_flag();
>>  	testflag = 0;
>>  	sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);
>> @@ -166,9 +165,8 @@ static void test_emcall(void)
>>  
>>  	report_prefix_push("emcall");
>>  	testflag = 0;
>> -	smp_cpu_destroy(1);
>>  
>> -	smp_cpu_setup(1, psw);
>> +	smp_cpu_start(1, psw);
>>  	wait_for_flag();
>>  	testflag = 0;
>>  	sigp(1, SIGP_EMERGENCY_SIGNAL, 0, NULL);
>> @@ -186,7 +184,7 @@ static void test_reset_initial(void)
>>  	psw.addr = (unsigned long)test_func;
>>  
>>  	report_prefix_push("reset initial");
>> -	smp_cpu_setup(1, psw);
>> +	smp_cpu_start(1, psw);
>>  
>>  	sigp_retry(1, SIGP_INITIAL_CPU_RESET, 0, NULL);
>>  	sigp(1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, NULL);
>> @@ -217,7 +215,7 @@ static void test_reset(void)
>>  	psw.addr = (unsigned long)test_func;
>>  
>>  	report_prefix_push("cpu reset");
>> -	smp_cpu_setup(1, psw);
>> +	smp_cpu_start(1, psw);
> 
> I think this also fixes a memory leak in case the code did not call
> smp_cpu_destroy() inbetween (since smp_cpu_setup() allocates new memory
> for the low-core). So as far as I can see, this is a good idea:

Well, if the cpu is active, we should just return in the setup function.
But I have another patch in the queue which cleans up lowcore allocation.

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [kvm-unit-tests PATCH 2/4] s390x: smp: Only use smp_cpu_setup once

[Cornelia Huck]

On Tue, 14 Jan 2020 10:30:51 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> Let's stop and start instead of using setup to run a function on a
> cpu.

Looking at the code, we only support active == operating state and
!active == stopped state anyway, right?

> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/smp.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [kvm-unit-tests PATCH 2/4] s390x: smp: Only use smp_cpu_setup once

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 778 bytes --]

On 1/14/20 6:44 PM, Cornelia Huck wrote:
> On Tue, 14 Jan 2020 10:30:51 -0500
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> Let's stop and start instead of using setup to run a function on a
>> cpu.
> 
> Looking at the code, we only support active == operating state and
> !active == stopped state anyway, right?

Yes, although I think it might make sense to go over the active tracking
in the future and rather make it a setup tracking. We can always ask via
sigp run if the cpu is running. More stuff to add to the todo...

> 
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  s390x/smp.c | 19 ++++++++++++-------
>>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[kvm-unit-tests PATCH 3/4] s390x: smp: Test all CRs on initial reset

[Janosch Frank]

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

Re: [kvm-unit-tests PATCH 3/4] s390x: smp: Test all CRs on initial reset

[Thomas Huth]

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

Re: [kvm-unit-tests PATCH 3/4] s390x: smp: Test all CRs on initial reset

[Cornelia Huck]

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

Re: [kvm-unit-tests PATCH 3/4] s390x: smp: Test all CRs on initial reset

[Christian Borntraeger]

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.

Re: [kvm-unit-tests PATCH 3/4] s390x: smp: Test all CRs on initial reset

[Thomas Huth]

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

Re: [kvm-unit-tests PATCH 3/4] s390x: smp: Test all CRs on initial reset

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 1271 bytes --]

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.



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [kvm-unit-tests PATCH 3/4] s390x: smp: Test all CRs on initial reset

[Thomas Huth]

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

[kvm-unit-tests PATCH 4/4] s390x: smp: Dirty fpc before initial reset test

[Janosch Frank]

Let's dirty the fpc, before we test if the initial reset sets it to 0.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/smp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/s390x/smp.c b/s390x/smp.c
index 11ab425..cd32342 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -177,6 +177,9 @@ static void test_emcall(void)
 
 static void test_func_initial(void)
 {
+	asm volatile(
+		"	sfpc	%0\n"
+		: : "d" (0x11) : );
 	lctlg(1, 0x42000UL);
 	lctlg(7, 0x43000UL);
 	lctlg(13, 0x44000UL);
-- 
2.20.1

Re: [kvm-unit-tests PATCH 4/4] s390x: smp: Dirty fpc before initial reset test

[Thomas Huth]

On 14/01/2020 16.30, Janosch Frank wrote:
> Let's dirty the fpc, before we test if the initial reset sets it to 0.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/smp.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/s390x/smp.c b/s390x/smp.c
> index 11ab425..cd32342 100644
> --- a/s390x/smp.c
> +++ b/s390x/smp.c
> @@ -177,6 +177,9 @@ static void test_emcall(void)
>  
>  static void test_func_initial(void)
>  {
> +	asm volatile(
> +		"	sfpc	%0\n"
> +		: : "d" (0x11) : );
>  	lctlg(1, 0x42000UL);
>  	lctlg(7, 0x43000UL);
>  	lctlg(13, 0x44000UL);
> 

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

Re: [kvm-unit-tests PATCH 4/4] s390x: smp: Dirty fpc before initial reset test

[Cornelia Huck]

On Tue, 14 Jan 2020 10:30:53 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> Let's dirty the fpc, before we test if the initial reset sets it to 0.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/smp.c | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [kvm-unit-tests PATCH 0/4] s390x: smp: Improve smp code and reset checks

[David Hildenbrand]

On 14.01.20 16:30, Janosch Frank wrote:
> The first two patches are badly needed cleanup for smp.c.
> The last two improve initial reset testing.
> 
> Janosch Frank (4):
>   s390x: smp: Cleanup smp.c
>   s390x: smp: Only use smp_cpu_setup once
>   s390x: smp: Test all CRs on initial reset
>   s390x: smp: Dirty fpc before initial reset test
> 
>  s390x/smp.c | 84 ++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 58 insertions(+), 26 deletions(-)
> 

I assume you will resend, right? So I'll wait with queuing.

-- 
Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH 0/4] s390x: smp: Improve smp code and reset checks

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 639 bytes --]

On 1/15/20 11:47 AM, David Hildenbrand wrote:
> On 14.01.20 16:30, Janosch Frank wrote:
>> The first two patches are badly needed cleanup for smp.c.
>> The last two improve initial reset testing.
>>
>> Janosch Frank (4):
>>   s390x: smp: Cleanup smp.c
>>   s390x: smp: Only use smp_cpu_setup once
>>   s390x: smp: Test all CRs on initial reset
>>   s390x: smp: Dirty fpc before initial reset test
>>
>>  s390x/smp.c | 84 ++++++++++++++++++++++++++++++++++++-----------------
>>  1 file changed, 58 insertions(+), 26 deletions(-)
>>
> 
> I assume you will resend, right? So I'll wait with queuing.
> 

Yes, please wait


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]