Re: [Qemu-devel] [PATCH v5 4/4] spapr: Implement H_JOIN

2019-07-17 Thread Nicholas Piggin
Greg Kurz's on July 18, 2019 3:30 am:
> On Wed, 17 Jul 2019 15:39:52 +1000
> Nicholas Piggin  wrote:
> 
>> This has been useful to modify and test the Linux pseries suspend
>> code but it requires modification to the guest to call it (due to
>> being gated by other unimplemented features). It is not otherwise
>> used by Linux yet, but work is slowly progressing there.
>> 
>> Signed-off-by: Nicholas Piggin 
>> ---
>> Changes since v4:
>> - Style
>> 
>>  hw/ppc/spapr.c   |  1 +
>>  hw/ppc/spapr_hcall.c | 45 
>>  2 files changed, 46 insertions(+)
>> 
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 13c423347e..59cd24f9c3 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -1066,6 +1066,7 @@ static void spapr_dt_rtas(SpaprMachineState *spapr, 
>> void *fdt)
>>  add_str(hypertas, "hcall-tce");
>>  add_str(hypertas, "hcall-vio");
>>  add_str(hypertas, "hcall-splpar");
>> +add_str(hypertas, "hcall-join");
>>  add_str(hypertas, "hcall-bulk");
>>  add_str(hypertas, "hcall-set-mode");
>>  add_str(hypertas, "hcall-sprg0");
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index 5e655172b2..57c1ee0fe1 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -1069,6 +1069,48 @@ 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 = >env;
>> +CPUState *cs;
>> +SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>> +bool last_unjoined = true;
>> +
>> +if (env->msr & (1ULL << MSR_EE)) {
>> +return H_BAD_MODE;
>> +}
>> +
>> +if (spapr_cpu->prod) {
>> +spapr_cpu->prod = false;
>> +return H_SUCCESS;
>> +}
>> +
> 
> PAPR says that H_JOIN "performs the equivalent of a H_CONFER (proc=self)",
> unless called by the last unjoined thread, in which case H_CONTINUE
> should be returned. It thus seems that the spapr_cpu->prod check should
> be done after the loop below otherwise if the last active thread was
> just prodded (can happen?), it won't return the expected value, and...

Good lawyering, I would say you are right.

>> +CPU_FOREACH(cs) {
>> +PowerPCCPU *c = POWERPC_CPU(cs);
>> +CPUPPCState *e = >env;
>> +if (c == cpu) {
>> +continue;
>> +}
>> +
>> +/* Don't have a way to indicate joined, so use halted && MSR[EE]=0 
>> */
>> +if (!cs->halted || (e->msr & (1ULL << MSR_EE))) {
>> +last_unjoined = false;
>> +break;
>> +}
>> +}
>> +if (last_unjoined) {
>> +return H_CONTINUE;
>> +}
>> +
>> +cs = CPU(cpu);
>> +cs->halted = 1;
>> +cs->exception_index = EXCP_HALTED;
>> +cs->exit_request = 1;
>> +
>> +return H_SUCCESS;
> 
> ... then, you can maybe factor out this code to an h_confer_self()
> helper to be called by h_join() and h_confer() ?

I'll take a look.

Thanks,
Nick



Re: [Qemu-devel] [PATCH v5 4/4] spapr: Implement H_JOIN

2019-07-17 Thread Greg Kurz
On Wed, 17 Jul 2019 15:39:52 +1000
Nicholas Piggin  wrote:

> This has been useful to modify and test the Linux pseries suspend
> code but it requires modification to the guest to call it (due to
> being gated by other unimplemented features). It is not otherwise
> used by Linux yet, but work is slowly progressing there.
> 
> Signed-off-by: Nicholas Piggin 
> ---
> Changes since v4:
> - Style
> 
>  hw/ppc/spapr.c   |  1 +
>  hw/ppc/spapr_hcall.c | 45 
>  2 files changed, 46 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 13c423347e..59cd24f9c3 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1066,6 +1066,7 @@ static void spapr_dt_rtas(SpaprMachineState *spapr, 
> void *fdt)
>  add_str(hypertas, "hcall-tce");
>  add_str(hypertas, "hcall-vio");
>  add_str(hypertas, "hcall-splpar");
> +add_str(hypertas, "hcall-join");
>  add_str(hypertas, "hcall-bulk");
>  add_str(hypertas, "hcall-set-mode");
>  add_str(hypertas, "hcall-sprg0");
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 5e655172b2..57c1ee0fe1 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1069,6 +1069,48 @@ 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 = >env;
> +CPUState *cs;
> +SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
> +bool last_unjoined = true;
> +
> +if (env->msr & (1ULL << MSR_EE)) {
> +return H_BAD_MODE;
> +}
> +
> +if (spapr_cpu->prod) {
> +spapr_cpu->prod = false;
> +return H_SUCCESS;
> +}
> +

PAPR says that H_JOIN "performs the equivalent of a H_CONFER (proc=self)",
unless called by the last unjoined thread, in which case H_CONTINUE
should be returned. It thus seems that the spapr_cpu->prod check should
be done after the loop below otherwise if the last active thread was
just prodded (can happen?), it won't return the expected value, and...

> +CPU_FOREACH(cs) {
> +PowerPCCPU *c = POWERPC_CPU(cs);
> +CPUPPCState *e = >env;
> +if (c == cpu) {
> +continue;
> +}
> +
> +/* Don't have a way to indicate joined, so use halted && MSR[EE]=0 */
> +if (!cs->halted || (e->msr & (1ULL << MSR_EE))) {
> +last_unjoined = false;
> +break;
> +}
> +}
> +if (last_unjoined) {
> +return H_CONTINUE;
> +}
> +
> +cs = CPU(cpu);
> +cs->halted = 1;
> +cs->exception_index = EXCP_HALTED;
> +cs->exit_request = 1;
> +
> +return H_SUCCESS;

... then, you can maybe factor out this code to an h_confer_self()
helper to be called by h_join() and h_confer() ?

> +}
> +
>  static target_ulong h_confer(PowerPCCPU *cpu, SpaprMachineState *spapr,
> target_ulong opcode, target_ulong *args)
>  {
> @@ -1979,6 +2021,9 @@ static void hypercall_register_types(void)
>  spapr_register_hypercall(H_CONFER, h_confer);
>  spapr_register_hypercall(H_PROD, h_prod);
>  
> +/* hcall-join */
> +spapr_register_hypercall(H_JOIN, h_join);
> +
>  spapr_register_hypercall(H_SIGNAL_SYS_RESET, h_signal_sys_reset);
>  
>  /* processor register resource access h-calls */