| Message ID | 20190412093603.18232-1-npiggin@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] spapr: add splpar hcalls H_JOIN, H_PROD, H_CONFER | expand |
On Fri, Apr 12, 2019 at 07:36:03PM +1000, Nicholas Piggin wrote: > These implementations have a few deficiencies that are noted, but are > good enough for Linux to use. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > > Cleaned up checkpatch warnings, sorry I didn't realise that exists. > > hw/ppc/spapr_hcall.c | 88 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 88 insertions(+) > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > index 8a736797b9..e985bb694d 100644 > --- a/hw/ppc/spapr_hcall.c > +++ b/hw/ppc/spapr_hcall.c > @@ -1065,6 +1065,90 @@ 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 = CPU(cpu); > + > + if (env->msr & (1ULL << MSR_EE)) { > + return H_BAD_MODE; > + } > + > + /* > + * This should check for single-threaded mode. In practice, Linux > + * does not try to H_JOIN all CPUs. > + */ > + > + cs->halted = 1; > + cs->exception_index = EXCP_HALTED; > + cs->exit_request = 1; > + > + return H_SUCCESS; > +} > + > +static target_ulong h_confer(PowerPCCPU *cpu, SpaprMachineState *spapr, > + target_ulong opcode, target_ulong *args) > +{ > + target_long target = args[0]; > + CPUState *cs = CPU(cpu); > + > + /* > + * This does not do a targeted yield or confer, but check the parameter > + * anyway. -1 means confer to all/any other CPUs. > + */ > + if (target != -1 && !CPU(spapr_find_cpu(target))) { > + return H_PARAMETER; > + } > + > + /* > + * H_CONFER with target == this is not exactly the same as H_JOIN > + * according to PAPR (e.g., MSR[EE] check and single threaded check > + * is not done in this case), but unlikely to matter. > + */ > + if (cpu == spapr_find_cpu(target)) { > + return h_join(cpu, spapr, opcode, args); > + } > + > + /* > + * This does not implement the dispatch sequence check that PAPR calls for, > + * but PAPR also specifies a stronger implementation where the target must > + * be run (or EE, or H_PROD) before H_CONFER returns. Without such a hard > + * scheduling requirement implemented, there is no correctness reason to > + * implement the dispatch sequence check. > + */ > + cs->exception_index = EXCP_YIELD; > + cpu_loop_exit(cs); > + > + return H_SUCCESS; > +} > + > +/* > + * H_PROD and H_CONFER are specified to only modify GPR r3, which is not > + * achievable running under KVM, Uh.. why not? > although KVM already implements H_CONFER > + * this way. And this seems to contradict the above. > + */ > +static target_ulong h_prod(PowerPCCPU *cpu, SpaprMachineState *spapr, > + target_ulong opcode, target_ulong *args) > +{ > + target_long target = args[0]; > + CPUState *cs; > + > + /* > + * Should set the prod flag in the VPA. So.. why doesn't it? > + */ > + > + cs = CPU(spapr_find_cpu(target)); > + if (!cs) { > + return H_PARAMETER; > + } > + > + cs->halted = 0; > + qemu_cpu_kick(cs); > + > + return H_SUCCESS; > +} > + > static target_ulong h_rtas(PowerPCCPU *cpu, SpaprMachineState *spapr, > target_ulong opcode, target_ulong *args) > { > @@ -1860,6 +1944,10 @@ 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_JOIN, h_join); I don't see any sign that H_JOIN is implemented in KVM, although H_CONFER and H_PROD certainly are. > + spapr_register_hypercall(H_PROD, h_prod); > + > spapr_register_hypercall(H_SIGNAL_SYS_RESET, h_signal_sys_reset); > > /* processor register resource access h-calls */ Don't we also need to add something to hypertas-calls to advertise the availability of these calls to the guest?
David Gibson's on April 15, 2019 2:13 pm: > On Fri, Apr 12, 2019 at 07:36:03PM +1000, Nicholas Piggin wrote: >> These implementations have a few deficiencies that are noted, but are >> good enough for Linux to use. >> >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >> --- >> >> Cleaned up checkpatch warnings, sorry I didn't realise that exists. >> >> hw/ppc/spapr_hcall.c | 88 ++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 88 insertions(+) >> >> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c >> index 8a736797b9..e985bb694d 100644 >> --- a/hw/ppc/spapr_hcall.c >> +++ b/hw/ppc/spapr_hcall.c >> @@ -1065,6 +1065,90 @@ 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 = CPU(cpu); >> + >> + if (env->msr & (1ULL << MSR_EE)) { >> + return H_BAD_MODE; >> + } >> + >> + /* >> + * This should check for single-threaded mode. In practice, Linux >> + * does not try to H_JOIN all CPUs. >> + */ >> + >> + cs->halted = 1; >> + cs->exception_index = EXCP_HALTED; >> + cs->exit_request = 1; >> + >> + return H_SUCCESS; >> +} >> + >> +static target_ulong h_confer(PowerPCCPU *cpu, SpaprMachineState *spapr, >> + target_ulong opcode, target_ulong *args) >> +{ >> + target_long target = args[0]; >> + CPUState *cs = CPU(cpu); >> + >> + /* >> + * This does not do a targeted yield or confer, but check the parameter >> + * anyway. -1 means confer to all/any other CPUs. >> + */ >> + if (target != -1 && !CPU(spapr_find_cpu(target))) { >> + return H_PARAMETER; >> + } >> + >> + /* >> + * H_CONFER with target == this is not exactly the same as H_JOIN >> + * according to PAPR (e.g., MSR[EE] check and single threaded check >> + * is not done in this case), but unlikely to matter. >> + */ >> + if (cpu == spapr_find_cpu(target)) { >> + return h_join(cpu, spapr, opcode, args); >> + } >> + >> + /* >> + * This does not implement the dispatch sequence check that PAPR calls for, >> + * but PAPR also specifies a stronger implementation where the target must >> + * be run (or EE, or H_PROD) before H_CONFER returns. Without such a hard >> + * scheduling requirement implemented, there is no correctness reason to >> + * implement the dispatch sequence check. >> + */ >> + cs->exception_index = EXCP_YIELD; >> + cpu_loop_exit(cs); >> + >> + return H_SUCCESS; >> +} >> + >> +/* >> + * H_PROD and H_CONFER are specified to only modify GPR r3, which is not >> + * achievable running under KVM, > > Uh.. why not? sc 1 handler kills ctr and cr0, but I misread the spec, they are not specified to modify _only_ GPR r3, but rather the only GPR modified is r3. cr0 and ctr still in the kill set. >> although KVM already implements H_CONFER >> + * this way. > > And this seems to contradict the above. I just meat it already is implemented with those clobbers, but as above that's not a problem. I'll take the comment out. >> + */ >> +static target_ulong h_prod(PowerPCCPU *cpu, SpaprMachineState *spapr, >> + target_ulong opcode, target_ulong *args) >> +{ >> + target_long target = args[0]; >> + CPUState *cs; >> + >> + /* >> + * Should set the prod flag in the VPA. > > So.. why doesn't it? It needs to be cleared at all vCPU dispatch points to SPEC, not just when calling H_CEDE as Ben's patch had. I think complexity would be significant for questionable benefit. Like the dispatch sequence, it seems like the test is trying to cover some race condition for the client but does not really do it well (and for Linux not necessary). prod bit is cleared after vCPU returns from preemption, so it can clear at any time and you can't rely on it, unless you look at dispatch sequence numbers to decipher if it was reset or not. KVM does implement something like the prodded flag as Ben's patch did but that's not to spec AFAIKS. > >> + */ >> + >> + cs = CPU(spapr_find_cpu(target)); >> + if (!cs) { >> + return H_PARAMETER; >> + } >> + >> + cs->halted = 0; >> + qemu_cpu_kick(cs); >> + >> + return H_SUCCESS; >> +} >> + >> static target_ulong h_rtas(PowerPCCPU *cpu, SpaprMachineState *spapr, >> target_ulong opcode, target_ulong *args) >> { >> @@ -1860,6 +1944,10 @@ 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_JOIN, h_join); > > I don't see any sign that H_JOIN is implemented in KVM, although > H_CONFER and H_PROD certainly are. H_JOIN is not. >> + spapr_register_hypercall(H_PROD, h_prod); >> + >> spapr_register_hypercall(H_SIGNAL_SYS_RESET, h_signal_sys_reset); >> >> /* processor register resource access h-calls */ > > Don't we also need to add something to hypertas-calls to advertise the > availability of these calls to the guest? hcall-splpar is for H_CONFER and H_PROD, H_JOIN also wants hcall-join actually, good point. I could split the patch up and add H_CONFER and H_PROD first. Thanks, Nick
On Mon, Apr 15, 2019 at 03:06:43PM +1000, Nicholas Piggin wrote: > David Gibson's on April 15, 2019 2:13 pm: > > On Fri, Apr 12, 2019 at 07:36:03PM +1000, Nicholas Piggin wrote: > >> These implementations have a few deficiencies that are noted, but are > >> good enough for Linux to use. > >> > >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > >> --- > >> > >> Cleaned up checkpatch warnings, sorry I didn't realise that exists. > >> > >> hw/ppc/spapr_hcall.c | 88 ++++++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 88 insertions(+) > >> > >> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > >> index 8a736797b9..e985bb694d 100644 > >> --- a/hw/ppc/spapr_hcall.c > >> +++ b/hw/ppc/spapr_hcall.c > >> @@ -1065,6 +1065,90 @@ 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 = CPU(cpu); > >> + > >> + if (env->msr & (1ULL << MSR_EE)) { > >> + return H_BAD_MODE; > >> + } > >> + > >> + /* > >> + * This should check for single-threaded mode. In practice, Linux > >> + * does not try to H_JOIN all CPUs. > >> + */ > >> + > >> + cs->halted = 1; > >> + cs->exception_index = EXCP_HALTED; > >> + cs->exit_request = 1; > >> + > >> + return H_SUCCESS; > >> +} > >> + > >> +static target_ulong h_confer(PowerPCCPU *cpu, SpaprMachineState *spapr, > >> + target_ulong opcode, target_ulong *args) > >> +{ > >> + target_long target = args[0]; > >> + CPUState *cs = CPU(cpu); > >> + > >> + /* > >> + * This does not do a targeted yield or confer, but check the parameter > >> + * anyway. -1 means confer to all/any other CPUs. > >> + */ > >> + if (target != -1 && !CPU(spapr_find_cpu(target))) { > >> + return H_PARAMETER; > >> + } > >> + > >> + /* > >> + * H_CONFER with target == this is not exactly the same as H_JOIN > >> + * according to PAPR (e.g., MSR[EE] check and single threaded check > >> + * is not done in this case), but unlikely to matter. > >> + */ > >> + if (cpu == spapr_find_cpu(target)) { > >> + return h_join(cpu, spapr, opcode, args); > >> + } > >> + > >> + /* > >> + * This does not implement the dispatch sequence check that PAPR calls for, > >> + * but PAPR also specifies a stronger implementation where the target must > >> + * be run (or EE, or H_PROD) before H_CONFER returns. Without such a hard > >> + * scheduling requirement implemented, there is no correctness reason to > >> + * implement the dispatch sequence check. > >> + */ > >> + cs->exception_index = EXCP_YIELD; > >> + cpu_loop_exit(cs); > >> + > >> + return H_SUCCESS; > >> +} > >> + > >> +/* > >> + * H_PROD and H_CONFER are specified to only modify GPR r3, which is not > >> + * achievable running under KVM, > > > > Uh.. why not? > > sc 1 handler kills ctr and cr0, but I misread the spec, they are not > specified to modify _only_ GPR r3, but rather the only GPR modified > is r3. cr0 and ctr still in the kill set. Ah, ok. > > >> although KVM already implements H_CONFER > >> + * this way. > > > > And this seems to contradict the above. > > I just meat it already is implemented with those clobbers, but as > above that's not a problem. I'll take the comment out. > > >> + */ > >> +static target_ulong h_prod(PowerPCCPU *cpu, SpaprMachineState *spapr, > >> + target_ulong opcode, target_ulong *args) > >> +{ > >> + target_long target = args[0]; > >> + CPUState *cs; > >> + > >> + /* > >> + * Should set the prod flag in the VPA. > > > > So.. why doesn't it? > > It needs to be cleared at all vCPU dispatch points to SPEC, not just > when calling H_CEDE as Ben's patch had. I think complexity would be > significant for questionable benefit. Like the dispatch sequence, it > seems like the test is trying to cover some race condition for the > client but does not really do it well (and for Linux not necessary). > > prod bit is cleared after vCPU returns from preemption, so it can > clear at any time and you can't rely on it, unless you look at > dispatch sequence numbers to decipher if it was reset or not. > > KVM does implement something like the prodded flag as Ben's patch did > but that's not to spec AFAIKS. Hm, ok. Maybe include this rationale in the comment here. > > > > >> + */ > >> + > >> + cs = CPU(spapr_find_cpu(target)); > >> + if (!cs) { > >> + return H_PARAMETER; > >> + } > >> + > >> + cs->halted = 0; > >> + qemu_cpu_kick(cs); > >> + > >> + return H_SUCCESS; > >> +} > >> + > >> static target_ulong h_rtas(PowerPCCPU *cpu, SpaprMachineState *spapr, > >> target_ulong opcode, target_ulong *args) > >> { > >> @@ -1860,6 +1944,10 @@ 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_JOIN, h_join); > > > > I don't see any sign that H_JOIN is implemented in KVM, although > > H_CONFER and H_PROD certainly are. > > H_JOIN is not. Right, so we shouldn't be trying to enable it. What will happen if we use a KVM H_CONFGER and H_PROD along with a userspace H_JOIN? > >> + spapr_register_hypercall(H_PROD, h_prod); > >> + > >> spapr_register_hypercall(H_SIGNAL_SYS_RESET, h_signal_sys_reset); > >> > >> /* processor register resource access h-calls */ > > > > Don't we also need to add something to hypertas-calls to advertise the > > availability of these calls to the guest? > > hcall-splpar is for H_CONFER and H_PROD, H_JOIN also wants > hcall-join actually, good point. I could split the patch up and add > H_CONFER and H_PROD first. Ok.
David Gibson's on April 17, 2019 10:47 am: > On Mon, Apr 15, 2019 at 03:06:43PM +1000, Nicholas Piggin wrote: >> It needs to be cleared at all vCPU dispatch points to SPEC, not just >> when calling H_CEDE as Ben's patch had. I think complexity would be >> significant for questionable benefit. Like the dispatch sequence, it >> seems like the test is trying to cover some race condition for the >> client but does not really do it well (and for Linux not necessary). >> >> prod bit is cleared after vCPU returns from preemption, so it can >> clear at any time and you can't rely on it, unless you look at >> dispatch sequence numbers to decipher if it was reset or not. >> >> KVM does implement something like the prodded flag as Ben's patch did >> but that's not to spec AFAIKS. > > Hm, ok. Maybe include this rationale in the comment here. Done (hopefully) >> > I don't see any sign that H_JOIN is implemented in KVM, although >> > H_CONFER and H_PROD certainly are. >> >> H_JOIN is not. > > Right, so we shouldn't be trying to enable it. What will happen if we > use a KVM H_CONFGER and H_PROD along with a userspace H_JOIN? Yeah good question I'll have to test before sending the H_JOIN part again. May have to modify KVM to make that work properly. Thanks, Nick
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c index 8a736797b9..e985bb694d 100644 --- a/hw/ppc/spapr_hcall.c +++ b/hw/ppc/spapr_hcall.c @@ -1065,6 +1065,90 @@ 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 = CPU(cpu); + + if (env->msr & (1ULL << MSR_EE)) { + return H_BAD_MODE; + } + + /* + * This should check for single-threaded mode. In practice, Linux + * does not try to H_JOIN all CPUs. + */ + + cs->halted = 1; + cs->exception_index = EXCP_HALTED; + cs->exit_request = 1; + + return H_SUCCESS; +} + +static target_ulong h_confer(PowerPCCPU *cpu, SpaprMachineState *spapr, + target_ulong opcode, target_ulong *args) +{ + target_long target = args[0]; + CPUState *cs = CPU(cpu); + + /* + * This does not do a targeted yield or confer, but check the parameter + * anyway. -1 means confer to all/any other CPUs. + */ + if (target != -1 && !CPU(spapr_find_cpu(target))) { + return H_PARAMETER; + } + + /* + * H_CONFER with target == this is not exactly the same as H_JOIN + * according to PAPR (e.g., MSR[EE] check and single threaded check + * is not done in this case), but unlikely to matter. + */ + if (cpu == spapr_find_cpu(target)) { + return h_join(cpu, spapr, opcode, args); + } + + /* + * This does not implement the dispatch sequence check that PAPR calls for, + * but PAPR also specifies a stronger implementation where the target must + * be run (or EE, or H_PROD) before H_CONFER returns. Without such a hard + * scheduling requirement implemented, there is no correctness reason to + * implement the dispatch sequence check. + */ + cs->exception_index = EXCP_YIELD; + cpu_loop_exit(cs); + + return H_SUCCESS; +} + +/* + * H_PROD and H_CONFER are specified to only modify GPR r3, which is not + * achievable running under KVM, although KVM already implements H_CONFER + * this way. + */ +static target_ulong h_prod(PowerPCCPU *cpu, SpaprMachineState *spapr, + target_ulong opcode, target_ulong *args) +{ + target_long target = args[0]; + CPUState *cs; + + /* + * Should set the prod flag in the VPA. + */ + + cs = CPU(spapr_find_cpu(target)); + if (!cs) { + return H_PARAMETER; + } + + cs->halted = 0; + qemu_cpu_kick(cs); + + return H_SUCCESS; +} + static target_ulong h_rtas(PowerPCCPU *cpu, SpaprMachineState *spapr, target_ulong opcode, target_ulong *args) { @@ -1860,6 +1944,10 @@ 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_JOIN, h_join); + spapr_register_hypercall(H_PROD, h_prod); + spapr_register_hypercall(H_SIGNAL_SYS_RESET, h_signal_sys_reset); /* processor register resource access h-calls */
These implementations have a few deficiencies that are noted, but are good enough for Linux to use. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- Cleaned up checkpatch warnings, sorry I didn't realise that exists. hw/ppc/spapr_hcall.c | 88 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+)