Re: [PATCH] xen: Don't call panic if ARM TF cpu off returns DENIED

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Julien,

Julien Grall <julien@xxxxxxx> writes:

> Hi,
>
> On 16/06/2022 14:55, dmitry.semenets@xxxxxxxxx wrote:
>> From: Dmytro Semenets <dmytro_semenets@xxxxxxxx>
>> According to PSCI specification ARM TF can return DENIED on CPU OFF.
>
> I am confused. The spec is talking about Trusted OS and not
> firmware. The docummentation is also not specific to ARM Trusted
> Firmware. So did you mean "Trusted OS"?

It should be "firmware", I believe.

>
> Also, did you reproduce on HW? If so, on which CPU this will fail?
>

Yes, we reproduced this on HW. In our case it failed on CPU0. To be
fair, in our case it had nothing to do with Trusted OS. It is just
platform limitation - it can't turn off CPU0. But from Xen perspective
there is no difference - CPU_OFF call returns DENIED.

>> This patch brings the hypervisor into compliance with the PSCI
>> specification.
>
> Now it means CPU off will never be turned off using PSCI. Instead, we
> would end up to spin in Xen. This would be a problem because we could
> save less power.

Agreed.

>
>> Refer to "Arm Power State Coordination Interface (DEN0022D.b)"
>> section 5.5.2
>
> Reading both 5.5.2 and 5.9.1 together, DENIED would be returned when
> the trusted OS can only run on one core.
>
> Some of the trusted OS are migratable. So I think we should first
> attempt to migrate the CPU. Then if it doesn't work, we should prevent
> the CPU to go offline.
>
> That said, upstream doesn't support cpu offlining (I don't know for
> your use case). In case of shutdown, it is not necessary to offline
> the CPU, so we could avoid to call CPU_OFF on all CPUs but
> one. Something like:
>

This is even better approach yes. But you mentioned CPU_OFF. Did you
mean SYSTEM_RESET?

> diff --git a/xen/arch/arm/shutdown.c b/xen/arch/arm/shutdown.c
> index 3dc6819d56de..d956812ef8f4 100644
> --- a/xen/arch/arm/shutdown.c
> +++ b/xen/arch/arm/shutdown.c
> @@ -8,7 +8,9 @@
>
>  static void noreturn halt_this_cpu(void *arg)
>  {
> -    stop_cpu();
> +    ASSERT(!local_irq_enable());
> +    while ( 1 )
> +        wfi();
>  }
>
>  void machine_halt(void)
> @@ -21,10 +23,6 @@ void machine_halt(void)
>      smp_call_function(halt_this_cpu, NULL, 0);
>      local_irq_disable();
>
> -    /* Wait at most another 10ms for all other CPUs to go offline. */
> -    while ( (num_online_cpus() > 1) && (timeout-- > 0) )
> -        mdelay(1);
> -
>      /* This is mainly for PSCI-0.2, which does not return if success. */
>      call_psci_system_off();
>
>> Signed-off-by: Dmytro Semenets <dmytro_semenets@xxxxxxxx>
>> Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
>
> I don't recall to see patch on the ML recently for this. So is this an
> internal review?

Yeah, sorry about that. Dmytro is a new member in our team and he is not
yet familiar with differences in internal reviews and reviews in ML.

If you are interested, we had internal review at [1]:

[1] https://github.com/xen-troops/xen/pull/184

>
>> ---
>>   xen/arch/arm/psci.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
>> index 0c90c2305c..55787fde58 100644
>> --- a/xen/arch/arm/psci.c
>> +++ b/xen/arch/arm/psci.c
>> @@ -63,8 +63,9 @@ void call_psci_cpu_off(void)
>>             /* If successfull the PSCI cpu_off call doesn't return
>> */
>>           arm_smccc_smc(PSCI_0_2_FN32_CPU_OFF, &res);
>> -        panic("PSCI cpu off failed for CPU%d err=%d\n", smp_processor_id(),
>> -              PSCI_RET(res));
>> +        if ( PSCI_RET(res) != PSCI_DENIED )
>> +            panic("PSCI cpu off failed for CPU%d err=%d\n", 
>> smp_processor_id(),
>> +                PSCI_RET(res));
>>       }
>>   }
>>   
>
> Cheers,


-- 
Volodymyr Babchuk at EPAM