[Qemu-devel] [PATCH v5 0/4] spapr: implement dispatch and suspend calls

[Qemu-devel] [PATCH v5 0/4] spapr: implement dispatch and suspend calls

[Nicholas Piggin]

Since the last post I tried to account for feedback, fix style, add
comments, fixed a bug or two in migration etc, more testing, and
dropped the rtas ibm,suspend-me patch for now pending some reworking.

Thanks,
Nick

Nicholas Piggin (4):
  spapr: Implement VPA dispatch counter and prod bit on tcg
  spapr: Implement H_PROD
  spapr: Implement H_CONFER
  spapr: Implement H_JOIN

 hw/ppc/spapr.c                  |  42 +++++++++
 hw/ppc/spapr_cpu_core.c         |   4 +-
 hw/ppc/spapr_hcall.c            | 147 ++++++++++++++++++++++++++++++--
 include/hw/ppc/spapr.h          |   7 ++
 include/hw/ppc/spapr_cpu_core.h |   1 +
 target/ppc/cpu.h                |   2 +
 target/ppc/translate_init.inc.c |  25 ++++++
 7 files changed, 222 insertions(+), 6 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH v5 1/4] spapr: Implement VPA dispatch counter and prod bit on tcg

[Nicholas Piggin]

Implement cpu_exec_enter/exit on ppc which calls into new methods of
the same name in PPCVirtualHypervisorClass. These are used by spapr
to implement these splpar elements, used in subsequent changes.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
Changes since v4:
- Store to VPA on the way out as well.
- Increment the dispatch counter directly in the VPA, which means it will
  migrate with guest memory the same as KVM.
- Prod need not be migrated, add a comment.

 hw/ppc/spapr.c                  | 41 +++++++++++++++++++++++++++++++++
 hw/ppc/spapr_cpu_core.c         |  4 +++-
 hw/ppc/spapr_hcall.c            |  5 ----
 include/hw/ppc/spapr.h          |  7 ++++++
 include/hw/ppc/spapr_cpu_core.h |  1 +
 target/ppc/cpu.h                |  2 ++
 target/ppc/translate_init.inc.c | 25 ++++++++++++++++++++
 7 files changed, 79 insertions(+), 6 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 821f0d4a49..13c423347e 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4302,6 +4302,45 @@ PowerPCCPU *spapr_find_cpu(int vcpu_id)
     return NULL;
 }
 
+static void spapr_cpu_exec_enter(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu)
+{
+    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
+
+    /* These are only called by TCG, KVM maintains dispatch state */
+
+    spapr_cpu->prod = false;
+    if (spapr_cpu->vpa_addr) {
+        CPUState *cs = CPU(cpu);
+        unsigned int dispatch;
+
+        dispatch = ldl_be_phys(cs->as,
+                               spapr_cpu->vpa_addr + VPA_DISPATCH_COUNTER);
+        dispatch++;
+        if ((dispatch & 1) != 0) /* guest set the "wrong" value */
+            dispatch++;
+        stl_be_phys(cs->as,
+                    spapr_cpu->vpa_addr + VPA_DISPATCH_COUNTER, dispatch);
+    }
+}
+
+static void spapr_cpu_exec_exit(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu)
+{
+    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
+
+    if (spapr_cpu->vpa_addr) {
+        CPUState *cs = CPU(cpu);
+        unsigned int dispatch;
+
+        dispatch = ldl_be_phys(cs->as,
+                               spapr_cpu->vpa_addr + VPA_DISPATCH_COUNTER);
+        dispatch++;
+        if ((dispatch & 1) != 1) /* guest set the "wrong" value */
+            dispatch++;
+        stl_be_phys(cs->as,
+                    spapr_cpu->vpa_addr + VPA_DISPATCH_COUNTER, dispatch);
+    }
+}
+
 static void spapr_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -4358,6 +4397,8 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     vhc->hpte_set_r = spapr_hpte_set_r;
     vhc->get_pate = spapr_get_pate;
     vhc->encode_hpt_for_kvm_pr = spapr_encode_hpt_for_kvm_pr;
+    vhc->cpu_exec_enter = spapr_cpu_exec_enter;
+    vhc->cpu_exec_exit = spapr_cpu_exec_exit;
     xic->ics_get = spapr_ics_get;
     xic->ics_resend = spapr_ics_resend;
     xic->icp_get = spapr_icp_get;
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 5621fb9a3d..54abf5308c 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -261,6 +261,7 @@ error:
 static PowerPCCPU *spapr_create_vcpu(SpaprCpuCore *sc, int i, Error **errp)
 {
     SpaprCpuCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(sc);
+    SpaprCpuState *spapr_cpu;
     CPUCore *cc = CPU_CORE(sc);
     Object *obj;
     char *id;
@@ -287,7 +288,8 @@ static PowerPCCPU *spapr_create_vcpu(SpaprCpuCore *sc, int i, Error **errp)
         goto err;
     }
 
-    cpu->machine_data = g_new0(SpaprCpuState, 1);
+    spapr_cpu = g_new0(SpaprCpuState, 1);
+    cpu->machine_data = spapr_cpu;
 
     object_unref(obj);
     return cpu;
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 6808d4cda8..e615881ac4 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -874,11 +874,6 @@ unmap_out:
 #define FLAGS_DEREGISTER_DTL       0x0000c00000000000ULL
 #define FLAGS_DEREGISTER_SLBSHADOW 0x0000e00000000000ULL
 
