s390x: smp: Improve smp code and reset checks

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

Reviewed-by Thomas Huth <[email protected]>

Reviewed-by Cornelia Huck <[email protected]>

---
 s390x/smp.c | 42 ++++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 18 deletions(-)

s390x/smp.c +9 -4

index ab7e46c..02204fd 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");
 }
 

Quoted 47 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]>

Reviewed-by Thomas Huth <[email protected]>

Reviewed-by Cornelia Huck <[email protected]>

---
 s390x/smp.c | 42 ++++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 18 deletions(-)

s390x/smp.c +9 -4

index ab7e46c..02204fd 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");
 }
 

s390x/smp.c static void test_store_status(void) · line 106

Quoted 4 lines

s390x/smp.c +1 -0

@@ -98,6 +103,7 @@ static void test_store_status(void)
 	report(1, "status written");
 	free_pages(status, PAGE_SIZE * 2);
 	report_prefix_pop();
+	smp_cpu_stop(1);

This hunk does not seem to belong into this patch.

Apart from that, looks good to me.

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

Signed-off-by Janosch Frank <[email protected]>

Reviewed-by Thomas Huth <[email protected]>

Reviewed-by Cornelia Huck <[email protected]>

---
 s390x/smp.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

s390x/smp.c +12 -7

index 02204fd..d430638 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");
 }

@@ -132,9 +132,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);

@@ -167,9 +166,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);

@@ -187,7 +185,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);

@@ -218,7 +216,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");

@@ -227,6 +225,7 @@ static void test_reset(void)
 
 int main(void)
 {
+	struct psw psw;
 	report_prefix_push("smp");
 
 	if (smp_query_num_cpus() == 1) {

@@ -234,6 +233,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();

Quoted 90 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]>

Reviewed-by Thomas Huth <[email protected]>

Reviewed-by Cornelia Huck <[email protected]>

---
 s390x/smp.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

s390x/smp.c +13 -7

index 02204fd..d430638 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");
 }

@@ -132,9 +132,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);

@@ -167,9 +166,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);

@@ -187,7 +185,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);

@@ -218,7 +216,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");

