ARM: Disable "DISCARD" for secure section if CONFIG_ARMV7_SECURE_BASE isn't defined

PATCH New Dongsheng Wang 3 participants 10 messages Dec 10, 2015 – Jan 18, 2016 Arm (32-bit)

lore

Share & tools

lore

Dongsheng Wang

author

2015-12-10 02:49 UTC #0 lore raw

PATCH 1/1 ARM: Disable "DISCARD" for secure section if CONFIG_ARMV7_SECURE_BASE isn't defined

From: Wang Dongsheng <[email protected]>

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 <[email protected]>

Reviewed-by Peng Fan <[email protected]>

Cc Albert Aribaud <[email protected]>

Cc Tom Warren <[email protected]>

Cc York Sun <[email protected]>

Cc Hans De Goede <[email protected]>

Cc Ian Campbell <[email protected]>

Cc Tom Rini <[email protected]>

Cc Jan Kiszka <[email protected]>

Cc Stefano Babic <[email protected]>

arch/arm/cpu/u-boot.lds +2 -0

index d48a905..413d459 100644
@@ -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);

Signature · 1 line

2.1.0.27.g96db324

Tom Rini

2015-12-10 12:25 UTC #1 lore raw

PATCH 1/1 ARM: Disable "DISCARD" for secure section if CONFIG_ARMV7_SECURE_BASE isn't defined

Quoted 66 lines · Dongsheng Wang

From: Wang Dongsheng <[email protected]>
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 <[email protected]>
Reviewed-by Peng Fan <[email protected]>
Cc Albert Aribaud <[email protected]>
Cc Tom Warren <[email protected]>
Cc York Sun <[email protected]>
Cc Hans De Goede <[email protected]>
Cc Ian Campbell <[email protected]>
Cc Tom Rini <[email protected]>
Cc Jan Kiszka <[email protected]>
Cc Stefano Babic <[email protected]>
arch/arm/cpu/u-boot.lds +1 -0
index d48a905..413d459 100644
@@ -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

arch/arm/cpu/u-boot.lds SECTIONS · line 38

Quoted 7 lines

arch/arm/cpu/u-boot.lds +1 -0

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

Signature · 8 lines

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>

Dongsheng Wang

author

2015-12-11 10:15 UTC #2 lore raw

PATCH 1/1 ARM: Disable "DISCARD" for secure section if CONFIG_ARMV7_SECURE_BASE isn't defined

Hi Tom,

Thanks for your review.

Quoted 55 lines · Dongsheng Wang

Quoted 54 lines
From: Wang Dongsheng <[email protected]>
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 <[email protected]>
Reviewed-by Peng Fan <[email protected]>
Cc Albert Aribaud <[email protected]>
Cc Tom Warren <[email protected]>
Cc York Sun <[email protected]>
Cc Hans De Goede <[email protected]>
Cc Ian Campbell <[email protected]>
Cc Tom Rini <[email protected]>
Cc Jan Kiszka <[email protected]>
Cc Stefano Babic <[email protected]>

Quoted 16 lines

