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

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

[Nicholas Piggin]

These implementations have a few deficiencies that are noted, but are
good enough for Linux to use.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---

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.
+     */
+    if (cpu == spapr_find_cpu(target)) {
+        cs->halted = 1;
+        cs->exception_index = EXCP_HALTED;
+        cs->exit_request = 1;
+
+        return H_SUCCESS;
+    }
+
+    /*
+     * 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;
+}
+
+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.
+     */
+
+    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);
 
     /* processor register resource access h-calls */
-- 
2.20.1

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

[Nicholas Piggin]

These implementations have a few deficiencies that are noted, but are
good enough for Linux to use.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---

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.
+     */
+    if (cpu == spapr_find_cpu(target)) {
+        cs->halted = 1;
+        cs->exception_index = EXCP_HALTED;
+        cs->exit_request = 1;
+
+        return H_SUCCESS;
+    }
+
+    /*
+     * 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;
+}
+
+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.
+     */
+
+    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);
 
     /* processor register resource access h-calls */
-- 
2.20.1

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

[David Gibson]

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

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 <npiggin@gmail.com>
> ---
> 
> 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.

> +     */
> +    if (cpu == spapr_find_cpu(target)) {
> +        cs->halted = 1;
> +        cs->exception_index = EXCP_HALTED;
> +        cs->exit_request = 1;
> +
> +        return H_SUCCESS;
> +    }
> +
> +    /*
> +     * 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;
> +}
> +
> +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.

> +     */
> +
> +    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?

>      /* processor register resource access h-calls */

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

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

[David Gibson]

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

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 <npiggin@gmail.com>
> ---
> 
> 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.

> +     */
> +    if (cpu == spapr_find_cpu(target)) {
> +        cs->halted = 1;
> +        cs->exception_index = EXCP_HALTED;
> +        cs->exit_request = 1;
> +
> +        return H_SUCCESS;
> +    }
> +
> +    /*
> +     * 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;
> +}
> +
> +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.

> +     */
> +
> +    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?

>      /* processor register resource access h-calls */

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

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 <npiggin@gmail.com>
>> ---
>> 
>> 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

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 <npiggin@gmail.com>
>> ---
>> 
>> 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

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

[Greg Kurz]

On Wed, 17 Apr 2019 21:20:00 +1000
Nicholas Piggin <npiggin@gmail.com> wrote:
> [...]
> >> @@ -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?  
> 

AFAICT they seem to be enabled by default in HV KVM.

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