-#define VPA_MIN_SIZE           640
-#define VPA_SIZE_OFFSET        0x4
-#define VPA_SHARED_PROC_OFFSET 0x9
-#define VPA_SHARED_PROC_VAL    0x2
-
 static target_ulong register_vpa(PowerPCCPU *cpu, target_ulong vpa)
 {
     CPUState *cs = CPU(cpu);
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 60553d32c4..5d36eec9d0 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -525,6 +525,13 @@ void spapr_register_hypercall(target_ulong opcode, spapr_hcall_fn fn);
 target_ulong spapr_hypercall(PowerPCCPU *cpu, target_ulong opcode,
                              target_ulong *args);
 
+/* Virtual Processor Area structure constants */
+#define VPA_MIN_SIZE           640
+#define VPA_SIZE_OFFSET        0x4
+#define VPA_SHARED_PROC_OFFSET 0x9
+#define VPA_SHARED_PROC_VAL    0x2
+#define VPA_DISPATCH_COUNTER   0x100
+
 /* ibm,set-eeh-option */
 #define RTAS_EEH_DISABLE                 0
 #define RTAS_EEH_ENABLE                  1
diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
index f9645a7290..a40cd08ea0 100644
--- a/include/hw/ppc/spapr_cpu_core.h
+++ b/include/hw/ppc/spapr_cpu_core.h
@@ -46,6 +46,7 @@ typedef struct SpaprCpuState {
     uint64_t vpa_addr;
     uint64_t slb_shadow_addr, slb_shadow_size;
     uint64_t dtl_addr, dtl_size;
+    bool prod; /* not migrated, only used to improve dispatch latencies */
     struct ICPState *icp;
     struct XiveTCTX *tctx;
 } SpaprCpuState;
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index c9beba2a5c..78d6504acb 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1224,6 +1224,8 @@ struct PPCVirtualHypervisorClass {
     void (*hpte_set_r)(PPCVirtualHypervisor *vhyp, hwaddr ptex, uint64_t pte1);
     void (*get_pate)(PPCVirtualHypervisor *vhyp, ppc_v3_pate_t *entry);
     target_ulong (*encode_hpt_for_kvm_pr)(PPCVirtualHypervisor *vhyp);
+    void (*cpu_exec_enter)(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu);
+    void (*cpu_exec_exit)(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu);
 };
 
 #define TYPE_PPC_VIRTUAL_HYPERVISOR "ppc-virtual-hypervisor"
diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index 86fc8f2e31..58d4a93b23 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -10473,6 +10473,28 @@ static bool ppc_cpu_is_big_endian(CPUState *cs)
 }
 #endif
 
+static void ppc_cpu_exec_enter(CPUState *cs)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+
+    if (cpu->vhyp) {
+        PPCVirtualHypervisorClass *vhc =
+            PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
+        vhc->cpu_exec_enter(cpu->vhyp, cpu);
+    }
+}
+
+static void ppc_cpu_exec_exit(CPUState *cs)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+
+    if (cpu->vhyp) {
+        PPCVirtualHypervisorClass *vhc =
+            PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
+        vhc->cpu_exec_exit(cpu->vhyp, cpu);
+    }
+}
+
 static void ppc_cpu_instance_init(Object *obj)
 {
     PowerPCCPU *cpu = POWERPC_CPU(obj);
@@ -10624,6 +10646,9 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
     cc->tcg_initialize = ppc_translate_init;
     cc->tlb_fill = ppc_cpu_tlb_fill;
 #endif
+    cc->cpu_exec_enter = ppc_cpu_exec_enter;
+    cc->cpu_exec_exit = ppc_cpu_exec_exit;
+
     cc->disas_set_info = ppc_disas_set_info;
 
     dc->fw_name = "PowerPC,UNKNOWN";
-- 
2.20.1

[Qemu-devel] [PATCH v5 2/4] spapr: Implement H_PROD

[Nicholas Piggin]

H_PROD is added, and H_CEDE is modified to test the prod bit
according to PAPR.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/spapr_hcall.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index e615881ac4..8b208ab259 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1050,14 +1050,41 @@ static target_ulong h_cede(PowerPCCPU *cpu, SpaprMachineState *spapr,
 {
     CPUPPCState *env = &cpu->env;
     CPUState *cs = CPU(cpu);
+    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
 
     env->msr |= (1ULL << MSR_EE);
     hreg_compute_hflags(env);
+
+    if (spapr_cpu->prod) {
+        spapr_cpu->prod = false;
+        return H_SUCCESS;
+    }
+
     if (!cpu_has_work(cs)) {
         cs->halted = 1;
         cs->exception_index = EXCP_HLT;
         cs->exit_request = 1;
     }
+
+    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;
+    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
+
+    cs = CPU(spapr_find_cpu(target));
+    if (!cs) {
+        return H_PARAMETER;
+    }
+
+    spapr_cpu->prod = true;
+    cs->halted = 0;
+    qemu_cpu_kick(cs);
+
     return H_SUCCESS;
 }
 
@@ -1882,6 +1909,8 @@ 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_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 v5 3/4] spapr: Implement H_CONFER

[Nicholas Piggin]

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>
---
Changes since v4:
- Style, added justification comments, spelling.
- Fixed trying to dereference spapr_cpu for a -1 target.

 hw/ppc/spapr_hcall.c | 68 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 8b208ab259..5e655172b2 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1069,6 +1069,73 @@ 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];
