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

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 13/11/2018 22:35, Stefano Stabellini wrote:
> On Mon, 12 Nov 2018, 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);
> >   
> > -    return -ENOSYS;
> > +    return status;
> >   }
> I think we need a spin_lock to protect system_suspend from concurrent
> calls, or (better) we need to make sure that the caller is only allowed
> to call system_suspend if there is just one vcpu active in the system,
> and that vcpu is blocked on this PSCI system suspend call.
I don't think this is correct. It is valid to have more than on vCPU running 
when calling system_suspend. An example would be Dom0 suspending while the other 
domain are still running.
This is handled properly via freeze_domains()/thaw_domains().

So a spinlock would be a better solution here. If we can't acquire the lock then 
the function would return -EBUSY.
Cheers,

--
Julien Grall

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