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

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

[Dongsheng Wang]

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.
 	 */
 	/DISCARD/ : { *(.rel._secure*) }
+#endif
 	. = 0x00000000;
 
 	. = ALIGN(4);
-- 
2.1.0.27.g96db324

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

[Tom Rini]

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.
>  	 */
>  	/DISCARD/ : { *(.rel._secure*) }
> +#endif
>  	. = 0x00000000;
>  
>  	. = ALIGN(4);

Please update the big comment that you're now #if'ing around to explain
why the statement isn't true in some cases and hence the test, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151210/eb38529e/attachment.sig>

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

[Dongsheng Wang]

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?

Regards,
-Dongsheng

> >  	/DISCARD/ : { *(.rel._secure*) }
> > +#endif
> >  	. = 0x00000000;
> >
> >  	. = ALIGN(4);
> 
> Please update the big comment that you're now #if'ing around to explain why
> the statement isn't true in some cases and hence the test, thanks!
> 
> --
> Tom

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

[Tom Rini]

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151211/776d6274/attachment.sig>

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

[Dongsheng Wang]

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.
         */

Regards,
-Dongsheng

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

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160104/bfc7978e/attachment.sig>

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

[Peng Fan]

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

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

[Dongsheng Wang]

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@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!
> 
> --
> Tom

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

[Tom Rini]

On Mon, Jan 11, 2016 at 02:51:39AM +0000, Dongsheng Wang wrote:
> 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

code offsets

>          * only needs to be copied from loading address to

from the loading address

>          * CONFIG_ARMV7_SECURE_BASE, which is the linking and running
>          * address for secure code.
>          *
>          * If undefine CONFIG_ARMV7_SECURE_BASE secure zone will be

If CONFIG_ARMV7_SECURE_BASE is undefined, the secure zone will be

>          * included in u-boot space, and some absolute address were used

u-boot address space

>          * in secure code. Accompanied by u-boot relocation secure code
>          * also need to relocate the absolute address.

in secure code.  The absolute addresses of the secure code also needs to
be relocated along with the accompanying u-boot code.

>          *
>          * So DISCARD is only for CONFIG_ARMV7_SECURE_BASE.
>          */
>         /DISCARD/ : { *(.rel._secure*) }
> #endif

Otherwise looks good, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160114/f8ac713e/attachment.sig>

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

[Dongsheng Wang]

Hi Tom,

Thanks for your review.

I will update this patch.

Regards,
-Dongsheng

> On Mon, Jan 11, 2016 at 02:51:39AM +0000, Dongsheng Wang wrote:
> > 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
> 
> code offsets
> 
> >          * only needs to be copied from loading address to
> 
> from the loading address
> 
> >          * CONFIG_ARMV7_SECURE_BASE, which is the linking and running
> >          * address for secure code.
> >          *
> >          * If undefine CONFIG_ARMV7_SECURE_BASE secure zone will be
> 
> If CONFIG_ARMV7_SECURE_BASE is undefined, the secure zone will be
> 
> >          * included in u-boot space, and some absolute address were
> > used
> 
> u-boot address space
> 
> >          * in secure code. Accompanied by u-boot relocation secure code
> >          * also need to relocate the absolute address.
> 
> in secure code.  The absolute addresses of the secure code also needs to be
> relocated along with the accompanying u-boot code.
> 
> >          *
> >          * So DISCARD is only for CONFIG_ARMV7_SECURE_BASE.
> >          */
> >         /DISCARD/ : { *(.rel._secure*) } #endif
> 
> Otherwise looks good, thanks!
> 
> --
> Tom