+    CPUState *cs = CPU(cpu);
+    SpaprCpuState *spapr_cpu;
+
+    /*
+     * -1 means confer to all other CPUs without dispatch counter check,
+     *  otherwise it's a targeted confer.
+     */
+    if (target != -1) {
+        PowerPCCPU *target_cpu = spapr_find_cpu(target);
+        CPUState *target_cs = CPU(target_cpu);
+        unsigned int target_dispatch;
+
+        if (!target_cs) {
+            return H_PARAMETER;
+        }
+
+        spapr_cpu = spapr_cpu_state(target_cpu);
+
+        /*
+         * target == self is a special case, we wait until prodded, without
+         * dispatch 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;
+
+            return H_SUCCESS;
+        }
+
+        if (!spapr_cpu->vpa_addr || ((dispatch & 1) == 0)) {
+            return H_SUCCESS;
+        }
+
+        target_dispatch = ldl_be_phys(cs->as,
+                                  spapr_cpu->vpa_addr + VPA_DISPATCH_COUNTER);
+        if (target_dispatch != dispatch) {
+            return H_SUCCESS;
+        }
+
+        /*
+         * The targeted confer does not do anything special beyond yielding
+         * the current vCPU, but even this should be better than nothing.
+         * At least for single-threaded tcg, it gives the target a chance to
+         * run before we run again. Multi-threaded tcg does not really do
+         * anything with EXCP_YIELD yet.
+         */
+    }
+
+    cs->exception_index = EXCP_YIELD;
+    cs->exit_request = 1;
+    cpu_loop_exit(cs);
+
+    return H_SUCCESS;
+}
+
 static target_ulong h_prod(PowerPCCPU *cpu, SpaprMachineState *spapr,
                            target_ulong opcode, target_ulong *args)
 {
@@ -1909,6 +1976,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);
-- 
2.20.1

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

[Nicholas Piggin]

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 <npiggin@gmail.com>
---
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 = &cpu->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;
+    }
+
+    CPU_FOREACH(cs) {
+        PowerPCCPU *c = POWERPC_CPU(cs);
+        CPUPPCState *e = &c->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;
+}
+
 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 */
-- 
2.20.1

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

[Cédric Le Goater]

On 17/07/2019 07:39, Nicholas Piggin wrote:
> H_PROD is added, and H_CEDE is modified to test the prod bit
> according to PAPR.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  hw/ppc/spapr_hcall.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index e615881ac4..8b208ab259 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1050,14 +1050,41 @@ static target_ulong h_cede(PowerPCCPU *cpu, SpaprMachineState *spapr,
>  {
>      CPUPPCState *env = &cpu->env;
>      CPUState *cs = CPU(cpu);
> +    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>  
>      env->msr |= (1ULL << MSR_EE);
>      hreg_compute_hflags(env);
> +
> +    if (spapr_cpu->prod) {
> +        spapr_cpu->prod = false;
> +        return H_SUCCESS;
> +    }
> +
>      if (!cpu_has_work(cs)) {
>          cs->halted = 1;
>          cs->exception_index = EXCP_HLT;
>          cs->exit_request = 1;
>      }
> +
> +    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;
> +    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);

shouldn't we grab the SpaprCpuState of 'target' instead ?  

> +    cs = CPU(spapr_find_cpu(target));
> +    if (!cs) {
> +        return H_PARAMETER;
> +    }
> +
> +    spapr_cpu->prod = true;
> +    cs->halted = 0;
> +    qemu_cpu_kick(cs);
> +
>      return H_SUCCESS;
>  }
>  
> @@ -1882,6 +1909,8 @@ 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_PROD, h_prod);
> +
>      spapr_register_hypercall(H_SIGNAL_SYS_RESET, h_signal_sys_reset);
>  
>      /* processor register resource access h-calls */
> 

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

[Nicholas Piggin]