arch/arm/cpu/u-boot.lds OUTPUT_ARCH(arm) · line 17
Quoted 9 lines
arch/arm/cpu/u-boot.lds index renamed from arch/arm/cpu/u-boot.lds +0 -0
d48a905..413d459 100644
arch/arm/cpu/u-boot.lds +1 -0
@@ -14,6 +14,7 @@ OUTPUT_ARCH(arm)
 ENTRY(_start)
 SECTIONS
 {
+#if defined(CONFIG_ARMV7_SECURE_BASE) &&
defined(CONFIG_ARMV7_NONSEC)
Quoted 6 lines
arch/arm/cpu/u-boot.lds +0 -1
 	/*
 	 * 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

Quoted 11 lines

arch/arm/cpu/u-boot.lds SECTIONS · line 38
Quoted 5 lines
arch/arm/cpu/u-boot.lds +1 -0
 	/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

Tom Rini

2015-12-11 12:29 UTC #3 lore raw

PATCH 1/1 ARM: Disable "DISCARD" for secure section if CONFIG_ARMV7_SECURE_BASE isn't defined

Quoted 69 lines · Dongsheng Wang

Hi Tom,
Thanks for your review.
Quoted 65 lines · Dongsheng Wang
Quoted 59 lines
From: Wang Dongsheng <[email protected]>
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 <[email protected]>
Reviewed-by Peng Fan <[email protected]>
Cc Albert Aribaud <[email protected]>
Cc Tom Warren <[email protected]>
Cc York Sun <[email protected]>
Cc Hans De Goede <[email protected]>
Cc Ian Campbell <[email protected]>
Cc Tom Rini <[email protected]>
Cc Jan Kiszka <[email protected]>
Cc Stefano Babic <[email protected]>
arch/arm/cpu/u-boot.lds index renamed from arch/arm/cpu/u-boot.lds +0 -0
d48a905..413d459 100644
arch/arm/cpu/u-boot.lds OUTPUT_ARCH(arm) · line 17
Quoted 4 lines
arch/arm/cpu/u-boot.lds +1 -0
@@ -14,6 +14,7 @@ OUTPUT_ARCH(arm)
 ENTRY(_start)
 SECTIONS
 {
+#if defined(CONFIG_ARMV7_SECURE_BASE) &&
defined(CONFIG_ARMV7_NONSEC)

Quoted 8 lines

Quoted 6 lines
arch/arm/cpu/u-boot.lds OUTPUT_ARCH(arm)
Quoted 6 lines
arch/arm/cpu/u-boot.lds +0 -1
 	/*
 	 * 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.

Signature · 8 lines

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>

Dongsheng Wang

author

2015-12-11 15:30 UTC #4 lore raw

PATCH 1/1 ARM: Disable "DISCARD" for secure section if CONFIG_ARMV7_SECURE_BASE isn't defined

Hi Tom,

Quoted 79 lines · Dongsheng Wang

Quoted 78 lines
Hi Tom,
Thanks for your review.
Quoted 73 lines · Dongsheng Wang
Quoted 60 lines
From: Wang Dongsheng <[email protected]>
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 <[email protected]>
Reviewed-by Peng Fan <[email protected]>
Cc Albert Aribaud <[email protected]>
Cc Tom Warren <[email protected]>
Cc York Sun <[email protected]>
Cc Hans De Goede <[email protected]>
Cc Ian Campbell <[email protected]>
Cc Tom Rini <[email protected]>
Cc Jan Kiszka <[email protected]>
Cc Stefano Babic <[email protected]>
arch/arm/cpu/u-boot.lds +0 -0
index
d48a905..413d459 100644
arch/arm/cpu/u-boot.lds OUTPUT_ARCH(arm) · line 17
Quoted 4 lines
arch/arm/cpu/u-boot.lds +1 -0
@@ -14,6 +14,7 @@ OUTPUT_ARCH(arm)
 ENTRY(_start)
 SECTIONS
 {
+#if defined(CONFIG_ARMV7_SECURE_BASE) &&
defined(CONFIG_ARMV7_NONSEC)
Quoted 7 lines
arch/arm/cpu/u-boot.lds +0 -0
 	/*
 	 * Discard the relocation entries for secure text.
 	 * The secure code is bundled with u-boot image, so there will
@@
-31,6 +32,7 @@ SECTIONS
arch/arm/cpu/u-boot.lds +0 -0
 	 * avoid hole in the final image.
 	 */

Quoted 4 lines

Quoted 1 line
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

Tom Rini

2016-01-04 15:50 UTC #5 lore raw

PATCH 1/1 ARM: Disable "DISCARD" for secure section if CONFIG_ARMV7_SECURE_BASE isn't defined

Quoted 117 lines · Dongsheng Wang

Hi Tom,
Quoted 79 lines · Dongsheng Wang
Quoted 78 lines
Hi Tom,
Thanks for your review.
Quoted 73 lines · Dongsheng Wang
Quoted 60 lines
From: Wang Dongsheng <[email protected]>
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 <[email protected]>
Reviewed-by Peng Fan <[email protected]>
Cc Albert Aribaud <[email protected]>
Cc Tom Warren <[email protected]>
Cc York Sun <[email protected]>
Cc Hans De Goede <[email protected]>
Cc Ian Campbell <[email protected]>
Cc Tom Rini <[email protected]>
Cc Jan Kiszka <[email protected]>
Cc Stefano Babic <[email protected]>
arch/arm/cpu/u-boot.lds +0 -0
index
d48a905..413d459 100644
arch/arm/cpu/u-boot.lds OUTPUT_ARCH(arm) · line 17
Quoted 4 lines
arch/arm/cpu/u-boot.lds +1 -0
@@ -14,6 +14,7 @@ OUTPUT_ARCH(arm)
 ENTRY(_start)
 SECTIONS
 {
+#if defined(CONFIG_ARMV7_SECURE_BASE) &&
defined(CONFIG_ARMV7_NONSEC)
Quoted 7 lines
arch/arm/cpu/u-boot.lds +0 -0
 	/*
 	 * Discard the relocation entries for secure text.
 	 * The secure code is bundled with u-boot image, so there will
@@
-31,6 +32,7 @@ SECTIONS
arch/arm/cpu/u-boot.lds +0 -0
 	 * avoid hole in the final image.
 	 */
Quoted 4 lines
Quoted 1 line
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.

Quoted 8 lines

* 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!

Signature · 8 lines

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>

Peng Fan

2016-01-05 01:07 UTC #6 lore raw

PATCH 1/1 ARM: Disable "DISCARD" for secure section if CONFIG_ARMV7_SECURE_BASE isn't defined

Hi Tom,

Quoted 127 lines · Tom Rini

Quoted 117 lines · Dongsheng Wang
Hi Tom,
Quoted 79 lines · Dongsheng Wang
Quoted 78 lines
Hi Tom,
Thanks for your review.
Quoted 73 lines · Dongsheng Wang
Quoted 60 lines
From: Wang Dongsheng <[email protected]>
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 <[email protected]>
Reviewed-by Peng Fan <[email protected]>
Cc Albert Aribaud <[email protected]>
Cc Tom Warren <[email protected]>
Cc York Sun <[email protected]>
Cc Hans De Goede <[email protected]>
Cc Ian Campbell <[email protected]>
Cc Tom Rini <[email protected]>
Cc Jan Kiszka <[email protected]>
Cc Stefano Babic <[email protected]>
arch/arm/cpu/u-boot.lds +0 -0
index
d48a905..413d459 100644
arch/arm/cpu/u-boot.lds OUTPUT_ARCH(arm) · line 17
Quoted 4 lines
arch/arm/cpu/u-boot.lds +1 -0
@@ -14,6 +14,7 @@ OUTPUT_ARCH(arm)
 ENTRY(_start)
 SECTIONS
 {
+#if defined(CONFIG_ARMV7_SECURE_BASE) &&
defined(CONFIG_ARMV7_NONSEC)
Quoted 7 lines
arch/arm/cpu/u-boot.lds +0 -0
 	/*
 	 * Discard the relocation entries for secure text.
 	 * The secure code is bundled with u-boot image, so there will
@@
-31,6 +32,7 @@ SECTIONS
arch/arm/cpu/u-boot.lds +0 -0
 	 * avoid hole in the final image.
 	 */
Quoted 4 lines
Quoted 1 line
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.
Quoted 8 lines
* 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.
*/

Quoted 5 lines

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.

Quoted 10 lines

Signature · 9 lines

-- 
Tom



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

Dongsheng Wang

author

2016-01-11 02:51 UTC #7 lore raw

PATCH 1/1 ARM: Disable "DISCARD" for secure section if CONFIG_ARMV7_SECURE_BASE isn't defined

In reply to Tom Rini

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

Quoted 139 lines · Dongsheng Wang

Quoted 14 lines
Hi Tom,
Quoted 12 lines · Dongsheng Wang
Quoted 11 lines
Hi Tom,
Thanks for your review.
Quoted 7 lines · Dongsheng Wang
Quoted 6 lines
From: Wang Dongsheng <[email protected]>
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.
Quoted 23 lines
Quoted 23 lines
Quoted 23 lines
Quoted 23 lines
Quoted 23 lines
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
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
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.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
2. Enable "DISCARD" only for CONFIG_ARMV7_SECURE_BASE has
defined.
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
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.
Quoted 67 lines
Quoted 31 lines
Quoted 31 lines
Quoted 30 lines
Quoted 18 lines
Signed-off-by Wang Dongsheng <[email protected]>
Reviewed-by Peng Fan <[email protected]>
Cc Albert Aribaud <[email protected]>
Cc Tom Warren <[email protected]>
Cc York Sun <[email protected]>
Cc Hans De Goede <[email protected]>
Cc Ian Campbell <[email protected]>
Cc Tom Rini <[email protected]>
Cc Jan Kiszka <[email protected]>
Cc Stefano Babic <[email protected]>
arch/arm/cpu/u-boot.lds +0 -0
index
d48a905..413d459 100644
arch/arm/cpu/u-boot.lds OUTPUT_ARCH(arm) · line 17
Quoted 4 lines
arch/arm/cpu/u-boot.lds +1 -0
@@ -14,6 +14,7 @@ OUTPUT_ARCH(arm)
 ENTRY(_start)
 SECTIONS
 {
+#if defined(CONFIG_ARMV7_SECURE_BASE) &&
defined(CONFIG_ARMV7_NONSEC)
Quoted 7 lines
arch/arm/cpu/u-boot.lds +0 -0
 	/*
 	 * Discard the relocation entries for secure text.
 	 * The secure code is bundled with u-boot image, so there
will @@
-31,6 +32,7 @@ SECTIONS
arch/arm/cpu/u-boot.lds +0 -0
 	 * avoid hole in the final image.
 	 */
Quoted 4 lines
Quoted 1 line
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.
Quoted 8 lines
* 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

Tom Rini

2016-01-14 17:56 UTC #8 lore raw

PATCH 1/1 ARM: Disable "DISCARD" for secure section if CONFIG_ARMV7_SECURE_BASE isn't defined

Quoted 11 lines · 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

code offsets

Quoted 1 line

* only needs to be copied from loading address to

from the loading address

Quoted 4 lines

* 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

Quoted 1 line

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

u-boot address space

Quoted 2 lines

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

Quoted 5 lines

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

#endif

Otherwise looks good, thanks!

Signature · 8 lines

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>

Dongsheng Wang

author

2016-01-18 03:02 UTC #9 lore raw

PATCH 1/1 ARM: Disable "DISCARD" for secure section if CONFIG_ARMV7_SECURE_BASE isn't defined

Hi Tom,

Thanks for your review.

I will update this patch.

Regards,
-Dongsheng

Quoted 48 lines · Dongsheng Wang

Quoted 5 lines
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?
Quoted 3 lines
If my understanding is wrong, please correct me, thanks:
#if defined(CONFIG_ARMV7_SECURE_BASE) &&
defined(CONFIG_ARMV7_NONSEC)
Quoted 3 lines
/*
* If CONFIG_ARMV7_SECURE_BASE is true, secure code will not
* bundle with u-boot, and codes offset are fixed. Secure zone
code offsets
Quoted 1 line
* only needs to be copied from loading address to
from the loading address
Quoted 4 lines
* 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
Quoted 2 lines
* included in u-boot space, and some absolute address were
used
u-boot address space
Quoted 2 lines
* 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.
Quoted 4 lines
         *
         * So DISCARD is only for CONFIG_ARMV7_SECURE_BASE.
         */
        /DISCARD/ : { *(.rel._secure*) } #endif
Otherwise looks good, thanks!
--
Tom