[U-Boot] Handling hangs (was: [PATCH v1] arm: remove unneeded symbol offsets and _TEXT_BASE)

[Benoît Thébaudeau]

Hi Albert,

On Monday, October 14, 2013 8:59:17 AM, Albert ARIBAUD wrote:
> On Sun, 13 Oct 2013 19:16:33 +0200, Albert ARIBAUD
> <albert.u.boot at aribaud.net> wrote:
> 
> > Hi Benoît,
> > 
> > On Sun, 13 Oct 2013 17:00:25 +0200 (CEST), Benoît Thébaudeau
> > <benoit.thebaudeau at advansee.com> wrote:
> > 
> > > Hi Albert,
> > > 
> > > On Sunday, October 13, 2013 9:10:28 AM, Albert ARIBAUD wrote:
> > > > Remove the last uses of symbol offsets in ARM U-Boot.
> > > > Remove some needless uses of _TEXT_BASE.
> > > > Remove all _TEXT_BASE definitions.
> > > > 
> > > > Signed-off-by: Albert ARIBAUD <albert.u.boot at aribaud.net>
> > > > ---
> > > > _TEXT_BASE was only used by ARM to allow resolution of
> > > > symbol offsets, themselves only needed due to absolute
> > > > relocations.
> > > > 
> > > > In some places, _TEXT_BASE was locally defined only
> > > > to provide a literal for CONFIG_SYS_TEXT_BASE when the
> > > > latter could have been used directly.
> > > > 
> > > > Sometimes even, _TEXT_BASE was defined but unused.
> > > > 
> > > > Since all relocations in ARM are relative, offsets,
> > > > _TEXT_BASE and CONFIG_SYS_SYM_OFFSETS can be completely
> > > > removed, and their uses can be replaced with adequate
> > > > use of compiler-generated symbols from sections.c file.
> > > 
> > > [...]
> > > 
> > > > diff --git a/arch/arm/cpu/arm1136/start.S
> > > > b/arch/arm/cpu/arm1136/start.S
> > > > index bd1e067..d15124b 100644
> > > > --- a/arch/arm/cpu/arm1136/start.S
> > > > +++ b/arch/arm/cpu/arm1136/start.S
> > > 
> > > [...]
> > > 
> > > > @@ -295,7 +269,6 @@ cpu_init_crit:
> > > >  #ifdef CONFIG_SPL_BUILD
> > > >  	.align	5
> > > >  do_hang:
> > > > -	ldr	sp, _TEXT_BASE			/* use 32 words about stack */
> > > >  	bl	hang				/* hang and never return */
> > > >  #else	/* !CONFIG_SPL_BUILD */
> > > >  	.align	5
> > > 
> > > Is this change (and the same change in the other start.S files) safe?
> > > 
> > > lib/hang.c/hang() may need a valid stack pointer because the functions
> > > that it
> > > calls may use the stack.
> > > 
> > > When the CPU lands in do_hang, it's because some exception occurred,
> > > which may
> > > follow a situation having corrupted sp. If sp is corrupted, the CPU won't
> > > be
> > > able to push the exception context onto the stack, but it might still be
> > > able to
> > > run the exception vector.
> > > 
> > > Setting sp to *_TEXT_BASE was not great, but at least this provided a few
> > > valid
> > > words of RAM for the stack.
> > 
> > Yes, there is a call to hang() which might or might not imply saving on
> > the stack depending on code generation, and sp might be incorrect
> > depending on the exception raised. I'll reintroduce the sp setting but
> > use the runtime value of _start as stack top. As you say, not great but
> > hey.
> 
> Thinking further about hangs: when we call do_hang(), by definition we
> are in a critical situation where for some reason we cannot proceed
> any more; and in the worst scenario, the only guaranteed valid register
> we have is pc. Therefore, I'm not sure it is wise to try to do anything
> else than actually hanging.
> 
> OTOH, I do understand that we may also want to do something else
> than hanging, such as trying to diagnose, but we should choose more
> clearly:
> 
> - either we hang for the purpose of being post-mortem-debugged from
>   there, and therefore, we limit alterations to the system state as
>   much as we can, by only affecting pc (to the point that 'bl hang'
>   should be turned into 'b do_hang' so that lr is also preserved);
> 
> - or we want e.g. to tell the operator about it, and we make sure we
>   have a correct setting, that is, we reserve and use a proper stack
>   rather than set it just below _start and hope for luck.
> 
> Choosing between one or the other would be done through a configuration
> option such as for instance CONFIG_SYS_LOG_HANGS.
> 
> Regarding the stack, we could use some existing exception stack, but it
> might be better if it was preserved, so that its contents could be
> looked into.
> 
> Comments?

This configuration option would be great. The verbose log should be enabled by
default, indicating the configuration option to change in order to investigate
the hang.

In the log case, we care about having a valid stack, not about preserving the
possible exception stack contents. In the post-mortem debug case, this is the
opposite.

Best regards,
Benoît