s390x: smp: Improve smp code and reset checks

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

Acked-by David Hildenbrand <[email protected]>

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

s390x/smp.c +13 -7

index e37eb56..3e8cf3e 100644
@@ -53,7 +53,7 @@ static void test_start(void)
 	psw.addr = (unsigned long)test_func;
 
 	set_flag(0);
-	smp_cpu_setup(1, psw);
+	smp_cpu_start(1, psw);
 	wait_for_flag();
 	report(1, "start");
 }

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

@@ -137,9 +138,8 @@ static void test_ecall(void)
 
 	report_prefix_push("ecall");
 	set_flag(0);
-	smp_cpu_destroy(1);
 
-	smp_cpu_setup(1, psw);
+	smp_cpu_start(1, psw);
 	wait_for_flag();
 	set_flag(0);
 	sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);

@@ -172,9 +172,8 @@ static void test_emcall(void)
 
 	report_prefix_push("emcall");
 	set_flag(0);
-	smp_cpu_destroy(1);
 
-	smp_cpu_setup(1, psw);
+	smp_cpu_start(1, psw);
 	wait_for_flag();
 	set_flag(0);
 	sigp(1, SIGP_EMERGENCY_SIGNAL, 0, NULL);

@@ -192,7 +191,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);

@@ -223,7 +222,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");

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

@@ -239,6 +239,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 99 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]>

Acked-by David Hildenbrand <[email protected]>

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

s390x/smp.c +14 -7

index e37eb56..3e8cf3e 100644
@@ -53,7 +53,7 @@ static void test_start(void)
 	psw.addr = (unsigned long)test_func;
 
 	set_flag(0);
-	smp_cpu_setup(1, psw);
+	smp_cpu_start(1, psw);
 	wait_for_flag();
 	report(1, "start");
 }

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

@@ -137,9 +138,8 @@ static void test_ecall(void)
 
 	report_prefix_push("ecall");
 	set_flag(0);
-	smp_cpu_destroy(1);
 
-	smp_cpu_setup(1, psw);
+	smp_cpu_start(1, psw);
 	wait_for_flag();
 	set_flag(0);
 	sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);

@@ -172,9 +172,8 @@ static void test_emcall(void)
 
 	report_prefix_push("emcall");
 	set_flag(0);
-	smp_cpu_destroy(1);
 
-	smp_cpu_setup(1, psw);
+	smp_cpu_start(1, psw);
 	wait_for_flag();
 	set_flag(0);
 	sigp(1, SIGP_EMERGENCY_SIGNAL, 0, NULL);

@@ -192,7 +191,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);

@@ -223,7 +222,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");

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

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

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

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

Quoted 3 lines

s390x/smp.c +0 -0

 done:
 	report_prefix_pop();

With this patch, I get timeouts under TCG. Seems to loop forever.

Quoted 100 lines · David Hildenbrand

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

Acked-by David Hildenbrand <[email protected]>

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

s390x/smp.c +14 -7

index e37eb56..3e8cf3e 100644
@@ -53,7 +53,7 @@ static void test_start(void)
 	psw.addr = (unsigned long)test_func;
 
 	set_flag(0);
-	smp_cpu_setup(1, psw);
+	smp_cpu_start(1, psw);
 	wait_for_flag();
 	report(1, "start");
 }

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

@@ -137,9 +138,8 @@ static void test_ecall(void)
 
 	report_prefix_push("ecall");
 	set_flag(0);
-	smp_cpu_destroy(1);
 
-	smp_cpu_setup(1, psw);
+	smp_cpu_start(1, psw);
 	wait_for_flag();
 	set_flag(0);
 	sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);

@@ -172,9 +172,8 @@ static void test_emcall(void)
 
 	report_prefix_push("emcall");
 	set_flag(0);
-	smp_cpu_destroy(1);
 
-	smp_cpu_setup(1, psw);
+	smp_cpu_start(1, psw);
 	wait_for_flag();
 	set_flag(0);
 	sigp(1, SIGP_EMERGENCY_SIGNAL, 0, NULL);

@@ -192,7 +191,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);

@@ -223,7 +222,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");

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

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

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

