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

[Albert ARIBAUD]

Hello Peng,

On Mon, 19 Oct 2015 15:19:09 +0800, Peng Fan <b51431 at freescale.com>
wrote:
> Hi Albert,
> On Mon, Oct 19, 2015 at 08:48:56AM +0200, Albert ARIBAUD wrote:
> >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.
> 
> Not from me -:)
> 
> >
> >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.
> 
> The commit msg needs to be refined.
> 
> >
> >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.
> 
> From my understanding, U-Boot will be relocated to high address of the DRAM space,
> exactly code from __image_copy_start to __image_copy_end.

Correct [minus the pedantic nitpick that __image_copy_start is included
in the copy while __image_copy_end is excluded from it].

> Following is part of the dump of .rel.dyn section of u-boot elf.
> SECURE BASE is 0x180000, uboot entry is 0x87800000.
> 
> Relocation section '.rel.dyn' at offset 0x577d4 contains 4032 entries:
>  Offset     Info    Type            Sym.Value  Sym. Name
>  0018410c  00000017 R_ARM_RELATIVE   
>  00184154  00000017 R_ARM_RELATIVE   
>  0018415c  00000017 R_ARM_RELATIVE   
>  00184164  00000017 R_ARM_RELATIVE   
>  0018416c  00000017 R_ARM_RELATIVE   
>  00184304  00000017 R_ARM_RELATIVE   
>  00184368  00000017 R_ARM_RELATIVE   
>  87800020  00000017 R_ARM_RELATIVE   
>  87800024  00000017 R_ARM_RELATIVE   
>  87800028  00000017 R_ARM_RELATIVE   
>  8780002c  00000017 R_ARM_RELATIVE
> 
> We can see that there are several relocate entries for secure code which is not
> copied to high address of DRAM.
> 
> However SECURE code will not be copied to high address and should not be copied.
> The code locates from __image_copy_start to __image_copy_end are copied to high
> address, after copied, we need to fix variable and function reference, means
> we need to fix this according to the relocation entry. The relocation entry
> for SECURE code is not needed and should not to be fixed, alought they are in
> the rel.dyn section.

Thanks, this makes the situation much clearer -- though not entirely.

Relocation is done by:

a) compiling and linking the image as relocatable code;

a) linking the image for loading and execution at the link-time base
   address (here 0x87800000); the image won't work at any other address
   without relocation;

b) copying the code from wherever it was loaded (which may or may not
   be the link-time base address) to its run-time base address (as high
   in DDR as possible); this makes the code unfit for execution until it
   is relocated [pedantry: ... unless the run-time base address happens
   to be the link-time base address] ;

c) fixing the image based on the relocation records, the link-time base
   address, and the run-time base address; this makes the code fit for
   execution again.

So obviously, phase c should only operate on code which was linked
between __image_copy_start [included] and __image_copy_end [excluded].

This could be an argument for checking relocation targets against those
limits... But then, only code that *requires* relocation should have
any relocation record -- IOW, there is no reason for the PSCI code to
have such records if it is not going to be relocated like the rest of
the image.

This, in turn, leads to new questions:

1. How is this PSCI code put in place? Is it bundled with the image,
   with a specificy copy routine which puts it in place then locks the
   memory range against writing? Or is it loaded in place by some other
   agent than U-Boot, and how does this agent know what to put where?

2. Does this code and the relocatable image refer to symbols of one
   another, or do they interact through an IPC independent of their
   respective location (in which case, one wonders why they are built
   into a single ELF file)?

More generally... which target do you see this problem on? Reproducing
the build myself would save both you and me some time, I guess. :)

> >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
> 
> part of an image not needs copying and not needs relocating.

This seems to imply the PSCI code is not even put in place by U-Boot.
So how does it end up in place?

> >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*'.
> 
> We have another fix is to add the following linker in u-boot.lds for arm
> +	.rel.secure :
> +	{
> +		*(.rel._secure*)
> +	}

Do you need these relocation records? If not, then just append the
'*(.rel._secure*) input section spec (with a suitable comment) to
the already existing DISCARD output section. By the way, doesn't this
linker script change alone fix the data abort?

Still, if there *are* relocation records emitted, that's because the
toolchain considered them necessary -- IOW, it was (wrongly) told the
PSCI code must be relocatable [or the code really is relocatable and we
just haven't not hit a bug about this yet].

So the real root cause fix would be to add suitable compiler and linker
flags to the PSCI object files so that they are generated as really non-
relocatable.

(still, I am wondering how this code ends up in place without U-Boot at
least copying it from the loaded image into its target location; I'll
certainly get a better understanding of it once I know which target
we're talking about.)

> Then relocation entry for secure code will not be touched.
> 
> But from my point, when doing relocation fix like the following c code:
> 
> if ((entry < __image_copy_end)  && (entry > __image_copy_start)) {
>      /* fixup according relocation entry */
> }

Testing every single relocation record target against __image_copy_start
and __image_copy_end would be rather costly in the tight loop of the
relocation routine, which should run as fast as possible.

There is at least one fix to your problem which removes the out-of-
bounds relocation records, and quite probably one better fix which
prevents generating them in the first place.

Therefore, instead of the currently proposed patch which increases the
size (very slightly) and boot time (statistically in O(n) proportion)
of the image, I prefer one of the two alternative solutions which
decrease the image size (by removing useless relocation records) and
leave the boot time unchanged (since the fix is at build time only).

> Regards,
> Peng.

Amicalement,
-- 
Albert.