Re: [Xen-devel] [PATCH 08/18] xen/arm: Disable/enable non-boot physical CPUs on suspend/resume

[Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>]

On 11/14/2018 11:52 AM, Julien Grall wrote:
> Hi Mirela,
>
> On 12/11/2018 11:30, Mirela Simonovic wrote:
> > Non-boot CPUs have to be disabled on suspend and enabled on resume
> > (hotplug-based mechanism). Disabling non-boot CPUs will lead to PSCI
> > CPU_OFF to be called by each non-boot CPU. Depending on the underlying
> > platform capabilities, this may lead to the physical powering down of
> > CPUs. Tested on Xilinx Zynq Ultrascale+ MPSoC (including power down of
> > each non-boot CPU).
> >
> > Signed-off-by: Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>
> > Signed-off-by: Saeed Nowshadi <saeed.nowshadi@xxxxxxxxxx>
> > ---
> >   xen/arch/arm/suspend.c | 15 ++++++++++++++-
> >   1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/arm/suspend.c b/xen/arch/arm/suspend.c
> > index 575afd5eb8..dae1b1f7d6 100644
> > --- a/xen/arch/arm/suspend.c
> > +++ b/xen/arch/arm/suspend.c
> > @@ -1,4 +1,5 @@
> >   #include <xen/sched.h>
> > +#include <xen/cpu.h>
> >   #include <asm/cpufeature.h>
> >   #include <asm/event.h>
> >   #include <asm/psci.h>
> > @@ -115,17 +116,29 @@ static void vcpu_suspend(register_t epoint, 
> > register_t cid)
> >   /* Xen suspend. Note: data is not used (suspend is the suspend to 
> > RAM) */
> >   static long system_suspend(void *data)
> >   {
> > +    int status;
> > +
> >       BUG_ON(system_state != SYS_STATE_active);
> >         system_state = SYS_STATE_suspend;
> >       freeze_domains();
> >   +    status = disable_nonboot_cpus();
> > +    if ( status )
> > +    {
> > +        system_state = SYS_STATE_resume;
> > +        goto resume_nonboot_cpus;
> > +    }
> > +
> >       system_state = SYS_STATE_resume;
> >   +resume_nonboot_cpus:
> > +    enable_nonboot_cpus();
> >       thaw_domains();
> >       system_state = SYS_STATE_active;
> > +    dsb(sy);
> Why do you need a dsb(sy) here?
Updated value of system_state variable needs to be visible to other CPUs 
before we move on
> >   -    return -ENOSYS;
> Why do you return -ENOSYS until this patch? Should not have it be 0?
To distinguish that Xen suspend wasn't supported until we at least do 
something useful in suspend procedure. This is not so important, we can 
return 0 but needs to be fixed in previous patches.
> > +    return status;
> >   }
> >     int32_t domain_suspend(register_t epoint, register_t cid)
> Cheers,

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