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

[Albert ARIBAUD]

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?

Amicalement,
-- 
Albert.