Cédric Le Goater's on July 17, 2019 8:16 pm:
> On 17/07/2019 07:39, Nicholas Piggin wrote:
>> H_PROD is added, and H_CEDE is modified to test the prod bit
>> according to PAPR.
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>  hw/ppc/spapr_hcall.c | 29 +++++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>> 
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index e615881ac4..8b208ab259 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -1050,14 +1050,41 @@ static target_ulong h_cede(PowerPCCPU *cpu, SpaprMachineState *spapr,
>>  {
>>      CPUPPCState *env = &cpu->env;
>>      CPUState *cs = CPU(cpu);
>> +    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>>  
>>      env->msr |= (1ULL << MSR_EE);
>>      hreg_compute_hflags(env);
>> +
>> +    if (spapr_cpu->prod) {
>> +        spapr_cpu->prod = false;
>> +        return H_SUCCESS;
>> +    }
>> +
>>      if (!cpu_has_work(cs)) {
>>          cs->halted = 1;
>>          cs->exception_index = EXCP_HLT;
>>          cs->exit_request = 1;
>>      }
>> +
>> +    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;
>> +    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
> 
> shouldn't we grab the SpaprCpuState of 'target' instead ?  

Yes we should, good catch.

Thanks,
Nick

Re: [Qemu-devel] [PATCH v5 1/4] spapr: Implement VPA dispatch counter and prod bit on tcg

[Cédric Le Goater]

On 17/07/2019 07:39, Nicholas Piggin wrote:
> Implement cpu_exec_enter/exit on ppc which calls into new methods of
> the same name in PPCVirtualHypervisorClass. These are used by spapr
> to implement these splpar elements, used in subsequent changes.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

This is nice. I am thinking of using these handlers to push/pull 
the XIVE OS CAM line and escalate to QEMU when a vCPU being notified 
is not dispatched.

Some minor comments below.

Reviewed-by: Cédric Le Goater <clg@kaod.org>


Thanks,

C.

> ---
> Changes since v4:
> - Store to VPA on the way out as well.
> - Increment the dispatch counter directly in the VPA, which means it will
>   migrate with guest memory the same as KVM.
> - Prod need not be migrated, add a comment.
> 
>  hw/ppc/spapr.c                  | 41 +++++++++++++++++++++++++++++++++
>  hw/ppc/spapr_cpu_core.c         |  4 +++-
>  hw/ppc/spapr_hcall.c            |  5 ----
>  include/hw/ppc/spapr.h          |  7 ++++++
>  include/hw/ppc/spapr_cpu_core.h |  1 +
>  target/ppc/cpu.h                |  2 ++
>  target/ppc/translate_init.inc.c | 25 ++++++++++++++++++++
>  7 files changed, 79 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 821f0d4a49..13c423347e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4302,6 +4302,45 @@ PowerPCCPU *spapr_find_cpu(int vcpu_id)
>      return NULL;
>  }
>  
> +static void spapr_cpu_exec_enter(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu)
> +{
> +    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
> +
> +    /* These are only called by TCG, KVM maintains dispatch state */
> +
> +    spapr_cpu->prod = false;

I would have kept this prod bool for the next patch as we don't use it here.

> +    if (spapr_cpu->vpa_addr) {
> +        CPUState *cs = CPU(cpu);
> +        unsigned int dispatch;
> +
> +        dispatch = ldl_be_phys(cs->as,
> +                               spapr_cpu->vpa_addr + VPA_DISPATCH_COUNTER);
> +        dispatch++;
> +        if ((dispatch & 1) != 0) /* guest set the "wrong" value */
> +            dispatch++;


You might want to add :

  qemu_log_mask(LOG_GUEST_ERROR, ...);  

when the yield value is wrong.

> +        stl_be_phys(cs->as,
> +                    spapr_cpu->vpa_addr + VPA_DISPATCH_COUNTER, dispatch);
> +    }
> +}
> +
> +static void spapr_cpu_exec_exit(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu)
> +{
> +    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
> +
> +    if (spapr_cpu->vpa_addr) {
> +        CPUState *cs = CPU(cpu);
> +        unsigned int dispatch;
> +
> +        dispatch = ldl_be_phys(cs->as,
> +                               spapr_cpu->vpa_addr + VPA_DISPATCH_COUNTER);
> +        dispatch++;
> +        if ((dispatch & 1) != 1) /* guest set the "wrong" value */
> +            dispatch++;
> +        stl_be_phys(cs->as,
> +                    spapr_cpu->vpa_addr + VPA_DISPATCH_COUNTER, dispatch);
> +    }
> +}
> +
>  static void spapr_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> @@ -4358,6 +4397,8 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      vhc->hpte_set_r = spapr_hpte_set_r;
>      vhc->get_pate = spapr_get_pate;
>      vhc->encode_hpt_for_kvm_pr = spapr_encode_hpt_for_kvm_pr;
> +    vhc->cpu_exec_enter = spapr_cpu_exec_enter;
> +    vhc->cpu_exec_exit = spapr_cpu_exec_exit;
>      xic->ics_get = spapr_ics_get;
>      xic->ics_resend = spapr_ics_resend;
>      xic->icp_get = spapr_icp_get;
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 5621fb9a3d..54abf5308c 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -261,6 +261,7 @@ error:
>  static PowerPCCPU *spapr_create_vcpu(SpaprCpuCore *sc, int i, Error **errp)
>  {
>      SpaprCpuCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(sc);
> +    SpaprCpuState *spapr_cpu;
>      CPUCore *cc = CPU_CORE(sc);
>      Object *obj;
>      char *id;
> @@ -287,7 +288,8 @@ static PowerPCCPU *spapr_create_vcpu(SpaprCpuCore *sc, int i, Error **errp)
>          goto err;
>      }
>  
> -    cpu->machine_data = g_new0(SpaprCpuState, 1);
> +    spapr_cpu = g_new0(SpaprCpuState, 1);
> +    cpu->machine_data = spapr_cpu;
>  
>      object_unref(obj);
>      return cpu;
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 6808d4cda8..e615881ac4 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -874,11 +874,6 @@ unmap_out:
>  #define FLAGS_DEREGISTER_DTL       0x0000c00000000000ULL
>  #define FLAGS_DEREGISTER_SLBSHADOW 0x0000e00000000000ULL
>  
> -#define VPA_MIN_SIZE           640
> -#define VPA_SIZE_OFFSET        0x4
> -#define VPA_SHARED_PROC_OFFSET 0x9
> -#define VPA_SHARED_PROC_VAL    0x2
> -
>  static target_ulong register_vpa(PowerPCCPU *cpu, target_ulong vpa)
>  {
>      CPUState *cs = CPU(cpu);
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 60553d32c4..5d36eec9d0 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -525,6 +525,13 @@ void spapr_register_hypercall(target_ulong opcode, spapr_hcall_fn fn);
>  target_ulong spapr_hypercall(PowerPCCPU *cpu, target_ulong opcode,
>                               target_ulong *args);
>  
> +/* Virtual Processor Area structure constants */
> +#define VPA_MIN_SIZE           640
> +#define VPA_SIZE_OFFSET        0x4
> +#define VPA_SHARED_PROC_OFFSET 0x9
> +#define VPA_SHARED_PROC_VAL    0x2
> +#define VPA_DISPATCH_COUNTER   0x100
> +
>  /* ibm,set-eeh-option */
>  #define RTAS_EEH_DISABLE                 0
>  #define RTAS_EEH_ENABLE                  1
> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> index f9645a7290..a40cd08ea0 100644
> --- a/include/hw/ppc/spapr_cpu_core.h
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -46,6 +46,7 @@ typedef struct SpaprCpuState {
>      uint64_t vpa_addr;
>      uint64_t slb_shadow_addr, slb_shadow_size;
>      uint64_t dtl_addr, dtl_size;
> +    bool prod; /* not migrated, only used to improve dispatch latencies */
>      struct ICPState *icp;
>      struct XiveTCTX *tctx;
>  } SpaprCpuState;
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index c9beba2a5c..78d6504acb 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1224,6 +1224,8 @@ struct PPCVirtualHypervisorClass {
>      void (*hpte_set_r)(PPCVirtualHypervisor *vhyp, hwaddr ptex, uint64_t pte1);
>      void (*get_pate)(PPCVirtualHypervisor *vhyp, ppc_v3_pate_t *entry);
>      target_ulong (*encode_hpt_for_kvm_pr)(PPCVirtualHypervisor *vhyp);
> +    void (*cpu_exec_enter)(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu);
> +    void (*cpu_exec_exit)(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu);
>  };
>  
>  #define TYPE_PPC_VIRTUAL_HYPERVISOR "ppc-virtual-hypervisor"
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index 86fc8f2e31..58d4a93b23 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -10473,6 +10473,28 @@ static bool ppc_cpu_is_big_endian(CPUState *cs)
>  }
>  #endif
>  
> +static void ppc_cpu_exec_enter(CPUState *cs)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +
> +    if (cpu->vhyp) {
> +        PPCVirtualHypervisorClass *vhc =
> +            PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
> +        vhc->cpu_exec_enter(cpu->vhyp, cpu);
> +    }
> +}
> +
> +static void ppc_cpu_exec_exit(CPUState *cs)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +
> +    if (cpu->vhyp) {
> +        PPCVirtualHypervisorClass *vhc =
> +            PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
> +        vhc->cpu_exec_exit(cpu->vhyp, cpu);
> +    }
> +}
> +
>  static void ppc_cpu_instance_init(Object *obj)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(obj);
> @@ -10624,6 +10646,9 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>      cc->tcg_initialize = ppc_translate_init;
>      cc->tlb_fill = ppc_cpu_tlb_fill;
>  #endif
> +    cc->cpu_exec_enter = ppc_cpu_exec_enter;
> +    cc->cpu_exec_exit = ppc_cpu_exec_exit;
> +
>      cc->disas_set_info = ppc_disas_set_info;
>  
>      dc->fw_name = "PowerPC,UNKNOWN";
> 

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

