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

[Nicholas Piggin]

David Gibson's on April 17, 2019 11:59 am:
> On Tue, Apr 16, 2019 at 02:55:52PM +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>
>> ---
>> 
>> v3: Removed wrong comment about GPR3, drop H_JOIN for now (at least until
>> it is tested some more in Linux/KVM), and expand the comment about not
>> prod bit.
>> 
>>  hw/ppc/spapr_hcall.c | 71 ++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 71 insertions(+)
>> 
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index 8a736797b9..8892ad008b 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -1065,6 +1065,74 @@ 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];
>> +    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;
>> +    }
>> +
>> +    /*
>> +     * PAPR calls for waiting until proded in this case (or presumably
>> +     * an external interrupt if MSR[EE]=1, without dispatch sequence count
>> +     * check.
> 
> Is this comment complete?  It's missing a closing parenthesis at the
> very least.

Needs closing parenthesis after EE=1 AFAIKS. Good catch.
 
>> +static target_ulong h_prod(PowerPCCPU *cpu, SpaprMachineState *spapr,
>> +                           target_ulong opcode, target_ulong *args)
>> +{
>> +    target_long target = args[0];
>> +    CPUState *cs;
>> +
>> +    /*
>> +     * PAPR specifies there should be a prod flag should be associated with
>> +     * a vCPU, which gets set here, tested by H_CEDE, and cleared any time
>> +     * the vCPU is dispatched, including via preemption.
>> +     *
>> +     * We don't implement this because it is not used by Linux. The bit 
>> would
>> +     * be difficult or impossible to use properly because preemption can not
>> +     * be prevented so dispatch sequence count would have to somehow be used
>> +     * to detect it.
> 
> Hm.  AFAIK the dispatch sequence count only exists with KVM, so I
> don't see how testing it would fit with a userspace implementation of PROD.

Right, I think even if you did have it, the prod bit really doesn't
offer much value. You could perhaps enter CEDE without hard disabling
interrupts race-free, something like --

  do {
    seq = dispatch_seq;
    if (work_pending)
      return;
  } while (seq != dispatch_seq);
  hcall(H_CEDE);

  vs

  work_pending = 1;
  hcall(H_PROD);

But Linux certainly doesn't do anything like this, and after the
barriers needed and added complexity to work out the idle state on
the producer side, it's unlikely to be worthwhile (and either way
dwarfed by the hcall cost).

Buut... in theory it does not conform to PAPR exactly. We would need
to clear it on all guest dispatch, and also implement the dispatch
counter if we are worried about this.

> 
>> +     */
>> +
>> +    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 +1928,9 @@ 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);
> 
> You're no longer enabling the KVM CONFER and PROD hypercalls.  Are
> they enabled by default, or is that an intentional change?

Oh, it was not intentional, I must not understand how this works. Why
is this no longer enabling the those hcalls?

Thanks,
Nick