Re: [Xen-devel] [PATCH v2 03/10] xen/arm: Implement CPU_OFF PSCI call (physical interface)

[Julien Grall <julien.grall@xxxxxxx>]

Hi Mirela,

On 20/04/18 13:25, Mirela Simonovic wrote:
> During the system suspend to RAM non-boot CPUs will be hotplugged.
> This will be triggered via disable_nonboot_cpus() call. When
> hotplugged the CPU will end up in an infinite wfi loop in stop_cpu().
> This patch adds PSCI CPU_OFF call to the EL3 with the aim to get powered
> down the calling CPU during the suspend.
> If PSCI CPU_OFF call to the EL3 succeeds it will not return. Otherwise,
> when the PSCI CPU_OFF call returns we'll raise panic, because the calling
> CPU could be enabled afterwards.
> If a CPU executes stop_cpu() when the system is not suspending the
> calling CPU will loop in the infinite while/wfi, as it was looping
> before this change.
> Note that there is no check for PSCI version in PSCI CPU_OFF
> implementation because the version will be checked prior to triggering
> the system suspend.
Then the code should contain an ASSERT + comment to make it clear on the 
interface. But I don't think this is the right way to go (see below).
> Signed-off-by: Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>
>
> ---
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Julien Grall <julien.grall@xxxxxxx>
> ---
> Changes in v2:
> -Issue PSCI CPU_OFF only if the system is suspending
> -If PSCI CPU_OFF call fails (unlikely to ever happen) raise panic
> -Fixed commit message
> ---
>   xen/arch/arm/psci.c        | 11 +++++++++++
>   xen/arch/arm/smpboot.c     |  3 +++
>   xen/include/asm-arm/psci.h |  1 +
>   3 files changed, 15 insertions(+)
>
> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> index 94b616df9b..7f7b0695a3 100644
> --- a/xen/arch/arm/psci.c
> +++ b/xen/arch/arm/psci.c
> @@ -46,6 +46,17 @@ int call_psci_cpu_on(int cpu)
>       return call_smc(psci_cpu_on_nr, cpu_logical_map(cpu), 
> __pa(init_secondary), 0);
>   }
>   
> +void call_psci_cpu_off(void)
> +{
> +    int errno;
> +
> +    /* If successfull the PSCI cpu_off call doesn't return */
> +    errno = call_smc(PSCI_0_2_FN32_CPU_OFF, 0, 0, 0);
CPU_OFF will only return on error. So the if is not necessary below.

> +    if ( errno )
> +        panic("PSCI cpu off failed for CPU%d err=%d\n", get_processor_id(),
> +              errno);
> +}
> +
>   void call_psci_system_off(void)
>   {
>       if ( psci_ver > PSCI_VERSION(0, 1) )
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index b2116f0d2d..1ca3d63261 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -395,6 +395,9 @@ void stop_cpu(void)
>       /* Make sure the write happens before we sleep forever */
>       dsb(sy);
>       isb();
> +    if ( system_state == SYS_STATE_suspend )
I don't think this should be call only on suspend/resume. System 
shutdown could also benefit of PSCI CPU off. This is also paving the way 
to more use case of turning off a CPU.
Cheers,

--
Julien Grall

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