[U-Boot] [PATCH] ARM: Disable "DISCARD" for secure section if CONFIG_ARMV7_SECURE_BASE isn't defined
Dongsheng Wang
dongsheng.wang at nxp.com
Mon Jan 11 03:51:39 CET 2016
Hi Tom,
Sorry for my late reply, and thanks for your reply.
How about the following comments, following your suggestion I remove some redundant comments?
If my understanding is wrong, please correct me, thanks:
#if defined(CONFIG_ARMV7_SECURE_BASE) && defined(CONFIG_ARMV7_NONSEC)
/*
* If CONFIG_ARMV7_SECURE_BASE is true, secure code will not
* bundle with u-boot, and codes offset are fixed. Secure zone
* only needs to be copied from loading address to
* CONFIG_ARMV7_SECURE_BASE, which is the linking and running
* address for secure code.
*
* If undefine CONFIG_ARMV7_SECURE_BASE secure zone will be
* included in u-boot space, and some absolute address were used
* in secure code. Accompanied by u-boot relocation secure code
* also need to relocate the absolute address.
*
* So DISCARD is only for CONFIG_ARMV7_SECURE_BASE.
*/
/DISCARD/ : { *(.rel._secure*) }
#endif
Regards,
-Dongsheng
>
> On Fri, Dec 11, 2015 at 03:30:24PM +0000, Dongsheng Wang wrote:
> > Hi Tom,
> >
> > > On Fri, Dec 11, 2015 at 10:15:03AM +0000, Dongsheng Wang wrote:
> > > > Hi Tom,
> > > >
> > > > Thanks for your review.
> > > >
> > > > > On Thu, Dec 10, 2015 at 10:49:01AM +0800, Dongsheng Wang wrote:
> > > > >
> > > > > > From: Wang Dongsheng <dongsheng.wang at nxp.com>
> > > > > >
> > > > > > Fix PSCI hang up without CONFIG_ARMV7_SECURE_BASE define.
> > > > > > "DISCARD" will remove ._secure.text relocate, but PSCI
> > > > > > framework has already used some absolute address those need to
> relocate.
> > > > > >
> > > > > > Use readelf -t -r u-boot show us:
> > > > > > .__secure_start addr: 601408e4
> > > > > > .__secure_end addr: 60141460
> > > > > >
> > > > > > 60141140 00000017 R_ARM_RELATIVE
> > > > > > 46 _secure_monitor:
> > > > > > 47 #ifdef CONFIG_ARMV7_PSCI
> > > > > > 48 ldr r5, =_psci_vectors
> > > > > >
> > > > > > 60141194 00000017 R_ARM_RELATIVE 6014119c 00000017
> > > > > > R_ARM_RELATIVE
> > > > > > 601411a4 00000017 R_ARM_RELATIVE 601411ac 00000017
> > > > > > R_ARM_RELATIVE
> > > > > > 64 _psci_table:
> > > > > > 66 .word psci_cpu_suspend
> > > > > > ...
> > > > > > 72 .word psci_migrate
> > > > > >
> > > > > > 60141344 00000017 R_ARM_RELATIVE 6014145c 00000017
> > > > > > R_ARM_RELATIVE
> > > > > > 202 ldr r5, =psci_text_end
> > > > > >
> > > > > > Solutions:
> > > > > > 1. Change absolute address to RelAdr.
> > > > > > Based on LDR (immediate, ARM), we only have 4K offset to jump.
> > > > > > Now PSCI code size is close to 4K size that is LDR limit jump
> > > > > > size, so even if the LDR is based on the current instruction
> > > > > > address, there is also have a risk for RelAdr. If we use two
> > > > > > jump steps I think we can fix this issue, but looks too hack, so give up
> this way.
> > > > > >
> > > > > > 2. Enable "DISCARD" only for CONFIG_ARMV7_SECURE_BASE has
> defined.
> > > > > > If CONFIG_ARMV7_SECURE_BASE is defined in platform, all of
> > > > > > secure will in the BASE address that is absolute.
> > > > > > psci_update_dt will relocate the PSCI code into link stage
> > > > > > address(CONFIG_ARMV7_SECURE_BASE address), so using this
> method.
> > > > > >
> > > > > > Signed-off-by: Wang Dongsheng <dongsheng.wang at nxp.com>
> > > > > > Reviewed-by: Peng Fan <Peng.Fan at nxp.com>
> > > > > > Cc: Albert Aribaud <albert.u.boot at aribaud.net>
> > > > > > Cc: Tom Warren <twarren at nvidia.com>
> > > > > > Cc: York Sun <yorksun at freescale.com>
> > > > > > Cc: Hans De Goede <hdegoede at redhat.com>
> > > > > > Cc: Ian Campbell <ijc at hellion.org.uk>
> > > > > > Cc: Tom Rini <trini at konsulko.com>
> > > > > > Cc: Jan Kiszka <jan.kiszka at siemens.com>
> > > > > > Cc: Stefano Babic <sbabic at denx.de>
> > > > > >
> > > > > > diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> > > > > > index
> > > > > > d48a905..413d459 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
> > > > > > {
> > > > > > +#if defined(CONFIG_ARMV7_SECURE_BASE) &&
> > > > > defined(CONFIG_ARMV7_NONSEC)
> > > > > > /*
> > > > > > * Discard the relocation entries for secure text.
> > > > > > * The secure code is bundled with u-boot image, so there
> > > > > > will @@
> > > > > > -31,6 +32,7 @@ SECTIONS
> > > > > > * avoid hole in the final image.
> > > > > > */
> > > >
> > > > Update this comment, not my patch's comment, right?
> > >
> > > Correct.
> > >
> > Not sure we hope a detailed comment or concise comment.
> > Could you review my comment?
> >
> > If my understanding is wrong, please correct me, thanks:
> > /*
> > * Based on the /DISCARD/ introduce by ARMv7 patch. And ARMv8 not
> > * for sure has the same issue. Based on conservative this is only for
> > * ARMv7, another point the /DISCARD/ may isn't necessary in platform.
> > * Please see below investigation:
> > *
> > * If undefine CONFIG_ARMV7_SECURE_BASE secure zone will be
> > * included in u-boot space, and some absolute address were used
> > * in secure code. Accompanied by u-boot relocation secure code
> > * also need to relocate the absolute address.
> > *
> > * If CONFIG_ARMV7_SECURE_BASE is true, secure code will not
> > * bundle with u-boot, and codes offset are fixed. Secure zone
> > * only needs to be copied from loading address to
> > * CONFIG_ARMV7_SECURE_BASE, which is the linking and running
> > * address for secure code.
> > *
> > * About below may depend on toolchain:
> > * 1. If keep the relocation entries in .rel.dyn section,
> > * "relocation offset + linking address" may locates into an
> > * address that is reserved by SoC, then will trigger data abort.
> > * The reason that move .rel._secure at the beginning, is to
> > * avoid hole in the final image.
> > *
> > * 2. .rel.dyn will not include secure code, becuase
> > * CONFIG_ARMV7_SECURE_BASE give us an real absolute address, all
> > * of codes offset has fixed on build and link stage, and the same
> > * to runtime address.
> > * e.g:
> > * NXP Layerscape platform, gcc version:
> > * crosstool-NG linaro-1.13.1-4.8-2013.12 - Linaro GCC 2013.11
> > * The secure code will not include in .rel.dyn. So /DISCARD/ is redundant.
> > *
> > * Considering the compatibility, so keep DISCARD for
> > * CONFIG_ARMV7_SECURE_BASE.
> > */
>
> Not exactly. We shouldn't need to mention other patches here (but you can
> reference git commit hashes (git rev-parse --short HASH) for additional clarity.
> Peng, can you please also comment on the new code comment here as this is
> from your patch initially? Thanks and sorry for the delay!
>
> --
> Tom
More information about the U-Boot
mailing list