[Qemu-devel] [PATCH v4 0/5] spapr: implement dispatch and suspend calls

[Qemu-devel] [PATCH v4 0/5] spapr: implement dispatch and suspend calls

[Nicholas Piggin]

This series follows on from the previous that added H_PROD and
H_CONFER, but I've now aimed to make it conform better to PAPR.

It's still not completely there (as explained in comments), but
it's better than before and actually better matches KVM that
does implement the prod bit and dispatch counter.

The first 3 patches implement these splpar hcalls for tcg, as
KVM implements its own H_PROD, H_CONFER, H_CEDE, and dispatch
management. These would be nice to merge as they make qemu
behave more like KVM and PowerVM with these calls.

The last 2 patches implement some parts of the guest suspend
APIs I've been using to test Linux modifications to the pseries
(and generic kernel) suspend/hibernate code, but they are not
really useful to a Linux guest (yet) due to other missing bits.

I have some Linux code I'll try to gradually upstream to work
around the missing bits and make this suspend on QEMU "work",
which is at least useful for testing without having PowerVM.

Thanks,
Nick

Nicholas Piggin (5):
  spapr: Implement dispatch counter and prod bit on tcg
  spapr: Implement H_PROD
  spapr: Implement H_CONFER
  spapr: Implement H_JOIN
  spapr: Implement ibm,suspend-me

 hw/ppc/spapr.c                  |  52 +++++++++++++
 hw/ppc/spapr_cpu_core.c         |   5 +-
 hw/ppc/spapr_hcall.c            | 126 ++++++++++++++++++++++++++++++--
 hw/ppc/spapr_rtas.c             |  32 ++++++++
 include/hw/ppc/spapr.h          |  14 +++-
 include/hw/ppc/spapr_cpu_core.h |   2 +
 target/ppc/cpu.h                |   2 +
 target/ppc/translate_init.inc.c |  25 +++++++
 8 files changed, 251 insertions(+), 7 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH v4 1/5] spapr: Implement 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>
---
 hw/ppc/spapr.c                  | 25 +++++++++++++++++++++++++
 hw/ppc/spapr_cpu_core.c         |  5 ++++-
 hw/ppc/spapr_hcall.c            |  5 -----
 include/hw/ppc/spapr.h          |  7 +++++++
 include/hw/ppc/spapr_cpu_core.h |  2 ++
 target/ppc/cpu.h                |  2 ++
 target/ppc/translate_init.inc.c | 25 +++++++++++++++++++++++++
 7 files changed, 65 insertions(+), 6 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 821f0d4a49..87b11e2484 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4302,6 +4302,29 @@ 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;
+    spapr_cpu->dispatch_counter++;
+    assert((spapr_cpu->dispatch_counter & 1) == 0);
+    if (spapr_cpu->vpa_addr) {
+        CPUState *cs = CPU(cpu);
+        stl_be_phys(cs->as, spapr_cpu->vpa_addr + VPA_DISPATCH_COUNTER, spapr_cpu->dispatch_counter);
+    }
+}
+
+static void spapr_cpu_exec_exit(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu)
+{
+    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
+
+    spapr_cpu->dispatch_counter++;
+    assert((spapr_cpu->dispatch_counter & 1) == 1);
+}
+
 static void spapr_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -4358,6 +4381,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..fb2ed9e95d 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,9 @@ 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);
+    spapr_cpu->dispatch_counter = 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..3032dfa7ee 100644
--- a/include/hw/ppc/spapr_cpu_core.h
+++ b/include/hw/ppc/spapr_cpu_core.h
@@ -46,6 +46,8 @@ typedef struct SpaprCpuState {
     uint64_t vpa_addr;
     uint64_t slb_shadow_addr, slb_shadow_size;
     uint64_t dtl_addr, dtl_size;
+    uint32_t dispatch_counter;
+    bool prod;
     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

Re: [Qemu-devel] [PATCH v4 1/5] spapr: Implement dispatch counter and prod bit on tcg

[David Gibson]

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

On Tue, Jul 16, 2019 at 12:47:22PM +1000, 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>
> ---
>  hw/ppc/spapr.c                  | 25 +++++++++++++++++++++++++
>  hw/ppc/spapr_cpu_core.c         |  5 ++++-
>  hw/ppc/spapr_hcall.c            |  5 -----
>  include/hw/ppc/spapr.h          |  7 +++++++
>  include/hw/ppc/spapr_cpu_core.h |  2 ++
>  target/ppc/cpu.h                |  2 ++
>  target/ppc/translate_init.inc.c | 25 +++++++++++++++++++++++++
>  7 files changed, 65 insertions(+), 6 deletions(-)

Mostly LGTM.  Please do address the style issues that the bot
reported.

In addition, do the dispatch_counter and prod values need to be
migrated?

> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 821f0d4a49..87b11e2484 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4302,6 +4302,29 @@ 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;
> +    spapr_cpu->dispatch_counter++;
> +    assert((spapr_cpu->dispatch_counter & 1) == 0);
> +    if (spapr_cpu->vpa_addr) {
> +        CPUState *cs = CPU(cpu);
> +        stl_be_phys(cs->as, spapr_cpu->vpa_addr + VPA_DISPATCH_COUNTER, spapr_cpu->dispatch_counter);
> +    }
> +}
> +
> +static void spapr_cpu_exec_exit(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu)
> +{
> +    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
> +
> +    spapr_cpu->dispatch_counter++;
> +    assert((spapr_cpu->dispatch_counter & 1) == 1);
> +}
> +
>  static void spapr_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> @@ -4358,6 +4381,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..fb2ed9e95d 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,9 @@ 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);
> +    spapr_cpu->dispatch_counter = 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..3032dfa7ee 100644
> --- a/include/hw/ppc/spapr_cpu_core.h
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -46,6 +46,8 @@ typedef struct SpaprCpuState {
>      uint64_t vpa_addr;
>      uint64_t slb_shadow_addr, slb_shadow_size;
>      uint64_t dtl_addr, dtl_size;
> +    uint32_t dispatch_counter;
> +    bool prod;
>      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";

-- 
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 v4 1/5] spapr: Implement dispatch counter and prod bit on tcg

[Nicholas Piggin]

