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 <npig...@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 | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
