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

[Julien Grall <julien.grall@xxxxxxx>]

On 16/04/18 17:52, Mirela Simonovic wrote:
> On Mon, Apr 16, 2018 at 4:26 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> > Hi Mirela,
> >
> >
> > On 16/04/18 11:02, Mirela Simonovic wrote:
> > > On Thu, Apr 12, 2018 at 3:31 PM, Julien Grall <julien.grall@xxxxxxx>
> > > wrote:
> > > > On 12/04/18 12:33, Mirela Simonovic wrote:
> > > > > On Wed, Apr 11, 2018 at 4:46 PM, Julien Grall <julien.grall@xxxxxxx>
> > > > > wrote:
> > > > > > On 11/04/18 14:19, Mirela Simonovic wrote:
> > I disagree here, if you are unable to turn off a CPU via PSCI then something
> > is definitely wrong. This means that CPU will forever spin in Xen code with
> > no way to exit. This could bring interesting issue with anything potentially
> > modifying Xen code (i.e livepatching).
> >
> > IHMO, the forever sleep in stop_cpu() is just a temporary solution to cater
> > shutdown of the platform. The state of secondary CPU does not much matter at
> > that time. In case of suspend/resume you want really want to be able to turn
> > off those CPUs correctly otherwise they are not going to come up again.
> If we follow that logic the CPU will not be able to exit WFI state
> either. So we should raise panic in that case as well and in cases
> where the system is suspending + the CPU is stopped + cpu off doesn't
> work so the CPU cannot be enabled again.
In case of suspend/resume you can check before hand whether we cpu_off 
is implemented for that platform. If not, you deny suspend.
This will avoid people using suspend/resume on those platforms.

> However, raising panic makes no sense for shutdown scenario.
I agree and that's why I suggested to move the panic in one of my 
previous e-mail:
"Furthermore, now on platform without CPU off support (e.g non-PSCI 
platform and PSCI 0.1) you will log an error message that may worry 
people. In reality, PSCI cpu_off will unlikely fail, so you probably 
want to add a panic in call_psci_cpu_off instead."
> How about we do it something like this:
>
> void stop_cpu(void)
> {
>      ...
>      if ( system_state == SYS_STATE_suspend )
>          call_psci_cpu_off();
>
>      while ( 1 )
>          wfi();
> }
>
> void call_psci_cpu_off(void)
> {
>      int errno;
>
>      /* If successfull the cpu_off call doesn't return */
>      errno = call_smc(PSCI_0_2_FN32_CPU_OFF, 0, 0, 0);
>      if ( errno )
>          panic("PSCI cpu off failed for CPU%d err=%d\n", get_processor_id(),
>                  errno);
> }
>
> call_psci_cpu_off should not check for PSCI version because we need to
> panic regardless.
Yes. There are no need for the check because it will never return on 
success.
Cheers,

--
Julien Grall

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