s390x: smp: Improve smp code and reset checks
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
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
@@ -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
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()andset_flagfunctions.Also let's remove some stray spaces and always set the tesflag before
s/tesflag/testflag/
Quoted 9 lines
using it.
Signed-off-by Janosch Frank <[email protected]>Reviewed-by Thomas Huth <[email protected]>Reviewed-by Cornelia Huck <[email protected]>Reviewed-by David Hildenbrand <[email protected]>--- s390x/smp.c | 55 ++++++++++++++++++++++++++++++++--------------------- 1 file changed, 33 insertions(+), 22 deletions(-)
PATCH v5 2/7 s390x: smp: Fix ecall and emcall report strings
In reply to Janosch FrankInstead 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
@@ -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
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".
Signed-off-by Janosch Frank <[email protected]>--- s390x/smp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)s390x/smp.c +1
-1@@ -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 ); }
Re: [kvm-unit-tests PATCH v5 2/7] s390x: smp: Fix ecall and emcall report strings
In reply to Janosch FrankQuoted 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".
Signed-off-by Janosch Frank <[email protected]>--- s390x/smp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
PATCH v5 3/7 s390x: Stop the cpu that is executing exit()
In reply to Janosch FrankCPU 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
@@ -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
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.Signed-off-by Janosch Frank <[email protected]>--- lib/s390x/io.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)lib/s390x/io.c +0
-0
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.
Re: [kvm-unit-tests PATCH v5 3/7] s390x: Stop the cpu that is executing exit()
In reply to Janosch FrankQuoted 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.Signed-off-by Janosch Frank <[email protected]>--- lib/s390x/io.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
PATCH v5 4/7 s390x: Add cpu id to interrupt error prints
In reply to Janosch FrankIt'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
@@ -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
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.
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(-)
PATCH v5 5/7 s390x: smp: Only use smp_cpu_setup once
In reply to Janosch FrankLet'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
@@ -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
PATCH v5 6/7 s390x: smp: Rework cpu start and active tracking
In reply to Janosch FrankThe 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
@@ -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
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.
Signed-off-by Janosch Frank <[email protected]>Reviewed-by Cornelia Huck <[email protected]>--- lib/s390x/smp.c | 56 ++++++++++++++++++++++++++++++------------------- 1 file changed, 35 insertions(+), 21 deletions(-)
PATCH v5 7/7 s390x: smp: Wait for cpu setup to finish
In reply to Janosch FrankWe 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
@@ -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
@@ -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