Quoted 6 lines

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

Quoted 3 lines

s390x/smp.c +0 -0

 done:
 	report_prefix_pop();

With this patch, I get timeouts under TCG. Seems to loop forever.

The branch works on lpar and kvm without a problem.

Quoted 107 lines · Janosch Frank

Quoted 106 lines · David Hildenbrand

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

Acked-by David Hildenbrand <[email protected]>

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

s390x/smp.c +13 -7

index e37eb56..3e8cf3e 100644
@@ -53,7 +53,7 @@ static void test_start(void)
 	psw.addr = (unsigned long)test_func;
 
 	set_flag(0);
-	smp_cpu_setup(1, psw);
+	smp_cpu_start(1, psw);
 	wait_for_flag();
 	report(1, "start");
 }

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

@@ -137,9 +138,8 @@ static void test_ecall(void)
 
 	report_prefix_push("ecall");
 	set_flag(0);
-	smp_cpu_destroy(1);
 
-	smp_cpu_setup(1, psw);
+	smp_cpu_start(1, psw);
 	wait_for_flag();
 	set_flag(0);
 	sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);

@@ -172,9 +172,8 @@ static void test_emcall(void)
 
 	report_prefix_push("emcall");
 	set_flag(0);
-	smp_cpu_destroy(1);
 
-	smp_cpu_setup(1, psw);
+	smp_cpu_start(1, psw);
 	wait_for_flag();
 	set_flag(0);
 	sigp(1, SIGP_EMERGENCY_SIGNAL, 0, NULL);

@@ -192,7 +191,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);

@@ -223,7 +222,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");

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

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

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

Quoted 7 lines

s390x/smp.c +1 -0

@@ -247,6 +253,7 @@ int main(void)
 	test_emcall();
 	test_reset();
 	test_reset_initial();
+	smp_cpu_destroy(1);
 
 done:
 	report_prefix_pop();

With this patch, I get timeouts under TCG. Seems to loop forever.

Quoted 1 line

The branch works on lpar and kvm without a problem.

Which could mean that either TCG is broken or your test is broken (e.g., a race condition that does not trigger under LPAR because it's faster, or some undocumented/not guaranteed behavior).

Quoted 108 lines · David Hildenbrand

Quoted 107 lines · Janosch Frank

Quoted 106 lines · David Hildenbrand

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

Acked-by David Hildenbrand <[email protected]>

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

s390x/smp.c +13 -7

index e37eb56..3e8cf3e 100644
@@ -53,7 +53,7 @@ static void test_start(void)
 	psw.addr = (unsigned long)test_func;
 
 	set_flag(0);
-	smp_cpu_setup(1, psw);
+	smp_cpu_start(1, psw);
 	wait_for_flag();
 	report(1, "start");
 }

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

@@ -137,9 +138,8 @@ static void test_ecall(void)
 
 	report_prefix_push("ecall");
 	set_flag(0);
-	smp_cpu_destroy(1);
 
-	smp_cpu_setup(1, psw);
+	smp_cpu_start(1, psw);
 	wait_for_flag();
 	set_flag(0);
 	sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);

@@ -172,9 +172,8 @@ static void test_emcall(void)
 
 	report_prefix_push("emcall");
 	set_flag(0);
-	smp_cpu_destroy(1);
 
-	smp_cpu_setup(1, psw);
+	smp_cpu_start(1, psw);
 	wait_for_flag();
 	set_flag(0);
 	sigp(1, SIGP_EMERGENCY_SIGNAL, 0, NULL);

@@ -192,7 +191,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);

@@ -223,7 +222,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");

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

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

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

Quoted 7 lines

s390x/smp.c +1 -0

@@ -247,6 +253,7 @@ int main(void)
 	test_emcall();
 	test_reset();
 	test_reset_initial();
+	smp_cpu_destroy(1);
 
 done:
 	report_prefix_pop();

With this patch, I get timeouts under TCG. Seems to loop forever.

Quoted 6 lines

Quoted 1 line

The branch works on lpar and kvm without a problem.