[Cédric Le Goater]

On 17/07/2019 07:39, Nicholas Piggin wrote:
> H_PROD is added, and H_CEDE is modified to test the prod bit
> according to PAPR.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  hw/ppc/spapr_hcall.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index e615881ac4..8b208ab259 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1050,14 +1050,41 @@ static target_ulong h_cede(PowerPCCPU *cpu, SpaprMachineState *spapr,
>  {
>      CPUPPCState *env = &cpu->env;
>      CPUState *cs = CPU(cpu);
> +    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>  
>      env->msr |= (1ULL << MSR_EE);
>      hreg_compute_hflags(env);
> +
> +    if (spapr_cpu->prod) {
> +        spapr_cpu->prod = false;
> +        return H_SUCCESS;
> +    }
> +
>      if (!cpu_has_work(cs)) {
>          cs->halted = 1;
>          cs->exception_index = EXCP_HLT;

Shouldn't that be EXCP_HALTED instead ? 

commit 1dd088946cf4 seems to say that h_cede is equivalent to the hlt 
instruction on x86 but in 7 years things have changed.

C. 

>          cs->exit_request = 1;
>      }
> +
> +    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;
> +    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
> +
> +    cs = CPU(spapr_find_cpu(target));
> +    if (!cs) {
> +        return H_PARAMETER;
> +    }
> +
> +    spapr_cpu->prod = true;
> +    cs->halted = 0;
> +    qemu_cpu_kick(cs);
> +
>      return H_SUCCESS;
>  }
>  
> @@ -1882,6 +1909,8 @@ 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_PROD, h_prod);
> +
>      spapr_register_hypercall(H_SIGNAL_SYS_RESET, h_signal_sys_reset);
>  
>      /* processor register resource access h-calls */
> 

Re: [Qemu-devel] [PATCH v5 1/4] spapr: Implement VPA dispatch counter and prod bit on tcg

[Greg Kurz]

On Wed, 17 Jul 2019 15:39:49 +1000
Nicholas Piggin <npiggin@gmail.com> wrote:

> Implement cpu_exec_enter/exit on ppc which calls into new methods of
> the same name in PPCVirtualHypervisorClass. These are used by spapr
> to implement these splpar elements, used in subsequent changes.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> Changes since v4:
> - Store to VPA on the way out as well.
> - Increment the dispatch counter directly in the VPA, which means it will
>   migrate with guest memory the same as KVM.
> - Prod need not be migrated, add a comment.
> 
>  hw/ppc/spapr.c                  | 41 +++++++++++++++++++++++++++++++++
>  hw/ppc/spapr_cpu_core.c         |  4 +++-
>  hw/ppc/spapr_hcall.c            |  5 ----
>  include/hw/ppc/spapr.h          |  7 ++++++
>  include/hw/ppc/spapr_cpu_core.h |  1 +
>  target/ppc/cpu.h                |  2 ++
>  target/ppc/translate_init.inc.c | 25 ++++++++++++++++++++
>  7 files changed, 79 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 821f0d4a49..13c423347e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4302,6 +4302,45 @@ PowerPCCPU *spapr_find_cpu(int vcpu_id)
>      return NULL;
>  }
>  
> +static void spapr_cpu_exec_enter(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu)
> +{
> +    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
> +
> +    /* These are only called by TCG, KVM maintains dispatch state */
> +
> +    spapr_cpu->prod = false;
> +    if (spapr_cpu->vpa_addr) {
> +        CPUState *cs = CPU(cpu);
> +        unsigned int dispatch;
> +
> +        dispatch = ldl_be_phys(cs->as,
> +                               spapr_cpu->vpa_addr + VPA_DISPATCH_COUNTER);
> +        dispatch++;
> +        if ((dispatch & 1) != 0) /* guest set the "wrong" value */
> +            dispatch++;
> +        stl_be_phys(cs->as,
> +                    spapr_cpu->vpa_addr + VPA_DISPATCH_COUNTER, dispatch);
> +    }
> +}
> +
> +static void spapr_cpu_exec_exit(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu)
> +{
> +    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
> +
> +    if (spapr_cpu->vpa_addr) {
> +        CPUState *cs = CPU(cpu);
> +        unsigned int dispatch;
> +
> +        dispatch = ldl_be_phys(cs->as,
> +                               spapr_cpu->vpa_addr + VPA_DISPATCH_COUNTER);
> +        dispatch++;
> +        if ((dispatch & 1) != 1) /* guest set the "wrong" value */
> +            dispatch++;
> +        stl_be_phys(cs->as,
> +                    spapr_cpu->vpa_addr + VPA_DISPATCH_COUNTER, dispatch);
> +    }
> +}
> +
>  static void spapr_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> @@ -4358,6 +4397,8 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      vhc->hpte_set_r = spapr_hpte_set_r;
>      vhc->get_pate = spapr_get_pate;
>      vhc->encode_hpt_for_kvm_pr = spapr_encode_hpt_for_kvm_pr;
> +    vhc->cpu_exec_enter = spapr_cpu_exec_enter;
> +    vhc->cpu_exec_exit = spapr_cpu_exec_exit;
>      xic->ics_get = spapr_ics_get;
>      xic->ics_resend = spapr_ics_resend;
>      xic->icp_get = spapr_icp_get;
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 5621fb9a3d..54abf5308c 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -261,6 +261,7 @@ error:
>  static PowerPCCPU *spapr_create_vcpu(SpaprCpuCore *sc, int i, Error **errp)
>  {
>      SpaprCpuCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(sc);
> +    SpaprCpuState *spapr_cpu;
>      CPUCore *cc = CPU_CORE(sc);
>      Object *obj;
>      char *id;
> @@ -287,7 +288,8 @@ static PowerPCCPU *spapr_create_vcpu(SpaprCpuCore *sc, int i, Error **errp)
>          goto err;
>      }
>  
> -    cpu->machine_data = g_new0(SpaprCpuState, 1);
> +    spapr_cpu = g_new0(SpaprCpuState, 1);
> +    cpu->machine_data = spapr_cpu;

What's the purpose of this change since there's no other
user of spapr_cpu in this function ?

>  
>      object_unref(obj);
>      return cpu;
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 6808d4cda8..e615881ac4 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -874,11 +874,6 @@ unmap_out:
>  #define FLAGS_DEREGISTER_DTL       0x0000c00000000000ULL
>  #define FLAGS_DEREGISTER_SLBSHADOW 0x0000e00000000000ULL
>  
> -#define VPA_MIN_SIZE           640
> -#define VPA_SIZE_OFFSET        0x4
> -#define VPA_SHARED_PROC_OFFSET 0x9
> -#define VPA_SHARED_PROC_VAL    0x2
> -
>  static target_ulong register_vpa(PowerPCCPU *cpu, target_ulong vpa)
>  {
>      CPUState *cs = CPU(cpu);
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 60553d32c4..5d36eec9d0 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -525,6 +525,13 @@ void spapr_register_hypercall(target_ulong opcode, spapr_hcall_fn fn);
>  target_ulong spapr_hypercall(PowerPCCPU *cpu, target_ulong opcode,
>                               target_ulong *args);
>  
> +/* Virtual Processor Area structure constants */
> +#define VPA_MIN_SIZE           640
> +#define VPA_SIZE_OFFSET        0x4
> +#define VPA_SHARED_PROC_OFFSET 0x9
> +#define VPA_SHARED_PROC_VAL    0x2
> +#define VPA_DISPATCH_COUNTER   0x100
> +
>  /* ibm,set-eeh-option */
>  #define RTAS_EEH_DISABLE                 0
>  #define RTAS_EEH_ENABLE                  1
> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> index f9645a7290..a40cd08ea0 100644
> --- a/include/hw/ppc/spapr_cpu_core.h
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -46,6 +46,7 @@ typedef struct SpaprCpuState {
>      uint64_t vpa_addr;
>      uint64_t slb_shadow_addr, slb_shadow_size;
>      uint64_t dtl_addr, dtl_size;
> +    bool prod; /* not migrated, only used to improve dispatch latencies */
>      struct ICPState *icp;
>      struct XiveTCTX *tctx;
>  } SpaprCpuState;
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index c9beba2a5c..78d6504acb 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1224,6 +1224,8 @@ struct PPCVirtualHypervisorClass {
>      void (*hpte_set_r)(PPCVirtualHypervisor *vhyp, hwaddr ptex, uint64_t pte1);
>      void (*get_pate)(PPCVirtualHypervisor *vhyp, ppc_v3_pate_t *entry);
>      target_ulong (*encode_hpt_for_kvm_pr)(PPCVirtualHypervisor *vhyp);
> +    void (*cpu_exec_enter)(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu);
> +    void (*cpu_exec_exit)(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu);
>  };
>  
>  #define TYPE_PPC_VIRTUAL_HYPERVISOR "ppc-virtual-hypervisor"
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index 86fc8f2e31..58d4a93b23 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -10473,6 +10473,28 @@ static bool ppc_cpu_is_big_endian(CPUState *cs)
>  }
>  #endif
>  
> +static void ppc_cpu_exec_enter(CPUState *cs)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +
> +    if (cpu->vhyp) {
> +        PPCVirtualHypervisorClass *vhc =
> +            PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
> +        vhc->cpu_exec_enter(cpu->vhyp, cpu);
> +    }
> +}
> +
> +static void ppc_cpu_exec_exit(CPUState *cs)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +
> +    if (cpu->vhyp) {
> +        PPCVirtualHypervisorClass *vhc =
> +            PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
> +        vhc->cpu_exec_exit(cpu->vhyp, cpu);
> +    }
> +}
> +
>  static void ppc_cpu_instance_init(Object *obj)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(obj);
> @@ -10624,6 +10646,9 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>      cc->tcg_initialize = ppc_translate_init;
>      cc->tlb_fill = ppc_cpu_tlb_fill;
>  #endif
> +    cc->cpu_exec_enter = ppc_cpu_exec_enter;
> +    cc->cpu_exec_exit = ppc_cpu_exec_exit;
> +

