kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [kvm-unit-tests PATCH 0/4] s390x: smp: Improve smp code and reset checks
@ 2020-01-14 15:30 Janosch Frank
  2020-01-14 15:30 ` [kvm-unit-tests PATCH 1/4] s390x: smp: Cleanup smp.c Janosch Frank
                   ` (4 more replies)
  0 siblings, 5 replies; 21+ messages in thread
From: Janosch Frank @ 2020-01-14 15:30 UTC (permalink / raw)
  To: kvm; +Cc: thuth, borntraeger, linux-s390, david, cohuck

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [kvm-unit-tests PATCH 1/4] s390x: smp: Cleanup smp.c
  2020-01-14 15:30 [kvm-unit-tests PATCH 0/4] s390x: smp: Improve smp code and reset checks Janosch Frank
@ 2020-01-14 15:30 ` Janosch Frank
  2020-01-14 15:39   ` Thomas Huth
  2020-01-14 17:32   ` Cornelia Huck
  2020-01-14 15:30 ` [kvm-unit-tests PATCH 2/4] s390x: smp: Only use smp_cpu_setup once Janosch Frank
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 21+ messages in thread
From: Janosch Frank @ 2020-01-14 15:30 UTC (permalink / raw)
  To: kvm; +Cc: thuth, borntraeger, linux-s390, david, cohuck

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


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [kvm-unit-tests PATCH 2/4] s390x: smp: Only use smp_cpu_setup once
  2020-01-14 15:30 [kvm-unit-tests PATCH 0/4] s390x: smp: Improve smp code and reset checks Janosch Frank
  2020-01-14 15:30 ` [kvm-unit-tests PATCH 1/4] s390x: smp: Cleanup smp.c Janosch Frank
@ 2020-01-14 15:30 ` Janosch Frank
  2020-01-14 16:45   ` Thomas Huth
  2020-01-14 17:44   ` Cornelia Huck
  2020-01-14 15:30 ` [kvm-unit-tests PATCH 3/4] s390x: smp: Test all CRs on initial reset Janosch Frank
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 21+ messages in thread
From: Janosch Frank @ 2020-01-14 15:30 UTC (permalink / raw)
  To: kvm; +Cc: thuth, borntraeger, linux-s390, david, cohuck

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


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [kvm-unit-tests PATCH 3/4] s390x: smp: Test all CRs on initial reset
  2020-01-14 15:30 [kvm-unit-tests PATCH 0/4] s390x: smp: Improve smp code and reset checks Janosch Frank
  2020-01-14 15:30 ` [kvm-unit-tests PATCH 1/4] s390x: smp: Cleanup smp.c Janosch Frank
  2020-01-14 15:30 ` [kvm-unit-tests PATCH 2/4] s390x: smp: Only use smp_cpu_setup once Janosch Frank
@ 2020-01-14 15:30 ` Janosch Frank
  2020-01-14 17:01   ` Thomas Huth
  2020-01-14 18:42   ` Christian Borntraeger
  2020-01-14 15:30 ` [kvm-unit-tests PATCH 4/4] s390x: smp: Dirty fpc before initial reset test Janosch Frank
  2020-01-15 10:47 ` [kvm-unit-tests PATCH 0/4] s390x: smp: Improve smp code and reset checks David Hildenbrand
  4 siblings, 2 replies; 21+ messages in thread
From: Janosch Frank @ 2020-01-14 15:30 UTC (permalink / raw)
  To: kvm; +Cc: thuth, borntraeger, linux-s390, david, cohuck

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


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [kvm-unit-tests PATCH 4/4] s390x: smp: Dirty fpc before initial reset test
  2020-01-14 15:30 [kvm-unit-tests PATCH 0/4] s390x: smp: Improve smp code and reset checks Janosch Frank
                   ` (2 preceding siblings ...)
  2020-01-14 15:30 ` [kvm-unit-tests PATCH 3/4] s390x: smp: Test all CRs on initial reset Janosch Frank
