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

[Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>]

Hi Julien,

On Mon, Apr 23, 2018 at 1:21 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> 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.
>

Correct

>> +    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.

Ok, but then we do need to check for PSCI version here.

Thanks,
Mirela

>
> Cheers,
>
> --
> Julien Grall

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