David Gibson's on July 16, 2019 5:34 pm:
> On Tue, Jul 16, 2019 at 12:47:22PM +1000, 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>
>> ---
>>  hw/ppc/spapr.c                  | 25 +++++++++++++++++++++++++
>>  hw/ppc/spapr_cpu_core.c         |  5 ++++-
>>  hw/ppc/spapr_hcall.c            |  5 -----
>>  include/hw/ppc/spapr.h          |  7 +++++++
>>  include/hw/ppc/spapr_cpu_core.h |  2 ++
>>  target/ppc/cpu.h                |  2 ++
>>  target/ppc/translate_init.inc.c | 25 +++++++++++++++++++++++++
>>  7 files changed, 65 insertions(+), 6 deletions(-)
> 
> Mostly LGTM.  Please do address the style issues that the bot
> reported.

Will do.

> In addition, do the dispatch_counter and prod values need to be
> migrated?

I was thinking no, it should be just a blip. But could the guest
be reading the dispatch counter in the vpa for some other reason?
There are other (unimplemented) dispatch statistics in there AFAIKS.
Linux doesn't, but others might.

And what about KVM, does any of the VPA data get migrated for 
KVM guests?

Thanks,
Nick

Re: [Qemu-devel] [PATCH v4 1/5] spapr: Implement dispatch counter and prod bit on tcg

[David Gibson]

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

On Tue, Jul 16, 2019 at 07:27:22PM +1000, Nicholas Piggin wrote:
> David Gibson's on July 16, 2019 5:34 pm:
> > On Tue, Jul 16, 2019 at 12:47:22PM +1000, 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>
> >> ---
> >>  hw/ppc/spapr.c                  | 25 +++++++++++++++++++++++++
> >>  hw/ppc/spapr_cpu_core.c         |  5 ++++-
> >>  hw/ppc/spapr_hcall.c            |  5 -----
> >>  include/hw/ppc/spapr.h          |  7 +++++++
> >>  include/hw/ppc/spapr_cpu_core.h |  2 ++
> >>  target/ppc/cpu.h                |  2 ++
> >>  target/ppc/translate_init.inc.c | 25 +++++++++++++++++++++++++
> >>  7 files changed, 65 insertions(+), 6 deletions(-)
> > 
> > Mostly LGTM.  Please do address the style issues that the bot
> > reported.
> 
> Will do.
> 
> > In addition, do the dispatch_counter and prod values need to be
> > migrated?
> 
> I was thinking no, it should be just a blip. But could the guest
> be reading the dispatch counter in the vpa for some other reason?
> There are other (unimplemented) dispatch statistics in there AFAIKS.
> Linux doesn't, but others might.

Right, I guess it's a question of whether a guest would get confused
if the dispatch count suddenly resets.

> And what about KVM, does any of the VPA data get migrated for 
> KVM guests?

Well, the stuff that's actually written into the VPA is implicitly
migrated, since it's part of guest memory.

-- 
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 v4 1/5] spapr: Implement dispatch counter and prod bit on tcg

[Nicholas Piggin]

David Gibson's on July 17, 2019 11:51 am:
> On Tue, Jul 16, 2019 at 07:27:22PM +1000, Nicholas Piggin wrote:
>> David Gibson's on July 16, 2019 5:34 pm:
>> > On Tue, Jul 16, 2019 at 12:47:22PM +1000, 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>
>> >> ---
>> >>  hw/ppc/spapr.c                  | 25 +++++++++++++++++++++++++
>> >>  hw/ppc/spapr_cpu_core.c         |  5 ++++-
>> >>  hw/ppc/spapr_hcall.c            |  5 -----
>> >>  include/hw/ppc/spapr.h          |  7 +++++++
>> >>  include/hw/ppc/spapr_cpu_core.h |  2 ++
>> >>  target/ppc/cpu.h                |  2 ++
>> >>  target/ppc/translate_init.inc.c | 25 +++++++++++++++++++++++++
>> >>  7 files changed, 65 insertions(+), 6 deletions(-)
>> > 
>> > Mostly LGTM.  Please do address the style issues that the bot
>> > reported.
>> 
>> Will do.
>> 
>> > In addition, do the dispatch_counter and prod values need to be
>> > migrated?
>> 
>> I was thinking no, it should be just a blip. But could the guest
>> be reading the dispatch counter in the vpa for some other reason?
>> There are other (unimplemented) dispatch statistics in there AFAIKS.
>> Linux doesn't, but others might.
> 
> Right, I guess it's a question of whether a guest would get confused
> if the dispatch count suddenly resets.

Linux wouldn't AFAIKS, but...

>> And what about KVM, does any of the VPA data get migrated for 
>> KVM guests?
> 
> Well, the stuff that's actually written into the VPA is implicitly
> migrated, since it's part of guest memory.

Yeah I just sent a patch which uses the VPA directly, so it matches
KVM and should get migrated.

Thanks,
Nick

[Qemu-devel] [PATCH v4 2/5] 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

Re: [Qemu-devel] [PATCH v4 2/5] spapr: Implement H_PROD

[David Gibson]

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

On Tue, Jul 16, 2019 at 12:47:23PM +1000, 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>

LGTM apart from the style issues the bot noted.

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

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

[Qemu-devel] [PATCH v4 3/5] 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>
---
 hw/ppc/spapr_hcall.c | 48 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 8b208ab259..28d58113be 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1069,6 +1069,53 @@ static target_ulong h_cede(PowerPCCPU *cpu, SpaprMachineState *spapr,
     return H_SUCCESS;
 }
 