@@ -227,6 +225,7 @@ static void test_reset(void)
 
 int main(void)
 {
+	struct psw psw;
 	report_prefix_push("smp");
 
 	if (smp_query_num_cpus() == 1) {

@@ -234,6 +233,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();

@@ -242,6 +247,7 @@ int main(void)
 	test_emcall();
 	test_reset();
 	test_reset_initial();
+	smp_cpu_destroy(1);
 

s390x/smp.c int main(void) · line 253

Quoted 3 lines

s390x/smp.c +0 -0

 done:
 	report_prefix_pop();

Acked-by David Hildenbrand <[email protected]>

It's good to know which cpu broke the test.

Signed-off-by Janosch Frank <[email protected]>

---
 lib/s390x/interrupt.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

lib/s390x/interrupt.c +0 -0

index 05f30be..773752a 100644

lib/s390x/interrupt.c +6 -6

 		return;
 	}
 

@@ -137,18 +137,18 @@ void handle_ext_int(void)
 
 void handle_mcck_int(void)
 {
-	report_abort("Unexpected machine check interrupt: at %#lx",
-		     lc->mcck_old_psw.addr);
+	report_abort("Unexpected machine check interrupt: on cpu %d at %#lx",
+		     stap(), lc->mcck_old_psw.addr);
 }
 
 void handle_io_int(void)
 {
-	report_abort("Unexpected io interrupt: at %#lx",
-		     lc->io_old_psw.addr);
+	report_abort("Unexpected io interrupt: on cpu %d at %#lx",
+		     stap(), lc->io_old_psw.addr);
 }
 
 void handle_svc_int(void)
 {
-	report_abort("Unexpected supervisor call interrupt: at %#lx",
-		     lc->svc_old_psw.addr);
+	report_abort("Unexpected supervisor call interrupt: on cpu %d at %#lx",
+		     stap(), lc->svc_old_psw.addr);
 }

Quoted 12 lines · Janosch Frank

It's good to know which cpu broke the test.

Signed-off-by Janosch Frank <[email protected]>

---
 lib/s390x/interrupt.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

lib/s390x/interrupt.c +0 -0

index 05f30be..773752a 100644

lib/s390x/interrupt.c static void fixup_pgm_int(void) · line 112

Quoted 8 lines

lib/s390x/interrupt.c +2 -2

@@ -107,8 +107,8 @@ static void fixup_pgm_int(void)
 void handle_pgm_int(void)
 {
 	if (!pgm_int_expected)
-		report_abort("Unexpected program interrupt: %d at %#lx, ilen %d\n",
-			     lc->pgm_int_code, lc->pgm_old_psw.addr,
+		report_abort("Unexpected program interrupt: %d on cpu %d at %#lx, ilen %d\n",
+			     lc->pgm_int_code, stap(), lc->pgm_old_psw.addr,
 			     lc->pgm_int_id);

nit: "cpu: %d"

Quoted 1 line

lib/s390x/interrupt.c void handle_ext_int(void) · line 123

Quoted 9 lines

lib/s390x/interrupt.c +2 -2

 	pgm_int_expected = false;

@@ -119,8 +119,8 @@ void handle_ext_int(void)
 {
 	if (!ext_int_expected &&
 	    lc->ext_int_code != EXT_IRQ_SERVICE_SIG) {
-		report_abort("Unexpected external call interrupt (code %#x): at %#lx",
-			     lc->ext_int_code, lc->ext_old_psw.addr);
+		report_abort("Unexpected external call interrupt (code %#x): on cpu %d at %#lx",
+			     stap(), lc->ext_int_code, lc->ext_old_psw.addr);

nit: "(code %#x) on cpu: %d" ...

Same comment for the ones below

Reviewed-by David Hildenbrand <[email protected]>

Quoted 29 lines

lib/s390x/interrupt.c +6 -6

 		return;
 	}
 

@@ -137,18 +137,18 @@ void handle_ext_int(void)
 
 void handle_mcck_int(void)
 {
-	report_abort("Unexpected machine check interrupt: at %#lx",
-		     lc->mcck_old_psw.addr);
+	report_abort("Unexpected machine check interrupt: on cpu %d at %#lx",
+		     stap(), lc->mcck_old_psw.addr);
 }
 
 void handle_io_int(void)
 {
-	report_abort("Unexpected io interrupt: at %#lx",
-		     lc->io_old_psw.addr);
+	report_abort("Unexpected io interrupt: on cpu %d at %#lx",
+		     stap(), lc->io_old_psw.addr);
 }
 
 void handle_svc_int(void)
 {
-	report_abort("Unexpected supervisor call interrupt: at %#lx",
-		     lc->svc_old_psw.addr);
+	report_abort("Unexpected supervisor call interrupt: on cpu %d at %#lx",
+		     stap(), lc->svc_old_psw.addr);
 }

Quoted 25 lines · David Hildenbrand

Quoted 12 lines · Janosch Frank

It's good to know which cpu broke the test.

Signed-off-by Janosch Frank <[email protected]>

---
 lib/s390x/interrupt.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

lib/s390x/interrupt.c +0 -0

index 05f30be..773752a 100644

lib/s390x/interrupt.c static void fixup_pgm_int(void) · line 112

Quoted 8 lines

lib/s390x/interrupt.c +2 -2

@@ -107,8 +107,8 @@ static void fixup_pgm_int(void)
 void handle_pgm_int(void)
 {
 	if (!pgm_int_expected)
-		report_abort("Unexpected program interrupt: %d at %#lx, ilen %d\n",
-			     lc->pgm_int_code, lc->pgm_old_psw.addr,
+		report_abort("Unexpected program interrupt: %d on cpu %d at %#lx, ilen %d\n",
+			     lc->pgm_int_code, stap(), lc->pgm_old_psw.addr,
 			     lc->pgm_int_id);

nit: "cpu: %d"

Quoted 1 line

Quoted 11 lines

lib/s390x/interrupt.c void handle_ext_int(void) · line 123

Quoted 9 lines

lib/s390x/interrupt.c +2 -2

 	pgm_int_expected = false;

@@ -119,8 +119,8 @@ void handle_ext_int(void)
 {
 	if (!ext_int_expected &&
 	    lc->ext_int_code != EXT_IRQ_SERVICE_SIG) {
-		report_abort("Unexpected external call interrupt (code %#x): at %#lx",
-			     lc->ext_int_code, lc->ext_old_psw.addr);
+		report_abort("Unexpected external call interrupt (code %#x): on cpu %d at %#lx",
+			     stap(), lc->ext_int_code, lc->ext_old_psw.addr);

nit: "(code %#x) on cpu: %d" ...

So, should I move the old : or add a second one?

It's good to know which cpu broke the test.

Signed-off-by Janosch Frank <[email protected]>

---
 lib/s390x/interrupt.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

Quoted 2 lines · On Thu, 16 Jan 2020 07:05:09 -0500 Janosch Frank <[email protected]> wrote:

It's good to know which cpu broke the test.

Quoted 4 lines

Signed-off-by Janosch Frank <[email protected]>

---
 lib/s390x/interrupt.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

With whatever formatting tweaks you choose:

Reviewed-by Cornelia Huck <[email protected]>

sigp is not synchronous on all hypervisors, so we need to wait until

the cpu runs until we return from the setup/start function.

As there was a lot of duplicate code a common function for cpu

restarts has been intropduced.

Signed-off-by Janosch Frank <[email protected]>

---
 lib/s390x/smp.c | 45 ++++++++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 21 deletions(-)

lib/s390x/smp.c +5 -0

index f57f420..f984a34 100644
@@ -104,35 +104,41 @@ int smp_cpu_stop_store_status(uint16_t addr)
 	return rc;
 }
 
+static int smp_cpu_restart_nolock(uint16_t addr, struct psw *psw)
+{
+	int rc;
+	struct cpu *cpu = smp_cpu_from_addr(addr);
+

Quoted 1 line · On Thu, 16 Jan 2020 07:05:10 -0500 Janosch Frank <[email protected]> wrote:

sigp is not synchronous on all hypervisors, so we need to wait until

"The architecture specifies that processing sigp orders may be asynchronous, and this is indeed the case on some hypervisors, so..."

? (Or is that overkill?)

Quoted 1 line

the cpu runs until we return from the setup/start function.

s/until we return/before we return/

Quoted 2 lines

As there was a lot of duplicate code a common function for cpu

s/code/code,/

Quoted 1 line

restarts has been intropduced.

s/intropduced/introduced/

Quoted 23 lines

Signed-off-by Janosch Frank <[email protected]>

---
 lib/s390x/smp.c | 45 ++++++++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 21 deletions(-)

lib/s390x/smp.c +9 -0

index f57f420..f984a34 100644
@@ -104,35 +104,41 @@ int smp_cpu_stop_store_status(uint16_t addr)
 	return rc;
 }
 
+static int smp_cpu_restart_nolock(uint16_t addr, struct psw *psw)
+{
+	int rc;
+	struct cpu *cpu = smp_cpu_from_addr(addr);
+
+	if (!cpu)
+		return -1;
+	if (psw) {
+		cpu->lowcore->restart_new_psw.mask = psw->mask;

lib/s390x/smp.c int smp_cpu_stop_store_status(uint16_t addr) · line 121

Quoted 6 lines

lib/s390x/smp.c +6 -0

+		cpu->lowcore->restart_new_psw.addr = psw->addr;
+	}
+	rc = sigp(addr, SIGP_RESTART, 0, NULL);
+	if (rc)
+		return rc;
+	while (!smp_cpu_running(addr)) { mb(); }

Maybe split this statement? Also, maybe add a comment

/*
* The order has been accepted, but the actual restart may not
* have been performed yet, so wait until the cpu is running. */

?

lib/s390x/smp.c int smp_cpu_stop_store_status(uint16_t addr) · line 124

Quoted 3 lines

lib/s390x/smp.c +3 -0

+	cpu->active = true;
+	return 0;
+}

The changes look good to me AFAICS.

Reviewed-by Cornelia Huck <[email protected]>

Quoted 12 lines · Cornelia Huck

Quoted 9 lines · On Thu, 16 Jan 2020 07:05:10 -0500 Janosch Frank <[email protected]> wrote:

lib/s390x/smp.c +9 -0

+static int smp_cpu_restart_nolock(uint16_t addr, struct psw *psw)
+{
+	int rc;
+	struct cpu *cpu = smp_cpu_from_addr(addr);
+
+	if (!cpu)
+		return -1;
+	if (psw) {
+		cpu->lowcore->restart_new_psw.mask = psw->mask;

Quoted 8 lines

lib/s390x/smp.c int smp_cpu_stop_store_status(uint16_t addr) · line 121

Quoted 6 lines

lib/s390x/smp.c +6 -0

+		cpu->lowcore->restart_new_psw.addr = psw->addr;
+	}
+	rc = sigp(addr, SIGP_RESTART, 0, NULL);
+	if (rc)
+		return rc;
+	while (!smp_cpu_running(addr)) { mb(); }  

Maybe split this statement? Also, maybe add a comment

/* Wait until the target cpu is running */
?

This is not QEMU with two line ifs taking up 3 lines :)

Quoted 9 lines

/*
* The order has been accepted, but the actual restart may not
* have been performed yet, so wait until the cpu is running. */

?

Quoted 7 lines

lib/s390x/smp.c int smp_cpu_stop_store_status(uint16_t addr) · line 124

Quoted 3 lines

lib/s390x/smp.c +3 -0

+	cpu->active = true;
+	return 0;
+}  

The changes look good to me AFAICS.

Reviewed-by Cornelia Huck <[email protected]>

Thanks!

We store the user provided psw address into restart new, so a psw restart does not lead us through setup again.

Also we wait on smp_cpu_setup() until the cpu has finished setup before returning. This is necessary for z/VM and LPAR where sigp is asynchronous.

Signed-off-by Janosch Frank <[email protected]>

---
 lib/s390x/smp.c  | 2 ++
 s390x/cstart64.S | 2 ++
 2 files changed, 4 insertions(+)

lib/s390x/smp.c +2 -0

index f984a34..0a0bc1c 100644
@@ -199,6 +199,8 @@ int smp_cpu_setup(uint16_t addr, struct psw psw)
 
 	/* Start processing */
 	smp_cpu_restart_nolock(addr, NULL);
+	/* Wait until the cpu has finished setup and started the provided psw */
+	while (lc->restart_new_psw.addr != psw.addr) { mb(); }
 out:
 	spin_unlock(&lock);
 	return rc;

s390x/cstart64.S +0 -0

index 86dd4c4..9af6bb3 100644

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 | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

s390x/smp.c +0 -0

index d430638..ce3215d 100644
@@ -176,16 +176,31 @@ static void test_emcall(void)
 	report_prefix_pop();
 }

s390x/smp.c +9 -1

+}
+
 static void test_reset_initial(void)
 {
 	struct cpu_status *status = alloc_pages(0);
+	uint64_t nullp[12] = {};
 	struct psw psw;
 
 	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);

@@ -196,6 +211,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");

Quoted 19 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 | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

s390x/smp.c +0 -0

index d430638..ce3215d 100644
@@ -176,16 +176,31 @@ static void test_emcall(void)
 	report_prefix_pop();
 }
 

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

Quoted 9 lines

s390x/smp.c +9 -0

+/* Used to dirty registers of cpu #1 before it is reset */
+static void test_func_initial(void)
+{
+	lctlg(1, 0x42000UL);
+	lctlg(7, 0x43000UL);
+	lctlg(13, 0x44000UL);
+	testflag = 1;
+	mb();
+	cpu_loop();

Can we make cpu_loop() the default when this function returns? (IOW, an endless loop whenever a cpu finished executing the function?)

Do we need the mb() here?

Quoted 31 lines

s390x/smp.c +9 -1

+}
+
 static void test_reset_initial(void)
 {
 	struct cpu_status *status = alloc_pages(0);
+	uint64_t nullp[12] = {};
 	struct psw psw;
 
 	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);

@@ -196,6 +211,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");

Quoted 20 lines · David Hildenbrand

Quoted 19 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 | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

s390x/smp.c +0 -0

index d430638..ce3215d 100644
@@ -176,16 +176,31 @@ static void test_emcall(void)
 	report_prefix_pop();
 }
 

Quoted 12 lines

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

Quoted 9 lines

s390x/smp.c +9 -0

+/* Used to dirty registers of cpu #1 before it is reset */
+static void test_func_initial(void)
+{
+	lctlg(1, 0x42000UL);
+	lctlg(7, 0x43000UL);
+	lctlg(13, 0x44000UL);
+	testflag = 1;
+	mb();
+	cpu_loop();

Can we make cpu_loop() the default when this function returns? (IOW, an endless loop whenever a cpu finished executing the function?)

So adding it to cstart64.S after the br 14?

Quoted 3 lines

Do we need the mb() here?

Would the compiler reorder the lctlcg and testflag if it could?

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

Signed-off-by Janosch Frank <[email protected]>

Reviewed-by Thomas Huth <[email protected]>

Reviewed-by Cornelia Huck <[email protected]>

---
 s390x/smp.c | 3 +++
 1 file changed, 3 insertions(+)

s390x/smp.c +0 -0

index ce3215d..97a9dda 100644

s390x/smp.c +0 -0

 	lctlg(1, 0x42000UL);
 	lctlg(7, 0x43000UL);
 	lctlg(13, 0x44000UL);

Quoted 14 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]>

Reviewed-by Thomas Huth <[email protected]>

Reviewed-by Cornelia Huck <[email protected]>

---
 s390x/smp.c | 3 +++
 1 file changed, 3 insertions(+)

s390x/smp.c +0 -0

index ce3215d..97a9dda 100644

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

Quoted 6 lines

s390x/smp.c +3 -0

@@ -179,6 +179,9 @@ static void test_emcall(void)
 /* Used to dirty registers of cpu #1 before it is reset */
 static void test_func_initial(void)
 {
+	asm volatile(
+		"	sfpc	%0\n"
+		: : "d" (0x11) : );

FWIW, I'd make this one easier to read

asm volatile("sfpc %0\n" :: "d" (0x11));

Quoted 24 lines · David Hildenbrand

Quoted 14 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]>

Reviewed-by Thomas Huth <[email protected]>

Reviewed-by Cornelia Huck <[email protected]>

---
 s390x/smp.c | 3 +++
 1 file changed, 3 insertions(+)

s390x/smp.c +0 -0

index ce3215d..97a9dda 100644

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

Quoted 6 lines

s390x/smp.c +3 -0

@@ -179,6 +179,9 @@ static void test_emcall(void)
 /* Used to dirty registers of cpu #1 before it is reset */
 static void test_func_initial(void)
 {
+	asm volatile(
+		"	sfpc	%0\n"
+		: : "d" (0x11) : );

FWIW, I'd make this one easier to read

Quoted 1 line

asm volatile("sfpc %0\n" :: "d" (0x11));

By the way, since it's only one line, you can also drop the \n here.

Thomas