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

[Julien Grall <julien@xxxxxxx>]

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"?
Also, did you reproduce on HW? If so, on which CPU this will fail?

> 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.
> 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:
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?
> ---
>   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,

--
Julien Grall