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

[Julien Grall <julien@xen.org>]

Hi Stefano,

On 23/06/2022 23:07, Stefano Stabellini wrote:
> On Thu, 23 Jun 2022, dmitry.semenets@gmail.com wrote:
>> From: Dmytro Semenets <dmytro_semenets@epam.com>
> So wouldn't it be better to remove the panic from the implementation of
> call_psci_cpu_off?

I have asked to keep the panic() in call_psci_cpu_off(). If you remove 
the panic() then we will hide the fact that the CPU was not properly 
turned off and will consume more energy than expected.

The WFI loop is fine when shutting down or rebooting because we know 
this will only happen for a short period of time.

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

This is one of the example where the CPU will not be stopped for a short 
period of time. We should deal with them differently (i.e. migrating the 
trusted OS or else) so we give all the chance for the CPU to be fully 
powered.

IMHO, this is a different issue and hence why I didn't ask Dmitry to 
solve it.

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

The section you are referring to is starting with "One way". This imply 
there are others methods.

FWIW, the spin loop above seems to be how Linux is dealing with the 
shutdown/reboot.

> 
> 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.
This reads strange. In the previous paragraph you suggested the CPU off 
is a less error prone way to place CPUs in a known state. But here, you 
are softening the stance and suggesting to fallback to the WFI loop.

So to me this indicates that WFI loop is fine. Otherwise, wouldn't the 
user risk to see firmware errors (which BTW, I don't understand what 
sort of firmware errors you are worried)? If yes, why would it be 
acceptable?

For instance, Dmitry situation, the CPU0 would always WFI loop...

Cheers,

-- 
Julien Grall