Re: [Xen-devel] [PATCH 3/7] xen/arm/psci: Implement CPU_OFF PSCI call (physical interface)

[Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>]

Hi Julien,

On Wed, Apr 11, 2018 at 4:46 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> Hi,
>
> On 11/04/18 14:19, Mirela Simonovic wrote:
>>
>> This patch adds the PSCI CPU_OFF call to the EL3 in order to
>> trigger powering down of the calling CPU when the CPU is stopped.
>> If CPU_OFF call fails for some reason, e.g. EL3 does not implement
>> the PSCI CPU_OFF function, the calling CPU will loop in the infinite >
>> while/wfi, as it was looping before this change.
>
>
> I am afraid that the example you give is wrong. That call should exist for
> all implementation of PSCI 0.2 and above. For 0.1, this should never be call
> as the ID is not reserved.

Thanks.

>
>>
>> Signed-off-by: Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>
>> ---
>>   xen/arch/arm/psci.c        | 5 +++++
>>   xen/arch/arm/smpboot.c     | 7 +++++++
>>   xen/include/asm-arm/psci.h | 1 +
>>   3 files changed, 13 insertions(+)
>>
>> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
>> index 94b616df9b..e9e756e56b 100644
>> --- a/xen/arch/arm/psci.c
>> +++ b/xen/arch/arm/psci.c
>> @@ -46,6 +46,11 @@ int call_psci_cpu_on(int cpu)
>>       return call_smc(psci_cpu_on_nr, cpu_logical_map(cpu),
>> __pa(init_secondary), 0);
>>   }
>>   +int call_psci_cpu_off(void)
>> +{
>
>
> You have to check the PSCI version here before calling the function.
>

Thanks, I fixed this.

>> +    return call_smc(PSCI_0_2_FN32_CPU_OFF, 0, 0, 0);
>> +}
>> +
>>   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..5666efcd3a 100644
>> --- a/xen/arch/arm/smpboot.c
>> +++ b/xen/arch/arm/smpboot.c
>> @@ -390,11 +390,18 @@ void __cpu_disable(void)
>>     void stop_cpu(void)
>>   {
>> +    int errno;
>
>
> newline.

Got it, thanks

>
>>       local_irq_disable();
>>       cpu_is_dead = true;
>>       /* Make sure the write happens before we sleep forever */
>>       dsb(sy);
>>       isb();
>> +    /* PSCI cpu off call will return only in case of an error */
>> +    errno = call_psci_cpu_off();
>> +    printk(XENLOG_DEBUG "PSCI cpu off call failed for CPU#%d err=%d\n",
>> +           get_processor_id(), errno);
>> +    isb();
>
>
> What are you trying to achieve with the isb() here?
>

I use to have a problem that the wfi below gets executed before the
call_psci_cpu_off(). Adding isb() fixed the issue. However, I tried
now to reproduce the problem and it doesn't show up. I still believe
isb() should be here, please let me know if you disagree (I obviously
can't prove the claim now).

Thanks,
Mirela

> Cheers,
>
>> +    /* If CPU_OFF PSCI call failed stay in the WFI loop */
>>       while ( 1 )
>>           wfi();
>>   }
>> diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
>> index 9ac820e94a..50d668a296 100644
>> --- a/xen/include/asm-arm/psci.h
>> +++ b/xen/include/asm-arm/psci.h
>> @@ -20,6 +20,7 @@ extern uint32_t psci_ver;
>>     int psci_init(void);
>>   int call_psci_cpu_on(int cpu);
>> +int call_psci_cpu_off(void);
>>   void call_psci_system_off(void);
>>   void call_psci_system_reset(void);
>>
>
>
> Cheers,
>
> --
> Julien Grall

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