Re: [Qemu-devel] [PATCH v4 3/5] spapr: Implement H_CONFER

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3167 bytes --]

On Tue, Jul 16, 2019 at 12:47:24PM +1000, Nicholas Piggin wrote:
> This does not do directed yielding and is not quite as strict as PAPR
> specifies in terms of precise dispatch behaviour. This generally will
> mean suboptimal performance, rather than guest misbehaviour. Linux
> does not rely on exact dispatch behaviour.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  hw/ppc/spapr_hcall.c | 48 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 8b208ab259..28d58113be 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1069,6 +1069,53 @@ 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];
> +    uint32_t dispatch = args[1];
> +    PowerPCCPU *target_cpu = spapr_find_cpu(target);
> +    CPUState *target_cs = CPU(target_cpu);
> +    CPUState *cs = CPU(cpu);
> +    SpaprCpuState *spapr_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 && !target_cs) {
> +        return H_PARAMETER;
> +    }

Should we return an error if a targeted yield is attempted, rather
than pretend we've done it?

> +
> +    spapr_cpu = spapr_cpu_state(target_cpu);
> +
> +    /*
> +     * PAPR specifies waiting until proded in this case, without dispatch

s/proded/prodded/

> +     * counter check.
> +     */
> +    if (cpu == target_cpu) {
> +        if (spapr_cpu->prod) {
> +            spapr_cpu->prod = false;
> +            return H_SUCCESS;
> +        }
> +
> +        cs->halted = 1;
> +        cs->exception_index = EXCP_HALTED;
> +        cs->exit_request = 1;

Now that we're using this sequence in a bunch of places, I wonder if
we want a little helper function.

> +
> +        return H_SUCCESS;
> +    }
> +
> +    if (spapr_cpu->dispatch_counter != dispatch || (dispatch & 1) == 0) {
> +        return H_SUCCESS;
> +    }
> +
> +    cs->exception_index = EXCP_YIELD;
> +    cpu_loop_exit(cs);
> +
> +    return H_SUCCESS;
> +}
> +
>  static target_ulong h_prod(PowerPCCPU *cpu, SpaprMachineState *spapr,
>                             target_ulong opcode, target_ulong *args)
>  {
> @@ -1909,6 +1956,7 @@ 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);

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]