Since linux commit 699a0ea0823d ("KVM: PPC: Book3S: Controls for in-kernel 
sPAPR hypercall handling"), in-kernel hypercalls are disabled by default
and must be explicitely enabled by userspace. QEMU does that for some
hypercalls already (search kvmppc_enable_set_mode_hcall() in QEMU for an
example).

Since H_CONFER and H_PROD are listed in default_hcall_list[] in book3s_hv.c,
no need for QEMU to enable them in KVM.

Not sure about David's "no longer" wording though.

> Thanks,
> Nick
> 
> 

Cheers,

--
Greg

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

[Greg Kurz]

On Wed, 17 Apr 2019 21:20:00 +1000
Nicholas Piggin <npiggin@gmail.com> wrote:
> [...]
> >> @@ -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?  
> 

AFAICT they seem to be enabled by default in HV KVM.

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

Since linux commit 699a0ea0823d ("KVM: PPC: Book3S: Controls for in-kernel 
sPAPR hypercall handling"), in-kernel hypercalls are disabled by default
and must be explicitely enabled by userspace. QEMU does that for some
hypercalls already (search kvmppc_enable_set_mode_hcall() in QEMU for an
example).

Since H_CONFER and H_PROD are listed in default_hcall_list[] in book3s_hv.c,
no need for QEMU to enable them in KVM.

Not sure about David's "no longer" wording though.

> Thanks,
> Nick
> 
> 

Cheers,

--
Greg

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

[David Gibson]

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

On Wed, Apr 17, 2019 at 02:01:29PM +0200, Greg Kurz wrote:
> On Wed, 17 Apr 2019 21:20:00 +1000
> Nicholas Piggin <npiggin@gmail.com> wrote:
> > [...]
> > >> @@ -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?  
> > 
> 
> AFAICT they seem to be enabled by default in HV KVM.
> 
> > Oh, it was not intentional, I must not understand how this works. Why
> > is this no longer enabling the those hcalls?
> > 
> 
> Since linux commit 699a0ea0823d ("KVM: PPC: Book3S: Controls for in-kernel 
> sPAPR hypercall handling"), in-kernel hypercalls are disabled by default
> and must be explicitely enabled by userspace. QEMU does that for some
> hypercalls already (search kvmppc_enable_set_mode_hcall() in QEMU for an
> example).
> 
> Since H_CONFER and H_PROD are listed in default_hcall_list[] in book3s_hv.c,
> no need for QEMU to enable them in KVM.

Ah, ok.  Oops, that means the guest environment has been visibly
different for KVM and TCG all this time, which isn't great.

> Not sure about David's "no longer" wording though.


"no longer" meaning the previous patch version had some
kvmppc_enable_hcall(), but this version doesn't.

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

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

[David Gibson]

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

On Wed, Apr 17, 2019 at 02:01:29PM +0200, Greg Kurz wrote:
> On Wed, 17 Apr 2019 21:20:00 +1000
> Nicholas Piggin <npiggin@gmail.com> wrote:
> > [...]
> > >> @@ -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?  
> > 
> 
> AFAICT they seem to be enabled by default in HV KVM.
> 
> > Oh, it was not intentional, I must not understand how this works. Why
> > is this no longer enabling the those hcalls?
> > 
> 
> Since linux commit 699a0ea0823d ("KVM: PPC: Book3S: Controls for in-kernel 
> sPAPR hypercall handling"), in-kernel hypercalls are disabled by default
> and must be explicitely enabled by userspace. QEMU does that for some
> hypercalls already (search kvmppc_enable_set_mode_hcall() in QEMU for an
> example).
> 
> Since H_CONFER and H_PROD are listed in default_hcall_list[] in book3s_hv.c,
> no need for QEMU to enable them in KVM.

Ah, ok.  Oops, that means the guest environment has been visibly
different for KVM and TCG all this time, which isn't great.

> Not sure about David's "no longer" wording though.


"no longer" meaning the previous patch version had some
kvmppc_enable_hcall(), but this version doesn't.

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

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

[Greg Kurz]

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

On Wed, 17 Apr 2019 22:47:34 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Apr 17, 2019 at 02:01:29PM +0200, Greg Kurz wrote:
> > On Wed, 17 Apr 2019 21:20:00 +1000
> > Nicholas Piggin <npiggin@gmail.com> wrote:  
> > > [...]  
> > > >> @@ -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?    
> > >   
> > 
> > AFAICT they seem to be enabled by default in HV KVM.
> >   
> > > Oh, it was not intentional, I must not understand how this works. Why
> > > is this no longer enabling the those hcalls?
> > >   
> > 
> > Since linux commit 699a0ea0823d ("KVM: PPC: Book3S: Controls for in-kernel 
> > sPAPR hypercall handling"), in-kernel hypercalls are disabled by default
> > and must be explicitely enabled by userspace. QEMU does that for some
> > hypercalls already (search kvmppc_enable_set_mode_hcall() in QEMU for an
> > example).
> > 
> > Since H_CONFER and H_PROD are listed in default_hcall_list[] in book3s_hv.c,
> > no need for QEMU to enable them in KVM.  
> 
> Ah, ok.  Oops, that means the guest environment has been visibly
> different for KVM and TCG all this time, which isn't great.
> 
> > Not sure about David's "no longer" wording though.  
> 
> 
> "no longer" meaning the previous patch version had some
> kvmppc_enable_hcall(), but this version doesn't.
> 

Neither do the two previous versions of this patch actually, hence
my questioning... No big deal :)


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

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

[Nicholas Piggin]

David Gibson's on April 17, 2019 10:47 pm:
> On Wed, Apr 17, 2019 at 02:01:29PM +0200, Greg Kurz wrote:
>> On Wed, 17 Apr 2019 21:20:00 +1000
>> Nicholas Piggin <npiggin@gmail.com> wrote:
>> > [...]
>> > >> @@ -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?  
>> > 
>> 
>> AFAICT they seem to be enabled by default in HV KVM.
>> 
>> > Oh, it was not intentional, I must not understand how this works. Why
>> > is this no longer enabling the those hcalls?
>> > 
>> 
>> Since linux commit 699a0ea0823d ("KVM: PPC: Book3S: Controls for in-kernel 
>> sPAPR hypercall handling"), in-kernel hypercalls are disabled by default
>> and must be explicitely enabled by userspace. QEMU does that for some
>> hypercalls already (search kvmppc_enable_set_mode_hcall() in QEMU for an
>> example).
>> 
>> Since H_CONFER and H_PROD are listed in default_hcall_list[] in book3s_hv.c,
>> no need for QEMU to enable them in KVM.
> 
> Ah, ok.  Oops, that means the guest environment has been visibly
> different for KVM and TCG all this time, which isn't great.
> 
>> Not sure about David's "no longer" wording though.
> 
> 
> "no longer" meaning the previous patch version had some
> kvmppc_enable_hcall(), but this version doesn't.

Let me do one more iteration with the comment fixed up at least,
and I'll do a bit of testing with KVM vs TCG behaviour and see
if there are any problems around this.

Thanks,
Nick

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

[Nicholas Piggin]

David Gibson's on April 17, 2019 10:47 pm:
> On Wed, Apr 17, 2019 at 02:01:29PM +0200, Greg Kurz wrote:
>> On Wed, 17 Apr 2019 21:20:00 +1000
>> Nicholas Piggin <npiggin@gmail.com> wrote:
>> > [...]
>> > >> @@ -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?  
>> > 
>> 
>> AFAICT they seem to be enabled by default in HV KVM.
>> 
>> > Oh, it was not intentional, I must not understand how this works. Why
>> > is this no longer enabling the those hcalls?
>> > 
>> 
>> Since linux commit 699a0ea0823d ("KVM: PPC: Book3S: Controls for in-kernel 
>> sPAPR hypercall handling"), in-kernel hypercalls are disabled by default
>> and must be explicitely enabled by userspace. QEMU does that for some
>> hypercalls already (search kvmppc_enable_set_mode_hcall() in QEMU for an
>> example).
>> 
>> Since H_CONFER and H_PROD are listed in default_hcall_list[] in book3s_hv.c,
>> no need for QEMU to enable them in KVM.
> 
> Ah, ok.  Oops, that means the guest environment has been visibly
> different for KVM and TCG all this time, which isn't great.
> 
>> Not sure about David's "no longer" wording though.
> 
> 
> "no longer" meaning the previous patch version had some
> kvmppc_enable_hcall(), but this version doesn't.

Let me do one more iteration with the comment fixed up at least,
and I'll do a bit of testing with KVM vs TCG behaviour and see
if there are any problems around this.

Thanks,
Nick

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

[Nicholas Piggin]

Greg Kurz's on April 17, 2019 10:01 pm:
> On Wed, 17 Apr 2019 21:20:00 +1000
> Nicholas Piggin <npiggin@gmail.com> wrote:
>> [...]
>> >> @@ -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?  
>> 
> 
> AFAICT they seem to be enabled by default in HV KVM.
> 
>> Oh, it was not intentional, I must not understand how this works. Why
>> is this no longer enabling the those hcalls?
>> 
> 
> Since linux commit 699a0ea0823d ("KVM: PPC: Book3S: Controls for in-kernel 
> sPAPR hypercall handling"), in-kernel hypercalls are disabled by default
> and must be explicitely enabled by userspace. QEMU does that for some
> hypercalls already (search kvmppc_enable_set_mode_hcall() in QEMU for an
> example).

Thanks for the explanation, that's a nice feature.

> Since H_CONFER and H_PROD are listed in default_hcall_list[] in book3s_hv.c,
> no need for QEMU to enable them in KVM.

It looks like we can disable even hcalls that are in the default list
which might help with qemu H_JOIN implementation if we need to send
H_PROD to qemu to do the wake-up.

Thanks,
Nick

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

[Nicholas Piggin]

Greg Kurz's on April 17, 2019 10:01 pm:
> On Wed, 17 Apr 2019 21:20:00 +1000
> Nicholas Piggin <npiggin@gmail.com> wrote:
>> [...]
>> >> @@ -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?  
>> 
> 
> AFAICT they seem to be enabled by default in HV KVM.
> 
>> Oh, it was not intentional, I must not understand how this works. Why
>> is this no longer enabling the those hcalls?
>> 
> 
> Since linux commit 699a0ea0823d ("KVM: PPC: Book3S: Controls for in-kernel 
> sPAPR hypercall handling"), in-kernel hypercalls are disabled by default
> and must be explicitely enabled by userspace. QEMU does that for some
> hypercalls already (search kvmppc_enable_set_mode_hcall() in QEMU for an
> example).

Thanks for the explanation, that's a nice feature.

> Since H_CONFER and H_PROD are listed in default_hcall_list[] in book3s_hv.c,
> no need for QEMU to enable them in KVM.

It looks like we can disable even hcalls that are in the default list
which might help with qemu H_JOIN implementation if we need to send
H_PROD to qemu to do the wake-up.

Thanks,
Nick

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

[David Gibson]

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

On Wed, Apr 17, 2019 at 09:20:00PM +1000, Nicholas Piggin wrote:
> 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 <npiggin@gmail.com>
> >> ---
> >> 
> >> 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?

spapr_register_hypercall() puts a handler in the table that qemu uses
to dispatch hypercalls that cause a KVM exit.  So, it makes qemu able
to handle it.

But for things which we want to handle within KVM, without causing an
exit there's kvmppc_enable_hcall().  That tells KVM to handle the
hcall itself without exiting.  We don't do that by default (except for
some grandfathered cases) so that changing host kernel won't cause the
guest to see different behaviour, which qemu wants to control for
compatibilty and migration.

Ah.. which makes we realize, we probably need to only enable these
hcalls for newer machine types, too.  Although we'd probably get away
with it in this case.

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

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

[David Gibson]

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

On Wed, Apr 17, 2019 at 09:20:00PM +1000, Nicholas Piggin wrote:
> 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 <npiggin@gmail.com>
> >> ---
> >> 
> >> 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?

spapr_register_hypercall() puts a handler in the table that qemu uses
to dispatch hypercalls that cause a KVM exit.  So, it makes qemu able
to handle it.

But for things which we want to handle within KVM, without causing an
exit there's kvmppc_enable_hcall().  That tells KVM to handle the
hcall itself without exiting.  We don't do that by default (except for
some grandfathered cases) so that changing host kernel won't cause the
guest to see different behaviour, which qemu wants to control for
compatibilty and migration.

Ah.. which makes we realize, we probably need to only enable these
hcalls for newer machine types, too.  Although we'd probably get away
with it in this case.

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