[U-Boot] [PATCH] ARM: Disable "DISCARD" for secure section if CONFIG_ARMV7_SECURE_BASE isn't defined
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);
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:
- 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.
- 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!
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:
- 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.
- 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
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:
- 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.
- 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.
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:
- 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.
- 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
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:
- 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.
- 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!
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:
- 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.
- 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@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
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:
- 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.
- 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
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!
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 zonecode offsets
* only needs to be copied from loading address tofrom 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 beIf CONFIG_ARMV7_SECURE_BASE is undefined, the secure zone will be
* included in u-boot space, and some absolute address wereused
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*) } #endifOtherwise looks good, thanks!
-- Tom
participants (3)
-
Dongsheng Wang -
Peng Fan -
Tom Rini