| Message ID | 20151020072039.GA26880@shlinux2 |
|---|---|
| State | Superseded |
| Delegated to: | Albert ARIBAUD |
| Headers | show |
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. Amicalement,
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". Regards, Peng. > >Amicalement, >-- >Albert.
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. > Regards, > Peng. > > > > >Amicalement, > >-- > >Albert. > > -- Amicalement,
diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds index 03cd9f6..65986e8 100644 --- a/arch/arm/cpu/u-boot.lds +++ b/arch/arm/cpu/u-boot.lds @@ -51,6 +51,8 @@ SECTIONS *(.__secure_end) LONG(0x1d1071c); /* Must output something to reset LMA */ } + + .rel.secure : { *(.rel._secure*) } #endif . = ALIGN(4);