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

[Albert ARIBAUD]

Hi Benoît,

On Mon, 14 Oct 2013 12:22:31 +0200 (CEST), Benoît Thébaudeau
<benoit.thebaudeau at advansee.com> wrote:

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

I suspect some people would argue that for SPL, the default should be
to hang rather than log.

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

Depends. You may want the log to o as faithful a state dump as feasible.

> Best regards,
> Benoît


Amicalement,
-- 
Albert.