[U-Boot] [PATCH 1/3] ARM: relocate: fix hang when CONFIG_ARMV7_SECURE_BASE

[Albert ARIBAUD]

Hello Peng,

On Mon, 19 Oct 2015 13:40:51 +0800, Peng Fan <b51431 at freescale.com>
wrote:
> On Tue, Oct 06, 2015 at 05:13:24PM -0500, Frank Li wrote:
> >When added above configuration, iram fix up plus relocate offset may locate
> >in invalidate space. Write back fix up value will cause data abort.
> >
> >Add address check, skip psci code.
> >
> >Signed-off-by: Frank Li <Frank.Li at freescale.com>
> >---
> > arch/arm/lib/relocate.S | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> >diff --git a/arch/arm/lib/relocate.S b/arch/arm/lib/relocate.S
> >index 475d503..6795a1b 100644
> >--- a/arch/arm/lib/relocate.S
> >+++ b/arch/arm/lib/relocate.S
> >@@ -99,6 +99,10 @@ fixloop:
> > 	cmp	r1, #23			/* relative fixup? */
> > 	bne	fixnext
> > 
> >+	ldr	r1, =__image_copy_start
> >+	cmp	r0, r1
> >+	blo	fixnext
> >+
> 
> Hi Tom, Albert,
> 
> This is a bug fix, please consider to apply this patch.

Sorry for not spotting your patch earlier.

Took me some time to understand the commit summary. Let me see if I'm
getting this right by paraphrasing it:

	If CONFIG_ARMV7_SECURE_BASE is defined and if there is
	a fixup to the location at __image_copy_start, then U-Boot
	will try to fix that location and since it is write-protected,
	it will result in a data abort.

If I got this right, then this raises the following questions:

1) if this location cannot be written into for relocation fixup, then
how could it be written into for the copying which precedes relocation?

2) if there is a fixup to the location, it means that either this
location will not work properly without a fix, or the compiler emitted
an erroneous relocation record. In either case, just ignoring the fixup
seems like papering over the issue.

Besides, this patch will prevent image base fixups on all targets, even
those for which CONFIG_ARMV7_SECURE_BASE is not defined.

So, for now, I would NAK it.

Now, assuming the answers to the two questions above do make a valid
point that the fix above should indeed be implemented, then:

> The secure code such as PSCI is not relocated, so there is no need to fix the code
> which generate relocate entry in rel.dyn section. We should only need take
> code from __image_copy_start to __image_copy_end into consideration.

If some part of an image needs copying but not relocating, then the
right solution is not to hard-code some arbitrary location as 'not
relocatable' in the relocation routine; the right solution is to
put in place a generic mechanism to allow the linker script to define
which part of the image is relocatable. This can be done as follows:

- in the linker script, border the non-relocatable part of the image
  with two symbols, say 'relocatable_image_start' and '..._end', and
  ensure that text (code) which should *not* be relocated is mapped
  either before '..._start' or '..._end'. Not-to-be-relocated code
  would be recognized by its text section name, e.g. '.nonreloc.text*'.

- in the relocation routine, only fix up those locations which lie
  between the 'relocatable_image_start' and '..._end' symbols.

- in the relevant makefile(s), make the non-relocatable object files
  use a test section name of the for defined in the first step (e.g.
  '.nonreloc.text*').

Again, this is suggested *only* if it is shown that the data abort
cannot be avoided some other way.

> Regards,
> Peng.

Amicalement,
-- 
Albert.