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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 14 Nov 2018, Julien Grall wrote:
> Hi Mirela,
> 
> On 14/11/2018 13:00, Mirela Simonovic wrote:
> > 
> > 
> > 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
> 
> We tend to write the reason on top of barrier why they are necessary. But I am
> still unsure to understand why this is important. What would happen if move on
> without it?

That is a good question. I went through the code and it seems that the
only effect could be potentially taking the wrong path in
cpupool_cpu_add, but since that's called from a .notifier_call I don't
think it can happen in practice. It is always difficult to prove that
we don't need a barrier, it is easier to prove when we need a barrier,
but in this case it does look like we do not need it after all.

 
> > > >   -    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.
> 
> If you return 0 before hand you can more easily bisect this series and know
> where it suspend/resume breaks.

Why 0 improves bisectability? 0 prevents other checks from figuring out
that there was an error. Also, the feature is not bisectable until the
full series is applied.

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