Skip to contentSkip to conversation

s390x: smp: Improve smp code and reset checks

PATCH In review Janosch Frank 3 participants 15 messages Feb 1, 2020 – Feb 3, 2020

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

v5 cover s390x: smp: Improve smp code and reset checks

Let's cleanup the smp library and smp tests.

GIT: https://github.com/frankjaa/kvm-unit-tests/tree/smp_cleanup

v5:
* Split up series into three parts to make review easier
* Greetings from FOSDEM :-)

V4:
* Introduce set_flag() for manipulating testflag
* Cleanup of stray braces and mb()s

v3:
* Added patch to introduce cpu loop in cpu setup
* Added patch that removes cpu loops in favor of the previously introduced one
* Fixed inline assembly for fpc dirtying
* Moved cpu stop hunk from first into the second patch
* Reworked patch #4 commit message and added a comment when waiting for PU

v2:
* Added cpu stop to test_store_status()
* Added smp_cpu_destroy() to the end of smp.c main()
* New patch that prints cpu id on interrupt errors
* New patch that reworks cpu start in the smp library (needed for lpar)
* nullp is now an array

Janosch Frank (7):
s390x: smp: Cleanup smp.c
s390x: smp: Fix ecall and emcall report strings
s390x: Stop the cpu that is executing exit()
s390x: Add cpu id to interrupt error prints
s390x: smp: Only use smp_cpu_setup once
s390x: smp: Rework cpu start and active tracking
s390x: smp: Wait for cpu setup to finish

 lib/s390x/interrupt.c | 20 +++++------
 lib/s390x/io.c        |  2 +-
 lib/s390x/smp.c       | 59 +++++++++++++++++++------------
 s390x/cstart64.S      |  2 ++
 s390x/smp.c           | 80 ++++++++++++++++++++++++++-----------------
 5 files changed, 100 insertions(+), 63 deletions(-)
Signature · 1 line
2.20.1
#1 lore raw

PATCH v5 1/7 s390x: smp: Cleanup smp.c

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

Also let's remove some stray spaces and always set the tesflag before using it.

---
 s390x/smp.c | 55 ++++++++++++++++++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 22 deletions(-)
s390x/smp.c +33 -22
index ab7e46c..e37eb56 100644
@@ -22,6 +22,19 @@
 
 static int testflag = 0;
 
+static void wait_for_flag(void)
+{
+	while (!testflag)
+		mb();
+}
+
+static void set_flag(int val)
+{
+	mb();
+	testflag = val;
+	mb();
+}
+
 static void cpu_loop(void)
 {
 	for (;;) {}
@@ -29,21 +42,19 @@ static void cpu_loop(void)
 
 static void test_func(void)
 {
-	testflag = 1;
-	mb();
+	set_flag(1);
 	cpu_loop();
 }
 
 static void test_start(void)
 {
 	struct psw psw;
-	psw.mask =  extract_psw_mask();
+	psw.mask = extract_psw_mask();
 	psw.addr = (unsigned long)test_func;
 
+	set_flag(0);
 	smp_cpu_setup(1, psw);
-	while (!testflag) {
-		mb();
-	}
+	wait_for_flag();
 	report(1, "start");
 }
 
@@ -112,27 +123,27 @@ static void ecall(void)
 	mask = extract_psw_mask();
 	mask |= PSW_MASK_EXT;
 	load_psw_mask(mask);
-	testflag = 1;
+	set_flag(1);
 	while (lc->ext_int_code != 0x1202) { mb(); }
 	report(1, "ecall");
-	testflag= 1;
+	set_flag(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;
+	set_flag(0);
 	smp_cpu_destroy(1);
 
 	smp_cpu_setup(1, psw);
-	while (!testflag) { mb(); }
-	testflag= 0;
+	wait_for_flag();
+	set_flag(0);
 	sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);
-	while(!testflag) {mb();}
+	wait_for_flag();
 	smp_cpu_stop(1);
 	report_prefix_pop();
 }
@@ -147,27 +158,27 @@ static void emcall(void)
 	mask = extract_psw_mask();
 	mask |= PSW_MASK_EXT;
 	load_psw_mask(mask);
-	testflag= 1;
+	set_flag(1);
 	while (lc->ext_int_code != 0x1201) { mb(); }
 	report(1, "ecall");
-	testflag = 1;
+	set_flag(1);
 }
 
 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;
+	set_flag(0);
 	smp_cpu_destroy(1);
 
 	smp_cpu_setup(1, psw);
-	while (!testflag) { mb(); }
-	testflag= 0;
+	wait_for_flag();
+	set_flag(0);
 	sigp(1, SIGP_EMERGENCY_SIGNAL, 0, NULL);
-	while(!testflag) { mb(); }
+	wait_for_flag();
 	smp_cpu_stop(1);
 	report_prefix_pop();
 }
@@ -177,7 +188,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 +219,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
#2 lore raw

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

Quoted 4 lines · On Sat, 1 Feb 2020 10:28:45 -0500 Janosch Frank <[email protected]> wrote:

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

Also let's remove some stray spaces and always set the tesflag before

s/tesflag/testflag/

Quoted 9 lines

using it.

---
 s390x/smp.c | 55 ++++++++++++++++++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 22 deletions(-)
#3 lore raw

PATCH v5 2/7 s390x: smp: Fix ecall and emcall report strings

In reply to Janosch Frank

Instead of "smp: ecall: ecall" we now get "smp: ecall: received".

---
 s390x/smp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
s390x/smp.c +2 -2
index e37eb56..93a9594 100644
@@ -125,7 +125,7 @@ static void ecall(void)
 	load_psw_mask(mask);
 	set_flag(1);
 	while (lc->ext_int_code != 0x1202) { mb(); }
-	report(1, "ecall");
+	report(1, "received");
 	set_flag(1);
 }
 
@@ -160,7 +160,7 @@ static void emcall(void)
 	load_psw_mask(mask);
 	set_flag(1);
 	while (lc->ext_int_code != 0x1201) { mb(); }
-	report(1, "ecall");
+	report(1, "received");
 	set_flag(1);
 }
 
Signature · 1 line
2.20.1
Reviewed-by on v5 2/7
#4 lore raw

Re: [kvm-unit-tests PATCH v5 2/7] s390x: smp: Fix ecall and emcall report strings

Quoted 21 lines · Janosch Frank

Instead of "smp: ecall: ecall" we now get "smp: ecall: received".

---
 s390x/smp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
s390x/smp.c +1 -1
index e37eb56..93a9594 100644
@@ -125,7 +125,7 @@ static void ecall(void)
 	load_psw_mask(mask);
 	set_flag(1);
 	while (lc->ext_int_code != 0x1202) { mb(); }
-	report(1, "ecall");
+	report(1, "received");
 	set_flag(1);
 }
 

s390x/smp.c static void emcall(void) · line 165

Quoted 7 lines
s390x/smp.c +1 -1
@@ -160,7 +160,7 @@ static void emcall(void)
 	load_psw_mask(mask);
 	set_flag(1);
 	while (lc->ext_int_code != 0x1201) { mb(); }
-	report(1, "ecall");
+	report(1, "received");
 	set_flag(1);
 }
Reviewed-by on v5 2/7
#5 lore raw

Re: [kvm-unit-tests PATCH v5 2/7] s390x: smp: Fix ecall and emcall report strings

In reply to Janosch Frank
Quoted 6 lines · On Sat, 1 Feb 2020 10:28:46 -0500 Janosch Frank <[email protected]> wrote:

Instead of "smp: ecall: ecall" we now get "smp: ecall: received".

---
 s390x/smp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
#6 lore raw

PATCH v5 3/7 s390x: Stop the cpu that is executing exit()

In reply to Janosch Frank

CPU 0 is not necessarily the CPU which does the exit if we ran into a test abort situation. So, let's ask stap() which cpu does the exit and stop it on exit.

---
 lib/s390x/io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
lib/s390x/io.c +1 -1
index 32f09b5..e091c37 100644
@@ -46,6 +46,6 @@ void exit(int code)
 	smp_teardown();
 	printf("\nEXIT: STATUS=%d\n", ((code) << 1) | 1);
 	while (1) {
-		sigp(0, SIGP_STOP, 0, NULL);
+		sigp(stap(), SIGP_STOP, 0, NULL);
 	}
 }
Signature · 1 line
2.20.1
Reviewed-by on v5 3/7
#7 lore raw

Re: [kvm-unit-tests PATCH v5 3/7] s390x: Stop the cpu that is executing exit()

Quoted 14 lines · Janosch Frank

CPU 0 is not necessarily the CPU which does the exit if we ran into a test abort situation. So, let's ask stap() which cpu does the exit and stop it on exit.

---
 lib/s390x/io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
lib/s390x/io.c +0 -0
index 32f09b5..e091c37 100644

lib/s390x/io.c void exit(int code) · line 51

Quoted 7 lines
lib/s390x/io.c +1 -1
@@ -46,6 +46,6 @@ void exit(int code)
 	smp_teardown();
 	printf("\nEXIT: STATUS=%d\n", ((code) << 1) | 1);
 	while (1) {
-		sigp(0, SIGP_STOP, 0, NULL);
+		sigp(stap(), SIGP_STOP, 0, NULL);
 	}
 }

Right, smp_teardown stops already all CPUs except for the current one, so this is the last one running here.

Reviewed-by on v5 3/7
#8 lore raw

Re: [kvm-unit-tests PATCH v5 3/7] s390x: Stop the cpu that is executing exit()

In reply to Janosch Frank
Quoted 8 lines · On Sat, 1 Feb 2020 10:28:47 -0500 Janosch Frank <[email protected]> wrote:

CPU 0 is not necessarily the CPU which does the exit if we ran into a test abort situation. So, let's ask stap() which cpu does the exit and stop it on exit.

---
 lib/s390x/io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
#9 lore raw

PATCH v5 4/7 s390x: Add cpu id to interrupt error prints

In reply to Janosch Frank

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

---
 lib/s390x/interrupt.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)
lib/s390x/interrupt.c +10 -10
index ccb376a..3a40cac 100644
@@ -109,8 +109,8 @@ void handle_pgm_int(void)
 	if (!pgm_int_expected) {
 		/* Force sclp_busy to false, otherwise we will loop forever */
 		sclp_handle_ext();
-		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);
 	}
 
@@ -122,8 +122,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",
+			     lc->ext_int_code, stap(), lc->ext_old_psw.addr);
 		return;
 	}
 
@@ -140,18 +140,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);
 }
Signature · 1 line
2.20.1
Reviewed-by on v5 4/7
#10 lore raw

Re: [kvm-unit-tests PATCH v5 4/7] s390x: Add cpu id to interrupt error prints

Quoted 8 lines · Janosch Frank

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

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

PATCH v5 5/7 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.

---
 s390x/smp.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)
s390x/smp.c +14 -7
index 93a9594..fa40753 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);
 
 done:
 	report_prefix_pop();
Signature · 1 line
2.20.1
#12 lore raw

PATCH v5 6/7 s390x: smp: Rework cpu start and active tracking

In reply to 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.

---
 lib/s390x/smp.c | 56 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 35 insertions(+), 21 deletions(-)
lib/s390x/smp.c +35 -21
index f57f420..4578003 100644
@@ -104,35 +104,52 @@ 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;
+	}
+	/*
+	 * Stop the cpu, so we don't have a race between a running cpu
+	 * and the restart in the test that checks if the cpu is
+	 * running after the restart.
+	 */
+	smp_cpu_stop_nolock(addr, false);
+	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.
+	 */
+	while (!smp_cpu_running(addr))
+		mb();
+	cpu->active = true;
+	return 0;
+}
+
 int smp_cpu_restart(uint16_t addr)
 {
-	int rc = -1;
-	struct cpu *cpu;
+	int rc;
 
 	spin_lock(&lock);
-	cpu = smp_cpu_from_addr(addr);
-	if (cpu) {
-		rc = sigp(addr, SIGP_RESTART, 0, NULL);
-		cpu->active = true;
-	}
+	rc = smp_cpu_restart_nolock(addr, NULL);
 	spin_unlock(&lock);
 	return rc;
 }
 
 int smp_cpu_start(uint16_t addr, struct psw psw)
 {
-	int rc = -1;
-	struct cpu *cpu;
-	struct lowcore *lc;
+	int rc;
 
 	spin_lock(&lock);
-	cpu = smp_cpu_from_addr(addr);
-	if (cpu) {
-		lc = cpu->lowcore;
-		lc->restart_new_psw.mask = psw.mask;
-		lc->restart_new_psw.addr = psw.addr;
-		rc = sigp(addr, SIGP_RESTART, 0, NULL);
-	}
+	rc = smp_cpu_restart_nolock(addr, &psw);
 	spin_unlock(&lock);
 	return rc;
 }
@@ -192,10 +209,7 @@ int smp_cpu_setup(uint16_t addr, struct psw psw)
 	lc->sw_int_crs[0] = 0x0000000000040000UL;
 
 	/* Start processing */
-	rc = sigp_retry(cpu->addr, SIGP_RESTART, 0, NULL);
-	if (!rc)
-		cpu->active = true;
-
+	smp_cpu_restart_nolock(addr, NULL);
 out:
 	spin_unlock(&lock);
 	return rc;
Signature · 1 line
2.20.1
Reviewed-by on v5 6/7
#13 lore raw

Re: [kvm-unit-tests PATCH v5 6/7] s390x: smp: Rework cpu start and active tracking

Quoted 5 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.

Quoted 8 lines

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

---
 lib/s390x/smp.c | 56 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 35 insertions(+), 21 deletions(-)
#14 lore raw

PATCH v5 7/7 s390x: smp: Wait for cpu setup to finish

In reply to Janosch Frank

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.

---
 lib/s390x/smp.c  | 3 +++
 s390x/cstart64.S | 2 ++
 2 files changed, 5 insertions(+)
lib/s390x/smp.c +3 -0
index 4578003..3f86243 100644
@@ -210,6 +210,9 @@ 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 +2 -0
index 86dd4c4..9af6bb3 100644
@@ -159,6 +159,8 @@ smp_cpu_setup_state:
 	xgr	%r1, %r1
 	lmg     %r0, %r15, GEN_LC_SW_INT_GRS
 	lctlg   %c0, %c0, GEN_LC_SW_INT_CRS
+	/* We should only go once through cpu setup and not for every restart */
+	stg	%r14, GEN_LC_RESTART_NEW_PSW + 8
 	br	%r14
 
 pgm_int:
Signature · 1 line
2.20.1