Re: [PATCH 05/18] xen/arm: Trigger Xen suspend when Dom0 completes suspend

[Stefano Stabellini <sstabellini@kernel.org>]

On Mon, 12 Nov 2018, Julien Grall wrote:
> Hi,
> 
> On 11/12/18 11:30 AM, Mirela Simonovic wrote:
> > When Dom0 finalizes its suspend procedure the suspend of Xen is
> > triggered by calling system_suspend(). Dom0 finalizes the suspend from
> > its boot core (VCPU#0), which could be mapped to any physical CPU,
> > i.e. the system_suspend() function could be executed by any physical
> > CPU. Since Xen suspend procedure has to be run by the boot CPU
> > (non-boot CPUs will be disabled at some point in suspend procedure),
> > system_suspend() execution has to continue on CPU#0.
> 
> Nothing in the domain_suspend code checks that domain_suspend is called by
> vCPU0. I also can't find any restriction in the PSCI spec to run on pCPU0. Can
> you provide more details why this required?

The spec says that "to use this API, a calling OS must power down all
but one core through calls to CPU_OFF". It is natural to think that the
remaining core would be (physical or virtual) cpu0, but actually it is
not clearly stated by the spec. For dom0, we could simply check that
only 1 vcpu is left ON.  For Xen and the physical system_suspend call,
it makes sense to make the call on pcpu0.

 
> > When the system_suspend() returns 0, it means that the system was
> > suspended and it is coming out of the resume procedure. Regardless
> > of the system_suspend() return value, after this function returns
> > Xen is fully functional, and its state, including all devices and data
> > structures, matches the state prior to calling system_suspend().
> > The status is returned by system_suspend() for debugging/logging
> > purposes and function prototype compatibility.
> > 
> > Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
> > Signed-off-by: Saeed Nowshadi <saeed.nowshadi@xilinx.com>
> > ---
> >   xen/arch/arm/suspend.c | 34 ++++++++++++++++++++++++++++++++++
> >   1 file changed, 34 insertions(+)
> > 
> > diff --git a/xen/arch/arm/suspend.c b/xen/arch/arm/suspend.c
> > index f2338e41db..21b45f8248 100644
> > --- a/xen/arch/arm/suspend.c
> > +++ b/xen/arch/arm/suspend.c
> > @@ -112,11 +112,20 @@ static void vcpu_suspend(register_t epoint, register_t
> > cid)
> >       _arch_set_info_guest(v, &ctxt);
> >   }
> >   +/* Xen suspend. Note: data is not used (suspend is the suspend to RAM) */
> > +static long system_suspend(void *data)
> > +{
> > +    BUG_ON(system_state != SYS_STATE_active);
> > +
> > +    return -ENOSYS;
> > +}
> > +
> >   int32_t domain_suspend(register_t epoint, register_t cid)
> >   {
> >       struct vcpu *v;
> >       struct domain *d = current->domain;
> >       bool is_thumb = epoint & 1;
> > +    int status;
> >         dprintk(XENLOG_DEBUG,
> >               "Dom%d suspend: epoint=0x%"PRIregister",
> > cid=0x%"PRIregister"\n",
> > @@ -156,6 +165,31 @@ int32_t domain_suspend(register_t epoint, register_t
> > cid)
> >        */
> >       vcpu_block_unless_event_pending(current);
> >   +    /* If this was dom0 the whole system should suspend: trigger Xen
> > suspend */
> > +    if ( is_hardware_domain(d) )
> > +    {
> > +        /*
> > +         * system_suspend should be called when Dom0 finalizes the suspend
> > +         * procedure from its boot core (VCPU#0). However, Dom0's VCPU#0
> > could
> > +         * be mapped to any PCPU (this function could be executed by any
> > PCPU).
> > +         * The suspend procedure has to be finalized by the PCPU#0
> > (non-boot
> > +         * PCPUs will be disabled during the suspend).
> > +         */
> > +        status = continue_hypercall_on_cpu(0, system_suspend, NULL);
> > +        /*
> > +         * If an error happened, there is nothing that needs to be done
> > here
> > +         * because the system_suspend always returns in fully functional
> > state
> > +         * no matter what the outcome of suspend procedure is. If the
> > system
> > +         * suspended successfully the function will return 0 after the
> > resume.
> > +         * Otherwise, if an error is returned it means Xen did not
> > suspended,
> > +         * but it is still in the same state as if the system_suspend was
> > never
> > +         * called. We dump a debug message in case of an error for
> > debugging/
> > +         * logging purpose.
> > +         */
> > +        if ( status )
> > +            dprintk(XENLOG_ERR, "Failed to suspend, errno=%d\n", status);
> > +    }
> > +
> >       return PSCI_SUCCESS;
> >   }
> >   
> 
> -- 
> Julien Grall
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel