Greg Kurz's on July 18, 2019 3:30 am: > On Wed, 17 Jul 2019 15:39:52 +1000 > Nicholas Piggin <npig...@gmail.com> wrote: > >> This has been useful to modify and test the Linux pseries suspend >> code but it requires modification to the guest to call it (due to >> being gated by other unimplemented features). It is not otherwise >> used by Linux yet, but work is slowly progressing there. >> >> Signed-off-by: Nicholas Piggin <npig...@gmail.com> >> --- >> Changes since v4: >> - Style >> >> hw/ppc/spapr.c | 1 + >> hw/ppc/spapr_hcall.c | 45 ++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 46 insertions(+) >> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >> index 13c423347e..59cd24f9c3 100644 >> --- a/hw/ppc/spapr.c >> +++ b/hw/ppc/spapr.c >> @@ -1066,6 +1066,7 @@ static void spapr_dt_rtas(SpaprMachineState *spapr, >> void *fdt) >> add_str(hypertas, "hcall-tce"); >> add_str(hypertas, "hcall-vio"); >> add_str(hypertas, "hcall-splpar"); >> + add_str(hypertas, "hcall-join"); >> add_str(hypertas, "hcall-bulk"); >> add_str(hypertas, "hcall-set-mode"); >> add_str(hypertas, "hcall-sprg0"); >> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c >> index 5e655172b2..57c1ee0fe1 100644 >> --- a/hw/ppc/spapr_hcall.c >> +++ b/hw/ppc/spapr_hcall.c >> @@ -1069,6 +1069,48 @@ static target_ulong h_cede(PowerPCCPU *cpu, >> SpaprMachineState *spapr, >> return H_SUCCESS; >> } >> >> +static target_ulong h_join(PowerPCCPU *cpu, SpaprMachineState *spapr, >> + target_ulong opcode, target_ulong *args) >> +{ >> + CPUPPCState *env = &cpu->env; >> + CPUState *cs; >> + SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu); >> + bool last_unjoined = true; >> + >> + if (env->msr & (1ULL << MSR_EE)) { >> + return H_BAD_MODE; >> + } >> + >> + if (spapr_cpu->prod) { >> + spapr_cpu->prod = false; >> + return H_SUCCESS; >> + } >> + > > PAPR says that H_JOIN "performs the equivalent of a H_CONFER (proc=self)", > unless called by the last unjoined thread, in which case H_CONTINUE > should be returned. It thus seems that the spapr_cpu->prod check should > be done after the loop below otherwise if the last active thread was > just prodded (can happen?), it won't return the expected value, and...
Good lawyering, I would say you are right. >> + CPU_FOREACH(cs) { >> + PowerPCCPU *c = POWERPC_CPU(cs); >> + CPUPPCState *e = &c->env; >> + if (c == cpu) { >> + continue; >> + } >> + >> + /* Don't have a way to indicate joined, so use halted && MSR[EE]=0 >> */ >> + if (!cs->halted || (e->msr & (1ULL << MSR_EE))) { >> + last_unjoined = false; >> + break; >> + } >> + } >> + if (last_unjoined) { >> + return H_CONTINUE; >> + } >> + >> + cs = CPU(cpu); >> + cs->halted = 1; >> + cs->exception_index = EXCP_HALTED; >> + cs->exit_request = 1; >> + >> + return H_SUCCESS; > > ... then, you can maybe factor out this code to an h_confer_self() > helper to be called by h_join() and h_confer() ? I'll take a look. Thanks, Nick