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@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@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:
- if this location cannot be written into for relocation fixup, then
how could it be written into for the copying which precedes relocation?
- 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.
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.
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.
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*) + }
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 */ }
Regards, Peng.
- 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.
--