+static target_ulong h_confer(PowerPCCPU *cpu, SpaprMachineState *spapr,
+                           target_ulong opcode, target_ulong *args)
+{
+    target_long target = args[0];
+    uint32_t dispatch = args[1];
+    PowerPCCPU *target_cpu = spapr_find_cpu(target);
+    CPUState *target_cs = CPU(target_cpu);
+    CPUState *cs = CPU(cpu);
+    SpaprCpuState *spapr_cpu;
+
+    /*
+     * This does not do a targeted yield or confer, but check the parameter
+     * anyway. -1 means confer to all/any other CPUs.
+     */
+    if (target != -1 && !target_cs) {
+        return H_PARAMETER;
+    }
+
+    spapr_cpu = spapr_cpu_state(target_cpu);
+
+    /*
+     * PAPR specifies waiting until proded in this case, 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->dispatch_counter != dispatch || (dispatch & 1) == 0) {
+        return H_SUCCESS;
+    }
+
+    cs->exception_index = EXCP_YIELD;
+    cpu_loop_exit(cs);
+
+    return H_SUCCESS;
+}
+
 static target_ulong h_prod(PowerPCCPU *cpu, SpaprMachineState *spapr,
                            target_ulong opcode, target_ulong *args)
 {
@@ -1909,6 +1956,7 @@ static void hypercall_register_types(void)
     /* hcall-splpar */
     spapr_register_hypercall(H_REGISTER_VPA, h_register_vpa);
     spapr_register_hypercall(H_CEDE, h_cede);
+    spapr_register_hypercall(H_CONFER, h_confer);
     spapr_register_hypercall(H_PROD, h_prod);
 
     spapr_register_hypercall(H_SIGNAL_SYS_RESET, h_signal_sys_reset);
-- 
2.20.1

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

[David Gibson]

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

On Tue, Jul 16, 2019 at 12:47:24PM +1000, Nicholas Piggin wrote:
> This does not do directed yielding and is not quite as strict as PAPR
> specifies in terms of precise dispatch behaviour. This generally will
> mean suboptimal performance, rather than guest misbehaviour. Linux
> does not rely on exact dispatch behaviour.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  hw/ppc/spapr_hcall.c | 48 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 8b208ab259..28d58113be 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1069,6 +1069,53 @@ static target_ulong h_cede(PowerPCCPU *cpu, SpaprMachineState *spapr,
>      return H_SUCCESS;
>  }
>  
> +static target_ulong h_confer(PowerPCCPU *cpu, SpaprMachineState *spapr,
> +                           target_ulong opcode, target_ulong *args)
> +{
> +    target_long target = args[0];
> +    uint32_t dispatch = args[1];
> +    PowerPCCPU *target_cpu = spapr_find_cpu(target);
> +    CPUState *target_cs = CPU(target_cpu);
> +    CPUState *cs = CPU(cpu);
> +    SpaprCpuState *spapr_cpu;
> +
> +    /*
> +     * This does not do a targeted yield or confer, but check the parameter
> +     * anyway. -1 means confer to all/any other CPUs.
> +     */
> +    if (target != -1 && !target_cs) {
> +        return H_PARAMETER;
> +    }

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

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

s/proded/prodded/

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

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

> +
> +        return H_SUCCESS;
> +    }
> +
> +    if (spapr_cpu->dispatch_counter != dispatch || (dispatch & 1) == 0) {
> +        return H_SUCCESS;
> +    }
> +
> +    cs->exception_index = EXCP_YIELD;
> +    cpu_loop_exit(cs);
> +
> +    return H_SUCCESS;
> +}
> +
>  static target_ulong h_prod(PowerPCCPU *cpu, SpaprMachineState *spapr,
>                             target_ulong opcode, target_ulong *args)
>  {
> @@ -1909,6 +1956,7 @@ static void hypercall_register_types(void)
>      /* hcall-splpar */
>      spapr_register_hypercall(H_REGISTER_VPA, h_register_vpa);
>      spapr_register_hypercall(H_CEDE, h_cede);
> +    spapr_register_hypercall(H_CONFER, h_confer);
>      spapr_register_hypercall(H_PROD, h_prod);
>  
>      spapr_register_hypercall(H_SIGNAL_SYS_RESET, h_signal_sys_reset);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

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

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

[Nicholas Piggin]

David Gibson's on July 16, 2019 6:25 pm:
> On Tue, Jul 16, 2019 at 12:47:24PM +1000, Nicholas Piggin wrote:
>> This does not do directed yielding and is not quite as strict as PAPR
>> specifies in terms of precise dispatch behaviour. This generally will
>> mean suboptimal performance, rather than guest misbehaviour. Linux
>> does not rely on exact dispatch behaviour.
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>  hw/ppc/spapr_hcall.c | 48 ++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 48 insertions(+)
>> 
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index 8b208ab259..28d58113be 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -1069,6 +1069,53 @@ static target_ulong h_cede(PowerPCCPU *cpu, SpaprMachineState *spapr,
>>      return H_SUCCESS;
>>  }
>>  
>> +static target_ulong h_confer(PowerPCCPU *cpu, SpaprMachineState *spapr,
>> +                           target_ulong opcode, target_ulong *args)
>> +{
>> +    target_long target = args[0];
>> +    uint32_t dispatch = args[1];
>> +    PowerPCCPU *target_cpu = spapr_find_cpu(target);
>> +    CPUState *target_cs = CPU(target_cpu);
>> +    CPUState *cs = CPU(cpu);
>> +    SpaprCpuState *spapr_cpu;
>> +
>> +    /*
>> +     * This does not do a targeted yield or confer, but check the parameter
>> +     * anyway. -1 means confer to all/any other CPUs.
>> +     */
>> +    if (target != -1 && !target_cs) {
>> +        return H_PARAMETER;
>> +    }
> 
> Should we return an error if a targeted yield is attempted, rather
> than pretend we've done it?

I don't think so, because we do _some_ kind of yield for the directed
case which is probably better than nothing, and Linux won't fall back.

PAPR is much more strict about dispatching. The H_CONFERing vCPU must 
not run until the target(s) has been dispatched (if runnable), for
example. So we don't really implement it to the letter, we just do
"some kind of yield, whatever generic tcg code has implemented".

For single threaded tcg it seems a signifcant complication to the
round robin algorithm to add a directed yield, yet simply yielding
to the next vCPU is a good idea here because useful work will get
done including by the lock holder before we run again.

If multi threaded tcg performance with lot of vCPUs and lock contention
starts becoming more important I guess directed yielding might be
something to look at.

Thanks,
Nick

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

[David Gibson]

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

On Tue, Jul 16, 2019 at 08:25:28PM +1000, Nicholas Piggin wrote:
> David Gibson's on July 16, 2019 6:25 pm:
> > On Tue, Jul 16, 2019 at 12:47:24PM +1000, Nicholas Piggin wrote:
> >> This does not do directed yielding and is not quite as strict as PAPR
> >> specifies in terms of precise dispatch behaviour. This generally will
> >> mean suboptimal performance, rather than guest misbehaviour. Linux
> >> does not rely on exact dispatch behaviour.
> >> 
> >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> >> ---
> >>  hw/ppc/spapr_hcall.c | 48 ++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 48 insertions(+)
> >> 
> >> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> >> index 8b208ab259..28d58113be 100644
> >> --- a/hw/ppc/spapr_hcall.c
> >> +++ b/hw/ppc/spapr_hcall.c
> >> @@ -1069,6 +1069,53 @@ static target_ulong h_cede(PowerPCCPU *cpu, SpaprMachineState *spapr,
> >>      return H_SUCCESS;
> >>  }
> >>  
> >> +static target_ulong h_confer(PowerPCCPU *cpu, SpaprMachineState *spapr,
> >> +                           target_ulong opcode, target_ulong *args)
> >> +{
> >> +    target_long target = args[0];
> >> +    uint32_t dispatch = args[1];
> >> +    PowerPCCPU *target_cpu = spapr_find_cpu(target);
> >> +    CPUState *target_cs = CPU(target_cpu);
> >> +    CPUState *cs = CPU(cpu);
> >> +    SpaprCpuState *spapr_cpu;
> >> +
> >> +    /*
> >> +     * This does not do a targeted yield or confer, but check the parameter
> >> +     * anyway. -1 means confer to all/any other CPUs.
> >> +     */
> >> +    if (target != -1 && !target_cs) {
> >> +        return H_PARAMETER;
> >> +    }
> > 
> > Should we return an error if a targeted yield is attempted, rather
> > than pretend we've done it?
> 
> I don't think so, because we do _some_ kind of yield for the directed
> case which is probably better than nothing, and Linux won't fall back.
> 
> PAPR is much more strict about dispatching. The H_CONFERing vCPU must 
> not run until the target(s) has been dispatched (if runnable), for
> example. So we don't really implement it to the letter, we just do
> "some kind of yield, whatever generic tcg code has implemented".
> 
> For single threaded tcg it seems a signifcant complication to the
> round robin algorithm to add a directed yield, yet simply yielding
> to the next vCPU is a good idea here because useful work will get
> done including by the lock holder before we run again.
> 
> If multi threaded tcg performance with lot of vCPUs and lock contention
> starts becoming more important I guess directed yielding might be
> something to look at.

Ok, makes sense to me.

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

[Qemu-devel] [PATCH v4 4/5] 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>
---
 hw/ppc/spapr.c       |  1 +
 hw/ppc/spapr_hcall.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 87b11e2484..5c54e1cb9a 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 28d58113be..52847a7047 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1069,6 +1069,47 @@ 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)
 {
@@ -1959,6 +2000,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 v4 4/5] spapr: Implement H_JOIN

[David Gibson]

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

On Tue, Jul 16, 2019 at 12:47:25PM +1000, Nicholas Piggin wrote:
> This has been useful to modify and test the Linux pseries suspend
> code but it requires modification to the guest to call it (due to
> being gated by other unimplemented features). It is not otherwise
> used by Linux yet, but work is slowly progressing there.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

LGTM apart from style issues as noted by the bot.

> ---
>  hw/ppc/spapr.c       |  1 +
>  hw/ppc/spapr_hcall.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 87b11e2484..5c54e1cb9a 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 28d58113be..52847a7047 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1069,6 +1069,47 @@ 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)
>  {
> @@ -1959,6 +2000,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 */

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

[Qemu-devel] [PATCH v4 5/5] spapr: Implement ibm,suspend-me

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

This allows a (lightly modified) guest kernel to suspend with
`echo mem > /sys/power/state` and be resumed with system_wakeup
monitor command.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/spapr.c         | 26 ++++++++++++++++++++++++++
 hw/ppc/spapr_rtas.c    | 32 ++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h |  7 ++++++-
 3 files changed, 64 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 5c54e1cb9a..b85d41bb1e 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1710,6 +1710,11 @@ static void spapr_machine_reset(MachineState *machine)
     void *fdt;
     int rc;
 
+    if (spapr->suspend_reset) {
+        spapr->suspend_reset = false;
+        return;
+    }
+
     spapr_caps_apply(spapr);
 
     first_ppc_cpu = POWERPC_CPU(first_cpu);
@@ -2721,6 +2726,23 @@ static PCIHostState *spapr_create_default_phb(void)
     return PCI_HOST_BRIDGE(dev);
 }
 
+static Notifier wakeup;
+static void spapr_notify_wakeup(Notifier *notifier, void *data)
+{
+    WakeupReason *reason = data;
+
+    switch (*reason) {
+    case QEMU_WAKEUP_REASON_RTC:
+        break;
+    case QEMU_WAKEUP_REASON_PMTIMER:
+        break;
+    case QEMU_WAKEUP_REASON_OTHER:
+        break;
+    default:
+        break;
+    }
+}
+
 /* pSeries LPAR / sPAPR hardware init */
 static void spapr_machine_init(MachineState *machine)
 {
@@ -3078,6 +3100,10 @@ static void spapr_machine_init(MachineState *machine)
 
     qemu_register_boot_set(spapr_boot_set, spapr);
 
+    wakeup.notify = spapr_notify_wakeup;
+    qemu_register_wakeup_notifier(&wakeup);
+    qemu_register_wakeup_support();
+
     if (kvm_enabled()) {
         /* to stop and start vmclock */
         qemu_add_vm_change_state_handler(cpu_ppc_clock_vm_state_change,
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index a618a2ac0f..60a007ec38 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -216,6 +216,36 @@ static void rtas_stop_self(PowerPCCPU *cpu, SpaprMachineState *spapr,
     qemu_cpu_kick(cs);
 }
 
+static void rtas_ibm_suspend_me(PowerPCCPU *cpu, SpaprMachineState *spapr,
+                           uint32_t token, uint32_t nargs,
+                           target_ulong args,
+                           uint32_t nret, target_ulong rets)
+{
+    CPUState *cs;
+
+    if (nargs != 0 || nret != 1) {
+        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
+        return;
+    }
+
+    CPU_FOREACH(cs) {
+        PowerPCCPU *c = POWERPC_CPU(cs);
+        CPUPPCState *e = &c->env;
+        if (c == cpu)
+            continue;
+
+	/* See h_join */
+        if (!cs->halted || (e->msr & (1ULL << MSR_EE))) {
+            rtas_st(rets, 0, H_MULTI_THREADS_ACTIVE);
+            return;
+        }
+    }
+
+    spapr->suspend_reset = true;
+    qemu_system_suspend_request();
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+}
+
 static inline int sysparm_st(target_ulong addr, target_ulong len,
                              const void *val, uint16_t vallen)
 {
@@ -483,6 +513,8 @@ static void core_rtas_register_types(void)
                         rtas_query_cpu_stopped_state);
     spapr_rtas_register(RTAS_START_CPU, "start-cpu", rtas_start_cpu);
     spapr_rtas_register(RTAS_STOP_SELF, "stop-self", rtas_stop_self);
+    spapr_rtas_register(RTAS_IBM_SUSPEND_ME, "ibm,suspend-me",
+                        rtas_ibm_suspend_me);
     spapr_rtas_register(RTAS_IBM_GET_SYSTEM_PARAMETER,
                         "ibm,get-system-parameter",
                         rtas_ibm_get_system_parameter);
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 5d36eec9d0..df0b0c15da 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -171,6 +171,10 @@ struct SpaprMachineState {
     bool use_hotplug_event_source;
     SpaprEventSource *event_sources;
 
+    /* Machine has been suspended, so the next machine_reset should not
+     * reset state, but just return and allow execution to resume. */
+    bool suspend_reset;
+
     /* ibm,client-architecture-support option negotiation */
     bool cas_reboot;
     bool cas_legacy_guest_workaround;
@@ -631,8 +635,9 @@ target_ulong spapr_hypercall(PowerPCCPU *cpu, target_ulong opcode,
 #define RTAS_IBM_CREATE_PE_DMA_WINDOW           (RTAS_TOKEN_BASE + 0x27)
 #define RTAS_IBM_REMOVE_PE_DMA_WINDOW           (RTAS_TOKEN_BASE + 0x28)
 #define RTAS_IBM_RESET_PE_DMA_WINDOW            (RTAS_TOKEN_BASE + 0x29)
+#define RTAS_IBM_SUSPEND_ME                     (RTAS_TOKEN_BASE + 0x2A)
 
-#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x2A)
+#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x2B)
 
 /* RTAS ibm,get-system-parameter token values */
 #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS      20
-- 
2.20.1

Re: [Qemu-devel] [PATCH v4 5/5] spapr: Implement ibm,suspend-me

[David Gibson]

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

On Tue, Jul 16, 2019 at 12:47:26PM +1000, Nicholas Piggin wrote:
> This has been useful to modify and test the Linux pseries suspend
> code but it requires modification to the guest to call it (due to
> being gated by other unimplemented features). It is not otherwise
> used by Linux yet, but work is slowly progressing there.
> 
> This allows a (lightly modified) guest kernel to suspend with
> `echo mem > /sys/power/state` and be resumed with system_wakeup
> monitor command.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  hw/ppc/spapr.c         | 26 ++++++++++++++++++++++++++
>  hw/ppc/spapr_rtas.c    | 32 ++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h |  7 ++++++-
>  3 files changed, 64 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 5c54e1cb9a..b85d41bb1e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1710,6 +1710,11 @@ static void spapr_machine_reset(MachineState *machine)
>      void *fdt;
>      int rc;
>  
> +    if (spapr->suspend_reset) {
> +        spapr->suspend_reset = false;

Do we need to migrate this value?

> +        return;
> +    }
> +
>      spapr_caps_apply(spapr);
>  
>      first_ppc_cpu = POWERPC_CPU(first_cpu);
> @@ -2721,6 +2726,23 @@ static PCIHostState *spapr_create_default_phb(void)
>      return PCI_HOST_BRIDGE(dev);
>  }
>  
> +static Notifier wakeup;

I think this should be in sPAPRMachineState rather than global.

> +static void spapr_notify_wakeup(Notifier *notifier, void *data)
> +{
> +    WakeupReason *reason = data;
> +
> +    switch (*reason) {
> +    case QEMU_WAKEUP_REASON_RTC:
> +        break;
> +    case QEMU_WAKEUP_REASON_PMTIMER:
> +        break;
> +    case QEMU_WAKEUP_REASON_OTHER:
> +        break;
> +    default:
> +        break;
> +    }

So.. you have a bunch of switch cases, all of which ignore the input..

> +}
> +
>  /* pSeries LPAR / sPAPR hardware init */
>  static void spapr_machine_init(MachineState *machine)
>  {
> @@ -3078,6 +3100,10 @@ static void spapr_machine_init(MachineState *machine)
>  
>      qemu_register_boot_set(spapr_boot_set, spapr);
>  
> +    wakeup.notify = spapr_notify_wakeup;
> +    qemu_register_wakeup_notifier(&wakeup);
> +    qemu_register_wakeup_support();
> +
>      if (kvm_enabled()) {
>          /* to stop and start vmclock */
>          qemu_add_vm_change_state_handler(cpu_ppc_clock_vm_state_change,
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index a618a2ac0f..60a007ec38 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -216,6 +216,36 @@ static void rtas_stop_self(PowerPCCPU *cpu, SpaprMachineState *spapr,
>      qemu_cpu_kick(cs);
>  }
>  
> +static void rtas_ibm_suspend_me(PowerPCCPU *cpu, SpaprMachineState *spapr,
> +                           uint32_t token, uint32_t nargs,
> +                           target_ulong args,
> +                           uint32_t nret, target_ulong rets)
> +{
> +    CPUState *cs;
> +
> +    if (nargs != 0 || nret != 1) {
> +        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> +        return;
> +    }
> +
> +    CPU_FOREACH(cs) {
> +        PowerPCCPU *c = POWERPC_CPU(cs);
> +        CPUPPCState *e = &c->env;
> +        if (c == cpu)
> +            continue;
> +
> +	/* See h_join */
> +        if (!cs->halted || (e->msr & (1ULL << MSR_EE))) {
> +            rtas_st(rets, 0, H_MULTI_THREADS_ACTIVE);
> +            return;
> +        }
> +    }
> +
> +    spapr->suspend_reset = true;
> +    qemu_system_suspend_request();
> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> +}
> +
>  static inline int sysparm_st(target_ulong addr, target_ulong len,
>                               const void *val, uint16_t vallen)
>  {
> @@ -483,6 +513,8 @@ static void core_rtas_register_types(void)
>                          rtas_query_cpu_stopped_state);
>      spapr_rtas_register(RTAS_START_CPU, "start-cpu", rtas_start_cpu);
>      spapr_rtas_register(RTAS_STOP_SELF, "stop-self", rtas_stop_self);
> +    spapr_rtas_register(RTAS_IBM_SUSPEND_ME, "ibm,suspend-me",
> +                        rtas_ibm_suspend_me);
>      spapr_rtas_register(RTAS_IBM_GET_SYSTEM_PARAMETER,
>                          "ibm,get-system-parameter",
>                          rtas_ibm_get_system_parameter);
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 5d36eec9d0..df0b0c15da 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -171,6 +171,10 @@ struct SpaprMachineState {
>      bool use_hotplug_event_source;
>      SpaprEventSource *event_sources;
>  
> +    /* Machine has been suspended, so the next machine_reset should not
> +     * reset state, but just return and allow execution to resume. */
> +    bool suspend_reset;

Hrm, this seems odd, but maybe it's part of the existing suspend
design.  Why would system_reset resume a suspend, rather than having a
specific operation for that.

> +
>      /* ibm,client-architecture-support option negotiation */
>      bool cas_reboot;
>      bool cas_legacy_guest_workaround;
> @@ -631,8 +635,9 @@ target_ulong spapr_hypercall(PowerPCCPU *cpu, target_ulong opcode,
>  #define RTAS_IBM_CREATE_PE_DMA_WINDOW           (RTAS_TOKEN_BASE + 0x27)
>  #define RTAS_IBM_REMOVE_PE_DMA_WINDOW           (RTAS_TOKEN_BASE + 0x28)
>  #define RTAS_IBM_RESET_PE_DMA_WINDOW            (RTAS_TOKEN_BASE + 0x29)
> +#define RTAS_IBM_SUSPEND_ME                     (RTAS_TOKEN_BASE + 0x2A)
>  
> -#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x2A)
> +#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x2B)
>  
>  /* RTAS ibm,get-system-parameter token values */
>  #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS      20

-- 
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 v4 5/5] spapr: Implement ibm,suspend-me

[Nicholas Piggin]

David Gibson's on July 16, 2019 6:30 pm:
> On Tue, Jul 16, 2019 at 12:47:26PM +1000, Nicholas Piggin wrote:
>> This has been useful to modify and test the Linux pseries suspend
>> code but it requires modification to the guest to call it (due to
>> being gated by other unimplemented features). It is not otherwise
>> used by Linux yet, but work is slowly progressing there.
>> 
>> This allows a (lightly modified) guest kernel to suspend with
>> `echo mem > /sys/power/state` and be resumed with system_wakeup
>> monitor command.
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>  hw/ppc/spapr.c         | 26 ++++++++++++++++++++++++++
>>  hw/ppc/spapr_rtas.c    | 32 ++++++++++++++++++++++++++++++++
>>  include/hw/ppc/spapr.h |  7 ++++++-
>>  3 files changed, 64 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 5c54e1cb9a..b85d41bb1e 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -1710,6 +1710,11 @@ static void spapr_machine_reset(MachineState *machine)
>>      void *fdt;
>>      int rc;
>>  
>> +    if (spapr->suspend_reset) {
>> +        spapr->suspend_reset = false;
> 
> Do we need to migrate this value?

I suppose we do if we can migrate a suspended machine?

> 
>> +        return;
>> +    }
>> +
>>      spapr_caps_apply(spapr);
>>  
>>      first_ppc_cpu = POWERPC_CPU(first_cpu);
>> @@ -2721,6 +2726,23 @@ static PCIHostState *spapr_create_default_phb(void)
>>      return PCI_HOST_BRIDGE(dev);
>>  }
>>  
>> +static Notifier wakeup;
> 
> I think this should be in sPAPRMachineState rather than global.

Sure.

> 
>> +static void spapr_notify_wakeup(Notifier *notifier, void *data)
>> +{
>> +    WakeupReason *reason = data;
>> +
>> +    switch (*reason) {
>> +    case QEMU_WAKEUP_REASON_RTC:
>> +        break;
>> +    case QEMU_WAKEUP_REASON_PMTIMER:
>> +        break;
>> +    case QEMU_WAKEUP_REASON_OTHER:
>> +        break;
>> +    default:
>> +        break;
>> +    }
> 
> So.. you have a bunch of switch cases, all of which ignore the input..

Yeah I kind of just copy and pasted I think. This part of the patch
may not have been quite as cooked as I remembered :\

>> +}
>> +
>>  /* pSeries LPAR / sPAPR hardware init */
>>  static void spapr_machine_init(MachineState *machine)
>>  {
>> @@ -3078,6 +3100,10 @@ static void spapr_machine_init(MachineState *machine)
>>  
>>      qemu_register_boot_set(spapr_boot_set, spapr);
>>  
>> +    wakeup.notify = spapr_notify_wakeup;
>> +    qemu_register_wakeup_notifier(&wakeup);
>> +    qemu_register_wakeup_support();
>> +
>>      if (kvm_enabled()) {
>>          /* to stop and start vmclock */
>>          qemu_add_vm_change_state_handler(cpu_ppc_clock_vm_state_change,
>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>> index a618a2ac0f..60a007ec38 100644
>> --- a/hw/ppc/spapr_rtas.c
>> +++ b/hw/ppc/spapr_rtas.c
>> @@ -216,6 +216,36 @@ static void rtas_stop_self(PowerPCCPU *cpu, SpaprMachineState *spapr,
>>      qemu_cpu_kick(cs);
>>  }
>>  
>> +static void rtas_ibm_suspend_me(PowerPCCPU *cpu, SpaprMachineState *spapr,
>> +                           uint32_t token, uint32_t nargs,
>> +                           target_ulong args,
>> +                           uint32_t nret, target_ulong rets)
>> +{
>> +    CPUState *cs;
>> +
>> +    if (nargs != 0 || nret != 1) {
>> +        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>> +        return;
>> +    }
>> +
>> +    CPU_FOREACH(cs) {
>> +        PowerPCCPU *c = POWERPC_CPU(cs);
>> +        CPUPPCState *e = &c->env;
>> +        if (c == cpu)
>> +            continue;
>> +
>> +	/* See h_join */
>> +        if (!cs->halted || (e->msr & (1ULL << MSR_EE))) {
>> +            rtas_st(rets, 0, H_MULTI_THREADS_ACTIVE);
>> +            return;
>> +        }
>> +    }
>> +
>> +    spapr->suspend_reset = true;
>> +    qemu_system_suspend_request();
>> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>> +}
>> +
>>  static inline int sysparm_st(target_ulong addr, target_ulong len,
>>                               const void *val, uint16_t vallen)
>>  {
>> @@ -483,6 +513,8 @@ static void core_rtas_register_types(void)
>>                          rtas_query_cpu_stopped_state);
>>      spapr_rtas_register(RTAS_START_CPU, "start-cpu", rtas_start_cpu);
>>      spapr_rtas_register(RTAS_STOP_SELF, "stop-self", rtas_stop_self);
>> +    spapr_rtas_register(RTAS_IBM_SUSPEND_ME, "ibm,suspend-me",
>> +                        rtas_ibm_suspend_me);
>>      spapr_rtas_register(RTAS_IBM_GET_SYSTEM_PARAMETER,
>>                          "ibm,get-system-parameter",
>>                          rtas_ibm_get_system_parameter);
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index 5d36eec9d0..df0b0c15da 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -171,6 +171,10 @@ struct SpaprMachineState {
>>      bool use_hotplug_event_source;
>>      SpaprEventSource *event_sources;
>>  
>> +    /* Machine has been suspended, so the next machine_reset should not
>> +     * reset state, but just return and allow execution to resume. */
>> +    bool suspend_reset;
> 
> Hrm, this seems odd, but maybe it's part of the existing suspend
> design.  Why would system_reset resume a suspend, rather than having a
> specific operation for that.

It is where `system_wakeup` cmd pops out, via qemu_system_reset,
main_loop_should_exit. I'm not sure if we have any existing state
we can use. runstate_is_running() doesn't seem to work because of
CAS I guess (maybe CAS is what makes spapr so much different from
x86 in terms of resetting the world here?)

Thanks,
Nick

Re: [Qemu-devel] [PATCH v4 5/5] spapr: Implement ibm,suspend-me

[David Gibson]

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

On Tue, Jul 16, 2019 at 09:15:23PM +1000, Nicholas Piggin wrote:
65;5603;1c> David Gibson's on July 16, 2019 6:30 pm:
> > On Tue, Jul 16, 2019 at 12:47:26PM +1000, Nicholas Piggin wrote:
> >> This has been useful to modify and test the Linux pseries suspend
> >> code but it requires modification to the guest to call it (due to
> >> being gated by other unimplemented features). It is not otherwise
> >> used by Linux yet, but work is slowly progressing there.
> >> 
> >> This allows a (lightly modified) guest kernel to suspend with
> >> `echo mem > /sys/power/state` and be resumed with system_wakeup
> >> monitor command.
> >> 
> >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> >> ---
> >>  hw/ppc/spapr.c         | 26 ++++++++++++++++++++++++++
> >>  hw/ppc/spapr_rtas.c    | 32 ++++++++++++++++++++++++++++++++
> >>  include/hw/ppc/spapr.h |  7 ++++++-
> >>  3 files changed, 64 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index 5c54e1cb9a..b85d41bb1e 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -1710,6 +1710,11 @@ static void spapr_machine_reset(MachineState *machine)
> >>      void *fdt;
> >>      int rc;
> >>  
> >> +    if (spapr->suspend_reset) {
> >> +        spapr->suspend_reset = false;
> > 
> > Do we need to migrate this value?
> 
> I suppose we do if we can migrate a suspended machine?

I don't see why we couldn't.  And it might not happen because of
explicit user choice in a managed environment like RHV or openstack.

> >> +        return;
> >> +    }
> >> +
> >>      spapr_caps_apply(spapr);
> >>  
> >>      first_ppc_cpu = POWERPC_CPU(first_cpu);
> >> @@ -2721,6 +2726,23 @@ static PCIHostState *spapr_create_default_phb(void)
> >>      return PCI_HOST_BRIDGE(dev);
> >>  }
> >>  
> >> +static Notifier wakeup;
> > 
> > I think this should be in sPAPRMachineState rather than global.
> 
> Sure.
> 
> > 
> >> +static void spapr_notify_wakeup(Notifier *notifier, void *data)
> >> +{
> >> +    WakeupReason *reason = data;
> >> +
> >> +    switch (*reason) {
> >> +    case QEMU_WAKEUP_REASON_RTC:
> >> +        break;
> >> +    case QEMU_WAKEUP_REASON_PMTIMER:
> >> +        break;
> >> +    case QEMU_WAKEUP_REASON_OTHER:
> >> +        break;
> >> +    default:
> >> +        break;
> >> +    }
> > 
> > So.. you have a bunch of switch cases, all of which ignore the input..
> 
> Yeah I kind of just copy and pasted I think. This part of the patch
> may not have been quite as cooked as I remembered :\

Heh :).

> >> +}
> >> +
> >>  /* pSeries LPAR / sPAPR hardware init */
> >>  static void spapr_machine_init(MachineState *machine)
> >>  {
> >> @@ -3078,6 +3100,10 @@ static void spapr_machine_init(MachineState *machine)
> >>  
> >>      qemu_register_boot_set(spapr_boot_set, spapr);
> >>  
> >> +    wakeup.notify = spapr_notify_wakeup;
> >> +    qemu_register_wakeup_notifier(&wakeup);
> >> +    qemu_register_wakeup_support();
> >> +
> >>      if (kvm_enabled()) {
> >>          /* to stop and start vmclock */
> >>          qemu_add_vm_change_state_handler(cpu_ppc_clock_vm_state_change,
> >> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> >> index a618a2ac0f..60a007ec38 100644
> >> --- a/hw/ppc/spapr_rtas.c
> >> +++ b/hw/ppc/spapr_rtas.c
> >> @@ -216,6 +216,36 @@ static void rtas_stop_self(PowerPCCPU *cpu, SpaprMachineState *spapr,
> >>      qemu_cpu_kick(cs);
> >>  }
> >>  
> >> +static void rtas_ibm_suspend_me(PowerPCCPU *cpu, SpaprMachineState *spapr,
> >> +                           uint32_t token, uint32_t nargs,
> >> +                           target_ulong args,
> >> +                           uint32_t nret, target_ulong rets)
> >> +{
> >> +    CPUState *cs;
> >> +
> >> +    if (nargs != 0 || nret != 1) {
> >> +        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> >> +        return;
> >> +    }
> >> +
> >> +    CPU_FOREACH(cs) {
> >> +        PowerPCCPU *c = POWERPC_CPU(cs);
> >> +        CPUPPCState *e = &c->env;
> >> +        if (c == cpu)
> >> +            continue;
> >> +
> >> +	/* See h_join */
> >> +        if (!cs->halted || (e->msr & (1ULL << MSR_EE))) {
> >> +            rtas_st(rets, 0, H_MULTI_THREADS_ACTIVE);
> >> +            return;
> >> +        }
> >> +    }
> >> +
> >> +    spapr->suspend_reset = true;
> >> +    qemu_system_suspend_request();
> >> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> >> +}
> >> +
> >>  static inline int sysparm_st(target_ulong addr, target_ulong len,
> >>                               const void *val, uint16_t vallen)
> >>  {
> >> @@ -483,6 +513,8 @@ static void core_rtas_register_types(void)
> >>                          rtas_query_cpu_stopped_state);
> >>      spapr_rtas_register(RTAS_START_CPU, "start-cpu", rtas_start_cpu);
> >>      spapr_rtas_register(RTAS_STOP_SELF, "stop-self", rtas_stop_self);
> >> +    spapr_rtas_register(RTAS_IBM_SUSPEND_ME, "ibm,suspend-me",
> >> +                        rtas_ibm_suspend_me);
> >>      spapr_rtas_register(RTAS_IBM_GET_SYSTEM_PARAMETER,
> >>                          "ibm,get-system-parameter",
> >>                          rtas_ibm_get_system_parameter);
> >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> >> index 5d36eec9d0..df0b0c15da 100644
> >> --- a/include/hw/ppc/spapr.h
> >> +++ b/include/hw/ppc/spapr.h
> >> @@ -171,6 +171,10 @@ struct SpaprMachineState {
> >>      bool use_hotplug_event_source;
> >>      SpaprEventSource *event_sources;
> >>  
> >> +    /* Machine has been suspended, so the next machine_reset should not
> >> +     * reset state, but just return and allow execution to resume. */
> >> +    bool suspend_reset;
> > 
> > Hrm, this seems odd, but maybe it's part of the existing suspend
> > design.  Why would system_reset resume a suspend, rather than having a
> > specific operation for that.
> 
> It is where `system_wakeup` cmd pops out, via qemu_system_reset,
> main_loop_should_exit. I'm not sure if we have any existing state
> we can use. runstate_is_running() doesn't seem to work because of
> CAS I guess (maybe CAS is what makes spapr so much different from
> x86 in terms of resetting the world here?)

CAS certainly complicates things.

-- 
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 v4 0/5] spapr: implement dispatch and suspend calls

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190716024726.17864-1-npiggin@gmail.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH v4 0/5] spapr: implement dispatch and suspend calls
Message-id: 20190716024726.17864-1-npiggin@gmail.com
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Switched to a new branch 'test'
c432dea spapr: Implement ibm,suspend-me
09bae6b spapr: Implement H_JOIN
031bd3f spapr: Implement H_CONFER
a3d8bc2 spapr: Implement H_PROD
ed23e67 spapr: Implement dispatch counter and prod bit on tcg

=== OUTPUT BEGIN ===
1/5 Checking commit ed23e6798f18 (spapr: Implement dispatch counter and prod bit on tcg)
ERROR: line over 90 characters
#33: FILE: hw/ppc/spapr.c:4316:
+        stl_be_phys(cs->as, spapr_cpu->vpa_addr + VPA_DISPATCH_COUNTER, spapr_cpu->dispatch_counter);

total: 1 errors, 0 warnings, 131 lines checked

Patch 1/5 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/5 Checking commit a3d8bc2510cc (spapr: Implement H_PROD)
3/5 Checking commit 031bd3f6986b (spapr: Implement H_CONFER)
4/5 Checking commit 09bae6bec0c1 (spapr: Implement H_JOIN)
ERROR: braces {} are necessary for all arms of this statement
#55: FILE: hw/ppc/spapr_hcall.c:1092:
+        if (c == cpu)
[...]

ERROR: code indent should never use tabs
#58: FILE: hw/ppc/spapr_hcall.c:1095:
+^I/* Don't have a way to indicate joined, so use halted && MSR[EE]=0 */$

total: 2 errors, 0 warnings, 63 lines checked

Patch 4/5 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

5/5 Checking commit c432deae5480 (spapr: Implement ibm,suspend-me)
ERROR: braces {} are necessary for all arms of this statement
#93: FILE: hw/ppc/spapr_rtas.c:234:
+        if (c == cpu)
[...]

ERROR: code indent should never use tabs
#96: FILE: hw/ppc/spapr_rtas.c:237:
+^I/* See h_join */$

WARNING: Block comments use a leading /* on a separate line
#128: FILE: include/hw/ppc/spapr.h:174:
+    /* Machine has been suspended, so the next machine_reset should not

WARNING: Block comments use a trailing */ on a separate line
#129: FILE: include/hw/ppc/spapr.h:175:
+     * reset state, but just return and allow execution to resume. */

total: 2 errors, 2 warnings, 108 lines checked

Patch 5/5 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190716024726.17864-1-npiggin@gmail.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com