Re: [Minios-devel] [UNIKRAFT PATCHv5 40/46] plat/common: Implement CPU reset for arm64

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

On 12.09.2018 11:11, Wei Chen (Arm Technology China) wrote:
> Hi Simon,
>
> > -----Original Message-----
> > From: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
> > Sent: 2018年9月12日 16:59
> > To: Wei Chen (Arm Technology China) <Wei.Chen@xxxxxxx>; minios-
> > devel@xxxxxxxxxxxxxxxxxxxx
> > Cc: Kaly Xin (Arm Technology China) <Kaly.Xin@xxxxxxx>; nd <nd@xxxxxxx>
> > Subject: Re: [Minios-devel] [UNIKRAFT PATCHv5 40/46] plat/common: Implement
> > CPU reset for arm64
> >
> > On 12.09.2018 08:19, Wei Chen (Arm Technology China) wrote:
> > > > -----Original Message-----
> > > > From: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
> > > > Sent: 2018年9月12日 4:23
> > > > To: Wei Chen (Arm Technology China) <Wei.Chen@xxxxxxx>; minios-
> > > > devel@xxxxxxxxxxxxxxxxxxxx
> > > > Cc: Kaly Xin (Arm Technology China) <Kaly.Xin@xxxxxxx>; nd <nd@xxxxxxx>
> > > > Subject: Re: [Minios-devel] [UNIKRAFT PATCHv5 40/46] plat/common: Implement
> > > > CPU reset for arm64
> > > >
> > > >
> > > >
> > > > On 11.09.2018 22:20, Simon Kuenzer wrote:
> > > > > On 10.08.2018 09:08, Wei Chen wrote:
> > > > > > From: Wei Chen <Wei.Chen@xxxxxxx>
> > > > > >
> > > > > > Using PSCI despatch function to call PSCI_CPU_RESET
> > > > > > function to reset CPU.
> > > > > >
> > > > > > Signed-off-by: Wei Chen <Wei.Chen@xxxxxxx>
> > > > > > ---
> > > > > >     plat/common/arm/cpu_native.c        | 7 +++++++
> > > > > >     plat/common/include/arm/arm64/cpu.h | 1 +
> > > > > >     2 files changed, 8 insertions(+)
> > > > > >
> > > > > > diff --git a/plat/common/arm/cpu_native.c b/plat/common/arm/cpu_native.c
> > > > > > index 2d1a1aa..429b172 100644
> > > > > > --- a/plat/common/arm/cpu_native.c
> > > > > > +++ b/plat/common/arm/cpu_native.c
> > > > > > @@ -43,3 +43,10 @@ void halt(void)
> > > > > >     {
> > > > > >         __asm__ __volatile__("wfi");
> > > > > >     }
> > > > > > +
> > > > > > +/* Systems support PSCI >= 0.2 can do system reset from PSCI */
> > > > > > +void reset(void)
> > > > > > +{
> > > > > > +    UK_BUGON (!smcc_psci_call);
> > > > > Maybe we should have failed earlier during boot when we failed to detect
> > > > > the mode. Then you could use UK_ASSERT() instead. What do you think?
> > > > Alternatively we could enter a busy loop if we fail to get the mode
> > > > (same for shutdown in the next patch).
> > > I am considering let return directly, and give its caller the chance
> > > to do some remedy if reset failed.
> > Hum... we defined ukplat_terminate() with '__noreturn' attribute. The
> > question would also be, what else would you like to do if you cannot
> > reset the system. This function is also called from trap handlers.
> >
> > Because of this attribute, returning wouldn't be possible without
> > changing the ukplat API. Personally, I would prefer to print that reset
> > failed (same for shutdown) and enter an endless halt loop. I think this
> > should also be the fallback behavior for the case were we failed to
> > detect the PSCI mode with the DTB.
> >
> > So, I agree it is fine (or even better) if you prefer continue booting
> > whenever you could not detect the PSCI mode but I would prefer that the
> > ukplat_terminate keeps the __noreturn attribute.
> >
> > What do you think?
> I agree with you.
;-) Then I think this UK_BUGON() is anyway going to get removed from 
these functions. It could have even caused a endless recursion problem 
because UK_BUGON() is calling ukplat_terminate() in an error case.
> > > > > > +    smcc_psci_call(PSCI_FNID_SYSTEM_RESET, 0, 0, 0);
> > > > > > +}
> > > > > > diff --git a/plat/common/include/arm/arm64/cpu.h
> > > > > > b/plat/common/include/arm/arm64/cpu.h
> > > > > > index b231d32..c156fd3 100644
> > > > > > --- a/plat/common/include/arm/arm64/cpu.h
> > > > > > +++ b/plat/common/include/arm/arm64/cpu.h
> > > > > > @@ -102,3 +102,4 @@ int32_t smcc_psci_smc_call(uint32_t, uint64_t,
> > > > > > uint64_t, uint64_t);
> > > > > >     /* CPU native APIs */
> > > > > >     void halt(void);
> > > > > > +void reset(void);
> > > > > _______________________________________________
> > > > > Minios-devel mailing list
> > > > > Minios-devel@xxxxxxxxxxxxxxxxxxxx
> > > > > https://lists.xenproject.org/mailman/listinfo/minios-devel
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel