[U-Boot] [PATCH V2 1/3] arm: discard relocation entry for secure section

[Peng Fan <b51431@freescale.com>]

Hello Albert,

On Tue, Oct 20, 2015 at 02:59:10PM +0200, Albert ARIBAUD wrote:
>Hello Peng,
>
>On Tue, 20 Oct 2015 15:41:30 +0800, Peng Fan <b51431@freescale.com>
>wrote:
>> Hi Albert,
>> 
>> On Tue, Oct 20, 2015 at 09:32:51AM +0200, Albert ARIBAUD wrote:
>> >Hello Peng,
>> >
>> >On Tue, 20 Oct 2015 15:20:43 +0800, Peng Fan <b51431@freescale.com>
>> >wrote:
>> >> Hi Albert,
>> >> 
>> >> On Tue, Oct 20, 2015 at 09:05:32AM +0200, Albert ARIBAUD wrote:
>> >> >Hello Peng,
>> >> >
>> >> >On Tue, 20 Oct 2015 13:59:53 +0800, Peng Fan <Peng.Fan@freescale.com>
>> >> >wrote:
>> >> >> The code such as PSCI in section named secure is bundled with
>> >> >> u-boot image, and when bootm, the code will be copied to their
>> >> >> runtime address same to compliation/linking address -
>> >> >> CONFIG_ARMV7_SECURE_BASE.
>> >> >> 
>> >> >> When compile the PSCI code and link it into the u-boot image,
>> >> >> there will be relocation entries in .rel.dyn section for PSCI.
>> >> >> Actually, we do not needs these relocation entries.
>> >> >> 
>> >> >> If still keep the relocation entries in .rel.dyn section,
>> >> >> r0 at line 103 and 106 in arch/arm/lib/relocate.S may be an invalid
>> >> >> address which may not support read/write for one SoC.
>> >> >> 102         /* relative fix: increase location by offset */
>> >> >> 103         add     r0, r0, r4
>> >> >> 104         ldr     r1, [r0]
>> >> >> 105         add     r1, r1, r4
>> >> >> 106         str     r1, [r0]
>> >> >> 
>> >> >> So discard the relocation entries for code in secure section.
>> >> >
>> >> >Actually, I'll need you to do a slight change -- that's my fault, and
>> >> >karma even ensured that my own self of two years ago would be the one
>> >> >to come and kick me. See:
>> >> >
>> >> >http://lists.denx.de/pipermail/u-boot/2013-December/168652.html
>> >> 
>> >> Ok. Then arch/arm/cpu/armv8/u-boot.lds should also have such fix,
>> >> since lots sections are discarded in u-boot.lds for armv8.
>> >
>> >You are correct, armv8 needs to be fixed, as well as zynq (and x86 but
>> >that's outside of my province). Anyway, that's a different problem
>> >which does not need to be addressed in this series.
>> >
>> >> >Which basically is about never discarding any section in the ELF file,
>> >> >and only copying a subset of the ELF sections into the binary file.
>> >> >
>> >> >So rather than discarding the secure relocation records, let's move
>> >> >them to another section as you had proposed, and thus change the line
>> >> >
>> >> >> +	/DISCARD/ : { *(.rel._secure*) }
>> >> >
>> >> >Into a
>> >> >
>> >> >	.rel._secure { *(.rel._secure*) }
>> >> >
>> >> >Placed right after the already present
>> >> >
>> >> >	.dynamic : { *(.dynamic*) }
>> >> 
>> >> It can not be placed after .dynamic, since the following is at front.
>> >> 87         .rel.dyn : {
>> >> 88                 *(.rel*)
>> >> 89         }
>> >> So relocation entry from secure text will first placed into .rel.dyn section.
>> >> 
>> >> If not DISCARD, then I prefer to put ".rel.secure : { *(.rel._secure*) }"
>> >> at line 55 which is wrapped by CONFIG_ARMV7_NONSEC in arch/arm/cpu/u-boot.lds.
>> >
>> >I prefer all 'unused' sections to be kept together near the end of the
>> >LDS file -- I'll add an explicit comment in the LDS about it.
>> >
>> >Besides, there no need to wrap such a section with a preprocessor
>> >conditional, as the linker will simply not emit an output section if
>> >there are no input sections at all for it; IOW, adding the '.rel._secure
>> >{ *(.rel._secure*) }' line will be binary-invariant for platforms which
>> >do not have PSCI or other secure code.
>> 
>> But ".rel._secure { *(.rel._secure*) }" can not be placed after
>> ".dynamic : { *(.dynamic*) }". Actually ".rel._secure { *(.rel._secure*) }"
>> should be placed before ".rel_dyn_start" in u-boot.lds, otherwise
>> the secure relocation entries will be placed into ".rel.dyn", but not
>> ".rel._secure".
>
>Hmm, you're correct, the linker will use the first pattern match, not
>the longest or best. :(
>
>But then, the secure reloc input section cannot go in line 55, because
>that's still inside the binary part of the image, which means it will
>waste space in there.
>
>So it's either use DISCARD at the very start of the SECTIONS (round
>line 17) or back to not generating relocatable code at all for PSCI.


I do not know how to generate non-relocatable code only for the secure text.
Since -mword-relocations and -pie are global flags, I do not know how to disable
mword-relocations when compiling PSCI and other secure text.

Is this ok for you?

diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
index 65986e8..fb2128a 100644
--- a/arch/arm/cpu/u-boot.lds
+++ b/arch/arm/cpu/u-boot.lds
@@ -14,6 +14,7 @@ OUTPUT_ARCH(arm)
ENTRY(_start)
SECTIONS
{
+       /DISCARD/ : { *(.rel._secure*) }
        . = 0x00000000;
   
        . = ALIGN(4);

Regards,
Peng.
>
>> Regards,
>> Peng.
>> 
>> >
>> >Amicalement,
>> >-- 
>> >Albert.
>> 
>> -- 
>
>
>
>Amicalement,
>-- 
>Albert.

--