[U-Boot] [PATCH] ARM: Disable "DISCARD" for secure section if CONFIG_ARMV7_SECURE_BASE isn't defined

[Peng Fan <van.freenix@gmail.com>]

Hi Tom,
On Mon, Jan 04, 2016 at 10:50:16AM -0500, Tom Rini wrote:
>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@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@nxp.com>
>> > > > > Reviewed-by: Peng Fan <Peng.Fan@nxp.com>
>> > > > > Cc: Albert Aribaud <albert.u.boot@aribaud.net>
>> > > > > Cc: Tom Warren <twarren@nvidia.com>
>> > > > > Cc: York Sun <yorksun@freescale.com>
>> > > > > Cc: Hans De Goede <hdegoede@redhat.com>
>> > > > > Cc: Ian Campbell <ijc@hellion.org.uk>
>> > > > > Cc: Tom Rini <trini@konsulko.com>
>> > > > > Cc: Jan Kiszka <jan.kiszka@siemens.com>
>> > > > > Cc: Stefano Babic <sbabic@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!

This patch d47cb0b61aa9e268f140455b2bc4421ae9e0b4bc has a assumption that
CONFIG_ARMV7_SECURE_BASE is defined and secure code does not need to be
relocated. But this is not always true. To those which not define
CONFIG_ARMV7_SECURE_BASE, see psci_update_dt, the space will be marked as
reserved. So DISCARD is only needed when CONFIG_ARMV7_SECURE_BASE and
CONFIG_ARMV7_NONSEC is defined.

Regards,
Peng.

>
>-- 
>Tom



>_______________________________________________
>U-Boot mailing list
>U-Boot at lists.denx.de
>http://lists.denx.de/mailman/listinfo/u-boot