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

[David Gibson <david@gibson.dropbear.id.au>]

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