On 1/20/20 1:06 PM, David Hildenbrand wrote: > On 17.01.20 11:46, Janosch Frank wrote: >> 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 >> Reviewed-by: Cornelia Huck >> --- >> lib/s390x/smp.c | 50 ++++++++++++++++++++++++++++--------------------- >> 1 file changed, 29 insertions(+), 21 deletions(-) >> >> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c >> index f57f420..84e681d 100644 >> --- a/lib/s390x/smp.c >> +++ b/lib/s390x/smp.c >> @@ -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); > > I'd exchange these two (reverse christmas tree) Christmas is over > >> + >> + if (!cpu) >> + return -1; > > -EINVAL? > >> + if (psw) { >> + cpu->lowcore->restart_new_psw.mask = psw->mask; >> + cpu->lowcore->restart_new_psw.addr = psw->addr; >> + } > > Does this make sense to have optional? (the other CPU will execute > random crap if not set, won't it?) Well, I have restarts in the smp test and I don't want to always pass a psw if I know what the last restart psw was. Simply restarting into test_func or wait_for_flag is certainly no problem. > >> + 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(); > > 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) Good point