| Message ID | 20190717053952.13729-4-npiggin@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | spapr: implement dispatch and suspend calls | expand |
On Wed, 17 Jul 2019 15:39:51 +1000 Nicholas Piggin <npiggin@gmail.com> wrote: > This does not do directed yielding and is not quite as strict as PAPR > specifies in terms of precise dispatch behaviour. This generally will > mean suboptimal performance, rather than guest misbehaviour. Linux > does not rely on exact dispatch behaviour. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- LGTM. Reviewed-by: Greg Kurz <groug@kaod.org> Just two minor comments, see below. > Changes since v4: > - Style, added justification comments, spelling. > - Fixed trying to dereference spapr_cpu for a -1 target. > > hw/ppc/spapr_hcall.c | 68 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 68 insertions(+) > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > index 8b208ab259..5e655172b2 100644 > --- a/hw/ppc/spapr_hcall.c > +++ b/hw/ppc/spapr_hcall.c > @@ -1069,6 +1069,73 @@ static target_ulong h_cede(PowerPCCPU *cpu, SpaprMachineState *spapr, > return H_SUCCESS; > } > > +static target_ulong h_confer(PowerPCCPU *cpu, SpaprMachineState *spapr, > + target_ulong opcode, target_ulong *args) > +{ > + target_long target = args[0]; > + uint32_t dispatch = args[1]; > + CPUState *cs = CPU(cpu); > + SpaprCpuState *spapr_cpu; > + > + /* > + * -1 means confer to all other CPUs without dispatch counter check, > + * otherwise it's a targeted confer. > + */ > + if (target != -1) { > + PowerPCCPU *target_cpu = spapr_find_cpu(target); > + CPUState *target_cs = CPU(target_cpu); > + unsigned int target_dispatch; Maybe make it uint32_t to be consistent with dispatch above, and this is the actual return type of ldl_be_phys() ? > + > + if (!target_cs) { This is the only user of target_cs, maybe drop it and use target_cpu instead ? > + return H_PARAMETER; > + } > + > + spapr_cpu = spapr_cpu_state(target_cpu); > + > + /* > + * target == self is a special case, we wait until prodded, without > + * dispatch counter check. > + */ > + if (cpu == target_cpu) { > + if (spapr_cpu->prod) { > + spapr_cpu->prod = false; > + > + return H_SUCCESS; > + } > + > + cs->halted = 1; > + cs->exception_index = EXCP_HALTED; > + cs->exit_request = 1; > + > + return H_SUCCESS; > + } > + > + if (!spapr_cpu->vpa_addr || ((dispatch & 1) == 0)) { > + return H_SUCCESS; > + } > + > + target_dispatch = ldl_be_phys(cs->as, > + spapr_cpu->vpa_addr + VPA_DISPATCH_COUNTER); > + if (target_dispatch != dispatch) { > + return H_SUCCESS; > + } > + > + /* > + * The targeted confer does not do anything special beyond yielding > + * the current vCPU, but even this should be better than nothing. > + * At least for single-threaded tcg, it gives the target a chance to > + * run before we run again. Multi-threaded tcg does not really do > + * anything with EXCP_YIELD yet. > + */ > + } > + > + cs->exception_index = EXCP_YIELD; > + cs->exit_request = 1; > + cpu_loop_exit(cs); > + > + return H_SUCCESS; > +} > + > static target_ulong h_prod(PowerPCCPU *cpu, SpaprMachineState *spapr, > target_ulong opcode, target_ulong *args) > { > @@ -1909,6 +1976,7 @@ static void hypercall_register_types(void) > /* hcall-splpar */ > spapr_register_hypercall(H_REGISTER_VPA, h_register_vpa); > spapr_register_hypercall(H_CEDE, h_cede); > + spapr_register_hypercall(H_CONFER, h_confer); > spapr_register_hypercall(H_PROD, h_prod); > > spapr_register_hypercall(H_SIGNAL_SYS_RESET, h_signal_sys_reset);
Greg Kurz's on July 18, 2019 3:00 am: > On Wed, 17 Jul 2019 15:39:51 +1000 > Nicholas Piggin <npiggin@gmail.com> wrote: > >> This does not do directed yielding and is not quite as strict as PAPR >> specifies in terms of precise dispatch behaviour. This generally will >> mean suboptimal performance, rather than guest misbehaviour. Linux >> does not rely on exact dispatch behaviour. >> >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >> --- > > LGTM. > > Reviewed-by: Greg Kurz <groug@kaod.org> > > Just two minor comments, see below. > >> Changes since v4: >> - Style, added justification comments, spelling. >> - Fixed trying to dereference spapr_cpu for a -1 target. >> >> hw/ppc/spapr_hcall.c | 68 ++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 68 insertions(+) >> >> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c >> index 8b208ab259..5e655172b2 100644 >> --- a/hw/ppc/spapr_hcall.c >> +++ b/hw/ppc/spapr_hcall.c >> @@ -1069,6 +1069,73 @@ static target_ulong h_cede(PowerPCCPU *cpu, SpaprMachineState *spapr, >> return H_SUCCESS; >> } >> >> +static target_ulong h_confer(PowerPCCPU *cpu, SpaprMachineState *spapr, >> + target_ulong opcode, target_ulong *args) >> +{ >> + target_long target = args[0]; >> + uint32_t dispatch = args[1]; >> + CPUState *cs = CPU(cpu); >> + SpaprCpuState *spapr_cpu; >> + >> + /* >> + * -1 means confer to all other CPUs without dispatch counter check, >> + * otherwise it's a targeted confer. >> + */ >> + if (target != -1) { >> + PowerPCCPU *target_cpu = spapr_find_cpu(target); >> + CPUState *target_cs = CPU(target_cpu); >> + unsigned int target_dispatch; > > Maybe make it uint32_t to be consistent with dispatch above, and this > is the actual return type of ldl_be_phys() ? Sure okay. >> + >> + if (!target_cs) { > > This is the only user of target_cs, maybe drop it and use target_cpu > instead ? That probably works, I'll try. Thanks, Nick
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c index 8b208ab259..5e655172b2 100644 --- a/hw/ppc/spapr_hcall.c +++ b/hw/ppc/spapr_hcall.c @@ -1069,6 +1069,73 @@ static target_ulong h_cede(PowerPCCPU *cpu, SpaprMachineState *spapr, return H_SUCCESS; } +static target_ulong h_confer(PowerPCCPU *cpu, SpaprMachineState *spapr, + target_ulong opcode, target_ulong *args) +{ + target_long target = args[0]; + uint32_t dispatch = args[1]; + CPUState *cs = CPU(cpu); + SpaprCpuState *spapr_cpu; + + /* + * -1 means confer to all other CPUs without dispatch counter check, + * otherwise it's a targeted confer. + */ + if (target != -1) { + PowerPCCPU *target_cpu = spapr_find_cpu(target); + CPUState *target_cs = CPU(target_cpu); + unsigned int target_dispatch; + + if (!target_cs) { + return H_PARAMETER; + } + + spapr_cpu = spapr_cpu_state(target_cpu); + + /* + * target == self is a special case, we wait until prodded, without + * dispatch counter check. + */ + if (cpu == target_cpu) { + if (spapr_cpu->prod) { + spapr_cpu->prod = false; + + return H_SUCCESS; + } + + cs->halted = 1; + cs->exception_index = EXCP_HALTED; + cs->exit_request = 1; + + return H_SUCCESS; + } + + if (!spapr_cpu->vpa_addr || ((dispatch & 1) == 0)) { + return H_SUCCESS; + } + + target_dispatch = ldl_be_phys(cs->as, + spapr_cpu->vpa_addr + VPA_DISPATCH_COUNTER); + if (target_dispatch != dispatch) { + return H_SUCCESS; + } + + /* + * The targeted confer does not do anything special beyond yielding + * the current vCPU, but even this should be better than nothing. + * At least for single-threaded tcg, it gives the target a chance to + * run before we run again. Multi-threaded tcg does not really do + * anything with EXCP_YIELD yet. + */ + } + + cs->exception_index = EXCP_YIELD; + cs->exit_request = 1; + cpu_loop_exit(cs); + + return H_SUCCESS; +} + static target_ulong h_prod(PowerPCCPU *cpu, SpaprMachineState *spapr, target_ulong opcode, target_ulong *args) { @@ -1909,6 +1976,7 @@ static void hypercall_register_types(void) /* hcall-splpar */ spapr_register_hypercall(H_REGISTER_VPA, h_register_vpa); spapr_register_hypercall(H_CEDE, h_cede); + spapr_register_hypercall(H_CONFER, h_confer); spapr_register_hypercall(H_PROD, h_prod); spapr_register_hypercall(H_SIGNAL_SYS_RESET, h_signal_sys_reset);
This does not do directed yielding and is not quite as strict as PAPR specifies in terms of precise dispatch behaviour. This generally will mean suboptimal performance, rather than guest misbehaviour. Linux does not rely on exact dispatch behaviour. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- Changes since v4: - Style, added justification comments, spelling. - Fixed trying to dereference spapr_cpu for a -1 target. hw/ppc/spapr_hcall.c | 68 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+)