Which could mean that either TCG is broken or your test is broken (e.g., a race condition that does not trigger under LPAR because it's faster, or some undocumented/not guaranteed behavior).

So, the test works every now and then under TCG.

It seems to work very reliably with "-smp 2"

With smp 8, it sometimes works, sometimes not.

t480s: ~/git/kvm-unit-tests (kein Branch, Rebase von s390x-prep) $ ./s390x-run s390x/smp.elf -smp 8 /home/dhildenb/git/qemu/s390x-softmmu/qemu-system-s390x -nodefaults

(quoted diff) +0 -2

-nographic -machine s390-ccw-virtio,accel=tcg -chardev stdio,id=con0
-device sclpconsole,chardev=con0 -kernel s390x/smp.elf -smp 8 # -initrd

/tmp/tmp.UL9ZaqoKBW
SMP: Initializing, found 8 cpus
PASS: smp: start
PASS: smp: stop
PASS: smp: stop store status: prefix
PASS: smp: stop store status: stack
PASS: smp: store status at address: running: incorrect state
PASS: smp: store status at address: running: status not written
PASS: smp: store status at address: stopped: status written
PASS: smp: ecall: ecall
PASS: smp: emcall: ecall
PASS: smp: cpu reset: cpu stopped
PASS: smp: reset initial: clear: psw
PASS: smp: reset initial: clear: prefix
PASS: smp: reset initial: clear: fpc
PASS: smp: reset initial: clear: cpu timer
PASS: smp: reset initial: clear: todpr
PASS: smp: reset initial: initialized: cr0 == 0xE0
PASS: smp: reset initial: initialized: cr14 == 0xC2000000
PASS: smp: reset initial: cpu stopped
SUMMARY: 18 tests

EXIT: STATUS=1

t480s: ~/git/kvm-unit-tests (kein Branch, Rebase von s390x-prep) $ ./s390x-run s390x/smp.elf -smp 8 /home/dhildenb/git/qemu/s390x-softmmu/qemu-system-s390x -nodefaults

(quoted diff) +0 -2

-nographic -machine s390-ccw-virtio,accel=tcg -chardev stdio,id=con0
-device sclpconsole,chardev=con0 -kernel s390x/smp.elf -smp 8 # -initrd

/tmp/tmp.csifrEDgnC
SMP: Initializing, found 8 cpus
PASS: smp: start
PASS: smp: stop
PASS: smp: stop store status: prefix
FAIL: smp: stop store status: stack
PASS: smp: store status at address: running: incorrect state
PASS: smp: store status at address: running: status not written
PASS: smp: store status at address: stopped: status written [... hang]

Note that there is a previous failure for "smp: stop store status:
stack" whenever the test will hang later.

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

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

Reviewed-by David Hildenbrand <[email protected]>

Reviewed-by Cornelia Huck <[email protected]>

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

lib/s390x/interrupt.c +2 -2

index 05f30be..773752a 100644
@@ -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);
 
 	pgm_int_expected = false;

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

The architecture specifies that processing sigp orders may be asynchronous, and this is indeed the case on some hypervisors, so we need to wait until the cpu runs before we return from the setup/start function.

As there was a lot of duplicate code, a common function for cpu restarts has been introduced.

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

Reviewed-by Cornelia Huck <[email protected]>

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

lib/s390x/smp.c +14 -0

index f57f420..84e681d 100644
@@ -104,35 +104,46 @@ 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;
+		cpu->lowcore->restart_new_psw.addr = psw->addr;
+	}
+	rc = sigp(addr, SIGP_RESTART, 0, NULL);
+	if (rc)
+		return rc;

Quoted 40 lines · Janosch Frank

The architecture specifies that processing sigp orders may be asynchronous, and this is indeed the case on some hypervisors, so we need to wait until the cpu runs before we return from the setup/start function.

As there was a lot of duplicate code, a common function for cpu restarts has been introduced.

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

Reviewed-by Cornelia Huck <[email protected]>

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

lib/s390x/smp.c +18 -0

