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

[Julien Grall <julien@xxxxxxx>]

On 16/06/2022 19:40, Volodymyr Babchuk wrote:
> Hi Julien,
Hi Volodymyr,

> 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.
Hmmm... I couldn't find a reference in the spec suggesting that CPU_OFF 
could return DENIED because of the firmware. Do you have a pointer to 
the spec?
> > 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.
Thanks for the clarification. I think I have seen that in the wild also 
but it never got on top of my queue. It is good that we are fixing it.
> > > 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?
By CPU_OFF I was referring to the fact that Xen will issue the call all 
CPUs but one. The remaining CPU will issue the command to reset/shutdown 
the system.
> >   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.
No worries. I usually classify internal review anything that was done 
privately. This looks to be a public review, althought not on xen-devel.
I understand that not all some of the patches are still in PoC stage and 
doing the review on your github is a good idea. But for those are meant 
to be for upstream (e.g. bug fixes, small patches), I would suggest to 
do the review on xen-devel directly.
Cheers,

--
Julien Grall