Skip to contentSkip to conversation

s390x: smp: Improve smp code and reset checks

PATCH Superseded Janosch Frank 5 participants 21 messages Jan 14, 2020 – Jan 15, 2020

Subsystems (2)
KVM s390

v1 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Janosch Frank

author
#0 lore raw

cover s390x: smp: Improve smp code and reset checks

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(-)
Signature · 1 line
2.20.1
author
#1 lore raw

PATCH 1/4 s390x: smp: Cleanup smp.c

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 <[email protected]>
---
 s390x/smp.c | 41 +++++++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 18 deletions(-)
s390x/smp.c +23 -18
index ab7e46c..4dee43e 100644
@@ -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");
Signature · 1 line
2.20.1
Reviewed-by on v1 1/4
#2 lore raw

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

Quoted 9 lines · 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 <[email protected]>
---
 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 <[email protected]>
Reviewed-by on v1 1/4
#3 lore raw

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

In reply to Janosch Frank
Quoted 9 lines · On Tue, 14 Jan 2020 10:30:50 -0500 Janosch Frank <[email protected]> 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 <[email protected]>
---
 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 <[email protected]>
author
#4 lore raw

PATCH 2/4 s390x: smp: Only use smp_cpu_setup once

In reply to Janosch Frank

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

Signed-off-by Janosch Frank <[email protected]>
---
 s390x/smp.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)
s390x/smp.c +12 -7
index 4dee43e..767d167 100644
@@ -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();
Signature · 1 line
2.20.1
Reviewed-by on v1 2/4
#5 lore raw

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

Quoted 53 lines · Janosch Frank

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

Signed-off-by Janosch Frank <[email protected]>
---
 s390x/smp.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)
s390x/smp.c +4 -6
index 4dee43e..767d167 100644
@@ -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);

s390x/smp.c static void test_reset(void) · line 218

Quoted 5 lines
s390x/smp.c +1 -1
@@ -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 <[email protected]>
author
#6 lore raw

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

Quoted 56 lines · Thomas Huth
Quoted 55 lines · Janosch Frank

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

Signed-off-by Janosch Frank <[email protected]>
---
 s390x/smp.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)
s390x/smp.c +4 -6
index 4dee43e..767d167 100644
@@ -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;
Quoted 7 lines

s390x/smp.c static void test_reset(void) · line 218

Quoted 3 lines
s390x/smp.c +1 -1
 	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.

Quoted 3 lines
Reviewed-by Thomas Huth <[email protected]>
Reviewed-by on v1 2/4
#7 lore raw

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

In reply to Janosch Frank
Quoted 2 lines · On Tue, 14 Jan 2020 10:30:51 -0500 Janosch Frank <[email protected]> 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?

Quoted 5 lines
Signed-off-by Janosch Frank <[email protected]>
---
 s390x/smp.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)
Reviewed-by Cornelia Huck <[email protected]>
author
#8 lore raw

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

Quoted 8 lines · Cornelia Huck
Quoted 2 lines · On Tue, 14 Jan 2020 10:30:51 -0500 Janosch Frank <[email protected]> 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...

Quoted 9 lines
Quoted 6 lines
Signed-off-by Janosch Frank <[email protected]>
---
 s390x/smp.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)
Reviewed-by Cornelia Huck <[email protected]>
author
#9 lore raw

PATCH 3/4 s390x: smp: Test all CRs on initial reset

In reply to 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 <[email protected]>
---
 s390x/smp.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)
s390x/smp.c +20 -1
index 767d167..11ab425 100644
@@ -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();
 }
Signature · 1 line
2.20.1
#10 lore raw

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

Quoted 23 lines · 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 <[email protected]>
---
 s390x/smp.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)
s390x/smp.c +4 -0
index 767d167..11ab425 100644
@@ -175,16 +175,31 @@ static void test_emcall(void)
 	report_prefix_pop();
 }
 
+static void test_func_initial(void)
+{
+	lctlg(1, 0x42000UL);
+	lctlg(7, 0x43000UL);

s390x/smp.c static void test_emcall(void) · line 191

Quoted 10 lines
s390x/smp.c +7 -0
+	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];

?

Quoted 33 lines
s390x/smp.c +8 -1
 	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();
 }

s390x/smp.c static void test_reset(void) · line 240

Quoted 4 lines
s390x/smp.c +1 -0
@@ -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()?

s390x/smp.c static void test_reset(void) · line 242

Quoted 2 lines
s390x/smp.c +0 -0
 	report_prefix_pop();
 }

Thomas

#11 lore raw

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

Quoted 14 lines · On Tue, 14 Jan 2020 18:01:32 +0100 Thomas Huth <[email protected]> wrote:
Quoted 13 lines · 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 <[email protected]>
---
 s390x/smp.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)
(quoted diff) +0 -0
@@ -219,6 +237,7 @@ static void test_reset(void)
Quoted 8 lines

s390x/smp.c static void test_reset(void) · line 240

Quoted 3 lines
s390x/smp.c +1 -0
 	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]

Quoted 5 lines

Review static void test_reset(void) · line 242

Quoted 3 lines
(quoted diff) +0 -0
 	report_prefix_pop();
 }

Thomas

#12 lore raw

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

In reply to Janosch Frank
Quoted 5 lines · 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.

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.

#13 lore raw

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

Quoted 6 lines · Christian Borntraeger
Quoted 3 lines · 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.

Quoted 6 lines
Quoted 2 lines

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

author
#14 lore raw

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

Quoted 10 lines · Thomas Huth
Quoted 9 lines · Christian Borntraeger
Quoted 5 lines · 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.

Quoted 8 lines
Quoted 3 lines

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.

#15 lore raw

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

Quoted 20 lines · Janosch Frank
Quoted 10 lines · Thomas Huth
Quoted 9 lines · Christian Borntraeger
Quoted 5 lines · 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.

Quoted 8 lines
Quoted 3 lines

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.

Quoted 8 lines

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

author
#16 lore raw

PATCH 4/4 s390x: smp: Dirty fpc before initial reset test

In reply to Janosch Frank

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

Signed-off-by Janosch Frank <[email protected]>
---
 s390x/smp.c | 3 +++
 1 file changed, 3 insertions(+)
s390x/smp.c +3 -0
index 11ab425..cd32342 100644
@@ -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);
Signature · 1 line
2.20.1
Reviewed-by on v1 4/4
#17 lore raw

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

Quoted 13 lines · Janosch Frank

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

Signed-off-by Janosch Frank <[email protected]>
---
 s390x/smp.c | 3 +++
 1 file changed, 3 insertions(+)
s390x/smp.c +0 -0
index 11ab425..cd32342 100644

s390x/smp.c static void test_emcall(void) · line 185

Quoted 9 lines
s390x/smp.c +3 -0
@@ -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 <[email protected]>
#19 lore raw
In reply to Janosch Frank
Quoted 12 lines · 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(-)

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

Signature · 3 lines
Thanks,

David / dhildenb
author
#20 lore raw
Quoted 10 lines · David Hildenbrand
Quoted 9 lines · 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

Quoted 6 lines
Quoted 3 lines
 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