[Qemu-devel] [PATCH v2 2/3] i386: use machine class ->wakeup method

[Qemu-devel] [PATCH v2 2/3] i386: use machine class ->wakeup method

[Nicholas Piggin]

Move the i386 suspend_wakeup logic out of the fallback path, and into
the new ->wakeup method.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/i386/pc.c | 8 ++++++++
 vl.c         | 2 --
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 549c437050..78c03d7f9d 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2828,6 +2828,13 @@ static void pc_machine_reset(MachineState *machine)
     }
 }
 
+static void pc_machine_wakeup(MachineState *machine)
+{
+    cpu_synchronize_all_states();
+    pc_machine_reset(machine);
+    cpu_synchronize_all_post_reset();
+}
+
 static CpuInstanceProperties
 pc_cpu_index_to_props(MachineState *ms, unsigned cpu_index)
 {
@@ -2940,6 +2947,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     mc->block_default_type = IF_IDE;
     mc->max_cpus = 255;
     mc->reset = pc_machine_reset;
+    mc->wakeup = pc_machine_wakeup;
     hc->pre_plug = pc_machine_device_pre_plug_cb;
     hc->plug = pc_machine_device_plug_cb;
     hc->unplug_request = pc_machine_device_unplug_request_cb;
diff --git a/vl.c b/vl.c
index 45ea034410..3f50dd685b 100644
--- a/vl.c
+++ b/vl.c
@@ -1567,8 +1567,6 @@ static void qemu_system_wakeup(void)
 
     if (mc && mc->wakeup) {
         mc->wakeup(current_machine);
-    } else {
-        qemu_system_reset(SHUTDOWN_CAUSE_NONE);
     }
 }
 
-- 
2.20.1

[Qemu-devel] [PATCH v2 3/3] 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         |  7 +++++++
 hw/ppc/spapr_rtas.c    | 32 ++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h |  3 ++-
 3 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 00f7735a31..a580466d01 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3078,6 +3078,13 @@ static void spapr_machine_init(MachineState *machine)
 
     qemu_register_boot_set(spapr_boot_set, spapr);
 
+    /*
+     * Nothing needs to be done to resume a suspended guest because
+     * suspending does not change the machine state, so no need for
+     * a ->wakeup method.
+     */
+    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..87175c1e0a 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;
+        }
+    }
+
+    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..6e8e18b077 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -631,8 +631,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 v2 3/3] spapr: Implement ibm,suspend-me

[David Gibson]

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

On Mon, Jul 22, 2019 at 04:17:52PM +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>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/ppc/spapr.c         |  7 +++++++
>  hw/ppc/spapr_rtas.c    | 32 ++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h |  3 ++-
>  3 files changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 00f7735a31..a580466d01 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3078,6 +3078,13 @@ static void spapr_machine_init(MachineState *machine)
>  
>      qemu_register_boot_set(spapr_boot_set, spapr);
>  
> +    /*
> +     * Nothing needs to be done to resume a suspended guest because
> +     * suspending does not change the machine state, so no need for
> +     * a ->wakeup method.
> +     */
> +    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..87175c1e0a 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;
> +        }
> +    }
> +
> +    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..6e8e18b077 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -631,8 +631,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 v2 2/3] i386: use machine class ->wakeup method

[David Gibson]

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

On Mon, Jul 22, 2019 at 04:17:51PM +1000, Nicholas Piggin wrote:
> Move the i386 suspend_wakeup logic out of the fallback path, and into
> the new ->wakeup method.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/i386/pc.c | 8 ++++++++
>  vl.c         | 2 --
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 549c437050..78c03d7f9d 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2828,6 +2828,13 @@ static void pc_machine_reset(MachineState *machine)
>      }
>  }
>  
> +static void pc_machine_wakeup(MachineState *machine)
> +{
> +    cpu_synchronize_all_states();
> +    pc_machine_reset(machine);
> +    cpu_synchronize_all_post_reset();
> +}
> +
>  static CpuInstanceProperties
>  pc_cpu_index_to_props(MachineState *ms, unsigned cpu_index)
>  {
> @@ -2940,6 +2947,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>      mc->block_default_type = IF_IDE;
>      mc->max_cpus = 255;
>      mc->reset = pc_machine_reset;
> +    mc->wakeup = pc_machine_wakeup;
>      hc->pre_plug = pc_machine_device_pre_plug_cb;
>      hc->plug = pc_machine_device_plug_cb;
>      hc->unplug_request = pc_machine_device_unplug_request_cb;
> diff --git a/vl.c b/vl.c
> index 45ea034410..3f50dd685b 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1567,8 +1567,6 @@ static void qemu_system_wakeup(void)
>  
>      if (mc && mc->wakeup) {
>          mc->wakeup(current_machine);
> -    } else {
> -        qemu_system_reset(SHUTDOWN_CAUSE_NONE);
>      }
>  }
>  

-- 
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 v2 2/3] i386: use machine class ->wakeup method

[Paolo Bonzini]

On 22/07/19 08:17, Nicholas Piggin wrote:
> Move the i386 suspend_wakeup logic out of the fallback path, and into
> the new ->wakeup method.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  hw/i386/pc.c | 8 ++++++++
>  vl.c         | 2 --
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 549c437050..78c03d7f9d 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2828,6 +2828,13 @@ static void pc_machine_reset(MachineState *machine)
>      }
>  }
>  
> +static void pc_machine_wakeup(MachineState *machine)
> +{
> +    cpu_synchronize_all_states();
> +    pc_machine_reset(machine);
> +    cpu_synchronize_all_post_reset();
> +}
> +
>  static CpuInstanceProperties
>  pc_cpu_index_to_props(MachineState *ms, unsigned cpu_index)
>  {
> @@ -2940,6 +2947,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>      mc->block_default_type = IF_IDE;
>      mc->max_cpus = 255;
>      mc->reset = pc_machine_reset;
> +    mc->wakeup = pc_machine_wakeup;
>      hc->pre_plug = pc_machine_device_pre_plug_cb;
>      hc->plug = pc_machine_device_plug_cb;
>      hc->unplug_request = pc_machine_device_unplug_request_cb;
> diff --git a/vl.c b/vl.c
> index 45ea034410..3f50dd685b 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1567,8 +1567,6 @@ static void qemu_system_wakeup(void)
>  
>      if (mc && mc->wakeup) {
>          mc->wakeup(current_machine);
> -    } else {
> -        qemu_system_reset(SHUTDOWN_CAUSE_NONE);
>      }
>  }
>  
> 

Looks good, I wouldn't mind adding a machine_reset() function with most
of the code from qemu_system_reset but it can be done later.

Paolo