This code only makes sense with system emulation. I guess it
should be guarded with !defined(CONFIG_USER_ONLY).

>      cc->disas_set_info = ppc_disas_set_info;
>  
>      dc->fw_name = "PowerPC,UNKNOWN";

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

[Greg Kurz]

On Wed, 17 Jul 2019 15:39:51 +1000
Nicholas Piggin <npiggin@gmail.com> 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>
> ---

LGTM.

Reviewed-by: Greg Kurz <groug@kaod.org>

Just two minor comments, see below.

> Changes since v4:
> - Style, added justification comments, spelling.
> - Fixed trying to dereference spapr_cpu for a -1 target.
> 
>  hw/ppc/spapr_hcall.c | 68 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 8b208ab259..5e655172b2 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1069,6 +1069,73 @@ 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];
> +    CPUState *cs = CPU(cpu);
> +    SpaprCpuState *spapr_cpu;
> +
> +    /*
> +     * -1 means confer to all other CPUs without dispatch counter check,
> +     *  otherwise it's a targeted confer.
> +     */
> +    if (target != -1) {
> +        PowerPCCPU *target_cpu = spapr_find_cpu(target);
> +        CPUState *target_cs = CPU(target_cpu);
> +        unsigned int target_dispatch;

Maybe make it uint32_t to be consistent with dispatch above, and this
is the actual return type of ldl_be_phys() ?

> +
> +        if (!target_cs) {

This is the only user of target_cs, maybe drop it and use target_cpu
instead ?

> +            return H_PARAMETER;
> +        }
> +
> +        spapr_cpu = spapr_cpu_state(target_cpu);
> +
> +        /*
> +         * target == self is a special case, we wait until prodded, without
> +         * dispatch 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;
> +
> +            return H_SUCCESS;
> +        }
> +
> +        if (!spapr_cpu->vpa_addr || ((dispatch & 1) == 0)) {
> +            return H_SUCCESS;
> +        }
> +
> +        target_dispatch = ldl_be_phys(cs->as,
> +                                  spapr_cpu->vpa_addr + VPA_DISPATCH_COUNTER);
> +        if (target_dispatch != dispatch) {
> +            return H_SUCCESS;
> +        }
> +
> +        /*
> +         * The targeted confer does not do anything special beyond yielding
> +         * the current vCPU, but even this should be better than nothing.
> +         * At least for single-threaded tcg, it gives the target a chance to
> +         * run before we run again. Multi-threaded tcg does not really do
> +         * anything with EXCP_YIELD yet.
> +         */
> +    }
> +
> +    cs->exception_index = EXCP_YIELD;
> +    cs->exit_request = 1;
> +    cpu_loop_exit(cs);
> +
> +    return H_SUCCESS;
> +}
> +
>  static target_ulong h_prod(PowerPCCPU *cpu, SpaprMachineState *spapr,
>                             target_ulong opcode, target_ulong *args)
>  {
> @@ -1909,6 +1976,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);

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

[Greg Kurz]

On Wed, 17 Jul 2019 15:39:52 +1000
Nicholas Piggin <npiggin@gmail.com> 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 <npiggin@gmail.com>
> ---
> 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 = &cpu->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 = &c->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 */

Re: [Qemu-devel] [PATCH v5 1/4] spapr: Implement VPA dispatch counter and prod bit on tcg

[Nicholas Piggin]

Cédric Le Goater's on July 17, 2019 10:50 pm:
> On 17/07/2019 07:39, Nicholas Piggin wrote:
>> Implement cpu_exec_enter/exit on ppc which calls into new methods of
>> the same name in PPCVirtualHypervisorClass. These are used by spapr
>> to implement these splpar elements, used in subsequent changes.
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> 
> This is nice. I am thinking of using these handlers to push/pull 
> the XIVE OS CAM line and escalate to QEMU when a vCPU being notified 
> is not dispatched.