index f57f420..84e681d 100644
@@ -104,35 +104,46 @@ 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;
+		cpu->lowcore->restart_new_psw.addr = psw->addr;
+	}
+	rc = sigp(addr, SIGP_RESTART, 0, NULL);
+	if (rc)
+		return rc;
+	/*
+	 * 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 130

Quoted 6 lines

lib/s390x/smp.c +6 -0

+	while (!smp_cpu_running(addr))
+		mb();
+	cpu->active = true;
+	return 0;
+}
+

Just wondering what happened to my comment

"Should you make sure to stop the CPU before issuing the restart? Otherwise you will get false positives if it is still running (but hasn't processed the RESTART yet)"

?

IOW, should we have a SIGP_STOP before issuing the SIGP_RESTART?

Up to now a secondary cpu could have returned from the function it was executing and ending up somewhere in cstart64.S. This was mostly circumvented by an endless loop in the function that it executed.

Let's add a loop to the end of the cpu setup, so we don't have to rely on added loops in the tests.

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

---
 s390x/cstart64.S | 2 ++
 1 file changed, 2 insertions(+)

s390x/cstart64.S +0 -0

index 9af6bb3..5fd8d2f 100644

Up to now a secondary cpu could have returned from the function it was executing and ending up somewhere in cstart64.S. This was mostly circumvented by an endless loop in the function that it executed.

Let's add a loop to the end of the cpu setup, so we don't have to rely on added loops in the tests.

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

---
 s390x/cstart64.S | 2 ++
 1 file changed, 2 insertions(+)

s390x/cstart64.S +0 -0

index 9af6bb3..5fd8d2f 100644

Up to now a secondary cpu could have returned from the function it was executing and ending up somewhere in cstart64.S. This was mostly circumvented by an endless loop in the function that it executed.

Let's add a loop to the end of the cpu setup, so we don't have to rely on added loops in the tests.

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

---
 s390x/cstart64.S | 2 ++
 1 file changed, 2 insertions(+)

s390x/cstart64.S +0 -0

index 9af6bb3..5fd8d2f 100644

Quoted 18 lines · Janosch Frank

Up to now a secondary cpu could have returned from the function it was executing and ending up somewhere in cstart64.S. This was mostly circumvented by an endless loop in the function that it executed.

Let's add a loop to the end of the cpu setup, so we don't have to rely on added loops in the tests.

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

---
 s390x/cstart64.S | 2 ++
 1 file changed, 2 insertions(+)

s390x/cstart64.S +0 -0

index 9af6bb3..5fd8d2f 100644

s390x/cstart64.S smp_cpu_setup_state: · line 169

Quoted 8 lines

s390x/cstart64.S +2 -0

@@ -162,6 +162,8 @@ smp_cpu_setup_state:
 	/* We should only go once through cpu setup and not for every restart */
 	stg	%r14, GEN_LC_RESTART_NEW_PSW + 8
 	br	%r14
+	/* If the function returns, just loop here */
+0:	j	0
 
 pgm_int:
 	SAVE_REGS

This patch collides with a patch I have still queued

Author: Janosch Frank <[email protected]>
Date:   Wed Dec 11 06:59:22 2019 -0500

s390x: smp: Use full PSW to bringup new cpu

Up to now we ignored the psw mask and only used the psw address when bringing up a new cpu. For DAT we need to also load the mask, so let's do that.

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

Reviewed-by David Hildenbrand <[email protected]>

Message-Id: <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

In that patch we use a lpswe to jump to the target code, not a br. So the return address will no longer be stored in %14 and this code here would stop working AFAIKS.

Shall I drop that patch for now?

Now that we have a loop which is executed after we return from the main function of a secondary cpu, we can remove the surplus loops.

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

---
 s390x/smp.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

Now that we have a loop which is executed after we return from the main function of a secondary cpu, we can remove the surplus loops.

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

---
 s390x/smp.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

s390x/smp.c +0 -6

index 3e8cf3e..45f1d80 100644
@@ -35,15 +35,9 @@ static void set_flag(int val)
 	mb();
 }
 
-static void cpu_loop(void)
-{
-	for (;;) {}
-}
-
 static void test_func(void)
 {
 	set_flag(1);
-	cpu_loop();
 }
 
 static void test_start(void)