@ 2020-01-14 15:30 ` Janosch Frank
  2020-01-14 17:22   ` Thomas Huth
  2020-01-14 17:53   ` Cornelia Huck
  2020-01-15 10:47 ` [kvm-unit-tests PATCH 0/4] s390x: smp: Improve smp code and reset checks David Hildenbrand
  4 siblings, 2 replies; 21+ messages in thread
From: Janosch Frank @ 2020-01-14 15:30 UTC (permalink / raw)
  To: kvm; +Cc: thuth, borntraeger, linux-s390, david, cohuck

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


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [kvm-unit-tests PATCH 1/4] s390x: smp: Cleanup smp.c
  2020-01-14 15:30 ` [kvm-unit-tests PATCH 1/4] s390x: smp: Cleanup smp.c Janosch Frank
@ 2020-01-14 15:39   ` Thomas Huth
  2020-01-14 17:32   ` Cornelia Huck
  1 sibling, 0 replies; 21+ messages in thread
From: Thomas Huth @ 2020-01-14 15:39 UTC (permalink / raw)
  To: Janosch Frank, kvm; +Cc: borntraeger, linux-s390, david, cohuck

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>


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [kvm-unit-tests PATCH 2/4] s390x: smp: Only use smp_cpu_setup once
  2020-01-14 15:30 ` [kvm-unit-tests PATCH 2/4] s390x: smp: Only use smp_cpu_setup once Janosch Frank
@ 2020-01-14 16:45   ` Thomas Huth
  2020-01-15  7:50     ` Janosch Frank
  2020-01-14 17:44   ` Cornelia Huck
  1 sibling, 1 reply; 21+ messages in thread
From: Thomas Huth @ 2020-01-14 16:45 UTC (permalink / raw)
  To: Janosch Frank, kvm; +Cc: borntraeger, linux-s390, david, cohuck

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>


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [kvm-unit-tests PATCH 3/4] s390x: smp: Test all CRs on initial reset
  2020-01-14 15:30 ` [kvm-unit-tests PATCH 3/4] s390x: smp: Test all CRs on initial reset Janosch Frank
@ 2020-01-14 17:01   ` Thomas Huth
  2020-01-14 17:51     ` Cornelia Huck
  2020-01-14 18:42   ` Christian Borntraeger
  1 sibling, 1 reply; 21+ messages in thread
From: Thomas Huth @ 2020-01-14 17:01 UTC (permalink / raw)
  To: Janosch Frank, kvm; +Cc: borntraeger, linux-s390, david, cohuck

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [kvm-unit-tests PATCH 4/4] s390x: smp: Dirty fpc before initial reset test
  2020-01-14 15:30 ` [kvm-unit-tests PATCH 4/4] s390x: smp: Dirty fpc before initial reset test Janosch Frank
@ 2020-01-14 17:22   ` Thomas Huth
  2020-01-14 17:53   ` Cornelia Huck
  1 sibling, 0 replies; 21+ messages in thread
From: Thomas Huth @ 2020-01-14 17:22 UTC (permalink / raw)
  To: Janosch Frank, kvm; +Cc: borntraeger, linux-s390, david, cohuck

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>


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [kvm-unit-tests PATCH 1/4] s390x: smp: Cleanup smp.c
  2020-01-14 15:30 ` [kvm-unit-tests PATCH 1/4] s390x: smp: Cleanup smp.c Janosch Frank
  2020-01-14 15:39   ` Thomas Huth