Great if it is useful.

>> +static void spapr_cpu_exec_enter(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu)
>> +{
>> +    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>> +
>> +    /* These are only called by TCG, KVM maintains dispatch state */
>> +
>> +    spapr_cpu->prod = false;
> 
> I would have kept this prod bool for the next patch as we don't use it here.

Okay I'll do that.

>> +    if (spapr_cpu->vpa_addr) {
>> +        CPUState *cs = CPU(cpu);
>> +        unsigned int dispatch;
>> +
>> +        dispatch = ldl_be_phys(cs->as,
>> +                               spapr_cpu->vpa_addr + VPA_DISPATCH_COUNTER);
>> +        dispatch++;
>> +        if ((dispatch & 1) != 0) /* guest set the "wrong" value */
>> +            dispatch++;
> 
> 
> You might want to add :
> 
>   qemu_log_mask(LOG_GUEST_ERROR, ...);  
> 
> when the yield value is wrong.

Hm didn't know about that, good point I can add it.

Thanks,
Nick

Re: [Qemu-devel] [PATCH v5 1/4] spapr: Implement VPA dispatch counter and prod bit on tcg

[Nicholas Piggin]

Greg Kurz's on July 18, 2019 1:29 am:
> On Wed, 17 Jul 2019 15:39:49 +1000
> Nicholas Piggin <npiggin@gmail.com> wrote:
> 
>> -    cpu->machine_data = g_new0(SpaprCpuState, 1);
>> +    spapr_cpu = g_new0(SpaprCpuState, 1);
>> +    cpu->machine_data = spapr_cpu;
> 
> What's the purpose of this change since there's no other
> user of spapr_cpu in this function ?

It is an orphan from when the previous patch kept a dispatch_counter
in the state data structure. Now we just use the VPA directly I can
remove this completely, good catch.

>> @@ -10624,6 +10646,9 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>>      cc->tcg_initialize = ppc_translate_init;
>>      cc->tlb_fill = ppc_cpu_tlb_fill;
>>  #endif
>> +    cc->cpu_exec_enter = ppc_cpu_exec_enter;
>> +    cc->cpu_exec_exit = ppc_cpu_exec_exit;
>> +
> 
> This code only makes sense with system emulation. I guess it
> should be guarded with !defined(CONFIG_USER_ONLY).

I can do that.

Thanks,
Nick

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

[Nicholas Piggin]

Greg Kurz's on July 18, 2019 3:00 am:
> On Wed, 17 Jul 2019 15:39:51 +1000
> Nicholas Piggin <npiggin@gmail.com> 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>
>> ---
> 
> LGTM.
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> Just two minor comments, see below.
> 
>> Changes since v4:
>> - Style, added justification comments, spelling.
>> - Fixed trying to dereference spapr_cpu for a -1 target.
>> 
>>  hw/ppc/spapr_hcall.c | 68 ++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 68 insertions(+)
>> 
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index 8b208ab259..5e655172b2 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -1069,6 +1069,73 @@ 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];
>> +    CPUState *cs = CPU(cpu);
>> +    SpaprCpuState *spapr_cpu;
>> +
>> +    /*
>> +     * -1 means confer to all other CPUs without dispatch counter check,
>> +     *  otherwise it's a targeted confer.
>> +     */
>> +    if (target != -1) {
>> +        PowerPCCPU *target_cpu = spapr_find_cpu(target);
>> +        CPUState *target_cs = CPU(target_cpu);
>> +        unsigned int target_dispatch;
> 
> Maybe make it uint32_t to be consistent with dispatch above, and this
> is the actual return type of ldl_be_phys() ?

Sure okay.

>> +
>> +        if (!target_cs) {
> 
> This is the only user of target_cs, maybe drop it and use target_cpu
> instead ?

That probably works, I'll try.

Thanks,
Nick

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

[Nicholas Piggin]

Cédric Le Goater's on July 17, 2019 11:33 pm:
> On 17/07/2019 07:39, Nicholas Piggin wrote:
>> H_PROD is added, and H_CEDE is modified to test the prod bit
>> according to PAPR.
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>  hw/ppc/spapr_hcall.c | 29 +++++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>> 
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index e615881ac4..8b208ab259 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -1050,14 +1050,41 @@ static target_ulong h_cede(PowerPCCPU *cpu, SpaprMachineState *spapr,
>>  {
>>      CPUPPCState *env = &cpu->env;
>>      CPUState *cs = CPU(cpu);
>> +    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>>  
>>      env->msr |= (1ULL << MSR_EE);
>>      hreg_compute_hflags(env);
>> +
>> +    if (spapr_cpu->prod) {
>> +        spapr_cpu->prod = false;
>> +        return H_SUCCESS;
>> +    }
>> +
>>      if (!cpu_has_work(cs)) {
>>          cs->halted = 1;
>>          cs->exception_index = EXCP_HLT;
> 
> Shouldn't that be EXCP_HALTED instead ? 

Possibly, I'm not sure. I don't know if it even makes a difference in
ppc code?

Thanks,
Nick

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

[Nicholas Piggin]

Greg Kurz's on July 18, 2019 3:30 am:
> On Wed, 17 Jul 2019 15:39:52 +1000
> Nicholas Piggin <npiggin@gmail.com> 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 <npiggin@gmail.com>
>> ---
>> 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 = &cpu->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 = &c->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