Re: [PATCH] xen: arm: Don't use stop_cpu() in halt_this_cpu()

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 23 Jun 2022, dmitry.semenets@xxxxxxxxx wrote:
> From: Dmytro Semenets <dmytro_semenets@xxxxxxxx>
> 
> When shutting down (or rebooting) the platform, Xen will call stop_cpu()
> on all the CPUs but one. The last CPU will then request the system to
> shutdown/restart.
> 
> On platform using PSCI, stop_cpu() will call PSCI CPU off. Per the spec
> (section 5.5.2 DEN0022D.b), the call could return DENIED if the Trusted
> OS is resident on the CPU that is about to be turned off.
> 
> As Xen doesn't migrate off the trusted OS (which BTW may not be
> migratable), it would be possible to hit the panic().
> 
> In the ideal situation, Xen should migrate the trusted OS or make sure
> the CPU off is not called. However, when shutting down (or rebooting)
> the platform, it is pointless to try to turn off all the CPUs (per
> section 5.10.2, it is only required to put the core in a known state).
> 
> So solve the problem by open-coding stop_cpu() in halt_this_cpu() and
> not call PSCI CPU off.
> 
> Signed-off-by: Dmytro Semenets <dmytro_semenets@xxxxxxxx>
> ---
>  xen/arch/arm/shutdown.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/shutdown.c b/xen/arch/arm/shutdown.c
> index 3dc6819d56..a9aea19e8e 100644
> --- a/xen/arch/arm/shutdown.c
> +++ b/xen/arch/arm/shutdown.c
> @@ -8,7 +8,12 @@
>  
>  static void noreturn halt_this_cpu(void *arg)
>  {
> -    stop_cpu();
> +    local_irq_disable();
> +    /* Make sure the write happens before we sleep forever */
> +    dsb(sy);
> +    isb();
> +    while ( 1 )
> +        wfi();
>  }


stop_cpu has already a wfi loop just after the psci call:

    call_psci_cpu_off();

    while ( 1 )
        wfi();


So wouldn't it be better to remove the panic from the implementation of
call_psci_cpu_off?

The reason I am saying this is that stop_cpu is called in a number of
places beyond halt_this_cpu and as far as I can tell any of them could
trigger the panic. (I admit they are unlikely places but still.)


Also the PSCI spec explicitely mention CPU_OFF as a way to place CPUs in
a "known state" and doesn't offer any other examples. So although what
you wrote in the commit message is correct, using CPU_OFF seems to be
the less error prone way (in the sense of triggering firmware errors) to
place CPUs in a known state.

So I would simply remove the panic from call_psci_cpu_off, so that we
try CPU_OFF first, and if it doesn't work, we use the WFI loop as
backup. Also we get to fix all the other callers of stop_cpu this way.