@ 2020-01-14 17:32   ` Cornelia Huck
  1 sibling, 0 replies; 21+ messages in thread
From: Cornelia Huck @ 2020-01-14 17:32 UTC (permalink / raw)
  To: Janosch Frank; +Cc: kvm, thuth, borntraeger, linux-s390, david

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>


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [kvm-unit-tests PATCH 2/4] s390x: smp: Only use smp_cpu_setup once
  2020-01-14 15:30 ` [kvm-unit-tests PATCH 2/4] s390x: smp: Only use smp_cpu_setup once Janosch Frank
  2020-01-14 16:45   ` Thomas Huth
@ 2020-01-14 17:44   ` Cornelia Huck
  2020-01-15  9:00     ` Janosch Frank
  1 sibling, 1 reply; 21+ messages in thread
From: Cornelia Huck @ 2020-01-14 17:44 UTC (permalink / raw)
  To: Janosch Frank; +Cc: kvm, thuth, borntraeger, linux-s390, david

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>


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [kvm-unit-tests PATCH 3/4] s390x: smp: Test all CRs on initial reset
  2020-01-14 17:01   ` Thomas Huth
@ 2020-01-14 17:51     ` Cornelia Huck
  0 siblings, 0 replies; 21+ messages in thread
From: Cornelia Huck @ 2020-01-14 17:51 UTC (permalink / raw)
  To: Thomas Huth; +Cc: Janosch Frank, kvm, borntraeger, linux-s390, david

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [kvm-unit-tests PATCH 4/4] s390x: smp: Dirty fpc before initial reset test
  2020-01-14 15:30 ` [kvm-unit-tests PATCH 4/4] s390x: smp: Dirty fpc before initial reset test Janosch Frank
  2020-01-14 17:22   ` Thomas Huth
@ 2020-01-14 17:53   ` Cornelia Huck
  1 sibling, 0 replies; 21+ messages in thread
From: Cornelia Huck @ 2020-01-14 17:53 UTC (permalink / raw)
  To: Janosch Frank; +Cc: kvm, thuth, borntraeger, linux-s390, david

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>


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [kvm-unit-tests PATCH 3/4] s390x: smp: Test all CRs on initial reset
  2020-01-14 15:30 ` [kvm-unit-tests PATCH 3/4] s390x: smp: Test all CRs on initial reset Janosch Frank
  2020-01-14 17:01   ` Thomas Huth
@ 2020-01-14 18:42   ` Christian Borntraeger
  2020-01-15  6:17     ` Thomas Huth
  1 sibling, 1 reply; 21+ messages in thread
From: Christian Borntraeger @ 2020-01-14 18:42 UTC (permalink / raw)
  To: Janosch Frank, kvm; +Cc: thuth, linux-s390, david, cohuck



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.


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [kvm-unit-tests PATCH 3/4] s390x: smp: Test all CRs on initial reset
  2020-01-14 18:42   ` Christian Borntraeger
@ 2020-01-15  6:17     ` Thomas Huth
  2020-01-15  7:57       ` Janosch Frank
  0 siblings, 1 reply; 21+ messages in thread
From: Thomas Huth @ 2020-01-15  6:17 UTC (permalink / raw)
  To: Christian Borntraeger, Janosch Frank, kvm; +Cc: linux-s390, david, cohuck

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [kvm-unit-tests PATCH 2/4] s390x: smp: Only use smp_cpu_setup once
  2020-01-14 16:45   ` Thomas Huth
@ 2020-01-15  7:50     ` Janosch Frank
  0 siblings, 0 replies; 21+ messages in thread
From: Janosch Frank @ 2020-01-15  7:50 UTC (permalink / raw)
  To: Thomas Huth, kvm; +Cc: borntraeger, linux-s390, david, cohuck


[-- 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 --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [kvm-unit-tests PATCH 3/4] s390x: smp: Test all CRs on initial reset
  2020-01-15  6:17     ` Thomas Huth
@ 2020-01-15  7:57       ` Janosch Frank
  2020-01-15  8:11         ` Thomas Huth
  0 siblings, 1 reply; 21+ messages in thread
From: Janosch Frank @ 2020-01-15  7:57 UTC (permalink / raw)
  To: Thomas Huth, Christian Borntraeger, kvm; +Cc: linux-s390, david, cohuck


