Re: [Qemu-devel] [PATCH v2] spapr: add splpar hcalls H_JOIN, H_PROD, H_CONFER

[David Gibson]

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 <address@hidden>
> >> ---
> >> 
> >> 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