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

[Nicholas Piggin]

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.

>> 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