[-- 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 --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [kvm-unit-tests PATCH 3/4] s390x: smp: Test all CRs on initial reset
  2020-01-15  7:57       ` Janosch Frank
@ 2020-01-15  8:11         ` Thomas Huth
  0 siblings, 0 replies; 21+ messages in thread
From: Thomas Huth @ 2020-01-15  8:11 UTC (permalink / raw)
  To: Janosch Frank, Christian Borntraeger, kvm; +Cc: linux-s390, david, cohuck

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [kvm-unit-tests PATCH 2/4] s390x: smp: Only use smp_cpu_setup once
  2020-01-14 17:44   ` Cornelia Huck
@ 2020-01-15  9:00     ` Janosch Frank
  0 siblings, 0 replies; 21+ messages in thread
From: Janosch Frank @ 2020-01-15  9:00 UTC (permalink / raw)
  To: Cornelia Huck; +Cc: kvm, thuth, borntraeger, linux-s390, david


[-- 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 --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [kvm-unit-tests PATCH 0/4] s390x: smp: Improve smp code and reset checks
  2020-01-14 15:30 [kvm-unit-tests PATCH 0/4] s390x: smp: Improve smp code and reset checks Janosch Frank
                   ` (3 preceding siblings ...)
  2020-01-14 15:30 ` [kvm-unit-tests PATCH 4/4] s390x: smp: Dirty fpc before initial reset test Janosch Frank
@ 2020-01-15 10:47 ` David Hildenbrand
  2020-01-15 13:03   ` Janosch Frank
  4 siblings, 1 reply; 21+ messages in thread
From: David Hildenbrand @ 2020-01-15 10:47 UTC (permalink / raw)
  To: Janosch Frank, kvm; +Cc: thuth, borntraeger, linux-s390, cohuck

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [kvm-unit-tests PATCH 0/4] s390x: smp: Improve smp code and reset checks
  2020-01-15 10:47 ` [kvm-unit-tests PATCH 0/4] s390x: smp: Improve smp code and reset checks David Hildenbrand
@ 2020-01-15 13:03   ` Janosch Frank
  0 siblings, 0 replies; 21+ messages in thread
From: Janosch Frank @ 2020-01-15 13:03 UTC (permalink / raw)
  To: David Hildenbrand, kvm; +Cc: thuth, borntraeger, linux-s390, cohuck


[-- 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 --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2020-01-15 13:03 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-14 15:30 [kvm-unit-tests PATCH 0/4] s390x: smp: Improve smp code and reset checks Janosch Frank
2020-01-14 15:30 ` [kvm-unit-tests PATCH 1/4] s390x: smp: Cleanup smp.c Janosch Frank
2020-01-14 15:39   ` Thomas Huth
2020-01-14 17:32   ` Cornelia Huck
2020-01-14 15:30 ` [kvm-unit-tests PATCH 2/4] s390x: smp: Only use smp_cpu_setup once Janosch Frank
2020-01-14 16:45   ` Thomas Huth
2020-01-15  7:50     ` Janosch Frank
2020-01-14 17:44   ` Cornelia Huck
2020-01-15  9:00     ` Janosch Frank
2020-01-14 15:30 ` [kvm-unit-tests PATCH 3/4] s390x: smp: Test all CRs on initial reset Janosch Frank
2020-01-14 17:01   ` Thomas Huth
2020-01-14 17:51     ` Cornelia Huck
2020-01-14 18:42   ` Christian Borntraeger
2020-01-15  6:17     ` Thomas Huth
2020-01-15  7:57       ` Janosch Frank
2020-01-15  8:11         ` Thomas Huth
2020-01-14 15:30 ` [kvm-unit-tests PATCH 4/4] s390x: smp: Dirty fpc before initial reset test Janosch Frank
2020-01-14 17:22   ` Thomas Huth
2020-01-14 17:53   ` Cornelia Huck
2020-01-15 10:47 ` [kvm-unit-tests PATCH 0/4] s390x: smp: Improve smp code and reset checks David Hildenbrand
2020-01-15 13:03   ` Janosch Frank

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).